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

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)

Reviewers: kadircet

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D64296

llvm-svn: 365522

diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index f76e752..8db8552 100644 (file)
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -499,13 +499,13 @@ void ClangdServer::findDocumentHighlights(
 
 void ClangdServer::findHover(PathRef File, Position Pos,
                              Callback<llvm::Optional<HoverInfo>> CB) {
-  auto Action = [Pos](decltype(CB) CB, Path File,
-                      llvm::Expected<InputsAndAST> InpAST) {
+  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,

diff --git a/clang-tools-extra/clangd/FormattedString.cpp b/clang-tools-extra/clangd/FormattedString.cpp
index 3be179b..102612a 100644 (file)
--- a/clang-tools-extra/clangd/FormattedString.cpp
+++ b/clang-tools-extra/clangd/FormattedString.cpp
@@ -89,14 +89,10 @@ static std::string renderCodeBlock(llvm::StringRef Input,
 } // 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;
+  Chunk C;
+  C.Kind = ChunkKind::PlainText;
+  C.Contents = Text;
+  Chunks.push_back(C);
 }
 
 void FormattedString::appendCodeBlock(std::string Code, std::string Language) {
@@ -146,28 +142,30 @@ std::string FormattedString::renderAsPlainText() const {
       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;
+      break;
     }
-    llvm_unreachable("unhanlded ChunkKind");
+    // Trim trailing whitespace in chunk.
+    while (!R.empty() && isWhitespace(R.back()))
+      R.pop_back();
   }
-  while (!R.empty() && isWhitespace(R.back()))
-    R.pop_back();
   return R;
 }
 

diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp
index 806d7b8..17a5a69 100644 (file)
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -14,6 +14,7 @@
 #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"
@@ -601,8 +602,32 @@ static const FunctionDecl *getUnderlyingFunction(const Decl *D) {
   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();
 
@@ -697,19 +722,22 @@ static HoverInfo getHoverContents(const Decl *D) {
   }
 
   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;
 }
 
@@ -859,7 +887,8 @@ bool hasDeducedType(ParsedAST &AST, SourceLocation SourceLocationBeg) {
 }
 
 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());
@@ -869,7 +898,7 @@ llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
   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;
@@ -878,7 +907,7 @@ llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
     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(
@@ -1225,6 +1254,9 @@ FormattedString HoverInfo::present() const {
     // Builtin types
     Output.appendCodeBlock(Name);
   }
+
+  if (!Documentation.empty())
+    Output.appendText(Documentation);
   return Output;
 }
 

diff --git a/clang-tools-extra/clangd/XRefs.h b/clang-tools-extra/clangd/XRefs.h
index 7016463..184cbef 100644 (file)
--- a/clang-tools-extra/clangd/XRefs.h
+++ b/clang-tools-extra/clangd/XRefs.h
@@ -118,7 +118,8 @@ inline bool operator==(const HoverInfo::Param &LHS,
 
 /// 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).

diff --git a/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp b/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
index 2159de9..da19f89 100644 (file)
--- a/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -21,9 +21,10 @@ TEST(FormattedString, Basic) {
   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");
@@ -42,15 +43,21 @@ 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();

diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
index f795ef7..d77f0a2 100644 (file)
--- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -12,9 +12,11 @@
 #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"
@@ -987,7 +989,7 @@ void foo())cpp";
     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();
@@ -1101,7 +1103,8 @@ TEST(Hover, All) {
           "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
@@ -1113,7 +1116,8 @@ TEST(Hover, All) {
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "int foo(int)\n"
-          "]",
+          "]\n"
+          "text[Function declaration via call]",
       },
       {
           R"cpp(// Field
@@ -1224,7 +1228,8 @@ TEST(Hover, All) {
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "typedef int Foo\n"
-          "]",
+          "]\n"
+          "text[Typedef]",
       },
       {
           R"cpp(// Namespace
@@ -1294,7 +1299,8 @@ TEST(Hover, All) {
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "class Foo {}\n"
-          "]",
+          "]\n"
+          "text[Forward class declaration]",
       },
       {
           R"cpp(// Function declaration
@@ -1305,7 +1311,8 @@ TEST(Hover, All) {
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "void foo()\n"
-          "]",
+          "]\n"
+          "text[Function declaration]",
       },
       {
           R"cpp(// Enum declaration
@@ -1319,7 +1326,8 @@ TEST(Hover, All) {
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "enum Hello {}\n"
-          "]",
+          "]\n"
+          "text[Enum declaration]",
       },
       {
           R"cpp(// Enumerator
@@ -1359,7 +1367,8 @@ TEST(Hover, All) {
           "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
@@ -1400,7 +1409,8 @@ TEST(Hover, All) {
           "text[Declared in]code[global namespace]\n"
           "codeblock(cpp) [\n"
           "template <typename T> T foo()\n"
-          "]",
+          "]\n"
+          "text[Templated function]",
       },
       {
           R"cpp(// Anonymous union
@@ -1417,6 +1427,18 @@ TEST(Hover, All) {
           "]",
       },
       {
+          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() {
               ^
@@ -1773,12 +1795,21 @@ TEST(Hover, All) {
       },
   };
 
+  // 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;