From 565cf5a89e0d197d52cd428b0d78556eb88fc9a0 Mon Sep 17 00:00:00 2001 From: blyxyas Date: Fri, 25 Apr 2025 20:25:51 +0200 Subject: [PATCH 1/2] Optimize documentation lints **a lot** (1/2) Turns out that `doc_markdown` uses a non-cheap rustdoc function to convert from markdown ranges into source spans. And it was using it a lot (about once every 18 lines of documentation on `tokio`, which ends up being about 1800 times). This ended up being about 18% of the total Clippy runtime as discovered by lintcheck --perf in docs-heavy crates. This PR optimizes one of the cases in which Clippy calls the function, and a future PR once pulldown-cmark/pulldown-cmark issue number 1034 is merged will be open. Note that not all crates were affected by this crate equally, those with more docs are affected far more than those light ones. --- clippy_lints/src/doc/markdown.rs | 70 +++++++++++++++++++++++++------- clippy_lints/src/doc/mod.rs | 9 ++-- 2 files changed, 60 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/doc/markdown.rs b/clippy_lints/src/doc/markdown.rs index 7a1c7c675d2e..e40b4e20ea45 100644 --- a/clippy_lints/src/doc/markdown.rs +++ b/clippy_lints/src/doc/markdown.rs @@ -6,13 +6,15 @@ use rustc_lint::LateContext; use rustc_span::{BytePos, Pos, Span}; use url::Url; -use crate::doc::DOC_MARKDOWN; +use crate::doc::{DOC_MARKDOWN, Fragments}; +use std::ops::{ControlFlow, Range}; pub fn check( cx: &LateContext<'_>, valid_idents: &FxHashSet, text: &str, - span: Span, + fragments: &Fragments<'_>, + fragment_range: Range, code_level: isize, blockquote_level: isize, ) { @@ -64,23 +66,38 @@ pub fn check( close_parens += 1; } - // Adjust for the current word - let offset = word.as_ptr() as usize - text.as_ptr() as usize; - let span = Span::new( - span.lo() + BytePos::from_usize(offset), - span.lo() + BytePos::from_usize(offset + word.len()), - span.ctxt(), - span.parent(), - ); + // We'll use this offset to calculate the span to lint. + let fragment_offset = word.as_ptr() as usize - text.as_ptr() as usize; - check_word(cx, word, span, code_level, blockquote_level); + // Adjust for the current word + if check_word( + cx, + word, + fragments, + &fragment_range, + fragment_offset, + code_level, + blockquote_level, + ) + .is_break() + { + return; + } } } -fn check_word(cx: &LateContext<'_>, word: &str, span: Span, code_level: isize, blockquote_level: isize) { +fn check_word( + cx: &LateContext<'_>, + word: &str, + fragments: &Fragments<'_>, + range: &Range, + fragment_offset: usize, + code_level: isize, + blockquote_level: isize, +) -> ControlFlow<()> { /// Checks if a string is upper-camel-case, i.e., starts with an uppercase and /// contains at least two uppercase letters (`Clippy` is ok) and one lower-case - /// letter (`NASA` is ok). + /// letter (`NASA` is ok).[ /// Plurals are also excluded (`IDs` is ok). fn is_camel_case(s: &str) -> bool { if s.starts_with(|c: char| c.is_ascii_digit() | c.is_ascii_lowercase()) { @@ -117,6 +134,17 @@ fn check_word(cx: &LateContext<'_>, word: &str, span: Span, code_level: isize, b // try to get around the fact that `foo::bar` parses as a valid URL && !url.cannot_be_a_base() { + let Some(fragment_span) = fragments.span(cx, range.clone()) else { + return ControlFlow::Break(()); + }; + + let span = Span::new( + fragment_span.lo() + BytePos::from_usize(fragment_offset), + fragment_span.lo() + BytePos::from_usize(fragment_offset + word.len()), + fragment_span.ctxt(), + fragment_span.parent(), + ); + span_lint_and_sugg( cx, DOC_MARKDOWN, @@ -126,17 +154,28 @@ fn check_word(cx: &LateContext<'_>, word: &str, span: Span, code_level: isize, b format!("<{word}>"), Applicability::MachineApplicable, ); - return; + return ControlFlow::Continue(()); } // We assume that mixed-case words are not meant to be put inside backticks. (Issue #2343) // // We also assume that backticks are not necessary if inside a quote. (Issue #10262) if code_level > 0 || blockquote_level > 0 || (has_underscore(word) && has_hyphen(word)) { - return; + return ControlFlow::Break(()); } if has_underscore(word) || word.contains("::") || is_camel_case(word) || word.ends_with("()") { + let Some(fragment_span) = fragments.span(cx, range.clone()) else { + return ControlFlow::Break(()); + }; + + let span = Span::new( + fragment_span.lo() + BytePos::from_usize(fragment_offset), + fragment_span.lo() + BytePos::from_usize(fragment_offset + word.len()), + fragment_span.ctxt(), + fragment_span.parent(), + ); + span_lint_and_then( cx, DOC_MARKDOWN, @@ -149,4 +188,5 @@ fn check_word(cx: &LateContext<'_>, word: &str, span: Span, code_level: isize, b }, ); } + ControlFlow::Continue(()) } diff --git a/clippy_lints/src/doc/mod.rs b/clippy_lints/src/doc/mod.rs index ab77edf1147c..5a6c0059e7cd 100644 --- a/clippy_lints/src/doc/mod.rs +++ b/clippy_lints/src/doc/mod.rs @@ -730,7 +730,10 @@ struct Fragments<'a> { } impl Fragments<'_> { - fn span(self, cx: &LateContext<'_>, range: Range) -> Option { + /// get the span for the markdown range. Note that this function is not cheap, use it with + /// caution. + #[must_use] + fn span(&self, cx: &LateContext<'_>, range: Range) -> Option { source_span_for_markdown_range(cx.tcx, self.doc, &range, self.fragments) } } @@ -1068,9 +1071,7 @@ fn check_doc<'a, Events: Iterator, Range Date: Wed, 21 May 2025 23:42:38 +0200 Subject: [PATCH 2/2] Review comments & Add testing --- clippy_lints/src/doc/markdown.rs | 24 +++---- tests/ui/doc/doc_markdown-issue_13097.fixed | 36 ++++++++-- tests/ui/doc/doc_markdown-issue_13097.rs | 36 ++++++++-- tests/ui/doc/doc_markdown-issue_13097.stderr | 74 ++++++++++++++++++-- 4 files changed, 138 insertions(+), 32 deletions(-) diff --git a/clippy_lints/src/doc/markdown.rs b/clippy_lints/src/doc/markdown.rs index e40b4e20ea45..69c3b9150c30 100644 --- a/clippy_lints/src/doc/markdown.rs +++ b/clippy_lints/src/doc/markdown.rs @@ -7,7 +7,7 @@ use rustc_span::{BytePos, Pos, Span}; use url::Url; use crate::doc::{DOC_MARKDOWN, Fragments}; -use std::ops::{ControlFlow, Range}; +use std::ops::Range; pub fn check( cx: &LateContext<'_>, @@ -70,7 +70,7 @@ pub fn check( let fragment_offset = word.as_ptr() as usize - text.as_ptr() as usize; // Adjust for the current word - if check_word( + check_word( cx, word, fragments, @@ -78,11 +78,7 @@ pub fn check( fragment_offset, code_level, blockquote_level, - ) - .is_break() - { - return; - } + ); } } @@ -94,10 +90,10 @@ fn check_word( fragment_offset: usize, code_level: isize, blockquote_level: isize, -) -> ControlFlow<()> { +) { /// Checks if a string is upper-camel-case, i.e., starts with an uppercase and /// contains at least two uppercase letters (`Clippy` is ok) and one lower-case - /// letter (`NASA` is ok).[ + /// letter (`NASA` is ok). /// Plurals are also excluded (`IDs` is ok). fn is_camel_case(s: &str) -> bool { if s.starts_with(|c: char| c.is_ascii_digit() | c.is_ascii_lowercase()) { @@ -135,9 +131,8 @@ fn check_word( && !url.cannot_be_a_base() { let Some(fragment_span) = fragments.span(cx, range.clone()) else { - return ControlFlow::Break(()); + return; }; - let span = Span::new( fragment_span.lo() + BytePos::from_usize(fragment_offset), fragment_span.lo() + BytePos::from_usize(fragment_offset + word.len()), @@ -154,19 +149,19 @@ fn check_word( format!("<{word}>"), Applicability::MachineApplicable, ); - return ControlFlow::Continue(()); + return; } // We assume that mixed-case words are not meant to be put inside backticks. (Issue #2343) // // We also assume that backticks are not necessary if inside a quote. (Issue #10262) if code_level > 0 || blockquote_level > 0 || (has_underscore(word) && has_hyphen(word)) { - return ControlFlow::Break(()); + return; } if has_underscore(word) || word.contains("::") || is_camel_case(word) || word.ends_with("()") { let Some(fragment_span) = fragments.span(cx, range.clone()) else { - return ControlFlow::Break(()); + return; }; let span = Span::new( @@ -188,5 +183,4 @@ fn check_word( }, ); } - ControlFlow::Continue(()) } diff --git a/tests/ui/doc/doc_markdown-issue_13097.fixed b/tests/ui/doc/doc_markdown-issue_13097.fixed index fb0f40b34a4b..e0136584f3de 100644 --- a/tests/ui/doc/doc_markdown-issue_13097.fixed +++ b/tests/ui/doc/doc_markdown-issue_13097.fixed @@ -1,11 +1,37 @@ -// This test checks that words starting with capital letters and ending with "ified" don't -// trigger the lint. - #![deny(clippy::doc_markdown)] +#![allow(clippy::doc_lazy_continuation)] + +mod issue13097 { + // This test checks that words starting with capital letters and ending with "ified" don't + // trigger the lint. + pub enum OutputFormat { + /// `HumaNified` + //~^ ERROR: item in documentation is missing backticks + Plain, + // Should not warn! + /// JSONified console output + Json, + } +} +#[rustfmt::skip] pub enum OutputFormat { - /// `HumaNified` - //~^ ERROR: item in documentation is missing backticks + /** + * `HumaNified` + //~^ ERROR: item in documentation is missing backticks + * Before \u{08888} `HumaNified` \{u08888} After + //~^ ERROR: item in documentation is missing backticks + * meow meow \[`meow_meow`\] meow meow? + //~^ ERROR: item in documentation is missing backticks + * \u{08888} `meow_meow` \[meow meow] meow? + //~^ ERROR: item in documentation is missing backticks + * Above + * \u{08888} + * \[hi\]() `HumaNified` \[example]() + //~^ ERROR: item in documentation is missing backticks + * \u{08888} + * Below + */ Plain, // Should not warn! /// JSONified console output diff --git a/tests/ui/doc/doc_markdown-issue_13097.rs b/tests/ui/doc/doc_markdown-issue_13097.rs index 8c1e1a3cd6c2..2e89fe6c56b4 100644 --- a/tests/ui/doc/doc_markdown-issue_13097.rs +++ b/tests/ui/doc/doc_markdown-issue_13097.rs @@ -1,11 +1,37 @@ -// This test checks that words starting with capital letters and ending with "ified" don't -// trigger the lint. - #![deny(clippy::doc_markdown)] +#![allow(clippy::doc_lazy_continuation)] + +mod issue13097 { + // This test checks that words starting with capital letters and ending with "ified" don't + // trigger the lint. + pub enum OutputFormat { + /// HumaNified + //~^ ERROR: item in documentation is missing backticks + Plain, + // Should not warn! + /// JSONified console output + Json, + } +} +#[rustfmt::skip] pub enum OutputFormat { - /// HumaNified - //~^ ERROR: item in documentation is missing backticks + /** + * HumaNified + //~^ ERROR: item in documentation is missing backticks + * Before \u{08888} HumaNified \{u08888} After + //~^ ERROR: item in documentation is missing backticks + * meow meow \[meow_meow\] meow meow? + //~^ ERROR: item in documentation is missing backticks + * \u{08888} meow_meow \[meow meow] meow? + //~^ ERROR: item in documentation is missing backticks + * Above + * \u{08888} + * \[hi\]() HumaNified \[example]() + //~^ ERROR: item in documentation is missing backticks + * \u{08888} + * Below + */ Plain, // Should not warn! /// JSONified console output diff --git a/tests/ui/doc/doc_markdown-issue_13097.stderr b/tests/ui/doc/doc_markdown-issue_13097.stderr index 65b8f2ed80b6..cea788301d4d 100644 --- a/tests/ui/doc/doc_markdown-issue_13097.stderr +++ b/tests/ui/doc/doc_markdown-issue_13097.stderr @@ -1,19 +1,79 @@ error: item in documentation is missing backticks - --> tests/ui/doc/doc_markdown-issue_13097.rs:7:9 + --> tests/ui/doc/doc_markdown-issue_13097.rs:8:13 | -LL | /// HumaNified - | ^^^^^^^^^^ +LL | /// HumaNified + | ^^^^^^^^^^ | note: the lint level is defined here - --> tests/ui/doc/doc_markdown-issue_13097.rs:4:9 + --> tests/ui/doc/doc_markdown-issue_13097.rs:1:9 | LL | #![deny(clippy::doc_markdown)] | ^^^^^^^^^^^^^^^^^^^^ help: try | -LL - /// HumaNified -LL + /// `HumaNified` +LL - /// HumaNified +LL + /// `HumaNified` | -error: aborting due to 1 previous error +error: item in documentation is missing backticks + --> tests/ui/doc/doc_markdown-issue_13097.rs:20:8 + | +LL | * HumaNified + | ^^^^^^^^^^ + | +help: try + | +LL - * HumaNified +LL + * `HumaNified` + | + +error: item in documentation is missing backticks + --> tests/ui/doc/doc_markdown-issue_13097.rs:22:25 + | +LL | * Before \u{08888} HumaNified \{u08888} After + | ^^^^^^^^^^ + | +help: try + | +LL - * Before \u{08888} HumaNified \{u08888} After +LL + * Before \u{08888} `HumaNified` \{u08888} After + | + +error: item in documentation is missing backticks + --> tests/ui/doc/doc_markdown-issue_13097.rs:24:20 + | +LL | * meow meow \[meow_meow\] meow meow? + | ^^^^^^^^^ + | +help: try + | +LL - * meow meow \[meow_meow\] meow meow? +LL + * meow meow \[`meow_meow`\] meow meow? + | + +error: item in documentation is missing backticks + --> tests/ui/doc/doc_markdown-issue_13097.rs:26:18 + | +LL | * \u{08888} meow_meow \[meow meow] meow? + | ^^^^^^^^^ + | +help: try + | +LL - * \u{08888} meow_meow \[meow meow] meow? +LL + * \u{08888} `meow_meow` \[meow meow] meow? + | + +error: item in documentation is missing backticks + --> tests/ui/doc/doc_markdown-issue_13097.rs:30:38 + | +LL | * \[hi\]() HumaNified \[example]() + | ^^^^^^^^^^ + | +help: try + | +LL - * \[hi\]() HumaNified \[example]() +LL + * \[hi\]() `HumaNified` \[example]() + | + +error: aborting due to 6 previous errors