Lint unnecessary safety comments on statements and block tail expressions

Veykril · Veykril · commit 4fa57575302a · 2022-11-25T10:09:34.000+01:00
diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs
@@ -1,6 +1,10 @@
+use std::ops::ControlFlow;
+
 use clippy_utils::diagnostics::span_lint_and_help;
 use clippy_utils::source::walk_span_to_context;
+use clippy_utils::visitors::{for_each_expr_with_closures, Descend};
 use clippy_utils::{get_parent_node, is_lint_allowed};
+use hir::HirId;
 use rustc_data_structures::sync::Lrc;
 use rustc_hir as hir;
 use rustc_hir::{Block, BlockCheckMode, ItemKind, Node, UnsafeSource};
@@ -90,8 +94,8 @@ declare_clippy_lint! {
 
 declare_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS, UNNECESSARY_SAFETY_COMMENT]);
 
-impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
-    fn check_block(&mut self, cx: &LateContext<'_>, block: &'_ Block<'_>) {
+impl<'tcx> LateLintPass<'tcx> for UndocumentedUnsafeBlocks {
+    fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) {
         if block.rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided)
             && !in_external_macro(cx.tcx.sess, block.span)
             && !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, block.hir_id)
@@ -115,6 +119,45 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
                 "consider adding a safety comment on the preceding line",
             );
         }
+
+        if let Some(tail) = block.expr
+            && !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, tail.hir_id)
+            && !in_external_macro(cx.tcx.sess, tail.span)
+            && let HasSafetyComment::Yes(pos) = stmt_has_safety_comment(cx, tail.span, tail.hir_id)
+            && let Some(help_span) = expr_has_unnecessary_safety_comment(cx, tail, pos)
+        {
+            span_lint_and_help(
+                cx,
+                UNNECESSARY_SAFETY_COMMENT,
+                tail.span,
+                "expression has unnecessary safety comment",
+                Some(help_span),
+                "consider removing the safety comment",
+            );
+        }
+    }
+
+    fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &hir::Stmt<'tcx>) {
+        let expr = match stmt.kind {
+            hir::StmtKind::Local(&hir::Local { init: Some(expr), .. })
+            | hir::StmtKind::Expr(expr)
+            | hir::StmtKind::Semi(expr) => expr,
+            _ => return,
+        };
+        if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, stmt.hir_id)
+            && !in_external_macro(cx.tcx.sess, stmt.span)
+            && let HasSafetyComment::Yes(pos) = stmt_has_safety_comment(cx, stmt.span, stmt.hir_id)
+            && let Some(help_span) = expr_has_unnecessary_safety_comment(cx, expr, pos)
+        {
+            span_lint_and_help(
+                cx,
+                UNNECESSARY_SAFETY_COMMENT,
+                stmt.span,
+                "statement has unnecessary safety comment",
+                Some(help_span),
+                "consider removing the safety comment",
+            );
+        }
     }
 
     fn check_item(&mut self, cx: &LateContext<'_>, item: &hir::Item<'_>) {
@@ -216,6 +259,36 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
     }
 }
 
+fn expr_has_unnecessary_safety_comment<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx hir::Expr<'tcx>,
+    comment_pos: BytePos,
+) -> Option<Span> {
+    // this should roughly be the reverse of `block_parents_have_safety_comment`
+    if for_each_expr_with_closures(cx, expr, |expr| match expr.kind {
+        hir::ExprKind::Block(
+            Block {
+                rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided),
+                ..
+            },
+            _,
+        ) => ControlFlow::Break(()),
+        // statements will be handled by check_stmt itself again
+        hir::ExprKind::Block(..) => ControlFlow::Continue(Descend::No),
+        _ => ControlFlow::Continue(Descend::Yes),
+    })
+    .is_some()
+    {
+        return None;
+    }
+
+    let source_map = cx.tcx.sess.source_map();
+    let span = Span::new(comment_pos, comment_pos, SyntaxContext::root(), None);
+    let help_span = source_map.span_extend_to_next_char(span, '\n', true);
+
+    Some(help_span)
+}
+
 fn is_unsafe_from_proc_macro(cx: &LateContext<'_>, span: Span) -> bool {
     let source_map = cx.sess().source_map();
     let file_pos = source_map.lookup_byte_offset(span.lo());
@@ -358,6 +431,55 @@ fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> HasSaf
     }
 }
 
