Skip to content

fix: undocumented_unsafe_blocks FP on trait/impl items #13888

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 1, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 88 additions & 45 deletions clippy_lints/src/undocumented_unsafe_blocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,33 @@ fn is_unsafe_from_proc_macro(cx: &LateContext<'_>, span: Span) -> bool {
.is_none_or(|src| !src.starts_with("unsafe"))
}

fn find_unsafe_block_parent_in_expr<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx hir::Expr<'tcx>,
) -> Option<(Span, HirId)> {
match cx.tcx.parent_hir_node(expr.hir_id) {
Node::LetStmt(hir::LetStmt { span, hir_id, .. })
| Node::Expr(hir::Expr {
hir_id,
kind: hir::ExprKind::Assign(_, _, span),
..
}) => Some((*span, *hir_id)),
Node::Expr(expr) => find_unsafe_block_parent_in_expr(cx, expr),
node if let Some((span, hir_id)) = span_and_hid_of_item_alike_node(&node)
&& is_const_or_static(&node) =>
{
Some((span, hir_id))
},

_ => {
if is_branchy(expr) {
return None;
}
Some((expr.span, expr.hir_id))
},
}
}

// Checks if any parent {expression, statement, block, local, const, static}
// has a safety comment
fn block_parents_have_safety_comment(
Expand All @@ -348,21 +375,7 @@ fn block_parents_have_safety_comment(
id: HirId,
) -> bool {
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)),
_ => {
if is_branchy(expr) {
return false;
}
(expr.span, expr.hir_id)
},
},
Node::Expr(expr) if let Some(inner) = find_unsafe_block_parent_in_expr(cx, expr) => inner,
Node::Stmt(hir::Stmt {
kind:
hir::StmtKind::Let(hir::LetStmt { span, hir_id, .. })
Expand All @@ -371,12 +384,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((span, hir_id)) = span_and_hid_of_item_alike_node(&node)
&& is_const_or_static(&node) =>
{
(span, hir_id)
},

_ => return false,
};
// if unsafe block is part of a let/const/static statement,
Expand Down Expand Up @@ -427,12 +441,12 @@ fn block_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool {
}

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())))
span.to(cx.tcx.hir().attrs(hir_id).iter().fold(span, |acc, attr| {
if attr.is_doc_comment() {
return acc;
}
acc.to(attr.span())
}))
}

