[Type checker] Improve diagnostics for trailing closures.

The change to the forward-scanning rule regressed some diagnostics,
because we no longer generated the special "trailing closure mismatch"
diagnostic. Reinstate the special-case "trailing closure mismatch"
diagnostic, but this time do so as part of the normal argument
mismatch diagnostics so it is based on type information.

While here, clean up the handling of missing-argument diagnostics to
deal with (multiple) trailing closures properly, so that we can (e.g)
suggest adding a new labeled trailing closure at the end, rather than
producing nonsensical Fix-Its.

And, note that SR-12291 is broken (again) by the forward-scan matching
rules.

Author: DougGregor

lib/Sema/CSDiagnostics.cpp

@@ -3915,9 +3915,9 @@ bool MissingArgumentsFailure::diagnoseAsError() {
   Expr *fnExpr = nullptr;
   Expr *argExpr = nullptr;
   unsigned numArguments = 0;
-  bool hasTrailingClosure = false;
+  Optional<unsigned> firstTrailingClosure = None;
 
-  std::tie(fnExpr, argExpr, numArguments, hasTrailingClosure) =
+  std::tie(fnExpr, argExpr, numArguments, firstTrailingClosure) =
       getCallInfo(getRawAnchor());
 
   // TODO(diagnostics): We should be able to suggest this fix-it
@@ -3980,46 +3980,66 @@ bool MissingArgumentsFailure::diagnoseSingleMissingArgument() const {
   auto position = argument.first;
   auto label = argument.second.getLabel();
 
-  SmallString<32> insertBuf;
-  llvm::raw_svector_ostream insertText(insertBuf);
-
-  if (position != 0)
-    insertText << ", ";
-
-  forFixIt(insertText, argument.second);
-
   Expr *fnExpr = nullptr;
   Expr *argExpr = nullptr;
-  unsigned insertableEndIdx = 0;
-  bool hasTrailingClosure = false;
+  unsigned numArgs = 0;
+  Optional<unsigned> firstTrailingClosure = None;
 
-  std::tie(fnExpr, argExpr, insertableEndIdx, hasTrailingClosure) =
+  std::tie(fnExpr, argExpr, numArgs, firstTrailingClosure) =
       getCallInfo(anchor);
 
-  if (!argExpr)
+  if (!argExpr) {
     return false;
+  }
+
+  // Will the parameter accept a trailing closure?
+  Type paramType = resolveType(argument.second.getPlainType());
+  bool paramAcceptsTrailingClosure = paramType
+      ->lookThroughAllOptionalTypes()->is<AnyFunctionType>();
 
-  if (hasTrailingClosure)
-    insertableEndIdx -= 1;
+  // Determine whether we're inserting as a trailing closure.
+  bool insertingTrailingClosure =
+    firstTrailingClosure && position > *firstTrailingClosure;
+
+  SmallString<32> insertBuf;
+  llvm::raw_svector_ostream insertText(insertBuf);
 
-  if (position == 0 && insertableEndIdx != 0)
+  if (insertingTrailingClosure)
+    insertText << " ";
+  else if (position != 0)
+    insertText << ", ";
+
+  forFixIt(insertText, argument.second);
+
+  if (position == 0 && numArgs > 0 &&
+      (!firstTrailingClosure || position < *firstTrailingClosure))
     insertText << ", ";
 
   SourceLoc insertLoc;
-  if (auto *TE = dyn_cast<TupleExpr>(argExpr)) {
-    // fn():
-    //   fn([argMissing])
+
+  if (position >= numArgs && insertingTrailingClosure) {
+    // Add a trailing closure to the end.
+
+    // fn { closure }:
+    //   fn {closure} label: [argMissing]
+    // fn() { closure }:
+    //   fn() {closure} label: [argMissing]
+    // fn(argX) { closure }:
+    //   fn(argX) { closure } label: [argMissing]
+    insertLoc = Lexer::getLocForEndOfToken(
+        ctx.SourceMgr, argExpr->getEndLoc());
+  } else if (auto *TE = dyn_cast<TupleExpr>(argExpr)) {
     // fn(argX, argY):
     //   fn([argMissing, ]argX, argY)
     //   fn(argX[, argMissing], argY)
-    //   fn(argX, argY[, argMissing])
     // fn(argX) { closure }:
     //   fn([argMissing, ]argX) { closure }
     //   fn(argX[, argMissing]) { closure }
-    //   fn(argX[, closureLabel: ]{closure}[, argMissing)] // Not impl.
-    if (insertableEndIdx == 0)
+    // fn(argX, argY):
+    //   fn(argX, argY[, argMissing])
+    if (numArgs == 0) {
       insertLoc = TE->getRParenLoc();
-    else if (position != 0) {
+    } else if (position != 0) {
       auto argPos = std::min(TE->getNumElements(), position) - 1;
       insertLoc = Lexer::getLocForEndOfToken(
           ctx.SourceMgr, TE->getElement(argPos)->getEndLoc());
@@ -4031,25 +4051,25 @@ bool MissingArgumentsFailure::diagnoseSingleMissingArgument() const {
   } else {
     auto *PE = cast<ParenExpr>(argExpr);
     if (PE->getRParenLoc().isValid()) {
+      // fn():
+      //   fn([argMissing])
       // fn(argX):
-      //   fn([argMissing, ]argX)
       //   fn(argX[, argMissing])
+      //   fn([argMissing, ]argX)
       // fn() { closure }:
       //   fn([argMissing]) {closure}
-      //   fn([closureLabel: ]{closure}[, argMissing]) // Not impl.
-      if (insertableEndIdx == 0)
-        insertLoc = PE->getRParenLoc();
-      else if (position == 0)
-        insertLoc = PE->getSubExpr()->getStartLoc();
-      else
+      if (position == 0) {
         insertLoc = Lexer::getLocForEndOfToken(ctx.SourceMgr,
-                                               PE->getSubExpr()->getEndLoc());
+                                               PE->getLParenLoc());
+      } else {
+        insertLoc = Lexer::getLocForEndOfToken(
+            ctx.SourceMgr, PE->getSubExpr()->getEndLoc());
+      }
     } else {
       // fn { closure }:
       //   fn[(argMissing)] { closure }
-      //   fn[(closureLabel:] { closure }[, missingArg)]  // Not impl.
       assert(!isExpr<SubscriptExpr>(anchor) && "bracket less subscript");
-      assert(PE->hasTrailingClosure() &&
+      assert(firstTrailingClosure &&
              "paren less ParenExpr without trailing closure");
       insertBuf.insert(insertBuf.begin(), '(');
       insertBuf.insert(insertBuf.end(), ')');
@@ -4061,16 +4081,28 @@ bool MissingArgumentsFailure::diagnoseSingleMissingArgument() const {
   if (insertLoc.isInvalid())
     return false;
 
+  // If we are trying to insert a trailing closure but the parameter
+  // corresponding to the missing argument doesn't support a trailing closure,
+  // don't provide a Fix-It.
+  // FIXME: It's possible to parenthesize and relabel the argument list to
+  // accomodate this, but it's tricky.
+  bool shouldEmitFixIt =
+    !(insertingTrailingClosure && !paramAcceptsTrailingClosure);
+
   if (label.empty()) {
-    emitDiagnosticAt(insertLoc, diag::missing_argument_positional, position + 1)
-        .fixItInsert(insertLoc, insertText.str());
+    auto diag = emitDiagnosticAt(
+        insertLoc, diag::missing_argument_positional, position + 1);
+    if (shouldEmitFixIt)
+      diag.fixItInsert(insertLoc, insertText.str());
   } else if (isPropertyWrapperInitialization()) {
     auto *TE = cast<TypeExpr>(fnExpr);
     emitDiagnosticAt(TE->getLoc(), diag::property_wrapper_missing_arg_init,
                      label, resolveType(TE->getInstanceType())->getString());
   } else {
-    emitDiagnosticAt(insertLoc, diag::missing_argument_named, label)
-        .fixItInsert(insertLoc, insertText.str());
+    auto diag = emitDiagnosticAt(
+        insertLoc, diag::missing_argument_named, label);
+    if (shouldEmitFixIt)
+      diag.fixItInsert(insertLoc, insertText.str());
   }
 
   if (auto selectedOverload =
@@ -4298,23 +4330,24 @@ bool MissingArgumentsFailure::isMisplacedMissingArgument(
   return TypeChecker::isConvertibleTo(argType, paramType, solution.getDC());
 }
 
-std::tuple<Expr *, Expr *, unsigned, bool>
+std::tuple<Expr *, Expr *, unsigned, Optional<unsigned>>
 MissingArgumentsFailure::getCallInfo(ASTNode anchor) const {
   if (auto *call = getAsExpr<CallExpr>(anchor)) {
     return std::make_tuple(call->getFn(), call->getArg(),
-                           call->getNumArguments(), call->hasTrailingClosure());
+                           call->getNumArguments(),
+                           call->getUnlabeledTrailingClosureIndex());
   } else if (auto *UME = getAsExpr<UnresolvedMemberExpr>(anchor)) {
     return std::make_tuple(UME, UME->getArgument(), UME->getNumArguments(),
-                           UME->hasTrailingClosure());
+                           UME->getUnlabeledTrailingClosureIndex());
   } else if (auto *SE = getAsExpr<SubscriptExpr>(anchor)) {
     return std::make_tuple(SE, SE->getIndex(), SE->getNumArguments(),
-                           SE->hasTrailingClosure());
+                           SE->getUnlabeledTrailingClosureIndex());
   } else if (auto *OLE = getAsExpr<ObjectLiteralExpr>(anchor)) {
     return std::make_tuple(OLE, OLE->getArg(), OLE->getNumArguments(),
-                           OLE->hasTrailingClosure());
+                           OLE->getUnlabeledTrailingClosureIndex());
   }
 
-  return std::make_tuple(nullptr, nullptr, 0, false);
+  return std::make_tuple(nullptr, nullptr, 0, None);
 }
 
 void MissingArgumentsFailure::forFixIt(
@@ -5509,6 +5542,9 @@ bool ArgumentMismatchFailure::diagnoseAsError() {
   if (diagnosePropertyWrapperMismatch())
     return true;
 
+  if (diagnoseTrailingClosureMismatch())
+    return true;
+
   auto argType = getFromType();
   auto paramType = getToType();
 
@@ -5743,6 +5779,26 @@ bool ArgumentMismatchFailure::diagnosePropertyWrapperMismatch() const {
   return true;
 }
 
+bool ArgumentMismatchFailure::diagnoseTrailingClosureMismatch() const {
+  if (!Info.isTrailingClosure())
+    return false;
+
+  auto paramType = getToType();
+  if (paramType->lookThroughAllOptionalTypes()->is<AnyFunctionType>())
+    return false;
+
+  emitDiagnostic(diag::trailing_closure_bad_param, paramType)
+      .highlight(getSourceRange());
+
+  if (auto overload = getCalleeOverloadChoiceIfAvailable(getLocator())) {
+    if (auto *decl = overload->choice.getDeclOrNull()) {
+      emitDiagnosticAt(decl, diag::decl_declared_here, decl->getName());
+    }
+  }
+
+  return true;
+}
+
 void ExpandArrayIntoVarargsFailure::tryDropArrayBracketsFixIt(
     const Expr *anchor) const {
   // If this is an array literal, offer to remove the brackets and pass the

lib/Sema/CSDiagnostics.h

@@ -1293,9 +1293,10 @@ class MissingArgumentsFailure final : public FailureDiagnostic {
   bool isPropertyWrapperInitialization() const;
 
   /// Gather information associated with expression that represents
-  /// a call - function, arguments, # of arguments and whether it has
-  /// a trailing closure.
-  std::tuple<Expr *, Expr *, unsigned, bool> getCallInfo(ASTNode anchor) const;
+  /// a call - function, arguments, # of arguments and the position of
+  /// the first trailing closure.
+  std::tuple<Expr *, Expr *, unsigned, Optional<unsigned>>
+      getCallInfo(ASTNode anchor) const;
 
   /// Transform given argument into format suitable for a fix-it
   /// text e.g. `[<label>:]? <#<type#>`
@@ -1815,6 +1816,10 @@ class ArgumentMismatchFailure : public ContextualFailure {
   /// or now deprecated `init(initialValue:)`.
   bool diagnosePropertyWrapperMismatch() const;
 
+  /// Tailored diagnostics for argument mismatches associated with trailing
+  /// closures being passed to non-closure parameters.
+  bool diagnoseTrailingClosureMismatch() const;
+
 protected:
   /// \returns The position of the argument being diagnosed, starting at 1.
   unsigned getArgPosition() const { return Info.getArgPosition(); }

lib/Sema/CSSimplify.cpp

@@ -421,7 +421,8 @@ matchCallArguments(SmallVectorImpl<AnyFunctionType::Param> &args,
 
       // If the parameter we are looking at does not support the (unlabeled)
       // trailing closure argument, this parameter is unfulfilled.
-      if (!paramInfo.acceptsUnlabeledTrailingClosureArgument(paramIdx)) {
+      if (!paramInfo.acceptsUnlabeledTrailingClosureArgument(paramIdx) &&
+          !ignoreNameMismatch) {
         haveUnfulfilledParams = true;
         return;
       }
@@ -739,7 +740,10 @@ matchCallArguments(SmallVectorImpl<AnyFunctionType::Param> &args,
         // 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.
-        const auto expectedLabel = params[paramIdx].getLabel();
+        const auto expectedLabel =
+          fromArgIdx == unlabeledTrailingClosureArgIndex
+            ? Identifier()
+            : params[paramIdx].getLabel();
         const auto argumentLabel = args[fromArgIdx].getLabel();
 
         if (argumentLabel != expectedLabel) {

lib/Sema/ConstraintSystem.h

@@ -595,6 +595,15 @@ class FunctionArgApplyInfo {
     return StringRef(scratch.data(), scratch.size());
   }
 
+  /// Whether the argument is a trailing closure.
+  bool isTrailingClosure() const {
+    if (auto trailingClosureArg =
+            ArgListExpr->getUnlabeledTrailingClosureIndexOfPackedArgument())
+      return ArgIdx >= *trailingClosureArg;
+
+    return false;
+  }
+
   /// \returns The interface type for the function being applied. Note that this
   /// may not a function type, for example it could be a generic parameter.
   Type getFnInterfaceType() const { return FnInterfaceType; }

test/Constraints/argument_matching.swift

@@ -1418,7 +1418,7 @@ struct RelabelAndTrailingClosure {
   func f2(aa: Int, bb: Int, _ cc: () -> Void = {}) {}
 
   func test() {
-    f1(aax: 1, bbx: 2) {} // expected-error {{incorrect argument labels in call (have 'aax:bbx:_:', expected 'aa:bb:cc:')}} {{8-11=aa}} {{16-19=bb}} {{none}}
+    f1(aax: 1, bbx: 2) {} // expected-error {{incorrect argument labels in call (have 'aax:bbx:_:', expected 'aa:bb:_:')}} {{8-11=aa}} {{16-19=bb}} {{none}}
     f2(aax: 1, bbx: 2) {} // expected-error {{incorrect argument labels in call (have 'aax:bbx:_:', expected 'aa:bb:_:')}} {{8-11=aa}} {{16-19=bb}} {{none}}
 
     f1(aax: 1, bbx: 2) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{8-11=aa}} {{16-19=bb}} {{none}}

test/Constraints/closures.swift

@@ -999,11 +999,13 @@ func rdar52204414() {
   // expected-error@-1 {{declared closure result 'Int' is incompatible with contextual type 'Void'}}
 }
 
-// SR-12291 - trailing closure is used as an argument to the last (positionally) parameter
-func overloaded_with_default(a: () -> Int, b: Int = 0, c: Int = 0) {}
-func overloaded_with_default(b: Int = 0, c: Int = 0, a: () -> Int) {}
+// SR-12291 - trailing closure is used as an argument to the last (positionally) parameter.
+// Note that this was accepted prior to Swift 5.3. SE-0286 changed the
+// order of argument resolution and made it ambiguous.
+func overloaded_with_default(a: () -> Int, b: Int = 0, c: Int = 0) {} // expected-note{{found this candidate}}
+func overloaded_with_default(b: Int = 0, c: Int = 0, a: () -> Int) {} // expected-note{{found this candidate}}
 
-overloaded_with_default { 0 } // Ok (could be ambiguous if trailing was allowed to match `a:` in first overload)
+overloaded_with_default { 0 } // expected-error{{ambiguous use of 'overloaded_with_default'}}
 
 func overloaded_with_default_and_autoclosure<T>(_ a: @autoclosure () -> T, b: Int = 0) {}
 func overloaded_with_default_and_autoclosure<T>(b: Int = 0, c: @escaping () -> T?) {}

test/Constraints/diag_missing_arg.swift

@@ -35,11 +35,9 @@ trailingClosureSingle1() { 1 } // expected-error {{missing argument for paramete
 
 func trailingClosureSingle2(x: () -> Int, y: Int) {} // expected-note * {{here}}
 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}}
+// expected-error@-1 {{missing argument for parameter 'y' in call}} {{none}}
 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}}
+// expected-error@-1 {{missing argument for parameter 'y' in call}} {{none}}
 
 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#>, }}
@@ -48,14 +46,17 @@ trailingClosureMulti1(x: 1, y: 1) // expected-error {{missing argument for param
 
 func trailingClosureMulti2(x: Int, y: () -> Int, z: Int) {} // expected-note * {{here}}
 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}}
+// expected-error@-1 {{missing arguments for parameters 'x', 'z' in call}}
 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}}
+// expected-error@-1 {{missing arguments for parameters 'x', 'z' in call}}
 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}}
+// expected-error@-1 {{missing argument for parameter 'z' in call}} {{none}}
+
+func trailingClosureMulti3(x: (Int) -> Int, y: (Int) -> Int, z: (Int) -> Int) {} // expected-note 2 {{declared here}}
+trailingClosureMulti3 { $0 } y: { $0 } // expected-error{{missing argument for parameter 'z' in call}}{{39-39= z: <#(Int) -> Int#>}}
+
+trailingClosureMulti3 { $0 } z: { $0 } // expected-error{{missing argument for parameter 'y' in call}}{{29-29= y: <#(Int) -> Int#>}}
+
 
 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/overload_filtering.swift

@@ -38,14 +38,3 @@ func testUnresolvedMember(i: Int) -> X {
   // CHECK-NEXT: introducing single enabled disjunction term {{.*}} bound to decl overload_filtering.(file).X.init(_:_:)
   return .init(i, i)
 }
-
-func trailing(x: Int = 0, y: () -> Void) { }
-func trailing(x: Int = 0, z: Float) { }
-
-func testTrailing() {
-  // CHECK: disabled disjunction term {{.*}} bound to decl overload_filtering.(file).trailing(x:z:)
-  trailing() { }
-
-  // CHECK: disabled disjunction term {{.*}} bound to decl overload_filtering.(file).trailing(x:z:)
-  trailing(x: 5) { }
-}