From 098ded31605c193ff35e97ff6c1e03973e21daa3 Mon Sep 17 00:00:00 2001 From: Linshu Yang Date: Sat, 1 Nov 2025 00:26:34 +0000 Subject: [PATCH] fix: `replace_box` FP when the box is moved --- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/replace_box.rs | 99 +++++++++++++++++++++++++++++++-- tests/ui/replace_box.fixed | 71 +++++++++++++++++++++++ tests/ui/replace_box.rs | 71 +++++++++++++++++++++++ tests/ui/replace_box.stderr | 50 ++++++++++++++++- 5 files changed, 286 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8e35883ae9d4..49e6fb07ca77 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -820,6 +820,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny)); store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg)); store.register_late_pass(|_| Box::new(volatile_composites::VolatileComposites)); - store.register_late_pass(|_| Box::new(replace_box::ReplaceBox)); + store.register_late_pass(|_| Box::new(replace_box::ReplaceBox::default())); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/replace_box.rs b/clippy_lints/src/replace_box.rs index 4bbd1803a78d..638f6dc1532b 100644 --- a/clippy_lints/src/replace_box.rs +++ b/clippy_lints/src/replace_box.rs @@ -3,11 +3,17 @@ use clippy_utils::res::{MaybeDef, MaybeResPath}; use clippy_utils::sugg::Sugg; use clippy_utils::ty::implements_trait; use clippy_utils::{is_default_equivalent_call, local_is_initialized}; +use rustc_data_structures::fx::FxHashSet; +use rustc_data_structures::smallvec::SmallVec; use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind, LangItem, QPath}; +use rustc_hir::{Body, BodyId, Expr, ExprKind, HirId, LangItem, QPath}; +use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::declare_lint_pass; -use rustc_span::sym; +use rustc_middle::hir::place::ProjectionKind; +use rustc_middle::mir::FakeReadCause; +use rustc_middle::ty; +use rustc_session::impl_lint_pass; +use rustc_span::{Symbol, sym}; declare_clippy_lint! { /// ### What it does @@ -33,17 +39,57 @@ declare_clippy_lint! { perf, "assigning a newly created box to `Box` is inefficient" } -declare_lint_pass!(ReplaceBox => [REPLACE_BOX]); + +#[derive(Default)] +pub struct ReplaceBox { + consumed_locals: FxHashSet, + loaded_bodies: SmallVec<[BodyId; 2]>, +} + +impl ReplaceBox { + fn get_consumed_locals(&mut self, cx: &LateContext<'_>) -> &FxHashSet { + if let Some(body_id) = cx.enclosing_body + && !self.loaded_bodies.contains(&body_id) + { + self.loaded_bodies.push(body_id); + ExprUseVisitor::for_clippy( + cx, + cx.tcx.hir_body_owner_def_id(body_id), + MovedVariablesCtxt { + consumed_locals: &mut self.consumed_locals, + }, + ) + .consume_body(cx.tcx.hir_body(body_id)) + .into_ok(); + } + + &self.consumed_locals + } +} + +impl_lint_pass!(ReplaceBox => [REPLACE_BOX]); impl LateLintPass<'_> for ReplaceBox { + fn check_body_post(&mut self, _: &LateContext<'_>, body: &Body<'_>) { + if self.loaded_bodies.first().is_some_and(|&x| x == body.id()) { + self.consumed_locals.clear(); + self.loaded_bodies.clear(); + } + } + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) { if let ExprKind::Assign(lhs, rhs, _) = &expr.kind && !lhs.span.from_expansion() && !rhs.span.from_expansion() && let lhs_ty = cx.typeck_results().expr_ty(lhs) + && let Some(inner_ty) = lhs_ty.boxed_ty() // No diagnostic for late-initialized locals && lhs.res_local_id().is_none_or(|local| local_is_initialized(cx, local)) - && let Some(inner_ty) = lhs_ty.boxed_ty() + // No diagnostic if this is a local that has been moved, or the field + // of a local that has been moved, or several chained field accesses of a local + && local_base(lhs).is_none_or(|(base_id, _)| { + !self.get_consumed_locals(cx).contains(&base_id) + }) { if let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default) && implements_trait(cx, inner_ty, default_trait_id, &[]) @@ -109,3 +155,46 @@ fn get_box_new_payload<'tcx>(cx: &LateContext<'_>, expr: &Expr<'tcx>) -> Option< None } } + +struct MovedVariablesCtxt<'a> { + consumed_locals: &'a mut FxHashSet, +} + +impl<'tcx> Delegate<'tcx> for MovedVariablesCtxt<'_> { + fn consume(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId) { + if let PlaceBase::Local(id) = cmt.place.base + && let mut projections = cmt + .place + .projections + .iter() + .filter(|x| matches!(x.kind, ProjectionKind::Deref)) + // Either no deref or multiple derefs + && (projections.next().is_none() || projections.next().is_some()) + { + self.consumed_locals.insert(id); + } + } + + fn use_cloned(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} + + fn borrow(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {} + + fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} + + fn fake_read(&mut self, _: &PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {} +} + +/// A local place followed by optional fields +type IdFields = (HirId, Vec); + +/// If `expr` is a local variable with optional field accesses, return it. +fn local_base(expr: &Expr<'_>) -> Option { + match expr.kind { + ExprKind::Path(qpath) => qpath.res_local_id().map(|id| (id, Vec::new())), + ExprKind::Field(expr, field) => local_base(expr).map(|(id, mut fields)| { + fields.push(field.name); + (id, fields) + }), + _ => None, + } +} diff --git a/tests/ui/replace_box.fixed b/tests/ui/replace_box.fixed index 58c8ed1691d7..e3fc7190a9c9 100644 --- a/tests/ui/replace_box.fixed +++ b/tests/ui/replace_box.fixed @@ -70,3 +70,74 @@ fn main() { let bb: Box; bb = Default::default(); } + +fn issue15951() { + struct Foo { + inner: String, + } + + fn embedded_body() { + let mut x = Box::new(()); + let y = x; + x = Box::new(()); + + let mut x = Box::new(Foo { inner: String::new() }); + let y = x.inner; + *x = Foo { inner: String::new() }; + //~^ replace_box + } + + let mut x = Box::new(Foo { inner: String::new() }); + let in_closure = || { + *x = Foo { inner: String::new() }; + //~^ replace_box + }; +} + +static R: fn(&mut Box) = |x| **x = String::new(); +//~^ replace_box + +fn field() { + struct T { + content: String, + } + + impl T { + fn new() -> Self { + Self { content: String::new() } + } + } + + struct S { + b: Box, + } + + let mut s = S { b: Box::new(T::new()) }; + let _b = s.b; + s.b = Box::new(T::new()); + + // Interestingly, the lint and fix are valid here as `s.b` is not really moved + let mut s = S { b: Box::new(T::new()) }; + _ = s.b; + *s.b = T::new(); + //~^ replace_box + + let mut s = S { b: Box::new(T::new()) }; + *s.b = T::new(); + //~^ replace_box + + struct Q(Box); + let mut q = Q(Box::new(T::new())); + let _b = q.0; + q.0 = Box::new(T::new()); + + let mut q = Q(Box::new(T::new())); + _ = q.0; + *q.0 = T::new(); + //~^ replace_box + + // This one is a false negative, but it will need MIR analysis to work properly + let mut x = Box::new(String::new()); + x = Box::new(String::new()); + x; +} diff --git a/tests/ui/replace_box.rs b/tests/ui/replace_box.rs index e1fb223e4f21..1d5ca1b24994 100644 --- a/tests/ui/replace_box.rs +++ b/tests/ui/replace_box.rs @@ -70,3 +70,74 @@ fn main() { let bb: Box; bb = Default::default(); } + +fn issue15951() { + struct Foo { + inner: String, + } + + fn embedded_body() { + let mut x = Box::new(()); + let y = x; + x = Box::new(()); + + let mut x = Box::new(Foo { inner: String::new() }); + let y = x.inner; + x = Box::new(Foo { inner: String::new() }); + //~^ replace_box + } + + let mut x = Box::new(Foo { inner: String::new() }); + let in_closure = || { + x = Box::new(Foo { inner: String::new() }); + //~^ replace_box + }; +} + +static R: fn(&mut Box) = |x| *x = Box::new(String::new()); +//~^ replace_box + +fn field() { + struct T { + content: String, + } + + impl T { + fn new() -> Self { + Self { content: String::new() } + } + } + + struct S { + b: Box, + } + + let mut s = S { b: Box::new(T::new()) }; + let _b = s.b; + s.b = Box::new(T::new()); + + // Interestingly, the lint and fix are valid here as `s.b` is not really moved + let mut s = S { b: Box::new(T::new()) }; + _ = s.b; + s.b = Box::new(T::new()); + //~^ replace_box + + let mut s = S { b: Box::new(T::new()) }; + s.b = Box::new(T::new()); + //~^ replace_box + + struct Q(Box); + let mut q = Q(Box::new(T::new())); + let _b = q.0; + q.0 = Box::new(T::new()); + + let mut q = Q(Box::new(T::new())); + _ = q.0; + q.0 = Box::new(T::new()); + //~^ replace_box + + // This one is a false negative, but it will need MIR analysis to work properly + let mut x = Box::new(String::new()); + x = Box::new(String::new()); + x; +} diff --git a/tests/ui/replace_box.stderr b/tests/ui/replace_box.stderr index 7d9c85da1731..4b7bd4a0eeae 100644 --- a/tests/ui/replace_box.stderr +++ b/tests/ui/replace_box.stderr @@ -48,5 +48,53 @@ LL | b = Box::new(mac!(three)); | = note: this creates a needless allocation -error: aborting due to 6 previous errors +error: creating a new box + --> tests/ui/replace_box.rs:86:9 + | +LL | x = Box::new(Foo { inner: String::new() }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace existing content with inner value instead: `*x = Foo { inner: String::new() }` + | + = note: this creates a needless allocation + +error: creating a new box + --> tests/ui/replace_box.rs:92:9 + | +LL | x = Box::new(Foo { inner: String::new() }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace existing content with inner value instead: `*x = Foo { inner: String::new() }` + | + = note: this creates a needless allocation + +error: creating a new box + --> tests/ui/replace_box.rs:97:38 + | +LL | static R: fn(&mut Box) = |x| *x = Box::new(String::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace existing content with inner value instead: `**x = String::new()` + | + = note: this creates a needless allocation + +error: creating a new box + --> tests/ui/replace_box.rs:122:5 + | +LL | s.b = Box::new(T::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace existing content with inner value instead: `*s.b = T::new()` + | + = note: this creates a needless allocation + +error: creating a new box + --> tests/ui/replace_box.rs:126:5 + | +LL | s.b = Box::new(T::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace existing content with inner value instead: `*s.b = T::new()` + | + = note: this creates a needless allocation + +error: creating a new box + --> tests/ui/replace_box.rs:136:5 + | +LL | q.0 = Box::new(T::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace existing content with inner value instead: `*q.0 = T::new()` + | + = note: this creates a needless allocation + +error: aborting due to 12 previous errors