Merge pull request #13130 from nathawes/rdar31378113-rename-of-swift-subscript-not-working

[Refactoring] Fix subscript rename

Author: Nathan Hawes

include/swift/IDE/Utils.h

@@ -255,6 +255,7 @@ enum class LabelRangeType {
   None,
   CallArg,    // foo([a: ]2) or .foo([a: ]String)
   Param,  // func([a b]: Int)
+  NoncollapsibleParam, // subscript([a a]: Int)
   Selector,   // #selector(foo.func([a]:))
 };
 
@@ -500,14 +501,15 @@ enum class RegionType {
 };
 
 enum class RefactoringRangeKind {
-  BaseName,              // func [foo](a b: Int)
-  KeywordBaseName,       // [init](a: Int)
-  ParameterName,         // func foo(a [b]: Int)
-  DeclArgumentLabel,     // func foo([a] b: Int)
-  CallArgumentLabel,     // foo([a]: 1)
-  CallArgumentColon,     // foo(a[: ]1)
-  CallArgumentCombined,  // foo([]1) could expand to foo([a: ]1)
-  SelectorArgumentLabel, // foo([a]:)
+  BaseName,                    // func [foo](a b: Int)
+  KeywordBaseName,             // [init](a: Int)
+  ParameterName,               // func foo(a[ b]: Int)
+  NoncollapsibleParameterName, // subscript(a[ a]: Int)
+  DeclArgumentLabel,           // func foo([a] b: Int)
+  CallArgumentLabel,           // foo([a]: 1)
+  CallArgumentColon,           // foo(a[: ]1)
+  CallArgumentCombined,        // foo([]1) could expand to foo([a: ]1)
+  SelectorArgumentLabel,       // foo([a]:)
 };
 
 struct NoteRegion {

lib/IDE/Refactoring.cpp

@@ -118,8 +118,7 @@ class Renamer {
     size_t Index = 0;
     for (const auto &LabelRange : LabelRanges) {
       assert(LabelRange.isValid());
-
-      if (!labelRangeMatches(LabelRange, OldLabels[Index]))
+      if (!labelRangeMatches(LabelRange, RangeType, OldLabels[Index]))
         return true;
       splitAndRenameLabel(LabelRange, RangeType, Index++);
     }
@@ -135,7 +134,9 @@ class Renamer {
     case LabelRangeType::CallArg:
       return splitAndRenameCallArg(Range, NameIndex);
     case LabelRangeType::Param:
-      return splitAndRenameParamLabel(Range, NameIndex);
+      return splitAndRenameParamLabel(Range, NameIndex, /*IsCollapsible=*/true);
+    case LabelRangeType::NoncollapsibleParam:
+      return splitAndRenameParamLabel(Range, NameIndex, /*IsCollapsible=*/false);
     case LabelRangeType::Selector:
       return doRenameLabel(
           Range, RefactoringRangeKind::SelectorArgumentLabel, NameIndex);
@@ -144,28 +145,47 @@ class Renamer {
     }
   }
 
-  void splitAndRenameParamLabel(CharSourceRange Range, size_t NameIndex) {
+  void splitAndRenameParamLabel(CharSourceRange Range, size_t NameIndex, bool IsCollapsible) {
     // Split parameter range foo([a b]: Int) into decl argument label [a] and
-    // parameter name [b].  If we have only foo([a]: Int), then we add an empty
-    // range for the local name.
+    // parameter name [b] or noncollapsible parameter name [b] if IsCollapsible
+    // is false (as for subscript decls). If we have only foo([a]: Int), then we
+    // add an empty range for the local name, or for the decl argument label if
+    // IsCollapsible is false.
     StringRef Content = Range.str();
     size_t ExternalNameEnd = Content.find_first_of(" \t\n\v\f\r/");
-    ExternalNameEnd =
-        ExternalNameEnd == StringRef::npos ? Content.size() : ExternalNameEnd;
-
-    CharSourceRange Ext{Range.getStart(), unsigned(ExternalNameEnd)};
-    doRenameLabel(Ext, RefactoringRangeKind::DeclArgumentLabel, NameIndex);
 
-    size_t LocalNameStart = Content.find_last_of(" \t\n\v\f\r/");
-    LocalNameStart =
-        LocalNameStart == StringRef::npos ? ExternalNameEnd : LocalNameStart;
-    // Note: we consider the leading whitespace part of the parameter name since
-    // when the parameter is removed we want to remove the whitespace too.
-    // FIXME: handle comments foo(a /*...*/b: Int).
-    auto LocalLoc = Range.getStart().getAdvancedLocOrInvalid(LocalNameStart);
-    assert(LocalLoc.isValid());
-    CharSourceRange Local{LocalLoc, unsigned(Content.size() - LocalNameStart)};
-    doRenameLabel(Local, RefactoringRangeKind::ParameterName, NameIndex);
+    if (ExternalNameEnd == StringRef::npos) { // foo([a]: Int)
+      if (IsCollapsible) {
+        doRenameLabel(Range, RefactoringRangeKind::DeclArgumentLabel, NameIndex);
+        doRenameLabel(CharSourceRange{Range.getEnd(), 0},
+                      RefactoringRangeKind::ParameterName, NameIndex);
+      } else {
+        doRenameLabel(CharSourceRange{Range.getStart(), 0},
+                      RefactoringRangeKind::DeclArgumentLabel, NameIndex);
+        doRenameLabel(Range, RefactoringRangeKind::NoncollapsibleParameterName,
+                      NameIndex);
+      }
+    } else { // foo([a b]: Int)
+      CharSourceRange Ext{Range.getStart(), unsigned(ExternalNameEnd)};
+
+      // Note: we consider the leading whitespace part of the parameter name
+      // if the parameter is collapsible, since if the parameter is collapsed
+      // into a matching argument label, we want to remove the whitespace too.
+      // FIXME: handle comments foo(a /*...*/b: Int).
+      size_t LocalNameStart = Content.find_last_of(" \t\n\v\f\r/");
+      assert(LocalNameStart != StringRef::npos);
+      if (!IsCollapsible)
+        ++LocalNameStart;
+      auto LocalLoc = Range.getStart().getAdvancedLocOrInvalid(LocalNameStart);
+      CharSourceRange Local{LocalLoc, unsigned(Content.size() - LocalNameStart)};
+
+      doRenameLabel(Ext, RefactoringRangeKind::DeclArgumentLabel, NameIndex);
+      if (IsCollapsible) {
+        doRenameLabel(Local, RefactoringRangeKind::ParameterName, NameIndex);
+      } else {
+        doRenameLabel(Local, RefactoringRangeKind::NoncollapsibleParameterName, NameIndex);
+      }
+    }
   }
 
   void splitAndRenameCallArg(CharSourceRange Range, size_t NameIndex) {
@@ -192,14 +212,24 @@ class Renamer {
     doRenameLabel(Rest, RefactoringRangeKind::CallArgumentColon, NameIndex);
   }
 
-  bool labelRangeMatches(CharSourceRange Range, StringRef Expected) {
+  bool labelRangeMatches(CharSourceRange Range, LabelRangeType RangeType, StringRef Expected) {
     if (Range.getByteLength()) {
-      StringRef ExistingLabel = Lexer::getCharSourceRangeFromSourceRange(SM,
-        Range.getStart()).str();
-      if (!Expected.empty())
-        return Expected == ExistingLabel;
-      else
-        return ExistingLabel == "_";
+      CharSourceRange ExistingLabelRange =
+          Lexer::getCharSourceRangeFromSourceRange(SM, Range.getStart());
+      StringRef ExistingLabel = ExistingLabelRange.str();
+
+      switch (RangeType) {
+      case LabelRangeType::NoncollapsibleParam:
+        if (ExistingLabelRange == Range && Expected.empty()) // subscript([x]: Int)
+          return true;
+        LLVM_FALLTHROUGH;
+      case LabelRangeType::CallArg:
+      case LabelRangeType::Param:
+      case LabelRangeType::Selector:
+        return ExistingLabel == (Expected.empty() ? "_" : Expected);
+      case LabelRangeType::None:
+        llvm_unreachable("Unhandled label range type");
+      }
     }
     return Expected.empty();
   }
@@ -238,7 +268,7 @@ class Renamer {
       if (NameIndex >= OldNames.size())
         return true;
 
-      while (!labelRangeMatches(Label, OldNames[NameIndex])) {
+      while (!labelRangeMatches(Label, RangeType, OldNames[NameIndex])) {
         if (++NameIndex >= OldNames.size())
           return true;
       };
@@ -279,13 +309,16 @@ class Renamer {
 
     assert(Config.Usage != NameUsage::Call || Config.IsFunctionLike);
 
-    bool isKeywordBase = Old.base() == "init" || Old.base() == "subscript";
+    // FIXME: handle escaped keyword names `init`
+    bool IsSubscript = Old.base() == "subscript" && Config.IsFunctionLike;
+    bool IsInit = Old.base() == "init" && Config.IsFunctionLike;
+    bool IsKeywordBase = IsInit || IsSubscript;
 
-    if (!Config.IsFunctionLike || !isKeywordBase) {
+    if (!Config.IsFunctionLike || !IsKeywordBase) {
       if (renameBase(Resolved.Range, RefactoringRangeKind::BaseName))
         return RegionType::Mismatch;
 
-    } else if (isKeywordBase && Config.Usage == NameUsage::Definition) {
+    } else if (IsKeywordBase && Config.Usage == NameUsage::Definition) {
       if (renameBase(Resolved.Range, RefactoringRangeKind::KeywordBaseName))
         return RegionType::Mismatch;
     }
@@ -300,7 +333,7 @@ class Renamer {
         HandleLabels = true;
         break;
       case NameUsage::Reference:
-        HandleLabels = Resolved.LabelType == LabelRangeType::Selector;
+        HandleLabels = Resolved.LabelType == LabelRangeType::Selector || IsSubscript;
         break;
       case NameUsage::Unknown:
         HandleLabels = Resolved.LabelType != LabelRangeType::None;
@@ -315,7 +348,7 @@ class Renamer {
 
     if (HandleLabels) {
       bool isCallSite = Config.Usage != NameUsage::Definition &&
-                        Config.Usage != NameUsage::Reference &&
+                        (Config.Usage != NameUsage::Reference || IsSubscript) &&
                         Resolved.LabelType == LabelRangeType::CallArg;
 
       if (renameLabels(Resolved.LabelRanges, Resolved.LabelType, isCallSite))
@@ -395,6 +428,17 @@ class TextReplacementsRenamer : public Renamer {
     return registerText(OldParam);
   }
 
+  StringRef getDeclArgumentLabelReplacement(StringRef OldLabelRange,
+                                            StringRef NewArgLabel) {
+      // OldLabelRange is subscript([]a: Int), foo([a]: Int) or foo([a] b: Int)
+      if (NewArgLabel.empty())
+        return OldLabelRange.empty() ? "" : "_";
+
+      if (OldLabelRange.empty())
+        return registerText((llvm::Twine(NewArgLabel) + " ").str());
+      return registerText(NewArgLabel);
+  }
+
   StringRef getReplacementText(StringRef LabelRange,
                                RefactoringRangeKind RangeKind,
                                StringRef OldLabel, StringRef NewLabel) {
@@ -407,9 +451,12 @@ class TextReplacementsRenamer : public Renamer {
       return getCallArgCombinedReplacement(LabelRange, NewLabel);
     case RefactoringRangeKind::ParameterName:
       return getParamNameReplacement(LabelRange, OldLabel, NewLabel);
+    case RefactoringRangeKind::NoncollapsibleParameterName:
+      return LabelRange;
     case RefactoringRangeKind::DeclArgumentLabel:
+      return getDeclArgumentLabelReplacement(LabelRange, NewLabel);
     case RefactoringRangeKind::SelectorArgumentLabel:
-      return registerText(NewLabel.empty() ? "_" : NewLabel);
+      return NewLabel.empty() ? "_" : registerText(NewLabel);
     default:
       llvm_unreachable("label range type is none but there are labels");
     }
@@ -2404,6 +2451,8 @@ struct swift::ide::FindRenameRangesAnnotatingConsumer::Implementation {
         return "keywordBase";
       case RefactoringRangeKind::ParameterName:
         return "param";
+      case RefactoringRangeKind::NoncollapsibleParameterName:
+        return "noncollapsibleparam";
       case RefactoringRangeKind::DeclArgumentLabel:
         return "arglabel";
       case RefactoringRangeKind::CallArgumentLabel:

lib/IDE/SwiftSourceDocInfo.cpp

@@ -349,7 +349,7 @@ bool NameMatcher::walkToDeclPre(Decl *D) {
     tryResolve(ASTWalker::ParentTy(D), D->getLoc(), LabelRangeType::Param,
                LabelRanges);
   } else if (SubscriptDecl *SD = dyn_cast<SubscriptDecl>(D)) {
-    tryResolve(ASTWalker::ParentTy(D), D->getLoc(), LabelRangeType::Param,
+    tryResolve(ASTWalker::ParentTy(D), D->getLoc(), LabelRangeType::NoncollapsibleParam,
                getLabelRanges(SD->getIndices(), getSourceMgr()));
   } else if (EnumElementDecl *EED = dyn_cast<EnumElementDecl>(D)) {
     if (TupleTypeRepr *TTR = dyn_cast_or_null<TupleTypeRepr>(EED->getArgumentTypeLoc().getTypeRepr())) {
@@ -434,6 +434,13 @@ std::pair<bool, Expr*> NameMatcher::walkToExprPre(Expr *E) {
           tryResolve(ASTWalker::ParentTy(E), nextLoc());
         } while (!shouldSkip(E));
         break;
+      case ExprKind::Subscript: {
+        auto Labels = getCallArgLabelRanges(getSourceMgr(),
+                                            cast<SubscriptExpr>(E)->getIndex(),
+                                            LabelRangeEndAt::BeforeElemStart);
+        tryResolve(ASTWalker::ParentTy(E), E->getLoc(), LabelRangeType::CallArg, Labels);
+        break;
+      }
       case ExprKind::Tuple: {
         TupleExpr *T = cast<TupleExpr>(E);
         // Handle arg label locations (the index reports property occurrences

lib/IDE/Utils.cpp

@@ -908,9 +908,11 @@ class ClangFileRewriterHelper {
       getBufferIdentifierForLoc(Range.getStart())).getValue();
     if (BufferId == InterestedId) {
       HasChange = true;
-      RewriteBuf.ReplaceText(
-                             SM.getLocOffsetInBuffer(Range.getStart(), BufferId),
-                             Range.str().size(), Text);
+      auto StartLoc = SM.getLocOffsetInBuffer(Range.getStart(), BufferId);
+      if (!Range.getByteLength())
+          RewriteBuf.InsertText(StartLoc, Text);
+      else
+          RewriteBuf.ReplaceText(StartLoc, Range.str().size(), Text);
     }
   }
 

test/refactoring/SyntacticRename/FindRangeOutputs/functions/arg-label.swift.expected

@@ -55,7 +55,7 @@ let _ = "Some text \(/*param-label:call*/aFunc(a:1)) around"
 class SomeClass {
     init() {}
     /*init:def*/init(a: Int, b:Int, c:Int) {}
-    /*sub:def*/subscript(x: Int, y: Int) -> Int {
+    /*sub:def*/subscript(x: Int, y j: Int) -> Int {
         get { return 1 }
         set {}
     }
@@ -64,8 +64,8 @@ class SomeClass {
 let someClass = SomeClass();
 let _ = /*init:call*/SomeClass(a:1, b:1, c:1)
 let _ = SomeClass . /*init*/init(a:b:c:)
-_ = someClass/*sub:ref*/[1, 2]
-someClass/*sub:ref*/[1, 2] = 2
+_ = someClass/*sub:ref*/[1, y: 2]
+someClass/*sub:ref*/[1, y: 2] = 2
 
 class AnotherClass {
     let bar = AnotherClass()

test/refactoring/SyntacticRename/FindRangeOutputs/functions/bar.swift.expected

@@ -55,7 +55,7 @@ let _ = "Some text \(/*param-label:call*/aFunc(a:1)) around"
 class SomeClass {
     init() {}
     /*init:def*/init(a: Int, b:Int, c:Int) {}
-    /*sub:def*/subscript(x: Int, y: Int) -> Int {
+    /*sub:def*/subscript(x: Int, y j: Int) -> Int {
         get { return 1 }
         set {}
     }
@@ -64,8 +64,8 @@ class SomeClass {
 let someClass = SomeClass();
 let _ = /*init:call*/SomeClass(a:1, b:1, c:1)
 let _ = SomeClass . /*init*/init(a:b:c:)
-_ = someClass/*sub:ref*/[1, 2]
-someClass/*sub:ref*/[1, 2] = 2
+_ = someClass/*sub:ref*/[1, y: 2]
+someClass/*sub:ref*/[1, y: 2] = 2
 
 class AnotherClass {
     let bar = AnotherClass()

test/refactoring/SyntacticRename/FindRangeOutputs/functions/infix-operator.swift.expected

@@ -55,7 +55,7 @@ let _ = "Some text \(/*param-label:call*/aFunc(a:1)) around"
 class SomeClass {
     init() {}
     /*init:def*/init(a: Int, b:Int, c:Int) {}
-    /*sub:def*/subscript(x: Int, y: Int) -> Int {
+    /*sub:def*/subscript(x: Int, y j: Int) -> Int {
         get { return 1 }
         set {}
     }
@@ -64,8 +64,8 @@ class SomeClass {
 let someClass = SomeClass();
 let _ = /*init:call*/SomeClass(a:1, b:1, c:1)
 let _ = SomeClass . /*init*/init(a:b:c:)
-_ = someClass/*sub:ref*/[1, 2]
-someClass/*sub:ref*/[1, 2] = 2
+_ = someClass/*sub:ref*/[1, y: 2]
+someClass/*sub:ref*/[1, y: 2] = 2
 
 class AnotherClass {
     let bar = AnotherClass()

test/refactoring/SyntacticRename/FindRangeOutputs/functions/init.swift.expected

@@ -55,7 +55,7 @@ let _ = "Some text \(/*param-label:call*/aFunc(a:1)) around"
 class SomeClass {
     init() {}
     /*init:def*/<keywordBase>init</keywordBase>(<arglabel index=0>a</arglabel><param index=0></param>: Int, <arglabel index=1>b</arglabel><param index=1></param>:Int, <arglabel index=2>c</arglabel><param index=2></param>:Int) {}
-    /*sub:def*/subscript(x: Int, y: Int) -> Int {
+    /*sub:def*/subscript(x: Int, y j: Int) -> Int {
         get { return 1 }
         set {}
     }
@@ -64,8 +64,8 @@ class SomeClass {
 let someClass = SomeClass();
 let _ = /*init:call*/SomeClass(<callarg index=0>a</callarg><callcolon index=0>:</callcolon>1, <callarg index=1>b</callarg><callcolon index=1>:</callcolon>1, <callarg index=2>c</callarg><callcolon index=2>:</callcolon>1)
 let _ = SomeClass . /*init*/init(<sel index=0>a</sel>:<sel index=1>b</sel>:<sel index=2>c</sel>:)
-_ = someClass/*sub:ref*/[1, 2]
-someClass/*sub:ref*/[1, 2] = 2
+_ = someClass/*sub:ref*/[1, y: 2]
+someClass/*sub:ref*/[1, y: 2] = 2
 
 class AnotherClass {
     let bar = AnotherClass()

test/refactoring/SyntacticRename/FindRangeOutputs/functions/memberwise-x.swift.expected

@@ -55,7 +55,7 @@ let _ = "Some text \(/*param-label:call*/aFunc(a:1)) around"
 class SomeClass {
     init() {}
     /*init:def*/init(a: Int, b:Int, c:Int) {}
-    /*sub:def*/subscript(x: Int, y: Int) -> Int {
+    /*sub:def*/subscript(x: Int, y j: Int) -> Int {
         get { return 1 }
         set {}
     }
@@ -64,8 +64,8 @@ class SomeClass {
 let someClass = SomeClass();
 let _ = /*init:call*/SomeClass(a:1, b:1, c:1)
 let _ = SomeClass . /*init*/init(a:b:c:)
-_ = someClass/*sub:ref*/[1, 2]
-someClass/*sub:ref*/[1, 2] = 2
+_ = someClass/*sub:ref*/[1, y: 2]
+someClass/*sub:ref*/[1, y: 2] = 2
 
 class AnotherClass {
     let bar = AnotherClass()

test/refactoring/SyntacticRename/FindRangeOutputs/functions/method.swift.expected

@@ -55,7 +55,7 @@ let _ = "Some text \(/*param-label:call*/aFunc(a:1)) around"
 class SomeClass {
     init() {}
     /*init:def*/init(a: Int, b:Int, c:Int) {}
-    /*sub:def*/subscript(x: Int, y: Int) -> Int {
+    /*sub:def*/subscript(x: Int, y j: Int) -> Int {
         get { return 1 }
         set {}
     }
@@ -64,8 +64,8 @@ class SomeClass {
 let someClass = SomeClass();
 let _ = /*init:call*/SomeClass(a:1, b:1, c:1)
 let _ = SomeClass . /*init*/init(a:b:c:)
-_ = someClass/*sub:ref*/[1, 2]
-someClass/*sub:ref*/[1, 2] = 2
+_ = someClass/*sub:ref*/[1, y: 2]
+someClass/*sub:ref*/[1, y: 2] = 2
 
 class AnotherClass {
     let bar = AnotherClass()