diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs index 09efb12f5da..1992f61bdb4 100644 --- a/clippy_lints/src/undocumented_unsafe_blocks.rs +++ b/clippy_lints/src/undocumented_unsafe_blocks.rs @@ -140,6 +140,7 @@ fn block_has_safety_comment(cx: &LateContext<'_>, block: &hir::Block<'_>) -> boo } /// 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) || span_in_body_has_safety_comment(cx, item.span) { return true; @@ -147,48 +148,51 @@ fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> bool { if item.span.ctxt() == SyntaxContext::root() { if let Some(parent_node) = get_parent_node(cx.tcx, item.hir_id()) { - let mut span_before_item = None; - let mut hi = false; - if let Node::Item(parent_item) = parent_node { - if let ItemKind::Mod(parent_mod) = &parent_item.kind { - for (idx, item_id) in parent_mod.item_ids.iter().enumerate() { - if *item_id == item.item_id() { - if idx == 0 { - // mod A { /* comment */ unsafe impl T {} ... } - // ^------------------------------------------^ gets this span - // ^---------------------^ finally checks the text in this range - hi = false; - span_before_item = Some(parent_item.span); - } else { - let prev_item = cx.tcx.hir().item(parent_mod.item_ids[idx - 1]); - // some_item /* comment */ unsafe impl T {} - // ^-------^ gets this span - // ^---------------^ finally checks the text in this range - hi = true; - span_before_item = Some(prev_item.span); - } - break; - } + let comment_start; + match parent_node { + Node::Crate(parent_mod) => { + comment_start = 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 = 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) => { + comment_start = 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 { + // Doesn't support impls in this position. Pretend a comment was found. + return true; + } + }, + _ => { + // Doesn't support impls in this position. Pretend a comment was found. + return true; + }, } - let span_before_item = span_before_item.unwrap(); let source_map = cx.sess().source_map(); - if let Some(item_span) = walk_span_to_context(item.span, SyntaxContext::root()) - && let Some(span_before_item) = walk_span_to_context(span_before_item, SyntaxContext::root()) - && let Ok(unsafe_line) = source_map.lookup_line(item_span.lo()) - && let Ok(line_before_unsafe) = source_map.lookup_line(if hi { - span_before_item.hi() - } else { - span_before_item.lo() - }) - && Lrc::ptr_eq(&unsafe_line.sf, &line_before_unsafe.sf) + 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() { - line_before_unsafe.line < unsafe_line.line && text_has_safety_comment( + comment_start_line.line < unsafe_line.line && text_has_safety_comment( src, - &unsafe_line.sf.lines[line_before_unsafe.line + 1..=unsafe_line.line], + &unsafe_line.sf.lines[comment_start_line.line + 1..=unsafe_line.line], unsafe_line.sf.start_pos.to_usize(), ) } else { @@ -204,6 +208,35 @@ fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> bool { } } +fn comment_start_before_impl_in_mod( + cx: &LateContext<'_>, + parent_mod: &hir::Mod<'_>, + parent_mod_span: Span, + imple: &hir::Item<'_>, +) -> Option { + 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 = span.ctxt(); diff --git a/tests/ui/undocumented_unsafe_blocks.rs b/tests/ui/undocumented_unsafe_blocks.rs index fd64c60384d..33b6a82f9d2 100644 --- a/tests/ui/undocumented_unsafe_blocks.rs +++ b/tests/ui/undocumented_unsafe_blocks.rs @@ -344,7 +344,7 @@ mod unsafe_impl_smoke_test { unsafe impl A for (i32) {} mod sub_mod { - // error: also works for the first item + // error: unsafe impl B for (u32) {} unsafe trait B {} } @@ -363,24 +363,51 @@ mod unsafe_impl_smoke_test { mod unsafe_impl_from_macro { unsafe trait T {} + // error macro_rules! no_safety_comment { ($t:ty) => { unsafe impl T for $t {} }; } - // error + + // 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 {} @@ -440,4 +467,22 @@ mod unsafe_impl_invalid_comment { 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() {} diff --git a/tests/ui/undocumented_unsafe_blocks.stderr b/tests/ui/undocumented_unsafe_blocks.stderr index 3a1f647b06d..b79949e9d06 100644 --- a/tests/ui/undocumented_unsafe_blocks.stderr +++ b/tests/ui/undocumented_unsafe_blocks.stderr @@ -164,7 +164,7 @@ 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:368:13 + --> $DIR/undocumented_unsafe_blocks.rs:369:13 | LL | unsafe impl T for $t {} | ^^^^^^^^^^^^^^^^^^^^^^^ @@ -176,7 +176,47 @@ LL | no_safety_comment!(()); = 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:427:5 + --> $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 () {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -184,7 +224,7 @@ 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:431:19 + --> $DIR/undocumented_unsafe_blocks.rs:458:19 | LL | /* SAFETY: */ unsafe impl InlineComment for () {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -192,7 +232,7 @@ 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:435:5 + --> $DIR/undocumented_unsafe_blocks.rs:462:5 | LL | unsafe impl TrailingComment for () {} // SAFETY: | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -200,12 +240,28 @@ 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:440:5 + --> $DIR/undocumented_unsafe_blocks.rs:467:5 | LL | unsafe impl Interference for () {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: consider adding a safety comment on the preceding line -error: aborting due to 25 previous errors +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