[PATCH] D135489: [clangd] Fix rename for symbol introduced by UsingDecl

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Want to probe a bit at the behavior we're aiming for here.
TL;DR is I think the handling is basically right but considering the general case tells us how to simplify filterBaseUsingDecl.

---

The testcase is great to illustrate the common case.
The thing we need to generalize it is multiple symbols (e.g. overloads).

  namespace ns { int foo(int); char foo(char); }
  using ns::foo;
  int x = foo(42);
  char y = foo('x');

What should happen when we rename `foo(int)` on line 1?

- rename both functions
- rename one function + the usingdecl
- rename one function and not the usingdecl
- rename one function and add a second usingdecl
- return an error

In general the UsingDecl will be in another file and not visible in the AST. Index only knows "there's a reference there". So I think our only real option is to rename one function + the UsingDecl.
(Assuming we want the common non-overloaded case to work. And assuming we don't want to do something drastic like "silently rename overload sets together in all cases").

What should happen when we rename `using ns::foo` on line 2?

- rename both functions
- rename one arbitrarily
- return an error

Renaming both functions sounds great here. However for now the rename implementation requires a single canonical decl, so we need to return an error for now.
If there's a single decl, renaming it seems fine.

What should happen when we rename `foo(42)` on line 3?

- rename both functions
- rename one function + the usingdecl
- rename one function and not the usingdecl
- rename one function and add a second usingdecl
- return an error

We *can* rely on the UsingDecl being visible here. However we should be reasonably consistent with the first case, which I think rules out options 1, 3 and 5.
Splitting the usingdecl is a neat trick, but complicated, so let's start with renaming one + functiondecl and maybe tack that on later.



================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:169
+// For renaming, we're only interested in foo's declaration, so drop the other one
+void filterBaseUsingDecl(llvm::DenseSet<const NamedDecl *>& Decls) {
+  if (Decls.size() == 2) {
----------------
tom-anders wrote:
> I'm not really happy with the name here, open for suggestions
This only comes up when renaming the UsingDecl itself (else we reach the UsingShadowDecl rather than this one).

I think we should just unconditionally drop the UsingDecl from the list. The target decls will be in the list, and we'll do the right thing (rename one if unambiguous, error if there are multiple).


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:169
+// For renaming, we're only interested in foo's declaration, so drop the other one
+void filterBaseUsingDecl(llvm::DenseSet<const NamedDecl *>& Decls) {
+  if (Decls.size() == 2) {
----------------
sammccall wrote:
> tom-anders wrote:
> > I'm not really happy with the name here, open for suggestions
> This only comes up when renaming the UsingDecl itself (else we reach the UsingShadowDecl rather than this one).
> 
> I think we should just unconditionally drop the UsingDecl from the list. The target decls will be in the list, and we'll do the right thing (rename one if unambiguous, error if there are multiple).
I'm not sure it's right to handle BaseUsingDecl instead of UsingDecl here.

The other case is UsingEnumDecl, and I don't see any reason to treat that as a non-renaming alias as opposed to a simple reference. It doesn't actually introduce an alias of the enum it names! (I see that we *are* treating it that way in FindTarget, but I guess we should just fix that instead).

Certainly if we *are* deliberately handling UsingEnumDecl here we should have a testcase for it.


================
Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:832
+      )cpp",
   };
   llvm::StringRef NewName = "NewName";
----------------
Can you add a test case (or argue with me that the behavior should be something else!):

```
namespace ns { int [[^foo]](int); char foo(char); }
using ns::[[foo]];
void f() {
  [[^foo]](42);
  foo('x');
}
```

And a negative case to Renameable:
```
namespace ns { int foo(int); char foo(char); }
using ns::^foo;
```
(the error should be "multiple symbols" ideally - "not a supported kind" is confusing if we support renaming on a non-overload using)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135489