[clang-tools-extra] [clangd] Add support for renaming ObjC properties and implicit properties (PR #81775)
David Goldman via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 14 10:48:07 PST 2024
https://github.com/DavidGoldman created https://github.com/llvm/llvm-project/pull/81775
Add support for renaming Objective-C properties. These are special since a single property decl could generate 3 different decls - a getter method, a setter method, and an instance variable. In addition, support renaming implicit properties, e.g. referring to a getter method `- (id)foo;` via `self.foo` or a setter method `- (void)setFoo:(id)foo;` via `self.foo =` without an explicit property decl.
>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 7ef4b15febad22..336bc3506bb360 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 9cdc57ec01f327..1d4e1c1d75ea23 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 1d4e1c1d75ea23..5c20b950e4eac0 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 a87da252b7a7e9..3e097cbeed5929 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 79579c22b788a9..6a9f097d551ae6 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 a6370649f5ad1c..f93a941a5c27e4 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 e88c804692568f..8fcfcd5e33f6ce 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 336bc3506bb360..f7afa5c099742e 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 11f9e4627af760..e8e7517ac7fe61 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 91728ba59e5d84..b5b9073aab66a0 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 9cbf59684fbc10..a0a271a911c6ae 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 5c20b950e4eac0..1e7a30e69fabe0 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 e8e7517ac7fe61..997e13f2a20c8d 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 8fcfcd5e33f6ce..4cb1752689f9b2 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 997e13f2a20c8d..64c3cec2845400 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 b5b9073aab66a0..be5c346ae150f7 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 527b4263443a70..0dc1a103002b26 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 a0a271a911c6ae..f76ad1513c45cd 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 4cb1752689f9b2..dc3fdb5b5f6c9b 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 64c3cec2845400..6f3ca54e663405 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 6f3ca54e663405..aaa79e92fbf021 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 97b1d7c54424038926e465f6c85a5d56610619f1 Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Fri, 26 Jan 2024 15:50:11 -0500
Subject: [PATCH 8/9] Add support for ObjC property renaming + implicit
property renaming
---
clang-tools-extra/clangd/FindTarget.cpp | 12 +
clang-tools-extra/clangd/refactor/Rename.cpp | 325 +++++++++++++++---
.../unittests/SemanticHighlightingTests.cpp | 2 +-
3 files changed, 283 insertions(+), 56 deletions(-)
diff --git a/clang-tools-extra/clangd/FindTarget.cpp b/clang-tools-extra/clangd/FindTarget.cpp
index 839cf6332fe8b0..8ef0ccc8785d15 100644
--- a/clang-tools-extra/clangd/FindTarget.cpp
+++ b/clang-tools-extra/clangd/FindTarget.cpp
@@ -733,6 +733,18 @@ llvm::SmallVector<ReferenceLoc> refInDecl(const Decl *D,
/*IsDecl=*/true,
{OIMD}});
}
+
+ void VisitObjCPropertyImplDecl(const ObjCPropertyImplDecl *OPID) {
+ if (OPID->isIvarNameSpecified())
+ Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
+ OPID->getPropertyIvarDeclLoc(),
+ /*IsDecl=*/false,
+ {OPID->getPropertyIvarDecl()}});
+ Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
+ OPID->getLocation(),
+ /*IsDecl=*/false,
+ {OPID->getPropertyDecl()}});
+ }
};
Visitor V{Resolver};
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index aaa79e92fbf021..1ed520d27f7187 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -21,6 +21,7 @@
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclObjC.h"
#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/ExprObjC.h"
#include "clang/AST/ParentMapContext.h"
#include "clang/AST/Stmt.h"
#include "clang/Basic/CharInfo.h"
@@ -153,8 +154,110 @@ const NamedDecl *pickInterestingTarget(const NamedDecl *D) {
return D;
}
-llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
- SourceLocation TokenStartLoc) {
+// Some AST nodes are synthesized by the compiler based on other nodes. e.g.
+// ObjC methods and ivars can be synthesized based on an Objective-C property.
+//
+// We perform this check outside of canonicalization since we need to know which
+// decl the user has actually triggered the rename on in order to remap all
+// derived decls properly, since the naming patterns can slightly differ for
+// every decl that the compiler synthesizes.
+const NamedDecl *findOriginDecl(const NamedDecl *D) {
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
+ if (const auto *PD = MD->findPropertyDecl(/*CheckOverrides=*/false))
+ // FIXME(davg): We should only map to the protocol if the user hasn't
+ // explicitly given a setter/getter for the method - if they have we
+ // should either fail the rename or support basic 1 arg selector renames.
+ return canonicalRenameDecl(PD);
+ }
+ if (const auto *ID = dyn_cast<ObjCIvarDecl>(D)) {
+ for (const auto *PD : ID->getContainingInterface()->properties()) {
+ if (PD->getPropertyIvarDecl() == ID)
+ return canonicalRenameDecl(PD);
+ }
+ }
+ return D;
+}
+
+std::string propertySetterName(llvm::StringRef PropertyName) {
+ std::string Setter = PropertyName.str();
+ if (!Setter.empty())
+ Setter[0] = llvm::toUpper(Setter[0]);
+ return "set" + Setter;
+}
+
+// Returns a non-empty string if valid.
+std::string setterToPropertyName(llvm::StringRef Setter) {
+ std::string Result;
+ if (!Setter.consume_front("set")) {
+ return Result;
+ }
+ Result = Setter.str();
+ if (!Result.empty())
+ Result[0] = llvm::toLower(Result[0]);
+ return Result;
+}
+
+llvm::DenseMap<const NamedDecl *, std::string>
+computeAllDeclsToNewName(const NamedDecl *Selected, llvm::StringRef NewName,
+ const NamedDecl *Origin) {
+ llvm::DenseMap<const NamedDecl *, std::string> DeclToName;
+ DeclToName[Selected] = NewName.str();
+
+ if (const auto *PD = dyn_cast<ObjCPropertyDecl>(Origin)) {
+ // Need to derive the property/getter name, setter name, and ivar name based
+ // on which Decl the user triggered the rename on and their single input.
+ std::string PropertyName;
+ std::string SetterName;
+ std::string IvarName;
+
+ if (isa<ObjCIvarDecl>(Selected)) {
+ IvarName = NewName.str();
+ NewName.consume_front("_");
+ PropertyName = NewName.str();
+ SetterName = propertySetterName(PropertyName);
+ } else if (isa<ObjCPropertyDecl>(Selected)) {
+ PropertyName = NewName.str();
+ IvarName = "_" + PropertyName;
+ SetterName = propertySetterName(PropertyName);
+ } else if (const auto *MD = dyn_cast<ObjCMethodDecl>(Selected)) {
+ if (MD->getReturnType()->isVoidType()) { // Setter selected.
+ SetterName = NewName.str();
+ PropertyName = setterToPropertyName(SetterName);
+ if (PropertyName.empty())
+ return DeclToName;
+ IvarName = "_" + PropertyName;
+ } else { // Getter selected.
+ PropertyName = NewName.str();
+ IvarName = "_" + PropertyName;
+ SetterName = propertySetterName(PropertyName);
+ }
+ } else {
+ return DeclToName;
+ }
+
+ DeclToName[PD] = PropertyName;
+ // We will only rename the getter/setter if the user didn't specify one
+ // explicitly in the property decl.
+ if (const auto *GD = PD->getGetterMethodDecl())
+ if (!PD->getGetterNameLoc().isValid())
+ DeclToName[GD] = PropertyName;
+ if (const auto *SD = PD->getSetterMethodDecl())
+ if (!PD->getSetterNameLoc().isValid())
+ DeclToName[SD] = SetterName;
+ // This is only visible in the impl, not the header. We only rename it if it
+ // follows the typical `foo` property => `_foo` ivar convention.
+ if (const auto *ID = PD->getPropertyIvarDecl())
+ if (ID->getNameAsString() == "_" + PD->getNameAsString())
+ DeclToName[ID] = IvarName;
+ }
+
+ return DeclToName;
+}
+
+llvm::DenseMap<const NamedDecl *, std::string>
+locateDeclAt(ParsedAST &AST, SourceLocation TokenStartLoc,
+ llvm::StringRef NewName) {
+ llvm::DenseMap<const NamedDecl *, std::string> Result;
unsigned Offset =
AST.getSourceManager().getDecomposedSpellingLoc(TokenStartLoc).second;
@@ -162,20 +265,34 @@ llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
AST.getASTContext(), AST.getTokens(), Offset, Offset);
const SelectionTree::Node *SelectedNode = Selection.commonAncestor();
if (!SelectedNode)
- return {};
+ return Result;
+
+ std::string SetterName;
+ const NamedDecl *Setter;
+ if (const auto *D = SelectedNode->ASTNode.get<ObjCPropertyRefExpr>()) {
+ if (D->isImplicitProperty() && D->isMessagingSetter()) {
+ SetterName = propertySetterName(NewName);
+ Setter = canonicalRenameDecl(D->getImplicitPropertySetter());
+ }
+ }
- llvm::DenseSet<const NamedDecl *> Result;
for (const NamedDecl *D :
targetDecl(SelectedNode->ASTNode,
DeclRelation::Alias | DeclRelation::TemplatePattern,
AST.getHeuristicResolver())) {
D = pickInterestingTarget(D);
- Result.insert(canonicalRenameDecl(D));
+ D = canonicalRenameDecl(D);
+ if (D == Setter) {
+ Result[D] = SetterName;
+ continue;
+ }
+ Result[D] = NewName.str();
}
return Result;
}
-void filterRenameTargets(llvm::DenseSet<const NamedDecl *> &Decls) {
+void filterRenameTargets(
+ llvm::DenseMap<const NamedDecl *, std::string> &Decls) {
// For something like
// namespace ns { void foo(); }
// void bar() { using ns::f^oo; foo(); }
@@ -183,8 +300,8 @@ void filterRenameTargets(llvm::DenseSet<const NamedDecl *> &Decls) {
// For renaming, we're only interested in foo's declaration, so drop the other
// one. There should never be more than one UsingDecl here, otherwise the
// rename would be ambiguos anyway.
- auto UD = std::find_if(Decls.begin(), Decls.end(), [](const NamedDecl *D) {
- return llvm::isa<UsingDecl>(D);
+ auto UD = std::find_if(Decls.begin(), Decls.end(), [](const auto &P) {
+ return llvm::isa<UsingDecl>(P.first);
});
if (UD != Decls.end()) {
Decls.erase(UD);
@@ -206,6 +323,7 @@ enum class ReasonToReject {
NonIndexable,
UnsupportedSymbol,
AmbiguousSymbol,
+ OnlyRenameableFromDefinition,
// name validation. FIXME: reconcile with InvalidName
SameName,
@@ -274,6 +392,8 @@ llvm::Error makeError(ReasonToReject Reason) {
return "symbol is not a supported kind (e.g. namespace, macro)";
case ReasonToReject::AmbiguousSymbol:
return "there are multiple symbols at the given location";
+ case ReasonToReject::OnlyRenameableFromDefinition:
+ return "only renameable from the implementation";
case ReasonToReject::SameName:
return "new name is the same as the old name";
}
@@ -289,13 +409,28 @@ std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
assert(canonicalRenameDecl(&ND) == &ND &&
"ND should be already canonicalized.");
+ bool IsSythesizedFromProperty = false;
+ if (const auto *ID = dyn_cast<ObjCIvarDecl>(&ND))
+ IsSythesizedFromProperty = ID->getSynthesize();
+ else if (const auto *MD = dyn_cast<ObjCMethodDecl>(&ND))
+ IsSythesizedFromProperty = MD->isPropertyAccessor() && MD->isImplicit();
+
std::vector<SourceLocation> Results;
+ // TODO(davg): Is this actually needed?
+ if (isa<ObjCPropertyDecl>(&ND))
+ Results.push_back(ND.getLocation());
+
for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
findExplicitReferences(
TopLevelDecl,
[&](ReferenceLoc Ref) {
if (Ref.Targets.empty())
return;
+ // Some synthesized decls report their locations as the same as the
+ // decl they were derived from. We need to skip such decls but keep
+ // references otherwise we would rename the wrong decl.
+ if (IsSythesizedFromProperty && Ref.IsDecl)
+ return;
for (const auto *Target : Ref.Targets) {
if (canonicalRenameDecl(Target) == &ND) {
Results.push_back(Ref.NameLoc);
@@ -815,39 +950,68 @@ renameObjCMethodWithinFile(ParsedAST &AST, const ObjCMethodDecl *MD,
// AST-based rename, it renames all occurrences in the main file.
llvm::Expected<tooling::Replacements>
-renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
- llvm::StringRef NewName) {
+renameWithinFile(ParsedAST &AST,
+ const llvm::DenseMap<const NamedDecl *, std::string> &DeclToNewName) {
trace::Span Tracer("RenameWithinFile");
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
- // spelled in a top-level macro argument in the main file.
- if (RenameLoc.isMacroID()) {
- if (isInMacroBody(SM, RenameLoc))
+ for (const auto &Entry : DeclToNewName) {
+ std::string ImplicitPropName;
+ std::string NewImplicitPropName;
+ if (const auto *MD = llvm::dyn_cast<ObjCMethodDecl>(Entry.first)) {
+ if (MD->getReturnType()->isVoidType() &&
+ MD->getSelector().getNumArgs() == 1) {
+ llvm::StringRef Name = MD->getSelector().getNameForSlot(0);
+ ImplicitPropName = setterToPropertyName(Name);
+ NewImplicitPropName = setterToPropertyName(Entry.second);
+ }
+ }
+
+ std::vector<SourceLocation> Locs;
+ for (SourceLocation Loc : findOccurrencesWithinFile(AST, *Entry.first)) {
+ SourceLocation RenameLoc = Loc;
+ // We don't rename in any macro bodies, but we allow rename the symbol
+ // spelled in a top-level macro argument in the main file.
+ if (RenameLoc.isMacroID()) {
+ if (isInMacroBody(SM, RenameLoc))
+ continue;
+ RenameLoc = SM.getSpellingLoc(Loc);
+ }
+ // Filter out locations not from main file.
+ // We traverse only main file decls, but locations could come from an
+ // non-preamble #include file e.g.
+ // void test() {
+ // int f^oo;
+ // #include "use_foo.inc"
+ // }
+ if (!isInsideMainFile(RenameLoc, SM))
continue;
- RenameLoc = SM.getSpellingLoc(Loc);
+ Locs.push_back(RenameLoc);
+ }
+
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(Entry.first)) {
+ if (MD->getSelector().getNumArgs() > 1) {
+ auto Res = renameObjCMethodWithinFile(AST, MD, Entry.second, std::move(Locs));
+ if (!Res)
+ return Res.takeError();
+ FilteredChanges = FilteredChanges.merge(Res.get());
+ continue;
+ }
+ }
+ for (const auto &Loc : Locs) {
+ llvm::StringRef NewName = Entry.second;
+ if (!ImplicitPropName.empty() && !NewImplicitPropName.empty()) {
+ const auto T = AST.getTokens().spelledTokenAt(Loc);
+ if (T && T->text(SM) == ImplicitPropName) {
+ NewName = NewImplicitPropName;
+ }
+ }
+
+ if (auto Err = FilteredChanges.add(tooling::Replacement(
+ SM, CharSourceRange::getTokenRange(Loc), NewName)))
+ return std::move(Err);
}
- // Filter out locations not from main file.
- // We traverse only main file decls, but locations could come from an
- // non-preamble #include file e.g.
- // void test() {
- // int f^oo;
- // #include "use_foo.inc"
- // }
- 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(Loc), NewName)))
- return std::move(Err);
}
return FilteredChanges;
}
@@ -1076,23 +1240,61 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
if (locateMacroAt(*IdentifierToken, AST.getPreprocessor()))
return makeError(ReasonToReject::UnsupportedSymbol);
- auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location());
+ auto DeclsUnderCursor =
+ locateDeclAt(AST, IdentifierToken->location(), RInputs.NewName);
filterRenameTargets(DeclsUnderCursor);
if (DeclsUnderCursor.empty())
return makeError(ReasonToReject::NoSymbolFound);
if (DeclsUnderCursor.size() > 1)
return makeError(ReasonToReject::AmbiguousSymbol);
- const auto &RenameDecl = **DeclsUnderCursor.begin();
- std::string Placeholder = getName(RenameDecl);
- auto Invalid = checkName(RenameDecl, RInputs.NewName, Placeholder);
- if (Invalid)
- return std::move(*Invalid);
-
- auto Reject =
- renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index, Opts);
- if (Reject)
- return makeError(*Reject);
+ const auto &First = *DeclsUnderCursor.begin();
+ const auto &RenameDecl = *First.first;
+ const auto NewName = First.second;
+ const auto *Origin = findOriginDecl(&RenameDecl);
+ // We can only rename the property decl if we're able to find the property
+ // impl decl.
+ if (const auto *PD = dyn_cast<ObjCPropertyDecl>(Origin)) {
+ const auto &ASTCtx = PD->getASTContext();
+ const auto *DC = PD->getDeclContext();
+ if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(DC)) {
+ if (!ASTCtx.getObjCPropertyImplDeclForPropertyDecl(
+ PD, ID->getImplementation())) {
+ return makeError(ReasonToReject::OnlyRenameableFromDefinition);
+ }
+ }
+ if (const auto *CD = dyn_cast<ObjCCategoryDecl>(DC)) {
+ if (const auto *Impl = CD->getImplementation()) {
+ if (!ASTCtx.getObjCPropertyImplDeclForPropertyDecl(PD, Impl)) {
+ return makeError(ReasonToReject::OnlyRenameableFromDefinition);
+ }
+ } else if (const auto *ID = CD->getClassInterface()) {
+ if (!ASTCtx.getObjCPropertyImplDeclForPropertyDecl(
+ PD, ID->getImplementation())) {
+ return makeError(ReasonToReject::OnlyRenameableFromDefinition);
+ }
+ }
+ }
+ }
+ // If we the user is triggering a rename on a ObjC Method from an implicit
+ // property decl, we need to fix up the name to be in the `setX` form.
+ const auto DeclToNewName =
+ computeAllDeclsToNewName(&RenameDecl, NewName, Origin);
+ std::string Placeholder;
+
+ for (const auto &Entry : DeclToNewName) {
+ std::string Name = getName(*Entry.first);
+ auto Invalid = checkName(*Entry.first, Entry.second, Name);
+ if (Invalid)
+ return std::move(*Invalid);
+ if (Entry.first == &RenameDecl)
+ Placeholder = Name;
+
+ auto Reject =
+ renameable(*Entry.first, RInputs.MainFilePath, RInputs.Index, Opts);
+ if (Reject)
+ return makeError(*Reject);
+ }
// We have two implementations of the rename:
// - AST-based rename: used for renaming local symbols, e.g. variables
@@ -1103,7 +1305,7 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
// To make cross-file rename work for local symbol, we use a hybrid solution:
// - run AST-based rename on the main file;
// - run index-based rename on other affected files;
- auto MainFileRenameEdit = renameWithinFile(AST, RenameDecl, RInputs.NewName);
+ auto MainFileRenameEdit = renameWithinFile(AST, DeclToNewName);
if (!MainFileRenameEdit)
return MainFileRenameEdit.takeError();
@@ -1159,14 +1361,27 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
return Result;
}
- auto OtherFilesEdits = renameOutsideFile(
- RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index,
- Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max()
- : Opts.LimitFiles,
- *RInputs.FS);
- if (!OtherFilesEdits)
- return OtherFilesEdits.takeError();
- Result.GlobalChanges = *OtherFilesEdits;
+ FileEdits GlobalChanges;
+ for (const auto &Entry : DeclToNewName) {
+ auto OtherFilesEdits = renameOutsideFile(
+ *Entry.first, RInputs.MainFilePath, Entry.second, *RInputs.Index,
+ Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max()
+ : Opts.LimitFiles,
+ *RInputs.FS);
+ if (!OtherFilesEdits)
+ return OtherFilesEdits.takeError();
+ // Merge all edits.
+ for (const auto &E : *OtherFilesEdits) {
+ auto It = GlobalChanges.find(E.getKey());
+ if (It == GlobalChanges.end()) {
+ GlobalChanges.try_emplace(E.getKey(), std::move(E.getValue()));
+ continue;
+ }
+ It->second.Replacements =
+ It->second.Replacements.merge(E.getValue().Replacements);
+ }
+ }
+ Result.GlobalChanges = GlobalChanges;
// Attach the rename edits for the main file.
Result.GlobalChanges.try_emplace(RInputs.MainFilePath,
std::move(MainFileEdits));
diff --git a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
index da12accc7898b6..1600d3d6fc5d42 100644
--- a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -733,7 +733,7 @@ sizeof...($TemplateParameter[[Elements]]);
@property(readonly, class) $Class[[Foo]] *$Field_decl_readonly_static[[sharedInstance]];
@end
@implementation $Class_def[[Foo]]
- @synthesize someProperty = _someProperty;
+ @synthesize $Field[[someProperty]] = $Field[[_someProperty]];
- (int)$Method_def[[otherMethod]] {
return 0;
}
>From 36f1457e235b687c31e308c496fb67b6a477c3ea Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Mon, 5 Feb 2024 09:22:35 -0800
Subject: [PATCH 9/9] Minor fixes + add tests
---
clang-tools-extra/clangd/FindTarget.cpp | 4 +
clang-tools-extra/clangd/refactor/Rename.cpp | 8 +-
.../clangd/unittests/RenameTests.cpp | 97 +++++++++++++++++++
3 files changed, 107 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clangd/FindTarget.cpp b/clang-tools-extra/clangd/FindTarget.cpp
index 8ef0ccc8785d15..559f971ef758f2 100644
--- a/clang-tools-extra/clangd/FindTarget.cpp
+++ b/clang-tools-extra/clangd/FindTarget.cpp
@@ -735,6 +735,10 @@ llvm::SmallVector<ReferenceLoc> refInDecl(const Decl *D,
}
void VisitObjCPropertyImplDecl(const ObjCPropertyImplDecl *OPID) {
+ // Skiped compiler synthesized property impl decls - they will always
+ // have an invalid loc.
+ if (OPID->getLocation().isInvalid())
+ return;
if (OPID->isIvarNameSpecified())
Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
OPID->getPropertyIvarDeclLoc(),
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 1ed520d27f7187..004a091bce7692 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -182,7 +182,7 @@ std::string propertySetterName(llvm::StringRef PropertyName) {
std::string Setter = PropertyName.str();
if (!Setter.empty())
Setter[0] = llvm::toUpper(Setter[0]);
- return "set" + Setter;
+ return "set" + Setter + ":";
}
// Returns a non-empty string if valid.
@@ -191,6 +191,7 @@ std::string setterToPropertyName(llvm::StringRef Setter) {
if (!Setter.consume_front("set")) {
return Result;
}
+ Setter.consume_back(":"); // Optional.
Result = Setter.str();
if (!Result.empty())
Result[0] = llvm::toLower(Result[0]);
@@ -989,7 +990,6 @@ renameWithinFile(ParsedAST &AST,
continue;
Locs.push_back(RenameLoc);
}
-
if (const auto *MD = dyn_cast<ObjCMethodDecl>(Entry.first)) {
if (MD->getSelector().getNumArgs() > 1) {
auto Res = renameObjCMethodWithinFile(AST, MD, Entry.second, std::move(Locs));
@@ -1001,6 +1001,10 @@ renameWithinFile(ParsedAST &AST,
}
for (const auto &Loc : Locs) {
llvm::StringRef NewName = Entry.second;
+ // Eat trailing : for single argument methods since they're actually
+ // considered a separate token during rename.
+ if (isa<ObjCMethodDecl>(Entry.first))
+ NewName.consume_back(":");
if (!ImplicitPropName.empty() && !NewImplicitPropName.empty()) {
const auto T = AST.getTokens().spelledTokenAt(Loc);
if (T && T->text(SM) == ImplicitPropName) {
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index f76ad1513c45cd..66c3a33fe6fa69 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -861,6 +861,19 @@ TEST(RenameTest, WithinFileRename) {
void func([[Fo^o]] *f) {}
)cpp",
+
+ // ObjC property.
+ R"cpp(
+ @interface Foo
+ @property(nonatomic) int [[f^oo]];
+ @end
+ @implementation Foo
+ @end
+
+ void func(Foo *f) {
+ f.[[f^oo]] += [f [[fo^o]]];
+ }
+ )cpp",
};
llvm::StringRef NewName = "NewName";
for (llvm::StringRef T : Tests) {
@@ -883,6 +896,90 @@ TEST(RenameTest, WithinFileRename) {
}
}
+TEST(RenameTest, WithinFileComplexRename) {
+ // For each "^" this test moves cursor to its location and applies renaming.
+ // This is meant for complex rename cases where multiple distinct tokens can
+ // be renamed differently.
+ struct TestCase {
+ llvm::StringRef NewName;
+ llvm::StringRef Code;
+ llvm::StringRef Renamed;
+ };
+ TestCase Tests[] = {
+ {
+ "setBar:",
+ R"cpp(
+ @interface Foo
+ @property(nonatomic) int foo;
+ @end
+ @implementation Foo
+ @end
+
+ void func(Foo *f) {
+ [f setF^oo:[f foo] ];
+ }
+ )cpp",
+ R"cpp(
+ @interface Foo
+ @property(nonatomic) int bar;
+ @end
+ @implementation Foo
+ @end
+
+ void func(Foo *f) {
+ [f setBar:[f bar] ];
+ }
+ )cpp",
+ },
+
+ {
+ "one:two:",
+ R"cpp(
+ @interface Foo
+ - (void)a:(int)a b:(int)b;
+ @end
+ @implementation Foo
+ - (void)a:(int)a b:(int)b {}
+ @end
+
+ void func(Foo *f) {
+ [f a:1 b:2];
+ }
+ )cpp",
+ R"cpp(
+ @interface Foo
+ - (void)one:(int)a two:(int)b;
+ @end
+ @implementation Foo
+ - (void)one:(int)a two:(int)b {}
+ @end
+
+ void func(Foo *f) {
+ [f one:1 two:2];
+ }
+ )cpp",
+ },
+ };
+ for (TestCase T : Tests) {
+ SCOPED_TRACE(T.Code);
+ Annotations Code(T.Code);
+ 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()});
+ ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
+ ASSERT_EQ(1u, RenameResult->GlobalChanges.size());
+ EXPECT_EQ(
+ applyEdits(std::move(RenameResult->GlobalChanges)).front().second,
+ T.Renamed);
+ }
+ }
+}
+
TEST(RenameTest, Renameable) {
struct Case {
const char *Code;
More information about the cfe-commits
mailing list