enum HasSafetyComment {
Expand Down Expand Up @@ -604,31 +618,35 @@ fn span_from_macro_expansion_has_safety_comment(cx: &LateContext<'_>, span: Span

fn get_body_search_span(cx: &LateContext<'_>) -> Option<Span> {
let body = cx.enclosing_body?;
let mut span = cx.tcx.hir_body(body).value.span;
let mut maybe_global_var = false;
for (_, node) in cx.tcx.hir_parent_iter(body.hir_id) {
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,
let mut maybe_mod_item = None;

for (_, parent_node) in cx.tcx.hir_parent_iter(body.hir_id) {
match parent_node {
Node::Crate(mod_) => return Some(mod_.spans.inner_span),
Node::Item(hir::Item {
kind: ItemKind::Mod(_),
span: item_span,
kind: ItemKind::Mod(mod_),
span,
..
}) => {
span = *item_span;
break;
return maybe_mod_item
.and_then(|item| comment_start_before_item_in_mod(cx, mod_, *span, &item))
.map(|comment_start| mod_.spans.inner_span.with_lo(comment_start))
.or(Some(*span));
},
node if let Some((span, _)) = span_and_hid_of_item_alike_node(&node)
&& !is_const_or_static(&node) =>
{
return Some(span);
},
Node::Item(item) => {
maybe_mod_item = Some(*item);
},
Node::Crate(mod_) if maybe_global_var => {
span = mod_.spans.inner_span;
_ => {
maybe_mod_item = None;
},
_ => break,
}
}
Some(span)
None
}

fn span_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool {
Expand Down Expand Up @@ -717,3 +735,28 @@ fn text_has_safety_comment(src: &str, line_starts: &[RelativeBytePos], start_pos
}
}
}

fn span_and_hid_of_item_alike_node(node: &Node<'_>) -> Option<(Span, HirId)> {
match node {
Node::Item(item) => Some((item.span, item.owner_id.into())),
Node::TraitItem(ti) => Some((ti.span, ti.owner_id.into())),
Node::ImplItem(ii) => Some((ii.span, ii.owner_id.into())),
_ => None,
}
}

fn is_const_or_static(node: &Node<'_>) -> bool {
matches!(
node,
Node::Item(hir::Item {
kind: ItemKind::Const(..) | ItemKind::Static(..),
..
}) | Node::ImplItem(hir::ImplItem {
kind: hir::ImplItemKind::Const(..),
..
}) | Node::TraitItem(hir::TraitItem {
kind: hir::TraitItemKind::Const(..),
..
})
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -310,5 +310,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:638:52
|
LL | const NO_SAFETY_IN_TRAIT_BUT_IN_IMPL: u8 = unsafe { 0 };
| ^^^^^^^^^^^^
|
= 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:647: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:657:42
|
LL | const HAS_SAFETY_IN_TRAIT: i32 = unsafe { 3 };
| ^^^^^^^^^^^^
|
= 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:662: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

Original file line number Diff line number Diff line change
Expand Up @@ -390,5 +390,53 @@ 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:638:52
|
LL | const NO_SAFETY_IN_TRAIT_BUT_IN_IMPL: u8 = unsafe { 0 };
| ^^^^^^^^^^^^
|
= 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:647: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:657:42
|
LL | const HAS_SAFETY_IN_TRAIT: i32 = unsafe { 3 };
| ^^^^^^^^^^^^
|
= 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:662:40
|
LL | const NO_SAFETY_IN_IMPL: 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:673:9
|
LL | unsafe { here_is_another_variable_with_long_name };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= 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:702:9
|
LL | unsafe { Date::__from_ordinal_date_unchecked(1970, 1) }.into_julian_day_just_make_this_line_longer();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider adding a safety comment on the preceding line

error: aborting due to 51 previous errors

Original file line number Diff line number Diff line change
Expand Up @@ -632,4 +632,75 @@ mod issue_11246 {
// Safety: Another safety comment
const FOO: () = unsafe {};

// trait items and impl items
mod issue_11709 {
trait MyTrait {
const NO_SAFETY_IN_TRAIT_BUT_IN_IMPL: u8 = unsafe { 0 };
//~^ ERROR: unsafe block missing a safety comment

// 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 };

const HAS_SAFETY_IN_TRAIT: i32 = unsafe { 3 };
//~^ ERROR: unsafe block missing a safety comment
}

impl UnsafeStruct {
const NO_SAFETY_IN_IMPL: i32 = unsafe { 1 };
//~^ ERROR: unsafe block missing a safety comment
}
}

fn issue_13024() {
let mut just_a_simple_variable_with_a_very_long_name_that_has_seventy_eight_characters = 0;
let here_is_another_variable_with_long_name = 100;

// SAFETY: fail ONLY if `accept-comment-above-statement = false`
just_a_simple_variable_with_a_very_long_name_that_has_seventy_eight_characters =
unsafe { here_is_another_variable_with_long_name };
//~[disabled]^ undocumented_unsafe_blocks
}

// https://docs.rs/time/0.3.36/src/time/offset_date_time.rs.html#35
mod issue_11709_regression {
use std::num::NonZeroI32;

struct Date {
value: NonZeroI32,
}

impl Date {
const unsafe fn __from_ordinal_date_unchecked(year: i32, ordinal: u16) -> Self {
Self {
// Safety: The caller must guarantee that `ordinal` is not zero.
value: unsafe { NonZeroI32::new_unchecked((year << 9) | ordinal as i32) },
}
}

const fn into_julian_day_just_make_this_line_longer(self) -> i32 {
42
}
}

/// The Julian day of the Unix epoch.
// SAFETY: fail ONLY if `accept-comment-above-attribute = false`
#[allow(unsafe_code)]
const UNIX_EPOCH_JULIAN_DAY: i32 =
unsafe { Date::__from_ordinal_date_unchecked(1970, 1) }.into_julian_day_just_make_this_line_longer();
//~[disabled]^ undocumented_unsafe_blocks
}

fn main() {}