[CSDiagnostics] A couple of fixes for the "self." immutability… (#28288)

[CSDiagnostics] A couple of fixes for the "self." immutability fix-it

Author: hamishknight

include/swift/AST/DiagnosticsSema.def

@@ -3424,9 +3424,9 @@ ERROR(assignment_bang_has_immutable_subcomponent,none,
 NOTE(change_to_mutating,none,
      "mark %select{method|accessor}0 'mutating' to make 'self' mutable",
      (bool))
-NOTE(masked_instance_variable,none,
-     "add explicit 'self.' to refer to mutable property of %0",
-     (Type))
+NOTE(masked_mutable_property,none,
+     "add explicit '%0' to refer to mutable %1 of %2",
+     (StringRef, DescriptiveDeclKind, Type))
 
 ERROR(assignment_let_property_delegating_init,none,
       "'let' property %0 may not be initialized directly; use "

lib/Sema/CSDiagnostics.cpp

@@ -1555,9 +1555,9 @@ bool AssignmentFailure::diagnoseAsError() {
 
   // Walk through the destination expression, resolving what the problem is.  If
   // we find a node in the lvalue path that is problematic, this returns it.
-  auto immInfo = resolveImmutableBase(DestExpr);
-
-  Optional<OverloadChoice> choice = immInfo.second;
+  Expr *immutableExpr;
+  Optional<OverloadChoice> choice;
+  std::tie(immutableExpr, choice) = resolveImmutableBase(DestExpr);
 
   // Attempt diagnostics based on the overload choice.
   if (choice.hasValue()) {
@@ -1571,17 +1571,17 @@ bool AssignmentFailure::diagnoseAsError() {
 
     if (!choice->isDecl()) {
       if (choice->getKind() == OverloadChoiceKind::KeyPathApplication &&
-          !isa<ApplyExpr>(immInfo.first)) {
+          !isa<ApplyExpr>(immutableExpr)) {
         std::string message = "key path is read-only";
-        if (auto *SE = dyn_cast<SubscriptExpr>(immInfo.first)) {
+        if (auto *SE = dyn_cast<SubscriptExpr>(immutableExpr)) {
           if (auto *DRE = dyn_cast<DeclRefExpr>(getKeyPathArgument(SE))) {
             auto identifier = DRE->getDecl()->getBaseName().getIdentifier();
             message =
                 "'" + identifier.str().str() + "' is a read-only key path";
           }
         }
         emitDiagnostic(Loc, DeclDiagnostic, message)
-            .highlight(immInfo.first->getSourceRange());
+            .highlight(immutableExpr->getSourceRange());
         return true;
       }
       return false;
@@ -1595,7 +1595,7 @@ bool AssignmentFailure::diagnoseAsError() {
       message += VD->getName().str().str();
       message += "'";
 
-      auto type = getType(immInfo.first);
+      auto type = getType(immutableExpr);
 
       if (isKnownKeyPathType(type))
         message += " is read-only";
@@ -1614,26 +1614,54 @@ bool AssignmentFailure::diagnoseAsError() {
       }
 
       emitDiagnostic(Loc, DeclDiagnostic, message)
-          .highlight(immInfo.first->getSourceRange());
+          .highlight(immutableExpr->getSourceRange());
 
-      // If there is a masked instance variable of the same type, emit a
-      // note to fixit prepend a 'self.'.
+      // If there is a masked property of the same type, emit a
+      // note to fixit prepend a 'self.' or 'Type.'.
       if (auto typeContext = DC->getInnermostTypeContext()) {
-        UnqualifiedLookup lookup(VD->getFullName(), typeContext);
-        for (auto &result : lookup.Results) {
-          const VarDecl *typeVar = dyn_cast<VarDecl>(result.getValueDecl());
-          if (typeVar && typeVar != VD && typeVar->isSettable(DC) &&
-              typeVar->isSetterAccessibleFrom(DC) &&
-              typeVar->getType()->isEqual(VD->getType())) {
-            // But not in its own accessor.
-            auto AD =
-                dyn_cast_or_null<AccessorDecl>(DC->getInnermostMethodContext());
-            if (!AD || AD->getStorage() != typeVar) {
-              emitDiagnostic(Loc, diag::masked_instance_variable,
-                             typeContext->getSelfTypeInContext())
-                  .fixItInsert(Loc, "self.");
-            }
+        SmallVector<ValueDecl *, 2> results;
+        DC->lookupQualified(typeContext->getSelfNominalTypeDecl(),
+                            VD->getFullName(),
+                            NL_QualifiedDefault | NL_RemoveNonVisible, results);
+
+        auto foundProperty = llvm::find_if(results, [&](ValueDecl *decl) {
+          // We're looking for a settable property that is the same type as the
+          // var we found.
+          auto *var = dyn_cast<VarDecl>(decl);
+          if (!var || var == VD)
+            return false;
+
+          if (!var->isSettable(DC) || !var->isSetterAccessibleFrom(DC))
+            return false;
+
+          if (!var->getType()->isEqual(VD->getType()))
+            return false;
+
+          // Don't suggest a property if we're in one of its accessors.
+          auto *methodDC = DC->getInnermostMethodContext();
+          if (auto *AD = dyn_cast_or_null<AccessorDecl>(methodDC))
+            if (AD->getStorage() == var)
+              return false;
+
+          return true;
+        });
+
+        if (foundProperty != results.end()) {
+          auto startLoc = immutableExpr->getStartLoc();
+          auto *property = *foundProperty;
+          auto selfTy = typeContext->getSelfTypeInContext();
+
+          // If we found an instance property, suggest inserting "self.",
+          // otherwise suggest "Type." for a static property.
+          std::string fixItText;
+          if (property->isInstanceMember()) {
+            fixItText = "self.";
+          } else {
+            fixItText = selfTy->getString() + ".";
           }
+          emitDiagnostic(startLoc, diag::masked_mutable_property,
+                         fixItText, property->getDescriptiveKind(), selfTy)
+              .fixItInsert(startLoc, fixItText);
         }
       }
 
@@ -1654,7 +1682,7 @@ bool AssignmentFailure::diagnoseAsError() {
         message = "subscript is immutable";
 
       emitDiagnostic(Loc, DeclDiagnostic, message)
-          .highlight(immInfo.first->getSourceRange());
+          .highlight(immutableExpr->getSourceRange());
       return true;
     }
 
@@ -1676,7 +1704,7 @@ bool AssignmentFailure::diagnoseAsError() {
         message += " is not settable";
 
       emitDiagnostic(Loc, diagID, message)
-          .highlight(immInfo.first->getSourceRange());
+          .highlight(immutableExpr->getSourceRange());
       return true;
     }
   }
@@ -1686,21 +1714,21 @@ bool AssignmentFailure::diagnoseAsError() {
 
   // If a keypath was the problem but wasn't resolved into a vardecl
   // it is ambiguous or unable to be used for setting.
-  if (auto *KPE = dyn_cast_or_null<KeyPathExpr>(immInfo.first)) {
+  if (auto *KPE = dyn_cast_or_null<KeyPathExpr>(immutableExpr)) {
     emitDiagnostic(Loc, DeclDiagnostic, "immutable key path")
         .highlight(KPE->getSourceRange());
     return true;
   }
 
-  if (auto LE = dyn_cast<LiteralExpr>(immInfo.first)) {
+  if (auto LE = dyn_cast<LiteralExpr>(immutableExpr)) {
     emitDiagnostic(Loc, DeclDiagnostic, "literals are not mutable")
         .highlight(LE->getSourceRange());
     return true;
   }
 
   // If the expression is the result of a call, it is an rvalue, not a mutable
   // lvalue.
-  if (auto *AE = dyn_cast<ApplyExpr>(immInfo.first)) {
+  if (auto *AE = dyn_cast<ApplyExpr>(immutableExpr)) {
     // Handle literals, which are a call to the conversion function.
     auto argsTuple =
         dyn_cast<TupleExpr>(AE->getArg()->getSemanticsProvidingExpr());
@@ -1733,22 +1761,22 @@ bool AssignmentFailure::diagnoseAsError() {
     return true;
   }
 
-  if (auto contextualType = cs.getContextualType(immInfo.first)) {
+  if (auto contextualType = cs.getContextualType(immutableExpr)) {
     Type neededType = contextualType->getInOutObjectType();
-    Type actualType = getType(immInfo.first)->getInOutObjectType();
+    Type actualType = getType(immutableExpr)->getInOutObjectType();
     if (!neededType->isEqual(actualType)) {
       if (DeclDiagnostic.ID != diag::cannot_pass_rvalue_inout_subelement.ID) {
         emitDiagnostic(Loc, DeclDiagnostic,
                        "implicit conversion from '" + actualType->getString() +
                            "' to '" + neededType->getString() +
                            "' requires a temporary")
-            .highlight(immInfo.first->getSourceRange());
+            .highlight(immutableExpr->getSourceRange());
       }
       return true;
     }
   }
 
-  if (auto IE = dyn_cast<IfExpr>(immInfo.first)) {
+  if (auto IE = dyn_cast<IfExpr>(immutableExpr)) {
     emitDiagnostic(Loc, DeclDiagnostic,
                    "result of conditional operator '? :' is never mutable")
         .highlight(IE->getQuestionLoc())
@@ -1757,7 +1785,7 @@ bool AssignmentFailure::diagnoseAsError() {
   }
 
   emitDiagnostic(Loc, TypeDiagnostic, getType(DestExpr))
-      .highlight(immInfo.first->getSourceRange());
+      .highlight(immutableExpr->getSourceRange());
   return true;
 }
 

test/Sema/immutability.swift

@@ -701,3 +701,43 @@ extension JustAProtocol {
     name = "World" // expected-error {{cannot assign to property: 'self' is immutable}}
   }
 }
+
+struct S {
+  var x = 0
+  static var y = 0
+
+  struct Nested {
+    func foo() {
+      // SR-11786: Make sure we don't offer the 'self.' fix-it here.
+      let x = 0 // expected-note {{change 'let' to 'var' to make it mutable}}
+      x += 1 // expected-error {{left side of mutating operator isn't mutable: 'x' is a 'let' constant}}
+    }
+  }
+
+  func bar() {
+    // SR-11787: Make sure we insert "self." in the right location.
+    let x = 0 // expected-note 3{{change 'let' to 'var' to make it mutable}}
+    x += 1 // expected-error {{left side of mutating operator isn't mutable: 'x' is a 'let' constant}}
+    // expected-note@-1 {{add explicit 'self.' to refer to mutable property of 'S'}} {{5-5=self.}}
+
+    (try x) += 1 // expected-error {{left side of mutating operator isn't mutable: 'x' is a 'let' constant}}
+    // expected-note@-1 {{add explicit 'self.' to refer to mutable property of 'S'}} {{10-10=self.}}
+
+    x = 1 // expected-error {{cannot assign to value: 'x' is a 'let' constant}}
+    // expected-note@-1 {{add explicit 'self.' to refer to mutable property of 'S'}} {{5-5=self.}}
+
+    // SR-11788: Insert "Type." for a static property.
+    let y = 0 // expected-note {{change 'let' to 'var' to make it mutable}}
+    y += 1 // expected-error {{left side of mutating operator isn't mutable: 'y' is a 'let' constant}}
+    // expected-note@-1 {{add explicit 'S.' to refer to mutable static property of 'S'}} {{5-5=S.}}
+  }
+}
+
+struct S2<T> {
+  static var y: Int { get { 0 } set {} }
+  func foo() {
+    let y = 0 // expected-note {{change 'let' to 'var' to make it mutable}}
+    y += 1 // expected-error {{left side of mutating operator isn't mutable: 'y' is a 'let' constant}}
+    // expected-note@-1 {{add explicit 'S2<T>.' to refer to mutable static property of 'S2<T>'}} {{5-5=S2<T>.}}
+  }
+}