[PATCH] D101816: [clangd] don't rename if the triggering loc is not actually being renamed.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman.
hokein requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

See context: https://github.com/clangd/clangd/issues/765


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101816

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
@@ -1071,6 +1071,16 @@
         struct B : priv^ate A {};
       )cpp",
        "Cannot rename symbol: there is no symbol at the given location", false},
+      {R"cpp(// Ensure it doesn't associate base specifier with base name.
+        /*error-ok*/
+        struct A {
+          int x;
+          // Without recovery-ast, there is no CtorInitializer, we should reject
+          // the rename rather then renaming the class A.
+          A() : x(inv^alid) {}
+        };
+      )cpp",
+       "Cannot rename symbol: no rename at the given location", false},
   };
 
   for (const auto& Case : Cases) {
@@ -1084,6 +1094,8 @@
       // Parsing the .h file as C++ include.
       TU.ExtraArgs.push_back("-xobjective-c++-header");
     }
+    TU.ExtraArgs.push_back("-Xclang");
+    TU.ExtraArgs.push_back("-fno-recovery-ast");
     auto AST = TU.build();
     llvm::StringRef NewName = Case.NewName;
     auto Results = rename({T.point(), NewName, AST, testPath(TU.Filename)});
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -180,6 +180,7 @@
   NonIndexable,
   UnsupportedSymbol,
   AmbiguousSymbol,
+  UnrenamableLoc,
 
   // name validation. FIXME: reconcile with InvalidName
   SameName,
@@ -245,6 +246,8 @@
       return "there are multiple symbols at the given location";
     case ReasonToReject::SameName:
       return "new name is the same as the old name";
+    case ReasonToReject::UnrenamableLoc:
+      return "no rename at the given location";
     }
     llvm_unreachable("unhandled reason kind");
   };
@@ -751,6 +754,25 @@
   auto MainFileRenameEdit = renameWithinFile(AST, RenameDecl, RInputs.NewName);
   if (!MainFileRenameEdit)
     return MainFileRenameEdit.takeError();
+
+  // Check the rename-triggering location is actually being renamed.
+  // This is a robust check to avoid surprising rename results -- if the
+  // triggering location is not renamed (e.g. for broken code), then rename is
+  // likely not what users expect, so we reject this kind of rename.
+  auto StartOffset = positionToOffset(MainFileCode, CurrentIdentifier.start);
+  auto EndOffset = positionToOffset(MainFileCode, CurrentIdentifier.end);
+  if (!StartOffset)
+    return StartOffset.takeError();
+  if (!EndOffset)
+    return EndOffset.takeError();
+  if (llvm::find_if(
+          *MainFileRenameEdit,
+          [&StartOffset, &EndOffset](const clang::tooling::Replacement &R) {
+            return R.getOffset() == *StartOffset &&
+                   R.getLength() == *EndOffset - *StartOffset;
+          }) == MainFileRenameEdit->end()) {
+    return makeError(ReasonToReject::UnrenamableLoc);
+  }
   RenameResult Result;
   Result.Target = CurrentIdentifier;
   Edit MainFileEdits = Edit(MainFileCode, std::move(*MainFileRenameEdit));


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D101816.342655.patch
Type: text/x-patch
Size: 3160 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210504/213e9166/attachment-0001.bin>