Merge pull request #38705 from hamishknight/to-many-avails

Author: hamishknight

include/swift/AST/DiagnosticsSema.def

@@ -1267,7 +1267,8 @@ ERROR(extra_argument_named,none,
 ERROR(extra_argument_positional,none,
       "extra argument in call", ())
 ERROR(extra_arguments_in_call,none,
-      "extra arguments at positions %0 in call", (StringRef))
+      "extra %select{arguments|trailing closures}0 at positions %1 in call",
+       (bool, StringRef))
 ERROR(extra_argument_to_nullary_call,none,
       "argument passed to call that takes no arguments", ())
 ERROR(extra_trailing_closure_in_call,none,

include/swift/Sema/IDETypeChecking.h

@@ -243,7 +243,32 @@ namespace swift {
     SmallVector<SourceLoc, 4> labelLocs;
     SourceLoc lParenLoc;
     SourceLoc rParenLoc;
-    bool hasTrailingClosure = false;
+    Optional<unsigned> unlabeledTrailingClosureIdx;
+
+    /// The number of trailing closures in the argument list.
+    unsigned getNumTrailingClosures() const {
+      if (!unlabeledTrailingClosureIdx)
+        return 0;
+      return args.size() - *unlabeledTrailingClosureIdx;
+    }
+
+    /// Whether any unlabeled or labeled trailing closures are present.
+    bool hasAnyTrailingClosures() const {
+      return unlabeledTrailingClosureIdx.hasValue();
+    }
+
+    /// Whether the given index is for an unlabeled trailing closure.
+    bool isUnlabeledTrailingClosureIdx(unsigned i) const {
+      return unlabeledTrailingClosureIdx && *unlabeledTrailingClosureIdx == i;
+    }
+
+    /// Whether the given index is for a labeled trailing closure in an
+    /// argument list with multiple trailing closures.
+    bool isLabeledTrailingClosureIdx(unsigned i) const {
+      if (!unlabeledTrailingClosureIdx)
+        return false;
+      return i > *unlabeledTrailingClosureIdx && i < args.size();
+    }
   };
 
   /// When applying a solution to a constraint system, the type checker rewrites

lib/Sema/CSApply.cpp

@@ -5837,8 +5837,9 @@ Expr *ExprRewriter::coerceCallArguments(
   // Apply labels to arguments.
   AnyFunctionType::relabelParams(args, argLabels);
 
-  auto unlabeledTrailingClosureIndex =
+  auto oldTrailingClosureIndex =
       arg->getUnlabeledTrailingClosureIndexOfPackedArgument();
+  Optional<unsigned> newTrailingClosureIndex;
 
   // Determine the parameter bindings that were applied.
   auto *locatorPtr = cs.getConstraintLocator(locator);
@@ -5912,6 +5913,12 @@ Expr *ExprRewriter::coerceCallArguments(
         auto arg = getArg(argIdx);
         auto argType = cs.getType(arg);
 
+        // Update the trailing closure index if needed.
+        if (oldTrailingClosureIndex && *oldTrailingClosureIndex == argIdx) {
+          assert(!newTrailingClosureIndex);
+          newTrailingClosureIndex = newArgs.size();
+        }
+
         // If the argument type exactly matches, this just works.
         if (argType->isEqual(param.getPlainType())) {
           variadicArgs.push_back(arg);
@@ -5974,6 +5981,12 @@ Expr *ExprRewriter::coerceCallArguments(
     auto arg = getArg(argIdx);
     auto argType = cs.getType(arg);
 
+    // Update the trailing closure index if needed.
+    if (oldTrailingClosureIndex && *oldTrailingClosureIndex == argIdx) {
+      assert(!newTrailingClosureIndex);
+      newTrailingClosureIndex = newArgs.size();
+    }
+
     // Save the original label location.
     newLabelLocs.push_back(getLabelLoc(argIdx));
 
@@ -6089,14 +6102,17 @@ Expr *ExprRewriter::coerceCallArguments(
   assert(newArgs.size() == newParams.size());
   assert(newArgs.size() == newLabels.size());
   assert(newArgs.size() == newLabelLocs.size());
+  assert(oldTrailingClosureIndex.hasValue() ==
+         newTrailingClosureIndex.hasValue());
+  assert(!newTrailingClosureIndex || *newTrailingClosureIndex < newArgs.size());
 
   // This is silly. SILGen gets confused if a 'self' parameter is wrapped
   // in a ParenExpr sometimes.
   if (!argTuple && !argParen &&
       (params[0].getValueOwnership() == ValueOwnership::Default ||
        params[0].getValueOwnership() == ValueOwnership::InOut)) {
     assert(newArgs.size() == 1);
-    assert(!unlabeledTrailingClosureIndex);
+    assert(!newTrailingClosureIndex);
     return newArgs[0];
   }
 
@@ -6111,7 +6127,7 @@ Expr *ExprRewriter::coerceCallArguments(
       bool isImplicit = arg->isImplicit();
       arg = new (ctx) ParenExpr(
           lParenLoc, newArgs[0], rParenLoc,
-          static_cast<bool>(unlabeledTrailingClosureIndex));
+          static_cast<bool>(newTrailingClosureIndex));
       arg->setImplicit(isImplicit);
     }
   } else {
@@ -6126,7 +6142,7 @@ Expr *ExprRewriter::coerceCallArguments(
     } else {
       // Build a new TupleExpr, re-using source location information.
       arg = TupleExpr::create(ctx, lParenLoc, rParenLoc, newArgs, newLabels,
-                              newLabelLocs, unlabeledTrailingClosureIndex,
+                              newLabelLocs, newTrailingClosureIndex,
                               /*implicit=*/arg->isImplicit());
     }
   }

lib/Sema/CSDiagnostics.cpp

@@ -5197,7 +5197,16 @@ bool ExtraneousArgumentsFailure::diagnoseAsError() {
       },
       [&] { OS << ", "; });
 
-  emitDiagnostic(diag::extra_arguments_in_call, OS.str());
+  bool areTrailingClosures = false;
+  if (auto *argExpr = getArgumentListExprFor(getLocator())) {
+    if (auto i = argExpr->getUnlabeledTrailingClosureIndexOfPackedArgument()) {
+      areTrailingClosures = llvm::all_of(ExtraArgs, [&](auto &pair) {
+        return pair.first >= i;
+      });
+    }
+  }
+
+  emitDiagnostic(diag::extra_arguments_in_call, areTrailingClosures, OS.str());
 
   if (auto overload = getCalleeOverloadChoiceIfAvailable(getLocator())) {
     if (auto *decl = overload->choice.getDeclOrNull()) {
@@ -5248,9 +5257,12 @@ bool ExtraneousArgumentsFailure::diagnoseSingleExtraArgument() const {
   auto argExpr = tuple ? tuple->getElement(index)
                        : cast<ParenExpr>(arguments)->getSubExpr();
 
+  auto trailingClosureIdx =
+      arguments->getUnlabeledTrailingClosureIndexOfPackedArgument();
+  auto isTrailingClosure = trailingClosureIdx && index >= *trailingClosureIdx;
+
   auto loc = argExpr->getLoc();
-  if (tuple && index == tuple->getNumElements() - 1 &&
-      tuple->hasTrailingClosure()) {
+  if (isTrailingClosure) {
     emitDiagnosticAt(loc, diag::extra_trailing_closure_in_call)
         .highlight(argExpr->getSourceRange());
   } else if (ContextualType->getNumParams() == 0) {

lib/Sema/CSGen.cpp

@@ -4232,9 +4232,17 @@ OriginalArgumentList
 swift::getOriginalArgumentList(Expr *expr) {
   OriginalArgumentList result;
 
-  auto add = [&](Expr *arg, Identifier label, SourceLoc labelLoc) {
-    if (isa<DefaultArgumentExpr>(arg)) {
+  auto oldTrailingClosureIdx =
+      expr->getUnlabeledTrailingClosureIndexOfPackedArgument();
+  Optional<unsigned> newTrailingClosureIdx;
+
+  auto add = [&](unsigned i, Expr *arg, Identifier label, SourceLoc labelLoc) {
+    if (isa<DefaultArgumentExpr>(arg))
       return;
+
+    if (oldTrailingClosureIdx && *oldTrailingClosureIdx == i) {
+      assert(!newTrailingClosureIdx);
+      newTrailingClosureIdx = result.args.size();
     }
 
     if (auto *varargExpr = dyn_cast<VarargExpansionExpr>(arg)) {
@@ -4260,24 +4268,25 @@ swift::getOriginalArgumentList(Expr *expr) {
   if (auto *parenExpr = dyn_cast<ParenExpr>(expr)) {
     result.lParenLoc = parenExpr->getLParenLoc();
     result.rParenLoc = parenExpr->getRParenLoc();
-    result.hasTrailingClosure = parenExpr->hasTrailingClosure();
-    add(parenExpr->getSubExpr(), Identifier(), SourceLoc());
+    add(0, parenExpr->getSubExpr(), Identifier(), SourceLoc());
   } else if (auto *tupleExpr = dyn_cast<TupleExpr>(expr)) {
     result.lParenLoc = tupleExpr->getLParenLoc();
     result.rParenLoc = tupleExpr->getRParenLoc();
-    result.hasTrailingClosure = tupleExpr->hasTrailingClosure();
 
     auto args = tupleExpr->getElements();
     auto labels = tupleExpr->getElementNames();
     auto labelLocs = tupleExpr->getElementNameLocs();
     for (unsigned i = 0, e = args.size(); i != e; ++i) {
       // Implicit TupleExprs don't always store label locations.
-      add(args[i], labels[i],
+      add(i, args[i], labels[i],
           labelLocs.empty() ? SourceLoc() : labelLocs[i]);
     }
   } else {
-    add(expr, Identifier(), SourceLoc());
+    add(0, expr, Identifier(), SourceLoc());
   }
+  assert(oldTrailingClosureIdx.hasValue() == newTrailingClosureIdx.hasValue());
+  assert(!newTrailingClosureIdx || *newTrailingClosureIdx < result.args.size());
 
+  result.unlabeledTrailingClosureIdx = newTrailingClosureIdx;
   return result;
 }

lib/Sema/MiscDiagnostics.cpp

@@ -1906,9 +1906,7 @@ bool swift::diagnoseArgumentLabelError(ASTContext &ctx,
     assert(oldName || newName && "We can't have oldName and newName out of "
                                  "bounds, otherwise n would be smaller");
 
-    if (oldName == newName ||
-        (argList.hasTrailingClosure && i == argList.args.size() - 1 &&
-         (numMissing > 0 || numExtra > 0 || numWrong > 0)))
+    if (oldName == newName || argList.isUnlabeledTrailingClosureIdx(i))
       continue;
 
     if (!oldName.hasValue() && newName.hasValue()) {
@@ -1988,11 +1986,17 @@ bool swift::diagnoseArgumentLabelError(ASTContext &ctx,
     if (i < newNames.size())
       newName = newNames[i];
 
-    if (oldName == newName || (i == n-1 && argList.hasTrailingClosure))
+    if (oldName == newName || argList.isUnlabeledTrailingClosureIdx(i))
       continue;
 
     if (newName.empty()) {
-      // Delete the old name.
+      // If this is a labeled trailing closure, we need to replace with '_'.
+      if (argList.isLabeledTrailingClosureIdx(i)) {
+        diag.fixItReplace(argList.labelLocs[i], "_");
+        continue;
+      }
+
+      // Otherwise, delete the old name.
       diag.fixItRemoveChars(argList.labelLocs[i],
                             argList.args[i]->getStartLoc());
       continue;
@@ -2006,7 +2010,10 @@ bool swift::diagnoseArgumentLabelError(ASTContext &ctx,
     if (newNameIsReserved)
       newStr += "`";
 
-    if (oldName.empty()) {
+    // If the argument was previously unlabeled, insert the new label. Note that
+    // we don't do this for labeled trailing closures as they write unlabeled
+    // args as '_:', and therefore need replacement.
+    if (oldName.empty() && !argList.isLabeledTrailingClosureIdx(i)) {
       // Insert the name.
       newStr += ": ";
       diag.fixItInsert(argList.args[i]->getStartLoc(), newStr);

lib/Sema/TypeCheckAvailability.cpp

@@ -1872,7 +1872,7 @@ static void fixItAvailableAttrRename(InFlightDiagnostic &diag,
     auto argList = getOriginalArgumentList(argExpr);
 
     size_t numElementsWithinParens = argList.args.size();
-    numElementsWithinParens -= argList.hasTrailingClosure;
+    numElementsWithinParens -= argList.getNumTrailingClosures();
     if (selfIndex >= numElementsWithinParens)
       return;
 
@@ -2120,18 +2120,16 @@ static void fixItAvailableAttrRename(InFlightDiagnostic &diag,
     return;
   }
 
-  auto argumentLabelsToCheck = llvm::makeArrayRef(argumentLabelIDs);
-  // The argument label for a trailing closure is ignored.
-  if (argList.hasTrailingClosure)
-    argumentLabelsToCheck = argumentLabelsToCheck.drop_back();
-
-  if (std::equal(argumentLabelsToCheck.begin(), argumentLabelsToCheck.end(),
-                 argList.labels.begin())) {
-    // Already matching.
-    return;
+  // If any of the argument labels are mismatched, perform label correction.
+  for (auto i : indices(argList.args)) {
+    // The argument label of an unlabeled trailing closure is ignored.
+    if (argList.isUnlabeledTrailingClosureIdx(i))
+      continue;
+    if (argumentLabelIDs[i] != argList.labels[i]) {
+      diagnoseArgumentLabelError(ctx, argExpr, argumentLabelIDs, false, &diag);
+      return;
+    }
   }
-
-  diagnoseArgumentLabelError(ctx, argExpr, argumentLabelIDs, false, &diag);
 }
 
 // Must be kept in sync with diag::availability_decl_unavailable_rename and

lib/Sema/TypeCheckCodeCompletion.cpp

@@ -288,18 +288,18 @@ class SanitizeExpr : public ASTWalker {
         new (C) ParenExpr(argList.lParenLoc,
                           argList.args[0],
                           argList.rParenLoc,
-                          argList.hasTrailingClosure);
+                          argList.hasAnyTrailingClosures());
       result->setImplicit();
       return result;
     }
 
     return TupleExpr::create(C,
                              argList.lParenLoc,
+                             argList.rParenLoc,
                              argList.args,
                              argList.labels,
                              argList.labelLocs,
-                             argList.rParenLoc,
-                             argList.hasTrailingClosure,
+                             argList.unlabeledTrailingClosureIdx,
                              /*implicit=*/true);
   }
 

test/Constraints/argument_matching.swift

@@ -1762,3 +1762,23 @@ func rdar70764991() {
     bar(str, S.foo) // expected-error {{unnamed argument #1 must precede unnamed argument #2}}  {{9-12=}} {{14-14=str}}
   }
 }
+
+func testExtraTrailingClosure() {
+  func foo() {}
+  foo() {} // expected-error@:9 {{extra trailing closure passed in call}}
+  foo {} // expected-error@:7 {{extra trailing closure passed in call}}
+  foo {} x: {} // expected-error@:7 {{argument passed to call that takes no arguments}}
+
+  func bar(_ x: Int) {} // expected-note 2{{'bar' declared here}}
+  bar(5) {} // expected-error@:10 {{extra trailing closure passed in call}}
+  bar(0) {} x: {} // expected-error@:6 {{extra trailing closures at positions #2, #3 in call}}
+  bar(5, "") {} // expected-error@:6 {{extra arguments at positions #2, #3 in call}}
+
+  func baz(_ fn: () -> Void) {} // expected-note {{'baz' declared here}}
+  baz {} x: {} // expected-error@:13 {{extra trailing closure passed in call}}
+  baz({}) {} // expected-error@:11 {{extra trailing closure passed in call}}
+  baz({}) {} y: {} // expected-error@:6 {{extra trailing closures at positions #2, #3 in call}}
+
+  func qux(x: () -> Void, y: () -> Void, z: () -> Void) {} // expected-note {{'qux(x:y:z:)' declared here}}
+  qux() {} m: {} y: {} n: {} z: {} o: {} // expected-error@:6 {{extra trailing closures at positions #2, #4, #6 in call}}
+}

test/Parse/trailing_closures.swift

@@ -82,7 +82,7 @@ func test_multiple_trailing_syntax_without_labels() {
 
   fn {} g: {} // Ok
 
-  fn {} _: {} // expected-error {{missing argument labels 'f:g:' in call}}
+  fn {} _: {} // expected-error {{missing argument label 'g:' in call}} {{9-10=g}} {{none}}
 
   fn {} g: <#T##() -> Void#> // expected-error {{editor placeholder in source file}}
 
@@ -93,28 +93,15 @@ func test_multiple_trailing_syntax_without_labels() {
   func mixed_args_1(a: () -> Void, _: () -> Void) {}
   func mixed_args_2(_: () -> Void, a: () -> Void, _: () -> Void) {} // expected-note {{'mixed_args_2(_:a:_:)' declared here}}
 
-  mixed_args_1
-    {}
-    _: {}
+  mixed_args_1 {} _: {}
 
-  mixed_args_1
-    {}  // expected-error {{incorrect argument labels in call (have '_:a:', expected 'a:_:')}}
-    a: {}
+  mixed_args_1 {} a: {}  // expected-error@:16 {{extraneous argument label 'a:' in call}} {{19-20=_}} {{none}}
 
-  mixed_args_2
-    {}
-    a: {}
-    _: {}
+  mixed_args_2 {} a: {} _: {}
 
-  mixed_args_2
-    {} // expected-error {{missing argument for parameter 'a' in call}}
-    _: {}
+  mixed_args_2 {} _: {} // expected-error@:18 {{missing argument for parameter 'a' in call}} {{18-18= a: <#() -> Void#>}} {{none}}
 
-  // FIXME: not a good diagnostic
-  mixed_args_2
-    {}  // expected-error {{missing argument label 'a:' in call}}
-    _: {}
-    _: {}
+  mixed_args_2 {} _: {} _: {} // expected-error@:16 {{missing argument label 'a:' in call}} {{19-20=a}} {{none}}
 }
 
 func produce(fn: () -> Int?, default d: () -> Int) -> Int { // expected-note {{declared here}}
@@ -129,7 +116,7 @@ func f() -> Int { 42 }
 // This should be interpreted as a trailing closure, instead of being 
 // interpreted as a computed property with undesired initial value.
 struct TrickyTest {
-    var x : Int = f () { // expected-error {{argument passed to call that takes no arguments}}
+    var x : Int = f () { // expected-error {{extra trailing closure passed in call}}
         3
     }
 }