Merge pull request #18227 from xedin/diagnose-missing-labels-using-fixes

[ConstraintSystem] Use fixes to diagnose missing/extraneous/incorrect argument labels

Author: xedin

lib/Sema/CSApply.cpp

@@ -7907,21 +7907,43 @@ Expr *ConstraintSystem::coerceToRValue(Expr *expr) {
 /// Emit the fixes computed as part of the solution, returning true if we were
 /// able to emit an error message, or false if none of the fixits worked out.
 bool ConstraintSystem::applySolutionFixes(Expr *E, const Solution &solution) {
-  bool diagnosed = false;
-  for (unsigned i = 0, e = solution.Fixes.size(); i != e; ++i)
-    diagnosed |= applySolutionFix(E, solution, i);
+  llvm::SmallDenseMap<Expr *,
+                      SmallVector<std::pair<Fix, ConstraintLocator *>, 4>>
+      fixesPerExpr;
+
+  for (const auto &fix : solution.Fixes)
+    fixesPerExpr[fix.second->getAnchor()].push_back(fix);
+
+  auto diagnoseExprFailures = [&](Expr *expr) -> bool {
+    auto fixes = fixesPerExpr.find(expr);
+    if (fixes == fixesPerExpr.end())
+      return false;
+
+    bool diagnosed = false;
+    for (auto &fix : fixes->second)
+      diagnosed |= applySolutionFix(expr, solution, fix);
+    return diagnosed;
+  };
 
+  bool diagnosed = false;
+  E->forEachChildExpr([&](Expr *subExpr) -> Expr * {
+    // Diagnose root expression at the end to
+    // preserve ordering.
+    if (subExpr != E)
+      diagnosed |= diagnoseExprFailures(subExpr);
+    return subExpr;
+  });
+
+  diagnosed |= diagnoseExprFailures(E);
   return diagnosed;
 }
 
-/// \brief Apply the specified Fix # to this solution, producing a fixit hint
-/// diagnostic for it and returning true.  If the fixit hint turned out to be
+/// \brief Apply the specified Fix to this solution, producing a fix-it hint
+/// diagnostic for it and returning true.  If the fix-it hint turned out to be
 /// bogus, this returns false and doesn't emit anything.
-bool ConstraintSystem::applySolutionFix(Expr *expr,
-                                        const Solution &solution,
-                                        unsigned fixNo) {
-  auto &fix = solution.Fixes[fixNo];
-  
+bool ConstraintSystem::applySolutionFix(
+    Expr *expr, const Solution &solution,
+    std::pair<Fix, ConstraintLocator *> &fix) {
   // Some fixes need more information from the locator.
   ConstraintLocator *locator = fix.second;
 
@@ -8098,6 +8120,13 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
                 getASTContext().TheAnyType);
     return true;
   }
+
+  case FixKind::RelabelArguments: {
+    auto *call = cast<CallExpr>(locator->getAnchor());
+    return diagnoseArgumentLabelError(getASTContext(), call->getArg(),
+                                      fix.first.getArgumentLabels(*this),
+                                      isa<SubscriptExpr>(call->getFn()));
+  }
   }
 
   // FIXME: It would be really nice to emit a follow-up note showing where

lib/Sema/CSSimplify.cpp

