Skip to content

Commit c326ec4

Browse files
author
Nathan Hawes
committed
[Refactoring] Fix subscript rename
Adds support for renaming subscripts with external names, e.g. subscript(x y: Int), and introduces a noncollapsible parameter name range for subscript parameters, since these shouldn't be collapsed with an argument label of the same name as function parameter names are.
1 parent fa1d07c commit c326ec4

39 files changed

+211
-148
lines changed

‎include/swift/IDE/Utils.h

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,7 @@ enum class LabelRangeType {
255255
None,
256256
CallArg, // foo([a: ]2) or .foo([a: ]String)
257257
Param, // func([a b]: Int)
258+
NoncollapsibleParam, // subscript([a a]: Int)
258259
Selector, // #selector(foo.func([a]:))
259260
};
260261

@@ -500,14 +501,15 @@ enum class RegionType {
500501
};
501502

502503
enum class RefactoringRangeKind {
503-
BaseName, // func [foo](a b: Int)
504-
KeywordBaseName, // [init](a: Int)
505-
ParameterName, // func foo(a [b]: Int)
506-
DeclArgumentLabel, // func foo([a] b: Int)
507-
CallArgumentLabel, // foo([a]: 1)
508-
CallArgumentColon, // foo(a[: ]1)
509-
CallArgumentCombined, // foo([]1) could expand to foo([a: ]1)
510-
SelectorArgumentLabel, // foo([a]:)
504+
BaseName, // func [foo](a b: Int)
505+
KeywordBaseName, // [init](a: Int)
506+
ParameterName, // func foo(a[ b]: Int)
507+
NoncollapsibleParameterName, // subscript(a[ a]: Int)
508+
DeclArgumentLabel, // func foo([a] b: Int)
509+
CallArgumentLabel, // foo([a]: 1)
510+
CallArgumentColon, // foo(a[: ]1)
511+
CallArgumentCombined, // foo([]1) could expand to foo([a: ]1)
512+
SelectorArgumentLabel, // foo([a]:)
511513
};
512514

