Merge pull request #26014 from xedin/diagnose-mutable-methods

[Diagnostics] Port invalid reference to `mutating` member diagnostics to new framework

Author: xedin

lib/Sema/CSDiag.cpp

@@ -469,13 +469,6 @@ class FailureDiagnosis :public ASTVisitor<FailureDiagnosis, /*exprresult*/bool>{
                                     CalleeCandidateInfo &calleeInfo,
                                     SourceLoc applyLoc);
 
-  /// Produce diagnostic for failures related to attributes associated with
-  /// candidate functions/methods e.g. mutability.
-  bool diagnoseMethodAttributeFailures(ApplyExpr *expr,
-                                       ArrayRef<Identifier> argLabels,
-                                       bool hasTrailingClosure,
-                                       CalleeCandidateInfo &candidates);
-
   /// Produce diagnostic for failures related to unfulfilled requirements
   /// of the generic parameters used as arguments.
   bool diagnoseArgumentGenericRequirements(TypeChecker &TC, Expr *callExpr,
@@ -777,14 +770,8 @@ void FailureDiagnosis::diagnoseUnviableLookupResults(
     }
     case MemberLookupResult::UR_MutatingMemberOnRValue:
     case MemberLookupResult::UR_MutatingGetterOnRValue: {
-      auto diagIDsubelt = diag::cannot_pass_rvalue_mutating_subelement;
-      auto diagIDmember = diag::cannot_pass_rvalue_mutating;
-      if (firstProblem == MemberLookupResult::UR_MutatingGetterOnRValue) {
-        diagIDsubelt = diag::cannot_pass_rvalue_mutating_getter_subelement;
-        diagIDmember = diag::cannot_pass_rvalue_mutating_getter;
-      }
-      assert(baseExpr && "Cannot have a mutation failure without a base");
-      AssignmentFailure failure(baseExpr, CS, loc, diagIDsubelt, diagIDmember);
+      MutatingMemberRefOnImmutableBase failure(E, CS, choice->getDecl(),
+                                               CS.getConstraintLocator(E));
       (void)failure.diagnose();
       return;
     }
@@ -3979,86 +3966,6 @@ bool FailureDiagnosis::diagnoseNilLiteralComparison(
   return true;
 }
 
-bool FailureDiagnosis::diagnoseMethodAttributeFailures(
-    swift::ApplyExpr *callExpr, ArrayRef<Identifier> argLabels,
-    bool hasTrailingClosure, CalleeCandidateInfo &candidates) {
-  auto UDE = dyn_cast<UnresolvedDotExpr>(callExpr->getFn());
-  if (!UDE)
-    return false;
-
-  auto argExpr = callExpr->getArg();
-  auto argType = CS.getType(argExpr);
-
-  // If type of the argument hasn't been established yet, we can't diagnose.
-  if (!argType || isUnresolvedOrTypeVarType(argType))
-    return false;
-
-  // Let's filter our candidate list based on that type.
-  candidates.filterListArgs(decomposeArgType(argType, argLabels));
-
-  if (candidates.closeness == CC_ExactMatch)
-    return false;
-
-  // And if filtering didn't give an exact match, such means that problem
-  // might be related to function attributes which is best diagnosed by
-  // unviable member candidates, if any.
-  auto base = UDE->getBase();
-  auto baseType = CS.getType(base);
-
-  // This handles following situation:
-  // struct S {
-  //   mutating func f(_ i: Int) {}
-  //   func f(_ f: Float) {}
-  // }
-  //
-  // Given struct has an overloaded method "f" with a single argument of
-  // multiple different types, one of the overloads is marked as
-  // "mutating", which means it can only be applied on LValue base type.
-  // So when struct is used like this:
-  //
-  // let answer: Int = 42
-  // S().f(answer)
-  //
-  // Constraint system generator is going to pick `f(_ f: Float)` as
-  // only possible overload candidate because "base" of the call is immutable
-  // and contextual information about argument type is not available yet.
-  // Such leads to incorrect contextual conversion failure diagnostic because
-  // type of the argument is going to resolved as (Int) no matter what.
-  // To workaround that fact and improve diagnostic of such cases we are going
-  // to try and collect all unviable candidates for a given call and check if
-  // at least one of them matches established argument type before even trying
-  // to re-check argument expression.
-  auto results = CS.performMemberLookup(
-      ConstraintKind::ValueMember, UDE->getName(), baseType,
-      UDE->getFunctionRefKind(), CS.getConstraintLocator(UDE),
-      /*includeInaccessibleMembers=*/false);
-
-  if (results.UnviableCandidates.empty())
-    return false;
-
-  SmallVector<OverloadChoice, 2> choices;
-  for (auto &unviable : results.UnviableCandidates)
-    choices.push_back(OverloadChoice(baseType, unviable.getDecl(),
-                                     UDE->getFunctionRefKind()));
-
-  CalleeCandidateInfo unviableCandidates(baseType, choices, hasTrailingClosure,
-                                         CS);
-
-  // Filter list of the unviable candidates based on the
-  // already established type of the argument expression.
-  unviableCandidates.filterListArgs(decomposeArgType(argType, argLabels));
-
-  // If one of the unviable candidates matches arguments exactly,
-  // that means that actual problem is related to function attributes.
-  if (unviableCandidates.closeness == CC_ExactMatch) {
-    diagnoseUnviableLookupResults(results, UDE, baseType, base, UDE->getName(),
-                                  UDE->getNameLoc(), UDE->getLoc());
-    return true;
-  }
-
-  return false;
-}
-
 bool FailureDiagnosis::diagnoseArgumentGenericRequirements(
     TypeChecker &TC, Expr *callExpr, Expr *fnExpr, Expr *argExpr,
     CalleeCandidateInfo &candidates, ArrayRef<Identifier> argLabels) {
@@ -4811,14 +4718,6 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
                               callExpr->getArg(), argLabels))
     return true;
 
