[clangd] Fix whitespace between chunks in markdown paragraphs.

[sammccall]

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)

[kadircet]

clang-tools-extra/clangd/FormattedString.cpp

419

i think we always want a space before code chunks, for example in case of multiple code chunks this would result in malformed makrdown:

`foo``bar`

can you also add a test case for that?

if you would like to keep SpaceBefore/After to text only chunks, I suppose we can also check for chunk's kind for outputting a space.

clang-tools-extra/clangd/FormattedString.h

66

argh... this one doesn't belong here :( wonder how we missed it. sent out 7a3be975b92fece93e07bfc6451e9a39eb6f5142 sorry for conflict :/

70

nit: define in two lines?

clang-tools-extra/clangd/Hover.cpp

886

nit Out.appendText().appendSpace() ?

[sammccall]

clang-tools-extra/clangd/FormattedString.cpp

419

Good catch!

I think it's better to solve this in appendCode as you suggest - inconsistencies between markdown and plaintext are good places for bugs to hide without being seen.

[sammccall]

Address comments.

[kadircet]

clang-tools-extra/clangd/FormattedString.cpp

414

I would say we should also put a space even when the previous chunk is text, as:

foo`bar`

is also malformed.

clang-tools-extra/clangd/FormattedString.h

67

this is no longer true :/

[sammccall]

tweak coment

[sammccall]

clang-tools-extra/clangd/FormattedString.cpp

414

Do you have a reference or example for that? I can't find it in the spec, and the couple of parsers I tried accept it.

clang-tools-extra/clangd/FormattedString.h

67

Weakened it a bit. Don't think it's worth giving details inline here.

[kadircet]

thanks lgtm!

clang-tools-extra/clangd/FormattedString.cpp

414

i remembered this rule from emphasis(IIRC asdf_foo_bar doesn't count as emphasis because it is not separated from surrounding letters.), and thought it also applies to code spans, tried it over phab and it didn't render as inline :D so i didn't check more.

but you are right, looks like preceding/trailing whitespaces are not required. sorry for the hussle.

clang-tools-extra/clangd/FormattedString.h

public:
   void renderPlainText(llvm::raw_ostream &OS) const override;
 
   /// Append plain text to the end of the string.
   Paragraph &appendText(llvm::StringRef Text);
 
   /// Append inline code, this translates to the ` block in markdown.
   Paragraph &appendCode(llvm::StringRef Code);
 
+  /// 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 {
       PlainText,
       InlineCode,
     } Kind = PlainText;
     std::string Contents;
     /// Language for code block chunks. Ignored for other chunks.
     std::string Language;
+    // Whether this chunk should be surrounded by whitespace.
+    // Consecutive SpaceAfter and SpaceBefore will be collapsed into one space.
+    // Code spans don't set this: their spaces belong "inside" the span.
+    bool SpaceBefore = false, SpaceAfter = false;
   };
   std::vector<Chunk> Chunks;
 };
 
 /// Represents a sequence of one or more documents. Knows how to print them in a
 /// list like format, e.g. by prepending with "- " and indentation.
 class BulletList : public Block {
 public:

clang-tools-extra/clangd/FormattedString.cpp

 std::string Block::asPlainText() const {
   std::string R;
   llvm::raw_string_ostream OS(R);
   renderPlainText(OS);
   return llvm::StringRef(OS.str()).trim().str();
 }
 
 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.
   // VSCode requires two spaces at the end of line to start a new one.
   OS << "  \n";
 }
 
 void Paragraph::renderPlainText(llvm::raw_ostream &OS) const {
-  llvm::StringRef Sep = "";
+  bool NeedsSpace = false;
   for (auto &C : Chunks) {
-    OS << Sep << C.Contents;
-    Sep = " ";
+    if (C.SpaceBefore || NeedsSpace)
+      OS << " ";
+    OS << C.Contents;
+    NeedsSpace = C.SpaceAfter;
   }
   OS << '\n';
 }
 
 void BulletList::renderMarkdown(llvm::raw_ostream &OS) const {
   for (auto &D : Items) {
     // Instead of doing this we might prefer passing Indent to children to get
     // rid of the copies, if it turns out to be a bottleneck.
     OS << "- " << indentLines(D.asMarkdown()) << '\n';
   }
   // We need a new line after list to terminate it in markdown.
   OS << '\n';
 }
 
 void BulletList::renderPlainText(llvm::raw_ostream &OS) const {
   for (auto &D : Items) {
     // Instead of doing this we might prefer passing Indent to children to get
     // rid of the copies, if it turns out to be a bottleneck.
     OS << "- " << indentLines(D.asPlainText()) << '\n';
   }
 }
 
+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())
     return *this;
   Chunks.emplace_back();
   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) {
   std::string Norm = canonicalizeSpaces(std::move(Code));
   if (Norm.empty())
     return *this;
   Chunks.emplace_back();
   Chunk &C = Chunks.back();
   C.Contents = std::move(Norm);
   C.Kind = Chunk::InlineCode;
   return *this;
 }
 
 class Document &BulletList::addItem() {
   Items.emplace_back();
   return Items.back();
 }
 

clang-tools-extra/clangd/Hover.cpp

markup::Document HoverInfo::present() const {
   //
   // expression
   //
   // Note that we are making use of a level-3 heading because VSCode renders
   // level 1 and 2 headers in a huge font, see
   // 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);
 
   // Put a linebreak after header to increase readability.
   Output.addRuler();
   // Print Types on their own lines to reduce chances of getting line-wrapped by
   // editor, as they might be long.
   if (ReturnType) {
     // For functions we display signature in a list form, e.g.:
     // → `x`
     // 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;
         llvm::raw_string_ostream OS(Buffer);
         OS << Param;
         L.addItem().addParagraph().appendCode(std::move(OS.str()));
       }
     }
   } else if (Type) {
     Output.addParagraph().appendText("Type: ").appendCode(*Type);
   }
 
   if (Value) {
     markup::Paragraph &P = Output.addParagraph();
-    P.appendText("Value =");
+    P.appendText("Value = ");
     P.appendCode(*Value);
   }
 
   if (Offset)
     Output.addParagraph().appendText(
         llvm::formatv("Offset: {0} byte{1}", *Offset, *Offset == 1 ? "" : "s")
             .str());
   if (Size)
switch (Line[I]) {
           Out.appendText(Line.substr(0, I));
           Out.appendCode(Range->trim("`"));
           return parseDocumentationLine(Line.substr(I+Range->size()), Out);
         }
         break;
     }
   }
   Out.appendText(Line);
+  Out.appendSpace();
 }
 
 void parseDocumentation(llvm::StringRef Input, markup::Document &Output) {
   std::vector<llvm::StringRef> ParagraphLines;
   auto FlushParagraph = [&] {
     if (ParagraphLines.empty())
       return;
     auto &P = Output.addParagraph();

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

 }
 
 TEST(Paragraph, SeparationOfChunks) {
   // This test keeps appending contents to a single Paragraph and checks
   // expected accumulated contents after each one.
   // 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");
 }
 
 TEST(Paragraph, ExtraSpaces) {
   // Make sure spaces inside chunks are dropped.
   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) {
   // New lines before and after chunks are dropped.
   Paragraph P;
   P.appendText(" \n foo\nbar\n ");
   P.appendCode(" \n foo\nbar \n ");
   EXPECT_EQ(P.asMarkdown(), "foo bar `foo bar`");
   EXPECT_EQ(P.asPlainText(), "foo bar foo bar");
 }

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

llvm::StringRef ExpectedRenderPlainText;
                {
                    "foo\nbar",
                    "foo bar",
                    "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`",
                    "'\\`' should not occur in `Code`",
                    "'`' should not occur in Code",
                },
                {
                    "`not\nparsed`",