From 2a73682f77fb169b3c5d60caf5c894c72f5873c3 Mon Sep 17 00:00:00 2001 From: Zihan Date: Sun, 7 Sep 2025 17:38:40 -0400 Subject: [PATCH] `unnecessary_safety_comment`: check for impl items as well changelog: [`unnecessary_safety_comment`]: check for impl items as well Signed-off-by: Zihan --- .../src/undocumented_unsafe_blocks.rs | 210 ++++++++++-------- tests/ui/unnecessary_safety_comment.rs | 26 +++ tests/ui/unnecessary_safety_comment.stderr | 38 +++- 3 files changed, 181 insertions(+), 93 deletions(-) diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs index ba0d4de5f3b3..de00c6c7fb50 100644 --- a/clippy_lints/src/undocumented_unsafe_blocks.rs +++ b/clippy_lints/src/undocumented_unsafe_blocks.rs @@ -189,90 +189,101 @@ impl<'tcx> LateLintPass<'tcx> for UndocumentedUnsafeBlocks { return; } - let mk_spans = |pos: BytePos| { - let source_map = cx.tcx.sess.source_map(); - let span = Span::new(pos, pos, SyntaxContext::root(), None); - let help_span = source_map.span_extend_to_next_char(span, '\n', true); - let span = if source_map.is_multiline(item.span) { - source_map.span_until_char(item.span, '\n') - } else { - item.span - }; - (span, help_span) - }; - - let item_has_safety_comment = item_has_safety_comment(cx, item); + let item_has_safety_comment = item_has_safety_comment(cx, item.span, item.hir_id()); match item_has_safety_comment { - HasSafetyComment::Yes(pos) => check_has_safety_comment(cx, item, mk_spans(pos)), - HasSafetyComment::No => check_has_no_safety_comment(cx, item), + HasSafetyComment::Yes(pos) => check_item_has_safety_comment(cx, item, mk_spans(cx, item.span, pos)), + HasSafetyComment::No => check_item_has_no_safety_comment(cx, item), HasSafetyComment::Maybe => {}, } } + + fn check_impl_item(&mut self, cx: &LateContext<'_>, item: &hir::ImplItem<'_>) { + if item.span.in_external_macro(cx.tcx.sess.source_map()) { + return; + } + + let item_has_safety_comment = item_has_safety_comment(cx, item.span, item.hir_id()); + if let HasSafetyComment::Yes(pos) = item_has_safety_comment { + check_impl_item_has_safety_comment(cx, item, mk_spans(cx, item.span, pos)); + } + } } -fn check_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>, (span, help_span): (Span, Span)) { +fn mk_spans(cx: &LateContext<'_>, item_span: Span, doc_pos: BytePos) -> (Span, Span) { + let source_map = cx.tcx.sess.source_map(); + let span = Span::new(doc_pos, doc_pos, SyntaxContext::root(), None); + let help_span = source_map.span_extend_to_next_char(span, '\n', true); + let span = if source_map.is_multiline(item_span) { + source_map.span_until_char(item_span, '\n') + } else { + item_span + }; + (span, help_span) +} + +fn check_impl_item_has_safety_comment(cx: &LateContext<'_>, item: &hir::ImplItem<'_>, spans: (Span, Span)) { + match &item.kind { + &hir::ImplItemKind::Const(.., body) => { + if !check_body_unsafeness(cx, body) { + lint_unnecessary_safety_comment(cx, cx.tcx.def_descr(item.owner_id.to_def_id()), spans, body.hir_id); + } + }, + hir::ImplItemKind::Type(..) => {}, + hir::ImplItemKind::Fn(sig, ..) => { + if sig.header.is_safe() { + lint_unnecessary_safety_comment(cx, cx.tcx.def_descr(item.owner_id.to_def_id()), spans, item.hir_id()); + } + }, + } +} + +fn check_item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>, spans: (Span, Span)) { match &item.kind { ItemKind::Impl(Impl { of_trait: Some(of_trait), .. - }) if of_trait.safety.is_safe() => { - if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, item.hir_id()) { - span_lint_and_then( - cx, - UNNECESSARY_SAFETY_COMMENT, - span, - "impl has unnecessary safety comment", - |diag| { - diag.span_help(help_span, "consider removing the safety comment"); - }, - ); - } - }, + }) if of_trait.safety.is_safe() => lint_unnecessary_safety_comment(cx, "impl", spans, item.hir_id()), ItemKind::Impl(_) => {}, // const and static items only need a safety comment if their body is an unsafe block, lint otherwise &ItemKind::Const(.., body) | &ItemKind::Static(.., body) => { - if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, body.hir_id) { - let body = cx.tcx.hir_body(body); - if !matches!( - body.value.kind, hir::ExprKind::Block(block, _) - if block.rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) - ) { - span_lint_and_then( - cx, - UNNECESSARY_SAFETY_COMMENT, - span, - format!( - "{} has unnecessary safety comment", - cx.tcx.def_descr(item.owner_id.to_def_id()), - ), - |diag| { - diag.span_help(help_span, "consider removing the safety comment"); - }, - ); - } + if !check_body_unsafeness(cx, body) { + lint_unnecessary_safety_comment(cx, cx.tcx.def_descr(item.owner_id.to_def_id()), spans, body.hir_id); } }, // Aside from unsafe impls and consts/statics with an unsafe block, items in general // do not have safety invariants that need to be documented, so lint those. - _ => { - if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, item.hir_id()) { - span_lint_and_then( - cx, - UNNECESSARY_SAFETY_COMMENT, - span, - format!( - "{} has unnecessary safety comment", - cx.tcx.def_descr(item.owner_id.to_def_id()), - ), - |diag| { - diag.span_help(help_span, "consider removing the safety comment"); - }, - ); - } - }, + _ => lint_unnecessary_safety_comment(cx, cx.tcx.def_descr(item.owner_id.to_def_id()), spans, item.hir_id()), } } -fn check_has_no_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) { + +fn lint_unnecessary_safety_comment( + cx: &LateContext<'_>, + item_name: &str, + (span, help_span): (Span, Span), + hir_id: HirId, +) { + if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, hir_id) { + span_lint_and_then( + cx, + UNNECESSARY_SAFETY_COMMENT, + span, + format!("{item_name} has unnecessary safety comment",), + |diag| { + diag.span_help(help_span, "consider removing the safety comment"); + }, + ); + } +} + +fn check_body_unsafeness(cx: &LateContext<'_>, body: hir::BodyId) -> bool { + let body = cx.tcx.hir_body(body); + matches!( + body.value.kind, hir::ExprKind::Block(block, _) + if block.rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) + ) +} + +fn check_item_has_no_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) { if let ItemKind::Impl(Impl { of_trait: Some(of_trait), .. @@ -488,26 +499,29 @@ enum HasSafetyComment { Maybe, } -/// Checks if the lines immediately preceding the item contain a safety comment. +/// Checks if the lines immediately preceding item or impl item contain a safety comment. #[allow(clippy::collapsible_match)] -fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> HasSafetyComment { - match span_from_macro_expansion_has_safety_comment(cx, item.span) { +fn item_has_safety_comment(cx: &LateContext<'_>, span: Span, hir_id: HirId) -> HasSafetyComment { + match span_from_macro_expansion_has_safety_comment(cx, span) { HasSafetyComment::Maybe => (), has_safety_comment => return has_safety_comment, } - if item.span.ctxt() != SyntaxContext::root() { + if span.ctxt() != SyntaxContext::root() { return HasSafetyComment::No; } - let comment_start = match cx.tcx.parent_hir_node(item.hir_id()) { - Node::Crate(parent_mod) => comment_start_before_item_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_item_in_mod(cx, parent_mod, parent_item.span, item) - } else { - // Doesn't support impls in this position. Pretend a comment was found. - return HasSafetyComment::Maybe; - } + let comment_start = match cx.tcx.parent_hir_node(hir_id) { + Node::Crate(parent_mod) => { + comment_start_before_item(cx, &mod_items_hir_id(parent_mod), parent_mod.spans.inner_span, hir_id) + }, + Node::Item(parent_item) => match &parent_item.kind { + ItemKind::Mod(_, parent_mod) => { + comment_start_before_item(cx, &mod_items_hir_id(parent_mod), parent_item.span, hir_id) + }, + ItemKind::Impl(impl_block) => { + comment_start_before_item(cx, &impl_items_hir_id(impl_block), parent_item.span, hir_id) + }, + _ => return HasSafetyComment::Maybe, }, Node::Stmt(stmt) => { if let Node::Block(block) = cx.tcx.parent_hir_node(stmt.hir_id) { @@ -527,7 +541,7 @@ fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> HasSaf let source_map = cx.sess().source_map(); // If the comment is in the first line of the file, there is no preceding line if let Some(comment_start) = comment_start - && let Ok(unsafe_line) = source_map.lookup_line(item.span.lo()) + && let Ok(unsafe_line) = source_map.lookup_line(span.lo()) && let Ok(comment_start_line) = source_map.lookup_line(comment_start.into()) && let include_first_line_of_file = matches!(comment_start, CommentStartBeforeItem::Start) && (include_first_line_of_file || Arc::ptr_eq(&unsafe_line.sf, &comment_start_line.sf)) @@ -550,6 +564,18 @@ fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> HasSaf HasSafetyComment::Maybe } +fn mod_items_hir_id(module: &hir::Mod<'_>) -> Vec { + module.item_ids.iter().map(|&item_id| item_id.hir_id()).collect() +} + +fn impl_items_hir_id(impl_block: &Impl<'_>) -> Vec { + impl_block + .items + .iter() + .map(|&impl_item_id| impl_item_id.hir_id()) + .collect() +} + /// Checks if the lines immediately preceding the item contain a safety comment. #[allow(clippy::collapsible_match)] fn stmt_has_safety_comment( @@ -572,8 +598,6 @@ fn stmt_has_safety_comment( _ => return HasSafetyComment::Maybe, }; - // if span_with_attrs_has_safety_comment(cx, span, hir_id, accept_comment_above_attrib - // } let mut span = span; if accept_comment_above_attributes { span = include_attrs_in_span(cx, hir_id, span); @@ -617,30 +641,30 @@ impl From for BytePos { } } -fn comment_start_before_item_in_mod( +fn comment_start_before_item( cx: &LateContext<'_>, - parent_mod: &hir::Mod<'_>, - parent_mod_span: Span, - item: &hir::Item<'_>, + items_at_same_level: &[HirId], + parent_span: Span, + target_item: HirId, ) -> Option { - parent_mod.item_ids.iter().enumerate().find_map(|(idx, item_id)| { - if *item_id == item.item_id() { + items_at_same_level.iter().enumerate().find_map(|(idx, &hir_id)| { + if hir_id == target_item { 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()) { + if let Some(sp) = walk_span_to_context(parent_span, SyntaxContext::root()) { return Some(CommentStartBeforeItem::Offset(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 prev_item.span.is_dummy() { + let prev_item_span = cx.tcx.hir_span(items_at_same_level[idx - 1]); + if prev_item_span.is_dummy() { return Some(CommentStartBeforeItem::Start); } - if let Some(sp) = walk_span_to_context(prev_item.span, SyntaxContext::root()) { + if let Some(sp) = walk_span_to_context(prev_item_span, SyntaxContext::root()) { return Some(CommentStartBeforeItem::Offset(sp.hi())); } } @@ -695,7 +719,9 @@ fn get_body_search_span(cx: &LateContext<'_>) -> Option { .. }) => { return maybe_mod_item - .and_then(|item| comment_start_before_item_in_mod(cx, mod_, *span, &item)) + .and_then(|item: hir::Item<'_>| { + comment_start_before_item(cx, &mod_items_hir_id(mod_), *span, item.hir_id()) + }) .map(|comment_start| mod_.spans.inner_span.with_lo(comment_start.into())) .or(Some(*span)); }, diff --git a/tests/ui/unnecessary_safety_comment.rs b/tests/ui/unnecessary_safety_comment.rs index 4440089b3633..e9226eac3586 100644 --- a/tests/ui/unnecessary_safety_comment.rs +++ b/tests/ui/unnecessary_safety_comment.rs @@ -99,4 +99,30 @@ mod issue_12048 { } } +mod issue_15034 { + struct Foo; + trait Baz { + fn baz() {} + } + + impl Baz for Foo { + // Safety: unnecessary + fn baz() {} + //~^ unnecessary_safety_comment + } + + impl Foo { + // SAFETY: unnecessary + const BAR: usize = 0; + //~^ unnecessary_safety_comment + + // Safety: unnecessary + fn foo() {} + //~^ unnecessary_safety_comment + + // Safety: necessary + unsafe fn bar() {} + } +} + fn main() {} diff --git a/tests/ui/unnecessary_safety_comment.stderr b/tests/ui/unnecessary_safety_comment.stderr index 732e6767c178..9a9ecc9a09d3 100644 --- a/tests/ui/unnecessary_safety_comment.stderr +++ b/tests/ui/unnecessary_safety_comment.stderr @@ -112,5 +112,41 @@ help: consider removing the safety comment LL | // SAFETY: unnecessary | ^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 9 previous errors +error: associated function has unnecessary safety comment + --> tests/ui/unnecessary_safety_comment.rs:110:9 + | +LL | fn baz() {} + | ^^^^^^^^^^^ + | +help: consider removing the safety comment + --> tests/ui/unnecessary_safety_comment.rs:109:9 + | +LL | // Safety: unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: associated constant has unnecessary safety comment + --> tests/ui/unnecessary_safety_comment.rs:116:9 + | +LL | const BAR: usize = 0; + | ^^^^^^^^^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> tests/ui/unnecessary_safety_comment.rs:115:9 + | +LL | // SAFETY: unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: associated function has unnecessary safety comment + --> tests/ui/unnecessary_safety_comment.rs:120:9 + | +LL | fn foo() {} + | ^^^^^^^^^^^ + | +help: consider removing the safety comment + --> tests/ui/unnecessary_safety_comment.rs:119:9 + | +LL | // Safety: unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 12 previous errors