-  // There might be a candidate with correct argument types but it's not
-  // used by constraint solver because it doesn't have correct attributes,
-  // let's try to diagnose such situation there right before type checking
-  // argument expression, because that would overwrite original argument types.
-  if (diagnoseMethodAttributeFailures(callExpr, argLabels, hasTrailingClosure,
-                                      calleeInfo))
-    return true;
-
   Type argType;  // argument list, if known.
   if (auto FTy = fnType->getAs<AnyFunctionType>()) {
     argType = FunctionType::composeInput(CS.getASTContext(), FTy->getParams(),

lib/Sema/CSDiagnostics.cpp

@@ -105,6 +105,20 @@ Expr *FailureDiagnostic::getArgumentExprFor(Expr *anchor) const {
   return nullptr;
 }
 
+Expr *FailureDiagnostic::getBaseExprFor(Expr *anchor) const {
+  if (!anchor)
+    return nullptr;
+
+  if (auto *UDE = dyn_cast<UnresolvedDotExpr>(anchor))
+    return UDE->getBase();
+  else if (auto *SE = dyn_cast<SubscriptExpr>(anchor))
+    return SE->getBase();
+  else if (auto *MRE = dyn_cast<MemberRefExpr>(anchor))
+    return MRE->getBase();
+
+  return nullptr;
+}
+
 Optional<SelectedOverload> FailureDiagnostic::getChoiceFor(Expr *expr) const {
   auto &cs = getConstraintSystem();
   return getOverloadChoiceIfAvailable(cs.getCalleeLocator(expr));
@@ -1484,13 +1498,11 @@ bool AssignmentFailure::diagnoseAsError() {
   }
 
   if (auto IE = dyn_cast<IfExpr>(immInfo.first)) {
-    if (isLoadedLValue(IE)) {
-      emitDiagnostic(Loc, DeclDiagnostic,
-                     "result of conditional operator '? :' is never mutable")
-          .highlight(IE->getQuestionLoc())
-          .highlight(IE->getColonLoc());
-      return true;
-    }
+    emitDiagnostic(Loc, DeclDiagnostic,
+                   "result of conditional operator '? :' is never mutable")
+        .highlight(IE->getQuestionLoc())
+        .highlight(IE->getColonLoc());
+    return true;
   }
 
   emitDiagnostic(Loc, TypeDiagnostic, getType(destExpr))
@@ -3459,3 +3471,25 @@ bool SkipUnhandledConstructInFunctionBuilderFailure::diagnoseAsNote() {
   diagnosePrimary(/*asNote=*/true);
   return true;
 }
+
+bool MutatingMemberRefOnImmutableBase::diagnoseAsError() {
+  auto *anchor = getRawAnchor();
+  auto baseExpr = getBaseExprFor(anchor);
+  if (!baseExpr)
+    return false;
+
+  auto diagIDsubelt = diag::cannot_pass_rvalue_mutating_subelement;
+  auto diagIDmember = diag::cannot_pass_rvalue_mutating;
+
+  if (auto *storage = dyn_cast<AbstractStorageDecl>(Member)) {
+    if (storage->isGetterMutating()) {
+      diagIDsubelt = diag::cannot_pass_rvalue_mutating_getter_subelement;
+      diagIDmember = diag::cannot_pass_rvalue_mutating_getter;
+    }
+  }
+
+  auto &cs = getConstraintSystem();
+  AssignmentFailure failure(baseExpr, cs, anchor->getLoc(), diagIDsubelt,
+                            diagIDmember);
+  return failure.diagnoseAsError();
+}

lib/Sema/CSDiagnostics.h

@@ -168,6 +168,10 @@ class FailureDiagnostic {
   /// in the root expression or `nullptr` otherwise.
   Expr *findParentExpr(Expr *subExpr) const;
 
+  /// If given expression is some kind of a member reference e.g.
+  /// `x.foo` or `x[0]` extract and return its base expression.
+  Expr *getBaseExprFor(Expr *anchor) const;
+
   /// \returns An argument expression if given anchor is a call, member
   /// reference or subscript, nullptr otherwise.
   Expr *getArgumentExprFor(Expr *anchor) const;
@@ -708,16 +712,6 @@ class AssignmentFailure final : public FailureDiagnostic {
 
   static Diag<StringRef> findDeclDiagonstic(ASTContext &ctx, Expr *destExpr);
 
-  static bool isLoadedLValue(Expr *expr) {
-    expr = expr->getSemanticsProvidingExpr();
-    if (isa<LoadExpr>(expr))
-      return true;
-    if (auto ifExpr = dyn_cast<IfExpr>(expr))
-      return isLoadedLValue(ifExpr->getThenExpr()) &&
-             isLoadedLValue(ifExpr->getElseExpr());
-    return false;
-  }
-
   /// Retrive an member reference associated with given member
   /// looking through dynamic member lookup on the way.
   Optional<OverloadChoice> getMemberRef(ConstraintLocator *locator) const;
@@ -1152,6 +1146,30 @@ class InaccessibleMemberFailure final : public FailureDiagnostic {
   bool diagnoseAsError() override;
 };
 
+/// Diagnose an attempt to reference member marked as `mutating`
+/// on immutable base e.g. `let` variable:
+///
+/// ```swift
+/// struct S {
+///   mutating func foo(_ i: Int) {}
+///   func foo(_ f: Float) {}
+/// }
+///
+/// func bar(_ s: S, _ answer: Int) {
+///  s.foo(answer)
+/// }
+/// ```
+class MutatingMemberRefOnImmutableBase final : public FailureDiagnostic {
+  ValueDecl *Member;
+
+public:
+  MutatingMemberRefOnImmutableBase(Expr *root, ConstraintSystem &cs,
+                                   ValueDecl *member,
+                                   ConstraintLocator *locator)
+      : FailureDiagnostic(root, cs, locator), Member(member) {}
+
+  bool diagnoseAsError() override;
+};
 
 // Diagnose an attempt to use AnyObject as the root type of a KeyPath
 //

lib/Sema/CSFix.cpp

@@ -342,14 +342,16 @@ AllowMemberRefOnExistential::create(ConstraintSystem &cs, Type baseType,
 }
 
 bool AllowMemberRefOnExistential::diagnose(Expr *root, bool asNote) const {
-  auto failure = InvalidMemberRefOnExistential(root, getConstraintSystem(),
-                                               BaseType, Name, getLocator());
+  auto failure =
+      InvalidMemberRefOnExistential(root, getConstraintSystem(), getBaseType(),
+                                    getMemberName(), getLocator());
   return failure.diagnose(asNote);
 }
 
 bool AllowTypeOrInstanceMember::diagnose(Expr *root, bool asNote) const {
   auto failure = AllowTypeOrInstanceMemberFailure(
-      root, getConstraintSystem(), BaseType, Member, UsedName, getLocator());
+      root, getConstraintSystem(), getBaseType(), getMember(), getMemberName(),
+      getLocator());
   return failure.diagnose(asNote);
 }
 
@@ -472,15 +474,17 @@ MoveOutOfOrderArgument *MoveOutOfOrderArgument::create(
 }
 
 bool AllowInaccessibleMember::diagnose(Expr *root, bool asNote) const {
-  InaccessibleMemberFailure failure(root, getConstraintSystem(), Member,
+  InaccessibleMemberFailure failure(root, getConstraintSystem(), getMember(),
                                     getLocator());
   return failure.diagnose(asNote);
 }
 
 AllowInaccessibleMember *
-AllowInaccessibleMember::create(ConstraintSystem &cs, ValueDecl *member,
+AllowInaccessibleMember::create(ConstraintSystem &cs, Type baseType,
+                                ValueDecl *member, DeclName name,
                                 ConstraintLocator *locator) {
-  return new (cs.getAllocator()) AllowInaccessibleMember(cs, member, locator);
+  return new (cs.getAllocator())
+      AllowInaccessibleMember(cs, baseType, member, name, locator);
 }
 
 bool AllowAnyObjectKeyPathRoot::diagnose(Expr *root, bool asNote) const {
@@ -639,3 +643,17 @@ bool SkipUnhandledConstructInFunctionBuilder::diagnose(Expr *root,
       root, getConstraintSystem(), unhandled, builder, getLocator());
   return failure.diagnose(asNote);
 }
+
+bool AllowMutatingMemberOnRValueBase::diagnose(Expr *root, bool asNote) const {
+  auto &cs = getConstraintSystem();
+  MutatingMemberRefOnImmutableBase failure(root, cs, getMember(), getLocator());
+  return failure.diagnose(asNote);
+}
+
+AllowMutatingMemberOnRValueBase *
+AllowMutatingMemberOnRValueBase::create(ConstraintSystem &cs, Type baseType,
+                                        ValueDecl *member, DeclName name,
+                                        ConstraintLocator *locator) {
+  return new (cs.getAllocator())
+      AllowMutatingMemberOnRValueBase(cs, baseType, member, name, locator);
+}