Auto merge of #9851 - Veykril:unnecessary-safety-comment, r=giraffate

Lint unnecessary safety comments changelog: [`unnecessary_safety_comment`]: Add unnecessary safety comment lint Addresses #7954 This does not necessarily catch all occurences, as doing so would require checking all expressions in the entire source which seems rather expensive. Instead what the lint does is it checks items, statements and the tail expression of blocks for safety comments, then checks if those comments are necessary or not, then linting for the unnecessary ones. I kept the tests in one file to check that the lints do not clash with each other.
rust-lang · Nov 25, 2022 · efadb55 · efadb55
2 parents 1a96571 + f96dd38
commit efadb55
Show file tree

Hide file tree

Showing 8 changed files with 539 additions and 105 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4451,6 +4451,7 @@ Released 2018-09-13
 [`unnecessary_mut_passed`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_mut_passed
 [`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation
 [`unnecessary_owned_empty_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_owned_empty_strings
+[`unnecessary_safety_comment`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_comment
 [`unnecessary_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_doc
 [`unnecessary_self_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_self_imports
 [`unnecessary_sort_by`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_sort_by

diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -584,6 +584,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::types::TYPE_COMPLEXITY_INFO,
     crate::types::VEC_BOX_INFO,
     crate::undocumented_unsafe_blocks::UNDOCUMENTED_UNSAFE_BLOCKS_INFO,
+    crate::undocumented_unsafe_blocks::UNNECESSARY_SAFETY_COMMENT_INFO,
     crate::unicode::INVISIBLE_CHARACTERS_INFO,
     crate::unicode::NON_ASCII_LITERAL_INFO,
     crate::unicode::UNICODE_NOT_NFC_INFO,

diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs
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
@@ -1,6 +1,6 @@
 // aux-build:proc_macro_unsafe.rs
 
-#![warn(clippy::undocumented_unsafe_blocks)]
+#![warn(clippy::undocumented_unsafe_blocks, clippy::unnecessary_safety_comment)]
 #![allow(clippy::let_unit_value, clippy::missing_safety_doc)]
 
 extern crate proc_macro_unsafe;

diff --git a/tests/ui/undocumented_unsafe_blocks.stderr b/tests/ui/undocumented_unsafe_blocks.stderr
@@ -239,6 +239,19 @@ LL |     unsafe impl TrailingComment for () {} // SAFETY:
    |
    = help: consider adding a safety comment on the preceding line
 
+error: constant item has unnecessary safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:471:5
+   |
+LL |     const BIG_NUMBER: i32 = 1000000;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider removing the safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:470:5
+   |
+LL |     // SAFETY:
+   |     ^^^^^^^^^^
+   = note: `-D clippy::unnecessary-safety-comment` implied by `-D warnings`
+
 error: unsafe impl missing a safety comment
   --> $DIR/undocumented_unsafe_blocks.rs:472:5
    |
@@ -271,6 +284,24 @@ LL |         unsafe {};
    |
    = help: consider adding a safety comment on the preceding line
 
+error: statement has unnecessary safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:501:5
+   |
+LL | /     let _ = {
+LL | |         if unsafe { true } {
+LL | |             todo!();
+LL | |         } else {
+...  |
+LL | |         }
+LL | |     };
+   | |______^
+   |
+help: consider removing the safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:500: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:502:12
    |
@@ -287,5 +318,5 @@ LL |             let bar = unsafe {};
    |
    = help: consider adding a safety comment on the preceding line
 
-error: aborting due to 34 previous errors
+error: aborting due to 36 previous errors
 
diff --git a/tests/ui/unnecessary_safety_comment.rs b/tests/ui/unnecessary_safety_comment.rs
@@ -0,0 +1,51 @@
+#![warn(clippy::undocumented_unsafe_blocks, clippy::unnecessary_safety_comment)]
+#![allow(clippy::let_unit_value, clippy::missing_safety_doc)]
+
+mod unsafe_items_invalid_comment {
+    // SAFETY:
+    const CONST: u32 = 0;
+    // SAFETY:
+    static STATIC: u32 = 0;
+    // SAFETY:
+    struct Struct;
+    // SAFETY:
+    enum Enum {}
+    // SAFETY:
+    mod module {}
+}
+
+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
+    if num > 24 {}
+
+    // SAFETY: unnecessary
+    24
+}
+
+fn main() {}
diff --git a/tests/ui/unnecessary_safety_comment.stderr b/tests/ui/unnecessary_safety_comment.stderr
@@ -0,0 +1,115 @@
+error: constant item has unnecessary safety comment
+  --> $DIR/unnecessary_safety_comment.rs:6:5
+   |
+LL |     const CONST: u32 = 0;
+   |     ^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider removing the safety comment
+  --> $DIR/unnecessary_safety_comment.rs:5:5
+   |
+LL |     // SAFETY:
+   |     ^^^^^^^^^^
+   = note: `-D clippy::unnecessary-safety-comment` implied by `-D warnings`
+
+error: static item has unnecessary safety comment
+  --> $DIR/unnecessary_safety_comment.rs:8:5
+   |
+LL |     static STATIC: u32 = 0;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider removing the safety comment
+  --> $DIR/unnecessary_safety_comment.rs:7:5
+   |
+LL |     // SAFETY:
+   |     ^^^^^^^^^^
+
+error: struct has unnecessary safety comment
+  --> $DIR/unnecessary_safety_comment.rs:10:5
+   |
+LL |     struct Struct;
+   |     ^^^^^^^^^^^^^^
+   |
+help: consider removing the safety comment
+  --> $DIR/unnecessary_safety_comment.rs:9:5
+   |
+LL |     // SAFETY:
+   |     ^^^^^^^^^^
+
+error: enum has unnecessary safety comment
+  --> $DIR/unnecessary_safety_comment.rs:12:5
+   |
+LL |     enum Enum {}
+   |     ^^^^^^^^^^^^
+   |
+help: consider removing the safety comment
+  --> $DIR/unnecessary_safety_comment.rs:11:5
+   |
+LL |     // SAFETY:
+   |     ^^^^^^^^^^
+
+error: module has unnecessary safety comment
+  --> $DIR/unnecessary_safety_comment.rs:14:5
+   |
+LL |     mod module {}
+   |     ^^^^^^^^^^^^^
+   |
+help: consider removing the safety comment
+  --> $DIR/unnecessary_safety_comment.rs:13:5
+   |
+LL |     // SAFETY:
+   |     ^^^^^^^^^^
+
+error: impl has unnecessary safety comment
+  --> $DIR/unnecessary_safety_comment.rs:33:13
+   |
+LL |             impl T for $t {}
+   |             ^^^^^^^^^^^^^^^^
+...
+LL |     with_safety_comment!(i32);
+   |     ------------------------- in this macro invocation
+   |
+help: consider removing the safety comment
+  --> $DIR/unnecessary_safety_comment.rs:32: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/unnecessary_safety_comment.rs:48:5
+   |
+LL |     24
+   |     ^^
+   |
+help: consider removing the safety comment
+  --> $DIR/unnecessary_safety_comment.rs:47:5
+   |
+LL |     // SAFETY: unnecessary
+   |     ^^^^^^^^^^^^^^^^^^^^^^
+
+error: statement has unnecessary safety comment
+  --> $DIR/unnecessary_safety_comment.rs:42:5
+   |
+LL |     let num = 42;
+   |     ^^^^^^^^^^^^^
+   |
+help: consider removing the safety comment
+  --> $DIR/unnecessary_safety_comment.rs:41:5
+   |
+LL |     // SAFETY: unnecessary
+   |     ^^^^^^^^^^^^^^^^^^^^^^
+
+error: statement has unnecessary safety comment
+  --> $DIR/unnecessary_safety_comment.rs:45:5
+   |
+LL |     if num > 24 {}
+   |     ^^^^^^^^^^^^^^
+   |
+help: consider removing the safety comment
+  --> $DIR/unnecessary_safety_comment.rs:44:5
+   |
+LL |     // SAFETY: unnecessary
+   |     ^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 9 previous errors
+