[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
Tue Feb 6 09:10:30 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/9] [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 7ef4b15febad2..336bc3506bb36 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 9cdc57ec01f32..1d4e1c1d75ea2 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/9] 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 1d4e1c1d75ea2..5c20b950e4eac 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/9] 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 a87da252b7a7e..3e097cbeed592 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 79579c22b788a..6a9f097d551ae 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 a6370649f5ad1..f93a941a5c27e 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 e88c804692568..8fcfcd5e33f6c 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 336bc3506bb36..f7afa5c099742 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 11f9e4627af76..e8e7517ac7fe6 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 91728ba59e5d8..b5b9073aab66a 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 9cbf59684fbc1..a0a271a911c6a 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 5c20b950e4eac..1e7a30e69fabe 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/9] 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 e8e7517ac7fe6..997e13f2a20c8 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/9] 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 8fcfcd5e33f6c..4cb1752689f9b 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 997e13f2a20c8..64c3cec284540 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 b5b9073aab66a..be5c346ae150f 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 527b4263443a7..0dc1a103002b2 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 a0a271a911c6a..f76ad1513c45c 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/9] 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 4cb1752689f9b..dc3fdb5b5f6c9 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 64c3cec284540..6f3ca54e66340 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/9] 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 6f3ca54e66340..aaa79e92fbf02 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);
}
>From eb0756148e56b2db798c6e13ac40d4e76726261a Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Tue, 6 Feb 2024 09:08:59 -0800
Subject: [PATCH 8/9] Rewrite message lexer + minor fixes for review
---
clang-tools-extra/clangd/refactor/Rename.cpp | 171 ++++++------------
.../clangd/unittests/RenameTests.cpp | 151 ++++++++++++++++
2 files changed, 210 insertions(+), 112 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index aaa79e92fbf02..7c6cf67a5f8e9 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -531,9 +531,9 @@ std::string getName(const NamedDecl &RenameDecl) {
// Check if we can rename the given RenameDecl into NewName.
// Return details if the rename would produce a conflict.
-std::optional<llvm::Error> checkName(const NamedDecl &RenameDecl,
- llvm::StringRef NewName,
- llvm::StringRef OldName) {
+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");
@@ -569,21 +569,7 @@ std::optional<llvm::Error> checkName(const NamedDecl &RenameDecl,
InvalidNameMetric.record(1, toString(Result->K));
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();
+ return llvm::Error::success();
}
bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
@@ -593,15 +579,23 @@ bool isSelectorLike(const syntax::Token &Cur, const syntax::Token &Next) {
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 isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token &Next,
+ const SourceManager &SM,
+ llvm::StringRef SelectorName) {
+ if (SelectorName.empty())
+ return Cur.kind() == tok::colon;
+ return isSelectorLike(Cur, Next) && Cur.text(SM) == SelectorName;
+}
+
+std::vector<Range> findAllSelectorPieces(llvm::ArrayRef<syntax::Token> Tokens,
+ const SourceManager &SM, Selector Sel,
+ tok::TokenKind Terminator) {
unsigned NumArgs = Sel.getNumArgs();
- llvm::SmallVector<char, 8> Closes;
- SelectorPieces.clear();
- while (Index < Last) {
+ llvm::SmallVector<tok::TokenKind, 8> Closes;
+ std::vector<Range> SelectorPieces;
+ unsigned Last = Tokens.size() - 1;
+ for (unsigned Index = 0; Index < Last; ++Index) {
const auto &Tok = Tokens[Index];
if (Closes.empty()) {
@@ -623,49 +617,39 @@ bool parseMessageExpression(llvm::ArrayRef<syntax::Token> Tokens,
// the selector we've found - should be skipped.
if (SelectorPieces.size() >= NumArgs &&
isSelectorLike(Tok, Tokens[Index + 1]))
- return false;
+ return std::vector<Range>();
}
+ if (Tok.kind() == Terminator)
+ return SelectorPieces.size() == NumArgs ? SelectorPieces : std::vector<Range>();
+
switch (Tok.kind()) {
case tok::l_square:
- Closes.push_back(']');
+ Closes.push_back(tok::r_square);
break;
case tok::l_paren:
- Closes.push_back(')');
+ Closes.push_back(tok::r_paren);
break;
case tok::l_brace:
- Closes.push_back('}');
+ Closes.push_back(tok::r_brace);
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;
+ if (Closes.empty() || Closes.back() != Tok.kind())
+ return std::vector<Range>();
Closes.pop_back();
break;
case tok::semi:
- // top level ; ends all statements.
+ // top level ; terminates all statements.
if (Closes.empty())
- return false;
+ return SelectorPieces.size() == NumArgs ? SelectorPieces : std::vector<Range>();
break;
default:
break;
}
-
- ++Index;
}
- return false;
+ return std::vector<Range>();
}
/// Collects all ranges of the given identifier/selector in the source code.
@@ -689,85 +673,48 @@ std::vector<SymbolRange> collectRenameIdentifierRanges(
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();
+ // We track parens and brackets 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).
+ llvm::SmallVector<tok::TokenKind, 8> Closes;
+ llvm::StringRef FirstSelPiece = Selector->getNameForSlot(0);
std::vector<Range> SelectorPieces;
+ std::vector<Range> MsgExprPieces;
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;
- }
+ // Search for the first selector piece to begin a match, but make sure we're
+ // not in () to avoid the @selector(foo:bar:) case.
+ if ((Closes.empty() || Closes.back() == tok::r_square) &&
+ isMatchingSelectorName(Tok, Tokens[Index + 1], SM, FirstSelPiece)) {
+ // We found a candidate for our match, this might be a method call, declaration, or unrelated identifier eg:
+ // - [obj ^sel0: X sel1: Y ... ]
+ // or
+ // @interface Foo
+ // - int ^sel0: X sel1: Y ...
+ // @end
+ // Check if we can find all the relevant selector peices starting from this token
+ auto SelectorPieces = findAllSelectorPieces(
+ ArrayRef(Tokens).slice(Index), SM, *Selector,
+ Closes.empty() ? tok::l_brace : Closes.back());
+ if (!SelectorPieces.empty())
+ Ranges.emplace_back(std::move(SelectorPieces));
}
switch (Tok.kind()) {
case tok::l_square:
- if (parseMessageExpression(Tokens, SM, Index + 1, Last, *Selector,
- SelectorPieces)) {
- Ranges.emplace_back(std::move(SelectorPieces));
- SelectorPieces.clear();
- }
+ Closes.push_back(tok::r_square);
break;
case tok::l_paren:
- ParenCount++;
+ Closes.push_back(tok::r_paren);
break;
+ case tok::r_square:
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();
- }
+ if (!Closes.empty() && Closes.back() == Tok.kind())
+ Closes.pop_back();
break;
default:
break;
@@ -1087,7 +1034,7 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
std::string Placeholder = getName(RenameDecl);
auto Invalid = checkName(RenameDecl, RInputs.NewName, Placeholder);
if (Invalid)
- return std::move(*Invalid);
+ return std::move(Invalid);
auto Reject =
renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index, Opts);
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index f76ad1513c45c..41ae3f9f1c0d8 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -883,6 +883,157 @@ TEST(RenameTest, WithinFileRename) {
}
}
+TEST(RenameTest, ObjCWithinFileRename) {
+ struct TestCase {
+ /// Annotated source code that should be renamed. Every point (indicated by
+ /// `^`) will be used as a rename location.
+ llvm::StringRef Input;
+ /// The new name that should be given to the rename locaitons.
+ llvm::StringRef NewName;
+ /// The expected rename source code or `nullopt` if we expect rename to
+ /// fail.
+ std::optional<llvm::StringRef> Expected;
+ };
+ TestCase Tests[] = {
+ // Simple rename
+ {
+ // Input
+ R"cpp(
+ @interface Foo
+ - (int)performA^ction:(int)action w^ith:(int)value;
+ @end
+ @implementation Foo
+ - (int)performAc^tion:(int)action w^ith:(int)value {
+ return [self performAction:action with:value];
+ }
+ @end
+ )cpp",
+ // New name
+ "performNewAction:by:",
+ // Expected
+ R"cpp(
+ @interface Foo
+ - (int)performNewAction:(int)action by:(int)value;
+ @end
+ @implementation Foo
+ - (int)performNewAction:(int)action by:(int)value {
+ return [self performNewAction:action by:value];
+ }
+ @end
+ )cpp",
+ },
+ // Rename selector with macro
+ {
+ // Input
+ R"cpp(
+ #define mySelector - (int)performAction:(int)action with:(int)value
+ @interface Foo
+ ^mySelector;
+ @end
+ @implementation Foo
+ mySelector {
+ return [self performAction:action with:value];
+ }
+ @end
+ )cpp",
+ // New name
+ "performNewAction:by:",
+ // Expected error
+ std::nullopt,
+ },
+ // Rename selector in macro definition
+ {
+ // Input
+ R"cpp(
+ #define mySelector - (int)perform^Action:(int)action with:(int)value
+ @interface Foo
+ mySelector;
+ @end
+ @implementation Foo
+ mySelector {
+ return [self performAction:action with:value];
+ }
+ @end
+ )cpp",
+ // New name
+ "performNewAction:by:",
+ // Expected error
+ std::nullopt,
+ },
+ // Don't rename `@selector`
+ // `@selector` is not tied to a single selector. Eg. there might be multiple
+ // classes in the codebase that implement that selector. It's thus more like
+ // a string literal and we shouldn't rename it.
+ {
+ // Input
+ R"cpp(
+ @interface Foo
+ - (void)performA^ction:(int)action with:(int)value;
+ @end
+ @implementation Foo
+ - (void)performAction:(int)action with:(int)value {
+ SEL mySelector = @selector(performAction:with:);
+ }
+ @end
+ )cpp",
+ // New name
+ "performNewAction:by:",
+ // Expected
+ R"cpp(
+ @interface Foo
+ - (void)performNewAction:(int)action by:(int)value;
+ @end
+ @implementation Foo
+ - (void)performNewAction:(int)action by:(int)value {
+ SEL mySelector = @selector(performAction:with:);
+ }
+ @end
+ )cpp",
+ },
+ // Fail if rename initiated inside @selector
+ {
+ // Input
+ R"cpp(
+ @interface Foo
+ - (void)performAction:(int)action with:(int)value;
+ @end
+ @implementation Foo
+ - (void)performAction:(int)action with:(int)value {
+ SEL mySelector = @selector(perfo^rmAction:with:);
+ }
+ @end
+ )cpp",
+ // New name
+ "performNewAction:by:",
+ // Expected
+ std::nullopt,
+ }
+ };
+ for (TestCase T : Tests) {
+ SCOPED_TRACE(T.Input);
+ Annotations Code(T.Input);
+ auto TU = TestTU::withCode(Code.code());
+ TU.ExtraArgs.push_back("-xobjective-c");
+ auto AST = TU.build();
+ auto Index = TU.index();
+ for (const auto &RenamePos : Code.points()) {
+ auto RenameResult =
+ rename({RenamePos, T.NewName, AST, testPath(TU.Filename),
+ getVFSFromAST(AST), Index.get()});
+ if (std::optional<StringRef> Expected = T.Expected) {
+ ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
+ ASSERT_EQ(1u, RenameResult->GlobalChanges.size());
+ EXPECT_EQ(
+ applyEdits(std::move(RenameResult->GlobalChanges)).front().second,
+ *Expected);
+ } else {
+ ASSERT_FALSE(bool(RenameResult));
+ consumeError(RenameResult.takeError());
+ }
+ }
+ }
+}
+
TEST(RenameTest, Renameable) {
struct Case {
const char *Code;
>From a7361b33295a95e2a9289b6478ad17c8f014675c Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Tue, 6 Feb 2024 09:10:11 -0800
Subject: [PATCH 9/9] Run clang-format
---
clang-tools-extra/clangd/refactor/Rename.cpp | 25 +++--
.../clangd/unittests/RenameTests.cpp | 104 +++++++++---------
2 files changed, 66 insertions(+), 63 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 7c6cf67a5f8e9..3ece644fa7558 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -531,8 +531,7 @@ std::string getName(const NamedDecl &RenameDecl) {
// Check if we can rename the given RenameDecl into NewName.
// Return details if the rename would produce a conflict.
-llvm::Error checkName(const NamedDecl &RenameDecl,
- llvm::StringRef NewName,
+llvm::Error checkName(const NamedDecl &RenameDecl, llvm::StringRef NewName,
llvm::StringRef OldName) {
trace::Span Tracer("CheckName");
static constexpr trace::Metric InvalidNameMetric(
@@ -588,8 +587,8 @@ bool isMatchingSelectorName(const syntax::Token &Cur, const syntax::Token &Next,
}
std::vector<Range> findAllSelectorPieces(llvm::ArrayRef<syntax::Token> Tokens,
- const SourceManager &SM, Selector Sel,
- tok::TokenKind Terminator) {
+ const SourceManager &SM, Selector Sel,
+ tok::TokenKind Terminator) {
unsigned NumArgs = Sel.getNumArgs();
llvm::SmallVector<tok::TokenKind, 8> Closes;
@@ -621,7 +620,8 @@ std::vector<Range> findAllSelectorPieces(llvm::ArrayRef<syntax::Token> Tokens,
}
if (Tok.kind() == Terminator)
- return SelectorPieces.size() == NumArgs ? SelectorPieces : std::vector<Range>();
+ return SelectorPieces.size() == NumArgs ? SelectorPieces
+ : std::vector<Range>();
switch (Tok.kind()) {
case tok::l_square:
@@ -643,7 +643,8 @@ std::vector<Range> findAllSelectorPieces(llvm::ArrayRef<syntax::Token> Tokens,
case tok::semi:
// top level ; terminates all statements.
if (Closes.empty())
- return SelectorPieces.size() == NumArgs ? SelectorPieces : std::vector<Range>();
+ return SelectorPieces.size() == NumArgs ? SelectorPieces
+ : std::vector<Range>();
break;
default:
break;
@@ -690,16 +691,18 @@ std::vector<SymbolRange> collectRenameIdentifierRanges(
// not in () to avoid the @selector(foo:bar:) case.
if ((Closes.empty() || Closes.back() == tok::r_square) &&
isMatchingSelectorName(Tok, Tokens[Index + 1], SM, FirstSelPiece)) {
- // We found a candidate for our match, this might be a method call, declaration, or unrelated identifier eg:
+ // We found a candidate for our match, this might be a method call,
+ // declaration, or unrelated identifier eg:
// - [obj ^sel0: X sel1: Y ... ]
// or
// @interface Foo
// - int ^sel0: X sel1: Y ...
// @end
- // Check if we can find all the relevant selector peices starting from this token
- auto SelectorPieces = findAllSelectorPieces(
- ArrayRef(Tokens).slice(Index), SM, *Selector,
- Closes.empty() ? tok::l_brace : Closes.back());
+ // Check if we can find all the relevant selector peices starting from
+ // this token
+ auto SelectorPieces =
+ findAllSelectorPieces(ArrayRef(Tokens).slice(Index), SM, *Selector,
+ Closes.empty() ? tok::l_brace : Closes.back());
if (!SelectorPieces.empty())
Ranges.emplace_back(std::move(SelectorPieces));
}
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index 41ae3f9f1c0d8..d91ef85d67271 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -894,11 +894,10 @@ TEST(RenameTest, ObjCWithinFileRename) {
/// fail.
std::optional<llvm::StringRef> Expected;
};
- TestCase Tests[] = {
- // Simple rename
- {
- // Input
- R"cpp(
+ TestCase Tests[] = {// Simple rename
+ {
+ // Input
+ R"cpp(
@interface Foo
- (int)performA^ction:(int)action w^ith:(int)value;
@end
@@ -908,10 +907,10 @@ TEST(RenameTest, ObjCWithinFileRename) {
}
@end
)cpp",
- // New name
- "performNewAction:by:",
- // Expected
- R"cpp(
+ // New name
+ "performNewAction:by:",
+ // Expected
+ R"cpp(
@interface Foo
- (int)performNewAction:(int)action by:(int)value;
@end
@@ -921,11 +920,11 @@ TEST(RenameTest, ObjCWithinFileRename) {
}
@end
)cpp",
- },
- // Rename selector with macro
- {
- // Input
- R"cpp(
+ },
+ // Rename selector with macro
+ {
+ // Input
+ R"cpp(
#define mySelector - (int)performAction:(int)action with:(int)value
@interface Foo
^mySelector;
@@ -936,15 +935,15 @@ TEST(RenameTest, ObjCWithinFileRename) {
}
@end
)cpp",
- // New name
- "performNewAction:by:",
- // Expected error
- std::nullopt,
- },
- // Rename selector in macro definition
- {
- // Input
- R"cpp(
+ // New name
+ "performNewAction:by:",
+ // Expected error
+ std::nullopt,
+ },
+ // Rename selector in macro definition
+ {
+ // Input
+ R"cpp(
#define mySelector - (int)perform^Action:(int)action with:(int)value
@interface Foo
mySelector;
@@ -955,18 +954,20 @@ TEST(RenameTest, ObjCWithinFileRename) {
}
@end
)cpp",
- // New name
- "performNewAction:by:",
- // Expected error
- std::nullopt,
- },
- // Don't rename `@selector`
- // `@selector` is not tied to a single selector. Eg. there might be multiple
- // classes in the codebase that implement that selector. It's thus more like
- // a string literal and we shouldn't rename it.
- {
- // Input
- R"cpp(
+ // New name
+ "performNewAction:by:",
+ // Expected error
+ std::nullopt,
+ },
+ // Don't rename `@selector`
+ // `@selector` is not tied to a single selector. Eg. there
+ // might be multiple
+ // classes in the codebase that implement that selector.
+ // It's thus more like
+ // a string literal and we shouldn't rename it.
+ {
+ // Input
+ R"cpp(
@interface Foo
- (void)performA^ction:(int)action with:(int)value;
@end
@@ -976,10 +977,10 @@ TEST(RenameTest, ObjCWithinFileRename) {
}
@end
)cpp",
- // New name
- "performNewAction:by:",
- // Expected
- R"cpp(
+ // New name
+ "performNewAction:by:",
+ // Expected
+ R"cpp(
@interface Foo
- (void)performNewAction:(int)action by:(int)value;
@end
@@ -989,11 +990,11 @@ TEST(RenameTest, ObjCWithinFileRename) {
}
@end
)cpp",
- },
- // Fail if rename initiated inside @selector
- {
- // Input
- R"cpp(
+ },
+ // Fail if rename initiated inside @selector
+ {
+ // Input
+ R"cpp(
@interface Foo
- (void)performAction:(int)action with:(int)value;
@end
@@ -1003,12 +1004,11 @@ TEST(RenameTest, ObjCWithinFileRename) {
}
@end
)cpp",
- // New name
- "performNewAction:by:",
- // Expected
- std::nullopt,
- }
- };
+ // New name
+ "performNewAction:by:",
+ // Expected
+ std::nullopt,
+ }};
for (TestCase T : Tests) {
SCOPED_TRACE(T.Input);
Annotations Code(T.Input);
@@ -1024,8 +1024,8 @@ TEST(RenameTest, ObjCWithinFileRename) {
ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
ASSERT_EQ(1u, RenameResult->GlobalChanges.size());
EXPECT_EQ(
- applyEdits(std::move(RenameResult->GlobalChanges)).front().second,
- *Expected);
+ applyEdits(std::move(RenameResult->GlobalChanges)).front().second,
+ *Expected);
} else {
ASSERT_FALSE(bool(RenameResult));
consumeError(RenameResult.takeError());
More information about the cfe-commits
mailing list