[clangd] Add more symbol information for code completion.

[sammccall]

clangd/index/Index.h

96

AFAIK this information isn't needed for retrieval/scoring, just for display.

LSP has completionItem/resolve that adds additional info to a completion item. This allows us to avoid sending a bunch of bulky comments, most of which will never be looked at.

In practice, there's nothing we particularly want to do differently for the memory index: we have to load the data into memory, and so including a pointer to it right away is no extra work.

However Symbol has two roles, and being the in-memory representation for MemIndex is only secondary. Its primary role is defining the protocol between indexes and clangd, including remote indexes where returning all documentation *is* expensive.

One option is to have Symbol just have the "core stuff" that's suitable for returning in bulk, and have an index method to retrieve more details that would be a point lookup only. (Maybe this is just the getSymbol method we've thought about). I'm not sure what it means for the data structure. OK if we discuss offline?

103

What are you planning to put here other than the return type of a function?
It's probably OK if we explicitly want this to be "whatever should go in the LSP detail field" but we should think about whether there's something more specific we can say.

104

How are you planning to use this?

This seems to be related to the completion text/edits. We had some early discussions about whether we'd encode something like CodeCompletionString, or LSP snippets, or something else entirely.
Honestly it would be great to have a doc describing this mapping between source -> index -> LSP for completion data.

clangd/index/SymbolCollector.cpp

62

it seems we'll want to share the(some?) doc logic between hover, AST-based complete, and indexing... See D35894 (which is ... complicated, no idea if it'll land soon).

Among other things:

• we may not want to make the logic too elaborate until we're able to merge interfaces
• we may want to consume AST nodes rather than CCS in the long run

68

Should these annotations go at the end?

91

nit: lowercase

91

How does this relate to the code in AST-based completion?

[ioeric]

• Merge with origin/master
• Fixed an error in merge
• Make documentation etc optional in symbols
• Merge remote-tracking branch 'origin/master' into symbol
• Merge branch 'index-completion' into symbol
• Address review comments; merge with D41450.

[ioeric]

Thanks for the review!

Logic around CodeCompletionString is pulled into a separate file in D41450.

I also propagate the new completion info to completion code. I am happy to split it out if it adds too much noise for the review.

clangd/index/Index.h

96

As discussed offline, putting non-core stuff in an optional structure.

104

As discussed offline, we now store the whole snippet in the insertion text.

clangd/index/SymbolCollector.cpp

62

I pulled CodeCompletionString handling logic into a separate file in D41450.

[sammccall]

clangd/index/Index.h

105

insert texts can be in details I think? They're not required for completion until after resolve.

122

I think you probably want a raw pointer rather than optional:

• reduce the size of the struct when it's absent
• make it inheritance-friendly so we can hang index-specific info off it