+/// Checks if the lines immediately preceding the item contain a safety comment.
+#[allow(clippy::collapsible_match)]
+fn stmt_has_safety_comment(cx: &LateContext<'_>, span: Span, hir_id: HirId) -> HasSafetyComment {
+    match span_from_macro_expansion_has_safety_comment(cx, span) {
+        HasSafetyComment::Maybe => (),
+        has_safety_comment => return has_safety_comment,
+    }
+
+    if span.ctxt() == SyntaxContext::root() {
+        if let Some(parent_node) = get_parent_node(cx.tcx, hir_id) {
+            let comment_start = match parent_node {
+                Node::Block(block) => walk_span_to_context(block.span, SyntaxContext::root()).map(Span::lo),
+                _ => return HasSafetyComment::Maybe,
+            };
+
+            let source_map = cx.sess().source_map();
+            if let Some(comment_start) = comment_start
+                && let Ok(unsafe_line) = source_map.lookup_line(span.lo())
+                && let Ok(comment_start_line) = source_map.lookup_line(comment_start)
+                && Lrc::ptr_eq(&unsafe_line.sf, &comment_start_line.sf)
+                && let Some(src) = unsafe_line.sf.src.as_deref()
+            {
+                unsafe_line.sf.lines(|lines| {
+                    if comment_start_line.line >= unsafe_line.line {
+                        HasSafetyComment::No
+                    } else {
+                        match text_has_safety_comment(
+                            src,
+                            &lines[comment_start_line.line + 1..=unsafe_line.line],
+                            unsafe_line.sf.start_pos.to_usize(),
+                        ) {
+                            Some(b) => HasSafetyComment::Yes(b),
+                            None => HasSafetyComment::No,
+                        }
+                    }
+                })
+            } else {
+                // Problem getting source text. Pretend a comment was found.
+                HasSafetyComment::Maybe
+            }
+        } else {
+            // No parent node. Pretend a comment was found.
+            HasSafetyComment::Maybe
+        }
+    } else {
+        HasSafetyComment::No
+    }
+}
+
 fn comment_start_before_item_in_mod(
     cx: &LateContext<'_>,
     parent_mod: &hir::Mod<'_>,
diff --git a/clippy_utils/src/visitors.rs b/clippy_utils/src/visitors.rs
@@ -170,36 +170,36 @@ where
         cb: F,
     }
 
-    struct WithStmtGuarg<'a, F> {
+    struct WithStmtGuard<'a, F> {
         val: &'a mut RetFinder<F>,
         prev_in_stmt: bool,
     }
 
     impl<F> RetFinder<F> {
-        fn inside_stmt(&mut self, in_stmt: bool) -> WithStmtGuarg<'_, F> {
+        fn inside_stmt(&mut self, in_stmt: bool) -> WithStmtGuard<'_, F> {
             let prev_in_stmt = std::mem::replace(&mut self.in_stmt, in_stmt);
-            WithStmtGuarg {
+            WithStmtGuard {
                 val: self,
                 prev_in_stmt,
             }
         }
     }
 
-    impl<F> std::ops::Deref for WithStmtGuarg<'_, F> {
+    impl<F> std::ops::Deref for WithStmtGuard<'_, F> {
         type Target = RetFinder<F>;
 
         fn deref(&self) -> &Self::Target {
             self.val
         }
     }
 
-    impl<F> std::ops::DerefMut for WithStmtGuarg<'_, F> {
+    impl<F> std::ops::DerefMut for WithStmtGuard<'_, F> {
         fn deref_mut(&mut self) -> &mut Self::Target {
             self.val
         }
     }
 
-    impl<F> Drop for WithStmtGuarg<'_, F> {
+    impl<F> Drop for WithStmtGuard<'_, F> {
         fn drop(&mut self) {
             self.val.in_stmt = self.prev_in_stmt;
         }
diff --git a/tests/ui/undocumented_unsafe_blocks.rs b/tests/ui/undocumented_unsafe_blocks.rs
@@ -522,4 +522,35 @@ fn issue_9142() {
     };
 }
 
+mod unnecessary_from_macro {
+    trait T {}
+
+    macro_rules! no_safety_comment {
+        ($t:ty) => {
+            impl T for $t {}
+        };
+    }
+
+    // FIXME: This is not caught
+    // Safety: unnecessary
+    no_safety_comment!(());
+
+    macro_rules! with_safety_comment {
+        ($t:ty) => {
+            // Safety: unnecessary
+            impl T for $t {}
+        };
+    }
+
+    with_safety_comment!(i32);
+}
+
+fn unnecessary_on_stmt_and_expr() -> u32 {
+    // SAFETY: unnecessary
+    let num = 42;
+
+    // SAFETY: unnecessary
+    24
+}
+
 fn main() {}
