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

[Tom Praschan via Phabricator via cfe-commits]

tom-anders marked 4 inline comments as done.
tom-anders added a comment.

In D135489#3844426 <https://reviews.llvm.org/D135489#3844426>, @sammccall wrote:

>   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").

Agreed.

> 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.

Agreed, I added a FIXME in the test that we might want to extend canonicalRenameDecl to return multiple Decls (Probably a `llvm::DenseSet`?)
There already is a similar issue when renaming virtual methods with `size_overridden_methods() > 1`.

> 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.

Sounds good, splitting the using decl would be fancy though, maybe also add a FIXME for this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135489