[Fixit] Add a fixit for converting non-trailing closures to trailing closures.

Reminded by Ben's last commit of code completion, I noticed that some users
do not use trailing closures when helping migrating their code. To enhance the discoverability
of this feature, this fix helps users to convert non-trailing closures to trailing
closures. To ensure disambiguity after conversion, we check name conflict before adding
the note.

Author: nkcsgexi

include/swift/AST/DiagnosticsSema.def

@@ -2082,6 +2082,10 @@ ERROR(string_literal_broken_proto,none,
 ERROR(bool_type_broken,none,
       "could not find a Bool type defined for 'is'", ())
 
+NOTE(convert_to_trailing_closure,none,
+"use trailing closure to simplify arguments", ())
+
+
 // Array literals
 ERROR(array_protocol_broken,none,
       "ArrayLiteralConvertible protocol definition is broken", ())

lib/Sema/MiscDiagnostics.cpp

@@ -1984,6 +1984,141 @@ static void diagAvailability(TypeChecker &TC, const Expr *E,
   const_cast<Expr*>(E)->walk(walker);
 }
 
+namespace {
+
+static ClosureExpr* getLastArgAsClosure(Expr* Arg) {
+  if (auto Paren = dyn_cast<ParenExpr>(Arg)) {
+
+    // If the argument is paren expression, get the sub expression as closure.
+    if (!Paren->hasTrailingClosure())
+      return dyn_cast_or_null<ClosureExpr>(Paren->getSubExpr());
+  } else if (auto Tuple = dyn_cast<TupleExpr>(Arg)) {
+
+    // If the argument is tuple expression, get the last expression in the tuple
+    // as closure.
+    if (!Tuple->hasTrailingClosure() && Tuple->getNumElements() > 0)
+      return dyn_cast<ClosureExpr>(Tuple->getElement(Tuple->getNumElements() - 1));
+  }
+  return nullptr;
+}
+
+static bool areIdentifiersSame(Identifier Left, Identifier Right) {
+
+  // If both are anonymous, returns true.
+  if (Left.empty() && Right.empty())
+    return true;
+
+  // If one is anonymous and the other is not, returns false.
+  if (Left.empty() || Right.empty())
+    return false;
+
+  // If both are not anonymous, compare the content.
+  return Left.str() == Right.str();
+}
+
+static bool willCauseAmbiguity(TypeChecker &TC, FuncDecl *SelectedFD) {
+  auto SelectedArgNames = SelectedFD->getEffectiveFullName().getArgumentNames();
+  auto SelectedArgNameExcludingClosure = SelectedArgNames.slice(0,
+    SelectedArgNames.size() - 1);
+
+  // Consider all decls from the same context with the simple
+  // identifier with SelectedFD.
+  for (ValueDecl *VD : TC.lookupUnqualified(SelectedFD->getDeclContext(),
+                                            SelectedFD->getName(), SourceLoc())) {
+
+    // Make sure the overload is not the selected function.
+    if (VD == SelectedFD)
+      continue;
+
+    // Make sure the overload is a function.
+    FuncDecl *FD = dyn_cast<FuncDecl>(VD);
+    if (!FD)
+      continue;
+    auto ArgNames = FD->getEffectiveFullName().getArgumentNames();
+
+    // If the overload requires more arguments than the selected; there are no
+    // chance of conflicts.
+    if (ArgNames.size() < SelectedArgNames.size())
+      continue;
+
+    // Assuming there is a conflict.
+    bool Conflict = true;
+    for (unsigned I = 0; I < SelectedArgNameExcludingClosure.size(); I ++) {
+      if (!areIdentifiersSame(ArgNames[I], SelectedArgNameExcludingClosure[I])) {
+        // Different argument names disprove our assumption.
+        Conflict = false;
+      }
+    }
+    if (Conflict)
+      return true;
+  }
+  return false;
+}
+
+static FuncDecl* digForFuncDecl(Expr *Fn) {
+  auto FD = dyn_cast_or_null<FuncDecl>(Fn->getReferencedDecl().getDecl());
+  if (FD)
+    return FD;
+  if (auto DE = dyn_cast<DotSyntaxCallExpr>(Fn)) {
+    return digForFuncDecl(DE->getFn());
+  }
+  return nullptr;
+}
+
+static void
+applyConvertToTrailingClosureFixit(TypeChecker &TC, SourceManager &SM,
+                                   ClosureExpr *Closure, Expr *Arg) {
+  if (auto *Paren = dyn_cast<ParenExpr>(Arg)) {
+    SourceRange LRemove(Paren->getLParenLoc(),
+                        Paren->getLParenLoc().getAdvancedLoc(1));
+    SourceRange RRemove(Paren->getRParenLoc(),
+                        Paren->getRParenLoc().getAdvancedLoc(1));
+
+    // Remove the left and right paren.
+    TC.diagnose(Closure->getStartLoc(), diag::convert_to_trailing_closure).
+      fixItRemove(LRemove).fixItRemove(RRemove);
+  } else if (auto *Tuple = dyn_cast<TupleExpr>(Arg)) {
+    if (Tuple->getNumElements() > 1) {
+      SourceLoc ReplaceStart = Lexer::getLocForEndOfToken(SM,
+        Tuple->getElement(Tuple->getNumElements() - 2)->getEndLoc());
+      SourceRange Replace(ReplaceStart, Closure->getStartLoc());
+      SourceRange ToRemove(Tuple->getRParenLoc(),
+                           Tuple->getRParenLoc().getAdvancedLoc(1));
+
+      // Closing the tuple before the closure; and remove the right paren after the
+      // closure.
+      TC.diagnose(Closure->getStartLoc(), diag::convert_to_trailing_closure).
+        fixItReplace(Replace, ") ").fixItRemove(ToRemove);
+    } else {
+      SourceRange LRemove(Tuple->getLParenLoc(), Closure->getStartLoc());
+      SourceRange RRemove(Lexer::getLocForEndOfToken(SM,Closure->getEndLoc()),
+                          Lexer::getLocForEndOfToken(SM, Tuple->getEndLoc()));
+      TC.diagnose(Closure->getStartLoc(), diag::convert_to_trailing_closure).
+        fixItRemove(LRemove).fixItRemove(RRemove);
+    }
+  }
+}
+}
+
+static void diagConvertToTrailingClosure(TypeChecker &TC, const Expr *E,
+                                      DeclContext *DC) {
+  auto* AE = dyn_cast<ApplyExpr>(E);
+  if (!AE)
+    return;
+
+  auto* Closure = getLastArgAsClosure(AE->getArg());
+  if (!Closure)
+    return;
+  if (auto* FD = digForFuncDecl(AE->getFn())) {
+    // Check the condition whether converting will cause ambiguity.
+    if (!willCauseAmbiguity(TC, FD))
+      // Without ambiguity, we can proceed to fix.
+      applyConvertToTrailingClosureFixit(TC, DC->getASTContext().SourceMgr,
+                                         Closure, AE->getArg());
+  }
+}
+
+
 //===----------------------------------------------------------------------===//
 // Per func/init diagnostics
 //===----------------------------------------------------------------------===//
@@ -3210,6 +3345,7 @@ void swift::performSyntacticExprDiagnostics(TypeChecker &TC, const Expr *E,
   diagRecursivePropertyAccess(TC, E, DC);
   diagnoseImplicitSelfUseInClosure(TC, E, DC);
   diagAvailability(TC, E, const_cast<DeclContext*>(DC));
+  diagConvertToTrailingClosure(TC, E, const_cast<DeclContext*>(DC));
   if (TC.Context.LangOpts.EnableObjCInterop)
     diagDeprecatedObjCSelectors(TC, DC, E);
 }

test/Sema/convert_trailing_closure.swift

@@ -0,0 +1,56 @@
+// RUN: %target-parse-verify-swift
+
+func bar(v : Int, c1 : () ->Void) {}
+func bar1(_ v : () ->Void) {}
+func bar2(v : Int, c1 : () ->Void) {}
+func bar2(v : Int, c2 : () ->Void) {}
+func bar3(v1 : Int, c1 : (Int) ->Void) {}
+func bar3(v2 : Int, c1 : (Int) ->Void) {}
+func bar4(c1 : (Int, Int, Int) ->Void) {}
+func bar4(c2: (Int, Int, Int) ->Void) {}
+func bar5(c1: (Int, Int, Int) ->Void) {}
+func bar6(c1 : () ->Void, v : Int) {}
+
+class C {
+  func bar(v : Int, c1 : () ->Void) {}
+  func bar1(_ v : () ->Void) {}
+  func bar2(v : Int, c1 : () ->Void) {}
+  func bar2(v : Int, c2 : () ->Void) {}
+  func bar3(v1 : Int, c1 : (Int) ->Void) {}
+  func bar3(v2 : Int, c1 : (Int) ->Void) {}
+}
+
+struct S {
+  func bar(v : Int, c1 : () ->Void) {}
+  func bar1(_ v : () ->Void) {}
+  func bar2(v : Int, c1 : () ->Void) {}
+  func bar2(v : Int, c2 : () ->Void) {}
+  func bar3(v1 : Int, c1 : (Int) ->Void) {}
+  func bar3(v2 : Int, c1 : (Int) ->Void) {}
+}
+
+func foo(c : C, s : S) {
+  bar(v : 3, c1: {}) // expected-note {{use trailing closure to simplify arguments}} {{12-19=) }} {{20-110=}}
+  bar1 ({}) // expected-note {{use trailing closure to simplify arguments}} {{8-10=}} {{11-97=}}
+  bar2(v : 3, c1: {}) // {{none}}
+  bar2(v : 3, c2: {}) // {{none}}
+  bar3(v1: 3, c1 : { (i) in }) // expected-note {{use trailing closure to simplify arguments}} {{13-21=) }} {{30-120=}}
+  bar3(v2: 3, c1 : { (i) in }) // expected-note {{use trailing closure to simplify arguments}} {{13-21=) }} {{30-120=}}
+  bar4(c1 : { (i, j, k) in }) // {{none}}
+  bar5(c1 : { (i, j, k) in }) // expected-note {{use trailing closure to simplify arguments}} {{7-14=}} {{29-116=}}
+  bar6(c1: {}, v: 3) // {{none}}
+
+  c.bar(v : 3, c1: {}) // expected-note {{use trailing closure to simplify arguments}} {{14-21=) }} {{22-112=}}
+  c.bar1({}) // expected-note {{use trailing closure to simplify arguments}} {{9-11=}} {{12-98=}}
+  c.bar2(v : 3, c1: {}) // {{none}}
+  c.bar2(v : 3, c2: {}) // {{none}}
+  c.bar3(v1: 3, c1 : { (i) in }) // expected-note {{use trailing closure to simplify arguments}} {{15-23=) }} {{32-122=}}
+  c.bar3(v2: 3, c1 : { (i) in }) // expected-note {{use trailing closure to simplify arguments}} {{15-23=) }} {{32-122=}}
+
+  s.bar(v : 3, c1: {}) // expected-note {{use trailing closure to simplify arguments}} {{14-21=) }} {{22-112=}}
+  s.bar1({}) // expected-note {{use trailing closure to simplify arguments}} {{9-11=}} {{12-98=}}
+  s.bar2(v : 3, c1: {}) // {{none}}
+  s.bar2(v : 3, c2: {}) // {{none}}
+  s.bar3(v1: 3, c1 : { (i) in }) // expected-note {{use trailing closure to simplify arguments}} {{15-23=) }} {{32-122=}}
+  s.bar3(v2: 3, c1 : { (i) in }) // expected-note {{use trailing closure to simplify arguments}} {{15-23=) }} {{32-122=}}
+}