[clangd] Refurbish HoverInfo::present

[kadircet]

Summary

Improves basic hover presentation logic to include more info.

[kadircet]

• Get rid of irrelevant change

[merge_guards_bot]

clang-format: pass.

Build artifacts: diff.json, CMakeCache.txt, console-log.txt

[merge_guards_bot]

clang-format: pass.

Build artifacts: diff.json, CMakeCache.txt, console-log.txt

[sammccall]

clang-tools-extra/clangd/Hover.cpp

418

It's worth noting that an alternative to mechanically transforming would simply be to change the getSymbolKindString(). It's only used in tests (we'd have to update the tests though).

470–485

Can we give P a real names like "Header"?

470–485

maybe add a comment with an example, to illustrate what this section does like
// variable x : int

471

is beautify() actually better here? I actually quite like the lowercase as it reduces the emphasis on the kind vs the name, and the hyphens as it makes it easier to pick out the name.

This is up to you of course.

474

if ReturnType exists and non-void, we could consider -> or → in place of :.

479

what *is* the case where something has a type but no name?

490

consider "Returns " without the colon, or "-> " or "→"

495

Random aside: If we could customize the bullet character, making it something like ← would somewhat compensate for the lack of header here.
In plain text this would be fairly easy (need to add the API) but markdown is trickier of course - can only be done through CSS, and there's no standard way to add a CSS class.

506

consider "Value = " or even just "= "

513

The assumption that unset namespace scope means no local scope etc is probably true, but seems fairly fragile/coupled.

What about
if (LocalScope && !LocalScope->empty()) {

// local scope

} else if (NamespaceScope && !NamespaceScope->empty())) {

// namespace scope

} else if (NamespaceScope) {

// global namespace

}

519

rtrim(':') would be clearer and more robust I think

521

Either " in namespace ..." or " namespace ... {" or "namespace ... {" ?

523

either "// in global namespace" or nothing at all here?

(This text is probably annoying for non-C++ projects, or C++ projects that don't use namespaces - I'd be tempted to let the absence of a NS stand for itself)

525

nit: seems clearer/less fragile to put this on each comment

528

If you're going to use comment syntax, shouldn't the comment be part of the code block?

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

1522

This is where I really miss the render-for-tests function.

I don't think just testing plain-text is really enough, as it doesn't e.g. distinguish code from non code (such a bug exists invisibly in these tests in fact).

On the other hand, the exact markdown string generated doesn't matter as long as it has the right structure, so asserting on it is unnecessarily fragile. Might still be the right option.

1522

Generally I think there are too many tests here, and they are too small, and so don't cover enough. I think we want to test

• output for all the attributes (this is covered)
• how they combine with each other (this is mostly not covered)
• that present/absent both work

So maybe 3-4 cases:

• a class with template parameters, documentation and definition, in global NS
• a function with type, return type and params, in a namespace
• a variable with type, in a function or class, with a value

should about cover it?

1527

I think this comment belongs on the test function rather than inside the array

1530

Just R"()"? The pt seems like noise.

1542

what does this test incrementally to the previous test case?

1549

this example is nonsensical I think? Classes don't have a type. Same for some below.

1577

This tests parameter ordering in isolation, but not e.g. how the parameter list is ordered with respect to the other sections.

[kadircet]

clang-tools-extra/clangd/Hover.cpp

418

or we can have our own SymbolKind to string conversion. I would be OK with doing that(rather than changing getSymbolKindString) if you believe mechanical conversion isn't enough.

471

