diff options
author | David Goldman <dallasftball@gmail.com> | 2024-02-23 14:11:39 -0500 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-02-23 14:11:39 -0500 |
commit | 59e5519c81c57a66424d657864ce69cb0efdc7d8 (patch) | |
tree | d3f3bcf1354e8e4c0acd3b9595a99d707f5d315b | |
parent | 07fd5ca3a8bd270b26b21ea28501f5edcb519709 (diff) |
[clangd] Fix renaming single argument ObjC methods (#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).
-rw-r--r-- | clang-tools-extra/clangd/refactor/Rename.cpp | 19 | ||||
-rw-r--r-- | clang-tools-extra/clangd/unittests/RenameTests.cpp | 138 |
2 files changed, 152 insertions, 5 deletions
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp index 650862c99bcd..4e135801f685 100644 --- a/clang-tools-extra/clangd/refactor/Rename.cpp +++ b/clang-tools-extra/clangd/refactor/Rename.cpp @@ -811,8 +811,18 @@ 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)) { + // The custom ObjC selector logic doesn't handle the zero arg selector + // 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)); + + // 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 +940,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 d91ef85d6727..7d9252110b27 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. |