[clangd] Fix hover on symbol introduced by using declaration

[tom-anders]

Summary

This fixes https://github.com/clangd/clangd/issues/1284. The example
there was C++20's "using enum", but I noticed that we had the same issue
for other using-declarations.

The problem is that Hover always uses the first element of explicitReferenceTargets which might not always be the one we want.
E.g. in the example from the linked issue, the first element is a UsingEnumDecl and the second one a EnumConstantDecl.

[kadircet]

clang-tools-extra/clangd/Hover.cpp

1126

we actually want to show the using decl in certain cases, e.g:

namespace ns {
void foo(int); void foo(char);
}
using ns::foo;
template <typename T> void bar() { fo^o(T{}); }

so rather than this, can we have some logic that looks like:

- If we have more than two candidates, and there's exactly one `BaseUsingDecl`, prefer that.
- If we have two candidates, prefer the non-`BaseUsingDecl` one.
- Prefer the first. (we hit this case when there's a single candidate or there are `multiple or no` using decls. i think multiple using decls is not possible, but at least we preserve the existing behaviour in such cases).

can you also export this logic to a helper function, probably called pickDeclToUse?

[tom-anders]

Move logic to pickDeclToUse

[kadircet]

clang-tools-extra/clangd/Hover.cpp

1023

nit: you can change this to Candidates.size() <= 2 and get rid of the extra single candidate case above (after changing it to always return Candidate.front() when Candidate.back() is a using decl.)

1026

just drop this if check and always return front, when it isn't a using decl.

1031

can we have a test case for this?

1032

nit: ns::foo not fo

[kadircet]

clang-tools-extra/clangd/Hover.cpp

1011

you can just pass in ArrayRef instead.

[tom-anders]

Add additional test, tidy up logic in pickDeclToUse()

clang-tools-extra/clangd/Hover.cpp

1011

Ah of course, forgot about that nice class

[kadircet]

thanks, lgtm! i think you have commit access now, but let me know if I should land this for you (preferably with an email address for commit attribution)

clang-tools-extra/clangd/Hover.cpp

1022

nit: drop the empty line

[tom-anders]

Yeah I do have commit access now, so I'll land this by myself.

I don't think I have the permissions to close the corresponding issue on Github though, so someone else would need to do that.

[nridge]

In D133664#3790144, @tom-anders wrote:

Yeah I do have commit access now, so I'll land this by myself.

I don't think I have the permissions to close the corresponding issue on Github though, so someone else would need to do that.

It happens automatically if the commit message includes "fixes <link to issue>", which it looks like this one does.

[tom-anders]

Hmm, Github noticed that I referenced the issue with this commit, but didn't close it.
According to https://github.blog/2013-03-18-closing-issues-across-repositories/ closing issues across repos should work, but only if you have push permissions in the repo that has the issue

[kadircet]

In D133664#3791694, @tom-anders wrote:

Hmm, Github noticed that I referenced the issue with this commit, but didn't close it.
According to https://github.blog/2013-03-18-closing-issues-across-repositories/ closing issues across repos should work, but only if you have push permissions in the repo that has the issue

thanks for the heads up, closed it.

clang-tools-extra/clangd/Hover.cpp

if (const auto *ImplicitCast = CastNode->ASTNode.get<ImplicitCastExpr>()) {
       PassType.PassBy = HoverInfo::PassType::Value;
       PassType.Converted = true;
     }
   }
 
   HI.CallPassType.emplace(PassType);
 }
 
