[clangd] Change line break behaviour for hoverinfo

[lolleko]

Summary

parseDocumentation retains hard line breaks and removes soft line breaks inside documentation comments.
Wether a line break is hard or soft is determined by the following rules (some of which have been discussed in https://github.com/clangd/clangd/issues/95):

• Hard Markdown line breaks (https://github.github.com/gfm/#hard-line-breaks) are retained
• Line breaks that are preceded by a punctuation are retained
• Line breaks that are followed by "interesting characters" (e.g. Markdown syntax, doxygen commands) are retained
• All other line breaks are removed

Since this is my first contribution feel free to be extra thorough.

Related issue: https://github.com/clangd/clangd/issues/95

[sammccall]

I understand the main goal here is preserving intended line breaks in documentation comments? Thanks for tackling this problem!

A design goal here is that the markup objects are the source of truth about the document structure. So if we've decided we want a comment to turn into multiple lines, we should create multiple Paragraph objects, this makes it easy to adjust the rendering strategy and isolate differences between text/markdown.

I think this means this splitting needs to happen on the way in instead of on the way out. In Hover.cpp:

if (!Documentation.empty())
  Output.addParagraph().appendText(Documentation);

would be come something like

parseDocumentationComment(Output, Documentation);

which would ultimately add a paragraph to Output for each line detected in Documentation.

Even though markdown doc comments in cpp are still rare, I removed the markdown escaping

Please let's leave this out, at least from this patch, if it's not the primary goal. This is a complicated tradeoff, there's plenty of reasonably common patterns where we should be escaping, such as vector<string>. Moreover if we're treating punctuation in the comments as formatting, we should likely also be doing so in plain-text mode.

FYI D75687 (less unneccesary escaping in markdown) is also in flight and likely to touch some of the same code.

clang-tools-extra/clangd/FormattedString.cpp

103 ↗

(On Diff #250027)

We can separate concerns more clearly by not having this function care about markdown.
Can it have a signature like:

• vector<StringRef> splitLines(StringRef Text) (which wouldn't normalize whitespace within the lines)
• vector<string> splitLines(StringRef Text) which would

and then continue to have line rendering handled elsewhere?

[lolleko]

Addressed review.

Paragraphs (lines separated by a double line break \n\n) are currently not retained. Because the current implementation of markup::Paragraph only appends a single white space after it's contents (https://github.com/llvm/llvm-project/blob/e26e9ba288ce156b851504ebbb7d8a775572957c/clang-tools-extra/clangd/FormattedString.cpp#L368).
I think this is semantically wrong because in natural language aswell as markup languages like md, html, ... a paragraph should be followed by a blank line.
But since a lot of code relies on that single white space paragraph, this should be addressed in a different patch.

[sammccall]

Thanks, this fits in a lot better!
I think there's some more details that could be handled, e.g. if a line is much shorter than the max line length, then that probably implies it's a "real" break. But if we get the code structure right, these should be easy to plug in later.

Because the current implementation of markup::Paragraph only appends a single white space after it's contents. I think this is semantically wrong because in natural language aswell as markup languages like md, html, ... a paragraph should be followed by a blank line

Yeah, we can look at this again, but we shouldn't do so in this patch. You're right on the semantics, but real implementations (particularly VSCode) use a distasteful amount of whitespace around HTML paragraphs vs other UI elements and don't give us any control over the CSS. (Similarly rendering paragraph breaks as \n\n in plaintext feels much too heavyweight in terminal-based editors in my experience)

BTW, it's really useful to upload diffs with full file context, I think -U99999 will do this, though I tend to use arc diff which always includes it.

clang-tools-extra/clangd/Hover.cpp

524

I think we're going to end up using this elsewhere, e.g. in code completion.
Fine to keep it in Hover for now, but please expose it from hover.h and test it directly (rather than via HoverInfo::present())

This will also allow moving this implementation to the end of the file, rather than interleaving it with unrelated parts of hover logic.

527

nit: input should be StringRef since you never mutate it

529

nit: we'd generally use static/anon-namespace functions rather than lambdas for helpers when we don't need to capture much stuff.

583

I think this could be easier to read by separating the strategy from some of the details.

From my reading of the code, the strategy is:

• examine the raw lines from the input to determine where true paragraph breaks occur. (In general, based on the end of the previous line and the start of the new line)
• form each groups of raw lines into a paragraph (by trimming whitespace and \, dropping blank lines, and joining the results with spaces)

Given this, I think we could extract a couple of functions like:

• bool breakBetween(StringRef Before, StringRef After)
• std::string joinRawLines(ArrayRef<StringRef>)

and the outer loop becomes simpler, like:

SmallVector<StringRef, N> RawLines;
llvm::SplitString(Input, RawLines, "\n");
ArrayRef Remaining = RawLines;
while (!Remaining.empty()) {
  int Para = 1;
  while (Para < Remaining.size() && !breakBetween(Remaining[Para-1], Remaining[Para]))
    Para++;
  std::string ParaText = joinRawLines(Remaining.take_front(Para));
  if (!ParaText.empty())
    Output.addParagraph().appendText(std::move(ParaText));
  Remaining = Remaining.drop_front(Para);
}

I think this signature for breakBetween in terms of stringrefs would result in the lambdas you have here being a bit more readable, and the main is clearer for not dealing directly with string, whitespace, etc.

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

1886

@kadircet this feels like a case where having a debug output mode like [Para:foo][Para:bar] would be useful!

[lolleko]

Adressed 2nd Review

The problem with the changes you proposed is that it doesn't handle paragraphs (\n\n).
I think the conditionals required to make that work wouldn't be much cleaner than the current solution.

And I do think parahraphs in comments should be retained. I agree that not every hard line break should be a pargraph.
But actual parahraphs should be retained e.g.:

	/**
	 * Condition for calling UpdateOverlaps() to initialize overlap state when loaded in during level streaming.
	 * If set to 'UseConfigDefault', the default specified in ini (displayed in 'DefaultUpdateOverlapsMethodDuringLevelStreaming') will be used.
	 * If overlaps are not initialized, this actor and attached components will not have an initial state of what objects are touching it,
	 * and overlap events may only come in once one of those objects update overlaps themselves (for example when moving).
	 * However if an object touching it *does* initialize state, both objects will know about their touching state with each other.
	 * This can be a potentially large performance savings during level streaming, and is safe if the object and others initially
	 * overlapping it do not need the overlap state because they will not trigger overlap notifications.
	 * 
	 * Note that if 'bGenerateOverlapEventsDuringLevelStreaming' is true, overlaps are always updated in this case, but that flag
	 * determines whether the Begin/End overlap events are triggered.
	 * 
	 * @see bGenerateOverlapEventsDuringLevelStreaming, DefaultUpdateOverlapsMethodDuringLevelStreaming, GetUpdateOverlapsMethodDuringLevelStreaming()
	 */

I think once proper paragraphs are in. The linebreak parsing could be rewritten into 2 steps:

1. Split on \n\n to extract actual paragraphs.
2. Use the solution you proposed to split and join new lines.

[sammccall]

Thanks for this! Mostly just some cleanups/comment nits left.

The substantive change is around the \ and at end-of-line. It looks like the goal is on improving the parsing of markdown-formatted comments, but we need to focus as well on non-markdown-formatted comments as these are the majority case. (Most of your patch improves these a lot!)

If you don't have commit access yet, I'm happy to land this for you, just let me know when ready.
(A couple of patches like this are good grounds to apply for commit access, you can just link to the review threads)

clang-tools-extra/clangd/Hover.cpp

530

We can make use of some StringRef helpers to make this more readable.
I think this is equivalent (to the whole function body):

return Str.substr(LineBreakIndex + 1).drop_while(isWhitespace).startswith("\n");

534

I'm not altogether happy with these rules: many comments are not markdown, and these don't seem very safe for non-markdown comments.

Comments ending in a trailing slash may be explicitly indicating a soft line break e.g.

// Example usage:
// my_prog --some_flag = foo \
//    --remote_database=/path/to/whatever \
//    input.txt

and trailing whitespace is often just sloppiness.

I did a search over a large corpus of open-source C++.

• for //.*\ \ $: Of a random 10 results, none appear to be markdown.
• for //.*\\$: Of a random 10 results, none were markdown. (4 were explicit soft breaks, 4 were ascii art, 2 were soft breaks in commented-out macro definitions)

535

What about:

StringRef Prev = Str.substr(0, LineBreakIndex);
return Prev.endswith("\\") || Prev.endswith("  ")

(this seems more readable, and fixes the bug if LineBreakIndex == 1 and the string is {backslash, newline, ...}.)

544

nit: Punctuation.contains(...)

733

nit: please put the doc on the declaration rather than the definition.

(I think the previous name was more appropriate to the signature, but up to you)

750

rather than repeating the code twice, please just use if isParagraph() || isHard(), with the comment that we may treat paragraph differently

751

nit: llvm tends to use "// FIXME: "

751

please soften this to "maybe".
(Because we need to consider the padding/css issues of doing so, whether a hard break vs double break reflects different intents or just different conventions in different codebases, and whether we have a reasonable way to implement this - paragraphs have leading as well as trailing padding, so it gets complicated fast).

and rephrase to "should distinguish between line breaks and paragraphs".
("Double linebreak" is confusing as it doesn't mean anything at the markup::Document level, and markdown paragraph isn't the same concept as a plaintext blank line, even if they are both encoded as "\n\n")

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

1891

please add tests starting/ending with newline as these are special cases in the code

1943

please drop these comments, as they don't seem to be true (unless assuming the content is markdown, which it usually isn't)

I think we can drop most of the tests too (for specific characters after newline), as we already have escaping tests and these don't actually test different linebreaking behaviour.

[lolleko]

This comment was removed by lolleko.

[lolleko]

This comment was removed by lolleko.

[lolleko]

Final Cleanup
Removed markdown linebreak parsing for now.

No I don't have commit rights yet. Feel fre to merge this for me.

Thanks for the thorough review.

clang-tools-extra/clangd/Hover.h

struct HoverInfo {
   /// Set for all templates(function, class, variable).
   llvm::Optional<std::vector<Param>> TemplateParameters;
   /// Contains the evaluated value of the symbol if available.
   llvm::Optional<std::string> Value;
 
   /// Produce a user-readable information.
   markup::Document present() const;
 };
+
+void parseLineBreaks(llvm::StringRef Input, markup::Document &Output);
+
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const HoverInfo::Param &);
 inline bool operator==(const HoverInfo::Param &LHS,
                        const HoverInfo::Param &RHS) {
   return std::tie(LHS.Type, LHS.Name, LHS.Default) ==
          std::tie(RHS.Type, RHS.Name, RHS.Default);
 }
 
 /// Get the hover information when hovering at \p Pos.
 llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
                                    format::FormatStyle Style,
                                    const SymbolIndex *Index);
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/Hover.cpp

if (auto Val = printExprValue(E, AST.getASTContext())) {
     Policy.SuppressTagKeyword = true;
     HI.Type = printType(E->getType(), Policy);
     HI.Value = *Val;
     HI.Name = std::string(getNameForExpr(E));
     return HI;
   }
   return llvm::None;
 }
