Skip to content

fix [undocumented_unsafe_blocks] FP with trait/impl items #12672

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix [undocumented_unsafe_blocks] FP with trait/impl items #12672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
83 changes: 48 additions & 35 deletions clippy_lints/src/undocumented_unsafe_blocks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,12 +342,13 @@ fn block_parents_have_safety_comment(
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)),

node if let Some((span, hir_id)) = span_and_hid_of_item_alike_node(&node)
&& is_const_or_static(&node) =>
{
(span, hir_id)
},

_ => {
if is_branchy(expr) {
return false;
Expand All @@ -363,12 +364,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 @@ -596,32 +598,18 @@ 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 map = cx.tcx.hir();
let mut span = map.body(body).value.span;
let mut maybe_global_var = false;
for (_, node) in map.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,
Node::Item(hir::Item {
kind: ItemKind::Mod(_),
span: item_span,
..
}) => {
span = *item_span;
break;
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 if let Some((span, _)) = span_and_hid_of_item_alike_node(&node)
&& !is_const_or_static(&node) =>
{
return Some(span);
},
Node::Crate(mod_) if maybe_global_var => {
span = mod_.spans.inner_span;
},
_ => break,
_ => {},
}
}
Some(span)
None
Comment on lines +601 to +612
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct for things in submodules? I think a static in a submodule (separate file) would get the span of mod foo; rather than module's inner span

}

fn span_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool {
Expand Down Expand Up @@ -710,3 +698,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(..),
..
})
)
}
42 changes: 41 additions & 1 deletion tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.default.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,5 +312,45 @@ 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:592: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:601: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:611: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:616: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:627:9
|
LL | unsafe { here_is_another_variable_with_long_name };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider adding a safety comment on the preceding line

error: aborting due to 40 previous errors

42 changes: 41 additions & 1 deletion tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.disabled.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,5 +392,45 @@ 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:592: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:601: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:611: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:616: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:627:9
|
LL | unsafe { here_is_another_variable_with_long_name };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider adding a safety comment on the preceding line

error: aborting due to 50 previous errors

41 changes: 41 additions & 0 deletions tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,4 +586,45 @@ 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: good
just_a_simple_variable_with_a_very_long_name_that_has_seventy_eight_characters =
unsafe { here_is_another_variable_with_long_name };
}

fn main() {}