I've actually found those hyphens annoying, but don't have any strong feelings towards those. I would land it this way and see how ppl feel about it. (but of course, if you've got any strong feelings we could also do it the other way around)

474

we've got a special Returns: x paragraph for function-likes, therefore I wasn't printing anything after the name for functions.

but thinking about it now, actually it makes sense to drop that line and print something like:

Function `foo` -> `int`
- `bool param1`

wdyt?

479

there's none, it was a leftover from old revisions, nvm.

521

going with In namespace ..., having a { in the end without an enclosing } doesn't seem right.

523

agreed, dropped that case and put some comments to explain why.

528

it is, I am putting everything to stream and then creating a codeblock out of it.

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

1542

it was rather for checking replacement of hyphens.

[kadircet]

• Address comments

[merge_guards_bot]

Unit tests: pass. 61307 tests passed, 0 failed and 736 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[sammccall]

Great stuff, let's finally ship it!

clang-tools-extra/clangd/Hover.cpp

471

ok, that's fine. If you don't care much about the case, consider dropping the case transform and just replacing - with , it seems less noisy. But up to you, fine to leave as-is.

474

Yeah, I think that'd be better and more consistent with Type.
(except in cases where the name and/or return type are long, but that's probably equally true with Type/ReturnType)

475

if there's no name, do we want to skip the header entirely?
(I can't think of examples for this case.)

528

Right, sorry!

nit: OS here seems a little obscure/overkill, could be just

std::string ScopeComment;
if (...) {
  ScopeComment = "//...\n";
}
Output.addCodeBlock(ScopeComment + Definition);

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

1542

right - maybe delete the "class foo" test then?

1630

I do find the uppercase A next to the lowercase foo here jarring.

This looks like "title case", but in title case the filler words are lowercase and the "important" ones are capitalized, and here we'd have the opposite.

"Namespace alias foo" or "namespace alias foo" both seem fine.

[kadircet]

Address comments

clang-tools-extra/clangd/Hover.cpp

471

got rid of beautify, printing getSymbolKindString as is.

475

HoverInfo::Name is always filled currently, as we always get a NamedDecl(after change to explicitReferenceTargets).

Changing it accordingly.

506

sorry somehow missed that one.

what about putting that into header part with something like:

variable `var` : `int`(=3)

function `foo` -> `int`(=3)

we can also drop equals signs, I am not sure if it looks confusing without those.

[merge_guards_bot]

Unit tests: pass. 61307 tests passed, 0 failed and 736 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[sammccall]

still LG, go ahead and we can iterate

clang-tools-extra/clangd/Hover.cpp

506

I'm not sure if this is a good idea as all of name, type, and value can be long. If we overrun and end up wrapping, I think it's less clear than a separate paragraph.

That said if you do go this way, I'd vote to keep the = and drop the (), I think that's clear and terse

[kadircet]

• Change Value: to Value =

[merge_guards_bot]

Unit tests: pass. 61307 tests passed, 0 failed and 736 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

clang-tools-extra/clangd/Hover.cpp

 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/iterator_range.h"
-#include "llvm/Support/Casting.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
+#include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 PrintingPolicy printingPolicyForDecls(PrintingPolicy Base) {
   PrintingPolicy Policy(Base);
 
void enhanceFromIndex(HoverInfo &Hover, const NamedDecl &ND,
   LookupRequest Req;
   Req.IDs.insert(*ID);
   Index->lookup(
       Req, [&](const Symbol &S) { Hover.Documentation = S.Documentation; });
 }
 
 // Populates Type, ReturnType, and Parameters for function-like decls.
 void fillFunctionTypeAndParams(HoverInfo &HI, const Decl *D,
-                                      const FunctionDecl *FD,
-                                      const PrintingPolicy &Policy) {
   HI.Parameters.emplace();
   for (const ParmVarDecl *PVD : FD->parameters()) {
     HI.Parameters->emplace_back();
     auto &P = HI.Parameters->back();
     if (!PVD->getType().isNull()) {
       P.Type.emplace();
       llvm::raw_string_ostream OS(*P.Type);
       PVD->getType().print(OS, Policy);
     } else {
       std::string Param;
       llvm::raw_string_ostream OS(Param);
       PVD->dump(OS);
       OS.flush();
       elog("Got param with null type: {0}", Param);
     }
     if (!PVD->getName().empty())
       P.Name = PVD->getNameAsString();
     if (PVD->hasDefaultArg()) {
       P.Default.emplace();
       llvm::raw_string_ostream Out(*P.Default);
       PVD->getDefaultArg()->printPretty(Out, nullptr, Policy);
     }
   }
 
-  if (const auto* CCD = llvm::dyn_cast<CXXConstructorDecl>(FD)) {
+  if (const auto *CCD = llvm::dyn_cast<CXXConstructorDecl>(FD)) {
     // Constructor's "return type" is the class type.
     HI.ReturnType = declaredType(CCD->getParent()).getAsString(Policy);
     // Don't provide any type for the constructor itself.
-  } else if (llvm::isa<CXXDestructorDecl>(FD)){
+  } else if (llvm::isa<CXXDestructorDecl>(FD)) {
     HI.ReturnType = "void";
   } else {
     HI.ReturnType = FD->getReturnType().getAsString(Policy);
 
     QualType FunctionType = FD->getType();
     if (const VarDecl *VD = llvm::dyn_cast<VarDecl>(D)) // Lambdas
       FunctionType = VD->getType().getDesugaredType(D->getASTContext());
     HI.Type = FunctionType.getAsString(Policy);
if (const Expr *E = N->ASTNode.get<Expr>()) {
       // This tries to ensure we're showing a value related to the cursor.
       break;
     }
   }
   return llvm::None;
 }
 
 /// Generate a \p Hover object given the declaration \p D.
-HoverInfo getHoverContents(const Decl *D, const SymbolIndex *Index) {
+HoverInfo getHoverContents(const NamedDecl *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())
     HI.LocalScope.append("::");
 
   PrintingPolicy Policy = printingPolicyForDecls(Ctx.getPrintingPolicy());
-  if (const NamedDecl *ND = llvm::dyn_cast<NamedDecl>(D)) {
-    HI.Name = printName(Ctx, *ND);
-    ND = getDeclForComment(ND);
-    HI.Documentation = getDeclComment(Ctx, *ND);
-    enhanceFromIndex(HI, *ND, Index);
-  }
+  HI.Name = printName(Ctx, *D);
+  const auto *CommentD = getDeclForComment(D);
+  HI.Documentation = getDeclComment(Ctx, *CommentD);
+  enhanceFromIndex(HI, *CommentD, Index);
 
   HI.Kind = index::getSymbolInfo(D).Kind;
 
   // Fill in template params.
   if (const TemplateDecl *TD = D->getDescribedTemplate()) {
     HI.TemplateParameters =
         fetchTemplateParameters(TD->getTemplateParameters(), Policy);
     D = TD;
if (!Invalid) {
         HI.Definition =
             ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))
                 .str();
     }
   }
   return HI;
 }
 } // namespace
 
 llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
                                    format::FormatStyle Style,
                                    const SymbolIndex *Index) {
   const SourceManager &SM = AST.getSourceManager();
   llvm::Optional<HoverInfo> HI;
   SourceLocation SourceLocationBeg = SM.getMacroArgExpandedLocation(
       getBeginningOfIdentifier(Pos, SM, AST.getLangOpts()));
 
if (!HI)
     return llvm::None;
 
   auto Replacements = format::reformat(
       Style, HI->Definition, tooling::Range(0, HI->Definition.size()));
   if (auto Formatted =
           tooling::applyAllReplacements(HI->Definition, Replacements))
     HI->Definition = *Formatted;
 
-  HI->SymRange = getTokenRange(AST.getSourceManager(),
-                               AST.getLangOpts(), SourceLocationBeg);
+  HI->SymRange = getTokenRange(AST.getSourceManager(), AST.getLangOpts(),
+                               SourceLocationBeg);
   return HI;
 }
 
 markup::Document HoverInfo::present() const {
   markup::Document Output;
-  if (NamespaceScope) {
-    auto &P = Output.addParagraph();
-    P.appendText("Declared in");
-    // Drop trailing "::".
-    if (!LocalScope.empty())
-      P.appendCode(llvm::StringRef(LocalScope).drop_back(2));
-    else if (NamespaceScope->empty())
-      P.appendCode("global namespace");
-    else
-      P.appendCode(llvm::StringRef(*NamespaceScope).drop_back(2));
+  // Header contains a text of the form:
+  // variable `var` : `int`
+  //
+  // class `X`
+  //
+  // function `foo` → `int`
+  markup::Paragraph &Header = Output.addParagraph();
+  Header.appendText(index::getSymbolKindString(Kind));
+  assert(!Name.empty() && "hover triggered on a nameless symbol");
+  Header.appendCode(Name);
+  if (ReturnType) {
+    Header.appendText("→");
+    Header.appendCode(*ReturnType);
+  } else if (Type) {
+    Header.appendText(":");
+    Header.appendCode(*Type);
+  }
+
+  // For functions we display signature in a list form, e.g.:
+  // - `bool param1`
+  // - `int param2 = 5`
+  if (Parameters && !Parameters->empty()) {
+    markup::BulletList &L = Output.addBulletList();
+    for (const auto &Param : *Parameters) {
+      std::string Buffer;
+      llvm::raw_string_ostream OS(Buffer);
+      OS << Param;
+      L.addItem().addParagraph().appendCode(std::move(OS.str()));
+    }
   }
 
-  if (!Definition.empty()) {
-    Output.addCodeBlock(Definition);
-  } else {
-    // Builtin types
-    Output.addCodeBlock(Name);
+  if (Value) {
+    markup::Paragraph &P = Output.addParagraph();
+    P.appendText("Value: ");
+    P.appendCode(*Value);
   }
 
   if (!Documentation.empty())
     Output.addParagraph().appendText(Documentation);
+
+  if (!Definition.empty()) {
+    std::string ScopeComment;
+    // Drop trailing "::".
+    if (!LocalScope.empty()) {
+      // Container name, e.g. class, method, function.
+      // We might want to propogate some info about container type to print
+      // function foo, class X, method X::bar, etc.
+      ScopeComment =
+          "// In " + llvm::StringRef(LocalScope).rtrim(':').str() + '\n';
+    } else if (NamespaceScope && !NamespaceScope->empty()) {
+      ScopeComment = "// In namespace " +
+                     llvm::StringRef(*NamespaceScope).rtrim(':').str() + '\n';
+    }
+    // Note that we don't print anything for global namespace, to not annoy
+    // non-c++ projects or projects that are not making use of namespaces.
+    Output.addCodeBlock(ScopeComment + Definition);
+  }
   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);
   if (P.Name)
     Output.push_back(*P.Name);
   OS << llvm::join(Output, " ");
   if (P.Default)
     OS << " = " << *P.Default;
   return OS;
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/test/hover.test

 # RUN: clangd -lit-test < %s | FileCheck %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
 ---
 {"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":0,"character":27}}}
 #      CHECK:  "id": 1,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "contents": {
 # CHECK-NEXT:      "kind": "plaintext",
