[clangd] Render doc-comment code spans with `backticks` in plaintext mode

[kadircet]

clang-tools-extra/clangd/FormattedString.cpp

387

should we rather use renderInlineBlock here ? because in presence of backticks inside the C.Contents it might become confusing e.g:

this is`foo(`x`)`

this would become:

this is `foo(``x``)`

and instead of keeping a marker maybe just:

if (Preserve && ..)
 OS << Sep << "`" << C.Contents << "`";
else
  OS << Sep << C.Contents;

[sammccall]

clang-tools-extra/clangd/FormattedString.cpp

387

rather use renderInlineBlock

I don't think humans deal with escaping better than with ambiguity - those two examples seem equally confusing to me :-\

I'd suggest better is to pick another marker that's unused in the chunk:

this is 'foo(`x`)'

Happy to do that in this patch if you like it.
instead of keeping a marker maybe just:

instead of keeping a marker maybe just

Happy to do that for now if you like, but I don't think it's much clearer. I factored it this way because I think the marker is likely to have more options in future (bold spans, avoiding conflicts) and wanted to hint at that.

[kadircet]

LGTM thanks!

clang-tools-extra/clangd/FormattedString.cpp

387

I don't think humans deal with escaping better than with ambiguity - those two examples seem equally confusing to me :-\
I'd suggest better is to pick another marker that's unused in the chunk:
this is 'foo(x)'
Happy to do that in this patch if you like it.

Yeah that looks like a better approach, feel free to leave a fixme as well. As I believe this should be a rather rare occurrence and we can take care of it once it becomes a real issue or someone has time.

instead of keeping a marker maybe just

Happy to do that for now if you like, but I don't think it's much clearer. I factored it this way because I think the marker is likely to have more options in future (bold spans, avoiding conflicts) and wanted to hint at that.

OK makes sense let's keep it that way.

clang-tools-extra/clangd/FormattedString.h

public:
   void renderMarkdown(llvm::raw_ostream &OS) const override;
   void renderPlainText(llvm::raw_ostream &OS) const override;
   std::unique_ptr<Block> clone() 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);
+  /// \p Preserve indicates the code span must be apparent even in plaintext.
+  Paragraph &appendCode(llvm::StringRef Code, bool Preserve = false);
 
 private:
   struct Chunk {
     enum {
       PlainText,
       InlineCode,
     } Kind = PlainText;
+    // Preserve chunk markers in plaintext.
+    bool Preserve = false;
     std::string Contents;
   };
   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 {

clang-tools-extra/clangd/FormattedString.cpp

void Paragraph::renderMarkdown(llvm::raw_ostream &OS) const {
   // 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 = "";
   for (auto &C : Chunks) {
-    OS << Sep << C.Contents;
+    llvm::StringRef Marker = "";
+    if (C.Preserve && C.Kind == Chunk::InlineCode)
+      Marker = chooseMarker({"`", "'", "\""}, C.Contents);
+    OS << Sep << Marker << C.Contents << Marker;
     Sep = " ";
   }
   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
if (Norm.empty())
     return *this;
   Chunks.emplace_back();
   Chunk &C = Chunks.back();
   C.Contents = std::move(Norm);
   C.Kind = Chunk::PlainText;
   return *this;
 }
 
-Paragraph &Paragraph::appendCode(llvm::StringRef Code) {
+Paragraph &Paragraph::appendCode(llvm::StringRef Code, bool Preserve) {
   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;
   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

 
 void parseDocumentationLine(llvm::StringRef Line, markup::Paragraph &Out) {
   // Probably this is appendText(Line), but scan for something interesting.
   for (unsigned I = 0; I < Line.size(); ++I) {
     switch (Line[I]) {
       case '`':
         if (auto Range = getBacktickQuoteRange(Line, I)) {
           Out.appendText(Line.substr(0, I));
-          Out.appendCode(Range->trim("`"));
+          Out.appendCode(Range->trim("`"), /*Preserve=*/true);
           return parseDocumentationLine(Line.substr(I+Range->size()), Out);
         }
         break;
     }
   }
   Out.appendText(Line);
 }
 

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

TEST(Render, Escaping) {
 
   // In code blocks we don't need to escape ASCII punctuation.
   Paragraph P = Paragraph();
   P.appendCode("* foo !+ bar * baz");
   EXPECT_EQ(P.asMarkdown(), "`* foo !+ bar * baz`");
 
   // But we have to escape the backticks.
   P = Paragraph();
-  P.appendCode("foo`bar`baz");
+  P.appendCode("foo`bar`baz", /*Preserve=*/true);
   EXPECT_EQ(P.asMarkdown(), "`foo``bar``baz`");
+  // In plain-text, we fall back to different quotes.
+  EXPECT_EQ(P.asPlainText(), "'foo`bar`baz'");
 
   // Inline code blocks starting or ending with backticks should add spaces.
   P = Paragraph();
   P.appendCode("`foo");
   EXPECT_EQ(P.asMarkdown(), "` ``foo `");
   P = Paragraph();
   P.appendCode("foo`");
   EXPECT_EQ(P.asMarkdown(), "` foo`` `");
EXPECT_EQ(D.asMarkdown(), "````cpp\n"
                             "````");
   D = Document();
   D.addCodeBlock("foobarbaz ` `` ``` ```` `\nqux");
   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.appendCode("fish", /*Preserve=*/true);
+
+  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");
   EXPECT_EQ(P.asMarkdown(), "after");

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` .",
                },
                {
                    "'`' should not occur in `Code`",
                    "'\\`' should not occur in `Code`",
-                   "'`' should not occur in Code",
+                   "'`' should not occur in `Code`",
                },
                {
                    "`not\nparsed`",
                    "\\`not parsed\\`",
                    "`not parsed`",
                }};
 
   for (const auto &C : Cases) {