[clangd] Show documentation in hover, and fetch docs from index if needed.

[sammccall]

Summary

I assume showing docs is going to be part of structured hover rendering, but
it's unclear whether that's going to make clangd 9 so this is low-hanging fruit.

(Also fixes a bug uncovered in FormattedString's plain text output: need blank
lines when text follows codeblocks)

[kadircet]

mostly nits, only important comment is around ordering of include in the rendered hover response

clang-tools-extra/clangd/FormattedString.cpp

148 ↗

(On Diff #208291)

could you add braces to outermost if, and clang-format this chunk.

148 ↗

(On Diff #208291)

nit: if the following change(regarding cleaning whitespaces at the end of each chunk) was unintentional,
why not make this one a lambda(like ensurewhitespaces) and just call it before and after we insert a codeblocks contents?

170 ↗

(On Diff #208291)

why perform this for every chunk? This was previously used to get rid of trailing whitespaces, now this also trims trailing whitespaces in chunks. this doesn't sound valid to me?

clang-tools-extra/clangd/XRefs.cpp

616 ↗

(On Diff #208291)

/*IsMainFileOnly=*/false

616 ↗

(On Diff #208291)

also could you add a comment explaining why it is checked for IsMainFileOnly=false.

IIRC we decided that it was because, main file symbols' comments should already exist in the ast.

736 ↗

(On Diff #208291)

clang-format

1257 ↗

(On Diff #208291)

I believe documentation should come first. Since it has a non-fixed length, I believe it would be nice if people could simply not notice it(assuming bottom is closer to cursor location) WDYT?

1258 ↗

(On Diff #208291)

clang-format

[sammccall]

clang-tools-extra/clangd/FormattedString.cpp

148 ↗

(On Diff #208291)

The problem then is if you insert two code blocks in a row, you get 4 newlines between them.

170 ↗

(On Diff #208291)

This normalizes the whitespace between inline chunks to one space. Note that the text/inline blocks ensure there's a space before following another inline chunk. Is there a case where we'd be adding inline text and *want* multiple spaces between chunks?

This was an incomplete thought though: it allows the \n\n block to be simplified.

(I'm not totally happy with this rendering logic either, but I'm fairly convinced we're going to have to replace FormattedString with something hierarchical before too long, so I'm trying to avoid doing too much surgery here)

clang-tools-extra/clangd/XRefs.cpp

1257 ↗

(On Diff #208291)

The problem is that the hover text is often limited length and will almost certainly be truncated at the bottom. Hover also sometimes appears below the cursor.

Prioritizing from bottom->top is an interesting idea, but I don't think editors or users are expecting it.

[sammccall]

address comments

[kadircet]

LGTM, thanks!

clang-tools-extra/clangd/FormattedString.cpp

170 ↗

(On Diff #208291)

This normalizes the whitespace between inline chunks to one space. Note that the text/inline blocks ensure there's a space before following another inline chunk. Is there a case where we'd be adding inline text and *want* multiple spaces between chunks?

I am not sure if we would ever want to put some spaces between chunks, but it is still doable by prepending it to the second chunk instead of appending to the first.

So, I suppose it is OK until we figure out some other use case.

clang-tools-extra/clangd/XRefs.cpp

1257 ↗

(On Diff #208291)

you are right, truncation is definitely a bigger concern

[sammccall]

clang-tools-extra/clangd/FormattedString.cpp

170 ↗

(On Diff #208291)

Yeah, I think that's unlikely to do by accident. Mostly I don't want to worry about whether a doc comment or prettyprinted decl or whatever ends in \n.

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

auto Action = [Pos](Callback<std::vector<DocumentHighlight>> CB,
     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<HoverInfo>> CB) {
-  auto Action = [Pos](decltype(CB) CB, Path File,
+  auto Action = [Pos, this](decltype(CB) CB, Path File,
                             llvm::Expected<InputsAndAST> InpAST) {
     if (!InpAST)
       return CB(InpAST.takeError());
     format::FormatStyle Style = getFormatStyleForFile(
         File, InpAST->Inputs.Contents, InpAST->Inputs.FS.get());
-    CB(clangd::getHover(InpAST->AST, Pos, std::move(Style)));
+    CB(clangd::getHover(InpAST->AST, Pos, std::move(Style), Index));
   };
 
   WorkScheduler.runWithAST("Hover", File,
                            Bind(Action, std::move(CB), File.str()));
 }
 
 void ClangdServer::typeHierarchy(PathRef File, Position Pos, int Resolve,
                                  TypeHierarchyDirection Direction,

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

static std::string renderCodeBlock(llvm::StringRef Input,
   // Use the corresponding number of backticks to start and end a code block.
   std::string BlockMarker(/*Repeat=*/std::max(3u, MaxBackticks + 1), '`');
   return BlockMarker + Language.str() + "\n" + Input.str() + "\n" + BlockMarker;
 }
 
 } // namespace
 
 void FormattedString::appendText(std::string Text) {
-  // We merge consecutive blocks of text to simplify the overall structure.
-  if (Chunks.empty() || Chunks.back().Kind != ChunkKind::PlainText) {
-    Chunk C;
-    C.Kind = ChunkKind::PlainText;
-    Chunks.push_back(C);
-  }
-  // FIXME: ensure there is a whitespace between the chunks.
-  Chunks.back().Contents += Text;
-}
+  C.Contents = Text;
 
 void FormattedString::appendCodeBlock(std::string Code, std::string Language) {
   Chunk C;
   C.Kind = ChunkKind::CodeBlock;
   C.Contents = std::move(Code);
   C.Language = std::move(Language);
   Chunks.push_back(std::move(C));
 }

 
 std::string FormattedString::renderAsPlainText() const {
   std::string R;
   auto EnsureWhitespace = [&]() {
     if (R.empty() || isWhitespace(R.back()))
       return;
     R += " ";
   };
+  Optional<bool> LastWasBlock;
   for (const auto &C : Chunks) {
+    bool IsBlock = C.Kind == ChunkKind::CodeBlock;
+    if (LastWasBlock.hasValue() && (IsBlock || *LastWasBlock))
+      R += "\n\n";
+    LastWasBlock = IsBlock;
+
     switch (C.Kind) {
     case ChunkKind::PlainText:
       EnsureWhitespace();
       R += C.Contents;
-      continue;
+      break;
     case ChunkKind::InlineCodeBlock:
       EnsureWhitespace();
       R += C.Contents;
-      continue;
+      break;
     case ChunkKind::CodeBlock:
-      if (!R.empty())
-        R += "\n\n";
       R += C.Contents;
-      if (!llvm::StringRef(C.Contents).endswith("\n"))
-        R += "\n";
-      continue;
-    }
-    llvm_unreachable("unhanlded ChunkKind");
+      break;
     }
+    // Trim trailing whitespace in chunk.
     while (!R.empty() && isWhitespace(R.back()))
       R.pop_back();
+  }
   return R;
 }
 
 std::string FormattedString::renderForTests() const {
   std::string R;
   for (const auto &C : Chunks) {
     switch (C.Kind) {
     case ChunkKind::PlainText:

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

 inline bool operator==(const HoverInfo::Param &LHS,
                        const HoverInfo::Param &RHS) {
   return std::tie(LHS.Type, LHS.Name, LHS.Default) ==
          std::tie(RHS.Type, RHS.Name, RHS.Default);
 }
 
 /// Get the hover information when hovering at \p Pos.
 llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
-                                   format::FormatStyle Style);
+                                   format::FormatStyle Style,
+                                   const SymbolIndex *Index);
 
 /// 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.

clang-tools-extra/trunk/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 "FormattedString.h"
 #include "Logger.h"
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "URI.h"
+#include "index/Index.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/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/ExprCXX.h"
if (!QT.isNull()) {
         return CD->getLambdaCallOperator();
     }
   }
 
   // Non-lambda functions.
   return D->getAsFunction();
 }
 
+// Look up information about D from the index, and add it to Hover.
+static void enhanceFromIndex(HoverInfo &Hover, const Decl *D,
+                             const SymbolIndex *Index) {
+  if (!Index || !llvm::isa<NamedDecl>(D))
+    return;
+  const NamedDecl &ND = *cast<NamedDecl>(D);
+  // We only add documentation, so don't bother if we already have some.
+  if (!Hover.Documentation.empty())
+    return;
+  // Skip querying for non-indexable symbols, there's no point.
+  // We're searching for symbols that might be indexed outside this main file.
+  if (!SymbolCollector::shouldCollectSymbol(ND, ND.getASTContext(),
+                                            SymbolCollector::Options(),
+                                            /*IsMainFileOnly=*/false))
+    return;
+  auto ID = getSymbolID(&ND);
+  if (!ID)
+    return;
+  LookupRequest Req;
+  Req.IDs.insert(*ID);
+  Index->lookup(
+      Req, [&](const Symbol &S) { Hover.Documentation = S.Documentation; });
+}
+
 /// Generate a \p Hover object given the declaration \p D.
-static HoverInfo getHoverContents(const Decl *D) {
+static HoverInfo getHoverContents(const Decl *D, const SymbolIndex *Index) {
   HoverInfo HI;
   const ASTContext &Ctx = D->getASTContext();
 
   HI.NamespaceScope = getNamespaceScope(D);
   if (!HI.NamespaceScope->empty())
     HI.NamespaceScope->append("::");
   HI.LocalScope = getLocalScope(D);
   if (!HI.LocalScope.empty())
if (const Expr *Init = Var->getInit()) {
         llvm::raw_string_ostream ValueOS(*HI.Value);
         Result.Val.printPretty(ValueOS, const_cast<ASTContext &>(Ctx),
                                Var->getType());
       }
     }
   }
 
   HI.Definition = printDefinition(D);
+  enhanceFromIndex(HI, D, Index);
   return HI;
 }
 
 /// Generate a \p Hover object given the type \p T.
-static HoverInfo getHoverContents(QualType T, const Decl *D,
-                                  ASTContext &ASTCtx) {
+static HoverInfo getHoverContents(QualType T, const Decl *D, ASTContext &ASTCtx,
+                                  const SymbolIndex *Index) {
   HoverInfo HI;
   llvm::raw_string_ostream OS(HI.Name);
   PrintingPolicy Policy = printingPolicyForDecls(ASTCtx.getPrintingPolicy());
   T.print(OS, Policy);
 
-  if (D)
+  if (D) {
     HI.Kind = indexSymbolKindToSymbolKind(index::getSymbolInfo(D).Kind);
+    enhanceFromIndex(HI, D, Index);
+  }
   return HI;
 }
 
 /// Generate a \p Hover object given the macro \p MacroDecl.
 static HoverInfo getHoverContents(const DefinedMacro &Macro, ParsedAST &AST) {
   HoverInfo HI;
   SourceManager &SM = AST.getSourceManager();
   HI.Name = Macro.Name;
bool hasDeducedType(ParsedAST &AST, SourceLocation SourceLocationBeg) {
   }
   AST.getPreprocessor().LookUpIdentifierInfo(Tok);
   if (!(Tok.is(tok::kw_auto) || Tok.is(tok::kw_decltype)))
     return false;
   return true;
 }
 
 llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
-                                   format::FormatStyle Style) {
+                                   format::FormatStyle Style,
+                                   const SymbolIndex *Index) {
   llvm::Optional<HoverInfo> HI;
   SourceLocation SourceLocationBeg = getBeginningOfIdentifier(
       AST, Pos, AST.getSourceManager().getMainFileID());
   // Identified symbols at a specific position.
   auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
 
   if (!Symbols.Macros.empty())
     HI = getHoverContents(Symbols.Macros[0], AST);
   else if (!Symbols.Decls.empty())
-    HI = getHoverContents(Symbols.Decls[0]);
+    HI = getHoverContents(Symbols.Decls[0], Index);
   else {
     if (!hasDeducedType(AST, SourceLocationBeg))
       return None;
 
     DeducedTypeVisitor V(SourceLocationBeg);
     V.TraverseAST(AST.getASTContext());
     if (V.DeducedType.isNull())
       return None;
-    HI = getHoverContents(V.DeducedType, V.D, AST.getASTContext());
+    HI = getHoverContents(V.DeducedType, V.D, AST.getASTContext(), Index);
   }
 
   auto Replacements = format::reformat(
       Style, HI->Definition, tooling::Range(0, HI->Definition.size()));
   if (auto Formatted =
           tooling::applyAllReplacements(HI->Definition, Replacements))
     HI->Definition = *Formatted;
 
FormattedString HoverInfo::present() const {
   }
 
   if (!Definition.empty()) {
     Output.appendCodeBlock(Definition);
   } else {
     // Builtin types
     Output.appendCodeBlock(Name);
   }
+
+  if (!Documentation.empty())
+    Output.appendText(Documentation);
   return Output;
 }
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
                               const HoverInfo::Param &P) {
   std::vector<llvm::StringRef> Output;
   if (P.Type)
     Output.push_back(*P.Type);

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

 namespace clangd {
 namespace {
 
 TEST(FormattedString, Basic) {
   FormattedString S;
   EXPECT_EQ(S.renderAsPlainText(), "");
   EXPECT_EQ(S.renderAsMarkdown(), "");
 
-  S.appendText("foobar");
-  EXPECT_EQ(S.renderAsPlainText(), "foobar");
-  EXPECT_EQ(S.renderAsMarkdown(), "foobar");
+  S.appendText("foobar  ");
+  S.appendText("baz");
+  EXPECT_EQ(S.renderAsPlainText(), "foobar baz");
+  EXPECT_EQ(S.renderAsMarkdown(), "foobar  baz");
 
   S = FormattedString();
   S.appendInlineCode("foobar");
   EXPECT_EQ(S.renderAsPlainText(), "foobar");
   EXPECT_EQ(S.renderAsMarkdown(), "`foobar`");
 
   S = FormattedString();
   S.appendCodeBlock("foobar");
   EXPECT_EQ(S.renderAsPlainText(), "foobar");
   EXPECT_EQ(S.renderAsMarkdown(), "```cpp\n"
                                   "foobar\n"
                                   "```\n");
 }
 
 TEST(FormattedString, CodeBlocks) {
   FormattedString S;
   S.appendCodeBlock("foobar");
   S.appendCodeBlock("bazqux", "javascript");
+  S.appendText("after");
+
+  std::string ExpectedText = R"(foobar
+
+bazqux
 
-  EXPECT_EQ(S.renderAsPlainText(), "foobar\n\n\nbazqux");
+after)";
+  EXPECT_EQ(S.renderAsPlainText(), ExpectedText);
   std::string ExpectedMarkdown = R"md(```cpp
 foobar
 ```
 ```javascript
 bazqux
 ```
-)md";
+after)md";
   EXPECT_EQ(S.renderAsMarkdown(), ExpectedMarkdown);
 
   S = FormattedString();
   S.appendInlineCode("foobar");
   S.appendInlineCode("bazqux");
   EXPECT_EQ(S.renderAsPlainText(), "foobar bazqux");
   EXPECT_EQ(S.renderAsMarkdown(), "`foobar` `bazqux`");
 

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

 //===-- XRefsTests.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 "Annotations.h"
 #include "ClangdUnit.h"
 #include "Compiler.h"
 #include "Matchers.h"
 #include "Protocol.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
+#include "TestIndex.h"
 #include "TestTU.h"
 #include "XRefs.h"
 #include "index/FileIndex.h"
+#include "index/MemIndex.h"
 #include "index/SymbolCollector.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/ADT/None.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <string>
for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Code);
 
     Annotations T(Case.Code);
     TestTU TU = TestTU::withCode(T.code());
     TU.ExtraArgs.push_back("-std=c++17");
     auto AST = TU.build();
     ASSERT_TRUE(AST.getDiagnostics().empty());
 
-    auto H = getHover(AST, T.point(), format::getLLVMStyle());
+    auto H = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
     ASSERT_TRUE(H);
     HoverInfo Expected;
     Expected.SymRange = T.range();
     Case.ExpectedBuilder(Expected);
 
     EXPECT_EQ(H->NamespaceScope, Expected.NamespaceScope);
     EXPECT_EQ(H->LocalScope, Expected.LocalScope);
     EXPECT_EQ(H->Name, Expected.Name);
OneTest Tests[] = {
             int foo(int) {}
             int main() {
               auto *X = &^foo;
             }
           )cpp",
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "int foo(int)\n"
-          "]",
+          "]\n"
+          "text[Function definition via pointer]",
       },
       {
           R"cpp(// Function declaration via call
             int foo(int);
             int main() {
               return ^foo(42);
             }
           )cpp",
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "int foo(int)\n"
-          "]",
+          "]\n"
+          "text[Function declaration via call]",
       },
       {
           R"cpp(// Field
             struct Foo { int x; };
             int main() {
               Foo bar;
               bar.^x;
             }
OneTest Tests[] = {
             typedef int Foo;
             int main() {
               ^Foo bar;
             }
           )cpp",
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "typedef int Foo\n"
-          "]",
+          "]\n"
+          "text[Typedef]",
       },
       {
           R"cpp(// Namespace
             namespace ns {
             struct Foo { static void bar(); }
             } // namespace ns
             int main() { ^ns::Foo::bar(); }
           )cpp",
])cpp",
           R"cpp(// Forward class declaration
             class Foo;
             class Foo {};
             F^oo* foo();
           )cpp",
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "class Foo {}\n"
-          "]",
+          "]\n"
+          "text[Forward class declaration]",
       },
       {
           R"cpp(// Function declaration
             void foo();
             void g() { f^oo(); }
             void foo() {}
           )cpp",
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "void foo()\n"
-          "]",
+          "]\n"
+          "text[Function declaration]",
       },
       {
           R"cpp(// Enum declaration
             enum Hello {
               ONE, TWO, THREE,
             };
             void foo() {
               Hel^lo hello = ONE;
             }
           )cpp",
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "enum Hello {}\n"
-          "]",
+          "]\n"
+          "text[Enum declaration]",
       },
       {
           R"cpp(// Enumerator
             enum Hello {
               ONE, TWO, THREE,
             };
             void foo() {
               Hello hello = O^NE;
])cpp",
             static int hey = 10;
             void foo() {
               he^y++;
             }
           )cpp",
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "static int hey = 10\n"
-          "]",
+          "]\n"
+          "text[Global variable]",
       },
       {
           R"cpp(// Global variable in namespace
             namespace ns1 {
               static int hey = 10;
             }
             void foo() {
               ns1::he^y++;
])cpp",
             T foo() {
               return 17;
             }
             void g() { auto x = f^oo<int>(); }
           )cpp",
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "template <typename T> T foo()\n"
-          "]",
+          "]\n"
+          "text[Templated function]",
       },
       {
           R"cpp(// Anonymous union
             struct outer {
               union {
                 int abc, def;
               } v;
             };
             void g() { struct outer o; o.v.d^ef++; }
           )cpp",
           "text[Declared in]code[outer::(anonymous union)]\n"
           "codeblock(cpp) [\n"
           "int def\n"
           "]",
       },
       {
+          R"cpp(// documentation from index
+            int nextSymbolIsAForwardDeclFromIndexWithNoLocalDocs;
+            void indexSymbol();
+            void g() { ind^exSymbol(); }
+          )cpp",
+          "text[Declared in]code[global namespace]\n"
+          "codeblock(cpp) [\n"
+          "void indexSymbol()\n"
+          "]\n"
+          "text[comment from index]",
+      },
+      {
           R"cpp(// Nothing
             void foo() {
               ^
             }
           )cpp",
           "",
       },
       {
])cpp",
             ^auto (*foo)() = bar;
           )cpp",
           "codeblock(cpp) [\n"
           "int\n"
           "]",
       },
   };
 
+  // Create a tiny index, so tests above can verify documentation is fetched.
+  Symbol IndexSym = func("indexSymbol");
+  IndexSym.Documentation = "comment from index";
+  SymbolSlab::Builder Symbols;
+  Symbols.insert(IndexSym);
+  auto Index =
+      MemIndex::build(std::move(Symbols).build(), RefSlab(), RelationSlab());
+
   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(), format::getLLVMStyle())) {
+    if (auto H =
+            getHover(AST, T.point(), format::getLLVMStyle(), Index.get())) {
       EXPECT_NE("", Test.ExpectedHover) << Test.Input;
       EXPECT_EQ(H->present().renderForTests(), Test.ExpectedHover.str())
           << Test.Input;
     } else
       EXPECT_EQ("", Test.ExpectedHover.str()) << Test.Input;
   }
 }