[clangd] Parse `foo` in documentation comments and render as code.

[sammccall]

Some saturday morning procrastination.

But I'm wondering how good an idea this is for plaintext: "Returns 'foo' on failure" is better than "Returns foo on failure", right? (With backticks instead of single quotes, phab is eating my formatting)
Maybe we should have an Emphasis flag or something on plaintext to preserve the quotes? (We don't want to include them e.g. in the return type chunks)

Also, realized our model for whitespace around paragraph chunks isn't ideal after all :-(

[kadircet]

LGTM, thanks!

Regarding spaces between code and text chunks, are you suggesting we should print:

Tests primality of`p`

if so, i do believe having a space before the backtick vs not having it is two totally different rendering decisions. And I think the former is more common, why do you think we should not put that space?
I suppose we can modify the spacing logic for markdown to only put a separator between same chunk types, but I don't think that'll look any better especially in plugins like coc.nvim.

clang-tools-extra/clangd/Hover.cpp

873

nit: s/Input/Line/

[kadircet]

But I'm wondering how good an idea this is for plaintext: "Returns 'foo' on failure" is better than "Returns foo on failure", right? (With backticks instead of single quotes, phab is eating my formatting)

ah sorry missed that one.

That's actually a good point, I didn't dwell on it too much because I don't think having backticks in plaintext is that useful. But thinking about it again some people might find it useful especially in long
documentations to figure out which parts of it to focus on. So this might be a regression for them, it would be nice to have some feedback mechanism for driving such decisions. So far it has been just
the reviewer and the author of the patch :D

So having a raw paragraph chunk(in addition to plaintext and inline code). might be a middle ground here. Plaintext renderers will keep showing the documentation as it is written in the source code,
including backticks, whereas markdown renderers would display a more rich text. WDYT?

[sammccall]

In D77456#1961374, @kadircet wrote:

LGTM, thanks!

Regarding spaces between code and text chunks, are you suggesting we should print:

Tests primality of`p`

No, I'm complaining about the space before the period in

Tests primality of `p` .

and the plaintext rendering too

Tests primality of p .

So having a raw paragraph chunk(in addition to plaintext and inline code). might be a middle ground here. Plaintext renderers will keep showing the documentation as it is written in the source code, including backticks, whereas markdown renderers would display a more rich text. WDYT?

Two main objections to the idea of "raw":

• we're going to emit arbitrary, potentially malformed markdown into the markdown stream, which can have arbitrary effects/glitches. I'd rather the emitter always emits valid markup and thus can't lose track of the context.
• this assumes the input is markdown. I want \c foo to also render as code-font foo in markdown and as backtick-foo in plaintext. So what do we do there, generate markdown as a string and then emit it as a raw chunk? What a mess.

I really do think what we want is a chunk with semantics "emphasized code" that renders as a code span in markdown and as backtick-delimited text in plaintext. Thus the proposal to put an emphasis bit on the code chunk. WDYT?

[kadircet]

In D77456#1991484, @sammccall wrote:

No, I'm complaining about the space before the period in

Tests primality of `p` .

and the plaintext rendering too

Tests primality of p .

Ah I see, yes this looks annoying, and it is only because the next block starts with a punctuation :/

Two main objections to the idea of "raw":

• we're going to emit arbitrary, potentially malformed markdown into the markdown stream, which can have arbitrary effects/glitches. I'd rather the emitter always emits valid markup and thus can't lose track of the context.
• this assumes the input is markdown. I want \c foo to also render as code-font foo in markdown and as backtick-foo in plaintext. So what do we do there, generate markdown as a string and then emit it as a raw chunk? What a mess.

I really do think what we want is a chunk with semantics "emphasized code" that renders as a code span in markdown and as backtick-delimited text in plaintext. Thus the proposal to put an emphasis bit on the code chunk. WDYT?

Sorry I wasn't clear on my comment. I was also suggesting putting some markdown generated by us while parsing the documentation into this raw field, which would be rendered as-is. In case we would like to perform this for other markers like *.
So going with an emphasis-only solution is also OK.

[sammccall]

Discussed offline, will follow up with fix for spaces and preserving backticks in text.

clang-tools-extra/clangd/FormattedString.h

 /// code block or plain text.
 /// One must introduce different paragraphs to create separate blocks.
 class Paragraph : public Block {
 public:
   void renderMarkdown(llvm::raw_ostream &OS) const override;
   void renderPlainText(llvm::raw_ostream &OS) const override;
 
   /// Append plain text to the end of the string.
-  Paragraph &appendText(std::string Text);
+  Paragraph &appendText(llvm::StringRef Text);
 
   /// Append inline code, this translates to the ` block in markdown.
-  Paragraph &appendCode(std::string Code);
+  Paragraph &appendCode(llvm::StringRef Code);
 
 private:
   struct Chunk {
     enum {
       PlainText,
       InlineCode,
     } Kind = PlainText;
     std::string Contents;

clang-tools-extra/clangd/FormattedString.cpp

for (char C : Input) {
     Backticks = 0;
   }
   MaxBackticks = std::max(Backticks, MaxBackticks);
   // Use the corresponding number of backticks to start and end a code block.
   return std::string(/*Repeat=*/std::max(3u, MaxBackticks + 1), '`');
 }
 
 // Trims the input and concatenates whitespace blocks into a single ` `.
-std::string canonicalizeSpaces(std::string Input) {
-  // Goes over the string and preserves only a single ` ` for any whitespace
-  // chunks, the rest is moved to the end of the string and dropped in the end.
-  auto WritePtr = Input.begin();
+std::string canonicalizeSpaces(llvm::StringRef Input) {
   llvm::SmallVector<llvm::StringRef, 4> Words;
   llvm::SplitString(Input, Words);
-  if (Words.empty())
-    return "";
-  // Go over each word and add it to the string.
-  for (llvm::StringRef Word : Words) {
-    if (WritePtr > Input.begin())
-      *WritePtr++ = ' '; // Separate from previous block.
-    llvm::for_each(Word, [&WritePtr](const char C) { *WritePtr++ = C; });
-  }
-  // Get rid of extra spaces.
-  Input.resize(WritePtr - Input.begin());
-  return Input;
+  return llvm::join(Words, " ");
 }
 
 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);
 
   // Trim rulers.

 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::appendText(std::string Text) {
-  Text = canonicalizeSpaces(std::move(Text));
-  if (Text.empty())
+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(Text);
+  C.Contents = std::move(Norm);
   C.Kind = Chunk::PlainText;
   return *this;
 }
 
-Paragraph &Paragraph::appendCode(std::string Code) {
-  Code = canonicalizeSpaces(std::move(Code));
-  if (Code.empty())
+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(Code);
+  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(std::string(index::getSymbolKindString(Kind)));
+    Header.appendText(index::getSymbolKindString(Kind));
   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) {
markup::Document HoverInfo::present() const {
   if (Value) {
     markup::Paragraph &P = Output.addParagraph();
     P.appendText("Value =");
     P.appendCode(*Value);
   }
 
   if (Offset)
     Output.addParagraph().appendText(
-        llvm::formatv("Offset: {0} byte{1}", *Offset, *Offset == 1 ? "" : "s"));
+        llvm::formatv("Offset: {0} byte{1}", *Offset, *Offset == 1 ? "" : "s")
+            .str());
   if (Size)
     Output.addParagraph().appendText(
-        llvm::formatv("Size: {0} byte{1}", *Size, *Size == 1 ? "" : "s"));
+        llvm::formatv("Size: {0} byte{1}", *Size, *Size == 1 ? "" : "s").str());
 
   if (!Documentation.empty())
     parseDocumentation(Documentation, Output);
 
   if (!Definition.empty()) {
     Output.addRuler();
     std::string ScopeComment;
     // Drop trailing "::".
if (!Definition.empty()) {
     }
     // Note that we don't print anything for global namespace, to not annoy
     // non-c++ projects or projects that are not making use of namespaces.
     Output.addCodeBlock(ScopeComment + Definition);
   }
   return Output;
 }
 
+// If the backtick at `Offset` starts a probable quoted range, return the range
+// (including the quotes).
+llvm::Optional<llvm::StringRef> getBacktickQuoteRange(llvm::StringRef Line,
+                                                      unsigned Offset) {
+  assert(Line[Offset] == '`');
+
+  // The open-quote is usually preceded by whitespace.
+  llvm::StringRef Prefix = Line.substr(0, Offset);
+  constexpr llvm::StringLiteral BeforeStartChars = " \t(=";
+  if (!Prefix.empty() && !BeforeStartChars.contains(Prefix.back()))
+    return llvm::None;
+
+  // The quoted string must be nonempty and usually has no leading/trailing ws.
+  auto Next = Line.find('`', Offset + 1);
+  if (Next == llvm::StringRef::npos)
+    return llvm::None;
+  llvm::StringRef Contents = Line.slice(Offset + 1, Next);
+  if (Contents.empty() || isWhitespace(Contents.front()) ||
+      isWhitespace(Contents.back()))
+    return llvm::None;
+
+  // The close-quote is usually followed by whitespace or punctuation.
+  llvm::StringRef Suffix = Line.substr(Next + 1);
+  constexpr llvm::StringLiteral AfterEndChars = " \t)=.,;:";
+  if (!Suffix.empty() && !AfterEndChars.contains(Suffix.front()))
+    return llvm::None;
+
+  return Line.slice(Offset, Next+1);
+}
+
+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("`"));
+          return parseDocumentationLine(Line.substr(I+Range->size()), Out);
+        }
+        break;
+    }
+  }
+  Out.appendText(Line);
+}
+
 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)