@@ -499,6 +499,15 @@ matchCallArguments(ArrayRef<AnyFunctionType::Param> args,
         auto fromArgIdx = boundArgIdx;
         auto toArgIdx = argIdx;
 
+        // If there is no re-ordering going on, and index is past
+        // the number of parameters, it could only mean that this
+        // is variadic parameter, so let's just move on.
+        if (fromArgIdx == toArgIdx && toArgIdx >= params.size()) {
+          assert(args[fromArgIdx].getLabel().empty());
+          argIdx++;
+          continue;
+        }
+
         // 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
@@ -692,6 +701,40 @@ getCalleeDeclAndArgs(ConstraintSystem &cs,
   return std::make_tuple(nullptr, 0, argLabels, hasTrailingClosure);
 }
 
+class ArgumentFailureTracker : public MatchCallArgumentListener {
+  ConstraintSystem &CS;
+  ConstraintLocatorBuilder Locator;
+
+public:
+  ArgumentFailureTracker(ConstraintSystem &cs, ConstraintLocatorBuilder locator)
+    : CS(cs), Locator(locator) {}
+
+  bool missingLabel(unsigned paramIndex) override {
+    return !CS.shouldAttemptFixes();
+  }
+
+  bool extraneousLabel(unsigned paramIndex) override {
+    return !CS.shouldAttemptFixes();
+  }
+
+  bool incorrectLabel(unsigned paramIndex) override {
+    return !CS.shouldAttemptFixes();
+  }
+
+  bool relabelArguments(ArrayRef<Identifier> newLabels) override {
+    if (!CS.shouldAttemptFixes())
+      return true;
+
+    auto *anchor = Locator.getBaseLocator()->getAnchor();
+    if (!anchor || !isa<CallExpr>(anchor))
+      return true;
+
+    CS.recordFix(Fix::fixArgumentLabels(CS, newLabels),
+                 CS.getConstraintLocator(anchor));
+    return false;
+  }
+};
+
 // Match the argument of a call to the parameter.
 static ConstraintSystem::TypeMatchResult
 matchCallArguments(ConstraintSystem &cs, ConstraintKind kind,
@@ -747,7 +790,7 @@ matchCallArguments(ConstraintSystem &cs, ConstraintKind kind,
   auto args = decomposeArgType(argType, argLabels);
 
   // Match up the call arguments to the parameters.
-  MatchCallArgumentListener listener;
+  ArgumentFailureTracker listener(cs, locator);
   SmallVector<ParamBinding, 4> parameterBindings;
   if (constraints::matchCallArguments(args, params,
                                       defaultMap,
@@ -4910,6 +4953,7 @@ ConstraintSystem::simplifyFixConstraint(Fix fix, Type type1, Type type2,
   case FixKind::ExplicitlyEscaping:
   case FixKind::ExplicitlyEscapingToAny:
   case FixKind::CoerceToCheckedCast:
+  case FixKind::RelabelArguments:
     llvm_unreachable("handled elsewhere");
   }
 

lib/Sema/Constraint.cpp

@@ -493,6 +493,13 @@ Fix Fix::getUnwrapOptionalBase(ConstraintSystem &cs, DeclName memberName) {
   return Fix(FixKind::UnwrapOptionalBase, index);
 }
 
+Fix Fix::fixArgumentLabels(ConstraintSystem &cs,
+                           ArrayRef<Identifier> newLabels) {
+  unsigned index = cs.FixedArgLabels.size();
+  cs.FixedArgLabels.push_back(newLabels);
+  return Fix(FixKind::RelabelArguments, index);
+}
+
 Type Fix::getTypeArgument(ConstraintSystem &cs) const {
   assert(getKind() == FixKind::ForceDowncast);
   return cs.FixedTypes[Data];
@@ -504,6 +511,11 @@ DeclName Fix::getDeclNameArgument(ConstraintSystem &cs) const {
   return cs.FixedDeclNames[Data];
 }
 
+ArrayRef<Identifier> Fix::getArgumentLabels(ConstraintSystem &cs) const {
+  assert(getKind() == FixKind::RelabelArguments);
+  return cs.FixedArgLabels[Data];
+}
+
 StringRef Fix::getName(FixKind kind) {
   switch (kind) {
   case FixKind::ForceOptional:
@@ -519,6 +531,8 @@ StringRef Fix::getName(FixKind kind) {
   case FixKind::ExplicitlyEscaping:
   case FixKind::ExplicitlyEscapingToAny:
     return "fix: add @escaping";
+  case FixKind::RelabelArguments:
+    return "fix: re-label argument(s)";
   }
 
   llvm_unreachable("Unhandled FixKind in switch.");

lib/Sema/Constraint.h

@@ -249,6 +249,10 @@ enum class FixKind : uint8_t {
   ExplicitlyEscaping,
   /// Mark function type as explicitly '@escaping' to be convertable to 'Any'.
   ExplicitlyEscapingToAny,
+
+  /// Arguments have labeling failures - missing/extraneous or incorrect
+  /// labels attached to the, fix it by suggesting proper labels.
+  RelabelArguments,
 };
 
 /// Describes a fix that can be applied to a constraint before visiting it.
@@ -276,6 +280,11 @@ class Fix {
   /// with the given name.
   static Fix getUnwrapOptionalBase(ConstraintSystem &cs, DeclName memberName);
 
+  /// Produce a new fix that re-labels existing arguments so they much
+  /// what parameters expect.
+  static Fix fixArgumentLabels(ConstraintSystem &cs,
+                               ArrayRef<Identifier> newLabels);
+
   /// Retrieve the kind of fix.
   FixKind getKind() const { return Kind; }
 
@@ -285,6 +294,9 @@ class Fix {
   /// If this fix has a name argument, retrieve it.
   DeclName getDeclNameArgument(ConstraintSystem &cs) const;
 
+  /// If this fix is an argument re-labeling, retrieve new labels.
+  ArrayRef<Identifier> getArgumentLabels(ConstraintSystem &cs) const;
+
   /// Return a string representation of a fix.
   static llvm::StringRef getName(FixKind kind);
 

lib/Sema/ConstraintSystem.h

@@ -1008,6 +1008,9 @@ class ConstraintSystem {
   /// Declaration names used in fixes.
   std::vector<DeclName> FixedDeclNames;
 
+  /// Argument labels fixed by the constraint solver.
+  SmallVector<std::vector<Identifier>, 4> FixedArgLabels;
+
   /// \brief The set of remembered disjunction choices used to reach
   /// the current constraint system.
   SmallVector<std::pair<ConstraintLocator*, unsigned>, 32>
@@ -1515,10 +1518,11 @@ class ConstraintSystem {
   /// able to emit an error message, or false if none of the fixits worked out.
   bool applySolutionFixes(Expr *E, const Solution &solution);
 
-  /// \brief Apply the specified Fix # to this solution, producing a fixit hint
-  /// diagnostic for it and returning true.  If the fixit hint turned out to be
+  /// \brief Apply the specified Fix to this solution, producing a fix-it hint
+  /// diagnostic for it and returning true.  If the fix-it hint turned out to be
   /// bogus, this returns false and doesn't emit anything.
-  bool applySolutionFix(Expr *expr, const Solution &solution, unsigned fixNo);
+  bool applySolutionFix(Expr *expr, const Solution &solution,
+                        std::pair<Fix, ConstraintLocator *> &fix);
 
   /// \brief If there is more than one viable solution,
   /// attempt to pick the best solution and remove all of the rest.

test/ClangImporter/objc_factory_method.swift

@@ -22,8 +22,7 @@ func testInstanceTypeFactoryMethodInherited() {
   _ = NSObjectFactorySub() // okay, prefers init method
   _ = NSObjectFactorySub(integer: 1)
   _ = NSObjectFactorySub(double: 314159)
-  _ = NSObjectFactorySub(float: 314159) // expected-error{{argument labels '(float:)' do not match any available overloads}} 
-  // expected-note @-1 {{overloads for 'NSObjectFactorySub' exist with these partially matching parameter lists: (integer: Int), (double: Double)}}
+  _ = NSObjectFactorySub(float: 314159) // expected-error{{incorrect argument label in call (have 'float:', expected 'integer:')}}
   let a = NSObjectFactorySub(buildingWidgets: ()) // expected-error{{argument labels '(buildingWidgets:)' do not match any available overloads}}
   // expected-note @-1 {{overloads for 'NSObjectFactorySub' exist with these partially matching parameter lists: (integer: Int), (double: Double)}}
   _ = a

test/ClangImporter/objc_implicit_with.swift

@@ -33,8 +33,7 @@ func testInstanceTypeFactoryMethodInherited() {
   _ = NSObjectFactorySub() // okay, prefers init method
   _ = NSObjectFactorySub(integer: 1)
   _ = NSObjectFactorySub(double: 314159)
-  _ = NSObjectFactorySub(float: 314159) // expected-error{{argument labels '(float:)' do not match any available overloads}} 
-  // expected-note @-1 {{overloads for 'NSObjectFactorySub' exist with these partially matching parameter lists: (integer: Int), (double: Double)}}
+  _ = NSObjectFactorySub(float: 314159) // expected-error{{incorrect argument label in call (have 'float:', expected 'integer:')}}
   let a = NSObjectFactorySub(buildingWidgets: ()) // expected-error{{argument labels '(buildingWidgets:)' do not match any available overloads}}
   // expected-note @-1 {{overloads for 'NSObjectFactorySub' exist with these partially matching parameter lists: (integer: Int), (double: Double)}}
   _ = a

test/Constraints/diagnostics_swift4.swift

@@ -24,8 +24,7 @@ extension C_2505 {
 class C2_2505: P_2505 {
 }
 
-let c_2505 = C_2505(arg: [C2_2505()]) // expected-error {{argument labels '(arg:)' do not match any available overloads}}
-// expected-note@-1 {{overloads for 'C_2505' exist with these partially matching parameter lists: (Any), (from: [T])}}
+let c_2505 = C_2505(arg: [C2_2505()]) // expected-error {{incorrect argument label in call (have 'arg:', expected 'from:')}}
 
 // rdar://problem/31898542 - Swift 4: 'type of expression is ambiguous without more context' errors, without a fixit
 

test/Constraints/dynamic_lookup.swift

@@ -338,6 +338,5 @@ func testOverloadedWithUnavailable(ao: AnyObject) {
 
 func dynamicInitCrash(ao: AnyObject.Type) {
   let sdk = ao.init(blahblah: ())
-  // expected-error@-1 {{argument labels '(blahblah:)' do not match any available overloads}}
-  // expected-note@-2 {{overloads for 'AnyObject.Type.init' exist with these partially matching parameter lists}}
+  // expected-error@-1 {{incorrect argument label in call (have 'blahblah:', expected 'toMemory:')}}
 }

test/Constraints/keyword_arguments.swift

@@ -8,7 +8,9 @@ struct X1 {
   init(_ a: Int) { }
   func f1(_ a: Int) {}
 }
-X1(a: 5).f1(b: 5) // expected-error{{extraneous argument label 'a:' in call}}{{4-7=}}
+X1(a: 5).f1(b: 5) 
+// expected-error@-1 {{extraneous argument label 'a:' in call}} {{4-7=}}
+// expected-error@-2 {{extraneous argument label 'b:' in call}} {{13-16=}}
 
 // <rdar://problem/16801056>
 enum Policy {
@@ -33,7 +35,9 @@ struct X2 {
   init(a: Int) { }
   func f2(b: Int) { }
 }
-X2(5).f2(5) // expected-error{{missing argument label 'a:' in call}}{{4-4=a: }}
+X2(5).f2(5)
+// expected-error@-1 {{missing argument label 'a:' in call}} {{4-4=a: }}
+// expected-error@-2 {{missing argument label 'b:' in call}} {{10-10=b: }}
 
 
 // -------------------------------------------
@@ -401,3 +405,8 @@ _ = acceptTuple2(tuple1)
 _ = acceptTuple2((1, "hello", 3.14159))
 
 
+func generic_and_missing_label(x: Int) {}
+func generic_and_missing_label<T>(x: T) {}
+
+generic_and_missing_label(42)
+// expected-error@-1 {{missing argument label 'x:' in call}} {{27-27=x: }}

test/Constraints/optional.swift

@@ -209,7 +209,7 @@ func sr2752(x: String?, y: String?) {
 var sr3248 : ((Int) -> ())!
 sr3248?(a: 2) // expected-error {{extraneous argument label 'a:' in call}}
 sr3248!(a: 2) // expected-error {{extraneous argument label 'a:' in call}}
-sr3248(a: 2)  // expected-error {{cannot call value of non-function type '((Int) -> ())?'}}
+sr3248(a: 2)  // expected-error {{extraneous argument label 'a:' in call}}
 
 struct SR_3248 {
     var callback: (([AnyObject]) -> Void)!

test/IDE/complete_unresolved_members.swift

@@ -207,7 +207,7 @@ var a = {() in
   OptionSetTaker5([.#^UNRESOLVED_18^#], .Option4, .South, .West)
 }
 var Container = OptionTakerContainer1()
-Container.OptionSetTaker1(.#^UNRESOLVED_19^#
+Container.OptionSetTaker1(.#^UNRESOLVED_19^#)
 Container.EnumTaker1(.#^UNRESOLVED_20^#
 
 func parserSync() {}

test/decl/inherit/initializer.swift

@@ -58,8 +58,7 @@ class NotInherited1 : D {
 
 func testNotInherited1() {
   var n1 = NotInherited1(int: 5)
-  var n2 = NotInherited1(double: 2.71828) // expected-error{{argument labels '(double:)' do not match any available overloads}}
-  // expected-note @-1 {{overloads for 'NotInherited1' exist with these partially matching parameter lists: (int: Int), (float: Float)}}
+  var n2 = NotInherited1(double: 2.71828) // expected-error{{incorrect argument label in call (have 'double:', expected 'float:')}}
 }
 
 class NotInherited1Sub : NotInherited1 {
@@ -71,8 +70,7 @@ class NotInherited1Sub : NotInherited1 {
 func testNotInherited1Sub() {
   var n1 = NotInherited1Sub(int: 5)
   var n2 = NotInherited1Sub(float: 3.14159)
-  var n3 = NotInherited1Sub(double: 2.71828) // expected-error{{argument labels '(double:)' do not match any available overloads}}
-  // expected-note @-1 {{overloads for 'NotInherited1Sub' exist with these partially matching parameter lists: (int: Int), (float: Float)}}
+  var n3 = NotInherited1Sub(double: 2.71828) // expected-error{{incorrect argument label in call (have 'double:', expected 'float:')}}
 }
 
 // Having a stored property without an initial value prevents

test/expr/postfix/dot/init_ref_delegation.swift

@@ -294,8 +294,7 @@ func foo<T: C>(_ x: T, y: T.Type) where T: P {
   var cs2 = T.init(x: 0) // expected-error{{'required' initializer}}
   var cs3 = T.init() // expected-error{{'required' initializer}}
   var cs4 = T.init(proto: "")
-  var cs5 = T.init(notfound: "") // expected-error{{argument labels '(notfound:)' do not match any available overloads}}
-  // expected-note @-1 {{overloads for 'T.Type.init' exist with these partially matching parameter lists: (x: Int), (required: Double), (proto: String)}}
+  var cs5 = T.init(notfound: "") // expected-error{{incorrect argument label in call (have 'notfound:', expected 'proto:')}}
 
   var csf1: (Double) -> T = T.init
   var csf2: (Int) -> T    = T.init // expected-error{{'required' initializer}}

validation-test/stdlib/FixedPointDiagnostics.swift.gyb

@@ -35,22 +35,19 @@ func test_truncatingBitPatternAPIIsStableAcrossPlatforms() {
   _ = UInt8(truncatingBitPattern: UInt(0))
   _ = UInt16(truncatingBitPattern: UInt(0))
   _ = UInt32(truncatingBitPattern: UInt(0))
-  UInt64(truncatingBitPattern: UInt(0)) // expected-error {{argument labels '(truncatingBitPattern:)' do not match any available overloads}}
-// expected-note @-1 {{overloads for 'UInt64' exist with these partially matching parameter lists}}
+  UInt64(truncatingBitPattern: UInt(0)) // expected-error {{extraneous argument label 'truncatingBitPattern:' in call}}
   UInt(truncatingBitPattern: UInt(0))   // expected-error {{}} expected-note * {{}}
 
   _ = Int8(truncatingBitPattern: UInt(0))
   _ = Int16(truncatingBitPattern: UInt(0))
   _ = Int32(truncatingBitPattern: UInt(0))
-  Int64(truncatingBitPattern: UInt(0)) // expected-error {{argument labels '(truncatingBitPattern:)' do not match any available overloads}}
-// expected-note @-1 {{overloads for 'Int64' exist with}}
+  Int64(truncatingBitPattern: UInt(0)) // expected-error {{extraneous argument label 'truncatingBitPattern:' in call}}
   Int(truncatingBitPattern: UInt(0))   // expected-error {{}} expected-note * {{}}
 
   _ = UInt8(truncatingBitPattern: Int(0))
   _ = UInt16(truncatingBitPattern: Int(0))
   _ = UInt32(truncatingBitPattern: Int(0))
-  UInt64(truncatingBitPattern: Int(0)) // expected-error {{argument labels '(truncatingBitPattern:)' do not match any available overloads}}
-// expected-note @-1 {{overloads for 'UInt64' exist with these partially matching parameter lists}}
+  UInt64(truncatingBitPattern: Int(0)) // expected-error {{extraneous argument label 'truncatingBitPattern:' in call}}
   UInt(truncatingBitPattern: Int(0))   // expected-error {{}} expected-note * {{}}
 
   _ = Int8(truncatingBitPattern: Int(0))