SR-2242: poor diagnostic when argument label is omitted

lib/Sema/CSDiag.cpp

@@ -1274,6 +1274,9 @@ CalleeCandidateInfo::evaluateCloseness(DeclContext *dc, Type candArgListType,
     void missingArgument(unsigned paramIdx) override {
       result = CC_ArgumentCountMismatch;
     }
+    void missingLabel(unsigned paramIdx) override {
+      result = CC_ArgumentLabelMismatch;
+    }
     void outOfOrderArgument(unsigned argIdx, unsigned prevArgIdx) override {
       result = CC_ArgumentLabelMismatch;
     }
@@ -4671,152 +4674,159 @@ static bool diagnoseSingleCandidateFailures(CalleeCandidateInfo &CCI,
       CCI.closeness != CC_ArgumentCountMismatch)
     return false;
 
-  SmallVector<Identifier, 4> correctNames;
-  unsigned OOOArgIdx = ~0U, OOOPrevArgIdx = ~0U;
-  unsigned extraArgIdx = ~0U, missingParamIdx = ~0U;
-
   // If we have a single candidate that failed to match the argument list,
   // attempt to use matchCallArguments to diagnose the problem.
-  struct OurListener : public MatchCallArgumentListener {
-    SmallVectorImpl<Identifier> &correctNames;
-    unsigned &OOOArgIdx, &OOOPrevArgIdx;
-    unsigned &extraArgIdx, &missingParamIdx;
+  class ArgumentDiagnostic : public MatchCallArgumentListener {
+    TypeChecker &TC;
+    Expr *ArgExpr;
+    llvm::SmallVectorImpl<CallArgParam> &Parameters;
+    llvm::SmallVectorImpl<CallArgParam> &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:
-    OurListener(SmallVectorImpl<Identifier> &correctNames,
-                unsigned &OOOArgIdx, unsigned &OOOPrevArgIdx,
-                unsigned &extraArgIdx, unsigned &missingParamIdx)
-    : correctNames(correctNames),
-    OOOArgIdx(OOOArgIdx), OOOPrevArgIdx(OOOPrevArgIdx),
-    extraArgIdx(extraArgIdx), missingParamIdx(missingParamIdx) {}
-    void extraArgument(unsigned argIdx) override {
-      extraArgIdx = argIdx;
-    }
-    void missingArgument(unsigned paramIdx) override {
-      missingParamIdx = paramIdx;
+    ArgumentDiagnostic(Expr *argExpr,
+                       llvm::SmallVectorImpl<CallArgParam> &params,
+                       llvm::SmallVectorImpl<CallArgParam> &args,
+                       CalleeCandidateInfo &CCI, bool isSubscript)
+        : TC(CCI.CS->TC), ArgExpr(argExpr), Parameters(params), Arguments(args),
+          CandidateInfo(CCI), IsSubscript(isSubscript) {}
+
+    void extraArgument(unsigned extraArgIdx) override {
+      auto name = Arguments[extraArgIdx].Label;
+      Expr *arg = ArgExpr;
+
+      auto tuple = dyn_cast<TupleExpr>(ArgExpr);
+      if (tuple)
+        arg = tuple->getElement(extraArgIdx);
+
+      auto loc = arg->getLoc();
+      if (tuple && extraArgIdx == tuple->getNumElements() - 1 &&
+          tuple->hasTrailingClosure())
+        TC.diagnose(loc, diag::extra_trailing_closure_in_call)
+            .highlight(arg->getSourceRange());
+      else if (Parameters.empty())
+        TC.diagnose(loc, diag::extra_argument_to_nullary_call)
+            .highlight(ArgExpr->getSourceRange());
+      else if (name.empty())
+        TC.diagnose(loc, diag::extra_argument_positional)
+            .highlight(arg->getSourceRange());
+      else
+        TC.diagnose(loc, diag::extra_argument_named, name)
+            .highlight(arg->getSourceRange());
+
+      Diagnosed = true;
     }
-    void outOfOrderArgument(unsigned argIdx, unsigned prevArgIdx) override{
-      OOOArgIdx = argIdx;
-      OOOPrevArgIdx = prevArgIdx;
+
+    void missingArgument(unsigned missingParamIdx) override {
+      Identifier name = Parameters[missingParamIdx].Label;
+      auto loc = ArgExpr->getStartLoc();
+      if (name.empty())
+        TC.diagnose(loc, diag::missing_argument_positional,
+                    missingParamIdx + 1);
+      else
+        TC.diagnose(loc, diag::missing_argument_named, name);
+
+      auto candidate = CandidateInfo[0];
+      if (candidate.getDecl())
+        TC.diagnose(candidate.getDecl(), diag::decl_declared_here,
+                    candidate.getDecl()->getFullName());
+
+      Diagnosed = true;
     }
-    bool relabelArguments(ArrayRef<Identifier> newNames) override {
-      correctNames.append(newNames.begin(), newNames.end());
-      return true;
+
+    void missingLabel(unsigned paramIdx) override {
+      auto tuple = cast<TupleExpr>(ArgExpr);
+      TC.diagnose(tuple->getElement(paramIdx)->getStartLoc(),
+                  diag::missing_argument_labels, false,
+                  Parameters[paramIdx].Label.str(), IsSubscript);
+
+      Diagnosed = true;
     }
-  } listener(correctNames, OOOArgIdx, OOOPrevArgIdx,
-             extraArgIdx, missingParamIdx);
 
-  // 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.
-  SmallVector<ParamBinding, 4> paramBindings;
-  if (!matchCallArguments(args, params, CCI.hasTrailingClosure,
-                          /*allowFixes:*/true, listener, paramBindings))
-    return false;
+    void outOfOrderArgument(unsigned argIdx, unsigned prevArgIdx) override {
+      auto tuple = cast<TupleExpr>(ArgExpr);
+      Identifier first = tuple->getElementName(argIdx);
+      Identifier second = tuple->getElementName(prevArgIdx);
+
+      // Build a mapping from arguments to parameters.
+      SmallVector<unsigned, 4> argBindings(tuple->getNumElements());
+      for (unsigned paramIdx = 0; paramIdx != Bindings.size(); ++paramIdx) {
+        for (auto argIdx : Bindings[paramIdx])
+          argBindings[argIdx] = paramIdx;
+      }
 
+      auto argRange = [&](unsigned argIdx, Identifier label) -> SourceRange {
+        auto range = tuple->getElement(argIdx)->getSourceRange();
+        if (!label.empty())
+          range.Start = tuple->getElementNameLoc(argIdx);
 
-  // If we are missing a parameter, diagnose that.
-  if (missingParamIdx != ~0U) {
-    Identifier name = params[missingParamIdx].Label;
-    auto loc = argExpr->getStartLoc();
-    if (name.empty())
-      TC.diagnose(loc, diag::missing_argument_positional,
-                  missingParamIdx+1);
-    else
-      TC.diagnose(loc, diag::missing_argument_named, name);
-
-    if (candidate.getDecl())
-      TC.diagnose(candidate.getDecl(), diag::decl_declared_here,
-                  candidate.getDecl()->getFullName());
-    return true;
-  }
+        unsigned paramIdx = argBindings[argIdx];
+        if (Bindings[paramIdx].size() > 1)
+          range.End = tuple->getElement(Bindings[paramIdx].back())->getEndLoc();
 
-  if (extraArgIdx != ~0U) {
-    auto name = args[extraArgIdx].Label;
-    Expr *arg = argExpr;
-    auto tuple = dyn_cast<TupleExpr>(argExpr);
-    if (tuple)
-      arg = tuple->getElement(extraArgIdx);
-    auto loc = arg->getLoc();
-    if (tuple && extraArgIdx == tuple->getNumElements()-1 &&
-        tuple->hasTrailingClosure())
-      TC.diagnose(loc, diag::extra_trailing_closure_in_call)
-        .highlight(arg->getSourceRange());
-    else if (params.empty())
-      TC.diagnose(loc, diag::extra_argument_to_nullary_call)
-        .highlight(argExpr->getSourceRange());
-    else if (name.empty())
-      TC.diagnose(loc, diag::extra_argument_positional)
-        .highlight(arg->getSourceRange());
-    else
-      TC.diagnose(loc, diag::extra_argument_named, name)
-        .highlight(arg->getSourceRange());
-    return true;
-  }
+        return range;
+      };
 
-  // If this is an argument label mismatch, then diagnose that error now.
-  if (!correctNames.empty() &&
-      diagnoseArgumentLabelError(TC, argExpr, correctNames,
-                                 isa<SubscriptExpr>(fnExpr)))
-    return true;
+      auto firstRange = argRange(argIdx, first);
+      auto secondRange = argRange(prevArgIdx, second);
+
+      SourceLoc diagLoc = firstRange.Start;
+
+      if (first.empty() && second.empty()) {
+        TC.diagnose(diagLoc, diag::argument_out_of_order_unnamed_unnamed,
+                    argIdx + 1, prevArgIdx + 1)
+            .fixItExchange(firstRange, secondRange);
+      } else if (first.empty() && !second.empty()) {
+        TC.diagnose(diagLoc, diag::argument_out_of_order_unnamed_named,
+                    argIdx + 1, second)
+            .fixItExchange(firstRange, secondRange);
+      } else if (!first.empty() && second.empty()) {
+        TC.diagnose(diagLoc, diag::argument_out_of_order_named_unnamed, first,
+                    prevArgIdx + 1)
+            .fixItExchange(firstRange, secondRange);
+      } else {
+        TC.diagnose(diagLoc, diag::argument_out_of_order_named_named, first,
+                    second)
+            .fixItExchange(firstRange, secondRange);
+      }
 
-  // If we have an out-of-order argument, diagnose it as such.
-  if (OOOArgIdx != ~0U && isa<TupleExpr>(argExpr)) {
-    auto tuple = cast<TupleExpr>(argExpr);
-    Identifier first = tuple->getElementName(OOOArgIdx);
-    Identifier second = tuple->getElementName(OOOPrevArgIdx);
-
-    // Build a mapping from arguments to parameters.
-    SmallVector<unsigned, 4> argBindings(tuple->getNumElements());
-    for (unsigned paramIdx = 0; paramIdx != paramBindings.size(); ++paramIdx) {
-      for (auto argIdx : paramBindings[paramIdx])
-        argBindings[argIdx] = paramIdx;
-    }
-
-    auto firstRange = tuple->getElement(OOOArgIdx)->getSourceRange();
-    if (!first.empty()) {
-      firstRange.Start = tuple->getElementNameLoc(OOOArgIdx);
-    }
-    unsigned OOOParamIdx = argBindings[OOOArgIdx];
-    if (paramBindings[OOOParamIdx].size() > 1) {
-      firstRange.End =
-        tuple->getElement(paramBindings[OOOParamIdx].back())->getEndLoc();
-    }
-
-    auto secondRange = tuple->getElement(OOOPrevArgIdx)->getSourceRange();
-    if (!second.empty()) {
-      secondRange.Start = tuple->getElementNameLoc(OOOPrevArgIdx);
-    }
-    unsigned OOOPrevParamIdx = argBindings[OOOPrevArgIdx];
-    if (paramBindings[OOOPrevParamIdx].size() > 1) {
-      secondRange.End =
-        tuple->getElement(paramBindings[OOOPrevParamIdx].back())->getEndLoc();
-    }
-
-    SourceLoc diagLoc = firstRange.Start;
-
-    if (first.empty() && second.empty()) {
-      TC.diagnose(diagLoc, diag::argument_out_of_order_unnamed_unnamed,
-                  OOOArgIdx + 1, OOOPrevArgIdx + 1)
-        .fixItExchange(firstRange, secondRange);
-    } else if (first.empty() && !second.empty()) {
-      TC.diagnose(diagLoc, diag::argument_out_of_order_unnamed_named,
-                  OOOArgIdx + 1, second)
-        .fixItExchange(firstRange, secondRange);
-    } else if (!first.empty() && second.empty()) {
-      TC.diagnose(diagLoc, diag::argument_out_of_order_named_unnamed,
-                  first, OOOPrevArgIdx + 1)
-        .fixItExchange(firstRange, secondRange);
-    } else {
-      TC.diagnose(diagLoc, diag::argument_out_of_order_named_named,
-                  first, second)
-        .fixItExchange(firstRange, secondRange);
+      Diagnosed = true;
     }
 
-    return true;
-  }
+    bool relabelArguments(ArrayRef<Identifier> newNames) override {
+      assert (!newNames.empty() && "No arguments were re-labeled");
 
-  return false;
+      // Let's diagnose labeling problem but only related to corrected ones.
+      if (diagnoseArgumentLabelError(TC, 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,
+                         CandidateInfo.hasTrailingClosure,
+                         /*allowFixes:*/ true, *this, Bindings);
+
+      return Diagnosed;
+    }
+  };
+
+  return ArgumentDiagnostic(argExpr, params, args, CCI,
+                            isa<SubscriptExpr>(fnExpr))
+      .diagnose();
 }
 
 /// If the candidate set has been narrowed down to a specific structural

lib/Sema/CSSimplify.cpp

@@ -28,6 +28,8 @@ void MatchCallArgumentListener::extraArgument(unsigned argIdx) { }
 
 void MatchCallArgumentListener::missingArgument(unsigned paramIdx) { }
 
+void MatchCallArgumentListener::missingLabel(unsigned paramIdx) {}
+
 void MatchCallArgumentListener::outOfOrderArgument(unsigned argIdx,
                                                    unsigned prevArgIdx) {
 }
@@ -454,6 +456,27 @@ matchCallArguments(ArrayRef<CallArgParam> args,
       }
 
       unsigned prevArgIdx = parameterBindings[prevParamIdx].front();
+
+      // First let's double check if out-of-order argument is nothing
+      // more than a simple label mismatch, because in situation where
+      // one argument requires label and another one doesn't, but caller
+      // doesn't provide either, problem is going to be identified as
+      // out-of-order argument instead of label mismatch.
+      auto &parameter = params[prevArgIdx];
+      if (parameter.hasLabel()) {
+        auto expectedLabel = parameter.Label;
+        auto argumentLabel = args[argIdx].Label;
+
+        // If there is a label but it's incorrect it can only mean
+        // situation like this: expected (x, _ y) got (y, _ x).
+        if (argumentLabel.empty() ||
+            (expectedLabel.compare(argumentLabel) != 0 &&
+             args[prevArgIdx].Label.empty())) {
+          listener.missingLabel(prevArgIdx);
+          return true;
+        }
+      }
+
       listener.outOfOrderArgument(argIdx, prevArgIdx);
       return true;
     }

lib/Sema/ConstraintSystem.h

@@ -2243,6 +2243,11 @@ class MatchCallArgumentListener {
   /// \param paramIdx The index of the parameter that is missing an argument.
   virtual void missingArgument(unsigned paramIdx);
 
+  /// Indicate that there was no label given when one was expected by parameter.
+  ///
+  /// \param paramIndex The index of the parameter that is missing a label.
+  virtual void missingLabel(unsigned paramIndex);
+
   /// Indicates that an argument is out-of-order with respect to a previously-
   /// seen argument.
   ///

test/Constraints/diagnostics.swift

@@ -713,10 +713,9 @@ func nilComparison(i: Int, o: AnyObject) {
   _ = o !== nil // expected-warning {{comparing non-optional value of type 'AnyObject' to nil always returns true}}
 }
 
-// FIXME: Bad diagnostic
 func secondArgumentNotLabeled(a:Int, _ b: Int) { }
 secondArgumentNotLabeled(10, 20)
-// expected-error@-1 {{unnamed argument #2 must precede unnamed argument #1}}
+// expected-error@-1 {{missing argument label 'a' in call}}
 
 // <rdar://problem/23709100> QoI: incorrect ambiguity error due to implicit conversion
 func testImplConversion(a : Float?) -> Bool {}
@@ -795,3 +794,24 @@ func valueForKey<K>(_ key: K) -> CacheValue? {
   let cache = NSCache<K, CacheValue>()
   return cache.object(forKey: key)?.value // expected-error {{ambiguous reference to member 'value(x:)'}}
 }
+
+// SR-2242: poor diagnostic when argument label is omitted
+
+func r27212391(x: Int, _ y: Int) {
+  let _: Int = x + y
+}
+
+func r27212391(a: Int, x: Int, _ y: Int) {
+  let _: Int = a + x + y
+}
+
+r27212391(3, 5)             // expected-error {{missing argument label 'x' in call}}
+r27212391(3, y: 5)          // expected-error {{missing argument label 'x' in call}}
+r27212391(3, x: 5)          // expected-error {{argument 'x' must precede unnamed argument #1}}
+r27212391(y: 3, x: 5)       // expected-error {{argument 'x' must precede argument 'y'}}
+r27212391(y: 3, 5)          // expected-error {{incorrect argument label in call (have 'y:_:', expected 'x:_:')}}
+r27212391(x: 3, x: 5)       // expected-error {{extraneous argument label 'x:' in call}}
+r27212391(a: 1, 3, y: 5)    // expected-error {{missing argument label 'x' in call}}
+r27212391(1, x: 3, y: 5)    // expected-error {{missing argument label 'a' in call}}
+r27212391(a: 1, y: 3, x: 5) // expected-error {{argument 'x' must precede argument 'y'}}
+r27212391(a: 1, 3, x: 5)    // expected-error {{argument 'x' must precede unnamed argument #2}}