Auto merge of #10986 - Centri3:undocumented_unsafe_blocks, r=Manishearth

Allow safety comment above attributes

Closes #8679

changelog: Enhancement: [`undocumented_safety_block`]: Added `accept-comment-above-attributes` configuration.

CHANGELOG.md

@@ -5385,4 +5385,5 @@ Released 2018-09-13
 [`allowed-idents-below-min-chars`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allowed-idents-below-min-chars
 [`min-ident-chars-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#min-ident-chars-threshold
 [`accept-comment-above-statement`]: https://doc.rust-lang.org/clippy/lint_configuration.html#accept-comment-above-statement
+[`accept-comment-above-attributes`]: https://doc.rust-lang.org/clippy/lint_configuration.html#accept-comment-above-attributes
 <!-- end autogenerated links to configuration documentation -->

book/src/lint_configuration.md

@@ -706,3 +706,13 @@ Whether to accept a safety comment to be placed above the statement containing t
 * [`undocumented_unsafe_blocks`](https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks)
 
 
+## `accept-comment-above-attributes`
+Whether to accept a safety comment to be placed above the attributes for the `unsafe` block
+
+**Default Value:** `false` (`bool`)
+
+---
+**Affected lints:**
+* [`undocumented_unsafe_blocks`](https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks)
+
+

clippy_lints/src/lib.rs

@@ -930,9 +930,11 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         ))
     });
     let accept_comment_above_statement = conf.accept_comment_above_statement;
+    let accept_comment_above_attributes = conf.accept_comment_above_attributes;
     store.register_late_pass(move |_| {
         Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks::new(
             accept_comment_above_statement,
+            accept_comment_above_attributes,
         ))
     });
     let allow_mixed_uninlined = conf.allow_mixed_uninlined_format_args;

clippy_lints/src/undocumented_unsafe_blocks.rs

@@ -95,12 +95,14 @@ declare_clippy_lint! {
 #[derive(Copy, Clone)]
 pub struct UndocumentedUnsafeBlocks {
     accept_comment_above_statement: bool,
+    accept_comment_above_attributes: bool,
 }
 
 impl UndocumentedUnsafeBlocks {
-    pub fn new(accept_comment_above_statement: bool) -> Self {
+    pub fn new(accept_comment_above_statement: bool, accept_comment_above_attributes: bool) -> Self {
         Self {
             accept_comment_above_statement,
+            accept_comment_above_attributes,
         }
     }
 }
@@ -114,7 +116,12 @@ impl<'tcx> LateLintPass<'tcx> for UndocumentedUnsafeBlocks {
             && !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, block.hir_id)
             && !is_unsafe_from_proc_macro(cx, block.span)
             && !block_has_safety_comment(cx, block.span)
-            && !block_parents_have_safety_comment(self.accept_comment_above_statement, cx, block.hir_id)
+            && !block_parents_have_safety_comment(
+                self.accept_comment_above_statement,
+                self.accept_comment_above_attributes,
+                cx,
+                block.hir_id,
+            )
         {
             let source_map = cx.tcx.sess.source_map();
             let span = if source_map.is_multiline(block.span) {
@@ -328,6 +335,7 @@ fn is_unsafe_from_proc_macro(cx: &LateContext<'_>, span: Span) -> bool {
 // has a safety comment
 fn block_parents_have_safety_comment(
     accept_comment_above_statement: bool,
+    accept_comment_above_attributes: bool,
     cx: &LateContext<'_>,
     id: hir::HirId,
 ) -> bool {
@@ -343,33 +351,77 @@ fn block_parents_have_safety_comment(
                     }),
                 ) = get_parent_node(cx.tcx, expr.hir_id)
                 {
+                    let hir_id = match get_parent_node(cx.tcx, expr.hir_id) {
+                        Some(Node::Local(hir::Local { hir_id, .. })) => *hir_id,
+                        Some(Node::Item(hir::Item { owner_id, .. })) => {
+                            cx.tcx.hir().local_def_id_to_hir_id(owner_id.def_id)
+                        },
+                        _ => unreachable!(),
+                    };
+
                     // if unsafe block is part of a let/const/static statement,
                     // and accept_comment_above_statement is set to true
                     // we accept the safety comment in the line the precedes this statement.
-                    accept_comment_above_statement && span_in_body_has_safety_comment(cx, *span)
+                    accept_comment_above_statement
+                        && span_with_attrs_in_body_has_safety_comment(
+                            cx,
+                            *span,
+                            hir_id,
+                            accept_comment_above_attributes,
+                        )
                 } else {
-                    !is_branchy(expr) && span_in_body_has_safety_comment(cx, expr.span)
+                    !is_branchy(expr)
+                        && span_with_attrs_in_body_has_safety_comment(
+                            cx,
+                            expr.span,
+                            expr.hir_id,
+                            accept_comment_above_attributes,
+                        )
                 }
             },
             Node::Stmt(hir::Stmt {
                 kind:
-                    hir::StmtKind::Local(hir::Local { span, .. })
-                    | hir::StmtKind::Expr(hir::Expr { span, .. })
-                    | hir::StmtKind::Semi(hir::Expr { span, .. }),
+                    hir::StmtKind::Local(hir::Local { span, hir_id, .. })
+                    | hir::StmtKind::Expr(hir::Expr { span, hir_id, .. })
+                    | hir::StmtKind::Semi(hir::Expr { span, hir_id, .. }),
                 ..
             })
-            | Node::Local(hir::Local { span, .. })
-            | Node::Item(hir::Item {
+            | Node::Local(hir::Local { span, hir_id, .. }) => {
+                span_with_attrs_in_body_has_safety_comment(cx, *span, *hir_id, accept_comment_above_attributes)
+            },
+            Node::Item(hir::Item {
                 kind: hir::ItemKind::Const(..) | ItemKind::Static(..),
                 span,
+                owner_id,
                 ..
-            }) => span_in_body_has_safety_comment(cx, *span),
+            }) => span_with_attrs_in_body_has_safety_comment(
+                cx,
+                *span,
+                cx.tcx.hir().local_def_id_to_hir_id(owner_id.def_id),
+                accept_comment_above_attributes,
+            ),
             _ => false,
         };
     }
     false
 }
 
+/// Extends `span` to also include its attributes, then checks if that span has a safety comment.
+fn span_with_attrs_in_body_has_safety_comment(
+    cx: &LateContext<'_>,
+    span: Span,
+    hir_id: HirId,
+    accept_comment_above_attributes: bool,
+) -> bool {
+    let span = if accept_comment_above_attributes {
+        include_attrs_in_span(cx, hir_id, span)
+    } else {
+        span
+    };
+
+    span_in_body_has_safety_comment(cx, span)
+}
+
 /// Checks if an expression is "branchy", e.g. loop, match/if/etc.
 fn is_branchy(expr: &hir::Expr<'_>) -> bool {
     matches!(
@@ -394,6 +446,15 @@ fn block_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool {
     ) || span_in_body_has_safety_comment(cx, span)
 }
 
+fn include_attrs_in_span(cx: &LateContext<'_>, hir_id: HirId, span: Span) -> Span {
+    span.to(cx
+        .tcx
+        .hir()
+        .attrs(hir_id)
+        .iter()
+        .fold(span, |acc, attr| acc.to(attr.span)))
+}
+
 enum HasSafetyComment {
     Yes(BytePos),
     No,

clippy_lints/src/utils/conf.rs

@@ -542,6 +542,10 @@ define_Conf! {
     ///
     /// Whether to accept a safety comment to be placed above the statement containing the `unsafe` block
     (accept_comment_above_statement: bool = false),
+        /// Lint: UNDOCUMENTED_UNSAFE_BLOCKS.
+    ///
+    /// Whether to accept a safety comment to be placed above the attributes for the `unsafe` block
+    (accept_comment_above_attributes: bool = false),
 }
 
 /// Search for the configuration file.

tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr

@@ -1,4 +1,5 @@
 error: error reading Clippy's configuration file: unknown field `foobar`, expected one of
+           accept-comment-above-attributes
            accept-comment-above-statement
            allow-dbg-in-tests
            allow-expect-in-tests
@@ -66,6 +67,7 @@ LL | foobar = 42
    | ^^^^^^
 
 error: error reading Clippy's configuration file: unknown field `barfoo`, expected one of
+           accept-comment-above-attributes
            accept-comment-above-statement
            allow-dbg-in-tests
            allow-expect-in-tests

tests/ui-toml/undocumented_unsafe_blocks/clippy.toml

@@ -1 +1,2 @@
 accept-comment-above-statement = true
+accept-comment-above-attributes = true

tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs

@@ -1,7 +1,8 @@
 //@aux-build:proc_macro_unsafe.rs
 
 #![warn(clippy::undocumented_unsafe_blocks, clippy::unnecessary_safety_comment)]
-#![allow(clippy::let_unit_value, clippy::missing_safety_doc)]
+#![allow(deref_nullptr, clippy::let_unit_value, clippy::missing_safety_doc)]
+#![feature(lint_reasons)]
 
 extern crate proc_macro_unsafe;
 
@@ -531,4 +532,36 @@ fn issue_10832() {
         unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
 }
 
+fn issue_8679<T: Copy>() {
+    // SAFETY:
+    #[allow(unsafe_code)]
+    unsafe {}
+
+    // SAFETY:
+    #[expect(unsafe_code, reason = "totally safe")]
+    unsafe {
+        *std::ptr::null::<T>()
+    };
+
+    // Safety: A safety comment
+    #[allow(unsafe_code)]
+    let _some_variable_with_a_very_long_name_to_break_the_line =
+        unsafe { a_function_with_a_very_long_name_to_break_the_line() };
+
+    // Safety: Another safety comment
+    #[allow(unsafe_code)]
+    const _SOME_CONST_WITH_A_VERY_LONG_NAME_TO_BREAK_THE_LINE: u32 =
+        unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
+
+    // Safety: Yet another safety comment
+    #[allow(unsafe_code)]
+    static _SOME_STATIC_WITH_A_VERY_LONG_NAME_TO_BREAK_THE_LINE: u32 =
+        unsafe { a_const_function_with_a_very_long_name_to_break_the_line() };
+
+    // SAFETY:
+    #[allow(unsafe_code)]
+    // This also works I guess
+    unsafe {}
+}
+
 fn main() {}