Skip to content

Parse doc comment markdown only once #16144

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
Alexendoo wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Parse doc comment markdown only once #16144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a8cb5d
Reuse existing markdown parser in doc_paragraphs_missing_punctuation
Alexendoo Nov 26, 2025
fb579d1
Reuse existing markdown parser in doc_link_code
Alexendoo Nov 26, 2025
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
90 changes: 90 additions & 0 deletions clippy_lints/src/doc/doc_link_code.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
use std::mem;
use std::ops::Range;

use clippy_utils::diagnostics::span_lint_and_then;
use rustc_errors::Applicability;
use rustc_lint::LateContext;
use rustc_resolve::rustdoc::pulldown_cmark::{Event, Tag, TagEnd};

use crate::doc::Fragments;

use super::DOC_LINK_CODE;

struct PendingLink {
range: Range<usize>,
seen_code: bool,
}

#[derive(Default)]
pub(super) struct LinkCode {
start: Option<usize>,
end: Option<usize>,
includes_link: bool,
pending_link: Option<PendingLink>,
}

impl LinkCode {
pub fn check(
&mut self,
cx: &LateContext<'_>,
event: &Event<'_>,
range: Range<usize>,
doc: &str,
fragments: Fragments<'_>,
) {
match event {
Event::Start(Tag::Link { .. }) => {
self.pending_link = Some(PendingLink {
range,
seen_code: false,
});
},
Event::End(TagEnd::Link) => {
if let Some(PendingLink { range, seen_code: true }) = self.pending_link.take() {
if self.start.is_some() {
self.end = Some(range.end);
} else {
self.start = Some(range.start);
}
self.includes_link = true;
}
},
Copy link
Contributor

@notriddle notriddle Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bug in this new version that isn't in the current version. Unfortunately, there's no test case for them in the code, but here's a couple of test cases that catch it (they pass in the main branch, but fail in this PR's branch):

/// `first`[`second`](https://example.com)[third](https://example.com)
//~^ ERROR: adjacent
pub struct FalseNegative;
/// `first`[](https://example.com)
pub struct FalsePositive;

Copy link
Member Author

@Alexendoo Alexendoo Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it now verifies that links contain a single code and nothing, which catches a FP on:

`first`[`second` not code](x)

Separate to this PR but I think it'd be worth moving out of nursery unless there's a known issue I'm missing

Copy link
Member Author

@Alexendoo Alexendoo Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an aside what does the arst at the end of the test file lines mean? As a brit I cannot help but read it as "arsed"

Copy link
Contributor

@notriddle notriddle Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use the Colemak layout, and arst just happens to be the left hand home row keys.

_ if let Some(pending_link) = &mut self.pending_link => {
if matches!(event, Event::Code(_)) && !pending_link.seen_code {
pending_link.seen_code = true;
} else {
self.consume(cx, fragments, doc);
}
},
Event::Code(_) => {
if self.start.is_some() {
self.end = Some(range.end);
} else {
self.start = Some(range.start);
}
},
_ => self.consume(cx, fragments, doc),
}
}

fn consume(&mut self, cx: &LateContext<'_>, fragments: Fragments<'_>, doc: &str) {
if let LinkCode {
start: Some(start),
end: Some(end),
includes_link: true,
pending_link: _,
} = mem::take(self)
&& let Some(span) = fragments.span(cx, start..end)
{
span_lint_and_then(cx, DOC_LINK_CODE, span, "code link adjacent to code text", |diag| {
diag.span_suggestion_verbose(
span,
"wrap the entire group in `<code>` tags",
format!("<code>{}</code>", doc[start..end].replace('`', "")),
Applicability::MaybeIncorrect,
);
diag.help("separate code snippets will be shown with a gap");
});
}
}
}
138 changes: 64 additions & 74 deletions clippy_lints/src/doc/doc_paragraphs_missing_punctuation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,62 +1,31 @@
use clippy_utils::diagnostics::span_lint_and_then;
use rustc_errors::Applicability;
use rustc_lint::LateContext;
use rustc_resolve::rustdoc::main_body_opts;

use rustc_resolve::rustdoc::pulldown_cmark::{Event, Options, Parser, Tag, TagEnd};
use rustc_resolve::rustdoc::pulldown_cmark::{Event, Tag, TagEnd};
use rustc_span::Span;
use std::ops::Range;

use super::{DOC_PARAGRAPHS_MISSING_PUNCTUATION, Fragments};

const MSG: &str = "doc paragraphs should end with a terminal punctuation mark";
const PUNCTUATION_SUGGESTION: char = '.';

