[clangd] Introduce a structured hover response

[kadircet]

Summary

Change ClangdServer layer to output a structured response for Hover,
which can be rendered by client according to their needs.

[kadircet]

This is still WIP, just looking for feedbacks on HoverInfo struct and overall layering

[sammccall]

Layering and such looks good. This should compose well with D58547

clang-tools-extra/clangd/XRefs.h

53

I'm not sure about reuse of LocatedSymbol - do we want to commit to returning decl/def locations?
Name might be enough here.

55

This comes from LSP, and within the scope of C++ I think we might want stronger semantics here.
I think it's likely we actually want to show namespace vs class scope differently, too - maybe these should be separate fields?

56

SymbolInfo is a bit of a mess. Maybe we just want SymbolInfo::Kind? (LSP SymbolKind is tempting but loses a lot of info for C++).

I do think we'll need to extend SymbolInfo::Kind or eventually use our own enum, e.g. lambdas may need their own kind (they're variables, but don't have a printable type and need to be displayed more like functions)

58

I think we probably want a struct to represent types, so we can annotate it with links later on if needed. (because type can be e.g. mytemplate<mytype>). This wouldn't matter except we should probably reuse it...

58

we may want ReturnType too (unless you want to overload Type for that - I'd suggest against it because of e.g. lambdas)

60

maybe vector<param> params, where param is struct { string name; Type type }?

We might render as:

---

Parameters:

• value: std::string
• size: int (default = 0)

64

TemplateArgs might want to follow the same structure as params?

[kadircet]

• Address comments

[kadircet]

clang-tools-extra/clangd/XRefs.h

53

It might be nice to provide editors with enough info to jump to definition(it was brought up during last meeting). But happy to reduce it to just name.

56

As you mentioned we might need to extend LSP's enum, but switching to it anyway. Currently it doesn't support "macro" kind exactly.

[ilya-biryukov]

Was passing by, just a small clarifying question...

clang-tools-extra/clangd/XRefs.h

73

What does Type mean for non-type and template template parameters?
Could you add a comment?

[kadircet]

clang-tools-extra/clangd/XRefs.h

73

added comment for template template parameters.

For non-type template params, isn't it clear that this will hold the type of that param? e.g template <C<int>... X> -> C<int>...

[kadircet]

• Add comments for non-type params

[ilya-biryukov]

clang-tools-extra/clangd/XRefs.h

73

Ah, sure, sorry, I meant the type parameters :-)

[ilya-biryukov]

clang-tools-extra/clangd/XRefs.h

57

Same about type template parameters.
Probably just holds class or typename?

[kadircet]

I will start adding test cases(and most likely change the way I populate the
struct at some places) if everyone is happy with current representation for
HoverInfo.

clang-tools-extra/clangd/XRefs.h

57

Exactly, adding comments.

[ilya-biryukov]

clang-tools-extra/clangd/XRefs.h

53

NIT: maybe go with std::string Type at a use-site instead?
The scope of the name is large enough to make single-letter names a bit confusing

[sammccall]

(I only got about halfway through the implementation - it's missing a lot of comments I think ;-))

clang-tools-extra/clangd/XRefs.cpp

539

I think this is basically ASTContext::getTypeDeclType()?

549

what does this have to do with decls?

563

printDefinition?

574

I don't think it's reasonable to define this private helper as an overload of operator<<.
Make it a function, or inline it? (I think used only once)

1207

avoid emitting the space if T/name are empty?

1214

This doesn't seem sufficiently self-documenting.

clang-tools-extra/clangd/XRefs.h

52

This needs docs :-)

53

(this is not done I think - LocatedSymbol is still here)

54

one line doc for this struct?

58

needs a real name
mention the no-type case (macros)

59

mention the unnamed case

60

mention the no-default case

64

qualiffied -> qualified

65

