Improve suggestion and add more tests

rust-lang · Jul 13, 2023 · 1bca699 · 1bca699
1 parent 885ce61
commit 1bca699
Show file tree

Hide file tree

Showing 7 changed files with 96 additions and 49 deletions.
diff --git 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 @@ impl<'tcx> LateLintPass<'tcx> for FourForwardSlashes {
         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);
+                let line_span = Span::with_root_ctxt(bounds.start, bounds.end);
+                span = line_span.to(span);
+                bad_comments.push((line_span, contents));
+            }
+        }
 
-                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 !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
@@ -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
@@ -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
@@ -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,56 +11,49 @@ 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 `/`
    |
 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 `/`
    |
 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
@@ -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
@@ -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
@@ -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
+