-      P.appendText(Line.str());
+      parseDocumentationLine(Line, P);
     ParagraphLines.clear();
   };
 
   llvm::StringRef Line, Rest;
   for (std::tie(Line, Rest) = Input.split('\n');
        !(Line.empty() && Rest.empty());
        std::tie(Line, Rest) = Rest.split('\n')) {
 

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

def)",
 
   for (const auto &C : Cases) {
     HoverInfo HI;
     C.Builder(HI);
     EXPECT_EQ(HI.present().asPlainText(), C.ExpectedRender);
   }
 }
 
-TEST(Hover, DocCommentLineBreakConversion) {
+TEST(Hover, ParseDocumentation) {
   struct Case {
     llvm::StringRef Documentation;
     llvm::StringRef ExpectedRenderMarkdown;
     llvm::StringRef ExpectedRenderPlainText;
   } Cases[] = {{
                    " \n foo\nbar",
                    "foo bar",
                    "foo bar",
llvm::StringRef ExpectedRenderPlainText;
                    "foo\n*bar",
                    "foo  \n\\*bar",
                    "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 .",
+               },
+               {
+                   "'`' 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) {
     markup::Document Output;
     parseDocumentation(C.Documentation, Output);
 
     EXPECT_EQ(Output.asMarkdown(), C.ExpectedRenderMarkdown);
     EXPECT_EQ(Output.asPlainText(), C.ExpectedRenderPlainText);