513515
struct NoteRegion {

‎lib/IDE/Refactoring.cpp

Lines changed: 84 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,7 @@ class Renamer {
118118
size_t Index = 0;
119119
for (const auto &LabelRange : LabelRanges) {
120120
assert(LabelRange.isValid());
121-
122-
if (!labelRangeMatches(LabelRange, OldLabels[Index]))
121+
if (!labelRangeMatches(LabelRange, RangeType, OldLabels[Index]))
123122
return true;
124123
splitAndRenameLabel(LabelRange, RangeType, Index++);
125124
}
@@ -135,7 +134,9 @@ class Renamer {
135134
case LabelRangeType::CallArg:
136135
return splitAndRenameCallArg(Range, NameIndex);
137136
case LabelRangeType::Param:
138-
return splitAndRenameParamLabel(Range, NameIndex);
137+
return splitAndRenameParamLabel(Range, NameIndex, /*IsCollapsible=*/true);
138+
case LabelRangeType::NoncollapsibleParam:
139+
return splitAndRenameParamLabel(Range, NameIndex, /*IsCollapsible=*/false);
139140
case LabelRangeType::Selector:
140141
return doRenameLabel(
141142
Range, RefactoringRangeKind::SelectorArgumentLabel, NameIndex);
@@ -144,28 +145,47 @@ class Renamer {
144145
}
145146
}
146147

147-
void splitAndRenameParamLabel(CharSourceRange Range, size_t NameIndex) {
148+
void splitAndRenameParamLabel(CharSourceRange Range, size_t NameIndex, bool IsCollapsible) {
148149
// Split parameter range foo([a b]: Int) into decl argument label [a] and
149-
// parameter name [b]. If we have only foo([a]: Int), then we add an empty
150-
// range for the local name.
150+
// parameter name [b] or noncollapsible parameter name [b] if IsCollapsible
151+
// is false (as for subscript decls). If we have only foo([a]: Int), then we
152+
// add an empty range for the local name, or for the decl argument label if
153+
// IsCollapsible is false.
151154
StringRef Content = Range.str();
152155
size_t ExternalNameEnd = Content.find_first_of(" \t\n\v\f\r/");
153-
ExternalNameEnd =
154-
ExternalNameEnd == StringRef::npos ? Content.size() : ExternalNameEnd;
155-
156-
CharSourceRange Ext{Range.getStart(), unsigned(ExternalNameEnd)};
157-
doRenameLabel(Ext, RefactoringRangeKind::DeclArgumentLabel, NameIndex);
158156

159-
size_t LocalNameStart = Content.find_last_of(" \t\n\v\f\r/");
160-
LocalNameStart =
161-
LocalNameStart == StringRef::npos ? ExternalNameEnd : LocalNameStart;
162-
// Note: we consider the leading whitespace part of the parameter name since
163-
// when the parameter is removed we want to remove the whitespace too.
164-
// FIXME: handle comments foo(a /*...*/b: Int).
165-
auto LocalLoc = Range.getStart().getAdvancedLocOrInvalid(LocalNameStart);
166-
assert(LocalLoc.isValid());
167-
CharSourceRange Local{LocalLoc, unsigned(Content.size() - LocalNameStart)};
168-
doRenameLabel(Local, RefactoringRangeKind::ParameterName, NameIndex);
157+
if (ExternalNameEnd == StringRef::npos) { // foo([a]: Int)
158+
if (IsCollapsible) {
159+
doRenameLabel(Range, RefactoringRangeKind::DeclArgumentLabel, NameIndex);
160+
doRenameLabel(CharSourceRange{Range.getEnd(), 0},
161+
RefactoringRangeKind::ParameterName, NameIndex);
162+
} else {
163+
doRenameLabel(CharSourceRange{Range.getStart(), 0},
164+
RefactoringRangeKind::DeclArgumentLabel, NameIndex);
165+
doRenameLabel(Range, RefactoringRangeKind::NoncollapsibleParameterName,
166+
NameIndex);
167+
}
168+
} else { // foo([a b]: Int)
169+
CharSourceRange Ext{Range.getStart(), unsigned(ExternalNameEnd)};
170+
171+
// Note: we consider the leading whitespace part of the parameter name
172+
// if the parameter is collapsible, since if the parameter is collapsed
173+
// into a matching argument label, we want to remove the whitespace too.
174+
// FIXME: handle comments foo(a /*...*/b: Int).
175+
size_t LocalNameStart = Content.find_last_of(" \t\n\v\f\r/");
176+
assert(LocalNameStart != StringRef::npos);
177+
if (!IsCollapsible)
178+
++LocalNameStart;
179+
auto LocalLoc = Range.getStart().getAdvancedLocOrInvalid(LocalNameStart);
180+
CharSourceRange Local{LocalLoc, unsigned(Content.size() - LocalNameStart)};
181+
182+
doRenameLabel(Ext, RefactoringRangeKind::DeclArgumentLabel, NameIndex);
183+
if (IsCollapsible) {
184+
doRenameLabel(Local, RefactoringRangeKind::ParameterName, NameIndex);
185+
} else {
186+
doRenameLabel(Local, RefactoringRangeKind::NoncollapsibleParameterName, NameIndex);
187+
}
188+
}
169189
}
170190

171191
void splitAndRenameCallArg(CharSourceRange Range, size_t NameIndex) {
@@ -192,14 +212,24 @@ class Renamer {
192212
doRenameLabel(Rest, RefactoringRangeKind::CallArgumentColon, NameIndex);
193213
}
194214

195-
bool labelRangeMatches(CharSourceRange Range, StringRef Expected) {
215+
bool labelRangeMatches(CharSourceRange Range, LabelRangeType RangeType, StringRef Expected) {
196216
if (Range.getByteLength()) {
197-
StringRef ExistingLabel = Lexer::getCharSourceRangeFromSourceRange(SM,
198-
Range.getStart()).str();
199-
if (!Expected.empty())
200-
return Expected == ExistingLabel;
201-
else
202-
return ExistingLabel == "_";
217+
CharSourceRange ExistingLabelRange =
218+
Lexer::getCharSourceRangeFromSourceRange(SM, Range.getStart());
219+
StringRef ExistingLabel = ExistingLabelRange.str();
220+
221+
switch (RangeType) {
222+
case LabelRangeType::NoncollapsibleParam:
223+
if (ExistingLabelRange == Range && Expected.empty()) // subscript([x]: Int)
224+
return true;
225+
LLVM_FALLTHROUGH;
226+
case LabelRangeType::CallArg:
227+
case LabelRangeType::Param:
228+
case LabelRangeType::Selector:
229+
return ExistingLabel == (Expected.empty() ? "_" : Expected);
230+
case LabelRangeType::None:
231+
llvm_unreachable("Unhandled label range type");
232+
}
203233
}
204234
return Expected.empty();
205235
}
@@ -238,7 +268,7 @@ class Renamer {
238268
if (NameIndex >= OldNames.size())
239269
return true;
240270

241-
while (!labelRangeMatches(Label, OldNames[NameIndex])) {
271+
while (!labelRangeMatches(Label, RangeType, OldNames[NameIndex])) {
242272
if (++NameIndex >= OldNames.size())
243273
return true;
244274
};
@@ -279,13 +309,16 @@ class Renamer {
279309

280310
assert(Config.Usage != NameUsage::Call || Config.IsFunctionLike);
281311

282-
bool isKeywordBase = Old.base() == "init" || Old.base() == "subscript";
312+
// FIXME: handle escaped keyword names `init`
313+
bool IsSubscript = Old.base() == "subscript" && Config.IsFunctionLike;
314+
bool IsInit = Old.base() == "init" && Config.IsFunctionLike;
315+
bool IsKeywordBase = IsInit || IsSubscript;
283316

284-
if (!Config.IsFunctionLike || !isKeywordBase) {
317+
if (!Config.IsFunctionLike || !IsKeywordBase) {
285318
if (renameBase(Resolved.Range, RefactoringRangeKind::BaseName))
286319
return RegionType::Mismatch;
287320

288-
} else if (isKeywordBase && Config.Usage == NameUsage::Definition) {
321+
} else if (IsKeywordBase && Config.Usage == NameUsage::Definition) {
289322
if (renameBase(Resolved.Range, RefactoringRangeKind::KeywordBaseName))
290323
return RegionType::Mismatch;
291324
}
@@ -300,7 +333,7 @@ class Renamer {
300333
HandleLabels = true;
301334
break;
302335
case NameUsage::Reference:
303-
HandleLabels = Resolved.LabelType == LabelRangeType::Selector;
336+
HandleLabels = Resolved.LabelType == LabelRangeType::Selector || IsSubscript;
304337
break;
305338
case NameUsage::Unknown:
306339
HandleLabels = Resolved.LabelType != LabelRangeType::None;
@@ -315,7 +348,7 @@ class Renamer {
315348

316349
if (HandleLabels) {
317350
bool isCallSite = Config.Usage != NameUsage::Definition &&
318-
Config.Usage != NameUsage::Reference &&
351+
(Config.Usage != NameUsage::Reference || IsSubscript) &&
319352
Resolved.LabelType == LabelRangeType::CallArg;
320353

321354
if (renameLabels(Resolved.LabelRanges, Resolved.LabelType, isCallSite))
@@ -395,6 +428,17 @@ class TextReplacementsRenamer : public Renamer {
395428
return registerText(OldParam);
396429
}
397430

431+
StringRef getDeclArgumentLabelReplacement(StringRef OldLabelRange,
432+
StringRef NewArgLabel) {
433+
// OldLabelRange is subscript([]a: Int), foo([a]: Int) or foo([a] b: Int)
434+
if (NewArgLabel.empty())
435+
return OldLabelRange.empty() ? "" : "_";
436+
437+
if (OldLabelRange.empty())
438+
return registerText((llvm::Twine(NewArgLabel) + " ").str());
439+
return registerText(NewArgLabel);
440+
}
441+
398442
StringRef getReplacementText(StringRef LabelRange,
399443
RefactoringRangeKind RangeKind,
400444
StringRef OldLabel, StringRef NewLabel) {
@@ -407,9 +451,12 @@ class TextReplacementsRenamer : public Renamer {
407451
return getCallArgCombinedReplacement(LabelRange, NewLabel);
408452
case RefactoringRangeKind::ParameterName:
409453
return getParamNameReplacement(LabelRange, OldLabel, NewLabel);
454+
case RefactoringRangeKind::NoncollapsibleParameterName:
455+
return LabelRange;
410456
case RefactoringRangeKind::DeclArgumentLabel:
457+
return getDeclArgumentLabelReplacement(LabelRange, NewLabel);
411458
case RefactoringRangeKind::SelectorArgumentLabel:
412-
return registerText(NewLabel.empty() ? "_" : NewLabel);
459+
return NewLabel.empty() ? "_" : registerText(NewLabel);
413460
default:
414461
llvm_unreachable("label range type is none but there are labels");
415462
}
@@ -2404,6 +2451,8 @@ struct swift::ide::FindRenameRangesAnnotatingConsumer::Implementation {
24042451
return "keywordBase";
24052452
case RefactoringRangeKind::ParameterName:
24062453
return "param";
2454+
case RefactoringRangeKind::NoncollapsibleParameterName:
2455+
return "noncollapsibleparam";
24072456
case RefactoringRangeKind::DeclArgumentLabel:
24082457
return "arglabel";
24092458
case RefactoringRangeKind::CallArgumentLabel:

‎lib/IDE/SwiftSourceDocInfo.cpp

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -349,7 +349,7 @@ bool NameMatcher::walkToDeclPre(Decl *D) {
349349
tryResolve(ASTWalker::ParentTy(D), D->getLoc(), LabelRangeType::Param,
350350
LabelRanges);
351351
} else if (SubscriptDecl *SD = dyn_cast<SubscriptDecl>(D)) {
352-
tryResolve(ASTWalker::ParentTy(D), D->getLoc(), LabelRangeType::Param,
352+
tryResolve(ASTWalker::ParentTy(D), D->getLoc(), LabelRangeType::NoncollapsibleParam,
353353
getLabelRanges(SD->getIndices(), getSourceMgr()));
354354
} else if (EnumElementDecl *EED = dyn_cast<EnumElementDecl>(D)) {
355355
if (TupleTypeRepr *TTR = dyn_cast_or_null<TupleTypeRepr>(EED->getArgumentTypeLoc().getTypeRepr())) {
@@ -434,6 +434,13 @@ std::pair<bool, Expr*> NameMatcher::walkToExprPre(Expr *E) {
434434
tryResolve(ASTWalker::ParentTy(E), nextLoc());
435435
} while (!shouldSkip(E));
436436
break;
437+
case ExprKind::Subscript: {
438+
auto Labels = getCallArgLabelRanges(getSourceMgr(),
439+
cast<SubscriptExpr>(E)->getIndex(),
440+
LabelRangeEndAt::BeforeElemStart);
441+
tryResolve(ASTWalker::ParentTy(E), E->getLoc(), LabelRangeType::CallArg, Labels);
442+
break;
443+
}
437444
case ExprKind::Tuple: {
438445
TupleExpr *T = cast<TupleExpr>(E);
439446
// Handle arg label locations (the index reports property occurrences

‎lib/IDE/Utils.cpp

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -908,9 +908,11 @@ class ClangFileRewriterHelper {
908908
getBufferIdentifierForLoc(Range.getStart())).getValue();
909909
if (BufferId == InterestedId) {
910910
HasChange = true;
911-
RewriteBuf.ReplaceText(
912-
SM.getLocOffsetInBuffer(Range.getStart(), BufferId),
913-
Range.str().size(), Text);
911+
auto StartLoc = SM.getLocOffsetInBuffer(Range.getStart(), BufferId);
912+
if (!Range.getByteLength())
913+
RewriteBuf.InsertText(StartLoc, Text);
914+
else
915+
RewriteBuf.ReplaceText(StartLoc, Range.str().size(), Text);
914916
}
915917
}
916918

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ let _ = "Some text \(/*param-label:call*/aFunc(a:1)) around"
5555
class SomeClass {
5656
init() {}
5757
/*init:def*/init(a: Int, b:Int, c:Int) {}
58-
/*sub:def*/subscript(x: Int, y: Int) -> Int {
58+
/*sub:def*/subscript(x: Int, y j: Int) -> Int {
5959
get { return 1 }
6060
set {}
6161
}
@@ -64,8 +64,8 @@ class SomeClass {
6464
let someClass = SomeClass();
6565
let _ = /*init:call*/SomeClass(a:1, b:1, c:1)
6666
let _ = SomeClass . /*init*/init(a:b:c:)
67-
_ = someClass/*sub:ref*/[1, 2]
68-
someClass/*sub:ref*/[1, 2] = 2
67+
_ = someClass/*sub:ref*/[1, y: 2]
68+
someClass/*sub:ref*/[1, y: 2] = 2
6969

7070
class AnotherClass {
7171
let bar = AnotherClass()

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ let _ = "Some text \(/*param-label:call*/aFunc(a:1)) around"
5555
class SomeClass {
5656
init() {}
5757
/*init:def*/init(a: Int, b:Int, c:Int) {}
58-
/*sub:def*/subscript(x: Int, y: Int) -> Int {
58+
/*sub:def*/subscript(x: Int, y j: Int) -> Int {
5959
get { return 1 }
6060
set {}
6161
}
@@ -64,8 +64,8 @@ class SomeClass {
6464
let someClass = SomeClass();
6565
let _ = /*init:call*/SomeClass(a:1, b:1, c:1)
6666
let _ = SomeClass . /*init*/init(a:b:c:)
67-
_ = someClass/*sub:ref*/[1, 2]
68-
someClass/*sub:ref*/[1, 2] = 2
67+
_ = someClass/*sub:ref*/[1, y: 2]
68+
someClass/*sub:ref*/[1, y: 2] = 2
6969

7070
class AnotherClass {
7171
let bar = AnotherClass()

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ let _ = "Some text \(/*param-label:call*/aFunc(a:1)) around"
5555
class SomeClass {
5656
init() {}
5757
/*init:def*/init(a: Int, b:Int, c:Int) {}
58-
/*sub:def*/subscript(x: Int, y: Int) -> Int {
58+
/*sub:def*/subscript(x: Int, y j: Int) -> Int {
5959
get { return 1 }
6060
set {}
6161
}
@@ -64,8 +64,8 @@ class SomeClass {
6464
let someClass = SomeClass();
6565
let _ = /*init:call*/SomeClass(a:1, b:1, c:1)
6666
let _ = SomeClass . /*init*/init(a:b:c:)
67-
_ = someClass/*sub:ref*/[1, 2]
68-
someClass/*sub:ref*/[1, 2] = 2
67+
_ = someClass/*sub:ref*/[1, y: 2]
68+
someClass/*sub:ref*/[1, y: 2] = 2
6969

7070
class AnotherClass {
7171
let bar = AnotherClass()

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ let _ = "Some text \(/*param-label:call*/aFunc(a:1)) around"
5555
class SomeClass {
5656
init() {}
5757
/*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) {}
58-
/*sub:def*/subscript(x: Int, y: Int) -> Int {
58+
/*sub:def*/subscript(x: Int, y j: Int) -> Int {
5959
get { return 1 }
6060
set {}
6161
}
@@ -64,8 +64,8 @@ class SomeClass {
6464
let someClass = SomeClass();
6565
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)
6666
let _ = SomeClass . /*init*/init(<sel index=0>a</sel>:<sel index=1>b</sel>:<sel index=2>c</sel>:)
67-
_ = someClass/*sub:ref*/[1, 2]
68-
someClass/*sub:ref*/[1, 2] = 2
67+
_ = someClass/*sub:ref*/[1, y: 2]
68+
someClass/*sub:ref*/[1, y: 2] = 2
6969

7070
class AnotherClass {
7171
let bar = AnotherClass()

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ let _ = "Some text \(/*param-label:call*/aFunc(a:1)) around"
5555
class SomeClass {
5656
init() {}
5757
/*init:def*/init(a: Int, b:Int, c:Int) {}
58-
/*sub:def*/subscript(x: Int, y: Int) -> Int {
58+
/*sub:def*/subscript(x: Int, y j: Int) -> Int {
5959
get { return 1 }
6060
set {}
6161
}
@@ -64,8 +64,8 @@ class SomeClass {
6464
let someClass = SomeClass();
6565
let _ = /*init:call*/SomeClass(a:1, b:1, c:1)
6666
let _ = SomeClass . /*init*/init(a:b:c:)
67-
_ = someClass/*sub:ref*/[1, 2]
68-
someClass/*sub:ref*/[1, 2] = 2
67+
_ = someClass/*sub:ref*/[1, y: 2]
68+
someClass/*sub:ref*/[1, y: 2] = 2
6969

7070
class AnotherClass {
7171
let bar = AnotherClass()

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ let _ = "Some text \(/*param-label:call*/aFunc(a:1)) around"
5555
class SomeClass {
5656
init() {}
5757
/*init:def*/init(a: Int, b:Int, c:Int) {}
58-
/*sub:def*/subscript(x: Int, y: Int) -> Int {
58+
/*sub:def*/subscript(x: Int, y j: Int) -> Int {
5959
get { return 1 }
6060
set {}
6161
}
@@ -64,8 +64,8 @@ class SomeClass {
6464
let someClass = SomeClass();
6565
let _ = /*init:call*/SomeClass(a:1, b:1, c:1)
6666
let _ = SomeClass . /*init*/init(a:b:c:)
67-
_ = someClass/*sub:ref*/[1, 2]
68-
someClass/*sub:ref*/[1, 2] = 2
67+
_ = someClass/*sub:ref*/[1, y: 2]
68+
someClass/*sub:ref*/[1, y: 2] = 2
6969

7070
class AnotherClass {
7171
let bar = AnotherClass()

0 commit comments

Comments
 (0)