[clang-tools-extra] 91ce6fb - [clangd] Abort rename when given the same name

[Kirill Bobyrev via cfe-commits]

Author: Kirill Bobyrev
Date: 2020-11-11T11:13:47+01:00
New Revision: 91ce6fb5a65f75c1b29c898f2cb76e2c5d1ac681

URL: https://github.com/llvm/llvm-project/commit/91ce6fb5a65f75c1b29c898f2cb76e2c5d1ac681
DIFF: https://github.com/llvm/llvm-project/commit/91ce6fb5a65f75c1b29c898f2cb76e2c5d1ac681.diff

LOG: [clangd] Abort rename when given the same name

When user wants to rename the symbol to the same name we shouldn't do any work.
Bail out early and return error to save compute.

Resolves: https://github.com/clangd/clangd/issues/580

Reviewed By: hokein

Differential Revision: https://reviews.llvm.org/D91134

Added: 
    

Modified: 
    clang-tools-extra/clangd/refactor/Rename.cpp
    clang-tools-extra/clangd/unittests/RenameTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 5adcf6d0e86e..33a9c845beaa 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -123,6 +123,7 @@ enum class ReasonToReject {
 
   // name validation.
   RenameToKeywords,
+  SameName,
 };
 
 llvm::Optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
@@ -213,6 +214,8 @@ llvm::Error makeError(ReasonToReject Reason) {
       return "there are multiple symbols at the given location";
     case ReasonToReject::RenameToKeywords:
       return "the chosen name is a keyword";
+    case ReasonToReject::SameName:
+      return "new name is the same as the old name";
     }
     llvm_unreachable("unhandled reason kind");
   };
@@ -556,6 +559,8 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
     return makeError(ReasonToReject::AmbiguousSymbol);
   const auto &RenameDecl =
       llvm::cast<NamedDecl>(*(*DeclsUnderCursor.begin())->getCanonicalDecl());
+  if (RenameDecl.getName() == RInputs.NewName)
+    return makeError(ReasonToReject::SameName);
   auto Invalid = checkName(RenameDecl, RInputs.NewName);
   if (Invalid)
     return makeError(*Invalid);

diff  --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index b1c0d1d5e2d9..daf63d25ed7f 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -774,6 +774,13 @@ TEST(RenameTest, Renameable) {
         }
       )cpp",
        nullptr, !HeaderFile, nullptr, "Conflict"},
+
+      {R"cpp(// Trying to rename into the same name, SameName == SameName.
+        void func() {
+          int S^ameName;
+        }
+      )cpp",
+       "new name is the same", !HeaderFile, nullptr, "SameName"},
   };
 
   for (const auto& Case : Cases) {
@@ -876,7 +883,7 @@ TEST(RenameTest, PrepareRename) {
 
   auto Results = runPrepareRename(Server, FooCCPath, FooCC.point(),
                                   /*NewName=*/llvm::None, {/*CrossFile=*/true});
-  // verify that for multi-file rename, we only return main-file occurrences.
+  // Verify that for multi-file rename, we only return main-file occurrences.
   ASSERT_TRUE(bool(Results)) << Results.takeError();
   // We don't know the result is complete in prepareRename (passing a nullptr
   // index internally), so GlobalChanges should be empty.
@@ -884,7 +891,7 @@ TEST(RenameTest, PrepareRename) {
   EXPECT_THAT(FooCC.ranges(),
               testing::UnorderedElementsAreArray(Results->LocalChanges));
 
-  // verify name validation.
+  // Name validation.
   Results =
       runPrepareRename(Server, FooCCPath, FooCC.point(),
                        /*NewName=*/std::string("int"), {/*CrossFile=*/true});
@@ -892,7 +899,7 @@ TEST(RenameTest, PrepareRename) {
   EXPECT_THAT(llvm::toString(Results.takeError()),
               testing::HasSubstr("keyword"));
 
-  // single-file rename on global symbols, we should report an error.
+  // Single-file rename on global symbols, we should report an error.
   Results = runPrepareRename(Server, FooCCPath, FooCC.point(),
                              /*NewName=*/llvm::None, {/*CrossFile=*/false});
   EXPECT_FALSE(Results);