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 <d...@google.com> Date: Tue, 20 Feb 2024 13:14:26 -0500 Subject: [PATCH 1/2] [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 <d...@google.com> Date: Thu, 22 Feb 2024 12:36:18 -0500 Subject: [PATCH 2/2] 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)); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits