From 885ce610fe2b257d2263e28954ab11a92e0372cf Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Wed, 12 Jul 2023 03:19:22 -0500 Subject: [PATCH 1/2] New lint [`four_forward_slashes`] Make it trim the contents --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/four_forward_slashes.rs | 82 ++++++++++++++++++++++++ clippy_lints/src/lib.rs | 2 + tests/ui/four_forward_slashes.fixed | 44 +++++++++++++ tests/ui/four_forward_slashes.rs | 44 +++++++++++++ tests/ui/four_forward_slashes.stderr | 75 ++++++++++++++++++++++ 7 files changed, 249 insertions(+) create mode 100644 clippy_lints/src/four_forward_slashes.rs create mode 100644 tests/ui/four_forward_slashes.fixed create mode 100644 tests/ui/four_forward_slashes.rs create mode 100644 tests/ui/four_forward_slashes.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index b3b6e3b865f..ec62d399aa4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4867,6 +4867,7 @@ Released 2018-09-13 [`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref [`format_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#format_in_format_args [`format_push_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#format_push_string +[`four_forward_slashes`]: https://rust-lang.github.io/rust-clippy/master/index.html#four_forward_slashes [`from_iter_instead_of_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_iter_instead_of_collect [`from_over_into`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into [`from_raw_with_void_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_raw_with_void_ptr diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 3723de7299b..d8b14a9f967 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -184,6 +184,7 @@ crate::formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING_INFO, crate::formatting::SUSPICIOUS_ELSE_FORMATTING_INFO, crate::formatting::SUSPICIOUS_UNARY_OP_FORMATTING_INFO, + crate::four_forward_slashes::FOUR_FORWARD_SLASHES_INFO, crate::from_over_into::FROM_OVER_INTO_INFO, crate::from_raw_with_void_ptr::FROM_RAW_WITH_VOID_PTR_INFO, crate::from_str_radix_10::FROM_STR_RADIX_10_INFO, diff --git a/clippy_lints/src/four_forward_slashes.rs b/clippy_lints/src/four_forward_slashes.rs new file mode 100644 index 00000000000..7efa8c1b31f --- /dev/null +++ b/clippy_lints/src/four_forward_slashes.rs @@ -0,0 +1,82 @@ +use clippy_utils::{diagnostics::span_lint_and_sugg, source::snippet_opt}; +use rustc_errors::Applicability; +use rustc_hir::Item; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{Span, SyntaxContext}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for outer doc comments written with 4 forward slashes (`////`). + /// + /// ### Why is this bad? + /// This is (probably) a typo, and results in it not being a doc comment; just a regular + /// comment. + /// + /// ### Example + /// ```rust + /// //// My amazing data structure + /// pub struct Foo { + /// // ... + /// } + /// ``` + /// + /// Use instead: + /// ```rust + /// /// My amazing data structure + /// pub struct Foo { + /// // ... + /// } + /// ``` + #[clippy::version = "1.72.0"] + pub FOUR_FORWARD_SLASHES, + suspicious, + "comments with 4 forward slashes (`////`) likely intended to be doc comments (`///`)" +} +declare_lint_pass!(FourForwardSlashes => [FOUR_FORWARD_SLASHES]); + +impl<'tcx> LateLintPass<'tcx> for FourForwardSlashes { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { + if item.span.from_expansion() { + return; + } + let src = cx.sess().source_map(); + let item_and_attrs_span = cx + .tcx + .hir() + .attrs(item.hir_id()) + .iter() + .fold(item.span.shrink_to_lo(), |span, attr| span.to(attr.span)); + let (Some(file), _, _, end_line, _) = src.span_to_location_info(item_and_attrs_span) else { + return; + }; + for line in (0..end_line.saturating_sub(1)).rev() { + let Some(contents) = file.get_line(line) else { + continue; + }; + let contents = contents.trim(); + if contents.is_empty() { + break; + } + if contents.starts_with("////") { + let bounds = file.line_bounds(line); + let span = Span::new(bounds.start, bounds.end, SyntaxContext::root(), None); + + if snippet_opt(cx, span).is_some_and(|s| s.trim().starts_with("////")) { + span_lint_and_sugg( + cx, + FOUR_FORWARD_SLASHES, + span, + "comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't", + "make this a doc comment by removing one `/`", + // It's a little unfortunate but the span includes the `\n` yet the contents + // do not, so we must add it back. If some codebase uses `\r\n` instead they + // will need normalization but it should be fine + contents.replacen("////", "///", 1) + "\n", + Applicability::MachineApplicable, + ); + } + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index cebe2e44c0f..607c718b562 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -137,6 +137,7 @@ mod format_impl; mod format_push_string; mod formatting; +mod four_forward_slashes; mod from_over_into; mod from_raw_with_void_ptr; mod from_str_radix_10; @@ -1081,6 +1082,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_early_pass(|| Box::new(visibility::Visibility)); store.register_late_pass(move |_| Box::new(tuple_array_conversions::TupleArrayConversions { msrv: msrv() })); store.register_late_pass(|_| Box::new(manual_float_methods::ManualFloatMethods)); + store.register_late_pass(|_| Box::new(four_forward_slashes::FourForwardSlashes)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/four_forward_slashes.fixed b/tests/ui/four_forward_slashes.fixed new file mode 100644 index 00000000000..4f9b1f4b7c8 --- /dev/null +++ b/tests/ui/four_forward_slashes.fixed @@ -0,0 +1,44 @@ +//// first line borked doc comment. doesn't combust! +//@run-rustfix +//@aux-build:proc_macros.rs:proc-macro +#![feature(custom_inner_attributes)] +#![allow(unused)] +#![warn(clippy::four_forward_slashes)] +#![no_main] +#![rustfmt::skip] + +#[macro_use] +extern crate proc_macros; + +/// whoops +fn a() {} + +/// whoops +#[allow(dead_code)] +fn b() {} + +/// whoops +/// two borked comments! +#[track_caller] +fn c() {} + +fn d() {} + +#[test] +/// between attributes +#[allow(dead_code)] +fn g() {} + +/// not very start of contents +fn h() {} + +external! { + //// don't lint me bozo + fn e() {} +} + +with_span! { + span + //// don't lint me bozo + fn f() {} +} diff --git a/tests/ui/four_forward_slashes.rs b/tests/ui/four_forward_slashes.rs new file mode 100644 index 00000000000..02f2084c17c --- /dev/null +++ b/tests/ui/four_forward_slashes.rs @@ -0,0 +1,44 @@ +//// first line borked doc comment. doesn't combust! +//@run-rustfix +//@aux-build:proc_macros.rs:proc-macro +#![feature(custom_inner_attributes)] +#![allow(unused)] +#![warn(clippy::four_forward_slashes)] +#![no_main] +#![rustfmt::skip] + +#[macro_use] +extern crate proc_macros; + +//// whoops +fn a() {} + +//// whoops +#[allow(dead_code)] +fn b() {} + +//// whoops +//// two borked comments! +#[track_caller] +fn c() {} + +fn d() {} + +#[test] +//// between attributes +#[allow(dead_code)] +fn g() {} + + //// not very start of contents +fn h() {} + +external! { + //// don't lint me bozo + fn e() {} +} + +with_span! { + span + //// don't lint me bozo + fn f() {} +} diff --git a/tests/ui/four_forward_slashes.stderr b/tests/ui/four_forward_slashes.stderr new file mode 100644 index 00000000000..1640b241bf3 --- /dev/null +++ b/tests/ui/four_forward_slashes.stderr @@ -0,0 +1,75 @@ +error: comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't + --> $DIR/four_forward_slashes.rs:13:1 + | +LL | / //// whoops +LL | | fn a() {} + | |_ + | + = note: `-D clippy::four-forward-slashes` implied by `-D warnings` +help: make this a doc comment by removing one `/` + | +LL + /// whoops + | + +error: comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't + --> $DIR/four_forward_slashes.rs:16:1 + | +LL | / //// whoops +LL | | #[allow(dead_code)] + | |_ + | +help: make this a doc comment by removing one `/` + | +LL + /// whoops + | + +error: comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't + --> $DIR/four_forward_slashes.rs:21:1 + | +LL | / //// two borked comments! +LL | | #[track_caller] + | |_ + | +help: make this a doc comment by removing one `/` + | +LL + /// two borked comments! + | + +error: comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't + --> $DIR/four_forward_slashes.rs:20:1 + | +LL | / //// whoops +LL | | //// two borked comments! + | |_ + | +help: make this a doc comment by removing one `/` + | +LL + /// whoops + | + +error: comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't + --> $DIR/four_forward_slashes.rs:28:1 + | +LL | / //// between attributes +LL | | #[allow(dead_code)] + | |_ + | +help: make this a doc comment by removing one `/` + | +LL + /// between attributes + | + +error: comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't + --> $DIR/four_forward_slashes.rs:32:1 + | +LL | / //// not very start of contents +LL | | fn h() {} + | |_ + | +help: make this a doc comment by removing one `/` + | +LL + /// not very start of contents + | + +error: aborting due to 6 previous errors + From 2e43d0c91717ed68cbfafc34d89dc95bca771c63 Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Wed, 12 Jul 2023 19:39:19 -0500 Subject: [PATCH 2/2] Improve suggestion and add more tests --- clippy_lints/src/four_forward_slashes.rs | 67 ++++++++++++------- tests/ui/four_forward_slashes.fixed | 6 +- tests/ui/four_forward_slashes.rs | 6 +- tests/ui/four_forward_slashes.stderr | 41 +++++------- .../ui/four_forward_slashes_first_line.fixed | 7 ++ tests/ui/four_forward_slashes_first_line.rs | 7 ++ .../ui/four_forward_slashes_first_line.stderr | 15 +++++ 7 files changed, 98 insertions(+), 51 deletions(-) create mode 100644 tests/ui/four_forward_slashes_first_line.fixed create mode 100644 tests/ui/four_forward_slashes_first_line.rs create mode 100644 tests/ui/four_forward_slashes_first_line.stderr diff --git a/clippy_lints/src/four_forward_slashes.rs b/clippy_lints/src/four_forward_slashes.rs index 7efa8c1b31f..419c7734344 100644 --- a/clippy_lints/src/four_forward_slashes.rs +++ b/clippy_lints/src/four_forward_slashes.rs @@ -1,9 +1,9 @@ -use clippy_utils::{diagnostics::span_lint_and_sugg, source::snippet_opt}; +use clippy_utils::diagnostics::span_lint_and_then; use rustc_errors::Applicability; use rustc_hir::Item; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::{Span, SyntaxContext}; +use rustc_span::Span; declare_clippy_lint! { /// ### What it does @@ -40,43 +40,60 @@ fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { if item.span.from_expansion() { return; } - let src = cx.sess().source_map(); - let item_and_attrs_span = cx + let sm = cx.sess().source_map(); + let mut span = cx .tcx .hir() .attrs(item.hir_id()) .iter() .fold(item.span.shrink_to_lo(), |span, attr| span.to(attr.span)); - let (Some(file), _, _, end_line, _) = src.span_to_location_info(item_and_attrs_span) else { + let (Some(file), _, _, end_line, _) = sm.span_to_location_info(span) else { return; }; + let mut bad_comments = vec![]; for line in (0..end_line.saturating_sub(1)).rev() { - let Some(contents) = file.get_line(line) else { - continue; + let Some(contents) = file.get_line(line).map(|c| c.trim().to_owned()) else { + return; }; - let contents = contents.trim(); - if contents.is_empty() { + // Keep searching until we find the next item + if !contents.is_empty() && !contents.starts_with("//") && !contents.starts_with("#[") { break; } - if contents.starts_with("////") { - let bounds = file.line_bounds(line); - let span = Span::new(bounds.start, bounds.end, SyntaxContext::root(), None); - if snippet_opt(cx, span).is_some_and(|s| s.trim().starts_with("////")) { - span_lint_and_sugg( - cx, - FOUR_FORWARD_SLASHES, - span, - "comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't", - "make this a doc comment by removing one `/`", - // It's a little unfortunate but the span includes the `\n` yet the contents - // do not, so we must add it back. If some codebase uses `\r\n` instead they - // will need normalization but it should be fine - contents.replacen("////", "///", 1) + "\n", + if contents.starts_with("////") && !matches!(contents.chars().nth(4), Some('/' | '!')) { + let bounds = file.line_bounds(line); + let line_span = Span::with_root_ctxt(bounds.start, bounds.end); + span = line_span.to(span); + bad_comments.push((line_span, contents)); + } + } + + if !bad_comments.is_empty() { + span_lint_and_then( + cx, + FOUR_FORWARD_SLASHES, + span, + "this item has comments with 4 forward slashes (`////`). These look like doc comments, but they aren't", + |diag| { + let msg = if bad_comments.len() == 1 { + "make this a doc comment by removing one `/`" + } else { + "turn these into doc comments by removing one `/`" + }; + + diag.multipart_suggestion( + msg, + bad_comments + .into_iter() + // It's a little unfortunate but the span includes the `\n` yet the contents + // do not, so we must add it back. If some codebase uses `\r\n` instead they + // will need normalization but it should be fine + .map(|(span, c)| (span, c.replacen("////", "///", 1) + "\n")) + .collect(), Applicability::MachineApplicable, ); - } - } + }, + ); } } } diff --git a/tests/ui/four_forward_slashes.fixed b/tests/ui/four_forward_slashes.fixed index 4f9b1f4b7c8..54b2c414b62 100644 --- a/tests/ui/four_forward_slashes.fixed +++ b/tests/ui/four_forward_slashes.fixed @@ -1,4 +1,3 @@ -//// first line borked doc comment. doesn't combust! //@run-rustfix //@aux-build:proc_macros.rs:proc-macro #![feature(custom_inner_attributes)] @@ -32,6 +31,11 @@ fn g() {} /// not very start of contents fn h() {} +fn i() { + //// don't lint me bozo + todo!() +} + external! { //// don't lint me bozo fn e() {} diff --git a/tests/ui/four_forward_slashes.rs b/tests/ui/four_forward_slashes.rs index 02f2084c17c..facdc8cb17d 100644 --- a/tests/ui/four_forward_slashes.rs +++ b/tests/ui/four_forward_slashes.rs @@ -1,4 +1,3 @@ -//// first line borked doc comment. doesn't combust! //@run-rustfix //@aux-build:proc_macros.rs:proc-macro #![feature(custom_inner_attributes)] @@ -32,6 +31,11 @@ fn g() {} //// not very start of contents fn h() {} +fn i() { + //// don't lint me bozo + todo!() +} + external! { //// don't lint me bozo fn e() {} diff --git a/tests/ui/four_forward_slashes.stderr b/tests/ui/four_forward_slashes.stderr index 1640b241bf3..89162e6b010 100644 --- a/tests/ui/four_forward_slashes.stderr +++ b/tests/ui/four_forward_slashes.stderr @@ -1,5 +1,5 @@ -error: comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't - --> $DIR/four_forward_slashes.rs:13:1 +error: this item has comments with 4 forward slashes (`////`). These look like doc comments, but they aren't + --> $DIR/four_forward_slashes.rs:12:1 | LL | / //// whoops LL | | fn a() {} @@ -11,11 +11,12 @@ help: make this a doc comment by removing one `/` LL + /// whoops | -error: comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't - --> $DIR/four_forward_slashes.rs:16:1 +error: this item has comments with 4 forward slashes (`////`). These look like doc comments, but they aren't + --> $DIR/four_forward_slashes.rs:15:1 | LL | / //// whoops LL | | #[allow(dead_code)] +LL | | fn b() {} | |_ | help: make this a doc comment by removing one `/` @@ -23,35 +24,27 @@ help: make this a doc comment by removing one `/` LL + /// whoops | -error: comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't - --> $DIR/four_forward_slashes.rs:21:1 - | -LL | / //// two borked comments! -LL | | #[track_caller] - | |_ - | -help: make this a doc comment by removing one `/` - | -LL + /// two borked comments! - | - -error: comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't - --> $DIR/four_forward_slashes.rs:20:1 +error: this item has comments with 4 forward slashes (`////`). These look like doc comments, but they aren't + --> $DIR/four_forward_slashes.rs:19:1 | LL | / //// whoops LL | | //// two borked comments! +LL | | #[track_caller] +LL | | fn c() {} | |_ | -help: make this a doc comment by removing one `/` +help: turn these into doc comments by removing one `/` | LL + /// whoops +LL ~ /// two borked comments! | -error: comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't - --> $DIR/four_forward_slashes.rs:28:1 +error: this item has comments with 4 forward slashes (`////`). These look like doc comments, but they aren't + --> $DIR/four_forward_slashes.rs:27:1 | LL | / //// between attributes LL | | #[allow(dead_code)] +LL | | fn g() {} | |_ | help: make this a doc comment by removing one `/` @@ -59,8 +52,8 @@ help: make this a doc comment by removing one `/` LL + /// between attributes | -error: comment with 4 forward slashes (`////`). This looks like a doc comment, but it isn't - --> $DIR/four_forward_slashes.rs:32:1 +error: this item has comments with 4 forward slashes (`////`). These look like doc comments, but they aren't + --> $DIR/four_forward_slashes.rs:31:1 | LL | / //// not very start of contents LL | | fn h() {} @@ -71,5 +64,5 @@ help: make this a doc comment by removing one `/` LL + /// not very start of contents | -error: aborting due to 6 previous errors +error: aborting due to 5 previous errors diff --git a/tests/ui/four_forward_slashes_first_line.fixed b/tests/ui/four_forward_slashes_first_line.fixed new file mode 100644 index 00000000000..ce272b4c6cd --- /dev/null +++ b/tests/ui/four_forward_slashes_first_line.fixed @@ -0,0 +1,7 @@ +/// borked doc comment on the first line. doesn't combust! +fn a() {} + +//@run-rustfix +// This test's entire purpose is to make sure we don't panic if the comment with four slashes +// extends to the first line of the file. This is likely pretty rare in production, but an ICE is an +// ICE. diff --git a/tests/ui/four_forward_slashes_first_line.rs b/tests/ui/four_forward_slashes_first_line.rs new file mode 100644 index 00000000000..d8f82d4410b --- /dev/null +++ b/tests/ui/four_forward_slashes_first_line.rs @@ -0,0 +1,7 @@ +//// borked doc comment on the first line. doesn't combust! +fn a() {} + +//@run-rustfix +// This test's entire purpose is to make sure we don't panic if the comment with four slashes +// extends to the first line of the file. This is likely pretty rare in production, but an ICE is an +// ICE. diff --git a/tests/ui/four_forward_slashes_first_line.stderr b/tests/ui/four_forward_slashes_first_line.stderr new file mode 100644 index 00000000000..7944da14feb --- /dev/null +++ b/tests/ui/four_forward_slashes_first_line.stderr @@ -0,0 +1,15 @@ +error: this item has comments with 4 forward slashes (`////`). These look like doc comments, but they aren't + --> $DIR/four_forward_slashes_first_line.rs:1:1 + | +LL | / //// borked doc comment on the first line. doesn't combust! +LL | | fn a() {} + | |_ + | + = note: `-D clippy::four-forward-slashes` implied by `-D warnings` +help: make this a doc comment by removing one `/` + | +LL + /// borked doc comment on the first line. doesn't combust! + | + +error: aborting due to previous error +