[CSSimplify] Determine if argument is out-of-order while matching labels

Instead of rerouting out-of-order into re-labeling during diagnostics,
let's do that as part of the label matching algorithm.

Author: xedin

lib/Sema/CSDiag.cpp

@@ -3369,8 +3369,6 @@ class ArgumentMatcher : public MatchCallArgumentListener {
   // Stores parameter bindings determined by call to matchCallArguments.
   SmallVector<ParamBinding, 4> Bindings;
 
-  unsigned NumLabelFailures = 0;
-
 public:
   ArgumentMatcher(Expr *fnExpr, Expr *argExpr,
                   ArrayRef<AnyFunctionType::Param> &params,
@@ -3547,68 +3545,22 @@ class ArgumentMatcher : public MatchCallArgumentListener {
   }
 
   bool missingLabel(unsigned paramIdx) override {
-    ++NumLabelFailures;
     return false;
   }
 
   bool extraneousLabel(unsigned paramIdx) override {
-    ++NumLabelFailures;
     return false;
   }
 
   bool incorrectLabel(unsigned paramIdx) override {
-    ++NumLabelFailures;
     return false;
   }
 
-  void outOfOrderArgument(unsigned argIdx, unsigned prevArgIdx) override {
+  bool outOfOrderArgument(unsigned argIdx, unsigned prevArgIdx) override {
     auto tuple = cast<TupleExpr>(ArgExpr);
     Identifier first = tuple->getElementName(argIdx);
     Identifier second = tuple->getElementName(prevArgIdx);
 
-    // If we've seen label failures and now there is an out-of-order
-    // parameter (or even worse - OoO parameter with label re-naming),
-    // we most likely have no idea what would be the best
-    // diagnostic for this situation, so let's just try to re-label.
-    auto shouldDiagnoseOoO = [&](Identifier newLabel, Identifier oldLabel) {
-      if (NumLabelFailures > 0)
-        return false;
-
-      unsigned actualIndex = prevArgIdx;
-      for (; actualIndex != argIdx; ++actualIndex) {
-        // Looks like new position (excluding defaulted parameters),
-        // has a valid label.
-        if (newLabel == Parameters[actualIndex].getLabel())
-          break;
-
-        // If we are moving the the position with a different label
-        // and there is no default value for it, can't diagnose the
-        // problem as a simple re-ordering.
-        if (!DefaultMap.test(actualIndex))
-          return false;
-      }
-
-      for (unsigned i = actualIndex + 1, n = Parameters.size(); i != n; ++i) {
-        if (oldLabel == Parameters[i].getLabel())
-          break;
-
-        if (!DefaultMap.test(i))
-          return false;
-      }
-
-      return true;
-    };
-
-    if (!shouldDiagnoseOoO(first, second)) {
-      SmallVector<Identifier, 8> paramLabels;
-      llvm::transform(Parameters, std::back_inserter(paramLabels),
-                      [](const AnyFunctionType::Param &param) {
-                        return param.getLabel();
-                      });
-      relabelArguments(paramLabels);
-      return;
-    }
-
     // Build a mapping from arguments to parameters.
     SmallVector<unsigned, 4> argBindings(tuple->getNumElements());
     for (unsigned paramIdx = 0; paramIdx != Bindings.size(); ++paramIdx) {
@@ -3671,6 +3623,7 @@ class ArgumentMatcher : public MatchCallArgumentListener {
     }
 
     Diagnosed = true;
+    return true;
   }
 
   bool relabelArguments(ArrayRef<Identifier> newNames) override {

lib/Sema/CSSimplify.cpp

@@ -44,8 +44,9 @@ bool MatchCallArgumentListener::incorrectLabel(unsigned paramIdx) {
   return true;
 }
 
-void MatchCallArgumentListener::outOfOrderArgument(unsigned argIdx,
+bool MatchCallArgumentListener::outOfOrderArgument(unsigned argIdx,
                                                    unsigned prevArgIdx) {
+  return true;
 }
 
 bool MatchCallArgumentListener::relabelArguments(ArrayRef<Identifier> newNames){
@@ -578,9 +579,47 @@ matchCallArguments(ArrayRef<AnyFunctionType::Param> args,
   // If any arguments were provided out-of-order, check whether we have
   // violated any of the reordering rules.
   if (potentiallyOutOfOrder) {
+    // If we've seen label failures and now there is an out-of-order
+    // parameter (or even worse - OoO parameter with label re-naming),
+    // we most likely have no idea what would be the best
+    // diagnostic for this situation, so let's just try to re-label.
+    auto isOutOfOrderArgument = [&](bool hadLabelMismatch, unsigned argIdx,
+                                    unsigned prevArgIdx) {
+      if (hadLabelMismatch)
+        return false;
+
+      auto newLabel = args[argIdx].getLabel();
+      auto oldLabel = args[prevArgIdx].getLabel();
+
+      unsigned actualIndex = prevArgIdx;
+      for (; actualIndex != argIdx; ++actualIndex) {
+        // Looks like new position (excluding defaulted parameters),
+        // has a valid label.
+        if (newLabel == params[actualIndex].getLabel())
+          break;
+
+        // If we are moving the the position with a different label
+        // and there is no default value for it, can't diagnose the
+        // problem as a simple re-ordering.
+        if (!defaultMap.test(actualIndex))
+          return false;
+      }
+
+      for (unsigned i = actualIndex + 1, n = params.size(); i != n; ++i) {
+        if (oldLabel == params[i].getLabel())
+          break;
+
+        if (!defaultMap.test(i))
+          return false;
+      }
+
+      return true;
+    };
+
     unsigned argIdx = 0;
     // Enumerate the parameters and their bindings to see if any arguments are
     // our of order
+    bool hadLabelMismatch = false;
     for (auto binding : parameterBindings) {
       for (auto boundArgIdx : binding) {
         // We've found the parameter that has an out of order
@@ -611,17 +650,20 @@ matchCallArguments(ArrayRef<AnyFunctionType::Param> args,
           // - The parameter is unnamed, in which case we try to fix the
           //   problem by removing the name.
           if (expectedLabel.empty()) {
+            hadLabelMismatch = true;
             if (listener.extraneousLabel(toArgIdx))
               return true;
           // - The argument is unnamed, in which case we try to fix the
           //   problem by adding the name.
           } else if (argumentLabel.empty()) {
+            hadLabelMismatch = true;
             if (listener.missingLabel(toArgIdx))
               return true;
           // - The argument label has a typo at the same position.
-          } else if (fromArgIdx == toArgIdx &&
-                     listener.incorrectLabel(toArgIdx)) {
-            return true;
+          } else if (fromArgIdx == toArgIdx) {
+            hadLabelMismatch = true;
+            if (listener.incorrectLabel(toArgIdx))
+                return true;
           }
         }
 
@@ -631,8 +673,18 @@ matchCallArguments(ArrayRef<AnyFunctionType::Param> args,
           continue;
         }
 
-        listener.outOfOrderArgument(fromArgIdx, toArgIdx);
-        return true;
+        // This situation looks like out-of-order argument but it's hard
+        // to say exactly without considering other factors, because it
+        // could be invalid labeling too.
+        if (isOutOfOrderArgument(hadLabelMismatch, fromArgIdx, toArgIdx))
+          return listener.outOfOrderArgument(fromArgIdx, toArgIdx);
+
+        SmallVector<Identifier, 8> expectedLabels;
+        llvm::transform(params, std::back_inserter(expectedLabels),
+                        [](const AnyFunctionType::Param &param) {
+                          return param.getLabel();
+                        });
+        return listener.relabelArguments(expectedLabels);
       }
     }
   }

lib/Sema/CalleeCandidateInfo.cpp

@@ -346,8 +346,9 @@ CalleeCandidateInfo::ClosenessResultTy CalleeCandidateInfo::evaluateCloseness(
       result = CC_ArgumentLabelMismatch;
       return true;
     }
-    void outOfOrderArgument(unsigned argIdx, unsigned prevArgIdx) override {
+    bool outOfOrderArgument(unsigned argIdx, unsigned prevArgIdx) override {
       result = CC_ArgumentLabelMismatch;
+      return true;
     }
     bool relabelArguments(ArrayRef<Identifier> newNames) override {
       result = CC_ArgumentLabelMismatch;

lib/Sema/ConstraintSystem.h

@@ -3506,7 +3506,10 @@ class MatchCallArgumentListener {
   ///
   /// \param argIdx The argument that came too late in the argument list.
   /// \param prevArgIdx The argument that the \c argIdx should have preceded.
-  virtual void outOfOrderArgument(unsigned argIdx, unsigned prevArgIdx);
+  ///
+  /// \returns true to indicate that this should cause a failure, false
+  /// otherwise.
+  virtual bool outOfOrderArgument(unsigned argIdx, unsigned prevArgIdx);
 
   /// Indicates that the arguments need to be relabeled to match the parameters.
   ///