diff --git a/tests/ui/undocumented_unsafe_blocks.stderr b/tests/ui/undocumented_unsafe_blocks.stderr
@@ -344,6 +344,24 @@ LL |         unsafe {};
    |
    = help: consider adding a safety comment on the preceding line
 
+error: statement has unnecessary safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:514:5
+   |
+LL | /     let _ = {
+LL | |         if unsafe { true } {
+LL | |             todo!();
+LL | |         } else {
+...  |
+LL | |         }
+LL | |     };
+   | |______^
+   |
+help: consider removing the safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:513:5
+   |
+LL |     // SAFETY: this is more than one level away, so it should warn
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
 error: unsafe block missing a safety comment
   --> $DIR/undocumented_unsafe_blocks.rs:515:12
    |
@@ -360,5 +378,45 @@ LL |             let bar = unsafe {};
    |
    = help: consider adding a safety comment on the preceding line
 
-error: aborting due to 40 previous errors
+error: impl has unnecessary safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:541:13
+   |
+LL |             impl T for $t {}
+   |             ^^^^^^^^^^^^^^^^
+...
+LL |     with_safety_comment!(i32);
+   |     ------------------------- in this macro invocation
+   |
+help: consider removing the safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:540:13
+   |
+LL |             // Safety: unnecessary
+   |             ^^^^^^^^^^^^^^^^^^^^^^
+   = note: this error originates in the macro `with_safety_comment` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: expression has unnecessary safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:553:5
+   |
+LL |     24
+   |     ^^
+   |
+help: consider removing the safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:552:5
+   |
+LL |     // SAFETY: unnecessary
+   |     ^^^^^^^^^^^^^^^^^^^^^^
+
+error: statement has unnecessary safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:550:5
+   |
+LL |     let num = 42;
+   |     ^^^^^^^^^^^^^
+   |
+help: consider removing the safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:549:5
+   |
+LL |     // SAFETY: unnecessary
+   |     ^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 44 previous errors
 

Original file line number	Diff line number	Diff line change
`@@ -170,36 +170,36 @@ where`
`170`	`170`	`cb: F,`
`171`	`171`	`}`
`172`	`172`
`173`		`- struct WithStmtGuarg<'a, F> {`
	`173`	`+ struct WithStmtGuard<'a, F> {`
`174`	`174`	`val: &'a mut RetFinder<F>,`
`175`	`175`	`prev_in_stmt: bool,`
`176`	`176`	`}`
`177`	`177`
`178`	`178`	`impl<F> RetFinder<F> {`
`179`		`- fn inside_stmt(&mut self, in_stmt: bool) -> WithStmtGuarg<'_, F> {`
	`179`	`+ fn inside_stmt(&mut self, in_stmt: bool) -> WithStmtGuard<'_, F> {`
`180`	`180`	`let prev_in_stmt = std::mem::replace(&mut self.in_stmt, in_stmt);`
`181`		`- WithStmtGuarg {`
	`181`	`+ WithStmtGuard {`
`182`	`182`	`val: self,`
`183`	`183`	`prev_in_stmt,`
`184`	`184`	`}`
`185`	`185`	`}`
`186`	`186`	`}`
`187`	`187`
`188`		`- impl<F> std::ops::Deref for WithStmtGuarg<'_, F> {`
	`188`	`+ impl<F> std::ops::Deref for WithStmtGuard<'_, F> {`
`189`	`189`	`type Target = RetFinder<F>;`
`190`	`190`
`191`	`191`	`fn deref(&self) -> &Self::Target {`
`192`	`192`	`self.val`
`193`	`193`	`}`
`194`	`194`	`}`
`195`	`195`
`196`		`- impl<F> std::ops::DerefMut for WithStmtGuarg<'_, F> {`
	`196`	`+ impl<F> std::ops::DerefMut for WithStmtGuard<'_, F> {`
`197`	`197`	`fn deref_mut(&mut self) -> &mut Self::Target {`
`198`	`198`	`self.val`
`199`	`199`	`}`
`200`	`200`	`}`
`201`	`201`
`202`		`- impl<F> Drop for WithStmtGuarg<'_, F> {`
	`202`	`+ impl<F> Drop for WithStmtGuard<'_, F> {`
`203`	`203`	`fn drop(&mut self) {`
`204`	`204`	`self.val.in_stmt = self.prev_in_stmt;`
`205`	`205`	`}`