Merge pull request #39091 from mininny/fix-rename-subscript

[Diagnostics] Fix diagnostics to produce valid rename fixit for SubscriptExpr

Author: xedin

include/swift/Parse/Parser.h

@@ -1869,6 +1869,8 @@ struct ParsedDeclName {
   /// Whether this is a setter for the named property.
   bool IsSetter = false;
 
+  bool IsSubscript = false;
+
   /// For a declaration name that makes the declaration into an
   /// instance member, the index of the "Self" parameter.
   Optional<unsigned> SelfIndex;

lib/Parse/Parser.cpp

@@ -1405,6 +1405,11 @@ ParsedDeclName swift::parseDeclName(StringRef name) {
     baseName = baseName.substr(7);
   }
 
+  // If the base name is prefixed by "subscript", it's an subscript.
+  if (baseName == "subscript") {
+    result.IsSubscript = true;
+  }
+
   // Parse the base name.
   if (parseBaseName(baseName)) return ParsedDeclName();
 

lib/Sema/TypeCheckAvailability.cpp

@@ -1855,7 +1855,7 @@ static void fixItAvailableAttrRename(InFlightDiagnostic &diag,
                                      SourceRange referenceRange,
                                      const ValueDecl *renamedDecl,
                                      const AvailableAttr *attr,
-                                     const ApplyExpr *call) {
+                                     const Expr *call) {
   if (isa<AccessorDecl>(renamedDecl))
     return;
 
@@ -1875,21 +1875,21 @@ static void fixItAvailableAttrRename(InFlightDiagnostic &diag,
 
   auto &ctx = renamedDecl->getASTContext();
   SourceManager &sourceMgr = ctx.SourceMgr;
-
   if (parsed.isInstanceMember()) {
+    auto *CE = dyn_cast_or_null<CallExpr>(call);
+    if (!CE)
+      return;
+
     // 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 (!call || !isa<CallExpr>(call))
-      return;
-
     unsigned selfIndex = parsed.SelfIndex.getValue();
     const Expr *selfExpr = nullptr;
     SourceLoc removeRangeStart;
     SourceLoc removeRangeEnd;
 
-    auto *argExpr = call->getArg();
+    auto *argExpr = CE->getArg();
     auto argList = getOriginalArgumentList(argExpr);
 
     size_t numElementsWithinParens = argList.args.size();
@@ -1986,54 +1986,91 @@ static void fixItAvailableAttrRename(InFlightDiagnostic &diag,
     selfReplace += base;
     if (needsParens)
       selfReplace.push_back(')');
+
     selfReplace.push_back('.');
     selfReplace += parsed.BaseName;
-    diag.fixItReplace(call->getFn()->getSourceRange(), selfReplace);
+
+    diag.fixItReplace(CE->getFn()->getSourceRange(), selfReplace);
 
     if (!parsed.isPropertyAccessor())
       diag.fixItRemoveChars(removeRangeStart, removeRangeEnd);
 
     // Continue on to diagnose any argument label renames.
 
   } else if (parsed.BaseName == "init" && isa_and_nonnull<CallExpr>(call)) {
+    auto *CE = cast<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(call->getFn()->getSourceRange(), parsed.ContextName);
-
-    } else if (auto *dotCall = dyn_cast<DotSyntaxCallExpr>(call->getFn())) {
+      diag.fixItReplace(CE->getFn()->getSourceRange(), parsed.ContextName);
+    } else if (auto *dotCall = dyn_cast<DotSyntaxCallExpr>(CE->getFn())) {
       SourceLoc removeLoc = dotCall->getDotLoc();
       if (removeLoc.isInvalid())
         return;
 
       diag.fixItRemove(SourceRange(removeLoc, dotCall->getFn()->getEndLoc()));
-    } else if (!isa<ConstructorRefCallExpr>(call->getFn())) {
+    } else if (!isa<ConstructorRefCallExpr>(CE->getFn())) {
       return;
     }
 
     // Continue on to diagnose any constructor argument label renames.
-    
+
+  } else if (parsed.IsSubscript) {
+    if (auto *CE = dyn_cast_or_null<CallExpr>(call)) {
+      // Renaming from CallExpr to SubscriptExpr. Remove function name and
+      // replace parens with square brackets.
+
+      if (auto *DSCE = dyn_cast<DotSyntaxCallExpr>(CE->getFn())) {
+        if (DSCE->getBase()->isImplicit()) {
+          // If self is implicit, self must be inserted before subscript syntax.
+          diag.fixItInsert(CE->getStartLoc(), "self");
+        }
+      }
+
+      diag.fixItReplace(CE->getFn()->getEndLoc(), "[");
+      diag.fixItReplace(CE->getEndLoc(), "]");
+    }
   } else {
     // Just replace the base name.
     SmallString<64> baseReplace;
+
     if (!parsed.ContextName.empty()) {
       baseReplace += parsed.ContextName;
       baseReplace += '.';
     }
     baseReplace += parsed.BaseName;
-    if (parsed.IsFunctionName && parsed.ArgumentLabels.empty() &&
-        isa<VarDecl>(renamedDecl)) {
-      // If we're going from a var to a function with no arguments, emit an
-      // empty parameter list.
-      baseReplace += "()";
+
+    if (parsed.IsFunctionName && isa_and_nonnull<SubscriptExpr>(call)) {
+      auto *SE = cast<SubscriptExpr>(call);
+
+      // Renaming from SubscriptExpr to CallExpr. Insert function name and
+      // replace square brackets with parens.
+      diag.fixItReplace(SE->getIndex()->getStartLoc(),
+                        ("." + baseReplace.str() + "(").str());
+      diag.fixItReplace(SE->getEndLoc(), ")");
+    } else {
+      if (parsed.IsFunctionName && parsed.ArgumentLabels.empty() &&
+          isa<VarDecl>(renamedDecl)) {
+        // If we're going from a var to a function with no arguments, emit an
+        // empty parameter list.
+        baseReplace += "()";
+      }
+      diag.fixItReplace(referenceRange, baseReplace);
     }
-    diag.fixItReplace(referenceRange, baseReplace);
   }
 
-  if (!call || !isa<CallExpr>(call))
+  if (!call)
+    return;
+
+  Expr *argExpr;
+  if (auto *CE = dyn_cast<CallExpr>(call))
+    argExpr = CE->getArg();
+  else if (auto *SE = dyn_cast<SubscriptExpr>(call))
+    argExpr = SE->getIndex();
+  else
     return;
 
-  auto *argExpr = call->getArg();
   auto argList = getOriginalArgumentList(argExpr);
 
   if (parsed.IsGetter) {
@@ -2146,7 +2183,8 @@ static void fixItAvailableAttrRename(InFlightDiagnostic &diag,
     if (argList.isUnlabeledTrailingClosureIdx(i))
       continue;
     if (argumentLabelIDs[i] != argList.labels[i]) {
-      diagnoseArgumentLabelError(ctx, argExpr, argumentLabelIDs, false, &diag);
+      diagnoseArgumentLabelError(ctx, argExpr, argumentLabelIDs,
+                                 parsed.IsSubscript, &diag);
       return;
     }
   }
@@ -2189,7 +2227,7 @@ describeRename(ASTContext &ctx, const AvailableAttr *attr, const ValueDecl *D,
     name << parsed.ContextName << '.';
 
   if (parsed.IsFunctionName) {
-    name << parsed.formDeclName(ctx);
+    name << parsed.formDeclName(ctx, (D && isa<SubscriptDecl>(D)));
   } else {
     name << parsed.BaseName;
   }
@@ -2206,7 +2244,7 @@ describeRename(ASTContext &ctx, const AvailableAttr *attr, const ValueDecl *D,
 void TypeChecker::diagnoseIfDeprecated(SourceRange ReferenceRange,
                                        const ExportContext &Where,
                                        const ValueDecl *DeprecatedDecl,
-                                       const ApplyExpr *Call) {
+                                       const Expr *Call) {
   const AvailableAttr *Attr = TypeChecker::getDeprecated(DeprecatedDecl);
   if (!Attr)
     return;
@@ -2390,10 +2428,9 @@ void swift::diagnoseUnavailableOverride(ValueDecl *override,
 
 /// Emit a diagnostic for references to declarations that have been
 /// marked as unavailable, either through "unavailable" or "obsoleted:".
-bool swift::diagnoseExplicitUnavailability(const ValueDecl *D,
-                                           SourceRange R,
+bool swift::diagnoseExplicitUnavailability(const ValueDecl *D, SourceRange R,
                                            const ExportContext &Where,
-                                           const ApplyExpr *call,
+                                           const Expr *call,
                                            DeclAvailabilityFlags Flags) {
   return diagnoseExplicitUnavailability(D, R, Where, Flags,
                                         [=](InFlightDiagnostic &diag) {
@@ -2760,7 +2797,7 @@ class ExprAvailabilityWalker : public ASTWalker {
       diagnoseDeclRefAvailability(DS->getMember(), DS->getSourceRange());
     if (auto S = dyn_cast<SubscriptExpr>(E)) {
       if (S->hasDecl()) {
-        diagnoseDeclRefAvailability(S->getDecl(), S->getSourceRange());
+        diagnoseDeclRefAvailability(S->getDecl(), S->getSourceRange(), S);
         maybeDiagStorageAccess(S->getDecl().getDecl(), S->getSourceRange(), DC);
       }
     }
@@ -2818,7 +2855,7 @@ class ExprAvailabilityWalker : public ASTWalker {
   }
 
   bool diagnoseDeclRefAvailability(ConcreteDeclRef declRef, SourceRange R,
-                                   const ApplyExpr *call = nullptr,
+                                   const Expr *call = nullptr,
                                    DeclAvailabilityFlags flags = None) const;
 
 private:
@@ -3001,9 +3038,8 @@ class ExprAvailabilityWalker : public ASTWalker {
 
 /// Diagnose uses of unavailable declarations. Returns true if a diagnostic
 /// was emitted.
-bool
-ExprAvailabilityWalker::diagnoseDeclRefAvailability(
-    ConcreteDeclRef declRef, SourceRange R, const ApplyExpr *call,
+bool ExprAvailabilityWalker::diagnoseDeclRefAvailability(
+    ConcreteDeclRef declRef, SourceRange R, const Expr *call,
     DeclAvailabilityFlags Flags) const {
   if (!declRef)
     return false;
@@ -3012,7 +3048,8 @@ ExprAvailabilityWalker::diagnoseDeclRefAvailability(
   if (auto *attr = AvailableAttr::isUnavailable(D)) {
     if (diagnoseIncDecRemoval(D, R, attr))
       return true;
-    if (call && diagnoseMemoryLayoutMigration(D, R, attr, call))
+    if (isa_and_nonnull<ApplyExpr>(call) &&
+        diagnoseMemoryLayoutMigration(D, R, attr, cast<ApplyExpr>(call)))
       return true;
   }
 
@@ -3030,12 +3067,10 @@ ExprAvailabilityWalker::diagnoseDeclRefAvailability(
 
 /// Diagnose uses of unavailable declarations. Returns true if a diagnostic
 /// was emitted.
-bool
-swift::diagnoseDeclAvailability(const ValueDecl *D,
-                                SourceRange R,
-                                const ApplyExpr *call,
-                                const ExportContext &Where,
-                                DeclAvailabilityFlags Flags) {
+bool swift::diagnoseDeclAvailability(const ValueDecl *D, SourceRange R,
+                                     const Expr *call,
+                                     const ExportContext &Where,
+                                     DeclAvailabilityFlags Flags) {
   assert(!Where.isImplicit());
 
   // Generic parameters are always available.
@@ -3095,7 +3130,6 @@ swift::diagnoseDeclAvailability(const ValueDecl *D,
   return false;
 }
 
-
 /// Return true if the specified type looks like an integer of floating point
 /// type.
 static bool isIntegerOrFloatingPointType(Type ty, ModuleDecl *M) {

lib/Sema/TypeCheckAvailability.h

@@ -230,10 +230,8 @@ diagnoseSubstitutionMapAvailability(SourceLoc loc,
 
 /// Diagnose uses of unavailable declarations. Returns true if a diagnostic
 /// was emitted.
-bool diagnoseDeclAvailability(const ValueDecl *D,
-                              SourceRange R,
-                              const ApplyExpr *call,
-                              const ExportContext &where,
+bool diagnoseDeclAvailability(const ValueDecl *D, SourceRange R,
+                              const Expr *call, const ExportContext &where,
                               DeclAvailabilityFlags flags = None);
 
 void diagnoseUnavailableOverride(ValueDecl *override,
@@ -242,10 +240,9 @@ void diagnoseUnavailableOverride(ValueDecl *override,
 
 /// Emit a diagnostic for references to declarations that have been
 /// marked as unavailable, either through "unavailable" or "obsoleted:".
-bool diagnoseExplicitUnavailability(const ValueDecl *D,
-                                    SourceRange R,
+bool diagnoseExplicitUnavailability(const ValueDecl *D, SourceRange R,
                                     const ExportContext &Where,
-                                    const ApplyExpr *call,
+                                    const Expr *call,
                                     DeclAvailabilityFlags Flags = None);
 
 /// Emit a diagnostic for references to declarations that have been

lib/Sema/TypeChecker.h

@@ -1049,10 +1049,8 @@ void diagnosePotentialAccessorUnavailability(
 const AvailableAttr *getDeprecated(const Decl *D);
 
 /// Emits a diagnostic for a reference to a declaration that is deprecated.
-void diagnoseIfDeprecated(SourceRange SourceRange,
-                          const ExportContext &Where,
-                          const ValueDecl *DeprecatedDecl,
-                          const ApplyExpr *Call);
+void diagnoseIfDeprecated(SourceRange SourceRange, const ExportContext &Where,
+                          const ValueDecl *DeprecatedDecl, const Expr *Call);
 
 /// Emits a diagnostic for a reference to a conformnace that is deprecated.
 bool diagnoseIfDeprecated(SourceLoc loc,

test/attr/attr_availability.swift

@@ -1179,3 +1179,55 @@ func testMultipleTrailingClosures(_ x: TypeWithTrailingClosures) {
   // expected-note@-1 {{use 'TypeWithTrailingClosures.variadicTrailingClosures(a:b:c:)' instead}} {{3-27=x.variadicTrailingClosures}} {{28-29=}} {{none}}
   x.variadicTrailingClosures() {} _: {} _: {}
 }
+
+struct UnavailableSubscripts { 
+  @available(*, unavailable, renamed: "subscript(new:)")
+  subscript(old index: Int) -> Int { 3 } // expected-note * {{'subscript(old:)' has been explicitly marked unavailable here}}
+  @available(*, unavailable, renamed: "subscript(new:)")
+  func getValue(old: Int) -> Int { 3 } // expected-note * {{'getValue(old:)' has been explicitly marked unavailable here}}
+
+  subscript(new index: Int) -> Int { 3 }
+
+  @available(*, unavailable, renamed: "getAValue(new:)")
+  subscript(getAValue index: Int) -> Int { 3 } // expected-note * {{'subscript(getAValue:)' has been explicitly marked unavailable here}}
+  func getAValue(new: Int) -> Int { 3 }
+
+  @available(*, unavailable, renamed: "subscript(arg1:arg2:arg3:)")
+  subscript(_ argg1: Int, _ argg2: Int, _ argg3: Int) -> Int { 3 } // expected-note * {{'subscript(_:_:_:)' has been explicitly marked unavailable here}}
+
+  @available(*, deprecated, renamed: "subscript(arg1:arg2:arg3:)")
+  subscript(argg1 argg1: Int, argg2 argg2: Int, argg3 argg3: Int) -> Int { 3 }
+
+  @available(*, deprecated, renamed: "subscript(arg1:arg2:arg3:)")
+  subscript(only1 only1: Int, only2 only2: Int) -> Int { 3 }
+
+  subscript(arg1 arg1: Int, arg2 arg2: Int, arg3 arg3: Int) -> Int { 3 }
+
+  @available(*, deprecated, renamed: "subscriptTo(_:)")
+  subscript(to to: Int) -> Int { 3 }
+  func subscriptTo(_ index: Int) -> Int { 3 }
+
+  func testUnavailableSubscripts(_ x: UnavailableSubscripts) { 
+    _ = self[old: 3] // expected-error {{'subscript(old:)' has been renamed to 'subscript(new:)'}} {{14-17=new}}
+    _ = x[old: 3] // expected-error {{'subscript(old:)' has been renamed to 'subscript(new:)'}} {{11-14=new}}
+
+    _ = self.getValue(old: 3) // expected-error {{'getValue(old:)' has been renamed to 'subscript(new:)'}} {{14-22=[}} {{29-30=]}} {{23-26=new}}
+    _ = getValue(old: 3) // expected-error {{'getValue(old:)' has been renamed to 'subscript(new:)'}} {{9-9=self}} {{9-17=[}} {{24-25=]}} {{18-21=new}}
+    _ = x.getValue(old: 3) // expected-error {{'getValue(old:)' has been renamed to 'subscript(new:)'}} {{11-19=[}} {{26-27=]}} {{20-23=new}}
+
+    _ = self[getAValue: 3] // expected-error {{'subscript(getAValue:)' has been renamed to 'getAValue(new:)'}} {{13-14=.getAValue(}} {{26-27=)}} {{14-23=new}}
+    _ = x[getAValue: 3] // expected-error {{'subscript(getAValue:)' has been renamed to 'getAValue(new:)'}} {{10-11=.getAValue(}} {{23-24=)}} {{11-20=new}}
+
+    _ = self[argg1: 3, argg2: 3, argg3: 3] // expected-warning {{'subscript(argg1:argg2:argg3:)' is deprecated: renamed to 'subscript(arg1:arg2:arg3:)'}} // expected-note {{use 'subscript(arg1:arg2:arg3:)' instead}} {{14-19=arg1}} {{24-29=arg2}} {{34-39=arg3}}
+    _ = x[argg1: 3, argg2: 3, argg3: 3] // expected-warning {{'subscript(argg1:argg2:argg3:)' is deprecated: renamed to 'subscript(arg1:arg2:arg3:)'}} // expected-note {{use 'subscript(arg1:arg2:arg3:)' instead}} {{11-16=arg1}} {{21-26=arg2}} {{31-36=arg3}}
+
+    // Different number of parameters emit no fixit
+    _ = self[only1: 3, only2: 3] // expected-warning {{'subscript(only1:only2:)' is deprecated: renamed to 'subscript(arg1:arg2:arg3:)'}} // expected-note {{use 'subscript(arg1:arg2:arg3:)' instead}} {{none}}
+
+    _ = self[3, 3, 3] // expected-error {{'subscript(_:_:_:)' has been renamed to 'subscript(arg1:arg2:arg3:)'}} {{14-14=arg1: }} {{17-17=arg2: }} {{20-20=arg3: }}
+    _ = x[3, 3, 3] // expected-error {{'subscript(_:_:_:)' has been renamed to 'subscript(arg1:arg2:arg3:)'}} {{11-11=arg1: }} {{14-14=arg2: }} {{17-17=arg3: }}
+
+    _ = self[to: 3] // expected-warning {{'subscript(to:)' is deprecated: renamed to 'subscriptTo(_:)'}} // expected-note {{use 'subscriptTo(_:)' instead}} {{13-14=.subscriptTo(}} {{19-20=)}} {{14-18=}}
+    _ = x[to: 3] // expected-warning {{'subscript(to:)' is deprecated: renamed to 'subscriptTo(_:)'}} // expected-note {{use 'subscriptTo(_:)' instead}} {{10-11=.subscriptTo(}} {{16-17=)}} {{11-15=}}
+  }
+}