[PATCH] D91134: [clangd] Abort rename when given the same name

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
kbobyrev requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91134

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


Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -876,7 +876,7 @@
 
   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 +884,7 @@
   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 +892,14 @@
   EXPECT_THAT(llvm::toString(Results.takeError()),
               testing::HasSubstr("keyword"));
 
-  // single-file rename on global symbols, we should report an error.
+  Results =
+      runPrepareRename(Server, FooCCPath, FooCC.point(),
+                       /*NewName=*/std::string("func"), {/*CrossFile=*/true});
+  EXPECT_FALSE(Results);
+  EXPECT_THAT(llvm::toString(Results.takeError()),
+              testing::HasSubstr("new name should differ"));
+
+  // 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);
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -123,6 +123,7 @@
 
   // name validation.
   RenameToKeywords,
+  SameName,
 };
 
 llvm::Optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
@@ -213,6 +214,8 @@
       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 should differ from the old name";
     }
     llvm_unreachable("unhandled reason kind");
   };
@@ -558,6 +561,8 @@
     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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D91134.304086.patch
Type: text/x-patch
Size: 2909 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201110/55fcdd81/attachment.bin>