Skip to content

[Refactoring] Fix subscript rename #13130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 29, 2017
Merged

[Refactoring] Fix subscript rename #13130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions include/swift/IDE/Utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]:))
};

Expand Down Expand Up @@ -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 {
Expand Down
119 changes: 84 additions & 35 deletions lib/IDE/Refactoring.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++);
}
Expand All @@ -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);
Expand All @@ -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) {
Expand All @@ -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();
}
Expand Down Expand Up @@ -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;
};
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
Expand All @@ -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))
Expand Down Expand Up @@ -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) {
Expand All @@ -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");
}
Expand Down Expand Up @@ -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:
Expand Down
9 changes: 8 additions & 1 deletion lib/IDE/SwiftSourceDocInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())) {
Expand Down Expand Up @@ -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
Expand Down
8 changes: 5 additions & 3 deletions lib/IDE/Utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
6 changes: 3 additions & 3 deletions test/refactoring/SyntacticRename/FindRangeOutputs/functions/arg-label.swift.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
}
Expand All @@ -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()
Expand Down
6 changes: 3 additions & 3 deletions test/refactoring/SyntacticRename/FindRangeOutputs/functions/bar.swift.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
}
Expand All @@ -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()
Expand Down
6 changes: 3 additions & 3 deletions test/refactoring/SyntacticRename/FindRangeOutputs/functions/infix-operator.swift.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
}
Expand All @@ -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()
Expand Down
6 changes: 3 additions & 3 deletions test/refactoring/SyntacticRename/FindRangeOutputs/functions/init.swift.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
}
Expand All @@ -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()
Expand Down
6 changes: 3 additions & 3 deletions test/refactoring/SyntacticRename/FindRangeOutputs/functions/memberwise-x.swift.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
}
Expand All @@ -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()
Expand Down
6 changes: 3 additions & 3 deletions test/refactoring/SyntacticRename/FindRangeOutputs/functions/method.swift.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
}
Expand All @@ -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()
Expand Down
Loading