diff options
| -rw-r--r-- | clippy_lints/src/undocumented_unsafe_blocks.rs | 126 | ||||
| -rw-r--r-- | clippy_utils/src/visitors.rs | 12 | ||||
| -rw-r--r-- | tests/ui/undocumented_unsafe_blocks.rs | 31 | ||||
| -rw-r--r-- | tests/ui/undocumented_unsafe_blocks.stderr | 60 |
4 files changed, 220 insertions, 9 deletions
diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs index 080a481e87f..c60144df757 100644 --- a/clippy_lints/src/undocumented_unsafe_blocks.rs +++ b/clippy_lints/src/undocumented_unsafe_blocks.rs @@ -1,6 +1,10 @@ +use std::ops::ControlFlow; + use clippy_utils::diagnostics::span_lint_and_help; use clippy_utils::source::walk_span_to_context; +use clippy_utils::visitors::{for_each_expr_with_closures, Descend}; use clippy_utils::{get_parent_node, is_lint_allowed}; +use hir::HirId; use rustc_data_structures::sync::Lrc; use rustc_hir as hir; use rustc_hir::{Block, BlockCheckMode, ItemKind, Node, UnsafeSource}; @@ -90,8 +94,8 @@ declare_clippy_lint! { declare_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS, UNNECESSARY_SAFETY_COMMENT]); -impl LateLintPass<'_> for UndocumentedUnsafeBlocks { - fn check_block(&mut self, cx: &LateContext<'_>, block: &'_ Block<'_>) { +impl<'tcx> LateLintPass<'tcx> for UndocumentedUnsafeBlocks { + fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) { 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) @@ -115,6 +119,45 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks { "consider adding a safety comment on the preceding line", ); } + + if let Some(tail) = block.expr + && !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, tail.hir_id) + && !in_external_macro(cx.tcx.sess, tail.span) + && let HasSafetyComment::Yes(pos) = stmt_has_safety_comment(cx, tail.span, tail.hir_id) + && let Some(help_span) = expr_has_unnecessary_safety_comment(cx, tail, pos) + { + span_lint_and_help( + cx, + UNNECESSARY_SAFETY_COMMENT, + tail.span, + "expression has unnecessary safety comment", + Some(help_span), + "consider removing the safety comment", + ); + } + } + + fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &hir::Stmt<'tcx>) { + let expr = match stmt.kind { + hir::StmtKind::Local(&hir::Local { init: Some(expr), .. }) + | hir::StmtKind::Expr(expr) + | hir::StmtKind::Semi(expr) => expr, + _ => return, + }; + if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, stmt.hir_id) + && !in_external_macro(cx.tcx.sess, stmt.span) + && let HasSafetyComment::Yes(pos) = stmt_has_safety_comment(cx, stmt.span, stmt.hir_id) + && let Some(help_span) = expr_has_unnecessary_safety_comment(cx, expr, pos) + { + span_lint_and_help( + cx, + UNNECESSARY_SAFETY_COMMENT, + stmt.span, + "statement has unnecessary safety comment", + Some(help_span), + "consider removing the safety comment", + ); + } } fn check_item(&mut self, cx: &LateContext<'_>, item: &hir::Item<'_>) { @@ -216,6 +259,36 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks { } } +fn expr_has_unnecessary_safety_comment<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx hir::Expr<'tcx>, + comment_pos: BytePos, +) -> Option<Span> { + // this should roughly be the reverse of `block_parents_have_safety_comment` + if for_each_expr_with_closures(cx, expr, |expr| match expr.kind { + hir::ExprKind::Block( + Block { + rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided), + .. + }, + _, + ) => ControlFlow::Break(()), + // statements will be handled by check_stmt itself again + hir::ExprKind::Block(..) => ControlFlow::Continue(Descend::No), + _ => ControlFlow::Continue(Descend::Yes), + }) + .is_some() + { + return None; + } + + let source_map = cx.tcx.sess.source_map(); + let span = Span::new(comment_pos, comment_pos, SyntaxContext::root(), None); + let help_span = source_map.span_extend_to_next_char(span, '\n', true); + + Some(help_span) +} + 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(span.lo()); @@ -358,6 +431,55 @@ fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> HasSaf } } +/// Checks if the lines immediately preceding the item contain a safety comment. +#[allow(clippy::collapsible_match)] +fn stmt_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 span.ctxt() == SyntaxContext::root() { + if let Some(parent_node) = get_parent_node(cx.tcx, hir_id) { + let comment_start = match parent_node { + Node::Block(block) => walk_span_to_context(block.span, SyntaxContext::root()).map(Span::lo), + _ => return HasSafetyComment::Maybe, + }; + + let source_map = cx.sess().source_map(); + if let Some(comment_start) = comment_start + && let Ok(unsafe_line) = source_map.lookup_line(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() + { + unsafe_line.sf.lines(|lines| { + if comment_start_line.line >= unsafe_line.line { + HasSafetyComment::No + } else { + match text_has_safety_comment( + src, + &lines[comment_start_line.line + 1..=unsafe_line.line], + unsafe_line.sf.start_pos.to_usize(), + ) { + Some(b) => HasSafetyComment::Yes(b), + None => HasSafetyComment::No, + } + } + }) + } else { + // Problem getting source text. Pretend a comment was found. + HasSafetyComment::Maybe + } + } else { + // No parent node. Pretend a comment was found. + HasSafetyComment::Maybe + } + } else { + HasSafetyComment::No + } +} + fn comment_start_before_item_in_mod( cx: &LateContext<'_>, parent_mod: &hir::Mod<'_>, diff --git a/clippy_utils/src/visitors.rs b/clippy_utils/src/visitors.rs index d4294f18fd5..863fb60fcfc 100644 --- a/clippy_utils/src/visitors.rs +++ b/clippy_utils/src/visitors.rs @@ -170,22 +170,22 @@ where cb: F, } - struct WithStmtGuarg<'a, F> { + struct WithStmtGuard<'a, F> { val: &'a mut RetFinder<F>, prev_in_stmt: bool, } impl<F> RetFinder<F> { - fn inside_stmt(&mut self, in_stmt: bool) -> WithStmtGuarg<'_, F> { + fn inside_stmt(&mut self, in_stmt: bool) -> WithStmtGuard<'_, F> { let prev_in_stmt = std::mem::replace(&mut self.in_stmt, in_stmt); - WithStmtGuarg { + WithStmtGuard { val: self, prev_in_stmt, } } } - impl<F> std::ops::Deref for WithStmtGuarg<'_, F> { + impl<F> std::ops::Deref for WithStmtGuard<'_, F> { type Target = RetFinder<F>; fn deref(&self) -> &Self::Target { @@ -193,13 +193,13 @@ where } } - impl<F> std::ops::DerefMut for WithStmtGuarg<'_, F> { + impl<F> std::ops::DerefMut for WithStmtGuard<'_, F> { fn deref_mut(&mut self) -> &mut Self::Target { self.val } } - impl<F> Drop for WithStmtGuarg<'_, F> { + impl<F> Drop for WithStmtGuard<'_, F> { fn drop(&mut self) { self.val.in_stmt = self.prev_in_stmt; } diff --git a/tests/ui/undocumented_unsafe_blocks.rs b/tests/ui/undocumented_unsafe_blocks.rs index f68e0b4915e..cb99ce0d421 100644 --- a/tests/ui/undocumented_unsafe_blocks.rs +++ b/tests/ui/undocumented_unsafe_blocks.rs @@ -522,4 +522,35 @@ fn issue_9142() { }; } +mod unnecessary_from_macro { + trait T {} + + macro_rules! no_safety_comment { + ($t:ty) => { + impl T for $t {} + }; + } + + // FIXME: This is not caught + // Safety: unnecessary + no_safety_comment!(()); + + macro_rules! with_safety_comment { + ($t:ty) => { + // Safety: unnecessary + impl T for $t {} + }; + } + + with_safety_comment!(i32); +} + +fn unnecessary_on_stmt_and_expr() -> u32 { + // SAFETY: unnecessary + let num = 42; + + // SAFETY: unnecessary + 24 +} + fn main() {} diff --git a/tests/ui/undocumented_unsafe_blocks.stderr b/tests/ui/undocumented_unsafe_blocks.stderr index becad4f61a9..919fd51351c 100644 --- a/tests/ui/undocumented_unsafe_blocks.stderr +++ b/tests/ui/undocumented_unsafe_blocks.stderr @@ -344,6 +344,24 @@ LL | unsafe {}; | = help: consider adding a safety comment on the preceding line +error: statement has unnecessary safety comment + --> $DIR/undocumented_unsafe_blocks.rs:514:5 + | +LL | / let _ = { +LL | | if unsafe { true } { +LL | | todo!(); +LL | | } else { +... | +LL | | } +LL | | }; + | |______^ + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:513:5 + | +LL | // SAFETY: this is more than one level away, so it should warn + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + error: unsafe block missing a safety comment --> $DIR/undocumented_unsafe_blocks.rs:515:12 | @@ -360,5 +378,45 @@ LL | let bar = unsafe {}; | = help: consider adding a safety comment on the preceding line -error: aborting due to 40 previous errors +error: impl has unnecessary safety comment + --> $DIR/undocumented_unsafe_blocks.rs:541:13 + | +LL | impl T for $t {} + | ^^^^^^^^^^^^^^^^ +... +LL | with_safety_comment!(i32); + | ------------------------- in this macro invocation + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:540:13 + | +LL | // Safety: unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `with_safety_comment` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: expression has unnecessary safety comment + --> $DIR/undocumented_unsafe_blocks.rs:553:5 + | +LL | 24 + | ^^ + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:552:5 + | +LL | // SAFETY: unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: statement has unnecessary safety comment + --> $DIR/undocumented_unsafe_blocks.rs:550:5 + | +LL | let num = 42; + | ^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:549:5 + | +LL | // SAFETY: unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 44 previous errors |