-# CHECK-NEXT:      "value": "Declared in global namespace\n\nvoid foo()"
+# CHECK-NEXT:      "value": "function foo → void\n\nvoid foo()"
 # CHECK-NEXT:    },
 # CHECK-NEXT:    "range": {
 # CHECK-NEXT:      "end": {
 # CHECK-NEXT:        "character": 28,
 # CHECK-NEXT:        "line": 0
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "start": {
 # CHECK-NEXT:        "character": 25,

 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main2.cpp","languageId":"cpp","version":1,"text":"enum foo{}; int main() { foo f; }\n"}}}
 ---
 {"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"test:///main2.cpp"},"position":{"line":0,"character":27}}}
 #      CHECK:  "id": 1,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "contents": {
 # CHECK-NEXT:      "kind": "plaintext",
-# CHECK-NEXT:      "value": "Declared in global namespace\n\nenum foo {}"
+# CHECK-NEXT:      "value": "enum foo\n\nenum foo {}"
 # CHECK-NEXT:    },
 # CHECK-NEXT:    "range": {
 # CHECK-NEXT:      "end": {
 # CHECK-NEXT:        "character": 28,
 # CHECK-NEXT:        "line": 0
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "start": {
 # CHECK-NEXT:        "character": 25,

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

for (const auto &Case : Cases) {
     EXPECT_EQ(H->ReturnType, Expected.ReturnType);
     EXPECT_EQ(H->Parameters, Expected.Parameters);
     EXPECT_EQ(H->TemplateParameters, Expected.TemplateParameters);
     EXPECT_EQ(H->SymRange, Expected.SymRange);
     EXPECT_EQ(H->Value, Expected.Value);
   }
 }
 
 TEST(Hover, DocsFromIndex) {
   Annotations T(R"cpp(
   template <typename T> class X {};
   void foo() {
     au^to t = X<int>();
     X^<int> w;
     (void)w;
   })cpp");
 
   TestTU TU = TestTU::withCode(T.code());
   auto AST = TU.build();
   for (const auto &D : AST.getDiagnostics())
     ADD_FAILURE() << D;
   ASSERT_TRUE(AST.getDiagnostics().empty());
 
   Symbol IndexSym;
   IndexSym.ID = *getSymbolID(&findDecl(AST, "X"));
   IndexSym.Documentation = "comment from index";
   SymbolSlab::Builder Symbols;
   Symbols.insert(IndexSym);
   auto Index =
       MemIndex::build(std::move(Symbols).build(), RefSlab(), RelationSlab());
 
   for (const auto &P : T.points()) {
     auto H = getHover(AST, P, format::getLLVMStyle(), Index.get());
     ASSERT_TRUE(H);
     EXPECT_EQ(H->Documentation, IndexSym.Documentation);
   }
 }
 
 TEST(Hover, DocsFromAST) {
   Annotations T(R"cpp(
   // doc
   template <typename T> class X {};
   // doc
   template <typename T> void bar() {}
TEST(Hover, DocsFromAST) {
   auto AST = TU.build();
   for (const auto &D : AST.getDiagnostics())
     ADD_FAILURE() << D;
   ASSERT_TRUE(AST.getDiagnostics().empty());
 
   for (const auto &P : T.points()) {
     auto H = getHover(AST, P, format::getLLVMStyle(), nullptr);
     ASSERT_TRUE(H);
     EXPECT_EQ(H->Documentation, "doc");
   }
 }
 
 TEST(Hover, DocsFromMostSpecial) {
   Annotations T(R"cpp(
   // doc1
   template <typename T> class $doc1^X {};
   // doc2
TEST(Hover, DocsFromMostSpecial) {
   for (auto Comment : {"doc1", "doc2", "doc3"}) {
     for (const auto &P : T.points(Comment)) {
       auto H = getHover(AST, P, format::getLLVMStyle(), nullptr);
       ASSERT_TRUE(H);
       EXPECT_EQ(H->Documentation, Comment);
     }
   }
 }
+TEST(Hover, Present) {
+  struct {
+    const std::function<void(HoverInfo &)> Builder;
+    llvm::StringRef ExpectedRender;
+  } Cases[] = {
+      {
+          [](HoverInfo &HI) {
+            HI.Kind = index::SymbolKind::Unknown;
+            HI.Name = "X";
+          },
+          R"(<unknown> X)",
+      },
+      {
+          [](HoverInfo &HI) {
+            HI.Kind = index::SymbolKind::NamespaceAlias;
+            HI.Name = "foo";
+          },
+          R"(namespace-alias foo)",
+      },
+      {
+          [](HoverInfo &HI) {
+            HI.Kind = index::SymbolKind::Class;
+            HI.TemplateParameters = {
+                {std::string("typename"), std::string("T"), llvm::None},
+                {std::string("typename"), std::string("C"),
+                 std::string("bool")},
+            };
+            HI.Documentation = "documentation";
+            HI.Definition =
+                "template <typename T, typename C = bool> class Foo {}";
+            HI.Name = "foo";
+            HI.NamespaceScope.emplace();
+          },
+          R"(class foo
+documentation
+
+template <typename T, typename C = bool> class Foo {})",
+      },
+      {
+          [](HoverInfo &HI) {
+            HI.Kind = index::SymbolKind::Function;
+            HI.Name = "foo";
+            HI.Type = "type";
+            HI.ReturnType = "ret_type";
+            HI.Parameters.emplace();
+            HoverInfo::Param P;
+            HI.Parameters->push_back(P);
+            P.Type = "type";
+            HI.Parameters->push_back(P);
+            P.Name = "foo";
+            HI.Parameters->push_back(P);
+            P.Default = "default";
+            HI.Parameters->push_back(P);
+            HI.NamespaceScope = "ns::";
+            HI.Definition = "ret_type foo(params) {}";
+          },
+          R"(function foo → ret_type
+- 
+- type
+- type foo
+- type foo = default
+
+// In namespace ns
+ret_type foo(params) {})",
+      },
+      {
+          [](HoverInfo &HI) {
+            HI.Kind = index::SymbolKind::Variable;
+            HI.LocalScope = "test::bar::";
+            HI.Value = "value";
+            HI.Name = "foo";
+            HI.Type = "type";
+            HI.Definition = "def";
+          },
+          R"(variable foo : type
+Value: value
+
+// In test::bar
+def)",
+      },
+  };
+
+  for (const auto &C : Cases) {
+    HoverInfo HI;
+    C.Builder(HI);
+    EXPECT_EQ(HI.present().asPlainText(), C.ExpectedRender);
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang