Offer fix-its to disambiguate based on a trailing closure's label.

(by making it a normal argument with a label and not a trailing
closure)

Diagnostic part of rdar://problem/25607552. A later commit will keep
us from getting in this situation quite so much when default arguments
are involved.

Author: jrose-apple

include/swift/AST/DiagnosticsSema.def

@@ -2235,6 +2235,10 @@ ERROR(ambiguous_decl_ref,none,
       "ambiguous use of %0", (DeclName))
 ERROR(ambiguous_operator_ref,none,
       "ambiguous use of operator %0", (DeclName))
+NOTE(ambiguous_because_of_trailing_closure,none,
+     "%select{use an explicit argument label instead of a trailing closure|"
+     "avoid using a trailing closure}0 to call %1",
+     (bool, DeclName))
 
 // Cannot capture inout-ness of a parameter
 // Partial application of foreign functions not supported

lib/Sema/CSDiag.cpp

@@ -334,6 +334,93 @@ static DeclName getOverloadChoiceName(ArrayRef<OverloadChoice> choices) {
   return name;
 }
 
+/// Returns true if any diagnostics were emitted.
+static bool
+tryDiagnoseTrailingClosureAmbiguity(TypeChecker &tc, const Expr *expr,
+                                    ArrayRef<OverloadChoice> choices) {
+  auto *callExpr = dyn_cast<CallExpr>(expr);
+  if (!callExpr)
+    return false;
+  if (!callExpr->hasTrailingClosure())
+    return false;
+
+  llvm::SmallMapVector<Identifier, const ValueDecl *, 8> choicesByLabel;
+  for (const OverloadChoice &choice : choices) {
+    auto *callee = dyn_cast<AbstractFunctionDecl>(choice.getDecl());
+    if (!callee)
+      return false;
+
+    const ParameterList *paramList = callee->getParameterLists().back();
+    const ParamDecl *param = paramList->getArray().back();
+
+    // Sanity-check that the trailing closure corresponds to this parameter.
+    if (!param->getType()->is<AnyFunctionType>())
+      return false;
+
+    Identifier trailingClosureLabel = param->getArgumentName();
+    auto &choiceForLabel = choicesByLabel[trailingClosureLabel];
+
+    // FIXME: Cargo-culted from diagnoseAmbiguity: apparently the same decl can
+    // appear more than once?
+    if (choiceForLabel == callee)
+      continue;
+
+    // If just providing the trailing closure label won't solve the ambiguity,
+    // don't bother offering the fix-it.
+    if (choiceForLabel != nullptr)
+      return false;
+
+    choiceForLabel = callee;
+  }
+
+  // If we got here, then all of the choices have unique labels. Offer them in
+  // order.
+  SourceManager &sourceMgr = tc.Context.SourceMgr;
+  SourceLoc replaceStartLoc;
+  SourceLoc closureStartLoc;
+  bool hasOtherArgs, needsParens;
+  if (auto tupleArgs = dyn_cast<TupleExpr>(callExpr->getArg())) {
+    assert(tupleArgs->getNumElements() >= 2);
+    const Expr *lastBeforeClosure = tupleArgs->getElements().drop_back().back();
+    replaceStartLoc =
+        Lexer::getLocForEndOfToken(sourceMgr, lastBeforeClosure->getEndLoc());
+    closureStartLoc = tupleArgs->getElements().back()->getStartLoc();
+    hasOtherArgs = true;
+    needsParens = false;
+  } else {
+    auto parenArgs = cast<ParenExpr>(callExpr->getArg());
+    replaceStartLoc = parenArgs->getRParenLoc();
+    closureStartLoc = parenArgs->getSubExpr()->getStartLoc();
+    hasOtherArgs = false;
+    needsParens = parenArgs->getRParenLoc().isInvalid();
+  }
+  if (!replaceStartLoc.isValid()) {
+    assert(callExpr->getFn()->getEndLoc().isValid());
+    replaceStartLoc =
+        Lexer::getLocForEndOfToken(sourceMgr, callExpr->getFn()->getEndLoc());
+  }
+
+  for (const auto &choicePair : choicesByLabel) {
+    SmallString<64> insertionString;
+    if (needsParens)
+      insertionString += "(";
+    else if (hasOtherArgs)
+      insertionString += ", ";
+
+    if (!choicePair.first.empty()) {
+      insertionString += choicePair.first.str();
+      insertionString += ": ";
+    }
+
+    tc.diagnose(expr->getLoc(), diag::ambiguous_because_of_trailing_closure,
+                choicePair.first.empty(), choicePair.second->getFullName())
+      .fixItReplaceChars(replaceStartLoc, closureStartLoc, insertionString)
+      .fixItInsertAfter(callExpr->getEndLoc(), ")");
+  }
+
+  return true;
+}
+
 static bool diagnoseAmbiguity(ConstraintSystem &cs,
                               ArrayRef<Solution> solutions,
                               Expr *expr) {
@@ -424,9 +511,12 @@ static bool diagnoseAmbiguity(ConstraintSystem &cs,
                                   : diag::ambiguous_decl_ref,
                 name);
 
+    if (tryDiagnoseTrailingClosureAmbiguity(tc, expr, overload.choices))
+      return true;
+
     // Emit candidates.  Use a SmallPtrSet to make sure only emit a particular
     // candidate once.  FIXME: Why is one candidate getting into the overload
-    // set multiple times?
+    // set multiple times? (See also tryDiagnoseTrailingClosureAmbiguity.)
     SmallPtrSet<Decl*, 8> EmittedDecls;
     for (auto choice : overload.choices) {
       switch (choice.getKind()) {

test/expr/closure/trailing.swift

@@ -180,3 +180,71 @@ func r23036383(foo: Foo23036383?, obj: Foo23036383) {
   // expected-error@-1 {{trailing closure requires parentheses for disambiguation in this context}} {{22-23=(}} {{28-28=)}} 
   // expected-error@-2 {{trailing closure requires parentheses for disambiguation in this context}} {{37-38=(x: }} {{43-43=)}} 
 }
+
+func overloadOnLabel(a: () -> Void) {}
+func overloadOnLabel(b: () -> Void) {}
+func overloadOnLabel(c: () -> Void) {}
+
+func overloadOnLabel2(a: () -> Void) {}
+func overloadOnLabel2(_: () -> Void) {}
+
+func overloadOnLabelArgs(_: Int, a: () -> Void) {}
+func overloadOnLabelArgs(_: Int, b: () -> Void) {}
+
+func overloadOnLabelArgs2(_: Int, a: () -> Void) {}
+func overloadOnLabelArgs2(_: Int, _: () -> Void) {}
+
+func overloadOnLabelDefaultArgs(x: Int = 0, a: () -> Void) {}
+func overloadOnLabelDefaultArgs(x: Int = 1, b: () -> Void) {}
+
+func overloadOnLabelSomeDefaultArgs(_: Int, x: Int = 0, a: () -> Void) {}
+func overloadOnLabelSomeDefaultArgs(_: Int, x: Int = 1, b: () -> Void) {}
+
+func overloadOnDefaultArgsOnly(x: Int = 0, a: () -> Void) {} // expected-note 2 {{found this candidate}}
+func overloadOnDefaultArgsOnly(y: Int = 1, a: () -> Void) {} // expected-note 2 {{found this candidate}}
+
+func overloadOnDefaultArgsOnly2(x: Int = 0, a: () -> Void) {} // expected-note 2 {{found this candidate}}
+func overloadOnDefaultArgsOnly2(y: Bool = true, a: () -> Void) {} // expected-note 2 {{found this candidate}}
+
+func overloadOnDefaultArgsOnly3(x: Int = 0, a: () -> Void) {} // expected-note 2 {{found this candidate}}
+func overloadOnDefaultArgsOnly3(x: Bool = true, a: () -> Void) {} // expected-note 2 {{found this candidate}}
+
+func overloadOnSomeDefaultArgsOnly(_: Int, x: Int = 0, a: () -> Void) {} // expected-note {{found this candidate}}
+func overloadOnSomeDefaultArgsOnly(_: Int, y: Int = 1, a: () -> Void) {} // expected-note {{found this candidate}}
+
+func overloadOnSomeDefaultArgsOnly2(_: Int, x: Int = 0, a: () -> Void) {} // expected-note {{found this candidate}}
+func overloadOnSomeDefaultArgsOnly2(_: Int, y: Bool = true, a: () -> Void) {} // expected-note {{found this candidate}}
+
+func overloadOnSomeDefaultArgsOnly3(_: Int, x: Int = 0, a: () -> Void) {} // expected-note {{found this candidate}}
+func overloadOnSomeDefaultArgsOnly3(_: Int, x: Bool = true, a: () -> Void) {} // expected-note {{found this candidate}}
+
+func testOverloadAmbiguity() {
+  overloadOnLabel {} // expected-error {{ambiguous use of 'overloadOnLabel'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel(a:)'}} {{18-19=(a: }} {{21-21=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel(b:)'}} {{18-19=(b: }} {{21-21=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel(c:)'}} {{18-19=(c: }} {{21-21=)}}
+  overloadOnLabel() {} // expected-error {{ambiguous use of 'overloadOnLabel'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel(a:)'}} {{19-21=a: }} {{23-23=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel(b:)'}} {{19-21=b: }} {{23-23=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel(c:)'}} {{19-21=c: }} {{23-23=)}}
+  overloadOnLabel2 {} // expected-error {{ambiguous use of 'overloadOnLabel2'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel2(a:)'}} {{19-20=(a: }} {{22-22=)}} expected-note {{avoid using a trailing closure to call 'overloadOnLabel2'}} {{19-20=(}} {{22-22=)}}
+  overloadOnLabel2() {} // expected-error {{ambiguous use of 'overloadOnLabel2'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabel2(a:)'}} {{20-22=a: }} {{24-24=)}} expected-note {{avoid using a trailing closure to call 'overloadOnLabel2'}} {{20-22=}} {{24-24=)}}
+  overloadOnLabelArgs(1) {} // expected-error {{ambiguous use of 'overloadOnLabelArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelArgs(_:a:)'}} {{24-26=, a: }} {{28-28=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelArgs(_:b:)'}} {{24-26=, b: }} {{28-28=)}}
+  overloadOnLabelArgs2(1) {} // expected-error {{ambiguous use of 'overloadOnLabelArgs2'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelArgs2(_:a:)'}} {{25-27=, a: }} {{29-29=)}} expected-note {{avoid using a trailing closure to call 'overloadOnLabelArgs2'}} {{25-27=, }} {{29-29=)}}
+  overloadOnLabelDefaultArgs {} // expected-error {{ambiguous use of 'overloadOnLabelDefaultArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelDefaultArgs(x:a:)'}} {{29-30=(a: }} {{32-32=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelDefaultArgs(x:b:)'}} {{29-30=(b: }} {{32-32=)}}
+  overloadOnLabelDefaultArgs() {} // expected-error {{ambiguous use of 'overloadOnLabelDefaultArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelDefaultArgs(x:a:)'}} {{30-32=a: }} {{34-34=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelDefaultArgs(x:b:)'}} {{30-32=b: }} {{34-34=)}}
+  overloadOnLabelDefaultArgs(x: 2) {} // expected-error {{ambiguous use of 'overloadOnLabelDefaultArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelDefaultArgs(x:a:)'}} {{34-36=, a: }} {{38-38=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelDefaultArgs(x:b:)'}} {{34-36=, b: }} {{38-38=)}}
+  overloadOnLabelSomeDefaultArgs(1) {} // expected-error {{ambiguous use of 'overloadOnLabelSomeDefaultArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelSomeDefaultArgs(_:x:a:)'}} {{35-37=, a: }} {{39-39=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelSomeDefaultArgs(_:x:b:)'}} {{35-37=, b: }} {{39-39=)}}
+  overloadOnLabelSomeDefaultArgs(1, x: 2) {} // expected-error {{ambiguous use of 'overloadOnLabelSomeDefaultArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelSomeDefaultArgs(_:x:a:)'}} {{41-43=, a: }} {{45-45=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelSomeDefaultArgs(_:x:b:)'}} {{41-43=, b: }} {{45-45=)}}
+
+  overloadOnLabelSomeDefaultArgs( // expected-error {{ambiguous use of 'overloadOnLabelSomeDefaultArgs'}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelSomeDefaultArgs(_:x:a:)'}} {{12-5=, a: }} {{4-4=)}} expected-note {{use an explicit argument label instead of a trailing closure to call 'overloadOnLabelSomeDefaultArgs(_:x:b:)'}} {{12-5=, b: }} {{4-4=)}}
+    1, x: 2
+  ) {
+    // some
+  }
+
+  overloadOnDefaultArgsOnly {} // expected-error {{ambiguous use of 'overloadOnDefaultArgsOnly'}}
+  overloadOnDefaultArgsOnly() {} // expected-error {{ambiguous use of 'overloadOnDefaultArgsOnly'}}
+  overloadOnDefaultArgsOnly2 {} // expected-error {{ambiguous use of 'overloadOnDefaultArgsOnly2'}}
+  overloadOnDefaultArgsOnly2() {} // expected-error {{ambiguous use of 'overloadOnDefaultArgsOnly2'}}
+  overloadOnDefaultArgsOnly3 {} // expected-error {{ambiguous use of 'overloadOnDefaultArgsOnly3(x:a:)'}}
+  overloadOnDefaultArgsOnly3() {} // expected-error {{ambiguous use of 'overloadOnDefaultArgsOnly3(x:a:)'}}
+
+  overloadOnSomeDefaultArgsOnly(1) {} // expected-error {{ambiguous use of 'overloadOnSomeDefaultArgsOnly'}}
+  overloadOnSomeDefaultArgsOnly2(1) {} // expected-error {{ambiguous use of 'overloadOnSomeDefaultArgsOnly2'}}
+  overloadOnSomeDefaultArgsOnly3(1) {} // expected-error {{ambiguous use of 'overloadOnSomeDefaultArgsOnly3(_:x:a:)'}}
+}