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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:250
+    case ReasonToReject::UnrenamableLoc:
+      return "no rename at the given location";
     }
----------------
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.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:758
+
+  // Check the rename-triggering location is actually being renamed.
+  // This is a robust check to avoid surprising rename results -- if the
----------------
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"


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:1098
+    TU.ExtraArgs.push_back("-Xclang");
+    TU.ExtraArgs.push_back("-fno-recovery-ast");
     auto AST = TU.build();
----------------
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) {}` ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101816/new/

https://reviews.llvm.org/D101816