+
+bool isParagraphLineBreak(llvm::StringRef Str, size_t LineBreakIndex) {
+  if (LineBreakIndex + 1 >= Str.size()) {
+    return false;
+  }
+  auto NextNonSpaceCharIndex = Str.find_first_not_of(' ', LineBreakIndex + 1);
+
+  return NextNonSpaceCharIndex != llvm::StringRef::npos &&
+         Str[NextNonSpaceCharIndex] == '\n';
+};
+
+bool isMardkownLineBreak(llvm::StringRef Str, size_t LineBreakIndex) {
+  return LineBreakIndex > 1 &&
+         (Str[LineBreakIndex - 1] == '\\' ||
+          (Str[LineBreakIndex - 1] == ' ' && Str[LineBreakIndex - 2] == ' '));
+};
+
+bool isPunctuationLineBreak(llvm::StringRef Str, size_t LineBreakIndex) {
+  constexpr llvm::StringLiteral Punctuation = R"txt(.:,;!?)txt";
+
+  return LineBreakIndex > 0 &&
+         Punctuation.find(Str[LineBreakIndex - 1]) != llvm::StringRef::npos;
+};
+
+bool isFollowedByHardLineBreakIndicator(llvm::StringRef Str,
+                                        size_t LineBreakIndex) {
+  // '-'/'*' md list, '@'/'\' documentation command, '>' md blockquote,
+  // '#' headings, '`' code blocks
+  constexpr llvm::StringLiteral LinbreakIdenticators = R"txt(-*@\>#`)txt";
+
+  auto NextNonSpaceCharIndex = Str.find_first_not_of(' ', LineBreakIndex + 1);
+
+  if (NextNonSpaceCharIndex == llvm::StringRef::npos) {
+    return false;
+  }
+
+  auto FollowedBySingleCharIndicator =
+      LinbreakIdenticators.find(Str[NextNonSpaceCharIndex]) !=
+      llvm::StringRef::npos;
+
+  auto FollowedByNumberedListIndicator =
+      llvm::isDigit(Str[NextNonSpaceCharIndex]) &&
+      NextNonSpaceCharIndex + 1 < Str.size() &&
+      (Str[NextNonSpaceCharIndex + 1] == '.' ||
+       Str[NextNonSpaceCharIndex + 1] == ')');
+
+  return FollowedBySingleCharIndicator || FollowedByNumberedListIndicator;
+};
+
+bool isHardLineBreak(llvm::StringRef Str, size_t LineBreakIndex) {
+  return isMardkownLineBreak(Str, LineBreakIndex) ||
+         isPunctuationLineBreak(Str, LineBreakIndex) ||
+         isFollowedByHardLineBreakIndicator(Str, LineBreakIndex);
+}
+
 } // namespace
 
 llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
                                    format::FormatStyle Style,
                                    const SymbolIndex *Index) {
   const SourceManager &SM = AST.getSourceManager();
   auto CurLoc = sourceLocationInMainFile(SM, Pos);
   if (!CurLoc) {
     llvm::consumeError(CurLoc.takeError());
     return llvm::None;
   }
   const auto &TB = AST.getTokens();
   auto TokensTouchingCursor = syntax::spelledTokensTouching(*CurLoc, TB);
   // Early exit if there were no tokens around the cursor.
markup::Document HoverInfo::present() const {
 
   if (Value) {
     markup::Paragraph &P = Output.addParagraph();
     P.appendText("Value =");
     P.appendCode(*Value);
   }
 
   if (!Documentation.empty())
-    Output.addParagraph().appendText(Documentation);
+    parseLineBreaks(Documentation, Output);
 
   if (!Definition.empty()) {
     Output.addRuler();
     std::string ScopeComment;
     // Drop trailing "::".
     if (!LocalScope.empty()) {
       // Container name, e.g. class, method, function.
       // We might want to propogate some info about container type to print
       // function foo, class X, method X::bar, etc.
       ScopeComment =
           "// In " + llvm::StringRef(LocalScope).rtrim(':').str() + '\n';
     } else if (NamespaceScope && !NamespaceScope->empty()) {
       ScopeComment = "// In namespace " +
                      llvm::StringRef(*NamespaceScope).rtrim(':').str() + '\n';
     }
     // 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;
 }
 
+// Tries to retain hard line breaks and drops soft line breaks.
+void parseLineBreaks(llvm::StringRef Input, markup::Document &Output) {
+
+  constexpr auto WhiteSpaceChars = "\t\n\v\f\r ";
+
+  auto TrimmedInput = Input.trim();
+
+  std::string CurrentLine;
+
+  for (size_t CharIndex = 0; CharIndex < TrimmedInput.size();) {
+    if (TrimmedInput[CharIndex] == '\n') {
+      // Trim whitespace infront of linebreak, also get rif of markdown
+      // linebreak character `\`
+      const auto LastNonSpaceCharIndex =
+          CurrentLine.find_last_not_of(std::string(WhiteSpaceChars) + "\\") + 1;
+      CurrentLine.erase(LastNonSpaceCharIndex);
+
+      if (isParagraphLineBreak(TrimmedInput, CharIndex)) {
+        // TODO this should add an actual paragraph (double linebreak)
+        Output.addParagraph().appendText(CurrentLine);
+        CurrentLine = "";
+      } else if (isHardLineBreak(TrimmedInput, CharIndex)) {
+        Output.addParagraph().appendText(CurrentLine);
+        CurrentLine = "";
+      } else {
+        // Ommit linebreak
+        CurrentLine += ' ';
+      }
+
+      CharIndex++;
+      // After a linebreak always remove spaces to avoid 4 space markdown code
+      // blocks, also skip all additional linebreaks since they have no effect
+      CharIndex = TrimmedInput.find_first_not_of(WhiteSpaceChars, CharIndex);
+    } else {
+      CurrentLine += TrimmedInput[CharIndex];
+      CharIndex++;
+    }
+  }
+  if (!CurrentLine.empty()) {
+    Output.addParagraph().appendText(CurrentLine);
+  }
+}
+
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
                               const HoverInfo::Param &P) {
   std::vector<llvm::StringRef> Output;
   if (P.Type)
     Output.push_back(*P.Type);
   if (P.Name)
     Output.push_back(*P.Name);
   OS << llvm::join(Output, " ");
   if (P.Default)
     OS << " = " << *P.Default;
   return OS;
 }
 
 } // namespace clangd
 } // namespace clang

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) {
+  struct Case {
+    llvm::StringRef Documentation;
+    llvm::StringRef ExpectedRenderMarkdown;
+    llvm::StringRef ExpectedRenderPlainText;
+  } Cases[] = {{
+                   "foo\n\n\nbar",
+                   "foo  \nbar",
+                   "foo\nbar",
+               },
+               {
+                   "foo\n\n\n\tbar",
+                   "foo  \nbar",
+                   "foo\nbar",
+               },
+               {
+                   "foo\n\n\n bar",
+                   "foo  \nbar",
+                   "foo\nbar",
+               },
+               {
+                   "foo.\nbar",
+                   "foo.  \nbar",
+                   "foo.\nbar",
+               },
+               {
+                   "foo:\nbar",
+                   "foo:  \nbar",
+                   "foo:\nbar",
+               },
+               {
+                   "foo,\nbar",
+                   "foo,  \nbar",
+                   "foo,\nbar",
+               },
+               {
+                   "foo;\nbar",
+                   "foo;  \nbar",
+                   "foo;\nbar",
+               },
+               {
+                   "foo!\nbar",
+                   "foo!  \nbar",
+                   "foo!\nbar",
+               },
+               {
+                   "foo?\nbar",
+                   "foo?  \nbar",
+                   "foo?\nbar",
+               },
+               {
+                   "foo\n-bar",
+                   "foo  \n-bar",
+                   "foo\n-bar",
+               },
+               {
+                   "foo\n*bar",
+                   // TODO `*` should probably not be escaped after line break
+                   "foo  \n\\*bar",
+                   "foo\n*bar",
+               },
+               {
+                   "foo\n@bar",
+                   "foo  \n@bar",
+                   "foo\n@bar",
+               },
+               {
+                   "foo\n\\bar",
+                   // TODO `\` should probably not be escaped after line break
+                   "foo  \n\\\\bar",
+                   "foo\n\\bar",
+               },
+               {
+                   "foo\n>bar",
+                   // TODO `>` should probably not be escaped after line break
+                   "foo  \n\\>bar",
+                   "foo\n>bar",
+               },
+               {
+                   "foo\n#bar",
+                   "foo  \n#bar",
+                   "foo\n#bar",
+               },
+               {
+                   "foo  \nbar",
+                   "foo  \nbar",
+                   "foo\nbar",
+               },
+               {
+                   "foo    \nbar",
+                   "foo  \nbar",
+                   "foo\nbar",
+               },
+               {
+                   "foo\\\nbar",
+                   "foo  \nbar",
+                   "foo\nbar",
+               },
+               {
+                   "foo\nbar",
+                   "foo bar",
+                   "foo bar",
+               }};
+
+  for (const auto &C : Cases) {
+    markup::Document Output;
+    parseLineBreaks(C.Documentation, Output);
+
+    EXPECT_EQ(Output.asMarkdown(), C.ExpectedRenderMarkdown);
+    EXPECT_EQ(Output.asPlainText(), C.ExpectedRenderPlainText);
+  }
+}
+
 // This is a separate test as headings don't create any differences in plaintext
 // mode.
 TEST(Hover, PresentHeadings) {
   HoverInfo HI;
   HI.Kind = index::SymbolKind::Variable;
   HI.Name = "foo";
 
   EXPECT_EQ(HI.present().asMarkdown(), "### variable `foo`");