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

[hokein]

Summary

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

[sammccall]

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

250

I don't know what a user would make of this message :-)
It's useful for debugging the implementation, but all we're saying is that we generated a list of edits and then our sanity check failed.

This case occurs when the user selected some code that resolved to a renamable node, but wasn't actually the name of it.
It's basically the same as trying to rename foo with void foo() {^}, which fails with NoSymbolFound.

So I think we should either return NoSymbolFound here or at least use the same error message.

758

nit: robust check -> robustness check

And
"to avoid surprising results if the triggering location *is not actually the name of the node we identified* (e.g. for broken code)."
rather than just repeating "is not renamed"

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

1098

this seems like a weird lever to have to use, and will make it awkward if we want to test behavior in the presence of recovery AST.

Haha, I remember now, I had a fix to generate recovery initializers in more cases which probably made tests harder to right.

That code only works if the field can be resolved though. Maybe try A() : inva^lid(0) {} ?

[hokein]

address comments

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

 }
 
 enum class ReasonToReject {
   NoSymbolFound,
   NoIndexProvided,
   NonIndexable,
   UnsupportedSymbol,
   AmbiguousSymbol,
+  UnrenamableLoc,
 
   // name validation. FIXME: reconcile with InvalidName
   SameName,
 };
 
 llvm::Optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
                                           StringRef MainFilePath,
                                           const SymbolIndex *Index) {
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::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");
   };
   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) {
   //     feasible for local symbols (as by design our index don't index these
   //     symbols by design;
   // To make cross-file rename work for local symbol, we use a hybrid solution:
   //   - run AST-based rename on the main file;
   //   - run index-based rename on other affected files;
   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));
   llvm::for_each(MainFileEdits.asTextEdits(), [&Result](const TextEdit &TE) {
     Result.LocalChanges.push_back(TE.range);
   });
 
   // return the main file edit if this is a within-file rename or the symbol

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

Case Cases[] = {
         }
       )cpp",
        "new name is the same", !HeaderFile, "SameName"},
       {R"cpp(// Ensure it doesn't associate base specifier with base name.
         struct A {};
         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) {
     SCOPED_TRACE(Case.Code);
     Annotations T(Case.Code);
     TestTU TU = TestTU::withCode(T.code());
     TU.ExtraArgs.push_back("-fno-delayed-template-parsing");
     if (Case.IsHeaderFile) {
       // We open the .h file as the main file.
       TU.Filename = "test.h";
       // 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)});
     bool WantRename = true;
     if (T.ranges().empty())
       WantRename = false;
     if (!WantRename) {
       assert(Case.ErrorMessage && "Error message must be set!");