pub fn check(cx: &LateContext<'_>, doc: &str, fragments: Fragments<'_>) {
for missing_punctuation in is_missing_punctuation(doc) {
match missing_punctuation {
MissingPunctuation::Fixable(offset) => {
// This ignores `#[doc]` attributes, which we do not handle.
if let Some(span) = fragments.span(cx, offset..offset) {
clippy_utils::diagnostics::span_lint_and_sugg(
cx,
DOC_PARAGRAPHS_MISSING_PUNCTUATION,
span,
MSG,
"end the paragraph with some punctuation",
PUNCTUATION_SUGGESTION.to_string(),
Applicability::MaybeIncorrect,
);
}
},
MissingPunctuation::Unfixable(offset) => {
// This ignores `#[doc]` attributes, which we do not handle.
if let Some(span) = fragments.span(cx, offset..offset) {
clippy_utils::diagnostics::span_lint_and_help(
cx,
DOC_PARAGRAPHS_MISSING_PUNCTUATION,
span,
MSG,
None,
"end the paragraph with some punctuation",
);
}
},
}
}
#[derive(Default)]
pub(super) struct MissingPunctuation {
no_report_depth: u32,
current_paragraph: Option<Position>,
}

#[must_use]
/// If punctuation is missing, returns the offset where new punctuation should be inserted.
fn is_missing_punctuation(doc_string: &str) -> Vec<MissingPunctuation> {
// The colon is not exactly a terminal punctuation mark, but this is required for paragraphs that
// introduce a table or a list for example.
const TERMINAL_PUNCTUATION_MARKS: &[char] = &['.', '?', '!', '…', ':'];
impl MissingPunctuation {
pub fn check(
&mut self,
cx: &LateContext<'_>,
event: &Event<'_>,
range: Range<usize>,
doc: &str,
fragments: Fragments<'_>,
) {
// The colon is not exactly a terminal punctuation mark, but this is required for paragraphs that
// introduce a table or a list for example.
const TERMINAL_PUNCTUATION_MARKS: &[char] = &['.', '?', '!', '…', ':'];

let mut no_report_depth = 0;
let mut missing_punctuation = Vec::new();
let mut current_paragraph = None;

for (event, offset) in
Parser::new_ext(doc_string, main_body_opts() - Options::ENABLE_SMART_PUNCTUATION).into_offset_iter()
{
match event {
Event::Start(
Tag::CodeBlock(..)
Expand All @@ -66,61 +35,82 @@ fn is_missing_punctuation(doc_string: &str) -> Vec<MissingPunctuation> {
| Tag::List(..)
| Tag::Table(_),
) => {
no_report_depth += 1;
self.no_report_depth += 1;
},
Event::End(TagEnd::FootnoteDefinition) => {
no_report_depth -= 1;
self.no_report_depth -= 1;
},
Event::End(
TagEnd::CodeBlock | TagEnd::Heading(_) | TagEnd::HtmlBlock | TagEnd::List(_) | TagEnd::Table,
) => {
no_report_depth -= 1;
current_paragraph = None;
self.no_report_depth -= 1;
self.current_paragraph = None;
},
Event::InlineHtml(_) | Event::Start(Tag::Image { .. }) | Event::End(TagEnd::Image) => {
current_paragraph = None;
self.current_paragraph = None;
},
Event::End(TagEnd::Paragraph) => {
if let Some(mp) = current_paragraph {
missing_punctuation.push(mp);
if let Some(position) = self.current_paragraph
&& let Some(span) = position.span(cx, fragments)
{
span_lint_and_then(
cx,
DOC_PARAGRAPHS_MISSING_PUNCTUATION,
span,
"doc paragraphs should end with a terminal punctuation mark",
|diag| {
if matches!(position, Position::Fixable(_)) {
diag.span_suggestion(
span,
"end the paragraph with some punctuation",
'.',
Applicability::MaybeIncorrect,
);
} else {
diag.help("end the paragraph with some punctuation");
}
},
);
}
},
Event::Code(..) | Event::Start(Tag::Link { .. }) | Event::End(TagEnd::Link)
if no_report_depth == 0 && !offset.is_empty() =>
if self.no_report_depth == 0 && !range.is_empty() =>
{
if doc_string[..offset.end]
.trim_end()
.ends_with(TERMINAL_PUNCTUATION_MARKS)
{
current_paragraph = None;
if doc[..range.end].trim_end().ends_with(TERMINAL_PUNCTUATION_MARKS) {
self.current_paragraph = None;
} else {
current_paragraph = Some(MissingPunctuation::Fixable(offset.end));
self.current_paragraph = Some(Position::Fixable(range.end));
}
},
Event::Text(..) if no_report_depth == 0 && !offset.is_empty() => {
let trimmed = doc_string[..offset.end].trim_end();
Event::Text(..) if self.no_report_depth == 0 && !range.is_empty() => {
let trimmed = doc[..range.end].trim_end();
if trimmed.ends_with(TERMINAL_PUNCTUATION_MARKS) {
current_paragraph = None;
self.current_paragraph = None;
} else if let Some(t) = trimmed.strip_suffix(|c| c == ')' || c == '"') {
if t.ends_with(TERMINAL_PUNCTUATION_MARKS) {
// Avoid false positives.
current_paragraph = None;
self.current_paragraph = None;
} else {
current_paragraph = Some(MissingPunctuation::Unfixable(offset.end));
self.current_paragraph = Some(Position::Unfixable(range.end));
}
} else {
current_paragraph = Some(MissingPunctuation::Fixable(offset.end));
self.current_paragraph = Some(Position::Fixable(range.end));
}
},
_ => {},
}
}

missing_punctuation
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum MissingPunctuation {
enum Position {
Fixable(usize),
Unfixable(usize),
}

impl Position {
fn span(self, cx: &LateContext<'_>, fragments: Fragments<'_>) -> Option<Span> {
let (Position::Fixable(pos) | Position::Unfixable(pos)) = self;
fragments.span(cx, pos..pos)
}
}
Loading