Merge pull request swiftlang#34092 from slavapestov/didset-access-semantics-cycle

Sema: Simplify adjustSelfTypeForMember() a little bit to avoid a cycle

Author: slavapestov

lib/AST/ASTVerifier.cpp

@@ -1828,30 +1828,30 @@ class Verifier : public ASTWalker {
         Out << "\n";
         abort();
       }
-      
+
+      if (!isa<VarDecl>(E->getMember().getDecl())) {
+        Out << "Member reference to a non-VarDecl\n";
+        E->dump(Out);
+        Out << "\n";
+        abort();
+      }
+
+      auto baseType = E->getBase()->getType();
+      if (baseType->is<InOutType>()) {
+        Out << "Member reference to an inout type\n";
+        E->dump(Out);
+        Out << "\n";
+        abort();
+      }
+
       // The base of a member reference cannot be an existential type.
-      if (E->getBase()->getType()->getWithoutSpecifierType()
-            ->isExistentialType()) {
+      if (baseType->getWithoutSpecifierType()->isExistentialType()) {
         Out << "Member reference into an unopened existential type\n";
         E->dump(Out);
         Out << "\n";
         abort();
       }
 
-      // The only time the base is allowed to be inout is if we are accessing
-      // a computed property or if the base is a protocol or existential.
-      if (auto *baseIOT = E->getBase()->getType()->getAs<InOutType>()) {
-        if (!baseIOT->getObjectType()->is<ArchetypeType>()) {
-          auto *VD = dyn_cast<VarDecl>(E->getMember().getDecl());
-          if (!VD || !VD->requiresOpaqueAccessors()) {
-            Out << "member_ref_expr on value of inout type\n";
-            E->dump(Out);
-            Out << "\n";
-            abort();
-          }
-        }
-      }
-      
       // FIXME: Check container/member types through substitutions.
 
       verifyCheckedBase(E);

lib/Sema/CSApply.cpp

@@ -1175,8 +1175,8 @@ namespace {
           if (cs.getType(base)->is<LValueType>())
             selfParamTy = InOutType::get(selfTy);
 
-        base = coerceObjectArgumentToType(
-                 base, selfParamTy, member, semantics,
+        base = coerceSelfArgumentToType(
+                 base, selfParamTy, member,
                  locator.withPathElement(ConstraintLocator::MemberRefBase));
       } else {
         if (!isExistentialMetatype || openedExistential) {
@@ -1589,7 +1589,7 @@ namespace {
                         ArrayRef<Identifier> argLabels,
                         ConstraintLocatorBuilder locator);
 
-    /// Coerce the given object argument (e.g., for the base of a
+    /// Coerce the given 'self' argument (e.g., for the base of a
     /// member expression) to the given type.
     ///
     /// \param expr The expression to coerce.
@@ -1598,13 +1598,10 @@ namespace {
     ///
     /// \param member The member being accessed.
     ///
-    /// \param semantics The kind of access we've been asked to perform.
-    ///
     /// \param locator Locator used to describe where in this expression we are.
-    Expr *coerceObjectArgumentToType(Expr *expr,
-                                     Type baseTy, ValueDecl *member,
-                                     AccessSemantics semantics,
-                                     ConstraintLocatorBuilder locator);
+    Expr *coerceSelfArgumentToType(Expr *expr,
+                                   Type baseTy, ValueDecl *member,
+                                   ConstraintLocatorBuilder locator);
 
   private:
     /// Build a new subscript.
@@ -1808,8 +1805,7 @@ namespace {
       // Handle dynamic lookup.
       if (choice.getKind() == OverloadChoiceKind::DeclViaDynamic ||
           subscript->getAttrs().hasAttribute<OptionalAttr>()) {
-        base = coerceObjectArgumentToType(base, baseTy, subscript,
-                                          AccessSemantics::Ordinary, locator);
+        base = coerceSelfArgumentToType(base, baseTy, subscript, locator);
         if (!base)
           return nullptr;
 
@@ -1833,8 +1829,8 @@ namespace {
       auto containerTy = solution.simplifyType(openedBaseType);
 
       if (baseIsInstance) {
-        base = coerceObjectArgumentToType(
-          base, containerTy, subscript, AccessSemantics::Ordinary,
+        base = coerceSelfArgumentToType(
+          base, containerTy, subscript,
           locator.withPathElement(ConstraintLocator::MemberRefBase));
       } else {
         base = coerceToType(base,
@@ -6870,9 +6866,14 @@ static bool isNonMutatingSetterPWAssignInsideInit(Expr *baseExpr,
 /// the given member.
 static Type adjustSelfTypeForMember(Expr *baseExpr,
                                     Type baseTy, ValueDecl *member,
-                                    AccessSemantics semantics,
                                     DeclContext *UseDC) {
-  auto baseObjectTy = baseTy->getWithoutSpecifierType();
+  assert(!baseTy->is<LValueType>());
+
+  auto inOutTy = baseTy->getAs<InOutType>();
+  if (!inOutTy)
+    return baseTy;
+
+  auto baseObjectTy = inOutTy->getObjectType();
 
   if (isa<ConstructorDecl>(member))
     return baseObjectTy;
@@ -6881,7 +6882,7 @@ static Type adjustSelfTypeForMember(Expr *baseExpr,
     // If 'self' is an inout type, turn the base type into an lvalue
     // type with the same qualifiers.
     if (func->isMutating())
-      return InOutType::get(baseObjectTy);
+      return baseTy;
 
     // Otherwise, return the rvalue type.
     return baseObjectTy;
@@ -6904,34 +6905,17 @@ static Type adjustSelfTypeForMember(Expr *baseExpr,
       !isNonMutatingSetterPWAssignInsideInit(baseExpr, member, UseDC))
     return baseObjectTy;
 
-  // If we're calling an accessor, keep the base as an inout type, because the
-  // getter may be mutating.
-  auto strategy = SD->getAccessStrategy(semantics,
-                                        isSettableFromHere
-                                          ? AccessKind::ReadWrite
-                                          : AccessKind::Read,
-                                        UseDC->getParentModule(),
-                                        UseDC->getResilienceExpansion());
-  if (baseTy->is<InOutType>() && strategy.getKind() != AccessStrategy::Storage)
-    return InOutType::get(baseObjectTy);
-
-  // Accesses to non-function members in value types are done through an @lvalue
-  // type.
-  if (baseTy->is<InOutType>())
-    return LValueType::get(baseObjectTy);
-  
-  // Accesses to members in values of reference type (classes, metatypes) are
-  // always done through a the reference to self.  Accesses to value types with
-  // a non-mutable self are also done through the base type.
-  return baseTy;
+  if (isa<SubscriptDecl>(member))
+    return baseTy;
+
+  return LValueType::get(baseObjectTy);
 }
 
 Expr *
-ExprRewriter::coerceObjectArgumentToType(Expr *expr,
-                                         Type baseTy, ValueDecl *member,
-                                         AccessSemantics semantics,
-                                         ConstraintLocatorBuilder locator) {
-  Type toType = adjustSelfTypeForMember(expr, baseTy, member, semantics, dc);
+ExprRewriter::coerceSelfArgumentToType(Expr *expr,
+                                       Type baseTy, ValueDecl *member,
+                                       ConstraintLocatorBuilder locator) {
+  Type toType = adjustSelfTypeForMember(expr, baseTy, member, dc);
 
   // If our expression already has the right type, we're done.
   Type fromType = cs.getType(expr);

lib/Sema/CSDiagnostics.cpp

@@ -1501,31 +1501,31 @@ bool RValueTreatedAsLValueFailure::diagnoseAsNote() {
   return true;
 }
 
-static Decl *findSimpleReferencedDecl(const Expr *E) {
+static VarDecl *findSimpleReferencedVarDecl(const Expr *E) {
   if (auto *LE = dyn_cast<LoadExpr>(E))
     E = LE->getSubExpr();
 
   if (auto *DRE = dyn_cast<DeclRefExpr>(E))
-    return DRE->getDecl();
+    return dyn_cast<VarDecl>(DRE->getDecl());
 
   return nullptr;
 }
 
-static std::pair<Decl *, Decl *> findReferencedDecl(const Expr *E) {
+static std::pair<VarDecl *, VarDecl *> findReferencedVarDecl(const Expr *E) {
   E = E->getValueProvidingExpr();
 
   if (auto *LE = dyn_cast<LoadExpr>(E))
-    return findReferencedDecl(LE->getSubExpr());
+    return findReferencedVarDecl(LE->getSubExpr());
 
   if (auto *AE = dyn_cast<AssignExpr>(E))
-    return findReferencedDecl(AE->getDest());
+    return findReferencedVarDecl(AE->getDest());
 
-  if (auto *D = findSimpleReferencedDecl(E))
+  if (auto *D = findSimpleReferencedVarDecl(E))
     return std::make_pair(nullptr, D);
 
   if (auto *MRE = dyn_cast<MemberRefExpr>(E)) {
-    if (auto *BaseDecl = findSimpleReferencedDecl(MRE->getBase()))
-      return std::make_pair(BaseDecl, MRE->getMember().getDecl());
+    if (auto *BaseDecl = findSimpleReferencedVarDecl(MRE->getBase()))
+      return std::make_pair(BaseDecl, cast<VarDecl>(MRE->getMember().getDecl()));
   }
 
   return std::make_pair(nullptr, nullptr);
@@ -1539,10 +1539,12 @@ bool TypeChecker::diagnoseSelfAssignment(const Expr *expr) {
   auto *dstExpr = assignExpr->getDest();
   auto *srcExpr = assignExpr->getSrc();
 
-  auto dstDecl = findReferencedDecl(dstExpr);
-  auto srcDecl = findReferencedDecl(srcExpr);
+  auto dstDecl = findReferencedVarDecl(dstExpr);
+  auto srcDecl = findReferencedVarDecl(srcExpr);
 
-  if (dstDecl.second && dstDecl == srcDecl) {
+  if (dstDecl.second &&
+      dstDecl.second->hasStorage() &&
+      dstDecl == srcDecl) {
     auto &DE = dstDecl.second->getASTContext().Diags;
     DE.diagnose(expr->getLoc(), dstDecl.first ? diag::self_assignment_prop
                                               : diag::self_assignment_var)

test/Sema/diag_self_assign.swift

@@ -21,6 +21,24 @@ class SA1 {
   }
 }
 
+struct SA1a {
+  var foo: Int = 0
+  init(fooi: Int) {
+    var foo = fooi
+    foo = foo // expected-error {{assigning a variable to itself}}
+    self.foo = self.foo // expected-error {{assigning a property to itself}}
+    foo = self.foo // no-error
+    self.foo = foo // no-error
+  }
+  mutating func f(fooi: Int) {
+    var foo = fooi
+    foo = foo // expected-error {{assigning a variable to itself}}
+    self.foo = self.foo // expected-error {{assigning a property to itself}}
+    foo = self.foo // no-error
+    self.foo = foo // no-error
+  }
+}
+
 class SA2 {
   var foo: Int {
     get {
@@ -31,14 +49,37 @@ class SA2 {
   init(fooi: Int) {
     var foo = fooi
     foo = foo // expected-error {{assigning a variable to itself}}
-    self.foo = self.foo // expected-error {{assigning a property to itself}}
+    self.foo = self.foo // no-error
     foo = self.foo // no-error
     self.foo = foo // no-error
   }
   func f(fooi: Int) {
     var foo = fooi
     foo = foo // expected-error {{assigning a variable to itself}}
-    self.foo = self.foo // expected-error {{assigning a property to itself}}
+    self.foo = self.foo // no-error
+    foo = self.foo // no-error
+    self.foo = foo // no-error
+  }
+}
+
+struct SA2a {
+  var foo: Int {
+    get {
+      return 0
+    }
+    set {}
+  }
+  init(fooi: Int) {
+    var foo = fooi
+    foo = foo // expected-error {{assigning a variable to itself}}
+    self.foo = self.foo // no-error
+    foo = self.foo // no-error
+    self.foo = foo // no-error
+  }
+  mutating func f(fooi: Int) {
+    var foo = fooi
+    foo = foo // expected-error {{assigning a variable to itself}}
+    self.foo = self.foo // no-error
     foo = self.foo // no-error
     self.foo = foo // no-error
   }
@@ -50,10 +91,24 @@ class SA3 {
       return foo // expected-warning {{attempting to access 'foo' within its own getter}} expected-note{{access 'self' explicitly to silence this warning}} {{14-14=self.}}
     }
     set {
-      foo = foo // expected-error {{assigning a property to itself}} expected-warning {{attempting to modify 'foo' within its own setter}} expected-note{{access 'self' explicitly to silence this warning}} {{7-7=self.}} expected-warning{{setter argument 'newValue' was never used, but the property was accessed}} expected-note{{did you mean to use 'newValue' instead of accessing the property's current value?}}
-      self.foo = self.foo // expected-error {{assigning a property to itself}}
-      foo = self.foo // expected-error {{assigning a property to itself}} expected-warning {{attempting to modify 'foo' within its own setter}} expected-note{{access 'self' explicitly to silence this warning}} {{7-7=self.}}
-      self.foo = foo // expected-error {{assigning a property to itself}}
+      foo = foo // expected-warning {{attempting to modify 'foo' within its own setter}} expected-note{{access 'self' explicitly to silence this warning}} {{7-7=self.}} expected-warning{{setter argument 'newValue' was never used, but the property was accessed}} expected-note{{did you mean to use 'newValue' instead of accessing the property's current value?}}
+      self.foo = self.foo // no-error
+      foo = self.foo // expected-warning {{attempting to modify 'foo' within its own setter}} expected-note{{access 'self' explicitly to silence this warning}} {{7-7=self.}}
+      self.foo = foo
+    }
+  }
+}
+
+struct SA3a {
+  var foo: Int {
+    get {
+      return foo // expected-warning {{attempting to access 'foo' within its own getter}} expected-note{{access 'self' explicitly to silence this warning}} {{14-14=self.}}
+    }
+    set {
+      foo = foo // expected-warning {{attempting to modify 'foo' within its own setter}} expected-note{{access 'self' explicitly to silence this warning}} {{7-7=self.}} expected-warning{{setter argument 'newValue' was never used, but the property was accessed}} expected-note{{did you mean to use 'newValue' instead of accessing the property's current value?}}
+      self.foo = self.foo // no-error
+      foo = self.foo // expected-warning {{attempting to modify 'foo' within its own setter}} expected-note{{access 'self' explicitly to silence this warning}} {{7-7=self.}}
+      self.foo = foo
     }
   }
 }
@@ -69,10 +124,29 @@ class SA4 {
   }
 }
 
+struct SA4a {
+  var foo: Int {
+    get {
+      return foo // expected-warning {{attempting to access 'foo' within its own getter}} expected-note{{access 'self' explicitly to silence this warning}} {{14-14=self.}}
+    }
+    set(value) {
+      value = value // expected-error {{cannot assign to value: 'value' is a 'let' constant}}
+    }
+  }
+}
+
 class SA5 {
   var foo: Int = 0
 }
-func SA5_test(a: SA4, b: SA4) {
+func SA5_test(a: SA5, b: SA5) {
+  a.foo = a.foo // expected-error {{assigning a property to itself}}
+  a.foo = b.foo
+}
+
+struct SA5a {
+  var foo: Int = 0
+}
+func SA5a_test(a: inout SA5, b: inout SA5) {
   a.foo = a.foo // expected-error {{assigning a property to itself}}
   a.foo = b.foo
 }

test/decl/var/didset_cycle.swift

@@ -0,0 +1,19 @@
+// RUN: %target-swift-frontend -typecheck %s
+
+func doSomething(_: Int) {}
+
+struct S {
+  var x: Int {
+    didSet {
+      doSomething(y)
+      doSomething(self.y)
+    }
+  }
+
+  var y: Int {
+    didSet {
+      doSomething(x)
+      doSomething(self.x)
+    }
+  }
+}