Auto merge of #8761 - tamaroning:fix_8505, r=Jarcho

`undocumented_unsafe_blocks` does not trigger on unsafe trait impls

Closes #8505

changelog: This lint checks unsafe impls NOT from macro expansions and checks ones in macro declarations.
~~`unsafe impl`s from macro invocations don't trigger the lint for now.~~
~~This lint checks unsafe impls from/not from macro expansions~~

clippy_lints/src/undocumented_unsafe_blocks.rs

@@ -1,17 +1,18 @@
 use clippy_utils::diagnostics::span_lint_and_help;
-use clippy_utils::is_lint_allowed;
 use clippy_utils::source::walk_span_to_context;
+use clippy_utils::{get_parent_node, is_lint_allowed};
 use rustc_data_structures::sync::Lrc;
-use rustc_hir::{Block, BlockCheckMode, UnsafeSource};
+use rustc_hir as hir;
+use rustc_hir::{Block, BlockCheckMode, ItemKind, Node, UnsafeSource};
 use rustc_lexer::{tokenize, TokenKind};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::{BytePos, Pos, SyntaxContext};
+use rustc_span::{BytePos, Pos, Span, SyntaxContext};
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for `unsafe` blocks without a `// SAFETY: ` comment
+    /// Checks for `unsafe` blocks and impls without a `// SAFETY: ` comment
     /// explaining why the unsafe operations performed inside
     /// the block are safe.
     ///
@@ -34,7 +35,7 @@ declare_clippy_lint! {
     /// ```
     ///
     /// ### Why is this bad?
-    /// Undocumented unsafe blocks can make it difficult to
+    /// Undocumented unsafe blocks and impls can make it difficult to
     /// read and maintain code, as well as uncover unsoundness
     /// and bugs.
     ///
@@ -66,7 +67,7 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
         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)
-            && !is_unsafe_from_proc_macro(cx, block)
+            && !is_unsafe_from_proc_macro(cx, block.span)
             && !block_has_safety_comment(cx, block)
         {
             let source_map = cx.tcx.sess.source_map();
@@ -86,11 +87,37 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
             );
         }
     }
+
+    fn check_item(&mut self, cx: &LateContext<'_>, item: &hir::Item<'_>) {
+        if let hir::ItemKind::Impl(imple) = item.kind
+            && imple.unsafety == hir::Unsafety::Unsafe
+            && !in_external_macro(cx.tcx.sess, item.span)
+            && !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, item.hir_id())
+            && !is_unsafe_from_proc_macro(cx, item.span)
+            && !item_has_safety_comment(cx, item)
+        {
+            let source_map = cx.tcx.sess.source_map();
+            let span = if source_map.is_multiline(item.span) {
+                source_map.span_until_char(item.span, '\n')
+            } else {
+                item.span
+            };
+
+            span_lint_and_help(
+                cx,
+                UNDOCUMENTED_UNSAFE_BLOCKS,
+                span,
+                "unsafe impl missing a safety comment",
+                None,
+                "consider adding a safety comment on the preceding line",
+            );
+        }
+    }
 }
 