what's the difference between Scope and ParentScope?
Can we give them more obvious names, or a comment?
(The current comment doesn't really say anything)

69

This sounds like it's a source location, (e.g. "file.cc:42:7")
But I think it's rather source code?

72

I'm not sure this is significant or should even be true (e.g. if T is a function reference?)

Could we explain the relationship between the different fields here instead, maybe briefly with examples? (A function will have return type and parameters set, etc)

73

I get the curiosity, but this is too much text, and only covers template parameters which are not the most important case.

Can we just say something like The pretty-printed parameter type, e.g. "int", or "typename" (in TemplateParameters)?

We should do something sensible for template template parameters, but it's not important or non-obvious enough to document.

73

T needs a real name.

[sammccall]

clang-tools-extra/clangd/XRefs.h

53

We had discussed making Type a struct, in case we want to add links etc to it later in a back-compatible way.

The typedef doesn't achieve that, I agree with just inlining std::string if we're not going to have the struct.

[kadircet]

• Address comments
• Make little modifications to existing tests

clang-tools-extra/clangd/XRefs.cpp

574

it is also used within render, making it a function

clang-tools-extra/clangd/XRefs.h

65

We've been using Name and Scope to represent two parts of qualified names across the codebase.
Comment was unfortunately misplaced :/ LMK if you have any other choices for the name

[kadircet]

clang-tools-extra/clangd/XRefs.cpp

539

No longer needed

549

No longer needed

1214

No longer needed

[sammccall]

comments on interface again, will take another pass at implementation

clang-tools-extra/clangd/XRefs.h

53

I'm having trouble following the "serialized" sentence. Maybe "It can be rendered as a hover panel, or embedding clients can use the structured information to provide their own UI"?

65

Yes, but here you've got three concepts: name/scope/parentscope (now called containedin?). And this covers lots of cases that our existing Name/Scope doesn't really (there are lots of symbols we don't index).

It wasn't a rhetorical question, I can't suggest better names because I don't know what they are.

My suggestion would be to split into namespace scope/local scope/name, such that Name has no ::, NamespaceScope has exactly the (non-inline) enclosing namespaces, and NamespaceScope + LocalScope + Name == QName.

So for a local in a class member, NamespaceScope == "clang::clangd"", LocalScope == "SymbolCollector::consume:: and Name = Symbols. Or just Name = Symbols and the others are blank, depending on how we choose to display locals.

76

This seems overlapping/non-orthogonal with scope. It doesn't let us render a member as ClassName::method for instance.
If we really need "In function x::Y" rather than "in x::Y", we could add a member "ImmediateScopeKind" or something. But I'm not sure we do.

89

nit: just "templates"?

(e.g. if we hover over a call to std::swap, we might be showing the instantiation rather than the declaration?)

93

Even if this struct ends up containing the range, it might be more obvious to have render() produce the MarkupContent only, leaving the caller to pull out the range themselves.

Converting the range isn't closely related to rendering, and I think for cleanest layering/testing you want to return FormattedString after rL360151 (which doesn't have a Hover equivalent).

[sammccall]

Main comment is that I think the code is doing too much work to exactly reproduce the current output, and include as much information as possible.
Minimizing the diff is good all else equal, but one of the goals here is to have richer hovercards that are more consistent across the different codepaths that generate them.

clang-tools-extra/clangd/XRefs.cpp

455

This has been pulled from DeclPrinter, so by definition duplicates info in the printed decl. We should only do this for info that's important, and I'm not sure most of this meets the bar.

• that a function is virtual is certainly important (but isVirtual(), not isVirtualAsWritten())
• that the declaration we found for a function is extern is certainly unimportant (it says something about the *decl*, not the function)
• that a function is static means *something* important, but being a non-instance member is pretty different from being an internal helper function. Saying Kind=StaticMethod seems more useful for presentation than putting "static" next to the type again.

Aside from this, the current implementation puts these specifiers in the type, which they're not - functions don't return virtual ints or static strings.

I think we can probably drop these for now, and leave them in the printed decl only. We may want to revisit this as part of having Kind describe results more precisely.

672

what are the storage class specifiers that are relevant to function parameters?

683

Not totally sure this is the best way to model variadics. We could consider leaving it out for now.

1169

This really seems like the wrong idea to me.
"Kind" is a category description suitable to be shown to the user, it's supposed to be useful.
In odd cases we may choose to do things like not show the type if the kind is function.

But HoverInfo is basically a box of facts to render, and using totally different rendering strategies for different kinds (and assuming strings mean macros) cuts against this.

1177

why do we have this no-definition case?
Please avoid reinventing code to print C++ syntax if possible...

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

940

hmm, this is a bit weird - this uses specialization syntax but there's no actual specialization here right?
I think the old output without template<> is probably better if possible.

[kadircet]

Thanks for also taking a look at the implementation, it is not complete yet. I am rather waiting for a green light on struct itself, so that I can write tests.

clang-tools-extra/clangd/XRefs.cpp

672

i suppose only "register" is allowed in the context of a function parameter. but doesn't matter now as requested above, leaving specifiers out.

1177

oops, this branch is a leftover :/

clang-tools-extra/clangd/XRefs.h

89

Existing hover behavior is to show declaration, except auto, in that case we might show instantiations.

But you are right it is not necessarily only declarations.

93

I don't think caller of this method would have enough context to deduce symbol's range so leaving the Range in the struct, but changing the output

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

940

The old behavior was inconsistent in the case of auto. We print the decl in all cases, but print the type in the case of auto. For example, if you had
initializer_list<int> i = {1,2} instead of auto i = {1,2} you would get the new response I've provided in this test.

I agree this looks weird in the case of instantiations but I believe it is more important to give a consistent look.

[kadircet]

• Address comments

[kadircet]

• Added tests, and went over implementation.

I am looking for input on:

• Behavior on lambdas, currently it just keeps the old behaviour. I believe they should look more like functions. Do you think it is necessary for first patch?
• Template classes/functions, what we have in definition is not great but I don't think it is too much of a problem since semantic representation carries all the useful information.
• Declared in #SOME_KIND# #SOME_PLACE#, currently we drop SOME_KIND and it needs additional info to be propogated since it is not the type of the symbol but rather it is immediate container. I don't think it is necessary for initial version.
• Discrepencies between hovering over auto/decltype and explicitly spelled types, again this keeps the existing behaviour. You can see details in testcases.

[kadircet]

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

940

as discussed offline, leaving this as it is in the initial version and adding tests with both auto and non-auto cases.

[sammccall]

Thanks, I think you're right about all of this. Implementation looks OK too. Please add TODOs for the cases where we're punting to later vs deciding not to fix.

In D61497#1512003, @kadircet wrote:

• Behavior on lambdas, currently it just keeps the old behaviour. I believe they should look more like functions. Do you think it is necessary for first patch?

Agree. Not necessary in first patch IMO. Add a TODO though.

• Template classes/functions, what we have in definition is not great but I don't think it is too much of a problem since semantic representation carries all the useful information.

These look OK to me, though examples are complicated and I might be missing something.

• Declared in #SOME_KIND# #SOME_PLACE#, currently we drop SOME_KIND and it needs additional info to be propogated since it is not the type of the symbol but rather it is immediate container. I don't think it is necessary for initial version.

This seems OK to me. As noted you can keep it in the case that it's "namespace". If you want to propagate the container kind in this patch that's also fine.

• Discrepencies between hovering over auto/decltype and explicitly spelled types, again this keeps the existing behaviour. You can see details in testcases.

Testcases look pretty good I think.

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

565

namespace?!

might be a clang-format bug?

572

using the constructor/aggregate init here has a couple of downsides:

• vebosity: you need to list every field, even those that are not set for this case (TemplateParameters), or are misleading (SymRange is none)
• brittleness: it makes it hard to change the struct at all
• readability: the /*foo=*/ comments aren't bad, but not ideal either

Using field-by-field initialization would be better I think, though it's a bit awkward here.

but e.g. you could make the member std::function<HoverInfo> ExpectedBuilder, and write

[&](HoverInfo &Expected) {
  Expected.Name = "foo";
  Expected.Kind = SymbolKind::Function;
  ...
}

590

I think this should be "ns1::ns2::", as we use scope internally.
This means we can simply concatenate parts to form a qname.

For the current rendering, it's easy to strip ::

597

It would be nice to add void() or void (&)() or so if it's easy.
This is what :YcmCompleter GetType would show

605

note that if you want to it's easy to keep "Declared in namespace" - the LocalScope is empty.
(Though we can't provide this info for non-namespace containers)

676

can we have a class template example where the instantiation isn't templated?
e.g.

template<typename T> class vector{};
[[vector]]<int> X;

(just because this is an example that we've discussed a lot, and these edge-case tests make it hard to see the common behavior)

780

is it easy to omit the body?

[kadircet]

• Address comments

[kadircet]

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

565

I must have messed the brackets at some point :D

572

yeah that looks a lot better, thanks!

590

should it also be "::" for global namespace ? which would also result in prefixing any symbol in global namespace with "::" when printing.

597

just put the type without any parameter names, but I am not sure whether users would want that. I believe people find current hover behavior a lot more useful then just showing type(which is done by libclang)

605

I believe it doesn't make sense to add only one, so leaving it until we hear from users that they need the kind information

780

yes, sent out D62487

[sammccall]

Still LG

I forgot to mention - it would be nice to clang-format the definition, what do you think?

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

590

We tend to use empty string for global namespace, and explicitly add it where we need it.
Using :: for global causes as many problems as it solves (e.g. in C).

597

I don't think we should include it in the actual hover, but we can handle that in rendering (if it's a Function, don't print the type).
It does seem odd not to include it in the structured API, but up to you.

I thought specifically the :GetType might still be useful, but maybe not.

[kadircet]

• Address comments

[kadircet]

• clang-format Definition

clang-tools-extra/clangd/ClangdLSPServer.cpp

void ClangdLSPServer::onDocumentHighlight(
     Callback<std::vector<DocumentHighlight>> Reply) {
   Server->findDocumentHighlights(Params.textDocument.uri.file(),
                                  Params.position, std::move(Reply));
 }
 
 void ClangdLSPServer::onHover(const TextDocumentPositionParams &Params,
                               Callback<llvm::Optional<Hover>> Reply) {
   Server->findHover(Params.textDocument.uri.file(), Params.position,
-                    std::move(Reply));
+                    Bind(
+                        [](decltype(Reply) Reply,
+                           llvm::Expected<llvm::Optional<HoverInfo>> HIorErr) {
+                          if (!HIorErr)
+                            return Reply(HIorErr.takeError());
+                          const auto &HI = HIorErr.get();
+                          if (!HI)
+                            return Reply(llvm::None);
+                          return Reply(HI->render());
+                        },
+                        std::move(Reply)));
 }
 
 void ClangdLSPServer::onTypeHierarchy(
     const TypeHierarchyParams &Params,
     Callback<Optional<TypeHierarchyItem>> Reply) {
   Server->typeHierarchy(Params.textDocument.uri.file(), Params.position,
                         Params.resolve, Params.direction, std::move(Reply));
 }

clang-tools-extra/clangd/ClangdServer.h

public:
   llvm::Optional<Path> switchSourceHeader(PathRef Path);
 
   /// Get document highlights for a given position.
   void findDocumentHighlights(PathRef File, Position Pos,
                               Callback<std::vector<DocumentHighlight>> CB);
 
   /// Get code hover for a given position.
   void findHover(PathRef File, Position Pos,
-                 Callback<llvm::Optional<Hover>> CB);
+                 Callback<llvm::Optional<HoverInfo>> CB);
 
   /// Get information about type hierarchy for a given position.
   void typeHierarchy(PathRef File, Position Pos, int Resolve,
                      TypeHierarchyDirection Direction,
                      Callback<llvm::Optional<TypeHierarchyItem>> CB);
 
   /// Retrieve the top symbols from the workspace matching a query.
   void workspaceSymbols(StringRef Query, int Limit,

clang-tools-extra/clangd/ClangdServer.cpp

if (!InpAST)
       return CB(InpAST.takeError());
     CB(clangd::findDocumentHighlights(InpAST->AST, Pos));
   };
 
   WorkScheduler.runWithAST("Highlights", File, Bind(Action, std::move(CB)));
 }
 
 void ClangdServer::findHover(PathRef File, Position Pos,
-                             Callback<llvm::Optional<Hover>> CB) {
-  auto Action = [Pos](Callback<llvm::Optional<Hover>> CB,
+                             Callback<llvm::Optional<HoverInfo>> CB) {
+  auto Action = [Pos](Callback<llvm::Optional<HoverInfo>> CB,
                       llvm::Expected<InputsAndAST> InpAST) {
     if (!InpAST)
       return CB(InpAST.takeError());
     CB(clangd::getHover(InpAST->AST, Pos));
   };
 
   WorkScheduler.runWithAST("Hover", File, Bind(Action, std::move(CB)));
 }

clang-tools-extra/clangd/XRefs.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_XREFS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_XREFS_H
 
 #include "ClangdUnit.h"
 #include "Protocol.h"
 #include "index/Index.h"
+#include "index/SymbolLocation.h"
+#include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/Support/raw_ostream.h"
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
 // Describes where a symbol is declared and defined (as far as clangd knows).
 // There are three cases:
 //  - a declaration only, no definition is known (e.g. only header seen)

 /// Multiple locations may be returned, corresponding to distinct symbols.
 std::vector<LocatedSymbol> locateSymbolAt(ParsedAST &AST, Position Pos,
                                           const SymbolIndex *Index = nullptr);
 
 /// Returns highlights for all usages of a symbol at \p Pos.
 std::vector<DocumentHighlight> findDocumentHighlights(ParsedAST &AST,
                                                       Position Pos);
 
+struct HoverInfo {
+  using Type = std::string;
+  struct Param {
+    // For template template params it holds the whole template decl, e.g,
+    // "template<typename> class".
+    // For type-template params will contain "typename" or "class".
+    Type T;
+    std::string Name;
+    std::string Default;
+  };
+
+  LocatedSymbol Sym;
+  /// Fully qualiffied name for the scope containing the Sym.
+  std::string Scope;
+  std::string ParentScope;
+  SymbolKind Kind;
+  std::string Documentation;
+  /// Line containing the definition of the symbol.
+  std::string Definition;
+
+  /// T and ReturnType are mutually exclusive.
+  llvm::Optional<Type> T;
+  llvm::Optional<Type> ReturnType;
+  llvm::Optional<std::vector<Param>> Parameters;
+  llvm::Optional<std::vector<Param>> TemplateParameters;
+
+  /// Lower to LSP struct.
+  Hover render() const;
+};
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const HoverInfo::Param &);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const HoverInfo &);
+
 /// Get the hover information when hovering at \p Pos.
-llvm::Optional<Hover> getHover(ParsedAST &AST, Position Pos);
+llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos);
 
 /// Returns reference locations of the symbol at a specified \p Pos.
 /// \p Limit limits the number of results returned (0 means no limit).
 std::vector<Location> findReferences(ParsedAST &AST, Position Pos,
                                      uint32_t Limit,
                                      const SymbolIndex *Index = nullptr);
 
 /// Get info about symbols at \p Pos.
 std::vector<SymbolDetails> getSymbolInfo(ParsedAST &AST, Position Pos);
 
 /// Find the record type references at \p Pos.
 const CXXRecordDecl *findRecordTypeAt(ParsedAST &AST, Position Pos);
 
 /// Given a record type declaration, find its base (parent) types.
 std::vector<const CXXRecordDecl *> typeParents(const CXXRecordDecl *CXXRD);
 

clang-tools-extra/clangd/XRefs.cpp

 //===--- XRefs.cpp -----------------------------------------------*- C++-*-===//
 //
 // 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 "XRefs.h"
 #include "AST.h"
+#include "CodeCompletionStrings.h"
 #include "FindSymbols.h"
 #include "Logger.h"
+#include "Protocol.h"
 #include "SourceCode.h"
 #include "URI.h"
 #include "index/Merge.h"
 #include "index/SymbolCollector.h"
 #include "index/SymbolLocation.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Type.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Index/USRGeneration.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // Returns the single definition of the entity declared by D, if visible.
 // In particular:
 // - for non-redeclarable kinds (e.g. local vars), return D
IdentifiedSymbol getSymbolAtPosition(ParsedAST &AST, SourceLocation Pos) {
   IndexOpts.IndexParametersInDeclarations = true;
   IndexOpts.IndexTemplateParameters = true;
   indexTopLevelDecls(AST.getASTContext(), AST.getPreprocessor(),
                      AST.getLocalTopLevelDecls(), DeclMacrosFinder, IndexOpts);
 
   return {DeclMacrosFinder.getFoundDecls(), DeclMacrosFinder.takeMacroInfos()};
 }
 
-Range getTokenRange(ParsedAST &AST, SourceLocation TokLoc) {
-  const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
-  SourceLocation LocEnd = Lexer::getLocForEndOfToken(
-      TokLoc, 0, SourceMgr, AST.getASTContext().getLangOpts());
+Range getTokenRange(ASTContext &AST, SourceLocation TokLoc) {
+  const SourceManager &SourceMgr = AST.getSourceManager();
+  SourceLocation LocEnd =
+      Lexer::getLocForEndOfToken(TokLoc, 0, SourceMgr, AST.getLangOpts());
   return {sourceLocToPosition(SourceMgr, TokLoc),
           sourceLocToPosition(SourceMgr, LocEnd)};
 }
 
-llvm::Optional<Location> makeLocation(ParsedAST &AST, SourceLocation TokLoc,
+llvm::Optional<Location> makeLocation(ASTContext &AST, SourceLocation TokLoc,
                                       llvm::StringRef TUPath) {
-  const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+  const SourceManager &SourceMgr = AST.getSourceManager();
   const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(TokLoc));
   if (!F)
     return None;
   auto FilePath = getCanonicalPath(F, SourceMgr);
   if (!FilePath) {
     log("failed to get path!");
     return None;
   }
std::vector<LocatedSymbol> locateSymbolAt(ParsedAST &AST, Position Pos,
   SourceLocation SourceLocationBeg =
       getBeginningOfIdentifier(AST, Pos, SM.getMainFileID());
   auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
 
   // Macros are simple: there's no declaration/definition distinction.
   // As a consequence, there's no need to look them up in the index either.
   std::vector<LocatedSymbol> Result;
   for (auto M : Symbols.Macros) {
-    if (auto Loc =
-            makeLocation(AST, M.Info->getDefinitionLoc(), *MainFilePath)) {
+    if (auto Loc = makeLocation(AST.getASTContext(), M.Info->getDefinitionLoc(),
+                                *MainFilePath)) {
       LocatedSymbol Macro;
       Macro.Name = M.Name;
       Macro.PreferredDeclaration = *Loc;
       Macro.Definition = Loc;
       Result.push_back(std::move(Macro));
     }
   }
 
   // Decls are more complicated.
   // The AST contains at least a declaration, maybe a definition.
   // These are up-to-date, and so generally preferred over index results.
   // We perform a single batch index lookup to find additional definitions.
 
   // Results follow the order of Symbols.Decls.
   // Keep track of SymbolID -> index mapping, to fill in index data later.
   llvm::DenseMap<SymbolID, size_t> ResultIndex;
 
   // Emit all symbol locations (declaration or definition) from AST.
   for (const Decl *D : Symbols.Decls) {
-    auto Loc = makeLocation(AST, findNameLoc(D), *MainFilePath);
+    auto Loc = makeLocation(AST.getASTContext(), findNameLoc(D), *MainFilePath);
     if (!Loc)
       continue;
 
     Result.emplace_back();
     if (auto *ND = dyn_cast<NamedDecl>(D))
       Result.back().Name = printName(AST.getASTContext(), *ND);
     Result.back().PreferredDeclaration = *Loc;
     // DeclInfo.D is always a definition if possible, so this check works.
findRefs(const std::vector<const Decl *> &Decls, ParsedAST &AST) {
   IndexOpts.IndexFunctionLocals = true;
   IndexOpts.IndexParametersInDeclarations = true;
   IndexOpts.IndexTemplateParameters = true;
   indexTopLevelDecls(AST.getASTContext(), AST.getPreprocessor(),
                      AST.getLocalTopLevelDecls(), RefFinder, IndexOpts);
   return std::move(RefFinder).take();
 }
 
 } // namespace
 
 std::vector<DocumentHighlight> findDocumentHighlights(ParsedAST &AST,
                                                       Position Pos) {
   const SourceManager &SM = AST.getASTContext().getSourceManager();
   auto Symbols = getSymbolAtPosition(
       AST, getBeginningOfIdentifier(AST, Pos, SM.getMainFileID()));
   auto References = findRefs(Symbols.Decls, AST);
 
   std::vector<DocumentHighlight> Result;
   for (const auto &Ref : References) {
     DocumentHighlight DH;
-    DH.range = getTokenRange(AST, Ref.Loc);
+    DH.range = getTokenRange(AST.getASTContext(), Ref.Loc);
     if (Ref.Role & index::SymbolRoleSet(index::SymbolRole::Write))
       DH.kind = DocumentHighlightKind::Write;
     else if (Ref.Role & index::SymbolRoleSet(index::SymbolRole::Read))
       DH.kind = DocumentHighlightKind::Read;
     else
       DH.kind = DocumentHighlightKind::Text;
     Result.push_back(std::move(DH));
   }
static llvm::Optional<std::string> getScopeName(const Decl *D) {
   else if (const NamespaceDecl *ND = dyn_cast<NamespaceDecl>(DC))
     return namedDeclQualifiedName(ND, "namespace");
   else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(DC))
     return namedDeclQualifiedName(FD, "function");
 
   return None;
 }
 
-/// Generate a \p Hover object given the declaration \p D.
-static Hover getHoverContents(const Decl *D) {
-  Hover H;
-  llvm::Optional<std::string> NamedScope = getScopeName(D);
+static QualType getDeclType(const Decl *D) {
+  if (const TypedefNameDecl *TDD = dyn_cast<TypedefNameDecl>(D))
+    return TDD->getUnderlyingType();
+  if (const ValueDecl *VD = dyn_cast<ValueDecl>(D))
+    return VD->getType();
+  if (const TagDecl *TD = dyn_cast<TagDecl>(D))
+    return TD->getTypeForDecl()->getCanonicalTypeInternal();
+  return QualType();
+}
 
-  // Generate the "Declared in" section.
-  if (NamedScope) {
-    assert(!NamedScope->empty());
+static llvm::Optional<Location> getDeclLoc(const SourceLocation &SL,
+                                           ASTContext &AST) {
+  if (SL.isInvalid())
+    return llvm::None;
+  const auto &SM = AST.getSourceManager();
+  auto MainFilePath =
+      getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
+  if (!MainFilePath) {
+    elog("Couldn't get main file path for: {0}", SL.printToString(SM));
+    return llvm::None;
+  }
+  return makeLocation(AST, SL, *MainFilePath);
+}
 
-    H.contents.value += "Declared in ";
-    H.contents.value += *NamedScope;
-    H.contents.value += "\n\n";
-  }
+static std::string getDefinitionLine(const Decl *D) {
+  std::string Definition;
+  {
+    llvm::raw_string_ostream OS(Definition);
+    PrintingPolicy Policy =
+        printingPolicyForDecls(D->getASTContext().getPrintingPolicy());
+    D->print(OS, Policy);
+  }
+  return Definition;
 
-  // We want to include the template in the Hover.
-  if (TemplateDecl *TD = D->getDescribedTemplate())
-    D = TD;
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
+                              const std::vector<HoverInfo::Param> &Params) {
+  for (size_t I = 0, E = Params.size(); I != E; ++I) {
+    if (I)
+      OS << ", ";
+    OS << Params.at(I);
+  }
+  return OS;
+}
+
+static std::vector<HoverInfo::Param>
+fetchTemplateParameters(const TemplateParameterList *Params,
+                        const PrintingPolicy &PP) {
+  assert(Params);
+  std::vector<HoverInfo::Param> TempParameters;
+
+  for (const Decl *Param : *Params) {
+    HoverInfo::Param P;
+    if (const auto TTP = dyn_cast<TemplateTypeParmDecl>(Param)) {
+      P.T = TTP->wasDeclaredWithTypename() ? "typename" : "class";
+      if (TTP->isParameterPack())
+        P.T += "...";
+
+      llvm::raw_string_ostream Out(P.Name);
+      Out << *TTP;
+      if (TTP->hasDefaultArgument())
+        P.Default = TTP->getDefaultArgument().getAsString(PP);
+    } else if (const auto NTTP = dyn_cast<NonTypeTemplateParmDecl>(Param)) {
+      if (IdentifierInfo *II = NTTP->getIdentifier())
+        P.Name = II->getName().str();
+
+      llvm::raw_string_ostream Out(P.T);
+      NTTP->getType().print(Out, PP);
+      if (NTTP->isParameterPack())
+        Out << "...";
+
+      if (NTTP->hasDefaultArgument()) {
+        llvm::raw_string_ostream Out(P.Default);
+        NTTP->getDefaultArgument()->printPretty(Out, nullptr, PP);
+      }
+    } else if (const auto TTPD = dyn_cast<TemplateTemplateParmDecl>(Param)) {
+      llvm::raw_string_ostream OS(P.T);
+      OS << "template <"
+         << fetchTemplateParameters(
+                TTPD->getDescribedTemplate()->getTemplateParameters(), PP)
+         << "> class"; // FIXME: TemplateTemplateParameter doesn't store the
+                       // info on whether this param was a "typename" or
+                       // "class".
+      if (TTPD->hasDefaultArgument()) {
+        llvm::raw_string_ostream Out(P.Default);
+        TTPD->getDefaultArgument().getArgument().print(PP, Out);
+      }
+    } else {
+      std::string ParamSerial;
+      llvm::raw_string_ostream Out(ParamSerial);
+      Param->print(Out, PP);
+      Out.flush();
+      vlog("Unhandled template param: {0}", ParamSerial);
+      continue;
+    }
+    TempParameters.push_back(std::move(P));
+  }
 
-  std::string DeclText;
-  llvm::raw_string_ostream OS(DeclText);
+  return TempParameters;
+}
+
+/// Generate a \p Hover object given the declaration \p D.
+static HoverInfo getHoverContents(const Decl *D) {
+  HoverInfo HI;
 
   PrintingPolicy Policy =
       printingPolicyForDecls(D->getASTContext().getPrintingPolicy());
+  if (const NamedDecl *ND = llvm::dyn_cast<NamedDecl>(D)) {
+    HI.Documentation = getDeclComment(ND->getASTContext(), *ND);
+    HI.Sym.Name = printName(D->getASTContext(), *ND);
+    HI.Scope = printQualifiedName(*ND);
+  }
 
-  D->print(OS, Policy);
+  if (llvm::Optional<std::string> NamedScope = getScopeName(D)) {
+    assert(!NamedScope->empty());
+    HI.ParentScope = std::move(*NamedScope);
+  }
+  HI.Kind = indexSymbolKindToSymbolKind(index::getSymbolInfo(D).Kind);
+
+  if (const TemplateDecl *TD = D->getDescribedTemplate()) {
+    HI.TemplateParameters =
+        fetchTemplateParameters(TD->getTemplateParameters(), Policy);
+    D = TD;
+  } else if (const FunctionDecl *FD = D->getAsFunction()) {
+    if (const auto FTD = FD->getDescribedTemplate()) {
+      HI.TemplateParameters =
+          fetchTemplateParameters(FTD->getTemplateParameters(), Policy);
+      D = FTD;
+    }
+  }
 
-  OS.flush();
+  if (const FunctionDecl *FD = D->getAsFunction()) {
+    HI.ReturnType.emplace();
+    llvm::raw_string_ostream OS(*HI.ReturnType);
+    FD->getReturnType().print(OS, Policy);
+
+    HI.Parameters.emplace();
+    for (const ParmVarDecl *PVD : FD->parameters()) {
+      HI.Parameters->emplace_back();
+      auto &P = HI.Parameters->back();
+      P.T = PVD->getNameAsString();
+      if (PVD->hasDefaultArg()) {
+        llvm::raw_string_ostream Out(P.Default);
+        PVD->getDefaultArg()->printPretty(Out, nullptr, Policy);
+      }
+    }
+    if (FD->isVariadic()) {
+      HI.Parameters->emplace_back();
+      HI.Parameters->back().T = "...";
+    }
+  } else {
+    auto QT = getDeclType(D);
+    if (!QT.isNull()) {
+      HI.T.emplace();
+      llvm::raw_string_ostream OS(*HI.T);
+      QT.print(OS, std::move(Policy));
+    }
+  }
 
-  H.contents.value += DeclText;
-  return H;
+  if (auto DefLoc = getDeclLoc(findNameLoc(D), D->getASTContext())) {
+    HI.Sym.PreferredDeclaration = *DefLoc;
+    if (getDefinition(D) == D)
+      HI.Sym.Definition = *DefLoc;
+  }
+  HI.Definition = getDefinitionLine(D);
+  return HI;
 }
 
 /// Generate a \p Hover object given the type \p T.
-static Hover getHoverContents(QualType T, ASTContext &ASTCtx) {
-  Hover H;
-  std::string TypeText;
-  llvm::raw_string_ostream OS(TypeText);
+static HoverInfo getHoverContents(QualType T, ASTContext &ASTCtx) {
+  HoverInfo HI;
+  {
+    HI.T.emplace();
+    llvm::raw_string_ostream OS(*HI.T);
     PrintingPolicy Policy = printingPolicyForDecls(ASTCtx.getPrintingPolicy());
     T.print(OS, Policy);
-  OS.flush();
-  H.contents.value += TypeText;
-  return H;
+  }
+  HI.Sym.Name = *HI.T;
+  if (const TagDecl *TD = T.getTypePtr()->getAsTagDecl()) {
+    if (auto ScopedName = getScopeName(TD))
+      HI.ParentScope = std::move(*ScopedName);
+    HI.Scope = printQualifiedName(*TD);
+    HI.Kind = indexSymbolKindToSymbolKind(index::getSymbolInfo(TD).Kind);
+    // FIXME: Populate documentation
+    if (auto DefLoc = getDeclLoc(findNameLoc(TD), TD->getASTContext())) {
+      HI.Sym.PreferredDeclaration = *DefLoc;
+      if (getDefinition(TD) == TD)
+        HI.Sym.Definition = *DefLoc;
+    }
+    HI.Definition = getDefinitionLine(TD);
+  }
+  return HI;
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
-static Hover getHoverContents(MacroDecl Decl, ParsedAST &AST) {
+static HoverInfo getHoverContents(MacroDecl Decl, ParsedAST &AST) {
+  HoverInfo HI;
   SourceManager &SM = AST.getASTContext().getSourceManager();
-  std::string Definition = Decl.Name;
+  HI.Sym.Name = Decl.Name;
+  HI.Kind = indexSymbolKindToSymbolKind(
+      index::getSymbolInfoForMacro(*Decl.Info).Kind);
+  // FIXME: Populate documentation
 
   // Try to get the full definition, not just the name
   SourceLocation StartLoc = Decl.Info->getDefinitionLoc();
   SourceLocation EndLoc = Decl.Info->getDefinitionEndLoc();
   if (EndLoc.isValid()) {
     EndLoc = Lexer::getLocForEndOfToken(EndLoc, 0, SM,
                                         AST.getASTContext().getLangOpts());
     bool Invalid;
     StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), &Invalid);
     if (!Invalid) {
       unsigned StartOffset = SM.getFileOffset(StartLoc);
       unsigned EndOffset = SM.getFileOffset(EndLoc);
       if (EndOffset <= Buffer.size() && StartOffset < EndOffset)
-        Definition = Buffer.substr(StartOffset, EndOffset - StartOffset).str();
+        HI.Definition =
+            Buffer.substr(StartOffset, EndOffset - StartOffset).str();
     }
   }
-
-  Hover H;
-  H.contents.kind = MarkupKind::PlainText;
-  H.contents.value = "#define " + Definition;
-  return H;
+  if (auto DefLoc = getDeclLoc(StartLoc, AST.getASTContext())) {
+    HI.Sym.PreferredDeclaration = *DefLoc;
+    HI.Sym.Definition = *DefLoc;
+  }
+  return HI;
 }
 
 namespace {
 /// Computes the deduced type at a given location by visiting the relevant
 /// nodes. We use this to display the actual type when hovering over an "auto"
 /// keyword or "decltype()" expression.
 /// FIXME: This could have been a lot simpler by visiting AutoTypeLocs but it
 /// seems that the AutoTypeLocs that can be visited along with their AutoType do
llvm::Optional<QualType> getDeducedType(ParsedAST &AST,
   if (!(Tok.is(tok::kw_auto) || Tok.is(tok::kw_decltype)))
     return {};
 
   DeducedTypeVisitor V(SourceLocationBeg);
   V.TraverseAST(AST.getASTContext());
   return V.getDeducedType();
 }
 
-llvm::Optional<Hover> getHover(ParsedAST &AST, Position Pos) {
+llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
   SourceLocation SourceLocationBeg =
       getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
   // Identified symbols at a specific position.
   auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
 
   if (!Symbols.Macros.empty())
     return getHoverContents(Symbols.Macros[0], AST);
std::vector<Location> findReferences(ParsedAST &AST, Position Pos,
   auto Loc = getBeginningOfIdentifier(AST, Pos, SM.getMainFileID());
   auto Symbols = getSymbolAtPosition(AST, Loc);
 
   // We traverse the AST to find references in the main file.
   // TODO: should we handle macros, too?
   auto MainFileRefs = findRefs(Symbols.Decls, AST);
   for (const auto &Ref : MainFileRefs) {
     Location Result;
-    Result.range = getTokenRange(AST, Ref.Loc);
+    Result.range = getTokenRange(AST.getASTContext(), Ref.Loc);
     Result.uri = URIForFile::canonicalize(*MainFilePath, *MainFilePath);
     Results.push_back(std::move(Result));
   }
 
   // Now query the index for references from other files.
   if (Index && Results.size() < Limit) {
     RefsRequest Req;
     Req.Limit = Limit;
Optional<TypeHierarchyItem> Result =
       getTypeAncestors(*CXXRD, AST.getASTContext(), RPSet);
 
   // FIXME(nridge): Resolve type descendants if direction is Children or Both,
   // and ResolveLevels > 0.
 
   return Result;
 }
 
+Hover HoverInfo::render() const {
+  Hover H;
+  H.range = Sym.PreferredDeclaration.range;
+  H.contents.kind = MarkupKind::PlainText;
+  std::vector<std::string> Output;
+
+  if (!Scope.empty())
+    H.contents.value = llvm::formatv("Declared in {0}\n\n", Scope);
+
+  if (Kind == SymbolKind::String) {
+    // We use SymbolKind::String for macro kinds
+    Output.push_back("#define");
+    if (!Definition.empty())
+      Output.push_back(Definition);
+    else
+      Output.push_back(Sym.Name);
+  } else if (T || ReturnType) {
+    // Non-macros
+    if (TemplateParameters) {
+      Output.emplace_back();
+      llvm::raw_string_ostream OS(Output.back());
+      OS << "template <" << *TemplateParameters << '>';
+    }
+    if (T)
+      Output.push_back(*T);
+    else if (ReturnType)
+      Output.push_back(*ReturnType);
+
+    if (!Sym.Name.empty())
+      if (!T || !llvm::StringRef(*T).endswith(Sym.Name))
+        Output.push_back(Sym.Name);
+
+    if (Parameters) {
+      // We don't want space before parameters.
+      llvm::raw_string_ostream OS(Output.back());
+      OS << '(' << *Parameters << ')';
+    }
+  } else {
+    // Symbols without types, like namespaces.
+    Output.push_back(Definition);
+  }
+  H.contents.value += llvm::join(Output, " ");
+  return H;
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
+                              const HoverInfo::Param &P) {
+  OS << P.T << ' ' << P.Name;
+  if (!P.Default.empty())
+    OS << " = " << P.Default;
+  return OS;
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const HoverInfo &HI) {
+  OS << HI.Definition << '\n';
+  OS << HI.Documentation << '\n';
+  OS << HI.Parameters << '\n';
+  OS << HI.ReturnType << '\n';
+  OS << HI.Scope << '\n';
+  OS << HI.Sym << '\n';
+  OS << HI.T << '\n';
+  OS << HI.TemplateParameters << '\n';
+  return OS;
+}
+
 } // namespace clangd
 } // namespace clang

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

EXPECT_THAT(*Locations,
               ElementsAre(Sym("bar_not_preamble",
                               HeaderNotInPreambleAnnotations.range())));
 }
 
 TEST(Hover, All) {
   struct OneTest {
     StringRef Input;
     StringRef ExpectedHover;
   };
 
   OneTest Tests[] = {
       {
           R"cpp(// No hover
             ^int main() {
             }
           )cpp",
           "",
       },
       {
           R"cpp(// Local variable
             int main() {
               int bonjour;
               ^bonjour = 2;
               int test1 = bonjour;
             }
           )cpp",
           "Declared in function main\n\nint bonjour",
       },
       {
           R"cpp(// Local variable in method
             struct s {
               void method() {
                 int bonjour;
                 ^bonjour = 2;
               }
             };
           )cpp",
           "Declared in function s::method\n\nint bonjour",
       },
       {
           R"cpp(// Struct
             namespace ns1 {
               struct MyClass {};
             } // namespace ns1
             int main() {
               ns1::My^Class* Params;
             }
           )cpp",
           "Declared in namespace ns1\n\nstruct MyClass {}",
       },
       {
           R"cpp(// Class
             namespace ns1 {
               class MyClass {};
             } // namespace ns1
             int main() {
               ns1::My^Class* Params;
OneTest Tests[] = {
           R"cpp(// Static field
             struct Foo { static int x; };
             int main() {
               Foo::^x;
             }
           )cpp",
           "Declared in struct Foo\n\nstatic int x",
       },
       {
           R"cpp(// Field, member initializer
             struct Foo {
               int x;
               Foo() : ^x(0) {}
             };
           )cpp",
           "Declared in struct Foo\n\nint x",
       },
OneTest Tests[] = {
             }
             int main() ^MACRO
           )cpp",
           R"cpp(#define MACRO {\
               return 0;\
             })cpp",
       },
       {
           R"cpp(// Forward class declaration
             class Foo;
             class Foo {};
             F^oo* foo();
           )cpp",
           "Declared in global namespace\n\nclass Foo {}",
       },
       {
           R"cpp(// Function declaration
OneTest Tests[] = {
             {
               template<class _E>
               class initializer_list {};
             }
             void foo() {
               ^auto i = {1,2};
             }
           )cpp",
           "class std::initializer_list<int>",
       },
       {
           R"cpp(// User defined conversion to auto
             struct Bar {
               operator ^auto() const { return 10; }
             };
           )cpp",
           "int",
TEST(Hover, All) {
 
   for (const OneTest &Test : Tests) {
     Annotations T(Test.Input);
     TestTU TU = TestTU::withCode(T.code());
     TU.ExtraArgs.push_back("-std=c++17");
     auto AST = TU.build();
     if (auto H = getHover(AST, T.point())) {
       EXPECT_NE("", Test.ExpectedHover) << Test.Input;
-      EXPECT_EQ(H->contents.value, Test.ExpectedHover.str()) << Test.Input;
+      EXPECT_EQ(H->render().contents.value, Test.ExpectedHover.str())
+          << Test.Input << '\n'
+          << H;
     } else
       EXPECT_EQ("", Test.ExpectedHover.str()) << Test.Input;
   }
 }
 
 TEST(GoToInclude, All) {
   MockFSProvider FS;
   IgnoreDiagnostics DiagConsumer;