[clangd] Abort rename when given the same name

[kbobyrev]

Summary

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

[hokein]

clang-tools-extra/clangd/refactor/Rename.cpp

218

returning an error seems to be an interesting idea, thinking more about that, it might be better than just returning an empty workspaceEdit silently without noticing user.

clang-tools-extra/clangd/unittests/RenameTests.cpp

907

this test is for prepareRename, I think we should put it to TEST(RenameTest, Renameable).

[sammccall]

clang-tools-extra/clangd/refactor/Rename.cpp

218

We don't know what the user's intent was here - it's at least somewhat likely they changed their mind about the operation but hit "enter" instead of "escape". So a message describing the situation "new name is the same as the old name" would be more appropriate than suggesting a correction.

But I'm wondering whether this error message actually helps (vs "succeeding" with no edits). Is it actionable? What's the scenario where the user:
a) doesn't already know that the name is the same, and
b) will take some action as a result (rather than leave the name unchanged)

[hokein]

clang-tools-extra/clangd/refactor/Rename.cpp

218

a message describing the situation "new name is the same as the old name" would be more appropriate than suggesting a correction.

SG.

In an ideal world, I'd expect we emit a non-error message to inform that there is nothing happen for the rename, but LSP doesn't have such option :(

The behavior of editors are varied on this case, e.g.

• VSCode just succeeds with no edits;
• our internal editor emits a message like "nothing to rename";

[kbobyrev]

• Change error message to description rather than suggestion.
• Move test to more appropriate location.

[kbobyrev]

clang-tools-extra/clangd/refactor/Rename.cpp

218

But I'm wondering whether this error message actually helps (vs "succeeding" with no edits). Is it actionable? What's the scenario where the user:
a) doesn't already know that the name is the same, and
b) will take some action as a result (rather than leave the name unchanged)

I can understand this point of view, but I'm a bit sceptical of the "fail" silently approach. The "actionable" for user would be to not type in the same name as before :)

Also, user might wanted to rename the symbol to something slightly different (e.g. fix typo) and then have a typo in new name, too. I think there is value in telling the user that there was no rename because the name is the same - I don't think it hurts them in any way. We're just being explicit in what happened/didn't happen. I think "new name = old name" can be a reasonable "error" scenario and I think there is a definite value in being explicit about what we do or decide not to do.

My point is that I can't see a scenario when user actually wants to rename the symbol deliberately using the same name and I think this should, in fact, be an error to do so.

[hokein]

clang-tools-extra/clangd/unittests/RenameTests.cpp

780

Add a comment or rename Var => SameName, to make the "new_name == old_name" more explicitly.

[kbobyrev]

Add a comment to the test to make it more explicit.

clang-tools-extra/clangd/refactor/Rename.cpp

enum class ReasonToReject {
   NoIndexProvided,
   NonIndexable,
   UsedOutsideFile, // for within-file rename only.
   UnsupportedSymbol,
   AmbiguousSymbol,
 
   // name validation.
   RenameToKeywords,
+  SameName,
 };
 
 llvm::Optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
                                           StringRef MainFilePath,
                                           const SymbolIndex *Index,
                                           bool CrossFile) {
   trace::Span Tracer("Renameable");
   // Filter out symbols that are unsupported in both rename modes.
auto Message = [](ReasonToReject Reason) {
     case ReasonToReject::NonIndexable:
       return "symbol may be used in other files (not eligible for indexing)";
     case ReasonToReject::UnsupportedSymbol:
       return "symbol is not a supported kind (e.g. namespace, macro)";
     case ReasonToReject::AmbiguousSymbol:
       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");
   };
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
 // Return all rename occurrences in the main file.
 std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
 
   auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location());
   if (DeclsUnderCursor.empty())
     return makeError(ReasonToReject::NoSymbolFound);
   if (DeclsUnderCursor.size() > 1)
     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);
 
   auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index,
                            Opts.AllowCrossFile);
   if (Reject)
     return makeError(*Reject);

clang-tools-extra/clangd/unittests/RenameTests.cpp

Case Cases[] = {
 
       {R"cpp(// FIXME: detecting local variables is not supported yet.
         void func() {
           int Conflict;
           int [[V^ar]];
         }
       )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) {
     SCOPED_TRACE(Case.Code);
     Annotations T(Case.Code);
     TestTU TU = TestTU::withCode(T.code());
     TU.HeaderCode = CommonHeader;
     TU.ExtraArgs.push_back("-fno-delayed-template-parsing");
TEST(RenameTest, PrepareRename) {
 
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, FS, ServerOpts);
   runAddDocument(Server, FooHPath, FooH.code());
   runAddDocument(Server, FooCCPath, FooCC.code());
 
   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.
   EXPECT_TRUE(Results->GlobalChanges.empty());
   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});
   EXPECT_FALSE(Results);
   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);
   EXPECT_THAT(llvm::toString(Results.takeError()),
               testing::HasSubstr("is used outside"));
 }
 
 TEST(CrossFileRenameTests, DirtyBuffer) {
   Annotations FooCode("class [[Foo]] {};");
   std::string FooPath = testPath("foo.cc");
   Annotations FooDirtyBuffer("class [[Foo]] {};\n// this is dirty buffer");
   Annotations BarCode("void [[Bar]]() {}");
   std::string BarPath = testPath("bar.cc");