fix [undocumented_unsafe_blocks] FP with trait/impl items

rust-lang · Apr 12, 2024 · fc4d88b · fc4d88b
1 parent 62fd1d5
commit fc4d88b
Show file tree

Hide file tree

Showing 4 changed files with 172 additions and 26 deletions.
diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs
@@ -343,12 +343,13 @@ fn block_parents_have_safety_comment(
     let (span, hir_id) = match cx.tcx.parent_hir_node(id) {
         Node::Expr(expr) => match cx.tcx.parent_hir_node(expr.hir_id) {
             Node::LetStmt(hir::LetStmt { span, hir_id, .. }) => (*span, *hir_id),
-            Node::Item(hir::Item {
-                kind: ItemKind::Const(..) | ItemKind::Static(..),
-                span,
-                owner_id,
-                ..
-            }) => (*span, cx.tcx.local_def_id_to_hir_id(owner_id.def_id)),
+
+            node if let Some(item) = ItemAlike::hir(&node)
+                && item.is_const_or_static() =>
+            {
+                (item.span, cx.tcx.local_def_id_to_hir_id(item.owner_id.def_id))
+            },
+
             _ => {
                 if is_branchy(expr) {
                     return false;
@@ -364,12 +365,13 @@ fn block_parents_have_safety_comment(
             ..
         })
         | Node::LetStmt(hir::LetStmt { span, hir_id, .. }) => (*span, *hir_id),
-        Node::Item(hir::Item {
-            kind: ItemKind::Const(..) | ItemKind::Static(..),
-            span,
-            owner_id,
-            ..
-        }) => (*span, cx.tcx.local_def_id_to_hir_id(owner_id.def_id)),
+
+        node if let Some(item) = ItemAlike::hir(&node)
+            && item.is_const_or_static() =>
+        {
+            (item.span, cx.tcx.local_def_id_to_hir_id(item.owner_id.def_id))
+        },
+
         _ => return false,
     };
     // if unsafe block is part of a let/const/static statement,
@@ -604,21 +606,19 @@ fn get_body_search_span(cx: &LateContext<'_>) -> Option<Span> {
         match node {
             Node::Expr(e) => span = e.span,
             Node::Block(_) | Node::Arm(_) | Node::Stmt(_) | Node::LetStmt(_) => (),
-            Node::Item(hir::Item {
-                kind: ItemKind::Const(..) | ItemKind::Static(..),
-                ..
-            }) => maybe_global_var = true,
-            Node::Item(hir::Item {
-                kind: ItemKind::Mod(_),
-                span: item_span,
-                ..
-            }) => {
-                span = *item_span;
-                break;
-            },
             Node::Crate(mod_) if maybe_global_var => {
                 span = mod_.spans.inner_span;
             },
+
+            node if let Some(item) = ItemAlike::hir(&node) => {
+                if item.is_const_or_static() {
+                    maybe_global_var = true;
+                } else if let ItemAlikeKind::Item(ItemKind::Mod(_)) = item.kind {
+                    span = item.span;
+                    break;
+                }
+            },
+
             _ => break,
         }
     }
@@ -711,3 +711,53 @@ fn text_has_safety_comment(src: &str, line_starts: &[RelativeBytePos], start_pos
         }
     }
 }
+
+macro_rules! item_like_kind_conversion {
+    ($varient:ident, $ty_convert_from:ty, $lft:lifetime) => {
+        impl<$lft> ::core::convert::From<$ty_convert_from> for ItemAlikeKind<$lft> {
+            fn from(value: $ty_convert_from) -> Self {
+                Self::$varient(value)
+            }
+        }
+    };
+}
+
+enum ItemAlikeKind<'hir> {
+    Item(ItemKind<'hir>),
+    TraitItem(hir::TraitItemKind<'hir>),
+    ImplItem(hir::ImplItemKind<'hir>),
+}
+
+item_like_kind_conversion!(Item, ItemKind<'hir>, 'hir);
+item_like_kind_conversion!(TraitItem, hir::TraitItemKind<'hir>, 'hir);
+item_like_kind_conversion!(ImplItem, hir::ImplItemKind<'hir>, 'hir);
+
+/// Representing the hir nodes that are item alike.
+///
+/// Note this does not includes [`Node::ForeignItem`] for now.
+struct ItemAlike<'hir> {
+    owner_id: hir::OwnerId,
+    kind: ItemAlikeKind<'hir>,
+    span: Span,
+}
+
+impl<'hir> ItemAlike<'hir> {
+    fn hir(node: &'hir Node<'hir>) -> Option<Self> {
+        let (owner_id, kind, span) = match node {
+            Node::Item(item) => (item.owner_id, item.kind.into(), item.span),
+            Node::TraitItem(ti) => (ti.owner_id, ti.kind.into(), ti.span),
+            Node::ImplItem(ii) => (ii.owner_id, ii.kind.into(), ii.span),
+            _ => return None,
+        };
+        Some(Self { owner_id, kind, span })
+    }
+
+    fn is_const_or_static(&self) -> bool {
+        matches!(
+            self.kind,
+            ItemAlikeKind::Item(ItemKind::Const(..) | ItemKind::Static(..))
+                | ItemAlikeKind::ImplItem(hir::ImplItemKind::Const(..))
+                | ItemAlikeKind::TraitItem(hir::TraitItemKind::Const(..))
+        )
+    }
+}
diff --git a/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.default.stderr b/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.default.stderr
@@ -312,5 +312,37 @@ LL |             let bar = unsafe {};
    |
    = help: consider adding a safety comment on the preceding line
 
-error: aborting due to 35 previous errors
+error: unsafe block missing a safety comment
+  --> tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs:594:52
+   |
+LL |         const NO_SAFETY_IN_TRAIT_BUT_IN_IMPL: u8 = unsafe { 0 }; // lint?
+   |                                                    ^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs:602:41
+   |
+LL |         const NO_SAFETY_IN_TRAIT: i32 = unsafe { 1 };
+   |                                         ^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs:613:42
+   |
+LL |         const HAS_SAFETY_IN_TRAIT: i32 = unsafe { 3 }; // lint?
+   |                                          ^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs:617:40
+   |
+LL |         const NO_SAFETY_IN_IMPL: i32 = unsafe { 1 };
+   |                                        ^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: aborting due to 39 previous errors
 
diff --git a/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.disabled.stderr b/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.disabled.stderr
@@ -392,5 +392,37 @@ LL |     unsafe {}
    |
    = help: consider adding a safety comment on the preceding line
 
-error: aborting due to 45 previous errors
+error: unsafe block missing a safety comment
+  --> tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs:594:52
+   |
+LL |         const NO_SAFETY_IN_TRAIT_BUT_IN_IMPL: u8 = unsafe { 0 }; // lint?
+   |                                                    ^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs:602:41
+   |
+LL |         const NO_SAFETY_IN_TRAIT: i32 = unsafe { 1 };
+   |                                         ^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs:613:42
+   |
+LL |         const HAS_SAFETY_IN_TRAIT: i32 = unsafe { 3 }; // lint?
+   |                                          ^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe block missing a safety comment
+  --> tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs:617:40
+   |
+LL |         const NO_SAFETY_IN_IMPL: i32 = unsafe { 1 };
+   |                                        ^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: aborting due to 49 previous errors
 
diff --git a/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs b/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs
@@ -587,4 +587,36 @@ mod issue_11246 {
 // Safety: Another safety comment
 const FOO: () = unsafe {};
 
+// trait items and impl items
+mod issue_11709 {
+    trait MyTrait {
+        //~v ERROR: unsafe block missing a safety comment
+        const NO_SAFETY_IN_TRAIT_BUT_IN_IMPL: u8 = unsafe { 0 }; // lint?
+
+        // SAFETY: safe
+        const HAS_SAFETY_IN_TRAIT: i32 = unsafe { 1 };
+
+        // SAFETY: unrelated
+        unsafe fn unsafe_fn() {}
+
+        const NO_SAFETY_IN_TRAIT: i32 = unsafe { 1 };
+        //~^ ERROR: unsafe block missing a safety comment
+    }
+
+    struct UnsafeStruct;
+
+    impl MyTrait for UnsafeStruct {
+        // SAFETY: safe in this impl
+        const NO_SAFETY_IN_TRAIT_BUT_IN_IMPL: u8 = unsafe { 2 };
+
+        //~v ERROR: unsafe block missing a safety comment
+        const HAS_SAFETY_IN_TRAIT: i32 = unsafe { 3 }; // lint?
+    }
+
+    impl UnsafeStruct {
+        const NO_SAFETY_IN_IMPL: i32 = unsafe { 1 };
+        //~^ ERROR: unsafe block missing a safety comment
+    }
+}
+
 fn main() {}