From 045e36d6a7b797e2eef8ee92bce11a499c319a5b Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Thu, 30 Jan 2025 14:57:09 -0700 Subject: [PATCH 1/2] doc_link_code: add check for `text`[`adjacent`] style links This is the lint described at https://github.com/rust-lang/rust/pull/136308#issuecomment-2625485331 that recommends using HTML to nest links inside code. --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/doc/mod.rs | 62 +++++++++++++++ tests/ui/doc/link_adjacent.fixed | 52 ++++++++++++ tests/ui/doc/link_adjacent.rs | 52 ++++++++++++ tests/ui/doc/link_adjacent.stderr | 124 +++++++++++++++++++++++++++++ 6 files changed, 292 insertions(+) create mode 100644 tests/ui/doc/link_adjacent.fixed create mode 100644 tests/ui/doc/link_adjacent.rs create mode 100644 tests/ui/doc/link_adjacent.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index cb149ccfeead..4909f15ecaea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5530,6 +5530,7 @@ Released 2018-09-13 [`diverging_sub_expression`]: https://rust-lang.github.io/rust-clippy/master/index.html#diverging_sub_expression [`doc_include_without_cfg`]: https://rust-lang.github.io/rust-clippy/master/index.html#doc_include_without_cfg [`doc_lazy_continuation`]: https://rust-lang.github.io/rust-clippy/master/index.html#doc_lazy_continuation +[`doc_link_code`]: https://rust-lang.github.io/rust-clippy/master/index.html#doc_link_code [`doc_link_with_quotes`]: https://rust-lang.github.io/rust-clippy/master/index.html#doc_link_with_quotes [`doc_markdown`]: https://rust-lang.github.io/rust-clippy/master/index.html#doc_markdown [`doc_nested_refdefs`]: https://rust-lang.github.io/rust-clippy/master/index.html#doc_nested_refdefs diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 1ce6f1256177..95dcd2c16482 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -139,6 +139,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::disallowed_types::DISALLOWED_TYPES_INFO, crate::doc::DOC_INCLUDE_WITHOUT_CFG_INFO, crate::doc::DOC_LAZY_CONTINUATION_INFO, + crate::doc::DOC_LINK_CODE_INFO, crate::doc::DOC_LINK_WITH_QUOTES_INFO, crate::doc::DOC_MARKDOWN_INFO, crate::doc::DOC_NESTED_REFDEFS_INFO, diff --git a/clippy_lints/src/doc/mod.rs b/clippy_lints/src/doc/mod.rs index 15530c3dbc50..c0ee904002c9 100644 --- a/clippy_lints/src/doc/mod.rs +++ b/clippy_lints/src/doc/mod.rs @@ -84,6 +84,28 @@ declare_clippy_lint! { "presence of `_`, `::` or camel-case outside backticks in documentation" } +declare_clippy_lint! { + /// ### What it does + /// Checks for links with code directly adjacent to code text: + /// `` [`MyItem`]`<`[`u32`]`>` ``. + /// + /// ### Why is this bad? + /// It can be written more simply using HTML-style `` tags. + /// + /// ### Example + /// ```no_run + /// //! [`first`](x)`second` + /// ``` + /// Use instead: + /// ```no_run + /// //! [first](x)second + /// ``` + #[clippy::version = "1.86.0"] + pub DOC_LINK_CODE, + nursery, + "link with code back-to-back with other code" +} + declare_clippy_lint! { /// ### What it does /// Checks for the doc comments of publicly visible @@ -638,6 +660,7 @@ impl Documentation { } impl_lint_pass!(Documentation => [ + DOC_LINK_CODE, DOC_LINK_WITH_QUOTES, DOC_MARKDOWN, DOC_NESTED_REFDEFS, @@ -844,6 +867,14 @@ enum Container { List(usize), } +#[derive(Clone, Copy, Eq, PartialEq)] +enum CodeCluster { + // true means already in a link, so only needs to be followed by code + // false means we've hit code, and need to find a link + First(usize, bool), + Nth(usize, usize), +} + /// Checks parsed documentation. /// This walks the "events" (think sections of markdown) produced by `pulldown_cmark`, /// so lints here will generally access that information. @@ -875,9 +906,40 @@ fn check_doc<'a, Events: Iterator, Range { + Some(CodeCluster::First(range.start - 1, true)) + }, + (None, Code(_)) => Some(CodeCluster::First(range.start, false)), + (Some(CodeCluster::First(pos, _)), Start(Link { .. })) | (Some(CodeCluster::First(pos, true)), Code(_)) => { + Some(CodeCluster::Nth(pos, range.end)) + }, + (Some(CodeCluster::Nth(start, end)), Code(_) | Start(Link { .. })) => { + Some(CodeCluster::Nth(start, range.end.max(end))) + }, + (code_cluster @ Some(_), Code(_) | End(TagEnd::Link)) => code_cluster, + (Some(CodeCluster::First(_, _)) | None, _) => None, + (Some(CodeCluster::Nth(start, end)), _) => { + if let Some(span) = fragments.span(cx, start..end) { + span_lint_and_then(cx, DOC_LINK_CODE, span, "code link adjacent to code text", |diag| { + let sugg = format!("{}", doc[start..end].replace('`', "")); + diag.span_suggestion_verbose( + span, + "wrap the entire group in `` tags", + sugg, + Applicability::MaybeIncorrect, + ); + diag.help("separate code snippets will be shown with a gap"); + }); + } + None + }, + }; match event { Html(tag) | InlineHtml(tag) => { if tag.starts_with("[first](x)second +//~^ ERROR: adjacent +//! +//! So is this first[second](x) +//~^ ERROR: adjacent +//! +//! So is this [first](x)[second](x) +//~^ ERROR: adjacent +//! +//! So is this [first](x)[second](x)[third](x) +//~^ ERROR: adjacent +//! +//! So is this [first](x)second[third](x) +//~^ ERROR: adjacent + +/// Test case for code links that are adjacent to code text. +/// +/// This is not an example: `first``second` arst +/// +/// Neither is this: [`first`](x) arst +/// +/// Neither is this: [`first`](x) `second` arst +/// +/// Neither is this: [first](x)`second` arst +/// +/// This is: [first](x)second arst +//~^ ERROR: adjacent +/// +/// So is this first[second](x) arst +//~^ ERROR: adjacent +/// +/// So is this [first](x)[second](x) arst +//~^ ERROR: adjacent +/// +/// So is this [first](x)[second](x)[third](x) arst +//~^ ERROR: adjacent +/// +/// So is this [first](x)second[third](x) arst +//~^ ERROR: adjacent +pub struct WithTrailing; diff --git a/tests/ui/doc/link_adjacent.rs b/tests/ui/doc/link_adjacent.rs new file mode 100644 index 000000000000..af6755eeff61 --- /dev/null +++ b/tests/ui/doc/link_adjacent.rs @@ -0,0 +1,52 @@ +#![warn(clippy::doc_link_code)] + +//! Test case for code links that are adjacent to code text. +//! +//! This is not an example: `first``second` +//! +//! Neither is this: [`first`](x) +//! +//! Neither is this: [`first`](x) `second` +//! +//! Neither is this: [first](x)`second` +//! +//! This is: [`first`](x)`second` +//~^ ERROR: adjacent +//! +//! So is this `first`[`second`](x) +//~^ ERROR: adjacent +//! +//! So is this [`first`](x)[`second`](x) +//~^ ERROR: adjacent +//! +//! So is this [`first`](x)[`second`](x)[`third`](x) +//~^ ERROR: adjacent +//! +//! So is this [`first`](x)`second`[`third`](x) +//~^ ERROR: adjacent + +/// Test case for code links that are adjacent to code text. +/// +/// This is not an example: `first``second` arst +/// +/// Neither is this: [`first`](x) arst +/// +/// Neither is this: [`first`](x) `second` arst +/// +/// Neither is this: [first](x)`second` arst +/// +/// This is: [`first`](x)`second` arst +//~^ ERROR: adjacent +/// +/// So is this `first`[`second`](x) arst +//~^ ERROR: adjacent +/// +/// So is this [`first`](x)[`second`](x) arst +//~^ ERROR: adjacent +/// +/// So is this [`first`](x)[`second`](x)[`third`](x) arst +//~^ ERROR: adjacent +/// +/// So is this [`first`](x)`second`[`third`](x) arst +//~^ ERROR: adjacent +pub struct WithTrailing; diff --git a/tests/ui/doc/link_adjacent.stderr b/tests/ui/doc/link_adjacent.stderr new file mode 100644 index 000000000000..f09762fb6a0b --- /dev/null +++ b/tests/ui/doc/link_adjacent.stderr @@ -0,0 +1,124 @@ +error: code link adjacent to code text + --> tests/ui/doc/link_adjacent.rs:13:14 + | +LL | //! This is: [`first`](x)`second` + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: separate code snippets will be shown with a gap + = note: `-D clippy::doc-link-code` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::doc_link_code)]` +help: wrap the entire group in `` tags + | +LL | //! This is: [first](x)second + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: code link adjacent to code text + --> tests/ui/doc/link_adjacent.rs:16:16 + | +LL | //! So is this `first`[`second`](x) + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: separate code snippets will be shown with a gap +help: wrap the entire group in `` tags + | +LL | //! So is this first[second](x) + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: code link adjacent to code text + --> tests/ui/doc/link_adjacent.rs:19:16 + | +LL | //! So is this [`first`](x)[`second`](x) + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: separate code snippets will be shown with a gap +help: wrap the entire group in `` tags + | +LL | //! So is this [first](x)[second](x) + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: code link adjacent to code text + --> tests/ui/doc/link_adjacent.rs:22:16 + | +LL | //! So is this [`first`](x)[`second`](x)[`third`](x) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: separate code snippets will be shown with a gap +help: wrap the entire group in `` tags + | +LL | //! So is this [first](x)[second](x)[third](x) + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: code link adjacent to code text + --> tests/ui/doc/link_adjacent.rs:25:16 + | +LL | //! So is this [`first`](x)`second`[`third`](x) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: separate code snippets will be shown with a gap +help: wrap the entire group in `` tags + | +LL | //! So is this [first](x)second[third](x) + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: code link adjacent to code text + --> tests/ui/doc/link_adjacent.rs:38:14 + | +LL | /// This is: [`first`](x)`second` arst + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: separate code snippets will be shown with a gap +help: wrap the entire group in `` tags + | +LL | /// This is: [first](x)second arst + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: code link adjacent to code text + --> tests/ui/doc/link_adjacent.rs:41:16 + | +LL | /// So is this `first`[`second`](x) arst + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: separate code snippets will be shown with a gap +help: wrap the entire group in `` tags + | +LL | /// So is this first[second](x) arst + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: code link adjacent to code text + --> tests/ui/doc/link_adjacent.rs:44:16 + | +LL | /// So is this [`first`](x)[`second`](x) arst + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: separate code snippets will be shown with a gap +help: wrap the entire group in `` tags + | +LL | /// So is this [first](x)[second](x) arst + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: code link adjacent to code text + --> tests/ui/doc/link_adjacent.rs:47:16 + | +LL | /// So is this [`first`](x)[`second`](x)[`third`](x) arst + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: separate code snippets will be shown with a gap +help: wrap the entire group in `` tags + | +LL | /// So is this [first](x)[second](x)[third](x) arst + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: code link adjacent to code text + --> tests/ui/doc/link_adjacent.rs:50:16 + | +LL | /// So is this [`first`](x)`second`[`third`](x) arst + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: separate code snippets will be shown with a gap +help: wrap the entire group in `` tags + | +LL | /// So is this [first](x)second[third](x) arst + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: aborting due to 10 previous errors + From aff497f17f4264fcaaa36d4db83968bbe2b8ef19 Mon Sep 17 00:00:00 2001 From: Michael Howell Date: Mon, 10 Feb 2025 14:32:36 -0700 Subject: [PATCH 2/2] Use a separate loop to drive the check for code clusters By using a separate loop, I can just skip nodes that I don't want to process twice, instead of having to hand-build a state machine with an enum. --- clippy_lints/src/doc/mod.rs | 110 ++++++++++++++++++++++++------------ 1 file changed, 73 insertions(+), 37 deletions(-) diff --git a/clippy_lints/src/doc/mod.rs b/clippy_lints/src/doc/mod.rs index c0ee904002c9..fc6d6c4282cd 100644 --- a/clippy_lints/src/doc/mod.rs +++ b/clippy_lints/src/doc/mod.rs @@ -844,6 +844,21 @@ fn check_attrs(cx: &LateContext<'_>, valid_idents: &FxHashSet, attrs: &[ let mut cb = fake_broken_link_callback; + check_for_code_clusters( + cx, + pulldown_cmark::Parser::new_with_broken_link_callback( + &doc, + main_body_opts() - Options::ENABLE_SMART_PUNCTUATION, + Some(&mut cb), + ) + .into_offset_iter(), + &doc, + Fragments { + doc: &doc, + fragments: &fragments, + }, + ); + // disable smart punctuation to pick up ['link'] more easily let opts = main_body_opts() - Options::ENABLE_SMART_PUNCTUATION; let parser = pulldown_cmark::Parser::new_with_broken_link_callback(&doc, opts, Some(&mut cb)); @@ -867,12 +882,64 @@ enum Container { List(usize), } -#[derive(Clone, Copy, Eq, PartialEq)] -enum CodeCluster { - // true means already in a link, so only needs to be followed by code - // false means we've hit code, and need to find a link - First(usize, bool), - Nth(usize, usize), +/// Scan the documentation for code links that are back-to-back with code spans. +/// +/// This is done separately from the rest of the docs, because that makes it easier to produce +/// the correct messages. +fn check_for_code_clusters<'a, Events: Iterator, Range)>>( + cx: &LateContext<'_>, + events: Events, + doc: &str, + fragments: Fragments<'_>, +) { + let mut events = events.peekable(); + let mut code_starts_at = None; + let mut code_ends_at = None; + let mut code_includes_link = false; + while let Some((event, range)) = events.next() { + match event { + Start(Link { .. }) if matches!(events.peek(), Some((Code(_), _range))) => { + if code_starts_at.is_some() { + code_ends_at = Some(range.end); + } else { + code_starts_at = Some(range.start); + } + code_includes_link = true; + // skip the nested "code", because we're already handling it here + let _ = events.next(); + }, + Code(_) => { + if code_starts_at.is_some() { + code_ends_at = Some(range.end); + } else { + code_starts_at = Some(range.start); + } + }, + End(TagEnd::Link) => {}, + _ => { + if let Some(start) = code_starts_at + && let Some(end) = code_ends_at + && code_includes_link + { + if let Some(span) = fragments.span(cx, start..end) { + span_lint_and_then(cx, DOC_LINK_CODE, span, "code link adjacent to code text", |diag| { + let sugg = format!("{}", doc[start..end].replace('`', "")); + diag.span_suggestion_verbose( + span, + "wrap the entire group in `` tags", + sugg, + Applicability::MaybeIncorrect, + ); + diag.help("separate code snippets will be shown with a gap"); + }); + } + } + code_includes_link = false; + code_starts_at = None; + code_ends_at = None; + }, + } + } } /// Checks parsed documentation. @@ -906,40 +973,9 @@ fn check_doc<'a, Events: Iterator, Range { - Some(CodeCluster::First(range.start - 1, true)) - }, - (None, Code(_)) => Some(CodeCluster::First(range.start, false)), - (Some(CodeCluster::First(pos, _)), Start(Link { .. })) | (Some(CodeCluster::First(pos, true)), Code(_)) => { - Some(CodeCluster::Nth(pos, range.end)) - }, - (Some(CodeCluster::Nth(start, end)), Code(_) | Start(Link { .. })) => { - Some(CodeCluster::Nth(start, range.end.max(end))) - }, - (code_cluster @ Some(_), Code(_) | End(TagEnd::Link)) => code_cluster, - (Some(CodeCluster::First(_, _)) | None, _) => None, - (Some(CodeCluster::Nth(start, end)), _) => { - if let Some(span) = fragments.span(cx, start..end) { - span_lint_and_then(cx, DOC_LINK_CODE, span, "code link adjacent to code text", |diag| { - let sugg = format!("{}", doc[start..end].replace('`', "")); - diag.span_suggestion_verbose( - span, - "wrap the entire group in `` tags", - sugg, - Applicability::MaybeIncorrect, - ); - diag.help("separate code snippets will be shown with a gap"); - }); - } - None - }, - }; match event { Html(tag) | InlineHtml(tag) => { if tag.starts_with("