[Trailing closures] Warn about use of deprecated "backward" scan.

Whenever we form a call that relies on the deprecated "backward" scan,
produce a warning to note the deprecation along with a Fix-It to label
the parameter appropriately (and suppress the warning). For example:

    warning: backward matching of the unlabeled trailing closure is
        deprecated; label the argument with 'g' to suppress this warning
      trailingClosureEitherDirection { $0 * $1 }
                                     ^
                                    (g:         )

Author: DougGregor

include/swift/AST/DiagnosticsSema.def

@@ -1183,6 +1183,9 @@ WARNING(unlabeled_trailing_closure_changed_behavior_same_param_name,none,
 NOTE(trailing_closure_select_parameter,none,
      "label the argument with %0 to %select{retain the pre-Swift 5.3 behavior|silence this warning for Swift 5.3 and newer}1",
      (Identifier, unsigned))
+WARNING(unlabeled_trailing_closure_deprecated,none,
+        "backward matching of the unlabeled trailing closure is deprecated; label the argument with %0 to suppress this warning",
+        (Identifier))
 NOTE(decl_multiple_defaulted_closure_parameters,none,
      "%0 contains defaulted closure parameters %1 and %2",
      (DeclName, Identifier, Identifier))

lib/Sema/CSApply.cpp

@@ -5537,6 +5537,96 @@ static unsigned functionParameterArity(AnyFunctionType::Param param) {
   return paramType->castTo<AnyFunctionType>()->getNumParams();
 }
 
+/// Attach a Fix-It to the given diagnostic to give the trailing closure
+/// argument a label.
+static void labelTrailingClosureArgument(
+    ASTContext &ctx, Expr *fn, Expr *arg, Identifier paramName,
+    Expr *trailingClosure, InFlightDiagnostic &diag) {
+  // Dig out source locations.
+  SourceLoc existingRParenLoc;
+  SourceLoc leadingCommaLoc;
+  if (auto tupleExpr = dyn_cast<TupleExpr>(arg)) {
+    existingRParenLoc = tupleExpr->getRParenLoc();
+    assert(tupleExpr->getNumElements() >= 2 && "Should be a ParenExpr?");
+    leadingCommaLoc = Lexer::getLocForEndOfToken(
+        ctx.SourceMgr,
+        tupleExpr->getElements()[tupleExpr->getNumElements()-2]->getEndLoc());
+  } else {
+    auto parenExpr = cast<ParenExpr>(arg);
+    existingRParenLoc = parenExpr->getRParenLoc();
+  }
+
+  // Figure out the text to be inserted before the trailing closure.
+  SmallString<16> insertionText;
+  SourceLoc insertionLoc;
+  if (leadingCommaLoc.isValid()) {
+    insertionText += ", ";
+    assert(existingRParenLoc.isValid());
+    insertionLoc = leadingCommaLoc;
+  } else if (existingRParenLoc.isInvalid()) {
+    insertionText += "(";
+    insertionLoc = Lexer::getLocForEndOfToken(
+        ctx.SourceMgr, fn->getEndLoc());
+  } else {
+    insertionLoc = existingRParenLoc;
+  }
+
+  // Add the label, if there is one.
+  if (!paramName.empty()) {
+    insertionText += paramName.str();
+    insertionText += ": ";
+  }
+
+  // If there is an existing right parentheses, remove it while we
+  // insert the new text.
+  if (existingRParenLoc.isValid()) {
+    SourceLoc afterExistingRParenLoc = Lexer::getLocForEndOfToken(
+        ctx.SourceMgr, existingRParenLoc);
+    diag.fixItReplaceChars(
+        insertionLoc, afterExistingRParenLoc, insertionText);
+  } else {
+    // Insert the appropriate prefix.
+    diag.fixItInsert(insertionLoc, insertionText);
+  }
+
+  // Insert a right parenthesis after the closing '}' of the trailing closure;
+  SourceLoc newRParenLoc = Lexer::getLocForEndOfToken(
+      ctx.SourceMgr, trailingClosure->getEndLoc());
+  diag.fixItInsert(newRParenLoc, ")");
+}
+
+/// Find the trailing closure argument of a tuple or parenthesized expression.
+///
+/// Due to a quirk of the backward scan that could allow reordering of
+/// arguments in the presence of a trailing closure, it might not be the last
+/// argument in the tuple.
+static Expr *findTrailingClosureArgument(Expr *arg) {
+  if (auto parenExpr = dyn_cast<ParenExpr>(arg)) {
+    return parenExpr->getSubExpr();
+  }
+
+  auto tupleExpr = cast<TupleExpr>(arg);
+  SourceLoc endLoc = tupleExpr->getEndLoc();
+  for (Expr *elt : llvm::reverse(tupleExpr->getElements())) {
+    if (elt->getEndLoc() == endLoc)
+      return elt;
+  }
+
+  return tupleExpr->getElements().back();
+}
+
+/// Find the index of the parameter that binds the given argument.
+static unsigned findParamBindingArgument(
+    ArrayRef<ParamBinding> parameterBindings, unsigned argIndex) {
+  for (unsigned paramIdx : indices(parameterBindings)) {
+    if (llvm::find(parameterBindings[paramIdx], argIndex)
+          != parameterBindings[paramIdx].end())
+      return paramIdx;
+  }
+
+  llvm_unreachable("No parameter binds the argument?");
+}
+
 /// SE-0286 changed the direction in which the unlabeled trailing closure
 /// argument is matched to a parameter, from backward (the pre-Swift 5.3
 /// semantics) to forward (after SE-0286). Identify cases where this may
@@ -5558,13 +5648,8 @@ static void maybeWarnAboutTrailingClosureBindingChange(
     return;
 
   // Find the parameter that bound the unlabeled trailing closure argument.
-  unsigned paramIdx;
-  for (paramIdx = 0; paramIdx != parameterBindings.size(); ++paramIdx) {
-    if (llvm::find(parameterBindings[paramIdx], *unlabeledTrailingClosureIndex)
-          != parameterBindings[paramIdx].end())
-      break;
-  }
-  assert(paramIdx != parameterBindings.size() && "Unbound trailing closure!");
+  unsigned paramIdx = findParamBindingArgument(
+      parameterBindings, *unlabeledTrailingClosureIndex);
 
   // If this parameter requires an argument, it would have been unfilled
   // prior to SE-2086; there is nothing to diagnose.
@@ -5594,30 +5679,16 @@ static void maybeWarnAboutTrailingClosureBindingChange(
         functionParameterArity(params[*matchingBackwardParamIdx]))
     return;
 
-  // Compute the various source locations where diagnostics will occur.
-  ASTContext &ctx = params[paramIdx].getPlainType()->getASTContext();
-  Expr *trailingClosure;
-  SourceLoc existingRParenLoc;
-  SourceLoc leadingCommaLoc;
-  if (auto tupleExpr = dyn_cast<TupleExpr>(arg)) {
-    trailingClosure = tupleExpr->getElements().back();
-    existingRParenLoc = tupleExpr->getRParenLoc();
-    assert(tupleExpr->getNumElements() >= 2 && "Should be a ParenExpr?");
-    leadingCommaLoc = Lexer::getLocForEndOfToken(
-        ctx.SourceMgr,
-        tupleExpr->getElements()[tupleExpr->getNumElements()-2]->getEndLoc());
-  } else {
-    auto parenExpr = cast<ParenExpr>(arg);
-    trailingClosure = parenExpr->getSubExpr();
-    existingRParenLoc = parenExpr->getRParenLoc();
-  }
+  // Dig out the trailing closure.
+  Expr *trailingClosure = findTrailingClosureArgument(arg);
 
   // Determine the names of the parameters that would be matched by the
   // forward and backward scans.
   Identifier paramName = params[paramIdx].getLabel();
   Identifier backwardParamName = params[*matchingBackwardParamIdx].getLabel();
 
   // Produce a diagnostic referencing the callee.
+  ASTContext &ctx = params[paramIdx].getPlainType()->getASTContext();
   auto noteCallee = [&] {
     auto decl = callee.getDecl();
     if (!decl)
@@ -5653,45 +5724,8 @@ static void maybeWarnAboutTrailingClosureBindingChange(
     auto diag = ctx.Diags.diagnose(
         trailingClosure->getStartLoc(), diag::trailing_closure_select_parameter,
         paramName, which);
-
-    // Figure out the text to be inserted before the trailing closure.
-    SmallString<16> insertionText;
-    SourceLoc insertionLoc;
-    if (leadingCommaLoc.isValid()) {
-      insertionText += ", ";
-      assert(existingRParenLoc.isValid());
-      insertionLoc = leadingCommaLoc;
-    } else if (existingRParenLoc.isInvalid()) {
-      insertionText += "(";
-      insertionLoc = Lexer::getLocForEndOfToken(
-          ctx.SourceMgr, fn->getEndLoc());
-    } else {
-      insertionLoc = existingRParenLoc;
-    }
-
-    // Add the label, if there is one.
-    if (!paramName.empty()) {
-      insertionText += paramName.str();
-      insertionText += ": ";
-    }
-
-    // If there is an existing right parentheses, remove it while we
-    // insert the new text.
-    if (existingRParenLoc.isValid()) {
-      SourceLoc afterExistingRParenLoc = Lexer::getLocForEndOfToken(
-          ctx.SourceMgr, existingRParenLoc);
-      diag.fixItReplaceChars(
-          insertionLoc, afterExistingRParenLoc, insertionText);
-    } else {
-      // Insert the appropriate prefix.
-      diag.fixItInsert(insertionLoc, insertionText);
-    }
-
-
-    // Insert a right parenthesis after the closing '}' of the trailing closure;
-    SourceLoc newRParenLoc = Lexer::getLocForEndOfToken(
-        ctx.SourceMgr, trailingClosure->getEndLoc());
-    diag.fixItInsert(newRParenLoc, ")");
+    labelTrailingClosureArgument(
+        ctx, fn, arg, paramName, trailingClosure, diag);
   };
 
   diagResolution(backwardParamName, 0);
@@ -5700,6 +5734,34 @@ static void maybeWarnAboutTrailingClosureBindingChange(
   noteCallee();
 }
 
+/// Warn about the use of the deprecated "backward" scan for matching the
+/// unlabeled trailing closure. It was needed to properly type check, but
+/// this code will break with a future version of Swift.
+static void warnAboutTrailingClosureBackwardScan(
+    ConcreteDeclRef callee, Expr *fn, Expr *arg,
+    ArrayRef<AnyFunctionType::Param> params,
+    Optional<unsigned> unlabeledTrailingClosureIndex,
+    ArrayRef<ParamBinding> parameterBindings) {
+
+  Expr *trailingClosure = findTrailingClosureArgument(arg);
+  unsigned paramIdx = findParamBindingArgument(
+      parameterBindings, *unlabeledTrailingClosureIndex);
+  ASTContext &ctx = params[paramIdx].getPlainType()->getASTContext();
+  Identifier paramName = params[paramIdx].getLabel();
+
+  {
+    auto diag = ctx.Diags.diagnose(
+        trailingClosure->getStartLoc(),
+        diag::unlabeled_trailing_closure_deprecated, paramName);
+    labelTrailingClosureArgument(
+        ctx, fn, arg, paramName, trailingClosure, diag);
+  }
+
+  if (auto decl = callee.getDecl()) {
+    ctx.Diags.diagnose(decl, diag::decl_declared_here, decl->getName());
+  }
+}
+
 Expr *ExprRewriter::coerceCallArguments(
     Expr *arg, AnyFunctionType *funcType,
     ConcreteDeclRef callee,
@@ -5790,9 +5852,15 @@ Expr *ExprRewriter::coerceCallArguments(
 
   // Warn if there was a recent change in trailing closure binding semantics
   // that might have lead to a silent change in behavior.
-  maybeWarnAboutTrailingClosureBindingChange(
-      callee, apply ? apply->getFn() : nullptr, arg, args, params, paramInfo,
-      unlabeledTrailingClosureIndex, parameterBindings);
+  if (trailingClosureMatching == TrailingClosureMatching::Forward) {
+    maybeWarnAboutTrailingClosureBindingChange(
+        callee, apply ? apply->getFn() : nullptr, arg, args, params, paramInfo,
+        unlabeledTrailingClosureIndex, parameterBindings);
+  } else {
+    warnAboutTrailingClosureBackwardScan(
+        callee, apply ? apply->getFn() : nullptr, arg, params,
+        unlabeledTrailingClosureIndex, parameterBindings);
+  }
 
   SourceLoc lParenLoc, rParenLoc;
   if (argTuple) {

test/expr/closure/trailing.swift

@@ -6,6 +6,7 @@ func takeValueAndFunc(_ value: Int, _ f: (Int) -> Int) {}
 func takeTwoFuncs(_ f: (Int) -> Int, _ g: (Int) -> Int) {}
 func takeFuncWithDefault(f : ((Int) -> Int)? = nil) {}
 func takeTwoFuncsWithDefaults(f1 : ((Int) -> Int)? = nil, f2 : ((String) -> String)? = nil) {} // expected-note{{contains defaulted closure parameters 'f1' and 'f2'}}
+// expected-note@-1{{'takeTwoFuncsWithDefaults(f1:f2:)' declared here}}
 
 struct X {
   func takeFunc(_ f: (Int) -> Int) {}
@@ -105,8 +106,8 @@ func labeledArgumentAndTrailingClosure() {
   takeFuncWithDefault(f: { $0 + 1 })
 
   // Trailing closure binds to first parameter... unless only the second matches.
- takeTwoFuncsWithDefaults { "Hello, " + $0 }
-  takeTwoFuncsWithDefaults { $0 + 1 } // expected-warning{{rather than}}
+ takeTwoFuncsWithDefaults { "Hello, " + $0 } // expected-warning{{backward matching of the unlabeled trailing closure is deprecated; label the argument with 'f2' to suppress this warning}}
+ takeTwoFuncsWithDefaults { $0 + 1 } // expected-warning{{rather than}}
   // expected-note@-1 2{{label the argument}}
   takeTwoFuncsWithDefaults(f1: {$0 + 1 })
 }

test/expr/postfix/call/forward_trailing_closure_ambiguity.swift

@@ -88,7 +88,7 @@ func testNotAmbiguous2() {
 }
 
 // Not ambiguous because of a missing default argument.
-func notAmbiguous3(
+func notAmbiguous3( // expected-note{{'notAmbiguous3(x:a:y:b:_:c:)' declared here}}
   x: (Int) -> Int = { $0 },
   a: Int = 5,
   y: (Int) -> Int = { $0 },
@@ -98,5 +98,5 @@ func notAmbiguous3(
 ) { }
 
 func testNotAmbiguous3() {
-  notAmbiguous3 { $0 }
+  notAmbiguous3 { $0 } // expected-warning{{backward matching of the unlabeled trailing closure is deprecated; label the argument with '_' to suppress this warning}}
 }

test/expr/postfix/call/forward_trailing_closure_fuzzy.swift

@@ -1,9 +1,9 @@
 // RUN: %target-typecheck-verify-swift
 
-func doSomething(onError: ((Error) -> Void)? = nil, onCompletion: (Int) -> Void) { }
+func doSomething(onError: ((Error) -> Void)? = nil, onCompletion: (Int) -> Void) { } // expected-note{{'doSomething(onError:onCompletion:)' declared here}}
 
 func testDoSomething() {
-  doSomething { x in
+  doSomething { x in // expected-warning{{backward matching of the unlabeled trailing closure is deprecated; label the argument with 'onCompletion' to suppress this warning}}
     print(x)
   }
 
@@ -31,13 +31,13 @@ func testTrailingClosures() {
 
 // Ensure that we can match either with the forward or the backward rule,
 // depending on additional type check information.
-func trailingClosureEitherDirection(
+func trailingClosureEitherDirection( // expected-note{{'trailingClosureEitherDirection(f:g:)' declared here}}
   f: (Int) -> Int = { $0 }, g: (Int, Int) -> Int = { $0 + $1 }
 ) { }
 
 func testTrailingClosureEitherDirection() {
   trailingClosureEitherDirection { -$0 }
-  trailingClosureEitherDirection { $0 * $1 }
+  trailingClosureEitherDirection { $0 * $1 } // expected-warning{{backward matching of the unlabeled trailing closure is deprecated; label the argument with 'g' to suppress this warning}}{{33-33=(g: }}{{45-45=)}}
 }
 
 // Check that we resolve ambiguities when both directions can be matched.
@@ -51,11 +51,11 @@ trailingClosureBothDirections { $0 * $1 } // expected-warning{{since Swift 5.3,
 
 // Check an amusing quirk of the "backward" rule that allows the order of
 // arguments to be swapped.
-struct AccidentalReorder {
+struct AccidentalReorder { // expected-note{{'init(content:optionalInt:)' declared here}}
   let content: () -> Int
   var optionalInt: Int?
 }
 
 func testAccidentalReorder() {
-  _ = AccidentalReorder(optionalInt: 17) { 42 }
+  _ = AccidentalReorder(optionalInt: 17) { 42 } // expected-warning{{backward matching of the unlabeled trailing closure is deprecated; label the argument with 'content' to suppress this warning}}
 }