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

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

include/swift/Basic/DiagnosticOptions.h

@@ -24,7 +24,11 @@ class DiagnosticOptions {
   /// Indicates whether the diagnostics produced during compilation should be
   /// checked against expected diagnostics, indicated by markers in the
   /// input source file.
-  bool VerifyDiagnostics = false;
+  enum {
+    NoVerify,
+    Verify,
+    VerifyAndApplyFixes
+  } VerifyMode = NoVerify;
 
   /// Indicates whether diagnostic passes should be skipped.
   bool SkipDiagnosticPasses = false;

include/swift/Frontend/DiagnosticVerifier.h

@@ -23,17 +23,17 @@
 namespace swift {
   class SourceManager;
 
-  /// enableDiagnosticVerifier - Set up the specified source manager so that
-  /// diagnostics are captured instead of being printed.
+  /// Set up the specified source manager so that diagnostics are captured
+  /// instead of being printed.
   void enableDiagnosticVerifier(SourceManager &SM);
 
-  /// verifyDiagnostics - Verify that captured diagnostics meet with the
-  /// expectations of the source files corresponding to the specified BufferIDs
-  /// and tear down our support for capturing and verifying diagnostics.
+  /// Verify that captured diagnostics meet with the expectations of the source
+  /// files corresponding to the specified \p BufferIDs and tear down our
+  /// support for capturing and verifying diagnostics.
   ///
   /// This returns true if there are any mismatches found.
-  ///
-  bool verifyDiagnostics(SourceManager &SM, ArrayRef<unsigned> BufferIDs);
+  bool verifyDiagnostics(SourceManager &SM, ArrayRef<unsigned> BufferIDs,
+                         bool autoApplyFixes);
 }
 
 #endif

include/swift/Option/FrontendOptions.td

@@ -61,6 +61,8 @@ def emit_fixits_path
 def verify : Flag<["-"], "verify">,
   HelpText<"Verify diagnostics against expected-{error|warning|note} "
            "annotations">;
+def verify_apply_fixes : Flag<["-"], "verify-apply-fixes">,
+  HelpText<"Like -verify, but updates the original source file">;
 
 def show_diagnostics_after_fatal : Flag<["-"], "show-diagnostics-after-fatal">,
   HelpText<"Keep emitting subsequent diagnostics after a fatal error">;

lib/Frontend/CompilerInvocation.cpp

@@ -981,7 +981,10 @@ static bool ParseDiagnosticArgs(DiagnosticOptions &Opts, ArgList &Args,
                                 DiagnosticEngine &Diags) {
   using namespace options;
 
-  Opts.VerifyDiagnostics |= Args.hasArg(OPT_verify);
+  if (Args.hasArg(OPT_verify))
+    Opts.VerifyMode = DiagnosticOptions::Verify;
+  if (Args.hasArg(OPT_verify_apply_fixes))
+    Opts.VerifyMode = DiagnosticOptions::VerifyAndApplyFixes;
   Opts.SkipDiagnosticPasses |= Args.hasArg(OPT_disable_diagnostic_passes);
   Opts.ShowDiagnosticsAfterFatalError |=
     Args.hasArg(OPT_show_diagnostics_after_fatal);

lib/Frontend/DiagnosticVerifier.cpp

@@ -677,30 +677,24 @@ void DiagnosticVerifier::autoApplyFixes(unsigned BufferID,
 // Main entrypoints
 //===----------------------------------------------------------------------===//
 
-/// VerifyModeDiagnosticHook - Every time a diagnostic is generated in -verify
-/// mode, this function is called with the diagnostic.  We just buffer them up
-/// until the end of the file.
+/// Every time a diagnostic is generated in -verify mode, this function is
+/// called with the diagnostic.  We just buffer them up until the end of the
+/// file.
 static void VerifyModeDiagnosticHook(const llvm::SMDiagnostic &Diag,
                                      void *Context) {
   ((DiagnosticVerifier*)Context)->addDiagnostic(Diag);
 }
 
 
-/// enableDiagnosticVerifier - Set up the specified source manager so that
-/// diagnostics are captured instead of being printed.
 void swift::enableDiagnosticVerifier(SourceManager &SM) {
   SM.getLLVMSourceMgr().setDiagHandler(VerifyModeDiagnosticHook,
                                        new DiagnosticVerifier(SM));
 }
 
-/// verifyDiagnostics - Verify that captured diagnostics meet with the
-/// expectations of the source files corresponding to the specified BufferIDs
-/// and tear down our support for capturing and verifying diagnostics.
-bool swift::verifyDiagnostics(SourceManager &SM, ArrayRef<unsigned> BufferIDs) {
+bool swift::verifyDiagnostics(SourceManager &SM, ArrayRef<unsigned> BufferIDs,
+                              bool autoApplyFixes) {
   auto *Verifier = (DiagnosticVerifier*)SM.getLLVMSourceMgr().getDiagContext();
   SM.getLLVMSourceMgr().setDiagHandler(nullptr, nullptr);
-
-  bool autoApplyFixes = false;
 
   bool HadError = false;
 

lib/FrontendTool/FrontendTool.cpp

@@ -1269,7 +1269,8 @@ int swift::performFrontend(ArrayRef<const char *> Args,
     llvm::EnableStatistics();
   }
 
-  if (Invocation.getDiagnosticOptions().VerifyDiagnostics) {
+  const DiagnosticOptions &diagOpts = Invocation.getDiagnosticOptions();
+  if (diagOpts.VerifyMode != DiagnosticOptions::NoVerify) {
     enableDiagnosticVerifier(Instance.getSourceMgr());
   }
 
@@ -1298,9 +1299,12 @@ int swift::performFrontend(ArrayRef<const char *> Args,
                        Invocation.getFrontendOptions().DumpAPIPath);
   }
 
-  if (Invocation.getDiagnosticOptions().VerifyDiagnostics) {
-    HadError = verifyDiagnostics(Instance.getSourceMgr(),
-                                 Instance.getInputBufferIDs());
+  if (diagOpts.VerifyMode != DiagnosticOptions::NoVerify) {
+    HadError = verifyDiagnostics(
+        Instance.getSourceMgr(),
+        Instance.getInputBufferIDs(),
+        diagOpts.VerifyMode == DiagnosticOptions::VerifyAndApplyFixes);
+
     DiagnosticEngine &diags = Instance.getDiags();
     if (diags.hasFatalErrorOccurred() &&
         !Invocation.getDiagnosticOptions().ShowDiagnosticsAfterFatalError) {

lib/Sema/CSDiag.cpp

@@ -312,12 +312,113 @@ static unsigned countDistinctOverloads(ArrayRef<OverloadChoice> choices) {
 
 /// \brief Determine the name of the overload in a set of overload choices.
 static DeclName getOverloadChoiceName(ArrayRef<OverloadChoice> choices) {
+  DeclName name;
   for (auto choice : choices) {
-    if (choice.isDecl())
-      return choice.getDecl()->getFullName();
+    if (!choice.isDecl())
+      continue;
+
+    DeclName nextName = choice.getDecl()->getFullName();
+    if (!name) {
+      name = nextName;
+      continue;
+    }
+
+    if (name != nextName) {
+      // Assume all choices have the same base name and only differ in
+      // argument labels. This may not be a great assumption, but we don't
+      // really have a way to recover for diagnostics otherwise.
+      return name.getBaseName();
+    }
   }
 
-  return DeclName();
+  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,
@@ -410,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:)'}}
+}

test/expr/unary/selector/selector.swift

@@ -15,6 +15,9 @@ class C1 {
   @objc class func method3(_ a: A, b: B) { } // expected-note{{found this candidate}}
   @objc class func method3(a: A, b: B) { } // expected-note{{found this candidate}}
 
+  @objc(ambiguous1:b:) class func ambiguous(a: A, b: A) { } // expected-note{{found this candidate}}
+  @objc(ambiguous2:b:) class func ambiguous(a: A, b: B) { } // expected-note{{found this candidate}}
+
   @objc func getC1() -> AnyObject { return self }
 
   @objc func testUnqualifiedSelector(_ a: A, b: B) {
@@ -83,7 +86,8 @@ func testSelector(_ c1: C1, p1: P1, obj: AnyObject) {
 }
 
 func testAmbiguity() {
-  _ = #selector(C1.method3) // expected-error{{ambiguous use of 'method3(_:b:)'}}
+  _ = #selector(C1.method3) // expected-error{{ambiguous use of 'method3'}}
+  _ = #selector(C1.ambiguous) // expected-error{{ambiguous use of 'ambiguous(a:b:)'}}
 }
 
 func testUnusedSelector() {

tools/sil-opt/SILOpt.cpp

@@ -354,7 +354,8 @@ int main(int argc, char **argv) {
   // If we're in -verify mode, we've buffered up all of the generated
   // diagnostics.  Check now to ensure that they meet our expectations.
   if (VerifyMode) {
-    HadError = verifyDiagnostics(CI.getSourceMgr(), CI.getInputBufferIDs());
+    HadError = verifyDiagnostics(CI.getSourceMgr(), CI.getInputBufferIDs(),
+                                 /*autoApplyFixes*/false);
     DiagnosticEngine &diags = CI.getDiags();
     if (diags.hasFatalErrorOccurred() &&
         !Invocation.getDiagnosticOptions().ShowDiagnosticsAfterFatalError) {