[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

419

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).

485–496

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

485–496

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

486

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.

489

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

494

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

501

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

506

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.

517

consider "Value = " or even just "= "

530

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

}

536

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

538

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

540

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)

542

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

545

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

419

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.

486

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)

489

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?

494

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

538

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

540

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

545

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

486

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.

489

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)

490

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

545

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

486

got rid of beautify, printing getSymbolKindString as is.

490

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

Changing it accordingly.

517

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

517

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"
 
 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 (!Invalid) {
       if (EndOffset <= Buffer.size() && StartOffset < EndOffset)
         HI.Definition =
             ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))
                 .str();
     }
   }
   return HI;
 }
+
+// Converts a string of the form `word1-word2-...` into `Word1 Word2 ...`.
+std::string beautify(llvm::StringRef Input) {
+  std::string Res;
+  llvm::SmallVector<llvm::StringRef, 2> Out;
+  llvm::SplitString(Input, Out, "-");
+  llvm::for_each(Out, [&Res](llvm::StringRef Word) {
+    if (!Res.empty())
+      Res += ' ';
+    Res += llvm::toUpper(Word.front());
+    Res.append(Word.drop_front());
+  });
+  return Res;
+}
 } // 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(
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`
+  // or
+  // `class X`
+  markup::Paragraph &Header = Output.addParagraph();
+  Header.appendText(beautify(index::getSymbolKindString(Kind)));
+  if (!Name.empty()) {
+    Header.appendCode(Name);
+    if (Type && !ReturnType) {
+      Header.appendText(":");
+      Header.appendCode(*Type);
+    }
   }
 
-  if (!Definition.empty()) {
-    Output.addCodeBlock(Definition);
-  } else {
-    // Builtin types
-    Output.addCodeBlock(Name);
+  // For functions we display signature in a list form, e.g.:
+  // Returns: `int`
+  // - `bool param1`
+  // - `int param2 = 5`
+  if (ReturnType) {
+    markup::Paragraph &P = Output.addParagraph();
+    P.appendText("Returns: ");
+    P.appendCode(*ReturnType);
+  }
+  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 (Value) {
+    markup::Paragraph &P = Output.addParagraph();
+    P.appendText("Value: ");
+    P.appendCode(*Value);
   }
 
   if (!Documentation.empty())
     Output.addParagraph().appendText(Documentation);
+
+  if (!Definition.empty()) {
+    std::string Code;
+    llvm::raw_string_ostream OS(Code);
+    // 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.
+      OS << "// In " << llvm::StringRef(LocalScope).rtrim(':') << '\n';
+    } else if (NamespaceScope && !NamespaceScope->empty()) {
+      OS << "// In namespace " << llvm::StringRef(*NamespaceScope).rtrim(':')
+         << '\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.
+    OS << Definition;
+    Output.addCodeBlock(OS.str());
+  }
   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)

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\nReturns: 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; },
+          R"(<unknown>)",
+      },
+      {
+          [](HoverInfo &HI) {
+            HI.Kind = index::SymbolKind::Class;
+            HI.Name = "foo";
+          },
+          R"(Class foo)",
+      },
+      {
+          [](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
+Returns: 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