(raw pointer rather than unique_ptr because it's owned by a slab not by malloc, but unique_ptr is ok for now)

clangd/index/SymbolCollector.cpp

65

this doesn't seem like the kind of thing we should be allocating per-symbol!
You can use a single one and Reset() it, I think

also why globalcodecompletionallocator, vs just codecompletionallocator?

[ioeric]

• Merged with origin/master
• Addressed some more comments.
• Add new fields to YAML.

clangd/index/Index.h

105

Quote from https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#completion-request

The request can delay the computation od the detail and documentation properties. However, properties that are needed for the inital sorting and filtering, like sortText, filterText, insertText, and textEdit must be provided in the textDocument/completion request and must not be changed during resolve.

122

This is not easy for now with unique_ptr because of this line :( https://github.com/llvm-mirror/clang-tools-extra/blob/3565d1a1a692fc9f5c21e634b470535da2bb4d25/clangd/index/SymbolYAML.cpp#L141).

This shouldn't be an issue when we have the optimized symbol slab, where we store raw pointers. And we would probably want to serialize the whole slab instead of the individual symbols anyway.

reduce the size of the struct when it's absent

llvm::Optional doesn't take much more space, so the size should be fine.

make it inheritance-friendly so we can hang index-specific info off it

Could you elaborate on index-specific info? It's unclear to me how this would be used.

clangd/index/SymbolCollector.cpp

65

You can use a single one and Reset() it, I think

It's unclear how reset would affect CodeCompletionTUInfo, but we can simply maintain one allocator for each TU.

also why globalcodecompletionallocator, vs just codecompletionallocator?

They are actually the same thing, but CodeCompletionTUInfo requires a global one. I don't really know why...

[sammccall]

clangd/index/Index.h

122

This is not easy for now with unique_ptr because of this line

Oh no, somehow i missed this during review.
We shouldn't be relying on symbols being copyable. I'll try to work out how to fix this and delete the copy constructor.

This shouldn't be an issue when we have the optimized symbol slab, where we store raw pointers.

Sure. That's not a big monolithic/mysterous thing though, storing the details in the slab can be done in this patch... If you think it'll be easier once strings are arena-based, then maybe we should delay this patch until that's done, rather than make that work bigger.

And we would probably want to serialize the whole slab instead of the individual symbols anyway.

This i'm less sure about, but I don't think it matters.

llvm::Optional doesn't take much more space, so the size should be fine.

Optional takes the same size as the details itself (plus one bool). This is fairly small for now, but I think a major point of Details is to expand it in the future?

Could you elaborate on index-specific info? It's unclear to me how this would be used.

Yeah, this is something we talked about in the meeting with Marc-Andre but it's not really obvious - what's the point of allowing Details to be extended if clangd has to consume it?

It sounded like he might have use cases for using index infrastructure outside clangd. We might also have google-internal index features we want (matching generated code to proto fields?). I'm not really sure how compelling this argument is.

clangd/index/SymbolCollector.h

22

can you add a comment to the class indicating that it needs to be used for one TU and then thrown away? This seems unfortunate but is probably simpler than the alternative. It also seems to be a new restriction with this patch.

[ioeric]

• Merge with origin/master. Use Arena for symbol details.

[ioeric]

clangd/index/Index.h

122

Thanks for the allocator change!

Details now contains just stringrefs. I wonder how much we want it to be inherit-friendly at this point, as the size is relatively small now. If you think this is a better way to go, I'll make the structure contain strings and store the whole structure in arena. This would require some tweaks for yamls tho but shouldn't be hard.

clangd/index/SymbolCollector.h

22

How about re-initializing the allocator for each new AST in intialize?

[sammccall]

Just some nits

clangd/index/Index.h

122

So this needs to be at least optional I think - at the moment, how does an API user know whether to fetch the rest of the details?

FWIW, the size difference is already significant: symbol is 136 bytes if this is a unique_ptr, and 164 bytes if it's optional (+20%) - StringRefs are still pretty big.
But I don't feel really strongly about this.

clangd/index/SymbolCollector.cpp

99

you shouldn't initialize these both here and in initialize(), right?

154

why conditional? note we're interning the strings anyway, and an empty stringref is the same size as a full one

[ioeric]

• [clangd] Address review comments; made Detail a pointer.

[ioeric]

• Minor cleanup.

[ioeric]

• Merge branch 'master' of http://llvm.org/git/clang-tools-extra into symbol

clangd/index/Index.h

122

Done. Made it a raw pointer. PTAL

[sammccall]

clangd/index/SymbolYAML.cpp

78 ↗

(On Diff #129075)

this removes the Detail object if it's empty - this seems maybe unneccesary and certainly the wrong layer. It seems enough to do

if (!outputting)
  Detail = (allocate)
else if (!Detail)
  return;

io.mapOptional("Documentation", Detail->Documentation);
// etc

[ioeric]

• Addrress review comment.

clangd/ClangdUnit.h

public:
   ParsedAST &operator=(ParsedAST &&Other);
 
   ~ParsedAST();
 
   ASTContext &getASTContext();
   const ASTContext &getASTContext() const;
 
   Preprocessor &getPreprocessor();
+  std::shared_ptr<Preprocessor> getPreprocessorPtr();
   const Preprocessor &getPreprocessor() const;
 
   /// This function returns all top-level decls, including those that come
   /// from Preamble. Decls, coming from Preamble, have to be deserialized, so
   /// this call might be expensive.
   ArrayRef<const Decl *> getTopLevelDecls();
 
   const std::vector<DiagWithFixIts> &getDiagnostics() const;

clangd/ClangdUnit.cpp

 ASTContext &ParsedAST::getASTContext() { return Clang->getASTContext(); }
 
 const ASTContext &ParsedAST::getASTContext() const {
   return Clang->getASTContext();
 }
 
 Preprocessor &ParsedAST::getPreprocessor() { return Clang->getPreprocessor(); }
 
+std::shared_ptr<Preprocessor> ParsedAST::getPreprocessorPtr() {
+  return Clang->getPreprocessorPtr();
+}
+
 const Preprocessor &ParsedAST::getPreprocessor() const {
   return Clang->getPreprocessor();
 }
 
 ArrayRef<const Decl *> ParsedAST::getTopLevelDecls() {
   ensurePreambleDeclsDeserialized();
   return TopLevelDecls;
 }

clangd/CodeComplete.cpp

 }
 
 CompletionItem indexCompletionItem(const Symbol &Sym, llvm::StringRef Filter,
                                    const SpecifiedScope &SSInfo) {
   CompletionItem Item;
   Item.kind = toCompletionItemKind(Sym.SymInfo.Kind);
   Item.label = Sym.Name;
   // FIXME(ioeric): support inserting/replacing scope qualifiers.
-  Item.insertText = Sym.Name;
+
   // FIXME(ioeric): support snippets.
+  Item.insertText = Sym.CompletionPlainInsertText;
   Item.insertTextFormat = InsertTextFormat::PlainText;
-  Item.filterText = Filter;
+  Item.filterText = Sym.CompletionFilterText;
 
   // FIXME(ioeric): sort symbols appropriately.
   Item.sortText = "";
 
   // FIXME(ioeric): use more symbol information (e.g. documentation, label) to
   // populate the completion item.
+  if (Sym.Detail) {
+    Item.documentation = Sym.Detail->Documentation;
+    Item.detail = Sym.Detail->CompletionDetail;
+  }
 
   return Item;
 }
 
 void completeWithIndex(const Context &Ctx, const SymbolIndex &Index,
                        llvm::StringRef Code, const SpecifiedScope &SSInfo,
                        llvm::StringRef Filter, CompletionList *Items) {
   FuzzyFindRequest Req;

clangd/index/FileIndex.cpp

 #include "clang/Index/IndexingAction.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 /// Retrieves namespace and class level symbols in \p Decls.
 std::unique_ptr<SymbolSlab> indexAST(ASTContext &Ctx,
+                                     std::shared_ptr<Preprocessor> PP,
                                      llvm::ArrayRef<const Decl *> Decls) {
   auto Collector = std::make_shared<SymbolCollector>();
+  Collector->setPreprocessor(std::move(PP));
   index::IndexingOptions IndexOpts;
   IndexOpts.SystemSymbolFilter =
       index::IndexingOptions::SystemSymbolFilterKind::All;
   IndexOpts.IndexFunctionLocals = false;
 
   index::indexTopLevelDecls(Ctx, Decls, Collector, IndexOpts);
   auto Symbols = llvm::make_unique<SymbolSlab>();
   *Symbols = Collector->takeSymbols();
std::shared_ptr<std::vector<const Symbol *>> FileSymbols::allSymbols() {
   // pointers.
   return {std::move(Snap), Pointers};
 }
 
 void FileIndex::update(const Context &Ctx, PathRef Path, ParsedAST *AST) {
   if (!AST) {
     FSymbols.update(Path, nullptr);
   } else {
-    auto Slab = indexAST(AST->getASTContext(), AST->getTopLevelDecls());
+    auto Slab = indexAST(AST->getASTContext(), AST->getPreprocessorPtr(),
+                         AST->getTopLevelDecls());
     FSymbols.update(Path, std::move(Slab));
   }
   auto Symbols = FSymbols.allSymbols();
   Index.build(std::move(Symbols));
 }
 
 bool FileIndex::fuzzyFind(const Context &Ctx, const FuzzyFindRequest &Req,
                           std::function<void(const Symbol &)> Callback) const {
   return Index.fuzzyFind(Ctx, Req, std::move(Callback));
 }
 
 } // namespace clangd
 } // namespace clang

clangd/index/Index.h

 //===--- Symbol.h -----------------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_INDEX_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_INDEX_H
 
 #include "../Context.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Hashing.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
 #include <array>
 #include <string>
 
 namespace clang {
 namespace clangd {
 
 struct SymbolLocation {
struct Symbol {
   //
   // A C++ symbol could have multiple declarations and one definition (e.g.
   // a function is declared in ".h" file, and is defined in ".cc" file).
   //   * For classes, the canonical declaration is usually definition.
   //   * For non-inline functions, the canonical declaration is a declaration
   //     (not a definition), which is usually declared in ".h" file.
   SymbolLocation CanonicalDeclaration;
 
+  /// A brief description of the symbol that can be displayed in the completion
+  /// candidate list. For example, "Foo(X x, Y y) const" is a labal for a
+  /// function.
+  std::string CompletionLabel;
+  /// The piece of text that the user is expected to type to match the
+  /// code-completion string, typically a keyword or the name of a declarator or
+  /// macro.
+  std::string CompletionFilterText;
+  /// What to insert when completing this symbol (plain text version).
+  std::string CompletionPlainInsertText;
+  /// What to insert when completing this symbol (snippet version).
+  std::string CompletionSnippetInsertText;
+
+  /// Optional symbol details that are not required to be set. For example, an
+  /// index fuzzy match can return a large number of symbol candidates, and it
+  /// is preferable to send only core symbol information in the batched results
+  /// and have clients resolve full symbol information for a specific candidate
+  /// if needed.
+  struct Details {
+    // Documentation including comment for the symbol declaration.
+    std::string Documentation;
+    // This is what goes into the LSP detail field in a completion item. For
+    // example, the result type of a function.
+    std::string CompletionDetail;
+  };
+
+  llvm::Optional<Details> Detail;
+
   // FIXME: add definition location of the symbol.
   // FIXME: add all occurrences support.
   // FIXME: add extra fields for index scoring signals.
-  // FIXME: add code completion information.
 };
 
 // A symbol container that stores a set of symbols. The container will maintain
 // the lifetime of the symbols.
 //
 // FIXME: Use a space-efficient implementation, a lot of Symbol fields could
 // share the same storage.
 class SymbolSlab {

clangd/index/SymbolCollector.h

 namespace clang {
 namespace clangd {
 
 // Collect all symbols from an AST.
 //
 // Clients (e.g. clangd) can use SymbolCollector together with
 // index::indexTopLevelDecls to retrieve all symbols when the source file is
 // changed.
 class SymbolCollector : public index::IndexDataConsumer {
 public:
   SymbolCollector() = default;
 
+  void initialize(ASTContext &Ctx) override { ASTCtx = &Ctx; }
+
+  void setPreprocessor(std::shared_ptr<Preprocessor> PP) override {
+    this->PP = std::move(PP);
+  }
+
   bool
   handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
                       ArrayRef<index::SymbolRelation> Relations, FileID FID,
                       unsigned Offset,
                       index::IndexDataConsumer::ASTNodeInfo ASTNode) override;
 
   void finish() override;
 
   SymbolSlab takeSymbols() { return std::move(Symbols); }
 
 private:
   // All Symbols collected from the AST.
   SymbolSlab Symbols;
+  ASTContext *ASTCtx;
+  std::shared_ptr<Preprocessor> PP;
 };
 
 } // namespace clangd
 } // namespace clang

clangd/index/SymbolCollector.cpp

 //===--- SymbolCollector.cpp -------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "SymbolCollector.h"
+#include "../CompletionString.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Index/USRGeneration.h"
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {
 
 namespace {
 // Make the Path absolute using the current working directory of the given
if (Dir) {
     llvm::sys::path::append(AbsoluteFilename, DirName,
                             llvm::sys::path::filename(AbsolutePath.str()));
     return llvm::StringRef(AbsoluteFilename.data(), AbsoluteFilename.size())
         .str();
   }
   return AbsolutePath.str();
 }
 
-// Split a qualified symbol name into scope and unqualified name, e.g. given
+void addSymbolCompletionInfo(ASTContext &Ctx, Preprocessor &PP,
+                             const NamedDecl *ND, Symbol *Sym) {
+  CodeCompletionResult SymbolCompletion(ND, 0);
+  auto Allocator = std::make_shared<GlobalCodeCompletionAllocator>();
+  CodeCompletionTUInfo TUInfo(Allocator);
+  const auto *CCS = SymbolCompletion.CreateCodeCompletionString(
+      Ctx, PP, CodeCompletionContext::CCC_Name, *Allocator, TUInfo,
+      /*IncludeBriefComments*/ true);
+  Sym->CompletionFilterText = getFilterText(*CCS);
+  bool IsSnippet = false;
+  auto LabelAndSnippetText = getLabelAndInsertText(*CCS, &IsSnippet);
+  Sym->CompletionLabel = LabelAndSnippetText.first;
+  if (IsSnippet) {
+    Sym->CompletionSnippetInsertText = LabelAndSnippetText.second;
+    Sym->CompletionPlainInsertText = getLabelAndInsertText(*CCS).second;
+  } else {
+    Sym->CompletionPlainInsertText = LabelAndSnippetText.second;
+  }
+
+  Sym->Detail.emplace();
+  Sym->Detail->Documentation = getDocumentation(*CCS);
+  Sym->Detail->CompletionDetail = getDetail(*CCS);
+}
+
 // "a::b::c", return {"a::b", "c"}. Scope is empty if it doesn't exist.
 std::pair<llvm::StringRef, llvm::StringRef>
 splitQualifiedName(llvm::StringRef QName) {
   assert(!QName.startswith("::") && "Qualified names should not start with ::");
   size_t Pos = QName.rfind("::");
   if (Pos == llvm::StringRef::npos)
     return {StringRef(), QName};
   return {QName.substr(0, Pos), QName.substr(Pos + 2)};
 }
 
 } // namespace
 
 // Always return true to continue indexing.
 bool SymbolCollector::handleDeclOccurence(
     const Decl *D, index::SymbolRoleSet Roles,
     ArrayRef<index::SymbolRelation> Relations, FileID FID, unsigned Offset,
     index::IndexDataConsumer::ASTNodeInfo ASTNode) {
   // FIXME: collect all symbol references.
   if (!(Roles & static_cast<unsigned>(index::SymbolRole::Declaration) ||
         Roles & static_cast<unsigned>(index::SymbolRole::Definition)))
     return true;
 
if (const NamedDecl *ND = llvm::dyn_cast<NamedDecl>(D)) {
     auto ID = SymbolID(USR);
     if (Symbols.find(ID) != Symbols.end())
       return true;
 
     auto &SM = ND->getASTContext().getSourceManager();
     SymbolLocation Location = {
         makeAbsolutePath(SM, SM.getFilename(D->getLocation())),
         SM.getFileOffset(D->getLocStart()), SM.getFileOffset(D->getLocEnd())};
+    Symbol Sym;
+    Sym.ID = std::move(ID);
+
     std::string QName = ND->getQualifiedNameAsString();
     auto ScopeAndName = splitQualifiedName(QName);
-    Symbols.insert({std::move(ID), ScopeAndName.second, ScopeAndName.first,
-                    index::getSymbolInfo(D), std::move(Location)});
+    Sym.Name = ScopeAndName.second;
+    Sym.Scope = ScopeAndName.first;
+
+    Sym.SymInfo = index::getSymbolInfo(D);
+    Sym.CanonicalDeclaration = std::move(Location);
+    assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
+    addSymbolCompletionInfo(*ASTCtx, *PP, ND, &Sym);
+    Symbols.insert(std::move(Sym));
   }
 
   return true;
 }
 
 void SymbolCollector::finish() { Symbols.freeze(); }
 
 } // namespace clangd
 } // namespace clang

unittests/clangd/CodeCompleteTests.cpp

StringWithPos parseTextMarker(StringRef Text) {
   return {std::move(WithoutMarker), MarkerPos};
 }
 
 // GMock helpers for matching completion items.
 MATCHER_P(Named, Name, "") { return arg.insertText == Name; }
 MATCHER_P(Labeled, Label, "") { return arg.label == Label; }
 MATCHER_P(Kind, K, "") { return arg.kind == K; }
 MATCHER_P(Filter, F, "") { return arg.filterText == F; }
+MATCHER_P(Doc, D, "") { return arg.documentation == D; }
+MATCHER_P(Detail, D, "") { return arg.detail == D; }
 MATCHER_P(PlainText, Text, "") {
   return arg.insertTextFormat == clangd::InsertTextFormat::PlainText &&
          arg.insertText == Text;
 }
 MATCHER_P(Snippet, Text, "") {
   return arg.insertTextFormat == clangd::InsertTextFormat::Snippet &&
          arg.insertText == Text;
 }
for (const auto &Pair : Symbols) {
     size_t Pos = QName.rfind("::");
     if (Pos == llvm::StringRef::npos) {
       Sym.Name = QName;
       Sym.Scope = "";
     } else {
       Sym.Name = QName.substr(Pos + 2);
       Sym.Scope = QName.substr(0, Pos);
     }
+    Sym.CompletionPlainInsertText = Sym.Name;
     Sym.SymInfo.Kind = Pair.second;
     Snap->Slab.insert(std::move(Sym));
   }
   for (auto &Iter : Snap->Slab)
     Snap->Pointers.push_back(&Iter.second);
   auto S = std::shared_ptr<std::vector<const Symbol *>>(std::move(Snap),
                                                         &Snap->Pointers);
   I->build(std::move(S));
TEST(CompletionTest, IndexBasedWithFilter) {
   auto I = simpleIndexFromSymbols({{"ns::XYZ", index::SymbolKind::Class},
                                    {"ns::foo", index::SymbolKind::Function}});
   Opts.Index = I.get();
 
   auto Results = completions(R"cpp(
       void f() { ns::x^ }
   )cpp",
                              Opts);
-  EXPECT_THAT(Results.items, Contains(AllOf(Named("XYZ"), Filter("x"))));
+  EXPECT_THAT(Results.items, Contains(Named("XYZ")));
   EXPECT_THAT(Results.items, Not(Has("foo")));
 }
 
 TEST(CompletionTest, GlobalQualified) {
   clangd::CodeCompleteOptions Opts;
   auto I = simpleIndexFromSymbols({{"XYZ", index::SymbolKind::Class}});
   Opts.Index = I.get();
 
TEST(CompletionTest, ASTIndexMultiFile) {
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
                       /*StorePreamblesInMemory=*/true,
                       /*BuildDynamicSymbolIndex=*/true);
 
   Server
       .addDocument(Context::empty(), getVirtualTestFilePath("foo.cpp"), R"cpp(
-      namespace ns { class XYZ {}; void foo() {} }
+      namespace ns { class XYZ {}; void foo(int x) {} }
   )cpp")
       .wait();
 
   auto File = getVirtualTestFilePath("bar.cpp");
   auto Test = parseTextMarker(R"cpp(
-      namespace ns { class XXX {}; void fooooo() {} }
+      namespace ns {
+      class XXX {};
+      /// Doooc
+      void fooooo() {}
+      }
       void f() { ns::^ }
   )cpp");
   Server.addDocument(Context::empty(), File, Test.Text).wait();
 
   auto Results = Server.codeComplete(Context::empty(), File, Test.MarkerPos, {})
                      .get()
                      .second.Value;
   // "XYZ" and "foo" are not included in the file being completed but are still
   // visible through the index.
   EXPECT_THAT(Results.items, Has("XYZ", CompletionItemKind::Class));
   EXPECT_THAT(Results.items, Has("foo", CompletionItemKind::Function));
   EXPECT_THAT(Results.items, Has("XXX", CompletionItemKind::Class));
-  EXPECT_THAT(Results.items, Has("fooooo", CompletionItemKind::Function));
+  EXPECT_THAT(Results.items, Contains(AllOf(Named("fooooo()"), Filter("fooooo"),
+                                            Kind(CompletionItemKind::Function),
+                                            Doc("Doooc"), Detail("void"))));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

unittests/clangd/SymbolCollectorTests.cpp

 //===-- SymbolCollectorTests.cpp  -------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "index/SymbolCollector.h"
 #include "index/SymbolYAML.h"
-#include "clang/Index/IndexingAction.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include <memory>
 #include <string>
 
+using testing::AllOf;
 using testing::Eq;
 using testing::Field;
 using testing::UnorderedElementsAre;
 
 // GMock helpers for matching Symbol.
+MATCHER_P(Labeled, Label, "") { return arg.second.CompletionLabel == Label; }
+MATCHER_P(Detail, D, "") {
+  return arg.second.Detail && arg.second.Detail->CompletionDetail == D;
+}
+MATCHER_P(Doc, D, "") {
+  return arg.second.Detail && arg.second.Detail->Documentation == D;
+}
+MATCHER_P(Plain, Text, "") {
+  return arg.second.CompletionPlainInsertText == Text;
+}
+MATCHER_P(Snippet, S, "") {
+  return arg.second.CompletionSnippetInsertText == S;
+}
 MATCHER_P(QName, Name, "") {
   return (arg.second.Scope + (arg.second.Scope.empty() ? "" : "::") +
           arg.second.Name) == Name;
 }
 
 namespace clang {
 namespace clangd {
 
const std::string Main = R"(
     }
     void f1() {}
   )";
   runSymbolCollector(Header, Main);
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo"), QName("Foo::f"),
                                             QName("f1"), QName("f2")));
 }
 
+TEST_F(SymbolCollectorTest, SymbolWithDocumentation) {
+  const std::string Main = R"(
+    namespace nx {
+    /// Foo comment.
+    int ff(int x, double y) { return 0; }
+    }
+  )";
+  runSymbolCollector(/*Header=*/"", Main);
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(QName("nx"),
+                                   AllOf(QName("nx::ff"),
+                                         Labeled("ff(int x, double y)"),
+                                         Detail("int"), Doc("Foo comment."))));
+}
+
+TEST_F(SymbolCollectorTest, PlainAndSnippet) {
+  const std::string Main = R"(
+    namespace nx {
+    void f() {}
+    int ff(int x, double y) { return 0; }
+    }
+  )";
+  runSymbolCollector(/*Header=*/"", Main);
+  EXPECT_THAT(
+      Symbols,
+      UnorderedElementsAre(
+          QName("nx"),
+          AllOf(QName("nx::f"), Labeled("f()"), Plain("f()"), Snippet("")),
+          AllOf(QName("nx::ff"), Labeled("ff(int x, double y)"), Plain("ff"),
+                Snippet("ff(${1:int x}, ${2:double y})"))));
+}
+
 TEST_F(SymbolCollectorTest, YAMLConversions) {
   const std::string YAML1 = R"(
 ---
 ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF856
 Name:   'Foo1'
 Scope:   'clang'
 SymInfo:
   Kind:            Function