Merge pull request swiftlang#28131 from xedin/remove-argument-matcher

[CSDiag] Remove obsolete `ArgumentMatcher` from `FailureDiagnosis`

Author: xedin

lib/Sema/CSDiag.cpp

@@ -1915,134 +1915,6 @@ bool FailureDiagnosis::diagnoseImplicitSelfErrors(
   return false;
 }
 
-class ArgumentMatcher : public MatchCallArgumentListener {
-  Expr *ArgExpr;
-  ArrayRef<AnyFunctionType::Param> &Parameters;
-  const ParameterListInfo &ParamInfo;
-  SmallVectorImpl<AnyFunctionType::Param> &Arguments;
-
-  CalleeCandidateInfo CandidateInfo;
-
-  // Indicates if problem has been found and diagnostic was emitted.
-  bool Diagnosed = false;
-  // Indicates if functions we are trying to call is a subscript.
-  bool IsSubscript;
-
-  // Stores parameter bindings determined by call to matchCallArguments.
-  SmallVector<ParamBinding, 4> Bindings;
-
-public:
-  ArgumentMatcher(Expr *argExpr,
-                  ArrayRef<AnyFunctionType::Param> &params,
-                  const ParameterListInfo &paramInfo,
-                  SmallVectorImpl<AnyFunctionType::Param> &args,
-                  CalleeCandidateInfo &CCI, bool isSubscript)
-      : ArgExpr(argExpr), Parameters(params),
-        ParamInfo(paramInfo), Arguments(args), CandidateInfo(CCI),
-        IsSubscript(isSubscript) {}
-
-  bool outOfOrderArgument(unsigned argIdx, unsigned prevArgIdx) override {
-    auto &cs = CandidateInfo.CS;
-    OutOfOrderArgumentFailure failure(nullptr, cs, argIdx, prevArgIdx, Bindings,
-                                      cs.getConstraintLocator(ArgExpr));
-    Diagnosed = failure.diagnoseAsError();
-    return true;
-  }
-
-  bool relabelArguments(ArrayRef<Identifier> newNames) override {
-    assert(!newNames.empty() && "No arguments were re-labeled");
-
-    // Let's diagnose labeling problem but only related to corrected ones.
-    if (diagnoseArgumentLabelError(CandidateInfo.CS.getASTContext(), ArgExpr,
-                                   newNames, IsSubscript))
-      Diagnosed = true;
-
-    return true;
-  }
-
-  bool diagnose() {
-    // Use matchCallArguments to determine how close the argument list is (in
-    // shape) to the specified candidates parameters.  This ignores the
-    // concrete types of the arguments, looking only at the argument labels.
-    matchCallArguments(Arguments, Parameters, ParamInfo,
-                       CandidateInfo.hasTrailingClosure,
-                       /*allowFixes:*/ true, *this, Bindings);
-
-    return Diagnosed;
-  }
-};
-
-/// Emit a class of diagnostics that we only know how to generate when
-/// there is exactly one candidate we know about.  Return true if an error
-/// is emitted.
-static bool
-diagnoseSingleCandidateFailures(CalleeCandidateInfo &CCI, Expr *fnExpr,
-                                Expr *argExpr,
-                                ArrayRef<Identifier> argLabels) {
-  // We only handle the situation where there is exactly one candidate
-  // here.
-  if (CCI.size() != 1)
-    return false;
-
-  auto candidate = CCI[0];
-
-  if (!candidate.hasParameters())
-    return false;
-
-  auto params = candidate.getParameters();
-  auto paramInfo = candidate.getParameterListInfo(params);
-  auto args = decomposeArgType(CCI.CS.getType(argExpr), argLabels);
-
-  // Check the case where a raw-representable type is constructed from an
-  // argument with the same type:
-  //
-  //    MyEnumType(MyEnumType.foo)
-  //
-  // This is missing 'rawValue:' label, but a better fix is to just remove
-  // the unnecessary constructor call:
-  //
-  //    MyEnumType.foo
-  //
-  if (params.size() == 1 && args.size() == 1 && candidate.getDecl() &&
-      isa<ConstructorDecl>(candidate.getDecl()) && candidate.skipCurriedSelf) {
-    AnyFunctionType::Param &arg = args[0];
-    auto resTy =
-        candidate.getResultType()->lookThroughAllOptionalTypes();
-    auto rawTy = isRawRepresentable(CCI.CS, resTy);
-    if (rawTy && arg.getOldType() && resTy->isEqual(arg.getOldType())) {
-      auto getInnerExpr = [](Expr *E) -> Expr * {
-        auto *parenE = dyn_cast<ParenExpr>(E);
-        if (!parenE)
-          return nullptr;
-        return parenE->getSubExpr();
-      };
-      Expr *innerE = getInnerExpr(argExpr);
-
-      InFlightDiagnostic diag = CCI.CS.getASTContext().Diags.diagnose(
-          fnExpr->getLoc(),
-          diag::invalid_initialization_parameter_same_type, resTy);
-      diag.highlight((innerE ? innerE : argExpr)->getSourceRange());
-      if (innerE) {
-        // Remove the unnecessary constructor call.
-        diag.fixItRemoveChars(fnExpr->getLoc(), innerE->getStartLoc())
-            .fixItRemove(argExpr->getEndLoc());
-      }
-      return true;
-    }
-  }
-
-  // We only handle structural errors here.
-  if (CCI.closeness != CC_ArgumentLabelMismatch &&
-      CCI.closeness != CC_ArgumentCountMismatch)
-    return false;
-
-  // If we have a single candidate that failed to match the argument list,
-  // attempt to use matchCallArguments to diagnose the problem.
-  return ArgumentMatcher(argExpr, params, paramInfo, args, CCI,
-                         isa<SubscriptExpr>(fnExpr))
-      .diagnose();
-}
-
 // Extract expression for failed argument number
 static Expr *getFailedArgumentExpr(CalleeCandidateInfo CCI, Expr *argExpr) {
   if (auto *TE = dyn_cast<TupleExpr>(argExpr))
@@ -2094,11 +1966,6 @@ bool FailureDiagnosis::diagnoseParameterErrors(CalleeCandidateInfo &CCI,
   if (diagnoseImplicitSelfErrors(fnExpr, argExpr, CCI, argLabels))
     return true;
 
-  // Do all the stuff that we only have implemented when there is a single
-  // candidate.
-  if (diagnoseSingleCandidateFailures(CCI, fnExpr, argExpr, argLabels))
-    return true;
-
   // If we have a failure where the candidate set differs on exactly one
   // argument, and where we have a consistent mismatch across the candidate set
   // (often because there is only one candidate in the set), then diagnose this

lib/Sema/CSSimplify.cpp

@@ -930,23 +930,36 @@ class ArgumentFailureTracker : public MatchCallArgumentListener {
     }
 
     auto *anchor = Locator.getBaseLocator()->getAnchor();
-    if (!anchor || Arguments.size() != newLabels.size())
+    if (!anchor)
       return true;
 
     unsigned numExtraneous = 0;
     unsigned numRenames = 0;
+    unsigned numOutOfOrder = 0;
+
     for (unsigned i : indices(newLabels)) {
+      // It's already known how many arguments are missing,
+      // it would be accounted for in the impact.
+      if (i >= Arguments.size())
+        continue;
+
       auto argLabel = Arguments[i].getLabel();
       auto paramLabel = newLabels[i];
 
       if (argLabel == paramLabel)
         continue;
 
       if (!argLabel.empty()) {
-        if (paramLabel.empty())
+        // Instead of this being a label mismatch which requires
+        // re-labeling, this could be an out-of-order argument
+        // instead which has a completely different impact.
+        if (llvm::count(newLabels, argLabel) == 1) {
+          ++numOutOfOrder;
+        } else if (paramLabel.empty()) {
           ++numExtraneous;
-        else
+        } else {
           ++numRenames;
+        }
       }
     }
 
@@ -955,8 +968,12 @@ class ArgumentFailureTracker : public MatchCallArgumentListener {
     // Re-labeling fixes with extraneous/incorrect labels should be
     // lower priority vs. other fixes on same/different overload(s)
     // where labels did line up correctly.
-    CS.recordFix(fix, /*impact=*/1 + numExtraneous * 2 + numRenames * 3);
-    return false;
+    //
+    // If there are not only labeling problems but also some of the
+    // arguments are missing, let's account of that in the impact.
+    auto impact = 1 + numOutOfOrder + numExtraneous * 2 + numRenames * 3 +
+                  NumSynthesizedArgs * 2;
+    return CS.recordFix(fix, impact);
   }
 
   bool trailingClosureMismatch(unsigned paramIdx, unsigned argIdx) override {

test/Constraints/diagnostics.swift

@@ -931,12 +931,10 @@ func valueForKey<K>(_ key: K) -> CacheValue? {
 // SR-2242: poor diagnostic when argument label is omitted
 
 func r27212391(x: Int, _ y: Int) {
-  // expected-note@-1 {{candidate '(Int, Int) -> ()' requires 2 arguments, but 3 were provided}}
   let _: Int = x + y
 }
 
 func r27212391(a: Int, x: Int, _ y: Int) {
-  // expected-note@-1 {{incorrect labels for candidate (have: '(a:y:x:)', expected: '(a:x:_:)')}}
   let _: Int = a + x + y
 }
 
@@ -948,7 +946,7 @@ r27212391(y: 3, 5)          // expected-error {{incorrect argument label in call
 r27212391(x: 3, x: 5)       // expected-error {{extraneous argument label 'x:' in call}}
 r27212391(a: 1, 3, y: 5)    // expected-error {{incorrect argument labels in call (have 'a:_:y:', expected 'a:x:_:')}}
 r27212391(1, x: 3, y: 5)    // expected-error {{incorrect argument labels in call (have '_:x:y:', expected 'a:x:_:')}}
-r27212391(a: 1, y: 3, x: 5) // expected-error {{no exact matches in call to global function 'r27212391'}}
+r27212391(a: 1, y: 3, x: 5) // expected-error {{incorrect argument labels in call (have 'a:y:x:', expected 'a:x:_:')}}
 r27212391(a: 1, 3, x: 5)    // expected-error {{argument 'x' must precede unnamed argument #2}} {{17-17=x: 5, }} {{18-24=}}
 
 // SR-1255

test/expr/expressions.swift

@@ -602,7 +602,7 @@ var ruleVar: Rule
 // FIXME(diagnostics): To be able to suggest different candidates here we need to teach the solver how to figure out to which parameter
 // does argument belong to in this case. If the `target` was of a different type, we currently suggest to add an argument for `dependencies:`
 // which is incorrect.
-ruleVar = Rule("a") // expected-error {{cannot convert value of type 'String' to expected argument type '(target: String, dependencies: String)'}}
+ruleVar = Rule("a") // expected-error {{missing argument label 'target:' in call}}
 
 
 class C {