[Trailing closures] Diagnose the change in behavior with SE-0268.

SE-0248 changes the backward-scan matching behavior for the unlabeled
trailing closure into a forward scan. In circumstances where this
could silently change the meaning of a call to a particular
function, i.e., when there are two defaulted closure parameters such
that a given closure to match either one of them, produce an warning
that describes the change in behavior. For example:

    t4.swift:2:24: warning: since Swift 5.3, unlabeled trailing
closure argument matches parameter 'x' rather than parameter 'z'
    trailingClosureSingle2 { $0 }
                           ^
    t4.swift:2:24: note: label the argument with 'z' to retain the
pre-Swift 5.3 behavior
    trailingClosureSingle2 { $0 }
                           ^
                          (z:    )
    t4.swift:2:24: note: label the argument with 'x' to silence this
warning for Swift 5.3 and newer
    trailingClosureSingle2 { $0 }
                           ^
                          (x:    )
    t4.swift:1:6: note: 'trailingClosureSingle2(x:y:z:)' contains
defaulted closure parameters 'x' and 'z'
    func trailingClosureSingle2(x: (Int) -> Int = { $0 } , y: (Int) ->
Int = { $0 }, z: (Int) -> Int = { $0 }) {}
         ^                      ~

This explains the (rare) case where SE-0286 silently changes the
meaning of a program, offering Fix-Its to either restore the
pre-SE-0286 behavior or silence the warning, as appropriate.

Author: DougGregor

include/swift/AST/DiagnosticsSema.def

@@ -1174,6 +1174,18 @@ ERROR(extra_trailing_closure_in_call,none,
 ERROR(trailing_closure_bad_param,none,
       "trailing closure passed to parameter of type %0 that does not "
       "accept a closure", (Type))
+WARNING(unlabeled_trailing_closure_changed_behavior,none,
+        "since Swift 5.3, unlabeled trailing closure argument matches parameter %0 rather than parameter %1",
+        (Identifier, Identifier))
+WARNING(unlabeled_trailing_closure_changed_behavior_same_param_name,none,
+        "since Swift 5.3, unlabeled trailing closure argument matches earlier parameter %0 rather than later parameter with the same name",
+        (Identifier))
+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))
+NOTE(decl_multiple_defaulted_closure_parameters,none,
+     "%0 contains defaulted closure parameters %1 and %2",
+     (DeclName, Identifier, Identifier))
 NOTE(candidate_with_extraneous_args,none,
       "candidate %0 requires %1 argument%s1, "
       "but %2 %select{were|was}3 %select{provided|used in closure body}4",

lib/Sema/CSApply.cpp

@@ -5521,6 +5521,195 @@ static bool hasCurriedSelf(ConstraintSystem &cs, ConcreteDeclRef callee,
   return false;
 }
 
+/// Determine the arity of the given parameter of function type.
+static unsigned functionParameterArity(AnyFunctionType::Param param) {
+  Type paramType = param.getPlainType();
+  if (param.isVariadic())
+    paramType = ParamDecl::getVarargBaseTy(paramType);
+
+  paramType = paramType->lookThroughAllOptionalTypes();
+
+  if (param.isAutoClosure()) {
+    paramType = paramType->castTo<AnyFunctionType>()->getResult();
+    paramType = paramType->lookThroughAllOptionalTypes();
+  }
+
+  return paramType->castTo<AnyFunctionType>()->getNumParams();
+}
+
+/// 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
+/// have resulted in a silent change in behavior.
+static void maybeWarnAboutTrailingClosureBindingChange(
+    ConcreteDeclRef callee,
+    Expr *fn,
+    Expr *arg,
+    ArrayRef<AnyFunctionType::Param> args,
+    ArrayRef<AnyFunctionType::Param> params,
+    const ParameterListInfo &paramInfo,
+    Optional<unsigned> unlabeledTrailingClosureIndex,
+    ArrayRef<ParamBinding> parameterBindings) {
+
+  if (!unlabeledTrailingClosureIndex)
+    return;
+
+  if (*unlabeledTrailingClosureIndex != args.size() - 1)
+    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!");
+
+  // If this parameter requires an argument, it would have been unfilled
+  // prior to SE-2086; there is nothing to diagnose.
+  if (parameterRequiresArgument(params, paramInfo, paramIdx))
+    return;
+
+  // Look for a later parameter that could match a trailing closure; the
+  // last one of these would have been picked prior to SE-0286.
+  Optional<unsigned> matchingBackwardParamIdx;
+  for (unsigned backwardParamIdx :
+           range(paramIdx + 1, parameterBindings.size())) {
+    if (!paramInfo.acceptsUnlabeledTrailingClosureArgument(backwardParamIdx))
+      continue;
+
+    matchingBackwardParamIdx = backwardParamIdx;
+  }
+
+  // If there is no other parameter that could match the unlabeled trailing
+  // closure, there is nothing to diagnose.
+  if (!matchingBackwardParamIdx)
+    return;
+
+  // Do a simple arity check; if the matched parameter and backward-matched
+  // parameter accept functions with different arity, this would not have
+  // type-checked with the backward scan, so there is nothing to report.
+  if (functionParameterArity(params[paramIdx]) !=
+        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();
+  }
+
+  // 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.
+  auto noteCallee = [&] {
+    auto decl = callee.getDecl();
+    if (!decl)
+      return;
+
+    auto diag = ctx.Diags.diagnose(
+        decl, diag::decl_multiple_defaulted_closure_parameters,
+        decl->getName(), paramName, backwardParamName);
+
+    // Dig out the parameter declarations so we can highlight them.
+    // FIXME: There should be a utility for this.
+    const ParameterList *paramList = nullptr;
+    if (auto *func = dyn_cast<AbstractFunctionDecl>(decl)) {
+      paramList = func->getParameters();
+    } else if (auto *subscript = dyn_cast<SubscriptDecl>(decl)) {
+      paramList = subscript->getIndices();
+    } else if (auto *enumElement = dyn_cast<EnumElementDecl>(decl)) {
+      paramList = enumElement->getParameterList();
+    }
+
+    if (paramList) {
+      diag.highlight(paramList->get(paramIdx)->getLoc());
+      diag.highlight(paramList->get(*matchingBackwardParamIdx)->getLoc());
+    }
+  };
+
+  // If the parameters have the same name, provide a custom diagnostic and then
+  // bail out early; there are no useful notes we can provide here.
+  if (paramName == backwardParamName) {
+    ctx.Diags.diagnose(trailingClosure->getStartLoc(), diag::unlabeled_trailing_closure_changed_behavior_same_param_name,
+        paramName);
+    noteCallee();
+    return;
+  }
+
+  // Produce the diagnostic.
+  ctx.Diags.diagnose(trailingClosure->getStartLoc(), diag::unlabeled_trailing_closure_changed_behavior, paramName,
+      backwardParamName);
+
+  // Produce a note with a Fix-It describing how to resolve the ambiguity.
+  auto diagResolution = [&](Identifier paramName, unsigned which) {
+    // Emit the note.
+    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, ")");
+  };
+
+  diagResolution(backwardParamName, 0);
+  diagResolution(paramName, 1);
+
+  noteCallee();
+}
+
 Expr *ExprRewriter::coerceCallArguments(
     Expr *arg, AnyFunctionType *funcType,
     ConcreteDeclRef callee,
@@ -5594,6 +5783,12 @@ Expr *ExprRewriter::coerceCallArguments(
   // FIXME: Eventually, we want to enforce that we have either argTuple or
   // argParen here.
 
+  // 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->getFn(), arg, args, params, paramInfo,
+      unlabeledTrailingClosureIndex, parameterBindings);
+
   SourceLoc lParenLoc, rParenLoc;
   if (argTuple) {
     lParenLoc = argTuple->getLParenLoc();

lib/Sema/CSSimplify.cpp

@@ -195,7 +195,7 @@ static ConstraintSystem::TypeMatchOptions getDefaultDecompositionOptions(
 }
 
 /// Whether the given parameter requires an argument.
-static bool parameterRequiresArgument(
+bool swift::parameterRequiresArgument(
     ArrayRef<AnyFunctionType::Param> params,
     const ParameterListInfo &paramInfo,
     unsigned paramIdx) {

lib/Sema/ConstraintSystem.h

@@ -5439,6 +5439,12 @@ BraceStmt *applyFunctionBuilderTransform(
           constraints::SolutionApplicationTarget)>
             rewriteTarget);
 
+/// Whether the given parameter requires an argument.
+bool parameterRequiresArgument(
+    ArrayRef<AnyFunctionType::Param> params,
+    const ParameterListInfo &paramInfo,
+    unsigned paramIdx);
+
 } // end namespace swift
 
 #endif // LLVM_SWIFT_SEMA_CONSTRAINT_SYSTEM_H

test/expr/postfix/call/forward_trailing_closure_ambiguity.swift

@@ -0,0 +1,102 @@
+// RUN: %target-typecheck-verify-swift
+
+// Ambiguity when calling.
+func ambiguous1( // expected-note 3 {{'ambiguous1(x:a:y:b:z:c:)' contains defaulted closure parameters '}}
+
+  x: (Int) -> Int = { $0 },
+  a: Int = 5,
+  y: (Int) -> Int = { $0 },
+  b: Int = 5,
+  z: (Int) -> Int = { $0 },
+  c: Int = 5
+) {}
+
+func testAmbiguous1() {
+  ambiguous1 { $0 } // expected-warning{{since Swift 5.3, unlabeled trailing closure argument matches parameter 'x' rather than parameter 'z'}}
+    // expected-note@-1{{label the argument with 'z' to retain the pre-Swift 5.3 behavior}}{{13-13=(z: }}{{20-20=)}}
+    // expected-note@-2{{label the argument with 'x' to silence this warning for Swift 5.3 and newer}}{{13-13=(x: }}{{20-20=)}}
+
+  ambiguous1() { $0 } // expected-warning{{since Swift 5.3, unlabeled trailing closure argument matches parameter 'x' rather than parameter 'z'}}
+    // expected-note@-1{{label the argument with 'z' to retain the pre-Swift 5.3 behavior}}{{14-15=z: }}{{22-22=)}}
+    // expected-note@-2{{label the argument with 'x' to silence this warning for Swift 5.3 and newer}}{{14-15=x: }}{{22-22=)}}
+
+  ambiguous1(a: 3) { $0 } // expected-warning{{since Swift 5.3, unlabeled trailing closure argument matches parameter 'y' rather than parameter 'z'}}
+    // expected-note@-1{{label the argument with 'z' to retain the pre-Swift 5.3 behavior}}{{18-19=, z: }}{{26-26=)}}
+    // expected-note@-2{{label the argument with 'y' to silence this warning for Swift 5.3 and newer}}{{18-19=, y: }}{{26-26=)}}
+
+  // No warning; this is matching the last parameter.
+  ambiguous1(b: 3) { $0 }
+}
+
+// Ambiguity with two unlabeled arguments.
+func ambiguous2( // expected-note{{'ambiguous2(_:a:y:b:_:x:)' contains defaulted closure parameters '_' and '_'}}
+  _: (Int) -> Int = { $0 },
+  a: Int = 5,
+  y: (Int) -> Int = { $0 },
+  b: Int = 5,
+  _: (Int) -> Int = { $0 },
+  x: Int = 5
+) {}
+
+func testAmbiguous2() {
+    ambiguous2 { $0 } // expected-warning{{since Swift 5.3, unlabeled trailing closure argument matches earlier parameter '_' rather than later parameter with the same name}}
+}
+
+// Ambiguity with one unlabeled argument.
+func ambiguous3( // expected-note{{'ambiguous3(x:a:y:b:_:c:)' contains defaulted closure parameters 'x' and '_'}}
+  x: (Int) -> Int = { $0 },
+  a: Int = 5,
+  y: (Int) -> Int = { $0 },
+  b: Int = 5,
+  _: (Int) -> Int = { $0 },
+  c: Int = 5
+) {}
+
+func testAmbiguous3() {
+  ambiguous3 { $0 } // expected-warning{{since Swift 5.3, unlabeled trailing closure argument matches parameter 'x' rather than parameter '_'}}
+    // expected-note@-1{{label the argument with '_' to retain the pre-Swift 5.3 behavior}}{{13-13=(}}{{20-20=)}}
+    // expected-note@-2{{label the argument with 'x' to silence this warning for Swift 5.3 and newer}}{{13-13=(x: }}{{20-20=)}}
+}
+
+// Not ambiguous because of an arity mismatch that would lead to different
+// type-checks.
+func notAmbiguous1(
+  x: (Int, Int) -> Int = { $0 + $1 },
+  a: Int = 5,
+  y: (Int) -> Int = { $0 },
+  b: Int = 5,
+  _: (Int) -> Int = { $0 },
+  c: Int = 5
+) { }
+
+func testNotAmbiguous1() {
+  notAmbiguous1 { $0 / $1 }
+}
+
+// Not ambiguous because of a missing default argument.
+func notAmbiguous2(
+  x: (Int) -> Int,
+  a: Int = 5,
+  y: (Int) -> Int = { $0 },
+  b: Int = 5,
+  _: (Int) -> Int = { $0 },
+  c: Int = 5
+) { }
+
+func testNotAmbiguous2() {
+  notAmbiguous2 { $0 }
+}
+
+// Not ambiguous because of a missing default argument.
+func notAmbiguous3(
+  x: (Int) -> Int = { $0 },
+  a: Int = 5,
+  y: (Int) -> Int = { $0 },
+  b: Int = 5,
+  _: (Int) -> Int ,
+  c: Int = 5
+) { }
+
+func testNotAmbiguous3() {
+  notAmbiguous3 { $0 }
+}