[clangd] Fix whitespace between chunks in markdown paragraphs.

Summary:
Old model: chunks are always separated by one space.
           This makes it impossible to render "Foo `bar`." correctly.

New model: chunks are separated by space if the left had trailing space, or
           the right had leading space, or space was explicitly requested.
           (Only leading/trailing space in plaintext chunks count, not code)

Reviewers: kadircet

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

Tags: #clang

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

clang-tools-extra/clangd/FormattedString.cpp

@@ -346,18 +346,21 @@ std::string Block::asPlainText() const {
 }
 
 void Paragraph::renderMarkdown(llvm::raw_ostream &OS) const {
-  llvm::StringRef Sep = "";
+  bool NeedsSpace = false;
+  bool HasChunks = false;
   for (auto &C : Chunks) {
-    OS << Sep;
+    if (C.SpaceBefore || NeedsSpace)
+      OS << " ";
     switch (C.Kind) {
     case Chunk::PlainText:
-      OS << renderText(C.Contents, Sep.empty());
+      OS << renderText(C.Contents, !HasChunks);
       break;
     case Chunk::InlineCode:
       OS << renderInlineBlock(C.Contents);
       break;
     }
-    Sep = " ";
+    HasChunks = true;
+    NeedsSpace = C.SpaceAfter;
   }
   // Paragraphs are translated into markdown lines, not markdown paragraphs.
   // Therefore it only has a single linebreak afterwards.
@@ -381,13 +384,15 @@ llvm::StringRef chooseMarker(llvm::ArrayRef<llvm::StringRef> Options,
 }
 
 void Paragraph::renderPlainText(llvm::raw_ostream &OS) const {
-  llvm::StringRef Sep = "";
+  bool NeedsSpace = false;
   for (auto &C : Chunks) {
+    if (C.SpaceBefore || NeedsSpace)
+      OS << " ";
     llvm::StringRef Marker = "";
     if (C.Preserve && C.Kind == Chunk::InlineCode)
       Marker = chooseMarker({"`", "'", "\""}, C.Contents);
-    OS << Sep << Marker << C.Contents << Marker;
-    Sep = " ";
+    OS << Marker << C.Contents << Marker;
+    NeedsSpace = C.SpaceAfter;
   }
   OS << '\n';
 }
@@ -410,6 +415,12 @@ void BulletList::renderPlainText(llvm::raw_ostream &OS) const {
   }
 }
 
