Merge pull request #33364 from xedin/trailing-closure-warning

[Diagnostics] Extract backward scan deprecation warning into a fix/diagnostic

Author: xedin

lib/Sema/CSApply.cpp

@@ -5469,124 +5469,6 @@ static bool hasCurriedSelf(ConstraintSystem &cs, ConcreteDeclRef callee,
   return false;
 }
 
-/// 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?");
-}
-
-/// 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,
@@ -5678,13 +5560,6 @@ Expr *ExprRewriter::coerceCallArguments(
   // FIXME: Eventually, we want to enforce that we have either argTuple or
   // argParen here.
 
-  // Warn about the backward scan being deprecated.
-  if (trailingClosureMatching == TrailingClosureMatching::Backward) {
-    warnAboutTrailingClosureBackwardScan(
-        callee, apply ? apply->getFn() : nullptr, arg, params,
-        unlabeledTrailingClosureIndex, parameterBindings);
-  }
-
   SourceLoc lParenLoc, rParenLoc;
   if (argTuple) {
     lParenLoc = argTuple->getLParenLoc();

lib/Sema/CSDiagnostics.cpp

@@ -6598,3 +6598,95 @@ SourceLoc MissingOptionalUnwrapKeyPathFailure::getLoc() const {
   auto *SE = castToExpr<SubscriptExpr>(getAnchor());
   return SE->getBase()->getEndLoc();
 }
+
+bool TrailingClosureRequiresExplicitLabel::diagnoseAsError() {
+  auto argInfo = *getFunctionArgApplyInfo(getLocator());
+
+  {
+    auto diagnostic = emitDiagnostic(
+        diag::unlabeled_trailing_closure_deprecated, argInfo.getParamLabel());
+    fixIt(diagnostic, argInfo);
+  }
+
+  if (auto *callee = argInfo.getCallee()) {
+    emitDiagnosticAt(callee, diag::decl_declared_here, callee->getName());
+  }
+
+  return true;
+}
+
+void TrailingClosureRequiresExplicitLabel::fixIt(
+    InFlightDiagnostic &diagnostic, const FunctionArgApplyInfo &info) const {
+  auto &ctx = getASTContext();
+
+  // Dig out source locations.
+  SourceLoc existingRParenLoc;
+  SourceLoc leadingCommaLoc;
+
+  auto anchor = getRawAnchor();
+  auto *arg = info.getArgListExpr();
+  Expr *fn = nullptr;
+
+  if (auto *applyExpr = getAsExpr<ApplyExpr>(anchor)) {
+    fn = applyExpr->getFn();
+  } else {
+    // Covers subscripts, unresolved members etc.
+    fn = getAsExpr(anchor);
+  }
+
+  if (!fn)
+    return;
+
+  auto *trailingClosure = info.getArgExpr();
+
+  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.
+  auto paramName = info.getParamLabel();
+  if (!paramName.empty()) {
+    insertionText += paramName.str();
+    insertionText += ": ";
+  }
+
+  // If there is an existing right parentheses/brace, remove it while we
+  // insert the new text.
+  if (existingRParenLoc.isValid()) {
+    SourceLoc afterExistingRParenLoc =
+        Lexer::getLocForEndOfToken(ctx.SourceMgr, existingRParenLoc);
+    diagnostic.fixItReplaceChars(insertionLoc, afterExistingRParenLoc,
+                                 insertionText);
+  } else {
+    // Insert the appropriate prefix.
+    diagnostic.fixItInsert(insertionLoc, insertionText);
+  }
+
+  // Insert a right parenthesis/brace after the closing '}' of the trailing
+  // closure;
+  SourceLoc newRParenLoc =
+      Lexer::getLocForEndOfToken(ctx.SourceMgr, trailingClosure->getEndLoc());
+  diagnostic.fixItInsert(newRParenLoc,
+                         isExpr<SubscriptExpr>(anchor) ? "]" : ")");
+}

lib/Sema/CSDiagnostics.h

@@ -2216,6 +2216,30 @@ class MissingOptionalUnwrapKeyPathFailure final : public ContextualFailure {
   SourceLoc getLoc() const override;
 };
 
+/// Diagnose situations when trailing closure has been matched to a specific
+/// parameter via a deprecated backward scan.
+///
+/// \code
+/// func multiple_trailing_with_defaults(
+///   duration: Int,
+///   animations: (() -> Void)? = nil,
+///   completion: (() -> Void)? = nil) {}
+///
+/// multiple_trailing_with_defaults(duration: 42) {} // picks `completion:`
+/// \endcode
+class TrailingClosureRequiresExplicitLabel final : public FailureDiagnostic {
+public:
+  TrailingClosureRequiresExplicitLabel(const Solution &solution,
+                                       ConstraintLocator *locator)
+      : FailureDiagnostic(solution, locator) {}
+
+  bool diagnoseAsError() override;
+
+private:
+  void fixIt(InFlightDiagnostic &diagnostic,
+             const FunctionArgApplyInfo &info) const;
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSFix.cpp

@@ -1526,3 +1526,16 @@ UnwrapOptionalBaseKeyPathApplication::attempt(ConstraintSystem &cs, Type baseTy,
   return new (cs.getAllocator())
       UnwrapOptionalBaseKeyPathApplication(cs, baseTy, rootTy, locator);
 }
+
+bool SpecifyLabelToAssociateTrailingClosure::diagnose(const Solution &solution,
+                                                      bool asNote) const {
+  TrailingClosureRequiresExplicitLabel failure(solution, getLocator());
+  return failure.diagnose(asNote);
+}
+
+SpecifyLabelToAssociateTrailingClosure *
+SpecifyLabelToAssociateTrailingClosure::create(ConstraintSystem &cs,
+                                               ConstraintLocator *locator) {
+  return new (cs.getAllocator())
+      SpecifyLabelToAssociateTrailingClosure(cs, locator);
+}

lib/Sema/CSFix.h

@@ -269,6 +269,10 @@ enum class FixKind : uint8_t {
 
   /// Unwrap optional base on key path application.
   UnwrapOptionalBaseKeyPathApplication,
+
+  /// Explicitly specify a label to match trailing closure to a certain
+  /// parameter in the call.
+  SpecifyLabelToAssociateTrailingClosure,
 };
 
 class ConstraintFix {
@@ -1926,6 +1930,34 @@ class UnwrapOptionalBaseKeyPathApplication final : public ContextualMismatch {
           ConstraintLocator *locator);
 };
 
+/// Diagnose situations when solver used old (backward scan) rule
+/// to match trailing closure to a parameter.
+///
+/// \code
+/// func multiple_trailing_with_defaults(
+///   duration: Int,
+///   animations: (() -> Void)? = nil,
+///   completion: (() -> Void)? = nil) {}
+///
+/// multiple_trailing_with_defaults(duration: 42) {} // picks `completion:`
+/// \endcode
+class SpecifyLabelToAssociateTrailingClosure final : public ConstraintFix {
+  SpecifyLabelToAssociateTrailingClosure(ConstraintSystem &cs,
+                                         ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::SpecifyLabelToAssociateTrailingClosure,
+                      locator, /*isWarning=*/true) {}
+
+public:
+  std::string getName() const override {
+    return "specify a label to associate trailing closure with parameter";
+  }
+
+  bool diagnose(const Solution &solution, bool asNote = false) const override;
+
+  static SpecifyLabelToAssociateTrailingClosure *
+  create(ConstraintSystem &cs, ConstraintLocator *locator);
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSSimplify.cpp

@@ -1208,6 +1208,9 @@ ConstraintSystem::TypeMatchResult constraints::matchCallArguments(
 
   // Match up the call arguments to the parameters.
   SmallVector<ParamBinding, 4> parameterBindings;
+  TrailingClosureMatching selectedTrailingMatching =
+      TrailingClosureMatching::Forward;
+
   {
     ArgumentFailureTracker listener(cs, argsWithLabels, params, locator);
     auto callArgumentMatch = constraints::matchCallArguments(
@@ -1224,10 +1227,10 @@ ConstraintSystem::TypeMatchResult constraints::matchCallArguments(
       return cs.getTypeMatchAmbiguous();
     }
 
+    selectedTrailingMatching = callArgumentMatch->trailingClosureMatching;
     // Record the direction of matching used for this call.
-    cs.recordTrailingClosureMatch(
-        cs.getConstraintLocator(locator),
-        callArgumentMatch->trailingClosureMatching);
+    cs.recordTrailingClosureMatch(cs.getConstraintLocator(locator),
+                                  selectedTrailingMatching);
 
     // If there was a disjunction because both forward and backward were
     // possible, increase the score for forward matches to bias toward the
@@ -1316,6 +1319,15 @@ ConstraintSystem::TypeMatchResult constraints::matchCallArguments(
         }
       }
 
+      // In case solver matched trailing based on the backward scan,
+      // let's produce a warning which would suggest to add a label
+      // to disambiguate in the future.
+      if (selectedTrailingMatching == TrailingClosureMatching::Backward &&
+          argIdx == *argInfo->UnlabeledTrailingClosureIndex) {
+        cs.recordFix(SpecifyLabelToAssociateTrailingClosure::create(
+            cs, cs.getConstraintLocator(loc)));
+      }
+
       // If argument comes for declaration it should loose
       // `@autoclosure` flag, because in context it's used
       // as a function type represented by autoclosure.
@@ -9967,7 +9979,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::AllowKeyPathRootTypeMismatch:
   case FixKind::UnwrapOptionalBaseKeyPathApplication:
   case FixKind::AllowCoercionToForceCast:
-  case FixKind::SpecifyKeyPathRootType: {
+  case FixKind::SpecifyKeyPathRootType:
+  case FixKind::SpecifyLabelToAssociateTrailingClosure: {
     return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
   }