[clang-tools-extra] [clangd] Fix renaming single argument ObjC methods (PR #82396)
David Goldman via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 23 11:04:31 PST 2024
https://github.com/DavidGoldman updated https://github.com/llvm/llvm-project/pull/82396
>From 8a9c09575ed143e762faa7abf48285ed6b4b0335 Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Tue, 20 Feb 2024 13:14:26 -0500
Subject: [PATCH 1/3] [clangd] Fix renaming single argument ObjC methods
Add a few more tests to verify this works (thanks to @ahoppen
for the tests and finding this bug).
---
clang-tools-extra/clangd/refactor/Rename.cpp | 15 +-
.../clangd/unittests/RenameTests.cpp | 138 ++++++++++++++++++
2 files changed, 148 insertions(+), 5 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 650862c99bcd12..1f0e54c19e2be1 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -811,8 +811,14 @@ renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
continue;
Locs.push_back(RenameLoc);
}
- if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl))
- return renameObjCMethodWithinFile(AST, MD, NewName, std::move(Locs));
+ if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
+ if (MD->getSelector().getNumArgs() > 1)
+ return renameObjCMethodWithinFile(AST, MD, NewName, std::move(Locs));
+
+ // Eat trailing : for single argument methods since they're actually
+ // considered a separate token during rename.
+ NewName.consume_back(":");
+ }
for (const auto &Loc : Locs) {
if (auto Err = FilteredChanges.add(tooling::Replacement(
SM, CharSourceRange::getTokenRange(Loc), NewName)))
@@ -930,10 +936,9 @@ renameOutsideFile(const NamedDecl &RenameDecl, llvm::StringRef MainFilePath,
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();
+ RenameIdentifier = MD->getSelector().getNameForSlot(0).str();
+ if (MD->getSelector().getNumArgs() > 1)
Selector = MD->getSelector();
- }
}
NewName.split(NewNames, ":");
diff --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index d91ef85d672711..7d9252110b27df 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1943,6 +1943,144 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) {
}
}
+TEST(CrossFileRenameTests, ObjC) {
+ MockCompilationDatabase CDB;
+ CDB.ExtraClangFlags = {"-xobjective-c"};
+ // rename is runnning on all "^" points in FooH.
+ struct Case {
+ llvm::StringRef FooH;
+ llvm::StringRef FooM;
+ llvm::StringRef NewName;
+ llvm::StringRef ExpectedFooH;
+ llvm::StringRef ExpectedFooM;
+ };
+ Case Cases[] = {// --- Zero arg selector
+ {
+ // Input
+ R"cpp(
+ @interface Foo
+ - (int)performA^ction;
+ @end
+ )cpp",
+ R"cpp(
+ @implementation Foo
+ - (int)performAction {
+ [self performAction];
+ }
+ @end
+ )cpp",
+ // New name
+ "performNewAction",
+ // Expected
+ R"cpp(
+ @interface Foo
+ - (int)performNewAction;
+ @end
+ )cpp",
+ R"cpp(
+ @implementation Foo
+ - (int)performNewAction {
+ [self performNewAction];
+ }
+ @end
+ )cpp",
+ },
+ // --- Single arg selector
+ {
+ // Input
+ R"cpp(
+ @interface Foo
+ - (int)performA^ction:(int)action;
+ @end
+ )cpp",
+ R"cpp(
+ @implementation Foo
+ - (int)performAction:(int)action {
+ [self performAction:action];
+ }
+ @end
+ )cpp",
+ // New name
+ "performNewAction:",
+ // Expected
+ R"cpp(
+ @interface Foo
+ - (int)performNewAction:(int)action;
+ @end
+ )cpp",
+ R"cpp(
+ @implementation Foo
+ - (int)performNewAction:(int)action {
+ [self performNewAction:action];
+ }
+ @end
+ )cpp",
+ },
+ // --- Multi arg selector
+ {
+ // Input
+ R"cpp(
+ @interface Foo
+ - (int)performA^ction:(int)action with:(int)value;
+ @end
+ )cpp",
+ R"cpp(
+ @implementation Foo
+ - (int)performAction:(int)action with:(int)value {
+ [self performAction:action with:value];
+ }
+ @end
+ )cpp",
+ // New name
+ "performNewAction:by:",
+ // Expected
+ R"cpp(
+ @interface Foo
+ - (int)performNewAction:(int)action by:(int)value;
+ @end
+ )cpp",
+ R"cpp(
+ @implementation Foo
+ - (int)performNewAction:(int)action by:(int)value {
+ [self performNewAction:action by:value];
+ }
+ @end
+ )cpp",
+ }};
+
+ trace::TestTracer Tracer;
+ for (const auto &T : Cases) {
+ SCOPED_TRACE(T.FooH);
+ Annotations FooH(T.FooH);
+ Annotations FooM(T.FooM);
+ std::string FooHPath = testPath("foo.h");
+ std::string FooMPath = testPath("foo.m");
+
+ MockFS FS;
+ FS.Files[FooHPath] = std::string(FooH.code());
+ FS.Files[FooMPath] = std::string(FooM.code());
+
+ auto ServerOpts = ClangdServer::optsForTest();
+ ServerOpts.BuildDynamicSymbolIndex = true;
+ ClangdServer Server(CDB, FS, ServerOpts);
+
+ // Add all files to clangd server to make sure the dynamic index has been
+ // built.
+ runAddDocument(Server, FooHPath, FooH.code());
+ runAddDocument(Server, FooMPath, FooM.code());
+
+ for (const auto &RenamePos : FooH.points()) {
+ EXPECT_THAT(Tracer.takeMetric("rename_files"), SizeIs(0));
+ auto FileEditsList =
+ llvm::cantFail(runRename(Server, FooHPath, RenamePos, T.NewName, {}));
+ EXPECT_THAT(Tracer.takeMetric("rename_files"), ElementsAre(2));
+ EXPECT_THAT(applyEdits(std::move(FileEditsList.GlobalChanges)),
+ UnorderedElementsAre(Pair(Eq(FooHPath), Eq(T.ExpectedFooH)),
+ Pair(Eq(FooMPath), Eq(T.ExpectedFooM))));
+ }
+ }
+}
+
TEST(CrossFileRenameTests, CrossFileOnLocalSymbol) {
// cross-file rename should work for function-local symbols, even there is no
// index provided.
>From 65a6b381d87e036442bbe05a868dbd2f0ca9e25e Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Thu, 22 Feb 2024 12:36:18 -0500
Subject: [PATCH 2/3] Add comment to explain zero/one arg selector case
---
clang-tools-extra/clangd/refactor/Rename.cpp | 3 +++
1 file changed, 3 insertions(+)
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 1f0e54c19e2be1..7ad6c896ddce18 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -812,6 +812,9 @@ renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
Locs.push_back(RenameLoc);
}
if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
+ // The custom ObjC selector logic doesn't handle the zero arg selector
+ // case. We could use it for the one arg selector case but it's simpler to
+ // use the standard one-token rename logic.
if (MD->getSelector().getNumArgs() > 1)
return renameObjCMethodWithinFile(AST, MD, NewName, std::move(Locs));
>From 45bd4724e0a331862c6f40417131dfdd7e69a76d Mon Sep 17 00:00:00 2001
From: David Goldman <davg at google.com>
Date: Fri, 23 Feb 2024 14:04:14 -0500
Subject: [PATCH 3/3] Update selector comment
---
clang-tools-extra/clangd/refactor/Rename.cpp | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 7ad6c896ddce18..4e135801f6853d 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -813,8 +813,9 @@ renameWithinFile(ParsedAST &AST, const NamedDecl &RenameDecl,
}
if (const auto *MD = dyn_cast<ObjCMethodDecl>(&RenameDecl)) {
// The custom ObjC selector logic doesn't handle the zero arg selector
- // case. We could use it for the one arg selector case but it's simpler to
- // use the standard one-token rename logic.
+ // case, as it relies on parsing selectors via the trailing `:`.
+ // We also choose to use regular rename logic for the single-arg selectors
+ // as the AST/Index has the right locations in that case.
if (MD->getSelector().getNumArgs() > 1)
return renameObjCMethodWithinFile(AST, MD, NewName, std::move(Locs));
More information about the cfe-commits
mailing list