+Paragraph &Paragraph::appendSpace() {
+  if (!Chunks.empty())
+    Chunks.back().SpaceAfter = true;
+  return *this;
+}
+
 Paragraph &Paragraph::appendText(llvm::StringRef Text) {
   std::string Norm = canonicalizeSpaces(Text);
   if (Norm.empty())
@@ -418,10 +429,14 @@ Paragraph &Paragraph::appendText(llvm::StringRef Text) {
   Chunk &C = Chunks.back();
   C.Contents = std::move(Norm);
   C.Kind = Chunk::PlainText;
+  C.SpaceBefore = isWhitespace(Text.front());
+  C.SpaceAfter = isWhitespace(Text.back());
   return *this;
 }
 
 Paragraph &Paragraph::appendCode(llvm::StringRef Code, bool Preserve) {
+  bool AdjacentCode =
+      !Chunks.empty() && Chunks.back().Kind == Chunk::InlineCode;
   std::string Norm = canonicalizeSpaces(std::move(Code));
   if (Norm.empty())
     return *this;
@@ -430,6 +445,8 @@ Paragraph &Paragraph::appendCode(llvm::StringRef Code, bool Preserve) {
   C.Contents = std::move(Norm);
   C.Kind = Chunk::InlineCode;
   C.Preserve = Preserve;
+  // Disallow adjacent code spans without spaces, markdown can't render them.
+  C.SpaceBefore = AdjacentCode;
   return *this;
 }
 

clang-tools-extra/clangd/FormattedString.h

@@ -54,6 +54,10 @@ public:
   /// \p Preserve indicates the code span must be apparent even in plaintext.
   Paragraph &appendCode(llvm::StringRef Code, bool Preserve = false);
 
+  /// Ensure there is space between the surrounding chunks.
+  /// Has no effect at the beginning or end of a paragraph.
+  Paragraph &appendSpace();
+
 private:
   struct Chunk {
     enum {
@@ -63,6 +67,11 @@ private:
     // Preserve chunk markers in plaintext.
     bool Preserve = false;
     std::string Contents;
+    // Whether this chunk should be surrounded by whitespace.
+    // Consecutive SpaceAfter and SpaceBefore will be collapsed into one space.
+    // Code spans don't usually set this: their spaces belong "inside" the span.
+    bool SpaceBefore = false;
+    bool SpaceAfter = false;
   };
   std::vector<Chunk> Chunks;
 };

clang-tools-extra/clangd/Hover.cpp

@@ -773,7 +773,7 @@ markup::Document HoverInfo::present() const {
   // https://github.com/microsoft/vscode/issues/88417 for details.
   markup::Paragraph &Header = Output.addHeading(3);
   if (Kind != index::SymbolKind::Unknown)
-    Header.appendText(index::getSymbolKindString(Kind));
+    Header.appendText(index::getSymbolKindString(Kind)).appendSpace();
   assert(!Name.empty() && "hover triggered on a nameless symbol");
   Header.appendCode(Name);
 
@@ -787,9 +787,9 @@ markup::Document HoverInfo::present() const {
     // Parameters:
     // - `bool param1`
     // - `int param2 = 5`
-    Output.addParagraph().appendText("→").appendCode(*ReturnType);
+    Output.addParagraph().appendText("→ ").appendCode(*ReturnType);
     if (Parameters && !Parameters->empty()) {
-      Output.addParagraph().appendText("Parameters:");
+      Output.addParagraph().appendText("Parameters: ");
       markup::BulletList &L = Output.addBulletList();
       for (const auto &Param : *Parameters) {
         std::string Buffer;
@@ -804,7 +804,7 @@ markup::Document HoverInfo::present() const {
 
   if (Value) {
     markup::Paragraph &P = Output.addParagraph();
-    P.appendText("Value =");
+    P.appendText("Value = ");
     P.appendCode(*Value);
   }
 
@@ -883,7 +883,7 @@ void parseDocumentationLine(llvm::StringRef Line, markup::Paragraph &Out) {
         break;
     }
   }
-  Out.appendText(Line);
+  Out.appendText(Line).appendSpace();
 }
 
 void parseDocumentation(llvm::StringRef Input, markup::Document &Output) {

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

@@ -155,11 +155,11 @@ TEST(Paragraph, Chunks) {
   Paragraph P = Paragraph();
   P.appendText("One ");
   P.appendCode("fish");
-  P.appendText(", two");
+  P.appendText(", two ");
   P.appendCode("fish", /*Preserve=*/true);
 
-  EXPECT_EQ(P.asMarkdown(), "One `fish` , two `fish`");
-  EXPECT_EQ(P.asPlainText(), "One fish , two `fish`");
+  EXPECT_EQ(P.asMarkdown(), "One `fish`, two `fish`");
+  EXPECT_EQ(P.asPlainText(), "One fish, two `fish`");
 }
 
 TEST(Paragraph, SeparationOfChunks) {
@@ -168,17 +168,21 @@ TEST(Paragraph, SeparationOfChunks) {
   // Purpose is to check for separation between different chunks.
   Paragraph P;
 
-  P.appendText("after");
+  P.appendText("after ");
   EXPECT_EQ(P.asMarkdown(), "after");
   EXPECT_EQ(P.asPlainText(), "after");
 
-  P.appendCode("foobar");
+  P.appendCode("foobar").appendSpace();
   EXPECT_EQ(P.asMarkdown(), "after `foobar`");
   EXPECT_EQ(P.asPlainText(), "after foobar");
 
   P.appendText("bat");
   EXPECT_EQ(P.asMarkdown(), "after `foobar` bat");
   EXPECT_EQ(P.asPlainText(), "after foobar bat");
+
+  P.appendCode("no").appendCode("space");
+  EXPECT_EQ(P.asMarkdown(), "after `foobar` bat`no` `space`");
+  EXPECT_EQ(P.asPlainText(), "after foobar batno space");
 }
 
 TEST(Paragraph, ExtraSpaces) {
@@ -186,8 +190,16 @@ TEST(Paragraph, ExtraSpaces) {
   Paragraph P;
   P.appendText("foo\n   \t   baz");
   P.appendCode(" bar\n");
-  EXPECT_EQ(P.asMarkdown(), "foo baz `bar`");
-  EXPECT_EQ(P.asPlainText(), "foo baz bar");
+  EXPECT_EQ(P.asMarkdown(), "foo baz`bar`");
+  EXPECT_EQ(P.asPlainText(), "foo bazbar");
+}
+
+TEST(Paragraph, SpacesCollapsed) {
+  Paragraph P;
+  P.appendText(" foo bar ");
+  P.appendText(" baz ");
+  EXPECT_EQ(P.asMarkdown(), "foo bar baz");
+  EXPECT_EQ(P.asPlainText(), "foo bar baz");
 }
 
 TEST(Paragraph, NewLines) {

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

@@ -2019,10 +2019,9 @@ TEST(Hover, ParseDocumentation) {
                    "foo bar",
                },
                {
-                   // FIXME: we insert spaces between code and text chunk.
                    "Tests primality of `p`.",
-                   "Tests primality of `p` .",
-                   "Tests primality of `p` .",
+                   "Tests primality of `p`.",
+                   "Tests primality of `p`.",
                },
                {
                    "'`' should not occur in `Code`",