Merge pull request #28013 from xedin/trailing-closures-not-so-trailing

[Diagnostics] Port invalid trailing closure use diagnostics

Author: xedin

lib/Sema/CSDiag.cpp

@@ -2021,32 +2021,6 @@ class ArgumentMatcher : public MatchCallArgumentListener {
     return true;
   }
 
-  bool trailingClosureMismatch(unsigned paramIdx, unsigned argIdx) override {
-    Expr *arg = ArgExpr;
-
-    auto tuple = dyn_cast<TupleExpr>(ArgExpr);
-    if (tuple)
-      arg = tuple->getElement(argIdx);
-
-    if (argIdx >= Parameters.size()) {
-      TC.diagnose(arg->getLoc(), diag::extra_trailing_closure_in_call)
-          .highlight(arg->getSourceRange());
-    } else {
-      auto &param = Parameters[paramIdx];
-      TC.diagnose(arg->getLoc(), diag::trailing_closure_bad_param,
-                  param.getPlainType())
-          .highlight(arg->getSourceRange());
-
-      auto candidate = CandidateInfo[0];
-      if (candidate.getDecl())
-        TC.diagnose(candidate.getDecl(), diag::decl_declared_here,
-                    candidate.getDecl()->getFullName());
-    }
-    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

lib/Sema/CSDiagnostics.cpp

@@ -5491,3 +5491,20 @@ bool ExtraneousCallFailure::diagnoseAsError() {
   removeParensFixIt(diagnostic);
   return true;
 }
+
+bool InvalidUseOfTrailingClosure::diagnoseAsError() {
+  auto *anchor = getAnchor();
+  auto &cs = getConstraintSystem();
+
+  emitDiagnostic(anchor->getLoc(), diag::trailing_closure_bad_param,
+                 getToType())
+      .highlight(anchor->getSourceRange());
+
+  if (auto overload = getChoiceFor(cs.getCalleeLocator(getLocator()))) {
+    if (auto *decl = overload->choice.getDeclOrNull()) {
+      emitDiagnostic(decl, diag::decl_declared_here, decl->getFullName());
+    }
+  }
+
+  return true;
+}

lib/Sema/CSDiagnostics.h

@@ -1921,6 +1921,15 @@ class ExtraneousCallFailure final : public FailureDiagnostic {
   bool diagnoseAsError() override;
 };
 
+class InvalidUseOfTrailingClosure final : public ArgumentMismatchFailure {
+public:
+  InvalidUseOfTrailingClosure(Expr *root, ConstraintSystem &cs, Type argType,
+                              Type paramType, ConstraintLocator *locator)
+      : ArgumentMismatchFailure(root, cs, argType, paramType, locator) {}
+
+  bool diagnoseAsError() override;
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSFix.cpp

@@ -994,3 +994,18 @@ RemoveInvalidCall *RemoveInvalidCall::create(ConstraintSystem &cs,
                                              ConstraintLocator *locator) {
   return new (cs.getAllocator()) RemoveInvalidCall(cs, locator);
 }
+
+bool AllowInvalidUseOfTrailingClosure::diagnose(Expr *expr, bool asNote) const {
+  auto &cs = getConstraintSystem();
+  InvalidUseOfTrailingClosure failure(expr, cs, getFromType(), getToType(),
+                                      getLocator());
+  return failure.diagnose(asNote);
+}
+
+AllowInvalidUseOfTrailingClosure *
+AllowInvalidUseOfTrailingClosure::create(ConstraintSystem &cs, Type argType,
+                                         Type paramType,
+                                         ConstraintLocator *locator) {
+  return new (cs.getAllocator())
+      AllowInvalidUseOfTrailingClosure(cs, argType, paramType, locator);
+}

lib/Sema/CSFix.h

@@ -216,6 +216,8 @@ enum class FixKind : uint8_t {
   /// Remove extraneous call to something which can't be invoked e.g.
   /// a variable, a property etc.
   RemoveCall,
+
+  AllowInvalidUseOfTrailingClosure,
 };
 
 class ConstraintFix {
@@ -1487,6 +1489,24 @@ class RemoveInvalidCall final : public ConstraintFix {
                                    ConstraintLocator *locator);
 };
 
+class AllowInvalidUseOfTrailingClosure final : public AllowArgumentMismatch {
+  AllowInvalidUseOfTrailingClosure(ConstraintSystem &cs, Type argType,
+                                   Type paramType, ConstraintLocator *locator)
+      : AllowArgumentMismatch(cs, FixKind::AllowInvalidUseOfTrailingClosure,
+                              argType, paramType, locator) {}
+
+public:
+  std::string getName() const {
+    return "allow invalid use of trailing closure";
+  }
+
+  bool diagnose(Expr *root, bool asNote = false) const;
+
+  static AllowInvalidUseOfTrailingClosure *create(ConstraintSystem &cs,
+                                                  Type argType, Type paramType,
+                                                  ConstraintLocator *locator);
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSSimplify.cpp

@@ -430,17 +430,56 @@ matchCallArguments(SmallVectorImpl<AnyFunctionType::Param> &args,
 
   // If we have a trailing closure, it maps to the last parameter.
   if (hasTrailingClosure && numParams > 0) {
+    unsigned lastParamIdx = numParams - 1;
+    bool lastAcceptsTrailingClosure =
+        acceptsTrailingClosure(params[lastParamIdx]);
+
+    // If the last parameter is defaulted, this might be
+    // an attempt to use a trailing closure with previous
+    // parameter that accepts a function type e.g.
+    //
+    // func foo(_: () -> Int, _ x: Int = 0) {}
+    // foo { 42 }
+    if (!lastAcceptsTrailingClosure && numParams > 1 &&
+        paramInfo.hasDefaultArgument(lastParamIdx)) {
+      auto paramType = params[lastParamIdx - 1].getPlainType();
+      // If the parameter before defaulted last accepts.
+      if (paramType->is<AnyFunctionType>()) {
+        lastAcceptsTrailingClosure = true;
+        lastParamIdx -= 1;
+      }
+    }
+
+    bool isExtraClosure = false;
     // If there is no suitable last parameter to accept the trailing closure,
     // notify the listener and bail if we need to.
-    if (!acceptsTrailingClosure(params[numParams - 1])) {
-      if (listener.trailingClosureMismatch(numParams - 1, numArgs - 1))
+    if (!lastAcceptsTrailingClosure) {
+      if (numArgs > numParams) {
+        // Argument before the trailing closure.
+        unsigned prevArg = numArgs - 2;
+        auto &arg = args[prevArg];
+        // If the argument before trailing closure matches
+        // last parameter, this is just a special case of
+        // an extraneous argument.
+        const auto param = params[numParams - 1];
+        if (param.hasLabel() && param.getLabel() == arg.getLabel()) {
+          isExtraClosure = true;
+          if (listener.extraArgument(numArgs - 1))
+            return true;
+        }
+      }
+
+      if (!isExtraClosure &&
+          listener.trailingClosureMismatch(lastParamIdx, numArgs - 1))
         return true;
     }
 
     // Claim the parameter/argument pair.
     claimedArgs[numArgs-1] = true;
     ++numClaimedArgs;
-    parameterBindings[numParams-1].push_back(numArgs-1);
+    // Let's claim the trailing closure unless it's an extra argument.
+    if (!isExtraClosure)
+      parameterBindings[lastParamIdx].push_back(numArgs - 1);
   }
 
   // Mark through the parameters, binding them to their arguments.
@@ -923,6 +962,27 @@ class ArgumentFailureTracker : public MatchCallArgumentListener {
     return false;
   }
 
+  bool trailingClosureMismatch(unsigned paramIdx, unsigned argIdx) override {
+    if (!CS.shouldAttemptFixes())
+      return true;
+
+    auto argType = Arguments[argIdx].getPlainType();
+    argType.visit([&](Type type) {
+      if (auto *typeVar = type->getAs<TypeVariableType>())
+        CS.recordHole(typeVar);
+    });
+
+    const auto &param = Parameters[paramIdx];
+
+    auto *argLoc = CS.getConstraintLocator(
+        Locator.withPathElement(LocatorPathElt::ApplyArgToParam(
+            argIdx, paramIdx, param.getParameterFlags())));
+
+    auto *fix = AllowInvalidUseOfTrailingClosure::create(
+        CS, argType, param.getPlainType(), argLoc);
+    return CS.recordFix(fix, /*impact=*/3);
+  }
+
   ArrayRef<std::pair<unsigned, AnyFunctionType::Param>>
   getExtraneousArguments() const {
     return ExtraArguments;
@@ -2696,6 +2756,10 @@ bool ConstraintSystem::repairFailures(
 
   case ConstraintLocator::ApplyArgToParam: {
     auto loc = getConstraintLocator(locator);
+
+    if (hasFixFor(loc, FixKind::AllowInvalidUseOfTrailingClosure))
+      return true;
+
     if (repairByInsertingExplicitCall(lhs, rhs))
       break;
 
@@ -8054,6 +8118,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::GenericArgumentsMismatch:
   case FixKind::AllowMutatingMemberOnRValueBase:
   case FixKind::AllowTupleSplatForSingleParameter:
+  case FixKind::AllowInvalidUseOfTrailingClosure:
     llvm_unreachable("handled elsewhere");
   }
 

lib/Sema/ConstraintSystem.h

@@ -2094,10 +2094,15 @@ class ConstraintSystem {
 
   /// Determine whether constraint system already has a fix recorded
   /// for a particular location.
-  bool hasFixFor(ConstraintLocator *locator) const {
-    return llvm::any_of(Fixes, [&locator](const ConstraintFix *fix) {
-      return fix->getLocator() == locator;
-    });
+  bool hasFixFor(ConstraintLocator *locator,
+                 Optional<FixKind> expectedKind = None) const {
+    return llvm::any_of(
+        Fixes, [&locator, &expectedKind](const ConstraintFix *fix) {
+          if (fix->getLocator() == locator) {
+            return !expectedKind || fix->getKind() == *expectedKind;
+          }
+          return false;
+        });
   }
 
   /// If an UnresolvedDotExpr, SubscriptMember, etc has been resolved by the

test/Constraints/closures.swift

@@ -907,3 +907,10 @@ do {
 // via the 'T -> U => T -> ()' implicit conversion.
 let badResult = { (fn: () -> ()) in fn }
 // expected-error@-1 {{expression resolves to an unused function}}
+
+// rdar://problem/55102498 - closure's result type can't be inferred if the last parameter has a default value
+func test_trailing_closure_with_defaulted_last() {
+  func foo<T>(fn: () -> T, value: Int = 0) {}
+  foo { 42 } // Ok
+  foo(fn: { 42 }) // Ok
+}

test/Constraints/diag_missing_arg.swift

@@ -34,18 +34,28 @@ trailingClosureSingle1 { 1 } // expected-error {{missing argument for parameter
 trailingClosureSingle1() { 1 } // expected-error {{missing argument for parameter 'x' in call}} {{24-24=x: <#Int#>}}
 
 func trailingClosureSingle2(x: () -> Int, y: Int) {} // expected-note * {{here}}
-trailingClosureSingle2 { 1 } // expected-error {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
-trailingClosureSingle2() { 1 } // expected-error {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
+trailingClosureSingle2 { 1 }
+// expected-error@-1 {{missing argument for parameter 'x' in call}} {{23-23=(x: <#() -> Int#>)}}
+// expected-error@-2 {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
+trailingClosureSingle2() { 1 }
+// expected-error@-1 {{missing argument for parameter 'x' in call}} {{24-24=x: <#() -> Int#>}}
+// expected-error@-2 {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
 
 func trailingClosureMulti1(x: Int, y: Int, z: () -> Int) {} // expected-note * {{here}}
 trailingClosureMulti1(y: 1) { 1 } // expected-error {{missing argument for parameter 'x' in call}} {{23-23=x: <#Int#>, }}
 trailingClosureMulti1(x: 1) { 1 } // expected-error {{missing argument for parameter 'y' in call}} {{27-27=, y: <#Int#>}}
 trailingClosureMulti1(x: 1, y: 1) // expected-error {{missing argument for parameter 'z' in call}} {{33-33=, z: <#() -> Int#>}}
 
 func trailingClosureMulti2(x: Int, y: () -> Int, z: Int) {} // expected-note * {{here}}
-trailingClosureMulti2 { 1 } // expected-error {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
-trailingClosureMulti2() { 1 } // expected-error {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
-trailingClosureMulti2(x: 1) { 1 } // expected-error {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
+trailingClosureMulti2 { 1 }
+// expected-error@-1 {{missing arguments for parameters 'x', 'y' in call}}
+// expected-error@-2 {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
+trailingClosureMulti2() { 1 }
+// expected-error@-1 {{missing arguments for parameters 'x', 'y' in call}}
+// expected-error@-2 {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
+trailingClosureMulti2(x: 1) { 1 }
+// expected-error@-1 {{missing argument for parameter 'y' in call}} {{27-27=, y: <#() -> Int#>}}
+// expected-error@-2 {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
 
 func param2Func(x: Int, y: Int) {} // expected-note * {{here}}
 param2Func(x: 1) // expected-error {{missing argument for parameter 'y' in call}} {{16-16=, y: <#Int#>}}

test/Constraints/diagnostics.swift

@@ -700,11 +700,10 @@ let a = safeAssign // expected-error {{generic parameter 'T' could not be inferr
 
 // <rdar://problem/21692808> QoI: Incorrect 'add ()' fixit with trailing closure
 struct Radar21692808<Element> {
-  init(count: Int, value: Element) {}
+  init(count: Int, value: Element) {} // expected-note {{'init(count:value:)' declared here}}
 }
 func radar21692808() -> Radar21692808<Int> {
-  return Radar21692808<Int>(count: 1) { // expected-error {{cannot invoke initializer for type 'Radar21692808<Int>' with an argument list of type '(count: Int, @escaping () -> Int)'}}
-    // expected-note @-1 {{expected an argument list of type '(count: Int, value: Element)'}}
+  return Radar21692808<Int>(count: 1) { // expected-error {{trailing closure passed to parameter of type 'Int' that does not accept a closure}}
     return 1
   }
 }