[clang-tools-extra] [clangd] Fix renaming single argument ObjC methods (PR #82396)
David Goldman via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 20 10:17:45 PST 2024
https://github.com/DavidGoldman created https://github.com/llvm/llvm-project/pull/82396
Use the legacy non-ObjC rename logic when dealing with selectors that have zero or one arguments. In addition, make sure we don't add an extra `:` during the rename.
Add a few more tests to verify this works (thanks to @ahoppen for the tests and finding this bug).
>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] [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.
More information about the cfe-commits
mailing list