Skip to content

[clangd] Fix regression regarding new line handling for hover/signature help content #162029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+328 −95
Open

[clangd] Fix regression regarding new line handling for hover/signature help content #162029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
194 changes: 119 additions & 75 deletions clang-tools-extra/clangd/support/Markup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,60 +450,102 @@ std::string Block::asPlainText() const {
return llvm::StringRef(OS.str()).trim().str();
}

void Paragraph::renderNewlinesMarkdown(llvm::raw_ostream &OS,
std::string &ParagraphText) const {
llvm::StringRef Line, Rest;

for (std::tie(Line, Rest) =
llvm::StringRef(ParagraphText).ltrim("\n").rtrim().split('\n');
!(Line.empty() && Rest.empty());
std::tie(Line, Rest) = Rest.split('\n')) {

if (Line.empty()) {
// Blank lines are preserved in markdown.
OS << '\n';
continue;
}

OS << Line;

if (!Rest.empty() && isHardLineBreakAfter(Line, Rest, /*IsMarkdown=*/true))
// In markdown, 2 spaces before a line break forces a line break.
OS << " ";
OS << '\n';
}
}

void Paragraph::renderEscapedMarkdown(llvm::raw_ostream &OS) const {
bool NeedsSpace = false;
bool HasChunks = false;
std::string ParagraphText;
ParagraphText.reserve(EstimatedStringSize);
llvm::raw_string_ostream ParagraphTextOS(ParagraphText);
for (auto &C : Chunks) {
if (C.SpaceBefore || NeedsSpace)
OS << " ";
ParagraphTextOS << " ";
switch (C.Kind) {
case ChunkKind::PlainText:
OS << renderText(C.Contents, !HasChunks, /*EscapeMarkdown=*/true);
ParagraphTextOS << renderText(C.Contents, !HasChunks,
/*EscapeMarkdown=*/true);
break;
case ChunkKind::InlineCode:
OS << renderInlineBlock(C.Contents);
ParagraphTextOS << renderInlineBlock(C.Contents);
break;
case ChunkKind::Bold:
OS << renderText("**" + C.Contents + "**", !HasChunks,
/*EscapeMarkdown=*/true);
ParagraphTextOS << renderText("**" + C.Contents + "**", !HasChunks,
/*EscapeMarkdown=*/true);
break;
case ChunkKind::Emphasized:
OS << renderText("*" + C.Contents + "*", !HasChunks,
/*EscapeMarkdown=*/true);
ParagraphTextOS << renderText("*" + C.Contents + "*", !HasChunks,
/*EscapeMarkdown=*/true);
break;
}
HasChunks = true;
NeedsSpace = C.SpaceAfter;
}

renderNewlinesMarkdown(OS, ParagraphText);

// A paragraph in markdown is separated by a blank line.
OS << "\n\n";
}

