[clangd] Add a ruler after header in hover

Reviewers: sammccall

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits

Tags: #clang

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

(cherry picked from commit d74a3d470c316f8fade90fe231fc0a51361c01e6)

5 files changed, 103 insertions, 15 deletions

diff --git a/clang-tools-extra/clangd/FormattedString.cpp b/clang-tools-extra/clangd/FormattedString.cpp
index 881c34e0071..9cb732eabcf 100644
--- a/clang-tools-extra/clangd/FormattedString.cpp
+++ b/clang-tools-extra/clangd/FormattedString.cpp
@@ -16,6 +16,7 @@
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstddef>
+#include <iterator>
 #include <memory>
 #include <string>
 #include <vector>
@@ -117,16 +118,52 @@ std::string renderBlocks(llvm::ArrayRef<std::unique_ptr<Block>> Children,
                          void (Block::*RenderFunc)(llvm::raw_ostream &) const) {
   std::string R;
   llvm::raw_string_ostream OS(R);
-  for (auto &C : Children)
+
+  // Trim rulers.
+  Children = Children.drop_while(
+      [](const std::unique_ptr<Block> &C) { return C->isRuler(); });
+  auto Last = llvm::find_if(
+      llvm::reverse(Children),
+      [](const std::unique_ptr<Block> &C) { return !C->isRuler(); });
+  Children = Children.drop_back(Children.end() - Last.base());
+
+  bool LastBlockWasRuler = true;
+  for (const auto &C : Children) {
+    if (C->isRuler() && LastBlockWasRuler)
+      continue;
+    LastBlockWasRuler = C->isRuler();
     ((*C).*RenderFunc)(OS);
-  return llvm::StringRef(OS.str()).trim().str();
+  }
+
+  // Get rid of redundant empty lines introduced in plaintext while imitating
+  // padding in markdown.
+  std::string AdjustedResult;
+  llvm::StringRef TrimmedText(OS.str());
+  TrimmedText = TrimmedText.trim();
+
+  llvm::copy_if(TrimmedText, std::back_inserter(AdjustedResult),
+                [&TrimmedText](const char &C) {
+                  return !llvm::StringRef(TrimmedText.data(),
+                                          &C - TrimmedText.data() + 1)
+                              // We allow at most two newlines.
+                              .endswith("\n\n\n");
+                });
+
+  return AdjustedResult;
 }
 
-// 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 isRuler() const override { return true; }
 };
 
 class CodeBlock : public Block {
@@ -272,7 +309,7 @@ Paragraph &Document::addParagraph() {
   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(
diff --git a/clang-tools-extra/clangd/FormattedString.h b/clang-tools-extra/clangd/FormattedString.h
index 1b7e9fc7521..effd0372592 100644
--- a/clang-tools-extra/clangd/FormattedString.h
+++ b/clang-tools-extra/clangd/FormattedString.h
@@ -33,6 +33,7 @@ public:
   std::string asMarkdown() const;
   std::string asPlainText() const;
 
+  virtual bool isRuler() const { return false; }
   virtual ~Block() = default;
 };
 
@@ -82,8 +83,8 @@ 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");
diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp
index 97e287d6f18..29814181cfe 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -532,6 +532,8 @@ markup::Document HoverInfo::present() const {
     Header.appendCode(*Type);
   }
 
+  // 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`
@@ -555,6 +557,7 @@ markup::Document HoverInfo::present() const {
     Output.addParagraph().appendText(Documentation);
 
   if (!Definition.empty()) {
+    Output.addRuler();
     std::string ScopeComment;
     // Drop trailing "::".
     if (!LocalScope.empty()) {
diff --git a/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp b/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
index 0a85a4cb27d..6489aa05f84 100644
--- a/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
@@ -136,13 +136,37 @@ 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();
+  EXPECT_EQ(D.asMarkdown(), "foo");
+  EXPECT_EQ(D.asPlainText(), "foo");
+
+  // Multiple rulers between blocks
+  D.addRuler();
+  D.addParagraph().appendText("foo");
+  EXPECT_EQ(D.asMarkdown(), "foo  \n\n---\nfoo");
+  EXPECT_EQ(D.asPlainText(), "foo\n\nfoo");
 }
 
 TEST(Document, Heading) {
@@ -182,15 +206,11 @@ foo
 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);
 }
diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp
index 54901faeaf8..bca8dc44cc7 100644
--- a/clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1699,6 +1699,7 @@ TEST(Hover, Present) {
             HI.NamespaceScope.emplace();
           },
           R"(class foo
+
 documentation
 
 template <typename T, typename C = bool> class Foo {})",
@@ -1722,6 +1723,7 @@ template <typename T, typename C = bool> class Foo {})",
             HI.Definition = "ret_type foo(params) {}";
           },
           R"(function foo → ret_type
+
 - 
 - type
 - type foo
@@ -1740,6 +1742,7 @@ ret_type foo(params) {})",
             HI.Definition = "def";
           },
           R"(variable foo : type
+
 Value = value
 
 // In test::bar
@@ -1765,6 +1768,30 @@ TEST(Hover, PresentHeadings) {
   EXPECT_EQ(HI.present().asMarkdown(), "### variable `foo` \\: `type`");
 }
 
+// This is a separate test as rulers behave differently in markdown vs
+// plaintext.
+TEST(Hover, PresentRulers) {
+  HoverInfo HI;
+  HI.Kind = index::SymbolKind::Variable;
+  HI.Name = "foo";
+  HI.Value = "val";
+  HI.Definition = "def";
+
+  EXPECT_EQ(HI.present().asMarkdown(), R"md(### variable `foo`  
+
+---
+Value \= `val`  
+
+---
+```cpp
+def
+```)md");
+  EXPECT_EQ(HI.present().asPlainText(), R"pt(variable foo
+
+Value = val
+
+def)pt");
+}
 } // namespace
 } // namespace clangd
 } // namespace clang