From 5a7ea1d3bffd3e3f268d3b3875d779cee8b93294 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Wed, 19 Nov 2025 14:34:00 +0100 Subject: [PATCH] Count unsafe operations only towards the innermost unsafe block Also, macro calls are counted as at most one unsafe operation. --- .../src/multiple_unsafe_ops_per_block.rs | 70 +++-- tests/ui/multiple_unsafe_ops_per_block.rs | 133 +++++++++ tests/ui/multiple_unsafe_ops_per_block.stderr | 253 +++++++++++++++--- 3 files changed, 390 insertions(+), 66 deletions(-) diff --git a/clippy_lints/src/multiple_unsafe_ops_per_block.rs b/clippy_lints/src/multiple_unsafe_ops_per_block.rs index 80cf081992cc..42dc9f2f1fa8 100644 --- a/clippy_lints/src/multiple_unsafe_ops_per_block.rs +++ b/clippy_lints/src/multiple_unsafe_ops_per_block.rs @@ -3,13 +3,14 @@ use clippy_utils::diagnostics::span_lint_and_then; use hir::def::{DefKind, Res}; use hir::{BlockCheckMode, ExprKind, QPath, UnOp}; use rustc_ast::{BorrowKind, Mutability}; +use rustc_data_structures::fx::FxHashMap; use rustc_hir as hir; use rustc_hir::intravisit::{Visitor, walk_body, walk_expr}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::nested_filter; use rustc_middle::ty::{self, TyCtxt, TypeckResults}; use rustc_session::declare_lint_pass; -use rustc_span::{DesugaringKind, Span}; +use rustc_span::Span; declare_clippy_lint! { /// ### What it does @@ -56,12 +57,16 @@ declare_clippy_lint! { /// } /// ``` /// - /// ### Note + /// ### Notes /// - /// Taking a raw pointer to a union field is always safe and will - /// not be considered unsafe by this lint, even when linting code written - /// with a specified Rust version of 1.91 or earlier (which required - /// using an `unsafe` block). + /// - Unsafe operations only count towards the total for the innermost + /// enclosing `unsafe` block. + /// - Each call to a macro expanding to unsafe operations count for one + /// unsafe operation. + /// - Taking a raw pointer to a union field is always safe and will + /// not be considered unsafe by this lint, even when linting code written + /// with a specified Rust version of 1.91 or earlier (which required + /// using an `unsafe` block). #[clippy::version = "1.69.0"] pub MULTIPLE_UNSAFE_OPS_PER_BLOCK, restriction, @@ -71,10 +76,7 @@ declare_lint_pass!(MultipleUnsafeOpsPerBlock => [MULTIPLE_UNSAFE_OPS_PER_BLOCK]) impl<'tcx> LateLintPass<'tcx> for MultipleUnsafeOpsPerBlock { fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) { - if !matches!(block.rules, BlockCheckMode::UnsafeBlock(_)) - || block.span.in_external_macro(cx.tcx.sess.source_map()) - || block.span.is_desugaring(DesugaringKind::Await) - { + if !matches!(block.rules, BlockCheckMode::UnsafeBlock(_)) || block.span.from_expansion() { return; } let unsafe_ops = UnsafeExprCollector::collect_unsafe_exprs(cx, block); @@ -100,7 +102,7 @@ impl<'tcx> LateLintPass<'tcx> for MultipleUnsafeOpsPerBlock { struct UnsafeExprCollector<'tcx> { tcx: TyCtxt<'tcx>, typeck_results: &'tcx TypeckResults<'tcx>, - unsafe_ops: Vec<(&'static str, Span)>, + unsafe_ops: FxHashMap, } impl<'tcx> UnsafeExprCollector<'tcx> { @@ -108,10 +110,33 @@ impl<'tcx> UnsafeExprCollector<'tcx> { let mut collector = Self { tcx: cx.tcx, typeck_results: cx.typeck_results(), - unsafe_ops: vec![], + unsafe_ops: FxHashMap::default(), }; collector.visit_block(block); - collector.unsafe_ops + #[allow( + rustc::potential_query_instability, + reason = "span ordering only needed inside the one expression being walked" + )] + let mut unsafe_ops = collector + .unsafe_ops + .into_iter() + .map(|(span, msg)| (msg, span)) + .collect::>(); + unsafe_ops.sort_unstable(); + unsafe_ops + } +} + +impl UnsafeExprCollector<'_> { + fn insert_span(&mut self, span: Span, message: &'static str) { + if span.from_expansion() { + self.unsafe_ops.insert( + span.source_callsite(), + "this macro call expands into one or more unsafe operations", + ); + } else { + self.unsafe_ops.insert(span, message); + } } } @@ -126,7 +151,10 @@ impl<'tcx> Visitor<'tcx> for UnsafeExprCollector<'tcx> { return self.visit_expr(e); }, - ExprKind::InlineAsm(_) => self.unsafe_ops.push(("inline assembly used here", expr.span)), + // Do not recurse inside an inner `unsafe` block, it will be checked on its own + ExprKind::Block(block, _) if matches!(block.rules, BlockCheckMode::UnsafeBlock(_)) => return, + + ExprKind::InlineAsm(_) => self.insert_span(expr.span, "inline assembly used here"), ExprKind::AddrOf(BorrowKind::Raw, _, mut inner) => { while let ExprKind::Field(prefix, _) = inner.kind @@ -139,7 +167,7 @@ impl<'tcx> Visitor<'tcx> for UnsafeExprCollector<'tcx> { ExprKind::Field(e, _) => { if self.typeck_results.expr_ty(e).is_union() { - self.unsafe_ops.push(("union field access occurs here", expr.span)); + self.insert_span(expr.span, "union field access occurs here"); } }, @@ -157,12 +185,11 @@ impl<'tcx> Visitor<'tcx> for UnsafeExprCollector<'tcx> { .. }, )) => { - self.unsafe_ops - .push(("access of a mutable static occurs here", expr.span)); + self.insert_span(expr.span, "access of a mutable static occurs here"); }, ExprKind::Unary(UnOp::Deref, e) if self.typeck_results.expr_ty(e).is_raw_ptr() => { - self.unsafe_ops.push(("raw pointer dereference occurs here", expr.span)); + self.insert_span(expr.span, "raw pointer dereference occurs here"); }, ExprKind::Call(path_expr, _) => { @@ -172,7 +199,7 @@ impl<'tcx> Visitor<'tcx> for UnsafeExprCollector<'tcx> { _ => None, }; if opt_sig.is_some_and(|sig| sig.safety().is_unsafe()) { - self.unsafe_ops.push(("unsafe function call occurs here", expr.span)); + self.insert_span(expr.span, "unsafe function call occurs here"); } }, @@ -182,7 +209,7 @@ impl<'tcx> Visitor<'tcx> for UnsafeExprCollector<'tcx> { .type_dependent_def_id(expr.hir_id) .map(|def_id| self.tcx.fn_sig(def_id)); if opt_sig.is_some_and(|sig| sig.skip_binder().safety().is_unsafe()) { - self.unsafe_ops.push(("unsafe method call occurs here", expr.span)); + self.insert_span(expr.span, "unsafe method call occurs here"); } }, @@ -203,8 +230,7 @@ impl<'tcx> Visitor<'tcx> for UnsafeExprCollector<'tcx> { } )) ) { - self.unsafe_ops - .push(("modification of a mutable static occurs here", expr.span)); + self.insert_span(expr.span, "modification of a mutable static occurs here"); return self.visit_expr(rhs); } }, diff --git a/tests/ui/multiple_unsafe_ops_per_block.rs b/tests/ui/multiple_unsafe_ops_per_block.rs index c1512ba3e269..0ff881472cbb 100644 --- a/tests/ui/multiple_unsafe_ops_per_block.rs +++ b/tests/ui/multiple_unsafe_ops_per_block.rs @@ -312,4 +312,137 @@ fn check_closures() { } } +fn issue16116() { + unsafe fn foo() -> u32 { + 0 + } + + // Do not lint even though `format!` expansion + // contains unsafe calls. + unsafe { + let _ = format!("{}", foo()); + } + + unsafe { + //~^ multiple_unsafe_ops_per_block + let _ = format!("{}", foo()); + let _ = format!("{}", foo()); + } + + // Do not lint: only one `assert!()` argument is unsafe + unsafe { + assert_eq!(foo(), 0, "{}", 1 + 2); + } + + // Each argument of a macro call may count as an unsafe operation. + unsafe { + //~^ multiple_unsafe_ops_per_block + assert_eq!(foo(), 0, "{}", foo()); // One unsafe operation + } + + macro_rules! twice { + ($e:expr) => {{ + $e; + $e; + }}; + } + + // Do not lint, a repeated argument used twice by a macro counts + // as at most one unsafe operation. + unsafe { + twice!(foo()); + } + + unsafe { + //~^ multiple_unsafe_ops_per_block + twice!(foo()); + twice!(foo()); + } + + unsafe { + //~^ multiple_unsafe_ops_per_block + assert_eq!(foo(), 0, "{}", 1 + 2); + assert_eq!(foo(), 0, "{}", 1 + 2); + } + + macro_rules! unsafe_twice { + ($e:expr) => { + unsafe { + $e; + $e; + } + }; + }; + + // A macro whose expansion contains unsafe blocks will not + // check inside the blocks. + unsafe { + unsafe_twice!(foo()); + } + + macro_rules! double_non_arg_unsafe { + () => {{ + _ = str::from_utf8_unchecked(&[]); + _ = str::from_utf8_unchecked(&[]); + }}; + } + + // Do not lint: each unsafe expression contained in the + // macro expansion will count towards the macro call. + unsafe { + double_non_arg_unsafe!(); + } + + unsafe { + //~^ multiple_unsafe_ops_per_block + double_non_arg_unsafe!(); + double_non_arg_unsafe!(); + } + + // Do not lint: the inner macro call counts as one unsafe op. + unsafe { + assert_eq!(double_non_arg_unsafe!(), ()); + } + + unsafe { + //~^ multiple_unsafe_ops_per_block + assert_eq!(double_non_arg_unsafe!(), ()); + assert_eq!(double_non_arg_unsafe!(), ()); + } + + unsafe { + //~^ multiple_unsafe_ops_per_block + assert_eq!((double_non_arg_unsafe!(), double_non_arg_unsafe!()), ((), ())); + } + + macro_rules! unsafe_with_arg { + ($e:expr) => {{ + _ = str::from_utf8_unchecked(&[]); + $e; + }}; + } + + // A confusing situation: the macro call counts towards two unsafe calls, + // one coming from inside the macro itself, and one coming from its argument. + // The error message may seem a bit strange as both the macro call and its + // argument will be marked as counting as unsafe ops, but a short investigation + // in those rare situations should sort it out easily. + unsafe { + //~^ multiple_unsafe_ops_per_block + unsafe_with_arg!(foo()); + } + + macro_rules! ignore { + ($e: expr) => {}; + } + + // Another surprising case is when the macro argument is not + // used in the expansion, but in this case we won't see the + // unsafe operation at all. + unsafe { + ignore!(foo()); + ignore!(foo()); + } +} + fn main() {} diff --git a/tests/ui/multiple_unsafe_ops_per_block.stderr b/tests/ui/multiple_unsafe_ops_per_block.stderr index 63f7742b734b..185225bd28c8 100644 --- a/tests/ui/multiple_unsafe_ops_per_block.stderr +++ b/tests/ui/multiple_unsafe_ops_per_block.stderr @@ -31,16 +31,16 @@ LL | | *raw_ptr(); LL | | } | |_____^ | -note: union field access occurs here - --> tests/ui/multiple_unsafe_ops_per_block.rs:50:14 - | -LL | drop(u.u); - | ^^^ note: raw pointer dereference occurs here --> tests/ui/multiple_unsafe_ops_per_block.rs:51:9 | LL | *raw_ptr(); | ^^^^^^^^^^ +note: union field access occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:50:14 + | +LL | drop(u.u); + | ^^^ error: this `unsafe` block contains 3 unsafe operations, expected only one --> tests/ui/multiple_unsafe_ops_per_block.rs:56:5 @@ -58,16 +58,16 @@ note: inline assembly used here | LL | asm!("nop"); | ^^^^^^^^^^^ -note: unsafe method call occurs here - --> tests/ui/multiple_unsafe_ops_per_block.rs:59:9 - | -LL | sample.not_very_safe(); - | ^^^^^^^^^^^^^^^^^^^^^^ note: modification of a mutable static occurs here --> tests/ui/multiple_unsafe_ops_per_block.rs:60:9 | LL | STATIC = 0; | ^^^^^^^^^^ +note: unsafe method call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:59:9 + | +LL | sample.not_very_safe(); + | ^^^^^^^^^^^^^^^^^^^^^^ error: this `unsafe` block contains 6 unsafe operations, expected only one --> tests/ui/multiple_unsafe_ops_per_block.rs:66:5 @@ -81,36 +81,36 @@ LL | | asm!("nop"); LL | | } | |_____^ | -note: union field access occurs here - --> tests/ui/multiple_unsafe_ops_per_block.rs:68:14 - | -LL | drop(u.u); - | ^^^ note: access of a mutable static occurs here --> tests/ui/multiple_unsafe_ops_per_block.rs:69:14 | LL | drop(STATIC); | ^^^^^^ -note: unsafe method call occurs here - --> tests/ui/multiple_unsafe_ops_per_block.rs:70:9 - | -LL | sample.not_very_safe(); - | ^^^^^^^^^^^^^^^^^^^^^^ -note: unsafe function call occurs here - --> tests/ui/multiple_unsafe_ops_per_block.rs:71:9 +note: inline assembly used here + --> tests/ui/multiple_unsafe_ops_per_block.rs:73:9 | -LL | not_very_safe(); - | ^^^^^^^^^^^^^^^ +LL | asm!("nop"); + | ^^^^^^^^^^^ note: raw pointer dereference occurs here --> tests/ui/multiple_unsafe_ops_per_block.rs:72:9 | LL | *raw_ptr(); | ^^^^^^^^^^ -note: inline assembly used here - --> tests/ui/multiple_unsafe_ops_per_block.rs:73:9 +note: union field access occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:68:14 | -LL | asm!("nop"); - | ^^^^^^^^^^^ +LL | drop(u.u); + | ^^^ +note: unsafe function call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:71:9 + | +LL | not_very_safe(); + | ^^^^^^^^^^^^^^^ +note: unsafe method call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:70:9 + | +LL | sample.not_very_safe(); + | ^^^^^^^^^^^^^^^^^^^^^^ error: this `unsafe` block contains 2 unsafe operations, expected only one --> tests/ui/multiple_unsafe_ops_per_block.rs:109:5 @@ -139,16 +139,16 @@ error: this `unsafe` block contains 2 unsafe operations, expected only one LL | unsafe { char::from_u32_unchecked(*ptr.cast::()) } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -note: unsafe function call occurs here - --> tests/ui/multiple_unsafe_ops_per_block.rs:118:18 - | -LL | unsafe { char::from_u32_unchecked(*ptr.cast::()) } - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ note: raw pointer dereference occurs here --> tests/ui/multiple_unsafe_ops_per_block.rs:118:43 | LL | unsafe { char::from_u32_unchecked(*ptr.cast::()) } | ^^^^^^^^^^^^^^^^^^ +note: unsafe function call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:118:18 + | +LL | unsafe { char::from_u32_unchecked(*ptr.cast::()) } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this `unsafe` block contains 2 unsafe operations, expected only one --> tests/ui/multiple_unsafe_ops_per_block.rs:139:9 @@ -224,16 +224,16 @@ LL | | foo().await; LL | | } | |_____^ | -note: unsafe function call occurs here - --> tests/ui/multiple_unsafe_ops_per_block.rs:194:9 - | -LL | not_very_safe(); - | ^^^^^^^^^^^^^^^ note: modification of a mutable static occurs here --> tests/ui/multiple_unsafe_ops_per_block.rs:195:9 | LL | STATIC += 1; | ^^^^^^^^^^^ +note: unsafe function call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:194:9 + | +LL | not_very_safe(); + | ^^^^^^^^^^^^^^^ error: this `unsafe` block contains 2 unsafe operations, expected only one --> tests/ui/multiple_unsafe_ops_per_block.rs:207:5 @@ -265,16 +265,16 @@ LL | | Some(foo_unchecked()).unwrap_unchecked().await; LL | | } | |_____^ | -note: unsafe method call occurs here - --> tests/ui/multiple_unsafe_ops_per_block.rs:216:9 - | -LL | Some(foo_unchecked()).unwrap_unchecked().await; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ note: unsafe function call occurs here --> tests/ui/multiple_unsafe_ops_per_block.rs:216:14 | LL | Some(foo_unchecked()).unwrap_unchecked().await; | ^^^^^^^^^^^^^^^ +note: unsafe method call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:216:9 + | +LL | Some(foo_unchecked()).unwrap_unchecked().await; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this `unsafe` block contains 2 unsafe operations, expected only one --> tests/ui/multiple_unsafe_ops_per_block.rs:236:5 @@ -359,5 +359,170 @@ note: unsafe function call occurs here LL | apply(|| f(0)); | ^^^^ -error: aborting due to 16 previous errors +error: this `unsafe` block contains 2 unsafe operations, expected only one + --> tests/ui/multiple_unsafe_ops_per_block.rs:326:5 + | +LL | / unsafe { +LL | | +LL | | let _ = format!("{}", foo()); +LL | | let _ = format!("{}", foo()); +LL | | } + | |_____^ + | +note: unsafe function call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:328:31 + | +LL | let _ = format!("{}", foo()); + | ^^^^^ +note: unsafe function call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:329:31 + | +LL | let _ = format!("{}", foo()); + | ^^^^^ + +error: this `unsafe` block contains 2 unsafe operations, expected only one + --> tests/ui/multiple_unsafe_ops_per_block.rs:338:5 + | +LL | / unsafe { +LL | | +LL | | assert_eq!(foo(), 0, "{}", foo()); // One unsafe operation +LL | | } + | |_____^ + | +note: unsafe function call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:340:20 + | +LL | assert_eq!(foo(), 0, "{}", foo()); // One unsafe operation + | ^^^^^ +note: unsafe function call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:340:36 + | +LL | assert_eq!(foo(), 0, "{}", foo()); // One unsafe operation + | ^^^^^ + +error: this `unsafe` block contains 2 unsafe operations, expected only one + --> tests/ui/multiple_unsafe_ops_per_block.rs:356:5 + | +LL | / unsafe { +LL | | +LL | | twice!(foo()); +LL | | twice!(foo()); +LL | | } + | |_____^ + | +note: unsafe function call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:358:16 + | +LL | twice!(foo()); + | ^^^^^ +note: unsafe function call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:359:16 + | +LL | twice!(foo()); + | ^^^^^ + +error: this `unsafe` block contains 2 unsafe operations, expected only one + --> tests/ui/multiple_unsafe_ops_per_block.rs:362:5 + | +LL | / unsafe { +LL | | +LL | | assert_eq!(foo(), 0, "{}", 1 + 2); +LL | | assert_eq!(foo(), 0, "{}", 1 + 2); +LL | | } + | |_____^ + | +note: unsafe function call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:364:20 + | +LL | assert_eq!(foo(), 0, "{}", 1 + 2); + | ^^^^^ +note: unsafe function call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:365:20 + | +LL | assert_eq!(foo(), 0, "{}", 1 + 2); + | ^^^^^ + +error: this `unsafe` block contains 2 unsafe operations, expected only one + --> tests/ui/multiple_unsafe_ops_per_block.rs:396:5 + | +LL | / unsafe { +LL | | +LL | | double_non_arg_unsafe!(); +LL | | double_non_arg_unsafe!(); +LL | | } + | |_____^ + | +note: this macro call expands into one or more unsafe operations + --> tests/ui/multiple_unsafe_ops_per_block.rs:398:9 + | +LL | double_non_arg_unsafe!(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ +note: this macro call expands into one or more unsafe operations + --> tests/ui/multiple_unsafe_ops_per_block.rs:399:9 + | +LL | double_non_arg_unsafe!(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this `unsafe` block contains 2 unsafe operations, expected only one + --> tests/ui/multiple_unsafe_ops_per_block.rs:407:5 + | +LL | / unsafe { +LL | | +LL | | assert_eq!(double_non_arg_unsafe!(), ()); +LL | | assert_eq!(double_non_arg_unsafe!(), ()); +LL | | } + | |_____^ + | +note: this macro call expands into one or more unsafe operations + --> tests/ui/multiple_unsafe_ops_per_block.rs:409:20 + | +LL | assert_eq!(double_non_arg_unsafe!(), ()); + | ^^^^^^^^^^^^^^^^^^^^^^^^ +note: this macro call expands into one or more unsafe operations + --> tests/ui/multiple_unsafe_ops_per_block.rs:410:20 + | +LL | assert_eq!(double_non_arg_unsafe!(), ()); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this `unsafe` block contains 2 unsafe operations, expected only one + --> tests/ui/multiple_unsafe_ops_per_block.rs:413:5 + | +LL | / unsafe { +LL | | +LL | | assert_eq!((double_non_arg_unsafe!(), double_non_arg_unsafe!()), ((), ())); +LL | | } + | |_____^ + | +note: this macro call expands into one or more unsafe operations + --> tests/ui/multiple_unsafe_ops_per_block.rs:415:21 + | +LL | assert_eq!((double_non_arg_unsafe!(), double_non_arg_unsafe!()), ((), ())); + | ^^^^^^^^^^^^^^^^^^^^^^^^ +note: this macro call expands into one or more unsafe operations + --> tests/ui/multiple_unsafe_ops_per_block.rs:415:47 + | +LL | assert_eq!((double_non_arg_unsafe!(), double_non_arg_unsafe!()), ((), ())); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this `unsafe` block contains 2 unsafe operations, expected only one + --> tests/ui/multiple_unsafe_ops_per_block.rs:430:5 + | +LL | / unsafe { +LL | | +LL | | unsafe_with_arg!(foo()); +LL | | } + | |_____^ + | +note: this macro call expands into one or more unsafe operations + --> tests/ui/multiple_unsafe_ops_per_block.rs:432:9 + | +LL | unsafe_with_arg!(foo()); + | ^^^^^^^^^^^^^^^^^^^^^^^ +note: unsafe function call occurs here + --> tests/ui/multiple_unsafe_ops_per_block.rs:432:26 + | +LL | unsafe_with_arg!(foo()); + | ^^^^^ + +error: aborting due to 24 previous errors