[clangd] Improve documentation for auto and implicit specs

[kadircet]

Summary

Clangd didn't fill documentation for auto when it wasn't available in
index. Also it wasn't showing any documentations for implicit instantiations.

This patch ensures auto and normal decl case behaves in the same way and also
makes use of the explicit template specialization while fetching comments for
implicit instantiations.

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

[kadircet]

• Rebase

[merge_guards_bot]

Unit tests: fail. 60980 tests passed, 1 failed and 727 were skipped.

failed: lit.lit/shtest-format.py

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, CMakeCache.txt, console-log.txt, test-results.xml

[kadircet]

• Rebase

[merge_guards_bot]

Unit tests: fail. 60981 tests passed, 1 failed and 727 were skipped.

failed: lit.lit/shtest-format.py

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, CMakeCache.txt, console-log.txt, test-results.xml

[ilya-biryukov]

clang-tools-extra/clangd/Hover.cpp

189

What's the purpose of this function?
I don't think its description has semantic meaning in C++, "implicit instantiations" do not have an "explicit specialization"...

It seems to be doing something to change a decl into something that could be used to query the index. If that's the case, we could probably have a name that's closer to the described goal.

207

NIT: maybe remove or Index isn't supplied.
It's clear from the code, no need to duplicate it.

[kadircet]

clang-tools-extra/clangd/Hover.cpp

189

yeah naming is hard :/

it is not just something that can be used to query index, but also for AST, as the decl of instantiation doesn't contain comments attached to specialization.
this is basically returning the explicit specialization used to instantiate D.

any suggestions for the name ?

[ilya-biryukov]

clang-tools-extra/clangd/Hover.cpp

189

I would go with the following (a different comment is welcome, just quickly came up with something):

/// Returns a decl that should be used to produce hover, i.e. the one
/// we should query for documentation comment and use in index queries.
Decl *getDeclForHoverInfo(Decl *D)

The name is not smart and one would definitely need to read the comment to understand why we need it in the first place, but at least it would make sure we avoid confusion.

[kadircet]

• Address comments

clang-tools-extra/clangd/Hover.cpp

189

the problem is, this is not the decl used to produce hover exactly (for example we want to make use of implicit instantiation for printing the name).
we only want to make use of it for comment retrieval, modifying according to that.

[ilya-biryukov]

clang-tools-extra/clangd/Hover.cpp

189

If it's just for the comment, getDeclForComment is perfect :-)

190

There's no such thing as explicit specialization for implicit instantiations.
Maybe remove this comment? It merely duplicates the code and it doesn't really matter what happens there, as long as we get the comment in hover.

192

You might need to do the same for specializations of functions and variables.
Maybe add corresponding tests?

[merge_guards_bot]

Unit tests: fail. 60982 tests passed, 1 failed and 727 were skipped.

failed: lit.lit/shtest-format.py

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[kadircet]

• Address comments
• Extend handling to:
  • var and function templates
  • non-auto case

[merge_guards_bot]

Unit tests: fail. 60981 tests passed, 2 failed and 727 were skipped.

failed: Clangd Unit Tests._/ClangdTests/Hover.DocsFromMostSpecial
failed: lit.lit/shtest-format.py

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[ilya-biryukov]

clang-tools-extra/clangd/Hover.cpp

188

NIT: AST

205

NIT: maybe add assert(&ND == getDeclForComment(&ND))?

377

NIT: ND suggests it's the same as D with a different type.
I'd use a different name, even if it's larger, e.g. CommentD

418

This seems to be doing exactly the same thing as explicitReferenceTargets from FindTarget.cpp

WDYT about exposing the latter in FindTarget.h and using it here?
It seems to be pretty well-defined and useful.

[kadircet]

• Expose explicitReferenceTargets

[merge_guards_bot]

Unit tests: fail. 60982 tests passed, 1 failed and 727 were skipped.

failed: lit.lit/shtest-format.py

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[ilya-biryukov]

clang-tools-extra/clangd/FindTarget.h

194 ↗