-fn is_unsafe_from_proc_macro(cx: &LateContext<'_>, block: &Block<'_>) -> bool {
+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(block.span.lo());
+    let file_pos = source_map.lookup_byte_offset(span.lo());
     file_pos
         .sf
         .src
@@ -100,7 +127,7 @@ fn is_unsafe_from_proc_macro(cx: &LateContext<'_>, block: &Block<'_>) -> bool {
 }
 
 /// Checks if the lines immediately preceding the block contain a safety comment.
-fn block_has_safety_comment(cx: &LateContext<'_>, block: &Block<'_>) -> bool {
+fn block_has_safety_comment(cx: &LateContext<'_>, block: &hir::Block<'_>) -> bool {
     // This intentionally ignores text before the start of a function so something like:
     // ```
     //     // SAFETY: reason
@@ -109,13 +136,115 @@ fn block_has_safety_comment(cx: &LateContext<'_>, block: &Block<'_>) -> bool {
     // won't work. This is to avoid dealing with where such a comment should be place relative to
     // attributes and doc comments.
 
+    span_from_macro_expansion_has_safety_comment(cx, block.span) || span_in_body_has_safety_comment(cx, block.span)
+}
+
+/// Checks if the lines immediately preceding the item contain a safety comment.
+#[allow(clippy::collapsible_match)]
+fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> bool {
+    if span_from_macro_expansion_has_safety_comment(cx, item.span) {
+        return true;
+    }
+
+    if item.span.ctxt() == SyntaxContext::root() {
+        if let Some(parent_node) = get_parent_node(cx.tcx, item.hir_id()) {
+            let comment_start = match parent_node {
+                Node::Crate(parent_mod) => {
+                    comment_start_before_impl_in_mod(cx, parent_mod, parent_mod.spans.inner_span, item)
+                },
+                Node::Item(parent_item) => {
+                    if let ItemKind::Mod(parent_mod) = &parent_item.kind {
+                        comment_start_before_impl_in_mod(cx, parent_mod, parent_item.span, item)
+                    } else {
+                        // Doesn't support impls in this position. Pretend a comment was found.
+                        return true;
+                    }
+                },
+                Node::Stmt(stmt) => {
+                    if let Some(stmt_parent) = get_parent_node(cx.tcx, stmt.hir_id) {
+                        match stmt_parent {
+                            Node::Block(block) => walk_span_to_context(block.span, SyntaxContext::root()).map(Span::lo),
+                            _ => {
+                                // Doesn't support impls in this position. Pretend a comment was found.
+                                return true;
+                            },
+                        }
+                    } else {
+                        // Problem getting the parent node. Pretend a comment was found.
+                        return true;
+                    }
+                },
+                _ => {
+                    // Doesn't support impls in this position. Pretend a comment was found.
+                    return true;
+                },
+            };
+
+            let source_map = cx.sess().source_map();
+            if let Some(comment_start) = comment_start
+                && let Ok(unsafe_line) = source_map.lookup_line(item.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()
+            {
+                comment_start_line.line < unsafe_line.line && text_has_safety_comment(
+                    src,
+                    &unsafe_line.sf.lines[comment_start_line.line + 1..=unsafe_line.line],
+                    unsafe_line.sf.start_pos.to_usize(),
+                )
+            } else {
+                // Problem getting source text. Pretend a comment was found.
+                true
+            }
+        } else {
+            // No parent node. Pretend a comment was found.
+            true
+        }
+    } else {
+        false
+    }
+}
+
+fn comment_start_before_impl_in_mod(
+    cx: &LateContext<'_>,
+    parent_mod: &hir::Mod<'_>,
+    parent_mod_span: Span,
+    imple: &hir::Item<'_>,
+) -> Option<BytePos> {
+    parent_mod.item_ids.iter().enumerate().find_map(|(idx, item_id)| {
+        if *item_id == imple.item_id() {
+            if idx == 0 {
+                // mod A { /* comment */ unsafe impl T {} ... }
+                // ^------------------------------------------^ returns the start of this span
+                // ^---------------------^ finally checks comments in this range
+                if let Some(sp) = walk_span_to_context(parent_mod_span, SyntaxContext::root()) {
+                    return Some(sp.lo());
+                }
+            } else {
+                // some_item /* comment */ unsafe impl T {}
+                // ^-------^ returns the end of this span
+                //         ^---------------^ finally checks comments in this range
+                let prev_item = cx.tcx.hir().item(parent_mod.item_ids[idx - 1]);
+                if let Some(sp) = walk_span_to_context(prev_item.span, SyntaxContext::root()) {
+                    return Some(sp.hi());
+                }
+            }
+        }
+        None
+    })
+}
+
+fn span_from_macro_expansion_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool {
     let source_map = cx.sess().source_map();
-    let ctxt = block.span.ctxt();
+    let ctxt = span.ctxt();
-    if ctxt != SyntaxContext::root() {
+    if ctxt == SyntaxContext::root() {
-        // From a macro expansion. Get the text from the start of the macro declaration to start of the unsafe block.
+        false
+    } else {
+        // From a macro expansion. Get the text from the start of the macro declaration to start of the
+        // unsafe block.
         //     macro_rules! foo { () => { stuff }; (x) => { unsafe { stuff } }; }
         //     ^--------------------------------------------^
-        if let Ok(unsafe_line) = source_map.lookup_line(block.span.lo())
+        if let Ok(unsafe_line) = source_map.lookup_line(span.lo())
             && let Ok(macro_line) = source_map.lookup_line(ctxt.outer_expn_data().def_site.lo())
             && Lrc::ptr_eq(&unsafe_line.sf, &macro_line.sf)
             && let Some(src) = unsafe_line.sf.src.as_deref()
@@ -129,24 +258,35 @@ fn block_has_safety_comment(cx: &LateContext<'_>, block: &Block<'_>) -> bool {
             // Problem getting source text. Pretend a comment was found.
             true
         }
-    } else if let Ok(unsafe_line) = source_map.lookup_line(block.span.lo())
+    }
+}
+
+fn span_in_body_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool {
+    let source_map = cx.sess().source_map();
+    let ctxt = span.ctxt();
+    if ctxt == SyntaxContext::root()
         && let Some(body) = cx.enclosing_body
-        && let Some(body_span) = walk_span_to_context(cx.tcx.hir().body(body).value.span, SyntaxContext::root())
-        && let Ok(body_line) = source_map.lookup_line(body_span.lo())
-        && Lrc::ptr_eq(&unsafe_line.sf, &body_line.sf)
-        && let Some(src) = unsafe_line.sf.src.as_deref()
     {
-        // Get the text from the start of function body to the unsafe block.
+        if let Ok(unsafe_line) = source_map.lookup_line(span.lo())
-        //     fn foo() { some_stuff; unsafe { stuff }; other_stuff; }
+            && let Some(body_span) = walk_span_to_context(cx.tcx.hir().body(body).value.span, SyntaxContext::root())
-        //              ^-------------^
+            && let Ok(body_line) = source_map.lookup_line(body_span.lo())
-        body_line.line < unsafe_line.line && text_has_safety_comment(
+            && Lrc::ptr_eq(&unsafe_line.sf, &body_line.sf)
-            src,
+            && let Some(src) = unsafe_line.sf.src.as_deref()
-            &unsafe_line.sf.lines[body_line.line + 1..=unsafe_line.line],
+        {
-            unsafe_line.sf.start_pos.to_usize(),
+            // Get the text from the start of function body to the unsafe block.
-        )
+            //     fn foo() { some_stuff; unsafe { stuff }; other_stuff; }
+            //              ^-------------^
+            body_line.line < unsafe_line.line && text_has_safety_comment(
+                src,
+                &unsafe_line.sf.lines[body_line.line + 1..=unsafe_line.line],
+                unsafe_line.sf.start_pos.to_usize(),
+            )
+        } else {
+            // Problem getting source text. Pretend a comment was found.
+            true
+        }
     } else {
-        // Problem getting source text. Pretend a comment was found.
+        false
-        true
     }
 }
 

tests/ui/undocumented_unsafe_blocks.rs

@@ -1,7 +1,7 @@
 // aux-build:proc_macro_unsafe.rs
 
 #![warn(clippy::undocumented_unsafe_blocks)]
-#![allow(clippy::let_unit_value)]
+#![allow(clippy::let_unit_value, clippy::missing_safety_doc)]
 
 extern crate proc_macro_unsafe;
 
@@ -334,4 +334,155 @@ pub fn print_binary_tree() {
     println!("{}", unsafe { String::from_utf8_unchecked(vec![]) });
 }
 
+mod unsafe_impl_smoke_test {
+    unsafe trait A {}
+
+    // error: no safety comment
+    unsafe impl A for () {}
+
+    // Safety: ok
+    unsafe impl A for (i32) {}
+
+    mod sub_mod {
+        // error:
+        unsafe impl B for (u32) {}
+        unsafe trait B {}
+    }
+
+    #[rustfmt::skip]
+    mod sub_mod2 {
+        // 
+        // SAFETY: ok
+        // 
+
+        unsafe impl B for (u32) {}
+        unsafe trait B {}
+    }
+}
+
+mod unsafe_impl_from_macro {
+    unsafe trait T {}
+
+    // error
+    macro_rules! no_safety_comment {
+        ($t:ty) => {
+            unsafe impl T for $t {}
+        };
+    }
+
+    // ok
+    no_safety_comment!(());
+
+    // ok
+    macro_rules! with_safety_comment {
+        ($t:ty) => {
+            // SAFETY:
+            unsafe impl T for $t {}
+        };
+    }
+
+    // ok
+    with_safety_comment!((i32));
+}
+
+mod unsafe_impl_macro_and_not_macro {
+    unsafe trait T {}
+
+    // error
+    macro_rules! no_safety_comment {
+        ($t:ty) => {
+            unsafe impl T for $t {}
+        };
+    }
+
+    // ok
+    no_safety_comment!(());
+
+    // error
+    unsafe impl T for (i32) {}
+
+    // ok
+    no_safety_comment!(u32);
+
+    // error
+    unsafe impl T for (bool) {}
+}
+
+#[rustfmt::skip]
+mod unsafe_impl_valid_comment {
+    unsafe trait SaFety {}
+    // SaFety:
+    unsafe impl SaFety for () {}
+
+    unsafe trait MultiLineComment {}
+    // The following impl is safe
+    // ...
+    // Safety: reason
+    unsafe impl MultiLineComment for () {}
+
+    unsafe trait NoAscii {}
+    // 安全 SAFETY: 以下のコードは安全です
+    unsafe impl NoAscii for () {}
+
+    unsafe trait InlineAndPrecedingComment {}
+    // SAFETY:
+    /* comment */ unsafe impl InlineAndPrecedingComment for () {}
+
+    unsafe trait BuriedSafety {}
+    // Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor
+    // incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation
+    // ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in
+    // reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint
+    // occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est
+    // laborum. Safety:
+    // Tellus elementum sagittis vitae et leo duis ut diam quam. Sit amet nulla facilisi
+    // morbi tempus iaculis urna. Amet luctus venenatis lectus magna. At quis risus sed vulputate odio
+    // ut. Luctus venenatis lectus magna fringilla urna. Tortor id aliquet lectus proin nibh nisl
+    // condimentum id venenatis. Vulputate dignissim suspendisse in est ante in nibh mauris cursus.
+    unsafe impl BuriedSafety for () {}
+
+    unsafe trait MultiLineBlockComment {}
+    /* This is a description
+     * Safety: */
+    unsafe impl MultiLineBlockComment for () {}
+}
+
+#[rustfmt::skip]
+mod unsafe_impl_invalid_comment {
+    unsafe trait NoComment {}
+
+    unsafe impl NoComment for () {}
+
+    unsafe trait InlineComment {}
+
+    /* SAFETY: */ unsafe impl InlineComment for () {}
+
+    unsafe trait TrailingComment {}
+
+    unsafe impl TrailingComment for () {} // SAFETY:
+
+    unsafe trait Interference {}
+    // SAFETY:
+    const BIG_NUMBER: i32 = 1000000;
+    unsafe impl Interference for () {}
+}
+
+unsafe trait ImplInFn {}
+
+fn impl_in_fn() {
+    // error
+    unsafe impl ImplInFn for () {}
+
+    // SAFETY: ok
+    unsafe impl ImplInFn for (i32) {}
+}
+
+unsafe trait CrateRoot {}
+
+// error
+unsafe impl CrateRoot for () {}
+
+// SAFETY: ok
+unsafe impl CrateRoot for (i32) {}
+
 fn main() {}

tests/ui/undocumented_unsafe_blocks.stderr

@@ -147,5 +147,121 @@ LL |     println!("{}", unsafe { String::from_utf8_unchecked(vec![]) });
    |
    = help: consider adding a safety comment on the preceding line
 
-error: aborting due to 18 previous errors
+error: unsafe impl missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:341:5
+   |
+LL |     unsafe impl A for () {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe impl missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:348:9
+   |
+LL |         unsafe impl B for (u32) {}
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe impl missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:369:13
+   |
+LL |             unsafe impl T for $t {}
+   |             ^^^^^^^^^^^^^^^^^^^^^^^
+...
+LL |     no_safety_comment!(());
+   |     ---------------------- in this macro invocation
+   |
+   = help: consider adding a safety comment on the preceding line
+   = note: this error originates in the macro `no_safety_comment` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: unsafe impl missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:394:13
+   |
+LL |             unsafe impl T for $t {}
+   |             ^^^^^^^^^^^^^^^^^^^^^^^
+...
+LL |     no_safety_comment!(());
+   |     ---------------------- in this macro invocation
+   |
+   = help: consider adding a safety comment on the preceding line
+   = note: this error originates in the macro `no_safety_comment` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: unsafe impl missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:402:5
+   |
+LL |     unsafe impl T for (i32) {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe impl missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:394:13
+   |
+LL |             unsafe impl T for $t {}
+   |             ^^^^^^^^^^^^^^^^^^^^^^^
+...
+LL |     no_safety_comment!(u32);
+   |     ----------------------- in this macro invocation
+   |
+   = help: consider adding a safety comment on the preceding line
+   = note: this error originates in the macro `no_safety_comment` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: unsafe impl missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:408:5
+   |
+LL |     unsafe impl T for (bool) {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe impl missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:454:5
+   |
+LL |     unsafe impl NoComment for () {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe impl missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:458:19
+   |
+LL |     /* SAFETY: */ unsafe impl InlineComment for () {}
+   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe impl missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:462:5
+   |
+LL |     unsafe impl TrailingComment for () {} // SAFETY:
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe impl missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:467:5
+   |
+LL |     unsafe impl Interference for () {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe impl missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:474:5
+   |
+LL |     unsafe impl ImplInFn for () {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: unsafe impl missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:483:1
+   |
+LL | unsafe impl CrateRoot for () {}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a safety comment on the preceding line
+
+error: aborting due to 31 previous errors