Fix replace_box FP when the box is moved

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`
 }

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<T>` is inefficient"
 }
-declare_lint_pass!(ReplaceBox => [REPLACE_BOX]);
+
+#[derive(Default)]
+pub struct ReplaceBox {
+    consumed_locals: FxHashSet<HirId>,
+    loaded_bodies: SmallVec<[BodyId; 2]>,
+}
+
+impl ReplaceBox {
+    fn get_consumed_locals(&mut self, cx: &LateContext<'_>) -> &FxHashSet<HirId> {
+        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<HirId>,
+}
+
+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<Symbol>);
+
+/// If `expr` is a local variable with optional field accesses, return it.
+fn local_base(expr: &Expr<'_>) -> Option<IdFields> {
+    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,
+    }
+}

tests/ui/replace_box.fixed

@@ -70,3 +70,74 @@ fn main() {
     let bb: Box<u32>;
     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<String>) = |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<T>,
+    }
+
+    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<T>);
+    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;
+}

tests/ui/replace_box.rs

@@ -70,3 +70,74 @@ fn main() {
     let bb: Box<u32>;
     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<String>) = |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<T>,
+    }
+
+    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<T>);
+    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;
+}

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<String>) = |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