[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
David Goldman via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 25 12:58:56 PST 2024
https://github.com/DavidGoldman updated https://github.com/llvm/llvm-project/pull/76466
>From 4caf5b3c779bf18236b4b0be5bc7147d10339f2b Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Tue, 26 Dec 2023 15:59:01 -0500
Subject: [PATCH 1/7] [clangd][SymbolCollector] Treat ObjC methods as spelled
We'll treat multi-arg methods as spelled once we have full rename
support for them.
---
.../clangd/index/SymbolCollector.cpp | 6 ++-
.../clangd/unittests/SymbolCollectorTests.cpp | 42 +++++++++++++++++++
2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index 7ef4b15febad22f..336bc3506bb3608 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -174,7 +174,9 @@ bool isSpelled(SourceLocation Loc, const NamedDecl &ND) {
auto Name = ND.getDeclName();
const auto NameKind = Name.getNameKind();
if (NameKind != DeclarationName::Identifier &&
- NameKind != DeclarationName::CXXConstructorName)
+ NameKind != DeclarationName::CXXConstructorName &&
+ NameKind != DeclarationName::ObjCZeroArgSelector &&
+ NameKind != DeclarationName::ObjCOneArgSelector)
return false;
const auto &AST = ND.getASTContext();
const auto &SM = AST.getSourceManager();
@@ -183,6 +185,8 @@ bool isSpelled(SourceLocation Loc, const NamedDecl &ND) {
if (clang::Lexer::getRawToken(Loc, Tok, SM, LO))
return false;
auto StrName = Name.getAsString();
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&ND))
+ StrName = MD->getSelector().getNameForSlot(0).str();
return clang::Lexer::getSpelling(Tok, SM, LO) == StrName;
}
} // namespace
diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
index 9cdc57ec01f3276..1d4e1c1d75ea230 100644
--- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -105,6 +105,9 @@ MATCHER(refRange, "") {
MATCHER_P2(OverriddenBy, Subject, Object, "") {
return arg == Relation{Subject.ID, RelationKind::OverriddenBy, Object.ID};
}
+MATCHER(isSpelled, "") {
+ return static_cast<bool>(arg.Kind & RefKind::Spelled);
+}
::testing::Matcher<const std::vector<Ref> &>
haveRanges(const std::vector<Range> Ranges) {
return ::testing::UnorderedPointwise(refRange(), Ranges);
@@ -524,6 +527,45 @@ TEST_F(SymbolCollectorTest, templateArgs) {
forCodeCompletion(false)))));
}
+TEST_F(SymbolCollectorTest, ObjCRefs) {
+ Annotations Header(R"(
+ @interface Person
+ - (void)$talk[[talk]];
+ - (void)$say[[say]]:(id)something;
+ @end
+ @interface Person (Category)
+ - (void)categoryMethod;
+ - (void)multiArg:(id)a method:(id)b;
+ @end
+ )");
+ Annotations Main(R"(
+ @implementation Person
+ - (void)$talk[[talk]] {}
+ - (void)$say[[say]]:(id)something {}
+ @end
+
+ void fff(Person *p) {
+ [p $talk[[talk]]];
+ [p $say[[say]]:0];
+ [p categoryMethod];
+ [p multiArg:0 method:0];
+ }
+ )");
+ CollectorOpts.RefFilter = RefKind::All;
+ CollectorOpts.CollectMainFileRefs = true;
+ TestFileName = testPath("test.m");
+ runSymbolCollector(Header.code(), Main.code(),
+ {"-fblocks", "-xobjective-c++", "-Wno-objc-root-class"});
+ EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Person::talk").ID,
+ haveRanges(Main.ranges("talk")))));
+ EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Person::say:").ID,
+ haveRanges(Main.ranges("say")))));
+ EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Person::categoryMethod").ID,
+ ElementsAre(isSpelled()))));
+ EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Person::multiArg:method:").ID,
+ ElementsAre(Not(isSpelled())))));
+}
+
TEST_F(SymbolCollectorTest, ObjCSymbols) {
const std::string Header = R"(
@interface Person
>From 1b6a09464ff5c7b1988fcb479d0a4ff876f696e6 Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Tue, 26 Dec 2023 16:12:03 -0500
Subject: [PATCH 2/7] Run clang-format
---
.../clangd/unittests/SymbolCollectorTests.cpp | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
index 1d4e1c1d75ea230..5c20b950e4eac0d 100644
--- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -560,10 +560,12 @@ TEST_F(SymbolCollectorTest, ObjCRefs) {
haveRanges(Main.ranges("talk")))));
EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Person::say:").ID,
haveRanges(Main.ranges("say")))));
- EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Person::categoryMethod").ID,
- ElementsAre(isSpelled()))));
- EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Person::multiArg:method:").ID,
- ElementsAre(Not(isSpelled())))));
+ EXPECT_THAT(Refs,
+ Contains(Pair(findSymbol(Symbols, "Person::categoryMethod").ID,
+ ElementsAre(isSpelled()))));
+ EXPECT_THAT(Refs,
+ Contains(Pair(findSymbol(Symbols, "Person::multiArg:method:").ID,
+ ElementsAre(Not(isSpelled())))));
}
TEST_F(SymbolCollectorTest, ObjCSymbols) {
>From 2e9b17ac3bd744631e760e0230676f81f40cfed9 Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Tue, 26 Dec 2023 17:12:23 -0500
Subject: [PATCH 3/7] ObjC method rename support
We now support renaming ObjC methods even with multiple selector pieces.
This requires clangd to lex each file to discover the selector pieces
since the index only stores the location of the initial selector token.
---
clang-tools-extra/clangd/ClangdLSPServer.cpp | 7 +-
clang-tools-extra/clangd/ClangdLSPServer.h | 2 +-
clang-tools-extra/clangd/Protocol.cpp | 9 +
clang-tools-extra/clangd/Protocol.h | 8 +
.../clangd/index/SymbolCollector.cpp | 9 +-
clang-tools-extra/clangd/refactor/Rename.cpp | 370 ++++++++++++++++--
clang-tools-extra/clangd/refactor/Rename.h | 41 +-
.../clangd/unittests/RenameTests.cpp | 62 ++-
.../clangd/unittests/SymbolCollectorTests.cpp | 2 +-
9 files changed, 439 insertions(+), 71 deletions(-)
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index a87da252b7a7e9b..3e097cbeed5929b 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -844,14 +844,17 @@ void ClangdLSPServer::onWorkspaceSymbol(
}
void ClangdLSPServer::onPrepareRename(const TextDocumentPositionParams &Params,
- Callback<std::optional<Range>> Reply) {
+ Callback<PrepareRenameResult> Reply) {
Server->prepareRename(
Params.textDocument.uri.file(), Params.position, /*NewName*/ std::nullopt,
Opts.Rename,
[Reply = std::move(Reply)](llvm::Expected<RenameResult> Result) mutable {
if (!Result)
return Reply(Result.takeError());
- return Reply(std::move(Result->Target));
+ PrepareRenameResult PrepareResult;
+ PrepareResult.range = Result->Target;
+ PrepareResult.placeholder = Result->Placeholder;
+ return Reply(std::move(PrepareResult));
});
}
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index 79579c22b788a9c..6a9f097d551ae6d 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -134,7 +134,7 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
void onWorkspaceSymbol(const WorkspaceSymbolParams &,
Callback<std::vector<SymbolInformation>>);
void onPrepareRename(const TextDocumentPositionParams &,
- Callback<std::optional<Range>>);
+ Callback<PrepareRenameResult>);
void onRename(const RenameParams &, Callback<WorkspaceEdit>);
void onHover(const TextDocumentPositionParams &,
Callback<std::optional<Hover>>);
diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp
index a6370649f5ad1cf..f93a941a5c27e4f 100644
--- a/clang-tools-extra/clangd/Protocol.cpp
+++ b/clang-tools-extra/clangd/Protocol.cpp
@@ -1187,6 +1187,15 @@ bool fromJSON(const llvm::json::Value &Params, RenameParams &R,
O.map("position", R.position) && O.map("newName", R.newName);
}
+llvm::json::Value toJSON(const PrepareRenameResult &PRR) {
+ if (PRR.placeholder.empty())
+ return toJSON(PRR.range);
+ return llvm::json::Object{
+ {"range", toJSON(PRR.range)},
+ {"placeholder", PRR.placeholder},
+ };
+}
+
llvm::json::Value toJSON(const DocumentHighlight &DH) {
return llvm::json::Object{
{"range", toJSON(DH.range)},
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index e88c804692568f5..8fcfcd5e33f6ce3 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -1436,6 +1436,14 @@ struct RenameParams {
};
bool fromJSON(const llvm::json::Value &, RenameParams &, llvm::json::Path);
+struct PrepareRenameResult {
+ /// Range of the string to rename.
+ Range range;
+ /// Placeholder text to use in the editor if non-empty.
+ std::string placeholder;
+};
+llvm::json::Value toJSON(const PrepareRenameResult &PRR);
+
enum class DocumentHighlightKind { Text = 1, Read = 2, Write = 3 };
/// A document highlight is a range inside a text document which deserves
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index 336bc3506bb3608..f7afa5c099742e4 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -176,7 +176,8 @@ bool isSpelled(SourceLocation Loc, const NamedDecl &ND) {
if (NameKind != DeclarationName::Identifier &&
NameKind != DeclarationName::CXXConstructorName &&
NameKind != DeclarationName::ObjCZeroArgSelector &&
- NameKind != DeclarationName::ObjCOneArgSelector)
+ NameKind != DeclarationName::ObjCOneArgSelector &&
+ NameKind != DeclarationName::ObjCMultiArgSelector)
return false;
const auto &AST = ND.getASTContext();
const auto &SM = AST.getSourceManager();
@@ -184,10 +185,10 @@ bool isSpelled(SourceLocation Loc, const NamedDecl &ND) {
clang::Token Tok;
if (clang::Lexer::getRawToken(Loc, Tok, SM, LO))
return false;
- auto StrName = Name.getAsString();
+ auto TokSpelling = clang::Lexer::getSpelling(Tok, SM, LO);
if (const auto *MD = dyn_cast<ObjCMethodDecl>(&ND))
- StrName = MD->getSelector().getNameForSlot(0).str();
- return clang::Lexer::getSpelling(Tok, SM, LO) == StrName;
+ return TokSpelling == MD->getSelector().getNameForSlot(0);
+ return TokSpelling == Name.getAsString();
}
} // namespace
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 11f9e4627af7609..e8e7517ac7fe61c 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -38,6 +38,11 @@
namespace clang {
namespace clangd {
+
+std::vector<SymbolRange> collectRenameIdentifierRanges(
+ llvm::StringRef Identifier, llvm::StringRef Content,
+ const LangOptions &LangOpts, std::optional<Selector> Selector);
+
namespace {
std::optional<std::string> filePath(const SymbolLocation &Loc,
@@ -498,7 +503,7 @@ llvm::Error makeError(InvalidName Reason) {
return error("invalid name: {0}", Message(Reason));
}
-static bool mayBeValidIdentifier(llvm::StringRef Ident) {
+static bool mayBeValidIdentifier(llvm::StringRef Ident, bool AllowColon) {
assert(llvm::json::isUTF8(Ident));
if (Ident.empty())
return false;
@@ -508,6 +513,8 @@ static bool mayBeValidIdentifier(llvm::StringRef Ident) {
!isAsciiIdentifierStart(Ident.front(), AllowDollar))
return false;
for (char C : Ident) {
+ if (AllowColon && C == ':')
+ continue;
if (llvm::isASCII(C) && !isAsciiIdentifierContinue(C, AllowDollar))
return false;
}
@@ -525,7 +532,7 @@ std::optional<InvalidName> checkName(const NamedDecl &RenameDecl,
std::optional<InvalidName> Result;
if (isKeyword(NewName, ASTCtx.getLangOpts()))
Result = InvalidName{InvalidName::Keywords, NewName.str()};
- else if (!mayBeValidIdentifier(NewName))
+ else if (!mayBeValidIdentifier(NewName, isa<ObjCMethodDecl>(&RenameDecl)))
Result = InvalidName{InvalidName::BadIdentifier, NewName.str()};
else {
// Name conflict detection.
@@ -543,6 +550,45 @@ std::optional<InvalidName> checkName(const NamedDecl &RenameDecl,
return Result;
}
+clangd::Range tokenRangeForLoc(ParsedAST &AST, SourceLocation TokLoc,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ const auto *Token = AST.getTokens().spelledTokenAt(TokLoc);
+ assert(Token && "got inclusion at wrong offset");
+ clangd::Range Result;
+ Result.start = sourceLocToPosition(SM, Token->location());
+ Result.end = sourceLocToPosition(SM, Token->endLocation());
+ return Result;
+}
+
+// AST-based ObjC method rename, it renames all occurrences in the main file
+// even for selectors which may have multiple tokens.
+llvm::Expected<tooling::Replacements>
+renameObjCMethodWithinFile(ParsedAST &AST, const ObjCMethodDecl *MD,
+ llvm::StringRef NewName,
+ std::vector<SourceLocation> Locs) {
+ const SourceManager &SM = AST.getSourceManager();
+ auto Code = SM.getBufferData(SM.getMainFileID());
+ auto RenameIdentifier = MD->getSelector().getNameForSlot(0).str();
+ llvm::SmallVector<llvm::StringRef, 8> NewNames;
+ NewName.split(NewNames, ":");
+ if (NewNames.empty())
+ NewNames.push_back(NewName);
+
+ std::vector<Range> Ranges;
+ const auto &LangOpts = MD->getASTContext().getLangOpts();
+ for (const auto &Loc : Locs)
+ Ranges.push_back(tokenRangeForLoc(AST, Loc, SM, LangOpts));
+ auto FilePath = AST.tuPath();
+ auto RenameRanges = collectRenameIdentifierRanges(
+ RenameIdentifier, Code, LangOpts, MD->getSelector());
+ auto RenameEdit = buildRenameEdit(FilePath, Code, RenameRanges, NewNames);
+ if (!RenameEdit)
+ return error("failed to rename in file {0}: {1}", FilePath,
+ RenameEdit.takeError());
+ return RenameEdit->Replacements;
+}
+
// AST-based rename, it renames all occurrences in the main file.
llvm::Expected<tooling::Replacements>
renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
@@ -551,6 +597,7 @@ renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
const SourceManager &SM = AST.getSourceManager();
tooling::Replacements FilteredChanges;
+ std::vector<SourceLocation> Locs;
for (SourceLocation Loc : findOccurrencesWithinFile(AST, RenameDecl)) {
SourceLocation RenameLoc = Loc;
// We don't rename in any macro bodies, but we allow rename the symbol
@@ -569,8 +616,13 @@ renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
// }
if (!isInsideMainFile(RenameLoc, SM))
continue;
+ Locs.push_back(RenameLoc);
+ }
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl))
+ return renameObjCMethodWithinFile(AST, MD, NewName, std::move(Locs));
+ for (const auto &Loc : Locs) {
if (auto Err = FilteredChanges.add(tooling::Replacement(
- SM, CharSourceRange::getTokenRange(RenameLoc), NewName)))
+ SM, CharSourceRange::getTokenRange(Loc), NewName)))
return std::move(Err);
}
return FilteredChanges;
@@ -681,12 +733,25 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
ExpBuffer.getError().message());
continue;
}
+ std::string RenameIdentifier = RenameDecl.getNameAsString();
+ std::optional<Selector> Selector = std::nullopt;
+ llvm::SmallVector<llvm::StringRef, 8> NewNames;
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
+ if (MD->getSelector().getNumArgs() > 1) {
+ RenameIdentifier = MD->getSelector().getNameForSlot(0).str();
+ Selector = MD->getSelector();
+ NewName.split(NewNames, ":");
+ }
+ }
+ if (NewNames.empty()) {
+ NewNames.push_back(NewName);
+ }
auto AffectedFileCode = (*ExpBuffer)->getBuffer();
auto RenameRanges =
- adjustRenameRanges(AffectedFileCode, RenameDecl.getNameAsString(),
+ adjustRenameRanges(AffectedFileCode, RenameIdentifier,
std::move(FileAndOccurrences.second),
- RenameDecl.getASTContext().getLangOpts());
+ RenameDecl.getASTContext().getLangOpts(), Selector);
if (!RenameRanges) {
// Our heuristics fails to adjust rename ranges to the current state of
// the file, it is most likely the index is stale, so we give up the
@@ -696,7 +761,7 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
FilePath);
}
auto RenameEdit =
- buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewName);
+ buildRenameEdit(FilePath, AffectedFileCode, *RenameRanges, NewNames);
if (!RenameEdit)
return error("failed to rename in file {0}: {1}", FilePath,
RenameEdit.takeError());
@@ -724,7 +789,7 @@ bool impliesSimpleEdit(const Position &LHS, const Position &RHS) {
// *line* or *column*, but not both (increases chance of a robust mapping)
void findNearMiss(
std::vector<size_t> &PartialMatch, ArrayRef<Range> IndexedRest,
- ArrayRef<Range> LexedRest, int LexedIndex, int &Fuel,
+ ArrayRef<SymbolRange> LexedRest, int LexedIndex, int &Fuel,
llvm::function_ref<void(const std::vector<size_t> &)> MatchedCB) {
if (--Fuel < 0)
return;
@@ -734,7 +799,8 @@ void findNearMiss(
MatchedCB(PartialMatch);
return;
}
- if (impliesSimpleEdit(IndexedRest.front().start, LexedRest.front().start)) {
+ if (impliesSimpleEdit(IndexedRest.front().start,
+ LexedRest.front().range().start)) {
PartialMatch.push_back(LexedIndex);
findNearMiss(PartialMatch, IndexedRest.drop_front(), LexedRest.drop_front(),
LexedIndex + 1, Fuel, MatchedCB);
@@ -746,6 +812,30 @@ void findNearMiss(
} // namespace
+SymbolRange::SymbolRange(Range R) : Ranges({R}) {}
+
+SymbolRange::SymbolRange(std::vector<Range> Ranges)
+ : Ranges(std::move(Ranges)) {}
+
+Range SymbolRange::range() const { return Ranges.front(); }
+
+bool operator==(const SymbolRange &LHS, const SymbolRange &RHS) {
+ return LHS.Ranges == RHS.Ranges;
+}
+bool operator!=(const SymbolRange &LHS, const SymbolRange &RHS) {
+ return !(LHS == RHS);
+}
+bool operator<(const SymbolRange &LHS, const SymbolRange &RHS) {
+ return LHS.range() < RHS.range();
+}
+
+std::vector<SymbolRange> symbolRanges(const std::vector<Range> Ranges) {
+ std::vector<SymbolRange> Result;
+ for (const auto &R : Ranges)
+ Result.emplace_back(R);
+ return Result;
+}
+
llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
assert(!RInputs.Index == !RInputs.FS &&
"Index and FS must either both be specified or both null.");
@@ -778,12 +868,44 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
return makeError(ReasonToReject::NoSymbolFound);
if (DeclsUnderCursor.size() > 1)
return makeError(ReasonToReject::AmbiguousSymbol);
+ std::string Placeholder;
+ // We expect the token under the cursor to be changed unless the user is
+ // renaming an Objective-C selector with multiple pieces and only renames
+ // some of the selector piece(s).
+ bool RenamingCurToken = true;
const auto &RenameDecl = **DeclsUnderCursor.begin();
- const auto *ID = RenameDecl.getIdentifier();
- if (!ID)
- return makeError(ReasonToReject::UnsupportedSymbol);
- if (ID->getName() == RInputs.NewName)
- return makeError(ReasonToReject::SameName);
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
+ const auto Sel = MD->getSelector();
+ if (Sel.getAsString() == RInputs.NewName)
+ return makeError(ReasonToReject::SameName);
+ if (Sel.getNumArgs() != RInputs.NewName.count(':') &&
+ RInputs.NewName != "__clangd_rename_placeholder")
+ return makeError(
+ InvalidName{InvalidName::BadIdentifier, RInputs.NewName.str()});
+ if (Sel.getNumArgs() > 1)
+ Placeholder = Sel.getAsString();
+
+ // See if the token under the cursor should actually be renamed.
+ if (RInputs.NewName != "__clangd_rename_placeholder") {
+ llvm::StringRef NewName = RInputs.NewName;
+ llvm::SmallVector<llvm::StringRef, 8> NewNames;
+ NewName.split(NewNames, ":");
+
+ unsigned NumSelectorLocs = MD->getNumSelectorLocs();
+ for (unsigned I = 0; I < NumSelectorLocs; ++I) {
+ if (MD->getSelectorLoc(I) == IdentifierToken->location()) {
+ RenamingCurToken = Sel.getNameForSlot(I) != NewNames[I];
+ break;
+ }
+ }
+ }
+ } else {
+ const auto *ID = RenameDecl.getIdentifier();
+ if (!ID)
+ return makeError(ReasonToReject::UnsupportedSymbol);
+ if (ID->getName() == RInputs.NewName)
+ return makeError(ReasonToReject::SameName);
+ }
auto Invalid = checkName(RenameDecl, RInputs.NewName);
if (Invalid)
return makeError(std::move(*Invalid));
@@ -817,7 +939,8 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
return StartOffset.takeError();
if (!EndOffset)
return EndOffset.takeError();
- if (llvm::none_of(
+ if (RenamingCurToken &&
+ llvm::none_of(
*MainFileRenameEdit,
[&StartOffset, &EndOffset](const clang::tooling::Replacement &R) {
return R.getOffset() == *StartOffset &&
@@ -827,6 +950,7 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
}
RenameResult Result;
Result.Target = CurrentIdentifier;
+ Result.Placeholder = Placeholder;
Edit MainFileEdits = Edit(MainFileCode, std::move(*MainFileRenameEdit));
for (const TextEdit &TE : MainFileEdits.asTextEdits())
Result.LocalChanges.push_back(TE.range);
@@ -862,8 +986,8 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
llvm::StringRef InitialCode,
- std::vector<Range> Occurrences,
- llvm::StringRef NewName) {
+ std::vector<SymbolRange> Occurrences,
+ llvm::ArrayRef<llvm::StringRef> NewNames) {
trace::Span Tracer("BuildRenameEdit");
SPAN_ATTACH(Tracer, "file_path", AbsFilePath);
SPAN_ATTACH(Tracer, "rename_occurrences",
@@ -893,22 +1017,41 @@ llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
return LastOffset;
};
- std::vector<std::pair</*start*/ size_t, /*end*/ size_t>> OccurrencesOffsets;
- for (const auto &R : Occurrences) {
- auto StartOffset = Offset(R.start);
- if (!StartOffset)
- return StartOffset.takeError();
- auto EndOffset = Offset(R.end);
- if (!EndOffset)
- return EndOffset.takeError();
- OccurrencesOffsets.push_back({*StartOffset, *EndOffset});
+ struct OccurrenceOffset {
+ size_t Start;
+ size_t End;
+ llvm::StringRef NewName;
+
+ OccurrenceOffset(size_t Start, size_t End, llvm::StringRef NewName)
+ : Start(Start), End(End), NewName(NewName) {}
+ };
+
+ std::vector<OccurrenceOffset> OccurrencesOffsets;
+ for (const auto &SR : Occurrences) {
+ for (auto It = SR.Ranges.begin(); It != SR.Ranges.end(); ++It) {
+ const auto &R = *It;
+ auto StartOffset = Offset(R.start);
+ if (!StartOffset)
+ return StartOffset.takeError();
+ auto EndOffset = Offset(R.end);
+ if (!EndOffset)
+ return EndOffset.takeError();
+ auto Index = It - SR.Ranges.begin();
+ // Nothing to do if the token/name hasn't changed.
+ auto CurName =
+ InitialCode.substr(*StartOffset, *EndOffset - *StartOffset);
+ if (CurName == NewNames[Index])
+ continue;
+ OccurrencesOffsets.emplace_back(*StartOffset, *EndOffset,
+ NewNames[Index]);
+ }
}
tooling::Replacements RenameEdit;
for (const auto &R : OccurrencesOffsets) {
- auto ByteLength = R.second - R.first;
+ auto ByteLength = R.End - R.Start;
if (auto Err = RenameEdit.add(
- tooling::Replacement(AbsFilePath, R.first, ByteLength, NewName)))
+ tooling::Replacement(AbsFilePath, R.Start, ByteLength, R.NewName)))
return std::move(Err);
}
return Edit(InitialCode, std::move(RenameEdit));
@@ -927,20 +1070,21 @@ llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
// ranges onto candidates in a plausible way (e.g. guess that lines
// were inserted). If such a "near miss" is found, the rename is still
// possible
-std::optional<std::vector<Range>>
+std::optional<std::vector<SymbolRange>>
adjustRenameRanges(llvm::StringRef DraftCode, llvm::StringRef Identifier,
- std::vector<Range> Indexed, const LangOptions &LangOpts) {
+ std::vector<Range> Indexed, const LangOptions &LangOpts,
+ std::optional<Selector> Selector) {
trace::Span Tracer("AdjustRenameRanges");
assert(!Indexed.empty());
assert(llvm::is_sorted(Indexed));
- std::vector<Range> Lexed =
- collectIdentifierRanges(Identifier, DraftCode, LangOpts);
+ std::vector<SymbolRange> Lexed =
+ collectRenameIdentifierRanges(Identifier, DraftCode, LangOpts, Selector);
llvm::sort(Lexed);
return getMappedRanges(Indexed, Lexed);
}
-std::optional<std::vector<Range>> getMappedRanges(ArrayRef<Range> Indexed,
- ArrayRef<Range> Lexed) {
+std::optional<std::vector<SymbolRange>>
+getMappedRanges(ArrayRef<Range> Indexed, ArrayRef<SymbolRange> Lexed) {
trace::Span Tracer("GetMappedRanges");
assert(!Indexed.empty());
assert(llvm::is_sorted(Indexed));
@@ -955,7 +1099,7 @@ std::optional<std::vector<Range>> getMappedRanges(ArrayRef<Range> Indexed,
}
// Fast check for the special subset case.
if (std::includes(Indexed.begin(), Indexed.end(), Lexed.begin(), Lexed.end()))
- return Indexed.vec();
+ return Lexed.vec();
std::vector<size_t> Best;
size_t BestCost = std::numeric_limits<size_t>::max();
@@ -985,7 +1129,7 @@ std::optional<std::vector<Range>> getMappedRanges(ArrayRef<Range> Indexed,
SPAN_ATTACH(Tracer, "error", "Didn't find a near miss");
return std::nullopt;
}
- std::vector<Range> Mapped;
+ std::vector<SymbolRange> Mapped;
for (auto I : Best)
Mapped.push_back(Lexed[I]);
SPAN_ATTACH(Tracer, "mapped_ranges", static_cast<int64_t>(Mapped.size()));
@@ -1007,7 +1151,8 @@ std::optional<std::vector<Range>> getMappedRanges(ArrayRef<Range> Indexed,
// diff[0]: line + 1 <- insert a line before edit 0.
// diff[1]: column + 1 <- remove a line between edits 0 and 1, and insert a
// character on edit 1.
-size_t renameRangeAdjustmentCost(ArrayRef<Range> Indexed, ArrayRef<Range> Lexed,
+size_t renameRangeAdjustmentCost(ArrayRef<Range> Indexed,
+ ArrayRef<SymbolRange> Lexed,
ArrayRef<size_t> MappedIndex) {
assert(Indexed.size() == MappedIndex.size());
assert(llvm::is_sorted(Indexed));
@@ -1017,9 +1162,10 @@ size_t renameRangeAdjustmentCost(ArrayRef<Range> Indexed, ArrayRef<Range> Lexed,
int LastDLine = 0, LastDColumn = 0;
int Cost = 0;
for (size_t I = 0; I < Indexed.size(); ++I) {
- int DLine = Indexed[I].start.line - Lexed[MappedIndex[I]].start.line;
- int DColumn =
- Indexed[I].start.character - Lexed[MappedIndex[I]].start.character;
+ int DLine =
+ Indexed[I].start.line - Lexed[MappedIndex[I]].range().start.line;
+ int DColumn = Indexed[I].start.character -
+ Lexed[MappedIndex[I]].range().start.character;
int Line = Indexed[I].start.line;
if (Line != LastLine)
LastDColumn = 0; // column offsets don't carry cross lines.
@@ -1029,5 +1175,151 @@ size_t renameRangeAdjustmentCost(ArrayRef<Range> Indexed, ArrayRef<Range> Lexed,
return Cost;
}
+static bool isMatchingSelectorName(const syntax::Token &Cur,
+ const syntax::Token &Next,
+ const SourceManager &SM,
+ llvm::StringRef SelectorName) {
+ if (SelectorName.empty())
+ return Cur.kind() == tok::colon;
+ return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ Cur.text(SM) == SelectorName &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+static bool isSelectorLike(const syntax::Token &Cur,
+ const syntax::Token &Next) {
+ return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+static void
+lex(llvm::StringRef Code, const LangOptions &LangOpts,
+ llvm::function_ref<void(const syntax::Token &, const SourceManager &SM)>
+ Action) {
+ // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!
+ std::string NullTerminatedCode = Code.str();
+ SourceManagerForFile FileSM("mock_file_name.cpp", NullTerminatedCode);
+ auto &SM = FileSM.get();
+ for (const auto &Tok : syntax::tokenize(SM.getMainFileID(), SM, LangOpts))
+ Action(Tok, SM);
+}
+
+std::vector<SymbolRange> collectRenameIdentifierRanges(
+ llvm::StringRef Identifier, llvm::StringRef Content,
+ const LangOptions &LangOpts, std::optional<Selector> Selector) {
+ std::vector<SymbolRange> Ranges;
+ if (!Selector) {
+ lex(Content, LangOpts,
+ [&](const syntax::Token &Tok, const SourceManager &SM) {
+ if (Tok.kind() != tok::identifier || Tok.text(SM) != Identifier)
+ return;
+ Ranges.emplace_back(
+ halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+ });
+ return Ranges;
+ }
+ // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!
+ std::string NullTerminatedCode = Content.str();
+ SourceManagerForFile FileSM("mock_file_name.cpp", NullTerminatedCode);
+ auto &SM = FileSM.get();
+
+ auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);
+ unsigned Last = Tokens.size() - 1;
+
+ // One parser state for top level and each `[]` pair, can be nested.
+ // Technically we should have a state or recursion for each ()/{} as well,
+ // but since we're expecting well formed code it shouldn't matter in practice.
+ struct ParserState {
+ unsigned ParenCount = 0;
+ unsigned BraceCount = 0;
+ std::vector<Range> Pieces;
+ };
+
+ // We have to track square brackets, parens and braces as we want to skip the
+ // tokens inside them. This ensures that we don't use identical selector
+ // pieces in inner message sends, blocks, lambdas and @selector expressions.
+ std::vector<ParserState> States = {ParserState()};
+ unsigned NumPieces = Selector->getNumArgs();
+
+ for (unsigned Index = 0; Index < Last; ++Index) {
+ auto &State = States.back();
+ auto &Pieces = State.Pieces;
+ const auto &Tok = Tokens[Index];
+ const auto Kind = Tok.kind();
+ auto PieceCount = Pieces.size();
+
+ if (State.ParenCount == 0) {
+ // Check for matches until we find all selector pieces.
+ if (PieceCount < NumPieces &&
+ isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
+ Selector->getNameForSlot(PieceCount))) {
+ if (!Selector->getNameForSlot(PieceCount).empty()) {
+ // Skip the ':' after the name. This ensures that it won't match a
+ // follow-up selector piece with an empty name.
+ ++Index;
+ }
+ Pieces.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+ continue;
+ }
+ // If we've found all pieces, we still need to try to consume more pieces
+ // as it's possible the selector being renamed is a prefix of this method
+ // name.
+ if (PieceCount >= NumPieces && isSelectorLike(Tok, Tokens[Index + 1])) {
+ ++Index;
+ Pieces.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+ continue;
+ }
+ }
+ switch (Kind) {
+ case tok::r_square:
+ if (States.size() > 1) {
+ // All the selector pieces in the method expr have been found.
+ if (Pieces.size() == NumPieces) {
+ Ranges.emplace_back(std::move(Pieces));
+ }
+ States.pop_back();
+ }
+ break;
+ case tok::l_square:
+ States.push_back(ParserState());
+ break;
+ case tok::l_paren:
+ State.ParenCount++;
+ break;
+ case tok::r_paren:
+ if (State.ParenCount > 0) {
+ --State.ParenCount;
+ }
+ break;
+ case tok::r_brace:
+ if (State.BraceCount > 0) {
+ --State.BraceCount;
+ }
+ break;
+ case tok::l_brace:
+ ++State.BraceCount;
+ LLVM_FALLTHROUGH;
+ case tok::semi:
+ // At the top level scope we should only have method decls/defs.
+ if (States.size() == 1) {
+ // All the selector pieces in the method decl/def have been found.
+ if (Pieces.size() == NumPieces) {
+ Ranges.emplace_back(std::move(Pieces));
+ }
+ // ; and { end method decl/defs.
+ Pieces.clear();
+ }
+ break;
+ default:
+ break;
+ }
+ }
+ return Ranges;
+}
+
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/refactor/Rename.h b/clang-tools-extra/clangd/refactor/Rename.h
index 91728ba59e5d84d..b5b9073aab66a09 100644
--- a/clang-tools-extra/clangd/refactor/Rename.h
+++ b/clang-tools-extra/clangd/refactor/Rename.h
@@ -11,7 +11,9 @@
#include "Protocol.h"
#include "SourceCode.h"
+#include "clang/Basic/IdentifierTable.h"
#include "clang/Basic/LangOptions.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Error.h"
#include <optional>
@@ -53,6 +55,8 @@ struct RenameInputs {
struct RenameResult {
// The range of the symbol that the user can attempt to rename.
Range Target;
+ // Placeholder text for the rename operation if non-empty.
+ std::string Placeholder;
// Rename occurrences for the current main file.
std::vector<Range> LocalChanges;
// Complete edits for the rename, including LocalChanges.
@@ -60,6 +64,27 @@ struct RenameResult {
FileEdits GlobalChanges;
};
+/// Represents a symbol range where the symbol can potentially have multiple
+/// tokens.
+struct SymbolRange {
+ /// Ranges for the tokens that make up the symbol's name.
+ /// Usually a single range, but there can be multiple ranges if the tokens for
+ /// the symbol are split, e.g. ObjC selectors.
+ std::vector<Range> Ranges;
+
+ SymbolRange(Range R);
+ SymbolRange(std::vector<Range> Ranges);
+
+ /// Returns the first range.
+ Range range() const;
+
+ friend bool operator==(const SymbolRange &LHS, const SymbolRange &RHS);
+ friend bool operator!=(const SymbolRange &LHS, const SymbolRange &RHS);
+ friend bool operator<(const SymbolRange &LHS, const SymbolRange &RHS);
+};
+
+std::vector<SymbolRange> symbolRanges(const std::vector<Range> Ranges);
+
/// Renames all occurrences of the symbol. The result edits are unformatted.
/// If AllowCrossFile is false, returns an error if rename a symbol that's used
/// in another file (per the index).
@@ -71,8 +96,8 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs);
/// REQUIRED: Occurrences is sorted and doesn't have duplicated ranges.
llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
llvm::StringRef InitialCode,
- std::vector<Range> Occurrences,
- llvm::StringRef NewName);
+ std::vector<SymbolRange> Occurrences,
+ llvm::ArrayRef<llvm::StringRef> NewNames);
/// Adjusts indexed occurrences to match the current state of the file.
///
@@ -84,9 +109,10 @@ llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
/// The API assumes that Indexed contains only named occurrences (each
/// occurrence has the same length).
/// REQUIRED: Indexed is sorted.
-std::optional<std::vector<Range>>
+std::optional<std::vector<SymbolRange>>
adjustRenameRanges(llvm::StringRef DraftCode, llvm::StringRef Identifier,
- std::vector<Range> Indexed, const LangOptions &LangOpts);
+ std::vector<Range> Indexed, const LangOptions &LangOpts,
+ std::optional<Selector> Selector);
/// Calculates the lexed occurrences that the given indexed occurrences map to.
/// Returns std::nullopt if we don't find a mapping.
@@ -94,15 +120,16 @@ adjustRenameRanges(llvm::StringRef DraftCode, llvm::StringRef Identifier,
/// Exposed for testing only.
///
/// REQUIRED: Indexed and Lexed are sorted.
-std::optional<std::vector<Range>> getMappedRanges(ArrayRef<Range> Indexed,
- ArrayRef<Range> Lexed);
+std::optional<std::vector<SymbolRange>>
+getMappedRanges(ArrayRef<Range> Indexed, ArrayRef<SymbolRange> Lexed);
/// Evaluates how good the mapped result is. 0 indicates a perfect match.
///
/// Exposed for testing only.
///
/// REQUIRED: Indexed and Lexed are sorted, Indexed and MappedIndex have the
/// same size.
-size_t renameRangeAdjustmentCost(ArrayRef<Range> Indexed, ArrayRef<Range> Lexed,
+size_t renameRangeAdjustmentCost(ArrayRef<Range> Indexed,
+ ArrayRef<SymbolRange> Lexed,
ArrayRef<size_t> MappedIndex);
} // namespace clangd
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index 9cbf59684fbc107..a0a271a911c6ae9 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -926,12 +926,38 @@ TEST(RenameTest, Renameable) {
void f(X x) {x+^+;})cpp",
"no symbol", HeaderFile},
- {R"cpp(// disallow rename on non-normal identifiers.
+ {R"cpp(
@interface Foo {}
- -(int) fo^o:(int)x; // Token is an identifier, but declaration name isn't a simple identifier.
+ - (int)[[fo^o]]:(int)x;
@end
)cpp",
- "not a supported kind", HeaderFile},
+ nullptr, HeaderFile, "newName:"},
+ {R"cpp(//disallow as : count must match
+ @interface Foo {}
+ - (int)fo^o:(int)x;
+ @end
+ )cpp",
+ "invalid name: the chosen name \"MockName\" is not a valid identifier",
+ HeaderFile},
+ {R"cpp(
+ @interface Foo {}
+ - (int)[[o^ne]]:(int)one two:(int)two;
+ @end
+ )cpp",
+ nullptr, HeaderFile, "a:two:"},
+ {R"cpp(
+ @interface Foo {}
+ - (int)[[o^ne]]:(int)one [[two]]:(int)two;
+ @end
+ )cpp",
+ nullptr, HeaderFile, "a:b:"},
+ {R"cpp(
+ @interface Foo {}
+ - (int)o^ne:(int)one [[two]]:(int)two;
+ @end
+ )cpp",
+ nullptr, HeaderFile, "one:three:"},
+
{R"cpp(
void foo(int);
void foo(char);
@@ -1137,7 +1163,7 @@ TEST(RenameTest, Renameable) {
int [[V^ar]];
}
)cpp",
- nullptr, !HeaderFile},
+ nullptr, !HeaderFile},
};
for (const auto& Case : Cases) {
@@ -1778,8 +1804,8 @@ TEST(CrossFileRenameTests, BuildRenameEdits) {
Annotations Code("[[😂]]");
auto LSPRange = Code.range();
llvm::StringRef FilePath = "/test/TestTU.cpp";
- llvm::StringRef NewName = "abc";
- auto Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewName);
+ llvm::SmallVector<llvm::StringRef, 2> NewNames = {"abc"};
+ auto Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewNames);
ASSERT_TRUE(bool(Edit)) << Edit.takeError();
ASSERT_EQ(1UL, Edit->Replacements.size());
EXPECT_EQ(FilePath, Edit->Replacements.begin()->getFilePath());
@@ -1787,7 +1813,7 @@ TEST(CrossFileRenameTests, BuildRenameEdits) {
// Test invalid range.
LSPRange.end = {10, 0}; // out of range
- Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewName);
+ Edit = buildRenameEdit(FilePath, Code.code(), {LSPRange}, NewNames);
EXPECT_FALSE(Edit);
EXPECT_THAT(llvm::toString(Edit.takeError()),
testing::HasSubstr("fail to convert"));
@@ -1798,10 +1824,11 @@ TEST(CrossFileRenameTests, BuildRenameEdits) {
[[range]]
[[range]]
)cpp");
- Edit = buildRenameEdit(FilePath, T.code(), T.ranges(), NewName);
+ Edit =
+ buildRenameEdit(FilePath, T.code(), symbolRanges(T.ranges()), NewNames);
ASSERT_TRUE(bool(Edit)) << Edit.takeError();
EXPECT_EQ(applyEdits(FileEdits{{T.code(), std::move(*Edit)}}).front().second,
- expectedResult(T, NewName));
+ expectedResult(T, NewNames[0]));
}
TEST(CrossFileRenameTests, adjustRenameRanges) {
@@ -1855,8 +1882,9 @@ TEST(CrossFileRenameTests, adjustRenameRanges) {
for (const auto &T : Tests) {
SCOPED_TRACE(T.DraftCode);
Annotations Draft(T.DraftCode);
- auto ActualRanges = adjustRenameRanges(
- Draft.code(), "x", Annotations(T.IndexedCode).ranges(), LangOpts);
+ auto ActualRanges = adjustRenameRanges(Draft.code(), "x",
+ Annotations(T.IndexedCode).ranges(),
+ LangOpts, std::nullopt);
if (!ActualRanges)
EXPECT_THAT(Draft.ranges(), testing::IsEmpty());
else
@@ -1970,11 +1998,11 @@ TEST(RangePatchingHeuristic, GetMappedRanges) {
for (const auto &T : Tests) {
SCOPED_TRACE(T.IndexedCode);
auto Lexed = Annotations(T.LexedCode);
- auto LexedRanges = Lexed.ranges();
- std::vector<Range> ExpectedMatches;
+ auto LexedRanges = symbolRanges(Lexed.ranges());
+ std::vector<SymbolRange> ExpectedMatches;
for (auto P : Lexed.points()) {
- auto Match = llvm::find_if(LexedRanges, [&P](const Range& R) {
- return R.start == P;
+ auto Match = llvm::find_if(LexedRanges, [&P](const SymbolRange &R) {
+ return R.range().start == P;
});
ASSERT_NE(Match, LexedRanges.end());
ExpectedMatches.push_back(*Match);
@@ -2093,8 +2121,8 @@ TEST(CrossFileRenameTests, adjustmentCost) {
std::vector<size_t> MappedIndex;
for (size_t I = 0; I < C.ranges("lex").size(); ++I)
MappedIndex.push_back(I);
- EXPECT_EQ(renameRangeAdjustmentCost(C.ranges("idx"), C.ranges("lex"),
- MappedIndex),
+ EXPECT_EQ(renameRangeAdjustmentCost(
+ C.ranges("idx"), symbolRanges(C.ranges("lex")), MappedIndex),
T.ExpectedCost);
}
}
diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
index 5c20b950e4eac0d..1e7a30e69fabe09 100644
--- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -565,7 +565,7 @@ TEST_F(SymbolCollectorTest, ObjCRefs) {
ElementsAre(isSpelled()))));
EXPECT_THAT(Refs,
Contains(Pair(findSymbol(Symbols, "Person::multiArg:method:").ID,
- ElementsAre(Not(isSpelled())))));
+ ElementsAre(isSpelled()))));
}
TEST_F(SymbolCollectorTest, ObjCSymbols) {
>From 6fc1f98412b8c518138c7ef052cf04de88fc7855 Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Tue, 23 Jan 2024 13:58:02 -0500
Subject: [PATCH 4/7] Use llvm::zip
---
clang-tools-extra/clangd/refactor/Rename.cpp | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index e8e7517ac7fe61c..997e13f2a20c8d8 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -1028,22 +1028,19 @@ llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,
std::vector<OccurrenceOffset> OccurrencesOffsets;
for (const auto &SR : Occurrences) {
- for (auto It = SR.Ranges.begin(); It != SR.Ranges.end(); ++It) {
- const auto &R = *It;
- auto StartOffset = Offset(R.start);
+ for (auto [Range, NewName] : llvm::zip(SR.Ranges, NewNames)) {
+ auto StartOffset = Offset(Range.start);
if (!StartOffset)
return StartOffset.takeError();
- auto EndOffset = Offset(R.end);
+ auto EndOffset = Offset(Range.end);
if (!EndOffset)
return EndOffset.takeError();
- auto Index = It - SR.Ranges.begin();
// Nothing to do if the token/name hasn't changed.
auto CurName =
InitialCode.substr(*StartOffset, *EndOffset - *StartOffset);
- if (CurName == NewNames[Index])
+ if (CurName == NewName)
continue;
- OccurrencesOffsets.emplace_back(*StartOffset, *EndOffset,
- NewNames[Index]);
+ OccurrencesOffsets.emplace_back(*StartOffset, *EndOffset, NewName);
}
}
>From be81025f01b03459cdd7e1b8602e2887ab74b272 Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Thu, 25 Jan 2024 12:40:01 -0500
Subject: [PATCH 5/7] Fixes for review; minor changes + parser change
---
clang-tools-extra/clangd/Protocol.h | 22 +
clang-tools-extra/clangd/refactor/Rename.cpp | 503 ++++++++++--------
clang-tools-extra/clangd/refactor/Rename.h | 2 -
clang-tools-extra/clangd/test/rename.test | 3 +
.../clangd/unittests/RenameTests.cpp | 7 +
5 files changed, 311 insertions(+), 226 deletions(-)
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index 8fcfcd5e33f6ce3..4cb1752689f9b2a 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -1977,6 +1977,28 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &, const ASTNode &);
} // namespace clang
namespace llvm {
+
+template <> struct DenseMapInfo<clang::clangd::Range> {
+ using Range = clang::clangd::Range;
+ static inline Range getEmptyKey() {
+ static clang::clangd::Position Tomb{-1, -1};
+ static Range R{Tomb, Tomb};
+ return R;
+ }
+ static inline Range getTombstoneKey() {
+ static clang::clangd::Position Tomb{-2, -2};
+ static Range R{Tomb, Tomb};
+ return R;
+ }
+ static unsigned getHashValue(const Range &Val) {
+ return llvm::hash_combine(
+ Val.start.line, Val.start.character, Val.end.line, Val.end.character);
+ }
+ static bool isEqual(const Range &LHS, const Range &RHS) {
+ return std::tie(LHS.start, LHS.end) == std::tie(RHS.start, RHS.end);
+ }
+};
+
template <> struct format_provider<clang::clangd::Position> {
static void format(const clang::clangd::Position &Pos, raw_ostream &OS,
StringRef Style) {
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 997e13f2a20c8d8..64c3cec28454006 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -38,11 +38,6 @@
namespace clang {
namespace clangd {
-
-std::vector<SymbolRange> collectRenameIdentifierRanges(
- llvm::StringRef Identifier, llvm::StringRef Content,
- const LangOptions &LangOpts, std::optional<Selector> Selector);
-
namespace {
std::optional<std::string> filePath(const SymbolLocation &Loc,
@@ -227,6 +222,11 @@ std::optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
return ReasonToReject::UnsupportedSymbol;
}
}
+ // We allow renaming ObjC methods although they don't have a simple
+ // identifier.
+ const auto *ID = RenameDecl.getIdentifier();
+ if (!ID && !isa<ObjCMethodDecl>(&RenameDecl))
+ return ReasonToReject::UnsupportedSymbol;
// Filter out symbols that are unsupported in both rename modes.
if (llvm::isa<NamespaceDecl>(&RenameDecl))
return ReasonToReject::UnsupportedSymbol;
@@ -521,13 +521,34 @@ static bool mayBeValidIdentifier(llvm::StringRef Ident, bool AllowColon) {
return true;
}
+std::string getName(const NamedDecl &RenameDecl) {
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl))
+ return MD->getSelector().getAsString();
+ if (const auto *ID = RenameDecl.getIdentifier())
+ return ID->getName().str();
+ return "";
+}
+
// Check if we can rename the given RenameDecl into NewName.
// Return details if the rename would produce a conflict.
-std::optional<InvalidName> checkName(const NamedDecl &RenameDecl,
- llvm::StringRef NewName) {
+std::optional<llvm::Error> checkName(const NamedDecl &RenameDecl,
+ llvm::StringRef NewName,
+ llvm::StringRef OldName) {
trace::Span Tracer("CheckName");
static constexpr trace::Metric InvalidNameMetric(
"rename_name_invalid", trace::Metric::Counter, "invalid_kind");
+
+ if (OldName == NewName)
+ return makeError(ReasonToReject::SameName);
+
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
+ const auto Sel = MD->getSelector();
+ if (Sel.getNumArgs() != NewName.count(':') &&
+ NewName != "__clangd_rename_placeholder")
+ return makeError(
+ InvalidName{InvalidName::BadIdentifier, NewName.str()});
+ }
+
auto &ASTCtx = RenameDecl.getASTContext();
std::optional<InvalidName> Result;
if (isKeyword(NewName, ASTCtx.getLangOpts()))
@@ -545,16 +566,218 @@ std::optional<InvalidName> checkName(const NamedDecl &RenameDecl,
Conflict->getLocation().printToString(ASTCtx.getSourceManager())};
}
}
- if (Result)
+ if (Result) {
InvalidNameMetric.record(1, toString(Result->K));
- return Result;
+ return makeError(*Result);
+ }
+ return std::nullopt;
+}
+
+bool isMatchingSelectorName(const syntax::Token &Cur,
+ const syntax::Token &Next,
+ const SourceManager &SM,
+ llvm::StringRef SelectorName) {
+ if (SelectorName.empty())
+ return Cur.kind() == tok::colon;
+ return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ Cur.text(SM) == SelectorName &&
+ // We require the selector name and : to be contiguous to avoid
+ // potential conflicts with ternary expression.
+ //
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool isSelectorLike(const syntax::Token &Cur,
+ const syntax::Token &Next) {
+ return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
+ // We require the selector name and : to be contiguous.
+ // e.g. support `foo:` but not `foo :`.
+ Cur.endLocation() == Next.location();
+}
+
+bool parseMessageExpression(
+ llvm::ArrayRef<syntax::Token> Tokens, const SourceManager &SM,
+ unsigned Index, unsigned Last,
+ Selector Sel, std::vector<Range> &SelectorPieces) {
+
+ unsigned NumArgs = Sel.getNumArgs();
+ llvm::SmallVector<char, 8> Closes;
+ SelectorPieces.clear();
+ while (Index < Last) {
+ const auto &Tok = Tokens[Index];
+
+ if (Closes.empty()) {
+ auto PieceCount = SelectorPieces.size();
+ if (PieceCount < NumArgs &&
+ isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
+ Sel.getNameForSlot(PieceCount))) {
+ // If 'foo:' instead of ':' (empty selector), we need to skip the ':'
+ // token after the name.
+ if (!Sel.getNameForSlot(PieceCount).empty()) {
+ ++Index;
+ }
+ SelectorPieces.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+ continue;
+ }
+ // If we've found all pieces but the current token looks like another
+ // selector piece, it means the method being renamed is a strict prefix of
+ // the selector we've found - should be skipped.
+ if (SelectorPieces.size() >= NumArgs && isSelectorLike(Tok, Tokens[Index + 1]))
+ return false;
+ }
+
+ switch (Tok.kind()) {
+ case tok::l_square:
+ Closes.push_back(']');
+ break;
+ case tok::l_paren:
+ Closes.push_back(')');
+ break;
+ case tok::l_brace:
+ Closes.push_back('}');
+ break;
+ case tok::r_square:
+ if (Closes.empty())
+ return SelectorPieces.size() == NumArgs;
+
+ if (Closes.back() != ']')
+ return false;
+ Closes.pop_back();
+ break;
+ case tok::r_paren:
+ if (Closes.empty() || Closes.back() != ')')
+ return false;
+ Closes.pop_back();
+ break;
+ case tok::r_brace:
+ if (Closes.empty() || Closes.back() != '}')
+ return false;
+ Closes.pop_back();
+ break;
+ case tok::semi:
+ // top level ; ends all statements.
+ if (Closes.empty())
+ return false;
+ break;
+ default:
+ break;
+ }
+
+ ++Index;
+ }
+ return false;
+}
+
+/// Collects all ranges of the given identifier/selector in the source code.
+///
+/// If a selector is given, this does a full lex of the given source code in
+/// order to identify all selector fragments (e.g. in method exprs/decls) since
+/// they are non-contiguous.
+std::vector<SymbolRange> collectRenameIdentifierRanges(
+ llvm::StringRef Identifier, llvm::StringRef Content,
+ const LangOptions &LangOpts, std::optional<Selector> Selector) {
+ std::vector<SymbolRange> Ranges;
+ if (!Selector) {
+ auto IdentifierRanges = collectIdentifierRanges(Identifier, Content, LangOpts);
+ for (const auto &R : IdentifierRanges)
+ Ranges.emplace_back(R);
+ return Ranges;
+ }
+ // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!
+ std::string NullTerminatedCode = Content.str();
+ SourceManagerForFile FileSM("mock_file_name.cpp", NullTerminatedCode);
+ auto &SM = FileSM.get();
+
+ // We track parens and braces to ensure that we don't accidentally try parsing
+ // a method declaration or definition which isn't at the top level or similar
+ // looking expressions (e.g. an @selector() expression).
+ unsigned ParenCount = 0;
+ unsigned BraceCount = 0;
+ unsigned NumArgs = Selector->getNumArgs();
+
+ std::vector<Range> SelectorPieces;
+ auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);
+ unsigned Last = Tokens.size() - 1;
+ for (unsigned Index = 0; Index < Last; ++Index) {
+ const auto &Tok = Tokens[Index];
+
+ if (BraceCount == 0 && ParenCount == 0) {
+ auto PieceCount = SelectorPieces.size();
+ if (PieceCount < NumArgs &&
+ isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
+ Selector->getNameForSlot(PieceCount))) {
+ // If 'foo:' instead of ':' (empty selector), we need to skip the ':'
+ // token after the name.
+ if (!Selector->getNameForSlot(PieceCount).empty()) {
+ ++Index;
+ }
+ SelectorPieces.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+ continue;
+ }
+ // If we've found all pieces, we still need to try to consume more pieces
+ // as it's possible the selector being renamed is a prefix of this method
+ // name.
+ if (PieceCount >= NumArgs && isSelectorLike(Tok, Tokens[Index + 1])) {
+ ++Index;
+ SelectorPieces.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+ continue;
+ }
+ }
+
+ switch (Tok.kind()) {
+ case tok::l_square:
+ if (parseMessageExpression(Tokens, SM, Index + 1, Last, *Selector, SelectorPieces)) {
+ Ranges.emplace_back(std::move(SelectorPieces));
+ SelectorPieces.clear();
+ }
+ break;
+ case tok::l_paren:
+ ParenCount++;
+ break;
+ case tok::r_paren:
+ if (ParenCount > 0) {
+ --ParenCount;
+ }
+ break;
+ case tok::r_brace:
+ if (BraceCount > 0) {
+ --BraceCount;
+ }
+ break;
+ case tok::l_brace:
+ // At the top level scope we should only have method defs.
+ if (BraceCount == 0 && ParenCount == 0) {
+ // All the selector pieces in the method def have been found.
+ if (SelectorPieces.size() == NumArgs) {
+ Ranges.emplace_back(std::move(SelectorPieces));
+ }
+ SelectorPieces.clear();
+ }
+ ++BraceCount;
+ break;
+ case tok::semi:
+ // At the top level scope we should only have method decls.
+ if (BraceCount == 0 && ParenCount == 0) {
+ // All the selector pieces in the method decl have been found.
+ if (SelectorPieces.size() == NumArgs) {
+ Ranges.emplace_back(std::move(SelectorPieces));
+ }
+ SelectorPieces.clear();
+ }
+ break;
+ default:
+ break;
+ }
+ }
+ return Ranges;
}
clangd::Range tokenRangeForLoc(ParsedAST &AST, SourceLocation TokLoc,
const SourceManager &SM,
const LangOptions &LangOpts) {
const auto *Token = AST.getTokens().spelledTokenAt(TokLoc);
- assert(Token && "got inclusion at wrong offset");
+ assert(Token && "rename expects spelled tokens");
clangd::Range Result;
Result.start = sourceLocToPosition(SM, Token->location());
Result.end = sourceLocToPosition(SM, Token->endLocation());
@@ -566,18 +789,16 @@ clangd::Range tokenRangeForLoc(ParsedAST &AST, SourceLocation TokLoc,
llvm::Expected<tooling::Replacements>
renameObjCMethodWithinFile(ParsedAST &AST, const ObjCMethodDecl *MD,
llvm::StringRef NewName,
- std::vector<SourceLocation> Locs) {
+ std::vector<SourceLocation> SelectorOccurences) {
const SourceManager &SM = AST.getSourceManager();
auto Code = SM.getBufferData(SM.getMainFileID());
auto RenameIdentifier = MD->getSelector().getNameForSlot(0).str();
llvm::SmallVector<llvm::StringRef, 8> NewNames;
NewName.split(NewNames, ":");
- if (NewNames.empty())
- NewNames.push_back(NewName);
std::vector<Range> Ranges;
const auto &LangOpts = MD->getASTContext().getLangOpts();
- for (const auto &Loc : Locs)
+ for (const auto &Loc : SelectorOccurences)
Ranges.push_back(tokenRangeForLoc(AST, Loc, SM, LangOpts));
auto FilePath = AST.tuPath();
auto RenameRanges = collectRenameIdentifierRanges(
@@ -740,12 +961,9 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
if (MD->getSelector().getNumArgs() > 1) {
RenameIdentifier = MD->getSelector().getNameForSlot(0).str();
Selector = MD->getSelector();
- NewName.split(NewNames, ":");
}
}
- if (NewNames.empty()) {
- NewNames.push_back(NewName);
- }
+ NewName.split(NewNames, ":");
auto AffectedFileCode = (*ExpBuffer)->getBuffer();
auto RenameRanges =
@@ -829,13 +1047,6 @@ bool operator<(const SymbolRange &LHS, const SymbolRange &RHS) {
return LHS.range() < RHS.range();
}
-std::vector<SymbolRange> symbolRanges(const std::vector<Range> Ranges) {
- std::vector<SymbolRange> Result;
- for (const auto &R : Ranges)
- Result.emplace_back(R);
- return Result;
-}
-
llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
assert(!RInputs.Index == !RInputs.FS &&
"Index and FS must either both be specified or both null.");
@@ -868,47 +1079,12 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
return makeError(ReasonToReject::NoSymbolFound);
if (DeclsUnderCursor.size() > 1)
return makeError(ReasonToReject::AmbiguousSymbol);
- std::string Placeholder;
- // We expect the token under the cursor to be changed unless the user is
- // renaming an Objective-C selector with multiple pieces and only renames
- // some of the selector piece(s).
- bool RenamingCurToken = true;
+
const auto &RenameDecl = **DeclsUnderCursor.begin();
- if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
- const auto Sel = MD->getSelector();
- if (Sel.getAsString() == RInputs.NewName)
- return makeError(ReasonToReject::SameName);
- if (Sel.getNumArgs() != RInputs.NewName.count(':') &&
- RInputs.NewName != "__clangd_rename_placeholder")
- return makeError(
- InvalidName{InvalidName::BadIdentifier, RInputs.NewName.str()});
- if (Sel.getNumArgs() > 1)
- Placeholder = Sel.getAsString();
-
- // See if the token under the cursor should actually be renamed.
- if (RInputs.NewName != "__clangd_rename_placeholder") {
- llvm::StringRef NewName = RInputs.NewName;
- llvm::SmallVector<llvm::StringRef, 8> NewNames;
- NewName.split(NewNames, ":");
-
- unsigned NumSelectorLocs = MD->getNumSelectorLocs();
- for (unsigned I = 0; I < NumSelectorLocs; ++I) {
- if (MD->getSelectorLoc(I) == IdentifierToken->location()) {
- RenamingCurToken = Sel.getNameForSlot(I) != NewNames[I];
- break;
- }
- }
- }
- } else {
- const auto *ID = RenameDecl.getIdentifier();
- if (!ID)
- return makeError(ReasonToReject::UnsupportedSymbol);
- if (ID->getName() == RInputs.NewName)
- return makeError(ReasonToReject::SameName);
- }
- auto Invalid = checkName(RenameDecl, RInputs.NewName);
+ std::string Placeholder = getName(RenameDecl);
+ auto Invalid = checkName(RenameDecl, RInputs.NewName, Placeholder);
if (Invalid)
- return makeError(std::move(*Invalid));
+ return std::move(*Invalid);
auto Reject =
renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index, Opts);
@@ -928,25 +1104,50 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
if (!MainFileRenameEdit)
return MainFileRenameEdit.takeError();
+ llvm::DenseSet<Range> RenamedRanges;
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
+ // TODO: Insert the ranges from the ObjCMethodDecl/ObjCMessageExpr selector
+ // pieces which are being renamed. This will require us to make changes to
+ // locateDeclAt to preserve this AST node.
+ //
+ // if (RInputs.NewName != "__clangd_rename_placeholder") {
+ // llvm::StringRef NewName = RInputs.NewName;
+ // llvm::SmallVector<llvm::StringRef, 8> NewNames;
+ // NewName.split(NewNames, ":");
+
+ // const auto Sel = MD->getSelector();
+ // unsigned NumSelectorLocs = MD->getNumSelectorLocs();
+ // for (unsigned I = 0; I < NumSelectorLocs; ++I) {
+ // if (Sel.getNameForSlot(I) != NewNames[I]) {
+ // RenamedRanges.insert(
+ // tokenRangeForLoc(AST, MD->getSelectorLoc(I), SM, LangOpts));
+ // }
+ // }
+ // }
+ } else {
+ RenamedRanges.insert(CurrentIdentifier);
+ }
+
// Check the rename-triggering location is actually being renamed.
// This is a robustness check to avoid surprising rename results -- if the
// the triggering location is not actually the name of the node we identified
// (e.g. for broken code), then rename is likely not what users expect, so we
// reject this kind of rename.
- auto StartOffset = positionToOffset(MainFileCode, CurrentIdentifier.start);
- auto EndOffset = positionToOffset(MainFileCode, CurrentIdentifier.end);
- if (!StartOffset)
- return StartOffset.takeError();
- if (!EndOffset)
- return EndOffset.takeError();
- if (RenamingCurToken &&
- llvm::none_of(
- *MainFileRenameEdit,
- [&StartOffset, &EndOffset](const clang::tooling::Replacement &R) {
- return R.getOffset() == *StartOffset &&
- R.getLength() == *EndOffset - *StartOffset;
- })) {
- return makeError(ReasonToReject::NoSymbolFound);
+ for (const auto &Range : RenamedRanges) {
+ auto StartOffset = positionToOffset(MainFileCode, Range.start);
+ auto EndOffset = positionToOffset(MainFileCode, Range.end);
+ if (!StartOffset)
+ return StartOffset.takeError();
+ if (!EndOffset)
+ return EndOffset.takeError();
+ if (llvm::none_of(
+ *MainFileRenameEdit,
+ [&StartOffset, &EndOffset](const clang::tooling::Replacement &R) {
+ return R.getOffset() == *StartOffset &&
+ R.getLength() == *EndOffset - *StartOffset;
+ })) {
+ return makeError(ReasonToReject::NoSymbolFound);
+ }
}
RenameResult Result;
Result.Target = CurrentIdentifier;
@@ -1172,151 +1373,5 @@ size_t renameRangeAdjustmentCost(ArrayRef<Range> Indexed,
return Cost;
}
-static bool isMatchingSelectorName(const syntax::Token &Cur,
- const syntax::Token &Next,
- const SourceManager &SM,
- llvm::StringRef SelectorName) {
- if (SelectorName.empty())
- return Cur.kind() == tok::colon;
- return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
- Cur.text(SM) == SelectorName &&
- // We require the selector name and : to be contiguous.
- // e.g. support `foo:` but not `foo :`.
- Cur.endLocation() == Next.location();
-}
-
-static bool isSelectorLike(const syntax::Token &Cur,
- const syntax::Token &Next) {
- return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
- // We require the selector name and : to be contiguous.
- // e.g. support `foo:` but not `foo :`.
- Cur.endLocation() == Next.location();
-}
-
-static void
-lex(llvm::StringRef Code, const LangOptions &LangOpts,
- llvm::function_ref<void(const syntax::Token &, const SourceManager &SM)>
- Action) {
- // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!
- std::string NullTerminatedCode = Code.str();
- SourceManagerForFile FileSM("mock_file_name.cpp", NullTerminatedCode);
- auto &SM = FileSM.get();
- for (const auto &Tok : syntax::tokenize(SM.getMainFileID(), SM, LangOpts))
- Action(Tok, SM);
-}
-
-std::vector<SymbolRange> collectRenameIdentifierRanges(
- llvm::StringRef Identifier, llvm::StringRef Content,
- const LangOptions &LangOpts, std::optional<Selector> Selector) {
- std::vector<SymbolRange> Ranges;
- if (!Selector) {
- lex(Content, LangOpts,
- [&](const syntax::Token &Tok, const SourceManager &SM) {
- if (Tok.kind() != tok::identifier || Tok.text(SM) != Identifier)
- return;
- Ranges.emplace_back(
- halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
- });
- return Ranges;
- }
- // FIXME: InMemoryFileAdapter crashes unless the buffer is null terminated!
- std::string NullTerminatedCode = Content.str();
- SourceManagerForFile FileSM("mock_file_name.cpp", NullTerminatedCode);
- auto &SM = FileSM.get();
-
- auto Tokens = syntax::tokenize(SM.getMainFileID(), SM, LangOpts);
- unsigned Last = Tokens.size() - 1;
-
- // One parser state for top level and each `[]` pair, can be nested.
- // Technically we should have a state or recursion for each ()/{} as well,
- // but since we're expecting well formed code it shouldn't matter in practice.
- struct ParserState {
- unsigned ParenCount = 0;
- unsigned BraceCount = 0;
- std::vector<Range> Pieces;
- };
-
- // We have to track square brackets, parens and braces as we want to skip the
- // tokens inside them. This ensures that we don't use identical selector
- // pieces in inner message sends, blocks, lambdas and @selector expressions.
- std::vector<ParserState> States = {ParserState()};
- unsigned NumPieces = Selector->getNumArgs();
-
- for (unsigned Index = 0; Index < Last; ++Index) {
- auto &State = States.back();
- auto &Pieces = State.Pieces;
- const auto &Tok = Tokens[Index];
- const auto Kind = Tok.kind();
- auto PieceCount = Pieces.size();
-
- if (State.ParenCount == 0) {
- // Check for matches until we find all selector pieces.
- if (PieceCount < NumPieces &&
- isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Selector->getNameForSlot(PieceCount))) {
- if (!Selector->getNameForSlot(PieceCount).empty()) {
- // Skip the ':' after the name. This ensures that it won't match a
- // follow-up selector piece with an empty name.
- ++Index;
- }
- Pieces.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
- continue;
- }
- // If we've found all pieces, we still need to try to consume more pieces
- // as it's possible the selector being renamed is a prefix of this method
- // name.
- if (PieceCount >= NumPieces && isSelectorLike(Tok, Tokens[Index + 1])) {
- ++Index;
- Pieces.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
- continue;
- }
- }
- switch (Kind) {
- case tok::r_square:
- if (States.size() > 1) {
- // All the selector pieces in the method expr have been found.
- if (Pieces.size() == NumPieces) {
- Ranges.emplace_back(std::move(Pieces));
- }
- States.pop_back();
- }
- break;
- case tok::l_square:
- States.push_back(ParserState());
- break;
- case tok::l_paren:
- State.ParenCount++;
- break;
- case tok::r_paren:
- if (State.ParenCount > 0) {
- --State.ParenCount;
- }
- break;
- case tok::r_brace:
- if (State.BraceCount > 0) {
- --State.BraceCount;
- }
- break;
- case tok::l_brace:
- ++State.BraceCount;
- LLVM_FALLTHROUGH;
- case tok::semi:
- // At the top level scope we should only have method decls/defs.
- if (States.size() == 1) {
- // All the selector pieces in the method decl/def have been found.
- if (Pieces.size() == NumPieces) {
- Ranges.emplace_back(std::move(Pieces));
- }
- // ; and { end method decl/defs.
- Pieces.clear();
- }
- break;
- default:
- break;
- }
- }
- return Ranges;
-}
-
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/refactor/Rename.h b/clang-tools-extra/clangd/refactor/Rename.h
index b5b9073aab66a09..be5c346ae150f78 100644
--- a/clang-tools-extra/clangd/refactor/Rename.h
+++ b/clang-tools-extra/clangd/refactor/Rename.h
@@ -83,8 +83,6 @@ struct SymbolRange {
friend bool operator<(const SymbolRange &LHS, const SymbolRange &RHS);
};
-std::vector<SymbolRange> symbolRanges(const std::vector<Range> Ranges);
-
/// Renames all occurrences of the symbol. The result edits are unformatted.
/// If AllowCrossFile is false, returns an error if rename a symbol that's used
/// in another file (per the index).
diff --git a/clang-tools-extra/clangd/test/rename.test b/clang-tools-extra/clangd/test/rename.test
index 527b4263443a70f..0dc1a103002b26a 100644
--- a/clang-tools-extra/clangd/test/rename.test
+++ b/clang-tools-extra/clangd/test/rename.test
@@ -10,6 +10,8 @@
# CHECK: "id": 1,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": {
+# CHECK-NEXT: "placeholder": "foo",
+# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 7,
# CHECK-NEXT: "line": 0
@@ -18,6 +20,7 @@
# CHECK-NEXT: "character": 4,
# CHECK-NEXT: "line": 0
# CHECK-NEXT: }
+# CHECK-NEXT: }
# CHECK-NEXT: }
---
{"jsonrpc":"2.0","id":2,"method":"textDocument/prepareRename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2}}}
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index a0a271a911c6ae9..f76ad1513c45cdb 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -103,6 +103,13 @@ std::string expectedResult(Annotations Test, llvm::StringRef NewName) {
return Result;
}
+std::vector<SymbolRange> symbolRanges(llvm::ArrayRef<Range> Ranges) {
+ std::vector<SymbolRange> Result;
+ for (const auto &R : Ranges)
+ Result.emplace_back(R);
+ return Result;
+}
+
TEST(RenameTest, WithinFileRename) {
// For each "^" this test moves cursor to its location and applies renaming
// while checking that all identifiers in [[]] ranges are also renamed.
>From a8b2cda919728c3aaa91015a4b524caeccb366a7 Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Thu, 25 Jan 2024 13:51:09 -0500
Subject: [PATCH 6/7] Run clang-format
---
clang-tools-extra/clangd/Protocol.h | 4 +-
clang-tools-extra/clangd/refactor/Rename.cpp | 47 +++++++++++---------
2 files changed, 27 insertions(+), 24 deletions(-)
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index 4cb1752689f9b2a..dc3fdb5b5f6c9b2 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -1991,8 +1991,8 @@ template <> struct DenseMapInfo<clang::clangd::Range> {
return R;
}
static unsigned getHashValue(const Range &Val) {
- return llvm::hash_combine(
- Val.start.line, Val.start.character, Val.end.line, Val.end.character);
+ return llvm::hash_combine(Val.start.line, Val.start.character, Val.end.line,
+ Val.end.character);
}
static bool isEqual(const Range &LHS, const Range &RHS) {
return std::tie(LHS.start, LHS.end) == std::tie(RHS.start, RHS.end);
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 64c3cec28454006..6f3ca54e6634056 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -545,8 +545,7 @@ std::optional<llvm::Error> checkName(const NamedDecl &RenameDecl,
const auto Sel = MD->getSelector();
if (Sel.getNumArgs() != NewName.count(':') &&
NewName != "__clangd_rename_placeholder")
- return makeError(
- InvalidName{InvalidName::BadIdentifier, NewName.str()});
+ return makeError(InvalidName{InvalidName::BadIdentifier, NewName.str()});
}
auto &ASTCtx = RenameDecl.getASTContext();
@@ -573,10 +572,9 @@ std::optional<llvm::Error> checkName(const NamedDecl &RenameDecl,
return std::nullopt;
}
-bool isMatchingSelectorName(const syntax::Token &Cur,
- const syntax::Token &Next,
- const SourceManager &SM,
- llvm::StringRef SelectorName) {
+bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token &Next,
+ const SourceManager &SM,
+ llvm::StringRef SelectorName) {
if (SelectorName.empty())
return Cur.kind() == tok::colon;
return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
@@ -588,18 +586,17 @@ bool isMatchingSelectorName(const syntax::Token &Cur,
Cur.endLocation() == Next.location();
}
-bool isSelectorLike(const syntax::Token &Cur,
- const syntax::Token &Next) {
+bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
return Cur.kind() == tok::identifier && Next.kind() == tok::colon &&
// We require the selector name and : to be contiguous.
// e.g. support `foo:` but not `foo :`.
Cur.endLocation() == Next.location();
}
-bool parseMessageExpression(
- llvm::ArrayRef<syntax::Token> Tokens, const SourceManager &SM,
- unsigned Index, unsigned Last,
- Selector Sel, std::vector<Range> &SelectorPieces) {
+bool parseMessageExpression(llvm::ArrayRef<syntax::Token> Tokens,
+ const SourceManager &SM, unsigned Index,
+ unsigned Last, Selector Sel,
+ std::vector<Range> &SelectorPieces) {
unsigned NumArgs = Sel.getNumArgs();
llvm::SmallVector<char, 8> Closes;
@@ -610,20 +607,22 @@ bool parseMessageExpression(
if (Closes.empty()) {
auto PieceCount = SelectorPieces.size();
if (PieceCount < NumArgs &&
- isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Sel.getNameForSlot(PieceCount))) {
+ isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
+ Sel.getNameForSlot(PieceCount))) {
// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
// token after the name.
if (!Sel.getNameForSlot(PieceCount).empty()) {
++Index;
}
- SelectorPieces.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+ SelectorPieces.push_back(
+ halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
continue;
}
// If we've found all pieces but the current token looks like another
// selector piece, it means the method being renamed is a strict prefix of
// the selector we've found - should be skipped.
- if (SelectorPieces.size() >= NumArgs && isSelectorLike(Tok, Tokens[Index + 1]))
+ if (SelectorPieces.size() >= NumArgs &&
+ isSelectorLike(Tok, Tokens[Index + 1]))
return false;
}
@@ -679,7 +678,8 @@ std::vector<SymbolRange> collectRenameIdentifierRanges(
const LangOptions &LangOpts, std::optional<Selector> Selector) {
std::vector<SymbolRange> Ranges;
if (!Selector) {
- auto IdentifierRanges = collectIdentifierRanges(Identifier, Content, LangOpts);
+ auto IdentifierRanges =
+ collectIdentifierRanges(Identifier, Content, LangOpts);
for (const auto &R : IdentifierRanges)
Ranges.emplace_back(R);
return Ranges;
@@ -705,14 +705,15 @@ std::vector<SymbolRange> collectRenameIdentifierRanges(
if (BraceCount == 0 && ParenCount == 0) {
auto PieceCount = SelectorPieces.size();
if (PieceCount < NumArgs &&
- isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
- Selector->getNameForSlot(PieceCount))) {
+ isMatchingSelectorName(Tok, Tokens[Index + 1], SM,
+ Selector->getNameForSlot(PieceCount))) {
// If 'foo:' instead of ':' (empty selector), we need to skip the ':'
// token after the name.
if (!Selector->getNameForSlot(PieceCount).empty()) {
++Index;
}
- SelectorPieces.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+ SelectorPieces.push_back(
+ halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
continue;
}
// If we've found all pieces, we still need to try to consume more pieces
@@ -720,14 +721,16 @@ std::vector<SymbolRange> collectRenameIdentifierRanges(
// name.
if (PieceCount >= NumArgs && isSelectorLike(Tok, Tokens[Index + 1])) {
++Index;
- SelectorPieces.push_back(halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
+ SelectorPieces.push_back(
+ halfOpenToRange(SM, Tok.range(SM).toCharRange(SM)));
continue;
}
}
switch (Tok.kind()) {
case tok::l_square:
- if (parseMessageExpression(Tokens, SM, Index + 1, Last, *Selector, SelectorPieces)) {
+ if (parseMessageExpression(Tokens, SM, Index + 1, Last, *Selector,
+ SelectorPieces)) {
Ranges.emplace_back(std::move(SelectorPieces));
SelectorPieces.clear();
}
>From 989c9e2931629afa6e00fa3dc10c34db32ff83a4 Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Thu, 25 Jan 2024 15:58:13 -0500
Subject: [PATCH 7/7] Deleted commented out code regarding selector ranges
check
---
clang-tools-extra/clangd/refactor/Rename.cpp | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 6f3ca54e6634056..aaa79e92fbf021a 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -1112,21 +1112,6 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
// TODO: Insert the ranges from the ObjCMethodDecl/ObjCMessageExpr selector
// pieces which are being renamed. This will require us to make changes to
// locateDeclAt to preserve this AST node.
- //
- // if (RInputs.NewName != "__clangd_rename_placeholder") {
- // llvm::StringRef NewName = RInputs.NewName;
- // llvm::SmallVector<llvm::StringRef, 8> NewNames;
- // NewName.split(NewNames, ":");
-
- // const auto Sel = MD->getSelector();
- // unsigned NumSelectorLocs = MD->getNumSelectorLocs();
- // for (unsigned I = 0; I < NumSelectorLocs; ++I) {
- // if (Sel.getNameForSlot(I) != NewNames[I]) {
- // RenamedRanges.insert(
- // tokenRangeForLoc(AST, MD->getSelectorLoc(I), SM, LangOpts));
- // }
- // }
- // }
} else {
RenamedRanges.insert(CurrentIdentifier);
}
More information about the cfe-commits
mailing list