Skip to content
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

undocumented_unsafe_blocks does not trigger on unsafe trait impls #8761

Merged
merged 5 commits into from
May 16, 2022
Merged

undocumented_unsafe_blocks does not trigger on unsafe trait impls #8761

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d0ca74
Resolved conflicts
tamaroning Apr 29, 2022
8d85889
fix
tamaroning May 7, 2022
aaf87c3
fix
tamaroning May 10, 2022
1926217
chore
tamaroning May 10, 2022
9e66f2d
fix
tamaroning May 12, 2022
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
216 changes: 132 additions & 84 deletions clippy_lints/src/undocumented_unsafe_blocks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +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 as hir;
use rustc_hir::{Block, BlockCheckMode, UnsafeSource};
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, Span, SyntaxContext};
use std::rc::Rc;

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.
///
Expand All @@ -36,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.
///
Expand Down Expand Up @@ -68,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();
Expand All @@ -89,70 +88,36 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
}
}

fn check_mod(&mut self, cx: &LateContext<'_>, module: &'_ hir::Mod<'_>, mod_span: Span, hir_id: hir::HirId) {
let source_map = cx.sess().source_map();
let mut item_and_spans: Vec<(&hir::Item<'_>, Span)> = Vec::new(); // (start, end, item)

// Collect all items and their spans
for item_id in module.item_ids {
let item = cx.tcx.hir().item(*item_id);
item_and_spans.push((item, item.span));
}
// Sort items by start position
item_and_spans.sort_by_key(|e| e.1.lo());

for (idx, (item, item_span)) in item_and_spans.iter().enumerate() {
if let hir::ItemKind::Impl(imple) = &item.kind
&& imple.unsafety == hir::Unsafety::Unsafe
&& !item_span.from_expansion()
&& !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, hir_id)
{
// Checks if the lines immediately preceding the impl contain a safety comment.
let impl_has_safety_comment = {
let span_before_impl = if idx == 0 {
// mod A { /* comment */ unsafe impl T {} }
// ^--------------------^
todo!();
//mod_span.until(module.spans)
} else {
// unsafe impl S {} /* comment */ unsafe impl T {}
// ^-------------^
item_and_spans[idx - 1].1.between(*item_span)
};

if let Ok(start) = source_map.lookup_line(span_before_impl.lo())
&& let Ok(end) = source_map.lookup_line(span_before_impl.hi())
&& let Some(src) = start.sf.src.as_deref()
{
start.line < end.line && text_has_safety_comment(
src,
&start.sf.lines[start.line + 1 ..= end.line],
start.sf.start_pos.to_usize()
)
} else {
// Problem getting source text. Pretend a comment was found.
true
}
};
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
};

if !impl_has_safety_comment {
span_lint_and_help(
cx,
UNDOCUMENTED_UNSAFE_BLOCKS,
*item_span,
"unsafe impl missing a safety comment",
None,
"consider adding a safety comment on the preceding line",
);
}
}
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
Expand All @@ -162,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
Expand All @@ -171,13 +136,85 @@ 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.
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) {
Copy link
Contributor

@Jarcho Jarcho May 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This should be returning false if the item is within a function, but doesn't have a safety comment. e.g.

unsafe trait Foo {}
fn f() {
    Struct S;
    // no comment
    unsafe impl Foo for S {}
}

Not a particularly big deal on it's own, but it contributes to the ICE later.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's still duplicate work here. Easiest fix would be to remove the call to span_in_body_has_safety_comment as it's handled afterwards when matching on Node::Stmt(_).

return true;
}

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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to use Iterator::find_map here. the extraction of either Span::lo or Span::hi could be done here as well.

This should get rid of the need to have span_before_item and hi as mutable.

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 span_before_item = span_before_item.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

This can fail when the impl is inside a function and I think the crate root.


let source_map = cx.sess().source_map();
if let Some(item_span) = walk_span_to_context(item.span, SyntaxContext::root())
Copy link
Contributor

Choose a reason for hiding this comment

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

walk_span_to_context isn't needed here. item.span is already at the root context.

&& 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)
&& let Some(src) = unsafe_line.sf.src.as_deref()
{
line_before_unsafe.line < unsafe_line.line && text_has_safety_comment(
src,
&unsafe_line.sf.lines[line_before_unsafe.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 span_from_macro_expansion_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool {
let source_map = cx.sess().source_map();
let ctxt = block.span.ctxt();
if ctxt != SyntaxContext::root() {
// From a macro expansion. Get the text from the start of the macro declaration to start of the unsafe block.
let ctxt = span.ctxt();
if ctxt == SyntaxContext::root() {
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()
Expand All @@ -191,24 +228,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.
// 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(),
)
if let Ok(unsafe_line) = source_map.lookup_line(span.lo())
&& 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.
// 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.
true
false
}
}

Expand Down
17 changes: 12 additions & 5 deletions tests/ui/undocumented_unsafe_blocks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,15 +363,22 @@ mod unsafe_impl_smoke_test {
mod unsafe_impl_from_macro {
unsafe trait T {}

macro_rules! unsafe_impl {
macro_rules! no_safety_comment {
($t:ty) => {
unsafe impl T for $t {}
};
}
// ok: from macro expanision
unsafe_impl!(());
// ok: from macro expansion
unsafe_impl!(i32);
// error
no_safety_comment!(());

macro_rules! with_safety_comment {
($t:ty) => {
// SAFETY:
unsafe impl T for $t {}
};
}
// ok
with_safety_comment!((i32));
}

#[rustfmt::skip]
Expand Down
22 changes: 17 additions & 5 deletions tests/ui/undocumented_unsafe_blocks.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,36 +164,48 @@ 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:420:5
--> $DIR/undocumented_unsafe_blocks.rs:368: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:427: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:424:19
--> $DIR/undocumented_unsafe_blocks.rs:431: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:428:5
--> $DIR/undocumented_unsafe_blocks.rs:435: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:433:5
--> $DIR/undocumented_unsafe_blocks.rs:440:5
|
LL | unsafe impl Interference for () {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider adding a safety comment on the preceding line

error: aborting due to 24 previous errors
error: aborting due to 25 previous errors