[clangd] Fix rename for symbol introduced by UsingDecl

[tom-anders]

Summary

Fixes https://github.com/clangd/clangd/issues/170

This patch actually consists of 2 fixes:

1. Add handling for UsingShadowDecl to canonicalRenameDecl(). This fixes the issue described in https://github.com/clangd/clangd/issues/170.

2. Avoid the "there are multiple symbols under the cursor error" by applying similar logic as in https://reviews.llvm.org/D133664. This also partly fixes https://github.com/clangd/clangd/issues/586.

[tom-anders]

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

169

I'm not really happy with the name here, open for suggestions

[sammccall]

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.

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

169

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

169

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.

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

843

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)

[sammccall]

(Thanks for fixing this BTW!)

[sammccall]

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

169

I sent https://reviews.llvm.org/D135506 to remove this behavior from FindTarget.

[tom-anders]

In 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?

[tom-anders]

Add additional tests, only handle UsingDecl instead of BaseUsingDecl

[sammccall]

Thanks!

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

169

you might want to call this filterRenameTargets or something, with the idea that we may add restrictions other than UsingDecl in future

170

nit: llvm::erase_if(Decls, [](const NamedDecl *D) { ... })?

[tom-anders]

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

170

Does not work unfortunately, DenseSet does not have the erase(iterator first, iterator last) overload, there's only erase(iterator pos).

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

for (const FieldDecl *Candidate : FieldParent->fields())
       if (Field->getDeclName() == Candidate->getDeclName())
         return Candidate->getCanonicalDecl();
     elog("FieldParent should have field with the same name as Field.");
   }
   if (const auto *VD = dyn_cast<VarDecl>(D)) {
     if (const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember())
       return canonicalRenameDecl(OriginalVD);
   }
+  if (const auto *UD = dyn_cast<UsingShadowDecl>(D)) {
+    if (const auto *TargetDecl = UD->getTargetDecl())
+      return canonicalRenameDecl(TargetDecl);
+  }
   return dyn_cast<NamedDecl>(D->getCanonicalDecl());
 }
 
 llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
                                                SourceLocation TokenStartLoc) {
   unsigned Offset =
       AST.getSourceManager().getDecomposedSpellingLoc(TokenStartLoc).second;
 
   SelectionTree Selection = SelectionTree::createRight(
       AST.getASTContext(), AST.getTokens(), Offset, Offset);
   const SelectionTree::Node *SelectedNode = Selection.commonAncestor();
   if (!SelectedNode)
     return {};
 
   llvm::DenseSet<const NamedDecl *> Result;
   for (const NamedDecl *D :
        targetDecl(SelectedNode->ASTNode,
                   DeclRelation::Alias | DeclRelation::TemplatePattern,
                   AST.getHeuristicResolver())) {
     Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
 
+void filterRenameTargets(llvm::DenseSet<const NamedDecl *> &Decls) {
+  // For something like
+  //     namespace ns { void foo(); }
+  //     void bar() { using ns::f^oo; foo(); }
+  // locateDeclAt() will return a UsingDecl and foo's actual declaration.
+  // For renaming, we're only interested in foo's declaration, so drop the other
+  // one. There should never be more than one UsingDecl here, otherwise the
+  // rename would be ambiguos anyway.
+  auto UD = std::find_if(Decls.begin(), Decls.end(), [](const NamedDecl *D) {
+    return llvm::isa<UsingDecl>(D);
+  });
+  if (UD != Decls.end()) {
+    Decls.erase(UD);
+  }
+}
+
 // By default, we exclude symbols from system headers and protobuf symbols as
 // renaming these symbols would change system/generated files which are unlikely
 // to be good candidates for modification.
 bool isExcluded(const NamedDecl &RenameDecl) {
   const auto &SM = RenameDecl.getASTContext().getSourceManager();
   return SM.isInSystemHeader(RenameDecl.getLocation()) ||
          isProtoFile(RenameDecl.getLocation(), SM);
 }
Range CurrentIdentifier = halfOpenToRange(
       SM, CharSourceRange::getCharRange(IdentifierToken->location(),
                                         IdentifierToken->endLocation()));
   // FIXME: Renaming macros is not supported yet, the macro-handling code should
   // be moved to rename tooling library.
   if (locateMacroAt(*IdentifierToken, AST.getPreprocessor()))
     return makeError(ReasonToReject::UnsupportedSymbol);
 
   auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location());
+  filterRenameTargets(DeclsUnderCursor);
   if (DeclsUnderCursor.empty())
     return makeError(ReasonToReject::NoSymbolFound);
   if (DeclsUnderCursor.size() > 1)
     return makeError(ReasonToReject::AmbiguousSymbol);
   const auto &RenameDecl = **DeclsUnderCursor.begin();
   const auto *ID = RenameDecl.getIdentifier();
   if (!ID)
     return makeError(ReasonToReject::UnsupportedSymbol);

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

llvm::StringRef Tests[] = {
       )cpp",
 
       // ObjC, should not crash.
       R"cpp(
         @interface ObjC {
           char [[da^ta]];
         } @end
       )cpp",
+
+      // Issue 170: Rename symbol introduced by UsingDecl
+      R"cpp(
+        namespace ns { void [[f^oo]](); } 
+
+        using ns::[[f^oo]];
+
+        void f() {
+            [[f^oo]]();
+            auto p = &[[f^oo]];
+        }
+      )cpp",
+
+      // Issue 170: using decl that imports multiple overloads
+      // -> Only the overload under the cursor is renamed
+      R"cpp(
+        namespace ns { int [[^foo]](int); char foo(char); }
+        using ns::[[foo]];
+        void f() {
+          [[^foo]](42);
+          foo('x');
+        }
+      )cpp",
   };
   llvm::StringRef NewName = "NewName";
   for (llvm::StringRef T : Tests) {
     SCOPED_TRACE(T);
     Annotations Code(T);
     auto TU = TestTU::withCode(Code.code());
     TU.ExtraArgs.push_back("-xobjective-c++");
     auto AST = TU.build();
     auto Index = TU.index();
Case Cases[] = {
        "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 {
           A() : inva^lid(0) {}
         };
       )cpp",
        "no symbol", false},
+
+      {R"cpp(// FIXME we probably want to rename both overloads here,
+             // but renaming currently assumes there's only a 
+             // single canonical declaration.
+        namespace ns { int foo(int); char foo(char); }
+        using ns::^foo;
+      )cpp",
+       "there are multiple symbols at the given location", !HeaderFile},
   };
 
   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) {