void Paragraph::renderMarkdown(llvm::raw_ostream &OS) const {
bool NeedsSpace = false;
bool HasChunks = false;
std::string ParagraphText;
ParagraphText.reserve(EstimatedStringSize);
llvm::raw_string_ostream ParagraphTextOS(ParagraphText);
for (auto &C : Chunks) {
if (C.SpaceBefore || NeedsSpace)
OS << " ";
ParagraphTextOS << " ";
switch (C.Kind) {
case ChunkKind::PlainText:
OS << renderText(C.Contents, !HasChunks, /*EscapeMarkdown=*/false);
ParagraphTextOS << renderText(C.Contents, !HasChunks,
/*EscapeMarkdown=*/false);
break;
case ChunkKind::InlineCode:
OS << renderInlineBlock(C.Contents);
ParagraphTextOS << renderInlineBlock(C.Contents);
break;
case ChunkKind::Bold:
OS << "**" << renderText(C.Contents, !HasChunks, /*EscapeMarkdown=*/false)
<< "**";
ParagraphTextOS << "**"
<< renderText(C.Contents, !HasChunks,
/*EscapeMarkdown=*/false)
<< "**";
break;
case ChunkKind::Emphasized:
OS << "*" << renderText(C.Contents, !HasChunks, /*EscapeMarkdown=*/false)
<< "*";
ParagraphTextOS << "*"
<< renderText(C.Contents, !HasChunks,
/*EscapeMarkdown=*/false)
<< "*";
break;
}
HasChunks = true;
NeedsSpace = C.SpaceAfter;
}

renderNewlinesMarkdown(OS, ParagraphText);

// A paragraph in markdown is separated by a blank line.
OS << "\n\n";
}
Expand All @@ -512,8 +554,6 @@ 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 Paragraph::chooseMarker(llvm::ArrayRef<llvm::StringRef> Options,
llvm::StringRef Text) const {
// Prefer a delimiter whose characters don't appear in the text.
Expand All @@ -523,23 +563,39 @@ llvm::StringRef Paragraph::chooseMarker(llvm::ArrayRef<llvm::StringRef> Options,
return Options.front();
}

bool Paragraph::punctuationIndicatesLineBreak(llvm::StringRef Line) const {
bool Paragraph::punctuationIndicatesLineBreak(llvm::StringRef Line,
bool IsMarkdown) const {
constexpr llvm::StringLiteral Punctuation = R"txt(.:,;!?)txt";

if (!IsMarkdown && Line.ends_with(" "))
return true;

Line = Line.rtrim();
return !Line.empty() && Punctuation.contains(Line.back());
}

bool Paragraph::isHardLineBreakIndicator(llvm::StringRef Rest) const {
bool Paragraph::isHardLineBreakIndicator(llvm::StringRef Rest,
bool IsMarkdown) const {
// Plaintext indicators:
// '-'/'*' md list, '@'/'\' documentation command, '>' md blockquote,
// '#' headings, '`' code blocks, two spaces (markdown force newline)
constexpr llvm::StringLiteral LinebreakIndicators = R"txt(-*@\>#`)txt";
// '#' headings, '`' code blocks
constexpr llvm::StringLiteral LinebreakIndicatorsPlainText =
R"txt(-*@\>#`)txt";
// Markdown indicators:
// Only '@' and '\' documentation commands/escaped markdown syntax.
constexpr llvm::StringLiteral LinebreakIndicatorsMarkdown = R"txt(@\)txt";

Rest = Rest.ltrim(" \t");
if (Rest.empty())
return false;

if (LinebreakIndicators.contains(Rest.front()))
if (IsMarkdown) {
if (LinebreakIndicatorsMarkdown.contains(Rest.front()))
return true;
return false;
}

if (LinebreakIndicatorsPlainText.contains(Rest.front()))
return true;

if (llvm::isDigit(Rest.front())) {
Expand All @@ -550,64 +606,18 @@ bool Paragraph::isHardLineBreakIndicator(llvm::StringRef Rest) const {
return false;
}

bool Paragraph::isHardLineBreakAfter(llvm::StringRef Line,
llvm::StringRef Rest) const {
// In Markdown, 2 spaces before a line break forces a line break.
// Add a line break for plaintext in this case too.
bool Paragraph::isHardLineBreakAfter(llvm::StringRef Line, llvm::StringRef Rest,
bool IsMarkdown) const {
// Should we also consider whether Line is short?
return Line.ends_with(" ") || punctuationIndicatesLineBreak(Line) ||
isHardLineBreakIndicator(Rest);
return (punctuationIndicatesLineBreak(Line, IsMarkdown) ||
isHardLineBreakIndicator(Rest, IsMarkdown));
}

void Paragraph::renderPlainText(llvm::raw_ostream &OS) const {
bool NeedsSpace = false;
std::string ConcatenatedText;
ConcatenatedText.reserve(EstimatedStringSize);

llvm::raw_string_ostream ConcatenatedOS(ConcatenatedText);

for (auto &C : Chunks) {

if (C.Kind == ChunkKind::PlainText) {
if (C.SpaceBefore || NeedsSpace)
ConcatenatedOS << ' ';

ConcatenatedOS << C.Contents;
NeedsSpace = llvm::isSpace(C.Contents.back()) || C.SpaceAfter;
continue;
}

if (C.SpaceBefore || NeedsSpace)
ConcatenatedOS << ' ';
llvm::StringRef Marker = "";
if (C.Preserve && C.Kind == ChunkKind::InlineCode)
Marker = chooseMarker({"`", "'", "\""}, C.Contents);
else if (C.Kind == ChunkKind::Bold)
Marker = "**";
else if (C.Kind == ChunkKind::Emphasized)
Marker = "*";
ConcatenatedOS << Marker << C.Contents << Marker;
NeedsSpace = C.SpaceAfter;
}

// We go through the contents line by line to handle the newlines
// and required spacing correctly.
//
// Newlines are added if:
// - the line ends with 2 spaces and a newline follows
// - the line ends with punctuation that indicates a line break (.:,;!?)
// - the next line starts with a hard line break indicator (-@>#`, or a digit
// followed by '.' or ')'), ignoring leading whitespace.
//
// Otherwise, newlines in the input are replaced with a single space.
//
// Multiple spaces are collapsed into a single space.
//
// Lines containing only whitespace are ignored.
void Paragraph::renderNewlinesPlaintext(llvm::raw_ostream &OS,
std::string &ParagraphText) const {
llvm::StringRef Line, Rest;

for (std::tie(Line, Rest) =
llvm::StringRef(ConcatenatedText).trim().split('\n');
for (std::tie(Line, Rest) = llvm::StringRef(ParagraphText).trim().split('\n');
!(Line.empty() && Rest.empty());
std::tie(Line, Rest) = Rest.split('\n')) {

Expand All @@ -628,14 +638,48 @@ void Paragraph::renderPlainText(llvm::raw_ostream &OS) const {

OS << canonicalizeSpaces(Line);

if (isHardLineBreakAfter(Line, Rest))
if (isHardLineBreakAfter(Line, Rest, /*IsMarkdown=*/false))
OS << '\n';
else if (!Rest.empty())
// Since we removed any trailing whitespace from the input using trim(),
// we know that the next line contains non-whitespace characters.
// Therefore, we can add a space without worrying about trailing spaces.
OS << ' ';
}
}

void Paragraph::renderPlainText(llvm::raw_ostream &OS) const {
bool NeedsSpace = false;
std::string ParagraphText;
ParagraphText.reserve(EstimatedStringSize);

llvm::raw_string_ostream ParagraphTextOS(ParagraphText);

for (auto &C : Chunks) {

if (C.Kind == ChunkKind::PlainText) {
if (C.SpaceBefore || NeedsSpace)
ParagraphTextOS << ' ';

ParagraphTextOS << C.Contents;
NeedsSpace = llvm::isSpace(C.Contents.back()) || C.SpaceAfter;
continue;
}

if (C.SpaceBefore || NeedsSpace)
ParagraphTextOS << ' ';
llvm::StringRef Marker = "";
if (C.Preserve && C.Kind == ChunkKind::InlineCode)
Marker = chooseMarker({"`", "'", "\""}, C.Contents);
else if (C.Kind == ChunkKind::Bold)
Marker = "**";
else if (C.Kind == ChunkKind::Emphasized)
Marker = "*";
ParagraphTextOS << Marker << C.Contents << Marker;
NeedsSpace = C.SpaceAfter;
}

renderNewlinesPlaintext(OS, ParagraphText);

// Paragraphs are separated by a blank line.
OS << "\n\n";
Expand Down
81 changes: 78 additions & 3 deletions clang-tools-extra/clangd/support/Markup.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,84 @@ class Paragraph : public Block {

llvm::StringRef chooseMarker(llvm::ArrayRef<llvm::StringRef> Options,
llvm::StringRef Text) const;
bool punctuationIndicatesLineBreak(llvm::StringRef Line) const;
bool isHardLineBreakIndicator(llvm::StringRef Rest) const;
bool isHardLineBreakAfter(llvm::StringRef Line, llvm::StringRef Rest) const;

/// Checks if the given line ends with punctuation that indicates a line break
/// (.:,;!?).
///
/// If \p IsMarkdown is false, lines ending with 2 spaces are also considered
/// as indicating a line break. This is not needed for markdown because the
/// client renderer will handle this case.
bool punctuationIndicatesLineBreak(llvm::StringRef Line,
bool IsMarkdown) const;

/// Checks if the given line starts with a hard line break indicator.
///
/// If \p IsMarkdown is true, only '@' and '\' are considered as indicators.
/// Otherwise, '-', '*', '@', '\', '>', '#', '`' and a digit followed by '.'
/// or ')' are also considered as indicators.
bool isHardLineBreakIndicator(llvm::StringRef Rest, bool IsMarkdown) const;

/// Checks if a hard line break should be added after the given line.
bool isHardLineBreakAfter(llvm::StringRef Line, llvm::StringRef Rest,
bool IsMarkdown) const;

/// \brief Go through the contents line by line to handle the newlines
/// and required spacing correctly for markdown rendering.
///
/// Newlines are added if:
/// - the line ends with punctuation that indicates a line break (.:,;!?)
/// - the next line starts with a hard line break indicator \\ (escaped
/// markdown/doxygen command) or @ (doxygen command)
///
/// This newline handling is only used when the client requests markdown
/// for hover/signature help content.
/// Markdown does not add any newlines inside paragraphs unless the user
/// explicitly adds them. For hover/signature help content, we still want to
/// add newlines in some cases to improve readability, especially when doxygen
/// parsing is disabled or not implemented (like for signature help).
/// Therefore we add newlines in the above mentioned cases.
///
/// In addition to that, we need to consider that the user can configure
/// clangd to treat documentation comments as plain text, while the client
/// requests markdown.
/// In this case, all markdown syntax is escaped and will
/// not be rendered as expected by markdown.
/// Examples are lists starting with '-' or headings starting with '#'.
/// With the above next line heuristics, these cases are also covered by the
/// '\\' new line indicator.
///
/// FIXME: The heuristic fails e.g. for lists starting with '*' because it is
/// also used for emphasis in markdown and should not be treated as a newline.
///
/// \param OS The stream to render to.
/// \param ParagraphText The text of the paragraph to render.
void renderNewlinesMarkdown(llvm::raw_ostream &OS,
std::string &ParagraphText) const;

/// \brief Go through the contents line by line to handle the newlines
/// and required spacing correctly for plain text rendering.
///
/// Newlines are added if:
/// - the line ends with 2 spaces and a newline follows
/// - the line ends with punctuation that indicates a line break (.:,;!?)
/// - the next line starts with a hard line break indicator (-@>#`\\ or a
/// digit followed by '.' or ')'), ignoring leading whitespace.
///
/// Otherwise, newlines in the input are replaced with a single space.
///
/// Multiple spaces are collapsed into a single space.
///
/// Lines containing only whitespace are ignored.
///
/// This newline handling is only used when the client requests plain
/// text for hover/signature help content.
/// Therefore with this approach we mimic the behavior of markdown rendering
/// for these clients.
///
/// \param OS The stream to render to.
/// \param ParagraphText The text of the paragraph to render.
void renderNewlinesPlaintext(llvm::raw_ostream &OS,
std::string &ParagraphText) const;
};

/// Represents a sequence of one or more documents. Knows how to print them in a
Expand Down
Loading