(On Diff #234522)

No need to fix this.

The name could probably be better, but we can fix this later. Don't have any good ideas.

195 ↗

(On Diff #234522)

Could you document Mask can't have TemplatePattern and TemplateInstantiation bits?

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

136

Is this the intended behavior?
I would've interepreted Definition as something that was written in the source code, but this one here is definitely synthesized by the compiler.

I'm happy to go either way, just wanted to make sure this is our conscious choice and understand why we're doing this.

[kadircet]

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

136

I also wasn't sure about this one, but thought this would be more useful to users.

Because shows more semantic information about the symbol under the cursor, and we want to achieve that with Hover.

[kadircet]

• Document limitation on Mask parameter

[merge_guards_bot]

Unit tests: fail. 60982 tests passed, 1 failed and 727 were skipped.

failed: lit.lit/shtest-format.py

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[ilya-biryukov]

LGTM

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

136

I actually think it would be more useful to say vector<int> in Name (or Name + TemplateArguments per Sam's comments) and have definition reflect what's written in the source code.

But up to you, can also keep as is.
We can always tweak it later if users complain.

[sammccall]

clang-tools-extra/clangd/FindTarget.h

194 ↗

(On Diff #234522)

I think as a public API this should be clearer on how/when to use and its relation to other things (planning to send a patch, but wanted to discuss a bit here first).

• This is essentially a filter of allTargetDecls (as is targetDecl), but the relationship between the two isn't clear. They should have closely related names (variant) or maybe better be more orthogonal and composed at the callsite.
• The most distinctive word in the name is explicit, but this function doesn't actually have anything to do with explicit vs non-explicit references (just history?)
• It's not clear to me whether we actually want to encapsulate/hide the preference for instantiations over templates here, or whether it should be expressed at the callsite because the policy is decided feature-by-feature. chooseBest(...) vs prefer(TemplateInstantiation, TemplatePattern, ...). The latter seems safer to me, based on experience with targetDecl so far.

A couple of ideas:

• caller invokes allTargetDecls() and then this is a helper function prefer(DeclRelation, DeclRelation, Results) that mutates Results
• targetDecl() becomes the swiss-army filter and accepts a list of "prefer-X-over-Y", which it applies before returning the results with flags dropped

[ilya-biryukov]

clang-tools-extra/clangd/FindTarget.h

194 ↗

(On Diff #234522)

I don't like the idea of targetDecl becoming the swiss-army knife, it's complicated to use as is.
The contract of explicitReferenceTargets is to expose only the decls written in the source code and nothing else, it's much simpler to explain and to use.

It's useful for many things, essentially for anything that needs to answer the question of "what does this name in the source code refers to".

The name could be clearer, the comments might need to be improved.
The only crucial improvement that I'd attempt is getting rid of the Mask parameter, I think there was just one or two cases where it was useful.

[sammccall]

clang-tools-extra/clangd/FindTarget.h

194 ↗

(On Diff #234522)

I don't like the idea of targetDecl becoming the swiss-army knife

Fair enough. I'll poke more in the direction of making it easier to compose allTargetDecls then.

The contract of explicitReferenceTargets is to expose only the decls written in the source code and nothing else, it's much simpler to explain and to use.

So I agree that the interface of targetDecl() is complicated, but I don't think this one is simpler to explain or use - or at least it hasn't been well-explained so far.

For example,

• "to expose only the decls written in the source code" - vector<int> isn't a decl written in the source code.
• "find declarations explicitly referenced in the source code" - return [[{foo}]]; - the class/constructor isn't explicitly referenced, but this function returns it.
• "what does this name in the source code refers to" - a template name refers to the primary template, the (possible) partial specialization, and specialization all at once. Features like find refs/highlight show the equivalence class of names that refer to the same thing, but they don't prefer the specialization for that.
• none of these explanations explains why the function is opinionated about template vs expansion but not about alias vs underlying.

No need to fix this.
The name could probably be better, but we can fix this later. Don't have any good ideas.

I think it's fine (and good!) that we this got added to unblock the hover work and to understand more use cases for this API. But I do think it's time to fix it now, and I'm not convinced a better name will do it (I can't think of a good name to describe the current functionality, either). Let me put a patch together and take a look?

[ilya-biryukov]

clang-tools-extra/clangd/FindTarget.h

194 ↗

(On Diff #234522)

What's the proposed fix?

I might miss some context with the examples you provided, but here's my view on how each of those should be handled:

• vector<int>. This function can return decls, not written in the source code (e.g. template instantiations). References to those decls should be written in the source code explicitly, not the decls themselves.
• return {foo}. I agree this one is a corner case that we should describe. It still provides sensible results that could be used by hover, etc.
• "what does this name in the source code refers to". That's exactly what this function is about. We want to pick the most specific thing possible (i.e. the instantiation). Client code can go from instantiation to template pattern, it can't go the other way. There are exceptions where returning instantiation is not possible - instantiations of template type aliases, dependent template instantiations. We choose template pattern there, but we don't really loose any information (if we choose to return Decl, there's nothing else we can return).
• "none of these explanations explains why the function is opinionated about template vs expansion but not about alias vs underlying." That I agree with, I think it's possible to make it opinionated there and remove the Mask parameter. I'm happy to take a look, but don't want to clash with you if you're doing the same thing. Would you want me to give it a try?

[sammccall]

clang-tools-extra/clangd/FindTarget.h

194 ↗

(On Diff #234522)

What's the proposed fix?

(FWIW I wrote this post upside-down, because I don't/didn't have the ideas worked out and this discussion has shaped my thinking a lot)

My proposal would be to evolve both targetDecl() and explicitReferenceTargets into composable functions that help select the desired results of allTargetDecls(). Policy would be encoded at callsites, so functions would be quite specific (e.g. preferTemplateInstantations), or data-driven filter(DeclRelationSet).
Some reasoning below - basically I'm not convinced that the "do what I mean" behavior is well-defined here.

I agree this one is a corner case that we should describe. It still provides sensible results that could be used by hover, etc.

Yeah, it's the right behaviour. My point is that I don't think the policy being implemented here is "explicitness" - another example from a recent patch, CTAD ^vector x{1,2,3} we want the instantiation.

Maybe it's "specificness", but this raises questions:

• how often is "most specific" the thing we want, vs "decl that's in source code" or "everything for completeness"
• does the concept of 'specificness' generalize to alias vs underlying?

My impression is that it's *sometimes* what we want, but not overwhelmingly often (e.g. not go-to-def or documentHighlights) and that it doesn't generalize well[1]. If that's the case, I think this should be apolicy expressed at callsites in features (preferTemplateInstantiations or something).

It would be nice if there's some global concept of "preferredness", but looking at the needs of different features, I really don't see one - that's why targetDecl is so complicated.

[1] Example of it not generalizing well - for hover we want instantiations over patterns, but after discussing with Kadir I think we want *both* the alias and the underlying so we can show the docs/association from the alias and the structured info from the underlying.

[ilya-biryukov]

clang-tools-extra/clangd/FindTarget.h

194 ↗

(On Diff #234522)

• vector x{1,2,3} should definitely return an instantiation, that aligns with the contract of the function.

I know explicit is a bad name for this concept, it's more intricate.

I would argue we want the most specific one for hover and if we want to look into the underlying decls hover code is simple if it does so explicitly, rather than passing custom filters to allTargetDecls.

My impression is that evolving this into custom filter for allTargetDecls makes the callsites more complicated. Having a single "swiss-knife"-style function is good for sharing implementation, but the client code is more complicated and error-prone with it.

I would strongly suggest to keep explicitReferenceTargets, most code is simpler with it. Most "filtering" and "getting underlying decls" can be done in the client code and the client code is more straightforward with it.

[sammccall]

clang-tools-extra/clangd/FindTarget.h

194 ↗

(On Diff #234522)

I know explicit is a bad name for this concept, it's more intricate.

So at this point, I don't understand what the concept is, neither the name nor the doc comment describe it well. If you want to keep it, can you describe what the concept is at a high level and how it should interact with aliases, and suggest a name that is specific enough that it at least hints at the true distinction vs generic allTargetDecls()?

rather than passing custom filters to allTargetDecls

allTargetDecls doesn't accept filters, and I don't suggest changing that. Are you referring to targetDecls? If so I think we're agreed on that point.

I would argue we want the most specific one for hover and if we want to look into the underlying decls hover code is simple if it does so explicitly

I guess you're saying the alias is more specific here? Unfortunately the alias doesn't preserve the underlying decl in general. We need both decls - one preserving the alias and the other preserving the type arg/overload.

I would strongly suggest to keep explicitReferenceTargets, most code is simpler with it. Most "filtering" and "getting underlying decls" can be done in the client code and the client code is more straightforward with it.

This is what I'm not convinced of. Currently there are 2 users of explicitReferenceTargets, the original internal user and now hover. Which of the other targetDecl callsites (Rename and 5 in XRefs) can use explicitReferenceTargets? How much "getting underlying decls" would they become responsible for? targetDecl() was in part an effort to unify this unwrapping because I found it too difficult to maintain the mechanical bits when they were embedded in each feature.

[ilya-biryukov]

clang-tools-extra/clangd/FindTarget.h

194 ↗

(On Diff #234522)

Here's rough description of the concept:

• pick the most specific Decl, whenever possible. That means instantiations for templates instead of patterns and never look through typedefs (Mask has to be removed, this needs a separate refactoring).
• I described behaviour on particular examples you mentioned.

It's useful because it's a simple way to get the most specific thing written in the source code (even for implicit nodes, we choose to not do complicated things like looking through typedefs).

I suggest to call it getReferencedDecls.

I guess you're saying the alias is more specific here? Unfortunately the alias doesn't preserve the underlying decl in general. We need both decls - one preserving the alias and the other preserving the type arg/overload.

We can get the underlying type in that particular case inside hover, it's just a few lines of code. I don't think there are other cases where we loose information.

This is what I'm not convinced of. Currently there are 2 users of explicitReferenceTargets, the original internal user and now hover. Which of the other targetDecl callsites (Rename and 5 in XRefs) can use explicitReferenceTargets?

Rename uses findExplicitReferences, which uses explicitReferenceTargets internally. XRefs, Rename, Code actions and any refactorings (if we ever do any of those) will benefit from it, but mostly indirectly by using findExplicitReferences. It also provides information about source locations of the identifiers that name things, which is crucial for many of those features.

How much "getting underlying decls" would they become responsible for? targetDecl() was in part an effort to unify this unwrapping because I found it too difficult to maintain the mechanical bits when they were embedded in each feature.

They would sometimes need to go from template instantiation to template declaration or template pattern and (very rarely) get from typedef to its underlying type. Both are very simple operations.

[sammccall]

clang-tools-extra/clangd/FindTarget.h

194 ↗

(On Diff #234522)

I described behaviour on particular examples you mentioned.

Those examples weren't cases where I was questioning the behavior, they were examples of where the behavior wasn't motivated by the description.

The questions I'm referring to are:

• how often is "most specific" the thing we want, vs "decl that's in source code" or "everything for completeness"
• does the concept of 'specificness' generalize to alias vs underlying?

I think the latter is answered but not the former.

We can get the underlying type in that particular case inside hover, it's just a few lines of code. I don't think there are other cases where we loose information.

There are several cases - at least alias instantiations and using decls of overloads. Note that the reported alias decl is the UsingDecl, not the UsingShadowDecl. (Changing that case is possible, but would make more work for all the other callers).

I suggest to call it getReferencedDecls.

This doesn't describe the distinction from targetDecl().

Rename uses findExplicitReferences, which uses explicitReferenceTargets internally. XRefs, Rename, Code actions and any refactorings (if we ever do any of those) will benefit from it, but mostly indirectly by using findExplicitReferences.

findExplicitReferences is certainly useful and has a clear API, but that doens't require explicitReferenceTargets to be public, which is what I'm asking here.

Rename uses targetDecl together with TemplatePattern, which is the callsite I was referring to there.

If the answer is that 2/7 current callsites will prefer explicitReferenceTargets and 5/7 need targetDecl, that doesn't seem like most code, nor does it seem self-evident that code actions added in the future will fall into the first bucket.

[ilya-biryukov]

clang-tools-extra/clangd/FindTarget.h

194 ↗

(On Diff #234522)

how often is "most specific" the thing we want, vs "decl that's in source code" or "everything for completeness"

As mentioned before, I believe the following features would benefit from this: rename, cross-references, code actions and other refactorings.

Note that the reported alias decl is the UsingDecl, not the UsingShadowDecl. (Changing that case is possible, but would make more work for all the other callers).

What are the callers that would need more work for UsingShadowDecl? I lack context to understand this example.

This function is useful because it's simpler to use in some cases than targetDecl (if we get rid of Mask).
Sure, you could write all code using targetDecl, it's also ok to add a few helpers like this to simplify some common cases from my POV.

[ilya-biryukov]

clang-tools-extra/clangd/FindTarget.h

194 ↗

(On Diff #234522)

Note that rename and other features using targetDecl now would actually be totally fine if they switched to the new function.
They actually don't care, but the new function has a simpler interface.

It was never intended to be a replacement. targetDecl does more stuff. However, this one can have a simpler interface. I prefer to have both and use the one with a simpler interface whenever possible.

clang-tools-extra/clangd/Hover.cpp

 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 PrintingPolicy printingPolicyForDecls(PrintingPolicy Base) {
if (!QT.isNull()) {
         return CD->getLambdaCallOperator();
     }
   }
 
   // Non-lambda functions.
   return D->getAsFunction();
 }
 
+// Returns the decl that should be used for querying comments, either from index
+// or ast.
+const NamedDecl *getDeclForComment(const NamedDecl *D) {
+  if (auto *CTSD = llvm::dyn_cast<ClassTemplateSpecializationDecl>(D))
+    if (!CTSD->isExplicitInstantiationOrSpecialization())
+      return CTSD->getSpecializedTemplate();
+  if (auto *VTSD = llvm::dyn_cast<VarTemplateSpecializationDecl>(D))
+    if (!VTSD->isExplicitInstantiationOrSpecialization())
+      return VTSD->getSpecializedTemplate();
+  if (auto *FD = D->getAsFunction())
+    if (FD->isTemplateInstantiation())
+      return FD->getTemplateSpecializationInfo()->getTemplate();
+  return D;
+}
+
 // Look up information about D from the index, and add it to Hover.
-void enhanceFromIndex(HoverInfo &Hover, const Decl *D,
+void enhanceFromIndex(HoverInfo &Hover, const NamedDecl &ND,
                       const SymbolIndex *Index) {
-  if (!Index || !llvm::isa<NamedDecl>(D))
-    return;
-  const NamedDecl &ND = *cast<NamedDecl>(D);
   // We only add documentation, so don't bother if we already have some.
-  if (!Hover.Documentation.empty())
+  if (!Hover.Documentation.empty() || !Index)
     return;
+
   // Skip querying for non-indexable symbols, there's no point.
   // We're searching for symbols that might be indexed outside this main file.
   if (!SymbolCollector::shouldCollectSymbol(ND, ND.getASTContext(),
                                             SymbolCollector::Options(),
                                             /*IsMainFileOnly=*/false))
     return;
   auto ID = getSymbolID(&ND);
   if (!ID)
HoverInfo getHoverContents(const Decl *D, const SymbolIndex *Index) {
   if (!HI.NamespaceScope->empty())
     HI.NamespaceScope->append("::");
   HI.LocalScope = getLocalScope(D);
   if (!HI.LocalScope.empty())
     HI.LocalScope.append("::");
 
   PrintingPolicy Policy = printingPolicyForDecls(Ctx.getPrintingPolicy());
   if (const NamedDecl *ND = llvm::dyn_cast<NamedDecl>(D)) {
-    HI.Documentation = getDeclComment(Ctx, *ND);
     HI.Name = printName(Ctx, *ND);
+    ND = getDeclForComment(ND);
+    HI.Documentation = getDeclComment(Ctx, *ND);
+    enhanceFromIndex(HI, *ND, Index);
   }
 
   HI.Kind = index::getSymbolInfo(D).Kind;
 
   // Fill in template params.
   if (const TemplateDecl *TD = D->getDescribedTemplate()) {
     HI.TemplateParameters =
         fetchTemplateParameters(TD->getTemplateParameters(), Policy);
if (const Expr *Init = Var->getInit())
       HI.Value = printExprValue(Init, Ctx);
   } else if (const auto *ECD = dyn_cast<EnumConstantDecl>(D)) {
     // Dependent enums (e.g. nested in template classes) don't have values yet.
     if (!ECD->getType()->isDependentType())
       HI.Value = ECD->getInitVal().toString(10);
   }
 
   HI.Definition = printDefinition(D);
-  enhanceFromIndex(HI, D, Index);
   return HI;
 }
 
 /// Generate a \p Hover object given the type \p T.
 HoverInfo getHoverContents(QualType T, ASTContext &ASTCtx,
                            const SymbolIndex *Index) {
   HoverInfo HI;
 
   if (const auto *D = T->getAsTagDecl()) {
-    HI.Name = printName(ASTCtx, *D);
     HI.Kind = index::getSymbolInfo(D).Kind;
-    enhanceFromIndex(HI, D, Index);
-  }
+    HI.Name = printName(ASTCtx, *D);
 
-  if (HI.Name.empty()) {
+    const auto *ND = getDeclForComment(D);
+    HI.Documentation = getDeclComment(ASTCtx, *ND);
+    enhanceFromIndex(HI, *ND, Index);
+  } else {
     // Builtin types
     llvm::raw_string_ostream OS(HI.Name);
     PrintingPolicy Policy = printingPolicyForDecls(ASTCtx.getPrintingPolicy());
     T.print(OS, Policy);
   }
   return HI;
 }
 
if (!Invalid) {
         HI.Definition =
             ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))
                 .str();
     }
   }
   return HI;
 }
 
+const Decl *preferInstantiation(llvm::iterator_range<const Decl **> Range) {
+  if (Range.empty())
+    return nullptr;
+
+  auto *D = *Range.begin();
+  for (const Decl *C : Range) {
+    if (llvm::isa<VarTemplateSpecializationDecl>(C) ||
+        llvm::isa<ClassTemplateSpecializationDecl>(C) ||
+        (C->getAsFunction() && C->getAsFunction()->isTemplateInstantiation())) {
+      D = C;
+      break;
+    }
+  }
+  return D;
+}
+
 } // namespace
 
 llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
                                    format::FormatStyle Style,
                                    const SymbolIndex *Index) {
   const SourceManager &SM = AST.getSourceManager();
   llvm::Optional<HoverInfo> HI;
   SourceLocation SourceLocationBeg = SM.getMacroArgExpandedLocation(
       getBeginningOfIdentifier(Pos, SM, AST.getLangOpts()));
 
   if (auto Deduced = getDeducedType(AST.getASTContext(), SourceLocationBeg)) {
     HI = getHoverContents(*Deduced, AST.getASTContext(), Index);
   } else if (auto M = locateMacroAt(SourceLocationBeg, AST.getPreprocessor())) {
     HI = getHoverContents(*M, AST);
   } else {
     auto Offset = positionToOffset(SM.getBufferData(SM.getMainFileID()), Pos);
     if (!Offset) {
       llvm::consumeError(Offset.takeError());
       return llvm::None;
     }
     SelectionTree Selection(AST.getASTContext(), AST.getTokens(), *Offset);
     std::vector<const Decl *> Result;
     if (const SelectionTree::Node *N = Selection.commonAncestor()) {
-      DeclRelationSet Rel = DeclRelation::TemplatePattern | DeclRelation::Alias;
-      auto Decls = targetDecl(N->ASTNode, Rel);
-      if (!Decls.empty()) {
-        HI = getHoverContents(Decls.front(), Index);
+      DeclRelationSet Rel = DeclRelation::TemplateInstantiation |
+                            DeclRelation::TemplatePattern | DeclRelation::Alias;
+      if (const Decl *D = preferInstantiation(targetDecl(N->ASTNode, Rel))) {
+        HI = getHoverContents(D, Index);
         // Look for a close enclosing expression to show the value of.
         if (!HI->Value)
           HI->Value = printExprValue(N, AST.getASTContext());
       }
       // FIXME: support hovers for other nodes?
       //  - certain expressions (sizeof etc)
       //  - built-in types
       //  - literals (esp user-defined)

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

 //===-- HoverTests.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "AST.h"
 #include "Annotations.h"
 #include "Hover.h"
 #include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/StringRef.h"
const std::function<void(HoverInfo &)> ExpectedBuilder;
        }},
       // Implicit template instantiation
       {R"cpp(
           template <typename T> class vector{};
           [[vec^tor]]<int> foo;
           )cpp",
        [](HoverInfo &HI) {
          HI.NamespaceScope = "";
-         HI.Name = "vector";
+         HI.Name = "vector<int>";
          HI.Kind = index::SymbolKind::Class;
-         HI.Definition = "template <typename T> class vector {}";
-         HI.TemplateParameters = {
-             {std::string("typename"), std::string("T"), llvm::None},
-         };
+         HI.Definition = "template <> class vector<int> {}";
        }},
       // Class template
       {R"cpp(
           template <template<typename, bool...> class C,
                     typename = char,
                     int = 0,
                     bool Q = false,
                     class... Ts> class Foo {};
class Foo {})cpp";
           void bar() {
             [[fo^o]]<Foo>();
           }
           )cpp",
        [](HoverInfo &HI) {
          HI.NamespaceScope = "";
          HI.Name = "foo";
          HI.Kind = index::SymbolKind::Function;
-         HI.Definition =
-             R"cpp(template <template <typename, bool...> class C, typename = char, int = 0,
-          bool Q = false, class... Ts>
-void foo())cpp";
+         HI.Definition = "template <> void foo<Foo, char, 0, false, <>>()";
          HI.ReturnType = "void";
          HI.Type = "void ()";
          HI.Parameters.emplace();
-         HI.TemplateParameters = {
-             {std::string("template <typename, bool...> class"),
-              std::string("C"), llvm::None},
-             {std::string("typename"), llvm::None, std::string("char")},
-             {std::string("int"), llvm::None, std::string("0")},
-             {std::string("bool"), std::string("Q"), std::string("false")},
-             {std::string("class..."), std::string("Ts"), llvm::None},
-         };
        }},
       // Function decl
       {R"cpp(
           template<typename, bool...> class Foo {};
           Foo<bool, true, false> foo(int, bool T = false);
 
           void bar() {
             [[fo^o]](3);
const std::function<void(HoverInfo &)> ExpectedBuilder;
               return 17;
             }
             void g() { auto x = [[f^oo]]<int>(); }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "foo";
             HI.Kind = index::SymbolKind::Function;
             HI.NamespaceScope = "";
-            HI.Type = "T ()";
-            HI.Definition = "template <typename T> T foo()";
+            HI.Type = "int ()";
+            HI.Definition = "template <> int foo<int>()";
             HI.Documentation = "Templated function";
-            HI.ReturnType = "T";
+            HI.ReturnType = "int";
             HI.Parameters = std::vector<HoverInfo::Param>{};
-            HI.TemplateParameters = {
-                {std::string("typename"), std::string("T"), llvm::None},
-            };
+            // FIXME: We should populate template parameters with arguments in
+            // case of instantiations.
           }},
       {
           R"cpp(// Anonymous union
             struct outer {
               union {
                 int abc, def;
               } v;
             };
const std::function<void(HoverInfo &)> ExpectedBuilder;
             struct Bar {};
             ^[[auto]] test() -> decltype(Bar()) {
               return Bar();
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.Documentation = "auto function return with trailing type";
           }},
       {
           R"cpp(// trailing return type
             struct Bar {};
             auto test() -> ^[[decltype]](Bar()) {
               return Bar();
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.Documentation = "trailing return type";
           }},
       {
           R"cpp(// auto in function return
             struct Bar {};
             ^[[auto]] test() {
               return Bar();
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.Documentation = "auto in function return";
           }},
       {
           R"cpp(// auto& in function return
             struct Bar {};
             ^[[auto]]& test() {
               static Bar x;
               return x;
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.Documentation = "auto& in function return";
           }},
       {
           R"cpp(// auto* in function return
             struct Bar {};
             ^[[auto]]* test() {
               Bar* bar;
               return bar;
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.Documentation = "auto* in function return";
           }},
       {
           R"cpp(// const auto& in function return
             struct Bar {};
             const ^[[auto]]& test() {
               static Bar x;
               return x;
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.Documentation = "const auto& in function return";
           }},
       {
           R"cpp(// decltype(auto) in function return
             struct Bar {};
             ^[[decltype]](auto) test() {
               return Bar();
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.Documentation = "decltype(auto) in function return";
           }},
       {
           R"cpp(// decltype(auto) reference in function return
             ^[[decltype]](auto) test() {
               static int a;
               return (a);
             }
           )cpp",
const std::function<void(HoverInfo &)> ExpectedBuilder;
             }
             void foo() {
               ^[[decltype]](test()) i = test();
             }
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "Bar";
             HI.Kind = index::SymbolKind::Struct;
+            HI.Documentation =
+                "decltype of function with trailing return type.";
           }},
       {
           R"cpp(// decltype of var with decltype.
             void foo() {
               int I = 0;
               decltype(I) J = I;
               ^[[decltype]](J) K = J;
             }
const std::function<void(HoverInfo &)> ExpectedBuilder;
           R"cpp(// auto on alias
           struct cls {};
           typedef cls cls_type;
           ^[[auto]] y = cls_type();
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "cls";
             HI.Kind = index::SymbolKind::Struct;
+            HI.Documentation = "auto on alias";
           }},
       {
           R"cpp(// auto on alias
           template <class>
           struct templ {};
           ^[[auto]] z = templ<int>();
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "templ<int>";
             HI.Kind = index::SymbolKind::Struct;
+            HI.Documentation = "auto on alias";
           }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
   Symbol IndexSym = func("indexSymbol");
   IndexSym.Documentation = "comment from index";
   SymbolSlab::Builder Symbols;
   Symbols.insert(IndexSym);
