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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev marked an inline comment as not done.
kbobyrev added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:218
+    case ReasonToReject::SameName:
+      return "new name should differ from the old name";
     }
----------------
hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > 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.
> > > 
> > 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)
> > 
> >  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";
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91134