[clangd] Add a ruler after header in hover

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

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

[sammccall]

clang-tools-extra/clangd/Hover.cpp

545

(When merging with D72623, I'm assuming return type will go below this line?)

573

seems to me we'd likely want one above definition too

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

12 ↗

(On Diff #237687)

Why are we getting two blank lines here?

(I understand why this patch increases it by one, but don't understand why it was two before. Ideally I think we never want \n\n\n in plaintext)

[kadircet]

• Increase control around paddings for plaintext rendering.
• Add another ruler before definition.

[kadircet]

• Update comments

[kadircet]

clang-tools-extra/clangd/Hover.cpp

545

yes it will

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

12 ↗

(On Diff #237687)

it was two before because of extra padding around codeblocks.

but agreed that we shouldn't have that much spacing.

[merge_guards_bot]

Unit tests: pass. 61802 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

[merge_guards_bot]

Unit tests: pass. 61802 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

[kadircet]

• Per offline discussions, do post processing to get rid of multiple ruler occurences.

[merge_guards_bot]

Unit tests: pass. 61819 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

[sammccall]

clang-tools-extra/clangd/FormattedString.cpp

121

Having thought about this a bit more:

I think it'd be clearer to apply the ruler-filtering "symbolically", dropping those blocks before attempting rendering. Need isRuler() I guess.
Then something like

vector<Block*> FilteredBlocks;
for (auto& C : Children) {
  if (C->isRuler() && (FilteredBlocks.empty() || FilteredBlocks.back()->isRuler()))
    continue;
  FilteredBlocks.push_back(C.get());
}
while (!FilteredBlocks.empty() && FilteredBlocks.back()->isRuler())
  FilteredBlocks.pop_back();

Whereas I can't see a good way to apply the plaintext-newline rule symbolically, textually seems reasonable:

llvm::erase_if(Text, [&](Char &C) { return StringRef(Text.data(), &C - Text.data()).endswith("\n\n"); });

clang-tools-extra/clangd/FormattedString.h

36

This is a bit weird layering-wise. I think it'd be cleaner just to do a substitution on the rendered output.
Still a hack, but we've got that either way.

[kadircet]

• Address comments

[merge_guards_bot]

Unit tests: pass. 61885 tests passed, 0 failed and 782 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

clang-tools-extra/clangd/FormattedString.h

 /// should trim them if need be.
 class Block {
 public:
   virtual void renderMarkdown(llvm::raw_ostream &OS) const = 0;
   virtual void renderPlainText(llvm::raw_ostream &OS) const = 0;
   std::string asMarkdown() const;
   std::string asPlainText() const;
 
+  /// Padding information to imitate spacing while rendering for plaintext. As
+  /// markdown renderers should already add appropriate padding between
+  /// different blocks.
+  virtual bool hasTrailingPadding() const { return false; }
+  virtual bool requiresPrecedingPadding() const { return false; }
+
   virtual ~Block() = default;
 };
 
 /// Represents parts of the markup that can contain strings, like inline code,
 /// code block or plain text.
 /// One must introduce different paragraphs to create separate blocks.
 class Paragraph : public Block {
 public:

 };
 
 /// A format-agnostic representation for structured text. Allows rendering into
 /// markdown and plaintext.
 class Document {
 public:
   /// Adds a semantical block that will be separate from others.
   Paragraph &addParagraph();
-  /// Inserts a vertical space into the document.
-  void addSpacer();
+  /// Inserts a horizontal separator to the document.
+  void addRuler();
   /// Adds a block of code. This translates to a ``` block in markdown. In plain
   /// text representation, the code block will be surrounded by newlines.
   void addCodeBlock(std::string Code, std::string Language = "cpp");
   /// Heading is a special type of paragraph that will be prepended with \p
   /// Level many '#'s in markdown.
   Paragraph &addHeading(size_t Level);
 
   BulletList &addBulletList();

clang-tools-extra/clangd/FormattedString.cpp

if (WritePtr > Input.begin())
       *WritePtr++ = ' '; // Separate from previous block.
     llvm::for_each(Word, [&WritePtr](const char C) { *WritePtr++ = C; });
   }
   // Get rid of extra spaces.
   Input.resize(WritePtr - Input.begin());
   return Input;
 }
 
+enum RenderType {
+  PlainText,
+  Markdown,
+};
 std::string renderBlocks(llvm::ArrayRef<std::unique_ptr<Block>> Children,
-                         void (Block::*RenderFunc)(llvm::raw_ostream &) const) {
+                         const RenderType RT) {
   std::string R;
   llvm::raw_string_ostream OS(R);
-  for (auto &C : Children)
-    ((*C).*RenderFunc)(OS);
-  return llvm::StringRef(OS.str()).trim().str();
+  // We initially start with true to not introduce redundant paddings at the
+  // beginning.
+  bool HasPadding = true;
+  switch (RT) {
+  case PlainText:
+    llvm::for_each(Children, [&](const std::unique_ptr<Block> &C) {
+      if (C->requiresPrecedingPadding() && !HasPadding)
+        OS << '\n';
+      C->renderPlainText(OS);
+      HasPadding = C->hasTrailingPadding();
+    });
+    break;
+
+  case Markdown:
+    llvm::for_each(Children, [&](const std::unique_ptr<Block> &C) {
+      C->renderMarkdown(OS);
+    });
+    break;
+  }
+  return llvm::StringRef(OS.str()).trim();
 }
 
-// Puts a vertical space between blocks inside a document.
-class Spacer : public Block {
+// Seperates two blocks with extra spacing. Note that it might render strangely
+// in vscode if the trailing block is a codeblock, see
+// https://github.com/microsoft/vscode/issues/88416 for details.
+class Ruler : public Block {
 public:
-  void renderMarkdown(llvm::raw_ostream &OS) const override { OS << '\n'; }
+  void renderMarkdown(llvm::raw_ostream &OS) const override {
+    // Note that we need an extra new line before the ruler, otherwise we might
+    // make previous block a title instead of introducing a ruler.
+    OS << "\n---\n";
+  }
   void renderPlainText(llvm::raw_ostream &OS) const override { OS << '\n'; }
+
+  bool hasTrailingPadding() const override { return true; }
 };
 
 class CodeBlock : public Block {
 public:
   void renderMarkdown(llvm::raw_ostream &OS) const override {
     std::string Marker = getMarkerForCodeBlock(Contents);
     // No need to pad from previous blocks, as they should end with a new line.
     OS << Marker << Language << '\n' << Contents << '\n' << Marker << '\n';
   }
 
   void renderPlainText(llvm::raw_ostream &OS) const override {
-    // In plaintext we want one empty line before and after codeblocks.
-    OS << '\n' << Contents << "\n\n";
+    // In plaintext we print one extra new line to imitate markdown padding.
+    OS << Contents << "\n\n";
   }
 
+  // Code blocks requires a padding before the previous block, and has a padding
+  // afterwards.
+  bool requiresPrecedingPadding() const override { return true; }
+  bool hasTrailingPadding() const override { return true; }
+
   CodeBlock(std::string Contents, std::string Language)
       : Contents(std::move(Contents)), Language(std::move(Language)) {}
 
 private:
   std::string Contents;
   std::string Language;
 };
 
class Document &BulletList::addItem() {
   return Items.back();
 }
 
 Paragraph &Document::addParagraph() {
   Children.push_back(std::make_unique<Paragraph>());
   return *static_cast<Paragraph *>(Children.back().get());
 }
 
-void Document::addSpacer() { Children.push_back(std::make_unique<Spacer>()); }
+void Document::addRuler() { Children.push_back(std::make_unique<Ruler>()); }
 
 void Document::addCodeBlock(std::string Code, std::string Language) {
   Children.emplace_back(
       std::make_unique<CodeBlock>(std::move(Code), std::move(Language)));
 }
 
 std::string Document::asMarkdown() const {
-  return renderBlocks(Children, &Block::renderMarkdown);
+  return renderBlocks(Children, Markdown);
 }
 
 std::string Document::asPlainText() const {
-  return renderBlocks(Children, &Block::renderPlainText);
+  return renderBlocks(Children, PlainText);
 }
 
 BulletList &Document::addBulletList() {
   Children.emplace_back(std::make_unique<BulletList>());
   return *static_cast<BulletList *>(Children.back().get());
 }
 
 Paragraph &Document::addHeading(size_t Level) {
   assert(Level > 0);
   Children.emplace_back(std::make_unique<Heading>(Level));
   return *static_cast<Paragraph *>(Children.back().get());
 }
 } // namespace markup
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/Hover.cpp

markup::Document HoverInfo::present() const {
   if (ReturnType) {
     Header.appendText("→");
     Header.appendCode(*ReturnType);
   } else if (Type) {
     Header.appendText(":");
     Header.appendCode(*Type);
   }
 
+  bool HasMiddleSection = false;
+  // Put a linebreak after header to increase readability.
+  Output.addRuler();
   // For functions we display signature in a list form, e.g.:
   // - `bool param1`
   // - `int param2 = 5`
   if (Parameters && !Parameters->empty()) {
+    HasMiddleSection = true;
     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) {
+    HasMiddleSection = true;
     markup::Paragraph &P = Output.addParagraph();
     P.appendText("Value =");
     P.appendCode(*Value);
   }
 
-  if (!Documentation.empty())
+  if (!Documentation.empty()) {
+    HasMiddleSection = true;
     Output.addParagraph().appendText(Documentation);
+  }
 
   if (!Definition.empty()) {
+    // No need to add an extra ruler if hovercard didn't have a middle section.
+    if (HasMiddleSection)
+      Output.addRuler();
     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';

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

bar)md";
   const char ExpectedText[] = R"pt(foo
 
 test
 
 bar)pt";
   EXPECT_EQ(D.asPlainText(), ExpectedText);
 }
 
-TEST(Document, Spacer) {
+TEST(Document, Ruler) {
   Document D;
   D.addParagraph().appendText("foo");
-  D.addSpacer();
+  D.addRuler();
+
+  // Ruler followed by paragraph.
   D.addParagraph().appendText("bar");
-  EXPECT_EQ(D.asMarkdown(), "foo  \n\nbar");
+  EXPECT_EQ(D.asMarkdown(), "foo  \n\n---\nbar");
+  EXPECT_EQ(D.asPlainText(), "foo\n\nbar");
+
+  D = Document();
+  D.addParagraph().appendText("foo");
+  D.addRuler();
+  D.addCodeBlock("bar");
+  // Ruler followed by a codeblock.
+  EXPECT_EQ(D.asMarkdown(), "foo  \n\n---\n```cpp\nbar\n```");
   EXPECT_EQ(D.asPlainText(), "foo\n\nbar");
+
+  // Ruler followed by another ruler
+  D = Document();
+  D.addParagraph().appendText("foo");
+  D.addRuler();
+  D.addRuler();
+  // FIXME: Should we trim trailing rulers also in markdown?
+  EXPECT_EQ(D.asMarkdown(), "foo  \n\n---\n\n---");
+  EXPECT_EQ(D.asPlainText(), "foo");
 }
 
 TEST(Document, Heading) {
   Document D;
   D.addHeading(1).appendText("foo");
   D.addHeading(2).appendText("bar");
   D.addParagraph().appendText("baz");
   EXPECT_EQ(D.asMarkdown(), "# foo  \n## bar  \nbaz");

 foo
   bar
   baz
 ```
 ```cpp
 foo
 ```)md";
   EXPECT_EQ(D.asMarkdown(), ExpectedMarkdown);
-  // FIXME: we shouldn't have 2 empty lines in between. A solution might be
-  // having a `verticalMargin` method for blocks, and let container insert new
-  // lines according to that before/after blocks.
   ExpectedPlainText =
       R"pt(foo
   bar
   baz
 
-
 foo)pt";
   EXPECT_EQ(D.asPlainText(), ExpectedPlainText);
 }
 
 TEST(BulletList, Render) {
   BulletList L;
   // Flat list
   L.addItem().addParagraph().appendText("foo");

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

llvm::StringRef ExpectedRender;
             };
             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)",
       },
       {
           [](HoverInfo &HI) {
             HI.Type = "type";

 // This is a separate test as headings don't create any differences in plaintext
 // mode.
 TEST(Hover, PresentHeadings) {
   HoverInfo HI;
   HI.Kind = index::SymbolKind::Variable;
   HI.Name = "foo";
   HI.Type = "type";
 
-  EXPECT_EQ(HI.present().asMarkdown(), "### variable `foo` \\: `type`");
+  EXPECT_EQ(HI.present().asMarkdown(),
+            "### variable `foo` \\: `type`  \n\n---");
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang