[clangd] Fetch documentation from the Index during signature help

[ilya-biryukov]

Summary

Sema can only be used for documentation in the current file, other doc
comments should be fetched from the index.

[ilya-biryukov]

• run clang-format

[kadircet]

LGTM

[hokein]

looks good, a few nits.

clangd/CodeComplete.cpp

742 ↗

(On Diff #160647)

nit: do we want to log anything here? It may be useful for debug.

clangd/index/Index.h

65 ↗

(On Diff #160647)

We already have this similar function in clangd/AST.h.

[ilya-biryukov]

• Log on index request, remove FIXME that was addressed
• Remove SymbolID::forDecl, use existing helper instead

[ioeric]

clangd/CodeComplete.cpp

755 ↗

(On Diff #160992)

drive by: I think this should be vlog or dlog.

[ilya-biryukov]

clangd/CodeComplete.cpp

742 ↗

(On Diff #160647)

Definitely useful. Thanks!

clangd/index/Index.h

65 ↗

(On Diff #160647)

Thanks for pointing this out.
It's somewhat hard to find. WDYT about moving it to Index.h? The concern would probably be a somewhat broken layering, right? E.g. Index.h should not directly depend on anything AST-specific

[hokein]

clangd/index/Index.h

65 ↗

(On Diff #160647)

Yes, I think we will not add any AST-specific stuff to Index.h, that's why we have AST.h.

[ilya-biryukov]

clangd/CodeComplete.cpp

755 ↗

(On Diff #160992)

Code completion also logs the number of results from sema, index, etc. using the log() call.
The added log message looks similar, so trying to be consistent with the rest of the code in this file.

Maybe we should turn all of them into vlog or dlog, but I'd rather leave it to a separate patch.

[ioeric]

clangd/CodeComplete.cpp

755 ↗

(On Diff #160992)

I'm not sure which level code completion log messages should be in, but I think we should follow the guidelines in the logger documentation instead of the existing uses.

Quote from Logger.h

// log() is used for information important to understanding a clangd session.
// e.g. the names of LSP messages sent are logged at this level.
// This level could be enabled in production builds to allow later inspection.

// vlog() is used for details often needed for debugging clangd sessions.
// This level would typically be enabled for clangd developers.

[hokein]

clangd/CodeComplete.cpp

755 ↗

(On Diff #160992)

My recent experience of debugging some weird GotoDef issues tells me that log of index should be on production (since it is a non-trivial part in a clangd session), it would be really helpful to understand what is going on.

[ioeric]

clangd/CodeComplete.cpp

755 ↗

(On Diff #160992)

I agree that they are useful for debugging, but they might not be interesting to end-users. And I think that's why there is vlog. Clangd developers could use a different log level with --log flag.

[ilya-biryukov]

clangd/CodeComplete.cpp

755 ↗

(On Diff #160992)

I agree that vlog is probably a better fit here, but still think we should change it across CodeComplete.cpp in a single commit, rather than partially apply the guidelines to different parts of the file.

However, if everyone agrees we should use vlog here, happy to use vlog too and also send a patch to update all the rest of the usages.
The situation I'm trying to avoid is:

1. We commit vlog here.
2. Someone argues that using log is actually better and we decide to not change other usages to log.
3. We end up with inconsistent choices across this file: vlog for signature help and log for code completion.

But if there's an agreement that we should use vlog everywhere, more than happy to change it :-)

[ioeric]

clangd/CodeComplete.cpp

755 ↗

(On Diff #160992)

That sounds good to me. Thanks!

clang-tools-extra/trunk/clangd/ClangdServer.cpp

WorkScheduler.runWithPreamble("CodeComplete", File,
                                 Bind(Task, File.str(), std::move(CB)));
 }
 
 void ClangdServer::signatureHelp(PathRef File, Position Pos,
                                  Callback<SignatureHelp> CB) {
 
   auto PCHs = this->PCHs;
   auto FS = FSProvider.getFileSystem();
-  auto Action = [Pos, FS, PCHs](Path File, Callback<SignatureHelp> CB,
+  auto *Index = this->Index;
+  auto Action = [Pos, FS, PCHs, Index](Path File, Callback<SignatureHelp> CB,
                                        llvm::Expected<InputsAndPreamble> IP) {
     if (!IP)
       return CB(IP.takeError());
 
     auto PreambleData = IP->Preamble;
     CB(clangd::signatureHelp(File, IP->Command,
                              PreambleData ? &PreambleData->Preamble : nullptr,
-                             IP->Contents, Pos, FS, PCHs));
+                             IP->Contents, Pos, FS, PCHs, Index));
   };
 
   WorkScheduler.runWithPreamble("SignatureHelp", File,
                                 Bind(Action, File.str(), std::move(CB)));
 }
 
 llvm::Expected<tooling::Replacements>
 ClangdServer::formatRange(StringRef Code, PathRef File, Range Rng) {

clang-tools-extra/trunk/clangd/CodeComplete.h

CodeCompleteResult codeComplete(PathRef FileName,
                                 PrecompiledPreamble const *Preamble,
                                 const IncludeStructure &PreambleInclusions,
                                 StringRef Contents, Position Pos,
                                 IntrusiveRefCntPtr<vfs::FileSystem> VFS,
                                 std::shared_ptr<PCHContainerOperations> PCHs,
                                 CodeCompleteOptions Opts);
 
 /// Get signature help at a specified \p Pos in \p FileName.
-SignatureHelp signatureHelp(PathRef FileName,
-                            const tooling::CompileCommand &Command,
-                            PrecompiledPreamble const *Preamble,
-                            StringRef Contents, Position Pos,
-                            IntrusiveRefCntPtr<vfs::FileSystem> VFS,
-                            std::shared_ptr<PCHContainerOperations> PCHs);
+SignatureHelp
+signatureHelp(PathRef FileName, const tooling::CompileCommand &Command,
+              PrecompiledPreamble const *Preamble, StringRef Contents,
+              Position Pos, IntrusiveRefCntPtr<vfs::FileSystem> VFS,
+              std::shared_ptr<PCHContainerOperations> PCHs, SymbolIndex *Index);
 
 // For index-based completion, we only consider:
 //   * symbols in namespaces or translation unit scopes (e.g. no class
 //     members, no locals)
 //   * enum constants in unscoped enum decl (e.g. "red" in "enum {red};")
 //   * primary templates (no specializations)
 // For the other cases, we let Clang do the completion because it does not
 // need any non-local information and it will be much better at following
 // lookup rules. Other symbols still appear in the index for other purposes,
 // like workspace/symbols or textDocument/definition, but are not used for code
 // completion.
 bool isIndexedForCodeCompletion(const NamedDecl &ND, ASTContext &ASTCtx);
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_CODECOMPLETE_H

clang-tools-extra/trunk/clangd/CodeComplete.cpp

 
 private:
   CodeCompleteOptions Opts;
   std::shared_ptr<GlobalCodeCompletionAllocator> CCAllocator;
   CodeCompletionTUInfo CCTUInfo;
   llvm::unique_function<void()> ResultsCallback;
 };
 
-using ScoredSignature =
-    std::pair<SignatureQualitySignals, SignatureInformation>;
+struct ScoredSignature {
+  // When set, requires documentation to be requested from the index with this
+  // ID.
+  llvm::Optional<SymbolID> IDForDoc;
+  SignatureInformation Signature;
+  SignatureQualitySignals Quality;
+};
 
 class SignatureHelpCollector final : public CodeCompleteConsumer {
-
 public:
   SignatureHelpCollector(const clang::CodeCompleteOptions &CodeCompleteOpts,
-                         SignatureHelp &SigHelp)
-      : CodeCompleteConsumer(CodeCompleteOpts, /*OutputIsBinary=*/false),
+                         SymbolIndex *Index, SignatureHelp &SigHelp)
+      : CodeCompleteConsumer(CodeCompleteOpts,
+                             /*OutputIsBinary=*/false),
         SigHelp(SigHelp),
         Allocator(std::make_shared<clang::GlobalCodeCompletionAllocator>()),
-        CCTUInfo(Allocator) {}
+        CCTUInfo(Allocator), Index(Index) {}
 
   void ProcessOverloadCandidates(Sema &S, unsigned CurrentArg,
                                  OverloadCandidate *Candidates,
                                  unsigned NumCandidates) override {
     std::vector<ScoredSignature> ScoredSignatures;
     SigHelp.signatures.reserve(NumCandidates);
     ScoredSignatures.reserve(NumCandidates);
     // FIXME(rwols): How can we determine the "active overload candidate"?
for (unsigned I = 0; I < NumCandidates; ++I) {
       if (auto *Func = Candidate.getFunction()) {
         if (auto *Pattern = Func->getTemplateInstantiationPattern())
           Candidate = OverloadCandidate(Pattern);
       }
 
       const auto *CCS = Candidate.CreateSignatureString(
           CurrentArg, S, *Allocator, CCTUInfo, true);
       assert(CCS && "Expected the CodeCompletionString to be non-null");
-      // FIXME: for headers, we need to get a comment from the index.
       ScoredSignatures.push_back(processOverloadCandidate(
           Candidate, *CCS,
           Candidate.getFunction()
               ? getDeclComment(S.getASTContext(), *Candidate.getFunction())
               : ""));
     }
-    std::sort(ScoredSignatures.begin(), ScoredSignatures.end(),
-              [](const ScoredSignature &L, const ScoredSignature &R) {
-                // Ordering follows:
-                // - Less number of parameters is better.
-                // - Function is better than FunctionType which is better than
-                // Function Template.
-                // - High score is better.
-                // - Shorter signature is better.
-                // - Alphebatically smaller is better.
-                if (L.first.NumberOfParameters != R.first.NumberOfParameters)
-                  return L.first.NumberOfParameters <
-                         R.first.NumberOfParameters;
-                if (L.first.NumberOfOptionalParameters !=
-                    R.first.NumberOfOptionalParameters)
-                  return L.first.NumberOfOptionalParameters <
-                         R.first.NumberOfOptionalParameters;
-                if (L.first.Kind != R.first.Kind) {
-                  using OC = CodeCompleteConsumer::OverloadCandidate;
-                  switch (L.first.Kind) {
-                  case OC::CK_Function:
-                    return true;
-                  case OC::CK_FunctionType:
-                    return R.first.Kind != OC::CK_Function;
-                  case OC::CK_FunctionTemplate:
-                    return false;
-                  }
-                  llvm_unreachable("Unknown overload candidate type.");
-                }
-                if (L.second.label.size() != R.second.label.size())
-                  return L.second.label.size() < R.second.label.size();
-                return L.second.label < R.second.label;
-              });
-    for (const auto &SS : ScoredSignatures)
-      SigHelp.signatures.push_back(SS.second);
+
+    // Sema does not load the docs from the preamble, so we need to fetch extra
+    // docs from the index instead.
+    llvm::DenseMap<SymbolID, std::string> FetchedDocs;
+    if (Index) {
+      LookupRequest IndexRequest;
+      for (const auto &S : ScoredSignatures) {
+        if (!S.IDForDoc)
+          continue;
+        IndexRequest.IDs.insert(*S.IDForDoc);
+      }
+      Index->lookup(IndexRequest, [&](const Symbol &S) {
+        if (!S.Detail || S.Detail->Documentation.empty())
+          return;
+        FetchedDocs[S.ID] = S.Detail->Documentation;
+      });
+      log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+          "symbols with non-empty docs in the response",
+          IndexRequest.IDs.size(), FetchedDocs.size());
+    }
+
+    std::sort(
+        ScoredSignatures.begin(), ScoredSignatures.end(),
+          if (L.Quality.NumberOfParameters != R.Quality.NumberOfParameters)
+            return L.Quality.NumberOfParameters < R.Quality.NumberOfParameters;
+          if (L.Quality.NumberOfOptionalParameters !=
+              R.Quality.NumberOfOptionalParameters)
+            return L.Quality.NumberOfOptionalParameters <
+                   R.Quality.NumberOfOptionalParameters;
+          if (L.Quality.Kind != R.Quality.Kind) {
+            switch (L.Quality.Kind) {
+              return R.Quality.Kind != OC::CK_Function;
+          if (L.Signature.label.size() != R.Signature.label.size())
+            return L.Signature.label.size() < R.Signature.label.size();
+          return L.Signature.label < R.Signature.label;
+
+    for (auto &SS : ScoredSignatures) {
+      auto IndexDocIt =
+          SS.IDForDoc ? FetchedDocs.find(*SS.IDForDoc) : FetchedDocs.end();
+      if (IndexDocIt != FetchedDocs.end())
+        SS.Signature.documentation = IndexDocIt->second;
+
+      SigHelp.signatures.push_back(std::move(SS.Signature));
+    }
   }
 
   GlobalCodeCompletionAllocator &getAllocator() override { return *Allocator; }
 
   CodeCompletionTUInfo &getCodeCompletionTUInfo() override { return CCTUInfo; }
 
 private:
   // FIXME(ioeric): consider moving CodeCompletionString logic here to
   // CompletionString.h.
   ScoredSignature processOverloadCandidate(const OverloadCandidate &Candidate,
                                            const CodeCompletionString &CCS,
                                            llvm::StringRef DocComment) const {
-    SignatureInformation Result;
+    SignatureInformation Signature;
     SignatureQualitySignals Signal;
     const char *ReturnType = nullptr;
 
-    Result.documentation = formatDocumentation(CCS, DocComment);
+    Signature.documentation = formatDocumentation(CCS, DocComment);
     Signal.Kind = Candidate.getKind();
 
     for (const auto &Chunk : CCS) {
       switch (Chunk.Kind) {
       case CodeCompletionString::CK_ResultType:
         // A piece of text that describes the type of an entity or,
         // for functions and methods, the return type.
         assert(!ReturnType && "Unexpected CK_ResultType");
         ReturnType = Chunk.Text;
         break;
       case CodeCompletionString::CK_Placeholder:
         // A string that acts as a placeholder for, e.g., a function call
         // argument.
         // Intentional fallthrough here.
       case CodeCompletionString::CK_CurrentParameter: {
         // A piece of text that describes the parameter that corresponds to
         // the code-completion location within a function call, message send,
         // macro invocation, etc.
-        Result.label += Chunk.Text;
+        Signature.label += Chunk.Text;
         ParameterInformation Info;
         Info.label = Chunk.Text;
-        Result.parameters.push_back(std::move(Info));
+        Signature.parameters.push_back(std::move(Info));
         Signal.NumberOfParameters++;
         Signal.ContainsActiveParameter = true;
         break;
       }
       case CodeCompletionString::CK_Optional: {
         // The rest of the parameters are defaulted/optional.
         assert(Chunk.Optional &&
                "Expected the optional code completion string to be non-null.");
-        Result.label +=
-            getOptionalParameters(*Chunk.Optional, Result.parameters, Signal);
+        Signature.label += getOptionalParameters(*Chunk.Optional,
+                                                 Signature.parameters, Signal);
         break;
       }
       case CodeCompletionString::CK_VerticalSpace:
         break;
       default:
-        Result.label += Chunk.Text;
+        Signature.label += Chunk.Text;
         break;
       }
     }
     if (ReturnType) {
-      Result.label += " -> ";
-      Result.label += ReturnType;
+      Signature.label += " -> ";
+      Signature.label += ReturnType;
     }
-    dlog("Signal for {0}: {1}", Result, Signal);
-    return {Signal, Result};
+    dlog("Signal for {0}: {1}", Signature, Signal);
+    ScoredSignature Result;
+    Result.Signature = std::move(Signature);
+    Result.Quality = Signal;
+    Result.IDForDoc =
+        Result.Signature.documentation.empty() && Candidate.getFunction()
+            ? clangd::getSymbolID(Candidate.getFunction())
+            : llvm::None;
+    return Result;
   }
 
   SignatureHelp &SigHelp;
   std::shared_ptr<clang::GlobalCodeCompletionAllocator> Allocator;
   CodeCompletionTUInfo CCTUInfo;
-
+  const SymbolIndex *Index;
 }; // SignatureHelpCollector
 
 struct SemaCompleteInput {
   PathRef FileName;
   const tooling::CompileCommand &Command;
   PrecompiledPreamble const *Preamble;
   StringRef Contents;
   Position Pos;
return CodeCompleteFlow(FileName, PreambleInclusions, Opts)
       .run({FileName, Command, Preamble, Contents, Pos, VFS, PCHs});
 }
 
 SignatureHelp signatureHelp(PathRef FileName,
                             const tooling::CompileCommand &Command,
                             PrecompiledPreamble const *Preamble,
                             StringRef Contents, Position Pos,
                             IntrusiveRefCntPtr<vfs::FileSystem> VFS,
-                            std::shared_ptr<PCHContainerOperations> PCHs) {
+                            std::shared_ptr<PCHContainerOperations> PCHs,
+                            SymbolIndex *Index) {
   SignatureHelp Result;
   clang::CodeCompleteOptions Options;
   Options.IncludeGlobals = false;
   Options.IncludeMacros = false;
   Options.IncludeCodePatterns = false;
   Options.IncludeBriefComments = false;
   IncludeStructure PreambleInclusions; // Unused for signatureHelp
-  semaCodeComplete(llvm::make_unique<SignatureHelpCollector>(Options, Result),
-                   Options,
-                   {FileName, Command, Preamble, Contents, Pos, std::move(VFS),
-                    std::move(PCHs)});
+  semaCodeComplete(
+      llvm::make_unique<SignatureHelpCollector>(Options, Index, Result),
   return Result;
 }
 
 bool isIndexedForCodeCompletion(const NamedDecl &ND, ASTContext &ASTCtx) {
   using namespace clang::ast_matchers;
   auto InTopLevelScope = hasDeclContext(
       anyOf(namespaceDecl(), translationUnitDecl(), linkageSpecDecl()));
   return !match(decl(anyOf(InTopLevelScope,

clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

#if 0
   par^
 #endif
     }
 )cpp");
 
   EXPECT_TRUE(Results.Completions.empty());
 }
 
-SignatureHelp signatures(StringRef Text) {
+SignatureHelp signatures(StringRef Text,
+                         std::vector<Symbol> IndexSymbols = {}) {
+  std::unique_ptr<SymbolIndex> Index;
+  if (!IndexSymbols.empty())
+    Index = memIndex(IndexSymbols);
+
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
-  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+  ClangdServer::Options Opts = ClangdServer::optsForTest();
+  Opts.StaticIndex = Index.get();
+
+  ClangdServer Server(CDB, FS, DiagConsumer, Opts);
   auto File = testPath("foo.cpp");
   Annotations Test(Text);
   runAddDocument(Server, File, Test.code());
   return cantFail(runSignatureHelp(Server, File, Test.point()));
 }
 
 MATCHER_P(ParamsAre, P, "") {
   if (P.size() != arg.parameters.size())
     return false;
   for (unsigned I = 0; I < P.size(); ++I)
     if (P[I] != arg.parameters[I].label)
       return false;
   return true;
 }
+MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; }
 
 Matcher<SignatureInformation> Sig(std::string Label,
                                   std::vector<std::string> Params) {
   return AllOf(SigHelpLabeled(Label), ParamsAre(Params));
 }
 
 TEST(SignatureHelpTest, Overloads) {
   auto Results = signatures(R"cpp(
int main() {
       X<int>().foo<double>(^)
     }
   )cpp";
 
   EXPECT_THAT(signatures(Sig3).signatures,
               ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
 }
 
+TEST(SignatureHelpTest, IndexDocumentation) {
+  Symbol::Details DocDetails;
+  DocDetails.Documentation = "Doc from the index";
+
+  Symbol Foo0 = sym("foo", index::SymbolKind::Function, "@F@\\0#");
+  Foo0.Detail = &DocDetails;
+  Symbol Foo1 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#");
+  Foo1.Detail = &DocDetails;
+  Symbol Foo2 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#I#");
+
+  EXPECT_THAT(
+      signatures(R"cpp(
+    int foo();
+    int foo(double);
+
+    void test() {
+      foo(^);
+    }
+  )cpp",
+                 {Foo0})
+          .signatures,
+      ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+                  AllOf(Sig("foo(double) -> int", {"double"}), SigDoc(""))));
+
+  EXPECT_THAT(
+      signatures(R"cpp(
+    int foo();
+    // Overriden doc from sema
+    int foo(int);
+    // Doc from sema
+    int foo(int, int);
+
+    void test() {
+      foo(^);
+    }
+  )cpp",
+                 {Foo0, Foo1, Foo2})
+          .signatures,
+      ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+                  AllOf(Sig("foo(int) -> int", {"int"}),
+                        SigDoc("Overriden doc from sema")),
+                  AllOf(Sig("foo(int, int) -> int", {"int", "int"}),
+                        SigDoc("Doc from sema"))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang