[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

446–449

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

69–75

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

73

nit: define in two lines?

clang-tools-extra/clangd/Hover.cpp

887

nit Out.appendText().appendSpace() ?

[sammccall]

clang-tools-extra/clangd/FormattedString.cpp

446–449

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

439

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

72

this is no longer true :/

[sammccall]

tweak coment

[sammccall]

clang-tools-extra/clangd/FormattedString.cpp

439

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

72

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

[kadircet]

thanks lgtm!

clang-tools-extra/clangd/FormattedString.cpp

439

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:
 
   /// Append plain text to the end of the string.
   Paragraph &appendText(llvm::StringRef Text);
 
   /// Append inline code, this translates to the ` block in markdown.
   /// \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 {
       PlainText,
       InlineCode,
     } Kind = PlainText;
     // 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;
 };
 
 /// 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:
   void renderMarkdown(llvm::raw_ostream &OS) const override;

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";
 }
 
 std::unique_ptr<Block> Paragraph::clone() const {
   return std::make_unique<Paragraph>(*this);
 }
 
 /// Choose a marker to delimit `Text` from a prioritized list of options.
 /// This is more readable than escaping for plain-text.
 llvm::StringRef chooseMarker(llvm::ArrayRef<llvm::StringRef> Options,
                              llvm::StringRef Text) {
   // Prefer a delimiter whose characters don't appear in the text.
   for (llvm::StringRef S : Options)
     if (Text.find_first_of(S) == llvm::StringRef::npos)
       return S;
   return Options.front();
 }
 
 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';
 }
 
 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, bool Preserve) {
+  bool AdjacentCode =
+      !Chunks.empty() && Chunks.back().Kind == Chunk::InlineCode;
   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;
   C.Preserve = Preserve;
+  // Disallow adjacent code spans without spaces, markdown can't render them.
+  C.SpaceBefore = AdjacentCode;
   return *this;
 }
 
 std::unique_ptr<Block> BulletList::clone() const {
   return std::make_unique<BulletList>(*this);
 }
 
 class Document &BulletList::addItem() {

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]) {
         if (auto Range = getBacktickQuoteRange(Line, I)) {
           Out.appendText(Line.substr(0, I));
           Out.appendCode(Range->trim("`"), /*Preserve=*/true);
           return parseDocumentationLine(Line.substr(I+Range->size()), Out);
         }
         break;
     }
   }
-  Out.appendText(Line);
+  Out.appendText(Line).appendSpace();
 }
 
 void parseDocumentation(llvm::StringRef Input, markup::Document &Output) {
   std::vector<llvm::StringRef> ParagraphLines;
   auto FlushParagraph = [&] {
     if (ParagraphLines.empty())
       return;
     auto &P = Output.addParagraph();
     for (llvm::StringRef Line : ParagraphLines)

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

EXPECT_EQ(D.asMarkdown(), "`````cpp\n"
                             "foobarbaz ` `` ``` ```` `\nqux\n"
                             "`````");
 }
 
 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) {
   // 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");
+
+  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) {
   // 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\n*bar",
                },
                {
                    "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`",