[AST] Bring 'mutating' and 'inout self' in sync. (#10375)

- A mutating method or accessor always has 'inout self'.
- A nonmutating method or accessor never has 'inout self'.
- Only instance members can be mutating.
- Addressors are still addressors even when on static members.

Came up after reviewing another patch that confused the two as
possibly distinct concepts.

Author: jrose-apple

lib/AST/ASTVerifier.cpp

@@ -2690,6 +2690,28 @@ class Verifier : public ASTWalker {
         }
       }
 
+      if (FD->isMutating()) {
+        if (!FD->isInstanceMember()) {
+          Out << "mutating function is not an instance member\n";
+          abort();
+        }
+        if (FD->getDeclContext()->getAsClassOrClassExtensionContext()) {
+          Out << "mutating function in a class\n";
+          abort();
+        }
+        const ParamDecl *selfParam = FD->getImplicitSelfDecl();
+        if (!selfParam->getInterfaceType()->is<InOutType>()) {
+          Out << "mutating function does not have inout 'self'\n";
+          abort();
+        }
+      } else {
+        const ParamDecl *selfParam = FD->getImplicitSelfDecl();
+        if (selfParam && selfParam->getInterfaceType()->is<InOutType>()) {
+          Out << "non-mutating function has inout 'self'\n";
+          abort();
+        }
+      }
+
       verifyCheckedBase(FD);
     }
 

lib/AST/Decl.cpp

@@ -3398,7 +3398,9 @@ bool AbstractStorageDecl::isSetterNonMutating() const {
   switch (getStorageKind()) {
   case AbstractStorageDecl::Stored:
   case AbstractStorageDecl::StoredWithTrivialAccessors:
-    return false;
+    // Instance member setters are mutating; static property setters and
+    // top-level setters are not.
+    return !isInstanceMember();
 
   case AbstractStorageDecl::StoredWithObservers:
   case AbstractStorageDecl::InheritedWithObservers:

lib/Parse/ParseDecl.cpp

@@ -3445,29 +3445,28 @@ static FuncDecl *createAccessorFunc(SourceLoc DeclLoc, ParameterList *param,
   // Non-static set/willSet/didSet/materializeForSet/mutableAddress
   // default to mutating.  get/address default to
   // non-mutating.
-  if (!D->isStatic()) {
-    switch (Kind) {
-    case AccessorKind::IsAddressor:
-      D->setAddressorKind(addressorKind);
-      break;
+  switch (Kind) {
+  case AccessorKind::IsAddressor:
+    D->setAddressorKind(addressorKind);
+    break;
 
-    case AccessorKind::IsGetter:
-      break;
+  case AccessorKind::IsGetter:
+    break;
 
-    case AccessorKind::IsMutableAddressor:
-      D->setAddressorKind(addressorKind);
-      LLVM_FALLTHROUGH;
+  case AccessorKind::IsMutableAddressor:
+    D->setAddressorKind(addressorKind);
+    LLVM_FALLTHROUGH;
 
-    case AccessorKind::IsSetter:
-    case AccessorKind::IsWillSet:
-    case AccessorKind::IsDidSet:
+  case AccessorKind::IsSetter:
+  case AccessorKind::IsWillSet:
+  case AccessorKind::IsDidSet:
+    if (D->isInstanceMember())
       D->setMutating();
-      break;
+    break;
 
-    case AccessorKind::IsMaterializeForSet:
-    case AccessorKind::NotAccessor:
-      llvm_unreachable("not parseable accessors");
-    }
+  case AccessorKind::IsMaterializeForSet:
+  case AccessorKind::NotAccessor:
+    llvm_unreachable("not parseable accessors");
   }
 
   return D;

lib/Sema/CodeSynthesis.cpp

@@ -352,18 +352,15 @@ static FuncDecl *createMaterializeForSetPrototype(AbstractStorageDecl *storage,
       params, TypeLoc::withoutLoc(retTy), DC);
   materializeForSet->setImplicit();
 
-  bool isStatic = storage->isStatic();
-
   // Open-code the setMutating() calculation since we might run before
-  // the setter has been type checked. Also as a hack, always mark the
-  // setter mutating if we're inside a protocol, because it seems some
-  // things break otherwise -- the root cause should be fixed eventually.
+  // the setter has been type checked.
+  Type contextTy = DC->getDeclaredInterfaceType();
   materializeForSet->setMutating(
-      storage->getDeclContext()->getAsProtocolOrProtocolExtensionContext() ||
-      (!setter->getAttrs().hasAttribute<NonMutatingAttr>() &&
-       !storage->isSetterNonMutating()));
+      contextTy && !contextTy->hasReferenceSemantics() &&
+      !setter->getAttrs().hasAttribute<NonMutatingAttr>() &&
+      !storage->isSetterNonMutating());
 
-  materializeForSet->setStatic(isStatic);
+  materializeForSet->setStatic(storage->isStatic());
 
   // materializeForSet is final if the storage is.
   if (storage->isFinal())

lib/Sema/TypeCheckDecl.cpp

@@ -5038,6 +5038,12 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     else if (FD->getAttrs().hasAttribute<NonMutatingAttr>())
       FD->setMutating(false);
 
+    if (FD->isMutating()) {
+      Type contextTy = FD->getDeclContext()->getDeclaredInterfaceType();
+      if (contextTy->hasReferenceSemantics())
+        FD->setMutating(false);
+    }
+
     // Check whether the return type is dynamic 'Self'.
     if (checkDynamicSelfReturn(FD))
       FD->setInvalid();

test/SILGen/addressors.swift

@@ -22,6 +22,13 @@ struct A {
       return base
     }
   }
+
+  static var staticProp: Int32 {
+    unsafeAddress {
+      // Just don't trip up the verifier.
+      fatalError()
+    }
+  }
 }
 
 // CHECK-LABEL: sil hidden @_T010addressors1AV9subscripts5Int32VAFcflu : $@convention(method) (Int32, A) -> UnsafePointer<Int32>

test/SILGen/guaranteed_self.swift

@@ -188,12 +188,16 @@ struct S: Fooable {
 // CHECK-NOT:     destroy_addr [[SELF_ADDR]]
 
 // Witness thunk for prop3 nonmutating materializeForSet
-// CHECK-LABEL: sil private [transparent] [thunk] @_T015guaranteed_self1SVAA7FooableA2aDP5prop3SifmTW : $@convention(witness_method) (Builtin.RawPointer, @inout Builtin.UnsafeValueBuffer, @inout S) -> (Builtin.RawPointer, Optional<Builtin.RawPointer>)
+// CHECK-LABEL: sil private [transparent] [thunk] @_T015guaranteed_self1SVAA7FooableA2aDP5prop3SifmTW : $@convention(witness_method) (Builtin.RawPointer, @inout Builtin.UnsafeValueBuffer, @in_guaranteed S) -> (Builtin.RawPointer, Optional<Builtin.RawPointer>)
 // CHECK:       bb0({{.*}} [[SELF_ADDR:%.*]] : $*S):
-// CHECK:         [[SELF:%.*]] = load [copy] [[SELF_ADDR]]
+// CHECK:         [[SELF_COPY:%.*]] = alloc_stack $S
+// CHECK:         copy_addr [[SELF_ADDR]] to [initialization] [[SELF_COPY]]
+// CHECK:         [[SELF:%.*]] = load [take] [[SELF_COPY]]
 // CHECK:         destroy_value [[SELF]]
 // CHECK-NOT:     destroy_value [[SELF]]
-// CHECK:       }
+// CHECK-NOT:     destroy_addr [[SELF_COPY]]
+// CHECK-NOT:     destroy_addr [[SELF_ADDR]]
+// CHECK:       } // end sil function '_T015guaranteed_self1SVAA7FooableA2aDP5prop3SifmTW'
 
 //
 // TODO: Expected output for the other cases