+template <unsigned N>
+const NamedDecl *
+pickDeclToUse(const llvm::SmallVector<const NamedDecl *, N> &Candidates) {
+  if (Candidates.empty())
+    return nullptr;
+  if (Candidates.size() == 1)
+    return Candidates.front();
+
+  // This is e.g the case for
+  //     namespace ns { void foo(); }
+  //     void bar() { using ns::foo; f^oo(); }
+  // One declaration in Candidates will refer to the using declaration,
+  // which isn't really useful for Hover. So use the other one,
+  // which in this example would be the actual declaration of foo.
+  if (Candidates.size() == 2) {
+    if (llvm::isa<BaseUsingDecl>(Candidates.front()))
+      return Candidates.back();
+    if (llvm::isa<BaseUsingDecl>(Candidates.back()))
+      return Candidates.front();
+  }
+
+  // For something like
+  //     namespace ns { void foo(int); void foo(char) }
+  //     using ns::fo
+  //     template <typename T> void bar() { fo^o(T{}); }
+  // we actually want to show the using declaration,
+  // it's not clear which declaration to pick otherwise.
+  auto BaseDecls = llvm::make_filter_range(Candidates, [](const NamedDecl *D) {
+    return llvm::isa<BaseUsingDecl>(D);
+  });
+  if (std::distance(BaseDecls.begin(), BaseDecls.end()) == 1)
+    return *BaseDecls.begin();
+
+  return Candidates.front();
+}
+
 } // namespace
 
 llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
                                    const format::FormatStyle &Style,
                                    const SymbolIndex *Index) {
   PrintingPolicy PP =
       getPrintingPolicy(AST.getASTContext().getPrintingPolicy());
   const SourceManager &SM = AST.getSourceManager();
if (!HI) {
     // Editors send the position on the left of the hovered character.
     // So our selection tree should be biased right. (Tested with VSCode).
     SelectionTree ST =
         SelectionTree::createRight(AST.getASTContext(), TB, Offset, Offset);
     if (const SelectionTree::Node *N = ST.commonAncestor()) {
       // FIXME: Fill in HighlightRange with range coming from N->ASTNode.
       auto Decls = explicitReferenceTargets(N->ASTNode, DeclRelation::Alias,
                                             AST.getHeuristicResolver());
-      if (!Decls.empty()) {
-        HI = getHoverContents(Decls.front(), PP, Index, TB);
+      if (const auto *DeclToUse = pickDeclToUse(Decls)) {
+        HI = getHoverContents(DeclToUse, PP, Index, TB);
         // Layout info only shown when hovering on the field/class itself.
-        if (Decls.front() == N->ASTNode.get<Decl>())
-          addLayoutInfo(*Decls.front(), *HI);
+        if (DeclToUse == N->ASTNode.get<Decl>())
+          addLayoutInfo(*DeclToUse, *HI);
         // Look for a close enclosing expression to show the value of.
         if (!HI->Value)
           HI->Value = printExprValue(N, AST.getASTContext());
         maybeAddCalleeArgInfo(N, *HI, PP);
       } else if (const Expr *E = N->ASTNode.get<Expr>()) {
         HI = getHoverContents(E, AST, PP, Index);
       } else if (const Attr *A = N->ASTNode.get<Attr>()) {
         HI = getHoverContents(A, AST);
       }
       // FIXME: support hovers for other nodes?

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

const std::function<void(HoverInfo &)> ExpectedBuilder;
           [](HoverInfo &HI) {
             HI.Name = "foo";
             HI.Kind = index::SymbolKind::Variable;
             HI.NamespaceScope = "ns::";
             HI.Type = "int";
             HI.Definition = "int foo";
           }},
       {
+          R"cpp(// Function definition via using declaration
+            namespace ns { 
+              void foo(); 
+            }
+            int main() {
+              using ns::foo;
+              ^[[foo]]();
+            }
+          )cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "foo";
+            HI.Kind = index::SymbolKind::Function;
+            HI.NamespaceScope = "ns::";
+            HI.Type = "void ()";
+            HI.Definition = "void foo()";
+            HI.Documentation = "";
+            HI.ReturnType = "void";
+            HI.Parameters = std::vector<HoverInfo::Param>{};
+          }},
+      {
           R"cpp(// Macro
             #define MACRO 0
             int main() { return ^[[MACRO]]; }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "MACRO";
             HI.Kind = index::SymbolKind::Macro;
             HI.Definition = "#define MACRO 0\n\n"
// Expands to
             HI.Kind = index::SymbolKind::EnumConstant;
             HI.NamespaceScope = "";
             HI.LocalScope = "Hello::";
             HI.Type = "enum Hello";
             HI.Definition = "ONE";
             HI.Value = "0";
           }},
       {
+          R"cpp(// C++20's using enum
+            enum class Hello {
+              ONE, TWO, THREE,
+            };
+            void foo() {
+              using enum Hello;
+              Hello hello = [[O^NE]];
+            }
+          )cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "ONE";
+            HI.Kind = index::SymbolKind::EnumConstant;
+            HI.NamespaceScope = "";
+            HI.LocalScope = "Hello::";
+            HI.Type = "enum Hello";
+            HI.Definition = "ONE";
+            HI.Value = "0";
+          }},
+      {
           R"cpp(// Enumerator in anonymous enum
             enum {
               ONE, TWO, THREE,
             };
             void foo() {
               int hello = [[O^NE]];
             }
           )cpp",