Correct renaming fix-its for trailing closures, disable them for operators (#2626)

Merge pull request #2626 from jrose-apple/trailing-closure-rename-fixits

Author: jrose-apple

lib/Sema/MiscDiagnostics.cpp

@@ -1006,36 +1006,48 @@ void swift::fixItAvailableAttrRename(TypeChecker &TC,
                                      InFlightDiagnostic &diag,
                                      SourceRange referenceRange,
                                      const AvailableAttr *attr,
-                                     const CallExpr *CE) {
+                                     const ApplyExpr *call) {
   ParsedDeclName parsed = swift::parseDeclName(attr->Rename);
   if (!parsed)
     return;
 
+  bool originallyWasKnownOperatorExpr = false;
+  if (call) {
+    originallyWasKnownOperatorExpr =
+        isa<BinaryExpr>(call) ||
+        isa<PrefixUnaryExpr>(call) ||
+        isa<PostfixUnaryExpr>(call);
+  }
+  if (parsed.isOperator() != originallyWasKnownOperatorExpr)
+    return;
+
   SourceManager &sourceMgr = TC.Context.SourceMgr;
 
   if (parsed.isInstanceMember()) {
     // Replace the base of the call with the "self argument".
     // We can only do a good job with the fix-it if we have the whole call
     // expression.
     // FIXME: Should we be validating the ContextName in some way?
-    if (!CE)
+    if (!dyn_cast_or_null<CallExpr>(call))
       return;
 
     unsigned selfIndex = parsed.SelfIndex.getValue();
     const Expr *selfExpr = nullptr;
     SourceLoc removeRangeStart;
     SourceLoc removeRangeEnd;
 
-    const Expr *argExpr = CE->getArg();
+    const Expr *argExpr = call->getArg();
     if (auto args = dyn_cast<TupleExpr>(argExpr)) {
-      if (selfIndex >= args->getNumElements())
+      size_t numElementsWithinParens = args->getNumElements();
+      numElementsWithinParens -= args->hasTrailingClosure();
+      if (selfIndex >= numElementsWithinParens)
         return;
 
       if (parsed.IsGetter) {
-        if (args->getNumElements() != 1)
+        if (numElementsWithinParens != 1)
           return;
       } else if (parsed.IsSetter) {
-        if (args->getNumElements() != 2)
+        if (numElementsWithinParens != 2)
           return;
       } else {
         if (parsed.ArgumentLabels.size() != args->getNumElements() - 1)
@@ -1044,7 +1056,7 @@ void swift::fixItAvailableAttrRename(TypeChecker &TC,
 
       selfExpr = args->getElement(selfIndex);
 
-      if (selfIndex + 1 == args->getNumElements()) {
+      if (selfIndex + 1 == numElementsWithinParens) {
         if (selfIndex > 0) {
           // Remove from the previous comma to the close-paren (half-open).
           removeRangeStart = args->getElement(selfIndex-1)->getEndLoc();
@@ -1055,7 +1067,12 @@ void swift::fixItAvailableAttrRename(TypeChecker &TC,
           removeRangeStart = Lexer::getLocForEndOfToken(sourceMgr,
                                                         argExpr->getStartLoc());
         }
-        removeRangeEnd = args->getEndLoc();
+
+        // Prefer the r-paren location, so that we get the right behavior when
+        // there's a trailing closure, but handle some implicit cases too.
+        removeRangeEnd = args->getRParenLoc();
+        if (removeRangeEnd.isInvalid())
+          removeRangeEnd = args->getEndLoc();
 
       } else {
         // Remove from the label to the start of the next argument (half-open).
@@ -1108,21 +1125,22 @@ void swift::fixItAvailableAttrRename(TypeChecker &TC,
       selfReplace.push_back(')');
     selfReplace.push_back('.');
     selfReplace += parsed.BaseName;
-    diag.fixItReplace(CE->getFn()->getSourceRange(), selfReplace);
+    diag.fixItReplace(call->getFn()->getSourceRange(), selfReplace);
 
     if (!parsed.isPropertyAccessor())
       diag.fixItRemoveChars(removeRangeStart, removeRangeEnd);
 
     // Continue on to diagnose any argument label renames.
 
-  } else if (parsed.BaseName == TC.Context.Id_init.str() && CE) {
+  } else if (parsed.BaseName == TC.Context.Id_init.str() &&
+             dyn_cast_or_null<CallExpr>(call)) {
     // For initializers, replace with a "call" of the context type...but only
     // if we know we're doing a call (rather than a first-class reference).
     if (parsed.isMember()) {
-      diag.fixItReplace(CE->getFn()->getSourceRange(), parsed.ContextName);
+      diag.fixItReplace(call->getFn()->getSourceRange(), parsed.ContextName);
 
     } else {
-      auto *dotCall = dyn_cast<DotSyntaxCallExpr>(CE->getFn());
+      auto *dotCall = dyn_cast<DotSyntaxCallExpr>(call->getFn());
       if (!dotCall)
         return;
 
@@ -1144,10 +1162,10 @@ void swift::fixItAvailableAttrRename(TypeChecker &TC,
     diag.fixItReplace(referenceRange, baseReplace);
   }
 
-  if (!CE)
+  if (!dyn_cast_or_null<CallExpr>(call))
     return;
 
-  const Expr *argExpr = CE->getArg();
+  const Expr *argExpr = call->getArg();
   if (parsed.IsGetter) {
     diag.fixItRemove(argExpr->getSourceRange());
     return;
@@ -1290,7 +1308,7 @@ void TypeChecker::diagnoseDeprecated(SourceRange ReferenceRange,
                                      const DeclContext *ReferenceDC,
                                      const AvailableAttr *Attr,
                                      DeclName Name,
-                                     const CallExpr *CE) {
+                                     const ApplyExpr *Call) {
   // We match the behavior of clang to not report deprecation warnings
   // inside declarations that are themselves deprecated on all deployment
   // targets.
@@ -1347,7 +1365,7 @@ void TypeChecker::diagnoseDeprecated(SourceRange ReferenceRange,
     auto renameDiag = diagnose(ReferenceRange.Start,
                                diag::note_deprecated_rename,
                                newName);
-    fixItAvailableAttrRename(*this, renameDiag, ReferenceRange, Attr, CE);
+    fixItAvailableAttrRename(*this, renameDiag, ReferenceRange, Attr, Call);
   }
 }
 
@@ -1400,11 +1418,11 @@ void TypeChecker::diagnoseUnavailableOverride(ValueDecl *override,
 bool TypeChecker::diagnoseExplicitUnavailability(const ValueDecl *D,
                                                  SourceRange R,
                                                  const DeclContext *DC,
-                                                 const CallExpr *CE) {
+                                                 const ApplyExpr *call) {
   return diagnoseExplicitUnavailability(D, R, DC,
                                         [=](InFlightDiagnostic &diag) {
     fixItAvailableAttrRename(*this, diag, R, AvailableAttr::isUnavailable(D),
-                             CE);
+                             call);
   });
 }
 
@@ -1539,19 +1557,19 @@ class AvailabilityWalker : public ASTWalker {
 
     if (auto DR = dyn_cast<DeclRefExpr>(E))
       diagAvailability(DR->getDecl(), DR->getSourceRange(),
-                       getEnclosingCallExpr());
+                       getEnclosingApplyExpr());
     if (auto MR = dyn_cast<MemberRefExpr>(E)) {
       walkMemberRef(MR);
       return skipChildren();
     }
     if (auto OCDR = dyn_cast<OtherConstructorDeclRefExpr>(E))
       diagAvailability(OCDR->getDecl(),
                        OCDR->getConstructorLoc().getSourceRange(),
-                       getEnclosingCallExpr());
+                       getEnclosingApplyExpr());
     if (auto DMR = dyn_cast<DynamicMemberRefExpr>(E))
       diagAvailability(DMR->getMember().getDecl(),
                        DMR->getNameLoc().getSourceRange(),
-                       getEnclosingCallExpr());
+                       getEnclosingApplyExpr());
     if (auto DS = dyn_cast<DynamicSubscriptExpr>(E))
       diagAvailability(DS->getMember().getDecl(), DS->getSourceRange());
     if (auto S = dyn_cast<SubscriptExpr>(E)) {
@@ -1579,12 +1597,12 @@ class AvailabilityWalker : public ASTWalker {
 
 private:
   bool diagAvailability(const ValueDecl *D, SourceRange R,
-                        const CallExpr *CE = nullptr);
+                        const ApplyExpr *call = nullptr);
   bool diagnoseIncDecRemoval(const ValueDecl *D, SourceRange R,
                              const AvailableAttr *Attr);
 
-  /// Walks up from a potential callee to the enclosing CallExpr.
-  const CallExpr *getEnclosingCallExpr() const {
+  /// Walks up from a potential callee to the enclosing ApplyExpr.
+  const ApplyExpr *getEnclosingApplyExpr() const {
     ArrayRef<const Expr *> parents = ExprStack;
     assert(!parents.empty() && "must be called while visiting an expression");
     size_t idx = parents.size() - 1;
@@ -1599,7 +1617,7 @@ class AvailabilityWalker : public ASTWalker {
              isa<ForceValueExpr>(parents[idx]) || // f!(a)
              isa<BindOptionalExpr>(parents[idx])); // f?(a)
 
-    auto *call = dyn_cast<CallExpr>(parents[idx]);
+    auto *call = dyn_cast<ApplyExpr>(parents[idx]);
     if (!call || call->getFn() != parents[idx+1])
       return nullptr;
     return call;
@@ -1716,20 +1734,20 @@ class AvailabilityWalker : public ASTWalker {
 /// Diagnose uses of unavailable declarations. Returns true if a diagnostic
 /// was emitted.
 bool AvailabilityWalker::diagAvailability(const ValueDecl *D, SourceRange R,
-                                          const CallExpr *CE) {
+                                          const ApplyExpr *call) {
   if (!D)
     return false;
 
   if (auto *attr = AvailableAttr::isUnavailable(D))
     if (diagnoseIncDecRemoval(D, R, attr))
       return true;
 
-  if (TC.diagnoseExplicitUnavailability(D, R, DC, CE))
+  if (TC.diagnoseExplicitUnavailability(D, R, DC, call))
     return true;
 
   // Diagnose for deprecation
   if (const AvailableAttr *Attr = TypeChecker::getDeprecated(D)) {
-    TC.diagnoseDeprecated(R, DC, Attr, D->getFullName(), CE);
+    TC.diagnoseDeprecated(R, DC, Attr, D->getFullName(), call);
   }
 
   if (TC.getLangOpts().DisableAvailabilityChecking)

lib/Sema/MiscDiagnostics.h

@@ -22,8 +22,8 @@
 
 namespace swift {
   class AbstractFunctionDecl;
+  class ApplyExpr;
   class AvailableAttr;
-  class CallExpr;
   class DeclContext;
   class Expr;
   class InFlightDiagnostic;
@@ -61,13 +61,13 @@ bool diagnoseArgumentLabelError(TypeChecker &TC, const Expr *expr,
                                 InFlightDiagnostic *existingDiag = nullptr);
 
 /// Emit fix-its to rename the base name at \p referenceRange based on the
-/// "renamed" argument in \p attr. If \p CE is provided, the argument labels
+/// "renamed" argument in \p attr. If \p call is provided, the argument labels
 /// will also be updated.
 void fixItAvailableAttrRename(TypeChecker &TC,
                               InFlightDiagnostic &diag,
                               SourceRange referenceRange,
                               const AvailableAttr *attr,
-                              const CallExpr *CE);
+                              const ApplyExpr *call);
 } // namespace swift
 
 #endif // SWIFT_SEMA_MISC_DIAGNOSTICS_H

lib/Sema/TypeChecker.h

@@ -1616,7 +1616,7 @@ class TypeChecker final : public LazyResolver {
   bool diagnoseExplicitUnavailability(const ValueDecl *D,
                                       SourceRange R,
                                       const DeclContext *DC,
-                                      const CallExpr *CE);
+                                      const ApplyExpr *call);
 
   /// Emit a diagnostic for references to declarations that have been
   /// marked as unavailable, either through "unavailable" or "obsoleted:".
@@ -1829,7 +1829,7 @@ class TypeChecker final : public LazyResolver {
                           const DeclContext *ReferenceDC,
                           const AvailableAttr *Attr,
                           DeclName Name,
-                          const CallExpr *CE);
+                          const ApplyExpr *Call);
   /// @}
 
   /// If LangOptions::DebugForbidTypecheckPrefix is set and the given decl

test/ClangModules/attr-swift_name_renaming.swift

@@ -37,7 +37,7 @@ func test() {
   // This particular instance method mapping previously caused a crash because
   // of the trailing closure.
   acceptsClosure(Foo(), test) // expected-error {{'acceptsClosure' has been replaced by instance method 'Foo.accepts(closure:)'}} {{3-17=(Foo()).accepts}} {{18-25=}} {{25-25=closure: }}
-  acceptsClosure(Foo()) {} // expected-error {{'acceptsClosure' has been replaced by instance method 'Foo.accepts(closure:)'}} {{3-17=(Foo()).accepts}} {{18-25=}}
+  acceptsClosure(Foo()) {} // expected-error {{'acceptsClosure' has been replaced by instance method 'Foo.accepts(closure:)'}} {{3-17=(Foo()).accepts}} {{18-23=}}
 
   Foo().accepts(closure: test)
   Foo().accepts() {}

test/attr/attr_availability.swift

@@ -479,6 +479,52 @@ func testRenameSetters() {
   unavailableSetInstancePropertyInout(a: &x, b: 2) // expected-error{{'unavailableSetInstancePropertyInout(a:b:)' has been replaced by property 'Int.prop'}} {{3-38=x.prop}} {{38-49= = }} {{50-51=}}
 }
 
+@available(*, unavailable, renamed: "Int.foo(self:execute:)")
+func trailingClosure(_ value: Int, fn: () -> Void) {} // expected-note {{here}}
+@available(*, unavailable, renamed: "Int.foo(self:bar:execute:)")
+func trailingClosureArg(_ value: Int, _ other: Int, fn: () -> Void) {} // expected-note {{here}}
+@available(*, unavailable, renamed: "Int.foo(bar:self:execute:)")
+func trailingClosureArg2(_ value: Int, _ other: Int, fn: () -> Void) {} // expected-note {{here}}
+
+func testInstanceTrailingClosure() {
+  trailingClosure(0) {} // expected-error {{'trailingClosure(_:fn:)' has been replaced by instance method 'Int.foo(execute:)'}} {{3-18=0.foo}} {{19-20=}}
+  trailingClosureArg(0, 1) {} // expected-error {{'trailingClosureArg(_:_:fn:)' has been replaced by instance method 'Int.foo(bar:execute:)'}} {{3-21=0.foo}} {{22-25=}} {{25-25=bar: }}
+  trailingClosureArg2(0, 1) {} // expected-error {{'trailingClosureArg2(_:_:fn:)' has been replaced by instance method 'Int.foo(bar:execute:)'}} {{3-22=1.foo}} {{23-23=bar: }} {{24-27=}}
+}
+
+@available(*, unavailable, renamed: "+")
+func add(_ value: Int, _ other: Int) {} // expected-note {{here}}
+
+infix operator *** {}
+@available(*, unavailable, renamed: "add")
+func ***(value: (), other: ()) {} // expected-note {{here}}
+@available(*, unavailable, renamed: "Int.foo(self:_:)")
+func ***(value: Int, other: Int) {} // expected-note {{here}}
+
+prefix operator *** {}
+@available(*, unavailable, renamed: "add")
+prefix func ***(value: Int?) {} // expected-note {{here}}
+@available(*, unavailable, renamed: "Int.foo(self:)")
+prefix func ***(value: Int) {} // expected-note {{here}}
+
+postfix operator *** {}
+@available(*, unavailable, renamed: "add")
+postfix func ***(value: Int?) {} // expected-note {{here}}
+@available(*, unavailable, renamed: "Int.foo(self:)")
+postfix func ***(value: Int) {} // expected-note {{here}}
+
+func testOperators() {
+  add(0, 1) // expected-error {{'add' has been renamed to '+'}} {{none}}
+  () *** () // expected-error {{'***' has been renamed to 'add'}} {{none}}
+  0 *** 1 // expected-error {{'***' has been replaced by instance method 'Int.foo(_:)'}} {{none}}
+
+  ***nil // expected-error {{'***' has been renamed to 'add'}} {{none}}
+  ***0 // expected-error {{'***' has been replaced by instance method 'Int.foo()'}} {{none}}
+  
+  nil*** // expected-error {{'***' has been renamed to 'add'}} {{none}}
+  0*** // expected-error {{'***' has been replaced by instance method 'Int.foo()'}} {{none}}
+}
+
 extension Int {
   @available(*, unavailable, renamed: "init(other:)")
   @discardableResult