for (const auto &Case : Cases) {
     EXPECT_EQ(H->ReturnType, Expected.ReturnType);
     EXPECT_EQ(H->Parameters, Expected.Parameters);
     EXPECT_EQ(H->TemplateParameters, Expected.TemplateParameters);
     EXPECT_EQ(H->SymRange, Expected.SymRange);
     EXPECT_EQ(H->Value, Expected.Value);
   }
 }
 
+TEST(Hover, DocsFromIndex) {
+  Annotations T(R"cpp(
+  template <typename T> class X {};
+  void foo() {
+    au^to t = X<int>();
+    X^<int> w;
+    (void)w;
+  })cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  for (const auto &D : AST.getDiagnostics())
+    ADD_FAILURE() << D;
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  Symbol IndexSym;
+  IndexSym.ID = *getSymbolID(&findDecl(AST, "X"));
+  IndexSym.Documentation = "comment from index";
+  SymbolSlab::Builder Symbols;
+  Symbols.insert(IndexSym);
+  auto Index =
+      MemIndex::build(std::move(Symbols).build(), RefSlab(), RelationSlab());
+
+  for (const auto &P : T.points()) {
+    auto H = getHover(AST, P, format::getLLVMStyle(), Index.get());
+    ASSERT_TRUE(H);
+    EXPECT_EQ(H->Documentation, IndexSym.Documentation);
+  }
+}
+
+TEST(Hover, DocsFromAST) {
+  Annotations T(R"cpp(
+  // doc
+  template <typename T> class X {};
+  // doc
+  template <typename T> void bar() {}
+  // doc
+  template <typename T> T baz;
+  void foo() {
+    au^to t = X<int>();
+    X^<int>();
+    b^ar<int>();
+    au^to T = ba^z<X<int>>;
+    ba^z<int> = 0;
+  })cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  for (const auto &D : AST.getDiagnostics())
+    ADD_FAILURE() << D;
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (const auto &P : T.points()) {
+    auto H = getHover(AST, P, format::getLLVMStyle(), nullptr);
+    ASSERT_TRUE(H);
+    EXPECT_EQ(H->Documentation, "doc");
+  }
+}
+
+TEST(Hover, DocsFromMostSpecial) {
+  Annotations T(R"cpp(
+  // doc1
+  template <typename T> class $doc1^X {};
+  // doc2
+  template <> class $doc2^X<int> {};
+  // doc3
+  template <typename T> class $doc3^X<T*> {};
+  void foo() {
+    X$doc1^<char>();
+    X$doc2^<int>();
+    X$doc3^<int*>();
+  })cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  auto AST = TU.build();
+  for (const auto &D : AST.getDiagnostics())
+    ADD_FAILURE() << D;
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (auto Comment : {"doc1", "doc2", "doc3"}) {
+    for (const auto &P : T.points(Comment)) {
+      auto H = getHover(AST, P, format::getLLVMStyle(), nullptr);
+      ASSERT_TRUE(H);
+      EXPECT_EQ(H->Documentation, Comment);
+    }
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang