Skip to content

Commit 098ded3

Browse files
profetiaprofetia
committed
fix: replace_box FP when the box is moved
1 parent d05f74e commit 098ded3

File tree

5 files changed

+286
-7
lines changed

5 files changed

+286
-7
lines changed

‎clippy_lints/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -820,6 +820,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
820820
store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny));
821821
store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg));
822822
store.register_late_pass(|_| Box::new(volatile_composites::VolatileComposites));
823-
store.register_late_pass(|_| Box::new(replace_box::ReplaceBox));
823+
store.register_late_pass(|_| Box::new(replace_box::ReplaceBox::default()));
824824
// add lints here, do not remove this comment, it's used in `new_lint`
825825
}

‎clippy_lints/src/replace_box.rs‎

Lines changed: 94 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,17 @@ use clippy_utils::res::{MaybeDef, MaybeResPath};
33
use clippy_utils::sugg::Sugg;
44
use clippy_utils::ty::implements_trait;
55
use clippy_utils::{is_default_equivalent_call, local_is_initialized};
6+
use rustc_data_structures::fx::FxHashSet;
7+
use rustc_data_structures::smallvec::SmallVec;
68
use rustc_errors::Applicability;
7-
use rustc_hir::{Expr, ExprKind, LangItem, QPath};
9+
use rustc_hir::{Body, BodyId, Expr, ExprKind, HirId, LangItem, QPath};
10+
use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
811
use rustc_lint::{LateContext, LateLintPass};
9-
use rustc_session::declare_lint_pass;
10-
use rustc_span::sym;
12+
use rustc_middle::hir::place::ProjectionKind;
13+
use rustc_middle::mir::FakeReadCause;
14+
use rustc_middle::ty;
15+
use rustc_session::impl_lint_pass;
16+
use rustc_span::{Symbol, sym};
1117

1218
declare_clippy_lint! {
1319
/// ### What it does
@@ -33,17 +39,57 @@ declare_clippy_lint! {
3339
perf,
3440
"assigning a newly created box to `Box<T>` is inefficient"
3541
}
36-
declare_lint_pass!(ReplaceBox => [REPLACE_BOX]);
42+
43+
#[derive(Default)]
44+
pub struct ReplaceBox {
45+
consumed_locals: FxHashSet<HirId>,
46+
loaded_bodies: SmallVec<[BodyId; 2]>,
47+
}
48+
49+
impl ReplaceBox {
50+
fn get_consumed_locals(&mut self, cx: &LateContext<'_>) -> &FxHashSet<HirId> {
51+
if let Some(body_id) = cx.enclosing_body
52+
&& !self.loaded_bodies.contains(&body_id)
53+
{
54+
self.loaded_bodies.push(body_id);
55+
ExprUseVisitor::for_clippy(
56+
cx,
57+
cx.tcx.hir_body_owner_def_id(body_id),
58+
MovedVariablesCtxt {
59+
consumed_locals: &mut self.consumed_locals,
60+
},
61+
)
62+
.consume_body(cx.tcx.hir_body(body_id))
63+
.into_ok();
64+
}
65+
66+
&self.consumed_locals
67+
}
68+
}
69+
70+
impl_lint_pass!(ReplaceBox => [REPLACE_BOX]);
3771

3872
impl LateLintPass<'_> for ReplaceBox {
73+
fn check_body_post(&mut self, _: &LateContext<'_>, body: &Body<'_>) {
74+
if self.loaded_bodies.first().is_some_and(|&x| x == body.id()) {
75+
self.consumed_locals.clear();
76+
self.loaded_bodies.clear();
77+
}
78+
}
79+
3980
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) {
4081
if let ExprKind::Assign(lhs, rhs, _) = &expr.kind
4182
&& !lhs.span.from_expansion()
4283
&& !rhs.span.from_expansion()
4384
&& let lhs_ty = cx.typeck_results().expr_ty(lhs)
85+
&& let Some(inner_ty) = lhs_ty.boxed_ty()
4486
// No diagnostic for late-initialized locals
4587
&& lhs.res_local_id().is_none_or(|local| local_is_initialized(cx, local))
46-
&& let Some(inner_ty) = lhs_ty.boxed_ty()
88+
// No diagnostic if this is a local that has been moved, or the field
89+
// of a local that has been moved, or several chained field accesses of a local
90+
&& local_base(lhs).is_none_or(|(base_id, _)| {
91+
!self.get_consumed_locals(cx).contains(&base_id)
92+
})
4793
{
4894
if let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
4995
&& implements_trait(cx, inner_ty, default_trait_id, &[])
@@ -109,3 +155,46 @@ fn get_box_new_payload<'tcx>(cx: &LateContext<'_>, expr: &Expr<'tcx>) -> Option<
109155
None
110156
}
111157
}
158+
159+
struct MovedVariablesCtxt<'a> {
160+
consumed_locals: &'a mut FxHashSet<HirId>,
161+
}
162+
163+
impl<'tcx> Delegate<'tcx> for MovedVariablesCtxt<'_> {
164+
fn consume(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId) {
165+
if let PlaceBase::Local(id) = cmt.place.base
166+
&& let mut projections = cmt
167+
.place
168+
.projections
169+
.iter()
170+
.filter(|x| matches!(x.kind, ProjectionKind::Deref))
171+
// Either no deref or multiple derefs
172+
&& (projections.next().is_none() || projections.next().is_some())
173+
{
174+
self.consumed_locals.insert(id);
175+
}
176+
}
177+
178+
fn use_cloned(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
179+
180+
fn borrow(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {}
181+
182+
fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
183+
184+
fn fake_read(&mut self, _: &PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
185+
}
186+
187+
/// A local place followed by optional fields
188+
type IdFields = (HirId, Vec<Symbol>);
189+
190+
/// If `expr` is a local variable with optional field accesses, return it.
191+
fn local_base(expr: &Expr<'_>) -> Option<IdFields> {
192+
match expr.kind {
193+
ExprKind::Path(qpath) => qpath.res_local_id().map(|id| (id, Vec::new())),
194+
ExprKind::Field(expr, field) => local_base(expr).map(|(id, mut fields)| {
195+
fields.push(field.name);
196+
(id, fields)
197+
}),
198+
_ => None,
199+
}
200+
}

‎tests/ui/replace_box.fixed‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,3 +70,74 @@ fn main() {
7070
let bb: Box<u32>;
7171
bb = Default::default();
7272
}
73+
74+
fn issue15951() {
75+
struct Foo {
76+
inner: String,
77+
}
78+
79+
fn embedded_body() {
80+
let mut x = Box::new(());
81+
let y = x;
82+
x = Box::new(());
83+
84+
let mut x = Box::new(Foo { inner: String::new() });
85+
let y = x.inner;
86+
*x = Foo { inner: String::new() };
87+
//~^ replace_box
88+
}
89+
90+
let mut x = Box::new(Foo { inner: String::new() });
91+
let in_closure = || {
92+
*x = Foo { inner: String::new() };
93+
//~^ replace_box
94+
};
95+
}
96+
97+
static R: fn(&mut Box<String>) = |x| **x = String::new();
98+
//~^ replace_box
99+
100+
fn field() {
101+
struct T {
102+
content: String,
103+
}
104+
105+
impl T {
106+
fn new() -> Self {
107+
Self { content: String::new() }
108+
}
109+
}
110+
111+
struct S {
112+
b: Box<T>,
113+
}
114+
115+
let mut s = S { b: Box::new(T::new()) };
116+
let _b = s.b;
117+
s.b = Box::new(T::new());
118+
119+
// Interestingly, the lint and fix are valid here as `s.b` is not really moved
120+
let mut s = S { b: Box::new(T::new()) };
121+
_ = s.b;
122+
*s.b = T::new();
123+
//~^ replace_box
124+
125+
let mut s = S { b: Box::new(T::new()) };
126+
*s.b = T::new();
127+
//~^ replace_box
128+
129+
struct Q(Box<T>);
130+
let mut q = Q(Box::new(T::new()));
131+
let _b = q.0;
132+
q.0 = Box::new(T::new());
133+
134+
let mut q = Q(Box::new(T::new()));
135+
_ = q.0;
136+
*q.0 = T::new();
137+
//~^ replace_box
138+
139+
// This one is a false negative, but it will need MIR analysis to work properly
140+
let mut x = Box::new(String::new());
141+
x = Box::new(String::new());
142+
x;
143+
}

‎tests/ui/replace_box.rs‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,3 +70,74 @@ fn main() {
7070
let bb: Box<u32>;
7171
bb = Default::default();
7272
}
73+
74+
fn issue15951() {
75+
struct Foo {
76+
inner: String,
77+
}
78+
79+
fn embedded_body() {
80+
let mut x = Box::new(());
81+
let y = x;
82+
x = Box::new(());
83+
84+
let mut x = Box::new(Foo { inner: String::new() });
85+
let y = x.inner;
86+
x = Box::new(Foo { inner: String::new() });
87+
//~^ replace_box
88+
}
89+
90+
let mut x = Box::new(Foo { inner: String::new() });
91+
let in_closure = || {
92+
x = Box::new(Foo { inner: String::new() });
93+
//~^ replace_box
94+
};
95+
}
96+
97+
static R: fn(&mut Box<String>) = |x| *x = Box::new(String::new());
98+
//~^ replace_box
99+
100+
fn field() {
101+
struct T {
102+
content: String,
103+
}
104+
105+
impl T {
106+
fn new() -> Self {
107+
Self { content: String::new() }
108+
}
109+
}
110+
111+
struct S {
112+
b: Box<T>,
113+
}
114+
115+
let mut s = S { b: Box::new(T::new()) };
116+
let _b = s.b;
117+
s.b = Box::new(T::new());
118+
119+
// Interestingly, the lint and fix are valid here as `s.b` is not really moved
120+
let mut s = S { b: Box::new(T::new()) };
121+
_ = s.b;
122+
s.b = Box::new(T::new());
123+
//~^ replace_box
124+
125+
let mut s = S { b: Box::new(T::new()) };
126+
s.b = Box::new(T::new());
127+
//~^ replace_box
128+
129+
struct Q(Box<T>);
130+
let mut q = Q(Box::new(T::new()));
131+
let _b = q.0;
132+
q.0 = Box::new(T::new());
133+
134+
let mut q = Q(Box::new(T::new()));
135+
_ = q.0;
136+
q.0 = Box::new(T::new());
137+
//~^ replace_box
138+
139+
// This one is a false negative, but it will need MIR analysis to work properly
140+
let mut x = Box::new(String::new());
141+
x = Box::new(String::new());
142+
x;
143+
}

‎tests/ui/replace_box.stderr‎

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,5 +48,53 @@ LL | b = Box::new(mac!(three));
4848
|
4949
= note: this creates a needless allocation
5050

51-
error: aborting due to 6 previous errors
51+
error: creating a new box
52+
--> tests/ui/replace_box.rs:86:9
53+
|
54+
LL | x = Box::new(Foo { inner: String::new() });
55+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace existing content with inner value instead: `*x = Foo { inner: String::new() }`
56+
|
57+
= note: this creates a needless allocation
58+
59+
error: creating a new box
60+
--> tests/ui/replace_box.rs:92:9
61+
|
62+
LL | x = Box::new(Foo { inner: String::new() });
63+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace existing content with inner value instead: `*x = Foo { inner: String::new() }`
64+
|
65+
= note: this creates a needless allocation
66+
67+
error: creating a new box
68+
--> tests/ui/replace_box.rs:97:38
69+
|
70+
LL | static R: fn(&mut Box<String>) = |x| *x = Box::new(String::new());
71+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace existing content with inner value instead: `**x = String::new()`
72+
|
73+
= note: this creates a needless allocation
74+
75+
error: creating a new box
76+
--> tests/ui/replace_box.rs:122:5
77+
|
78+
LL | s.b = Box::new(T::new());
79+
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace existing content with inner value instead: `*s.b = T::new()`
80+
|
81+
= note: this creates a needless allocation
82+
83+
error: creating a new box
84+
--> tests/ui/replace_box.rs:126:5
85+
|
86+
LL | s.b = Box::new(T::new());
87+
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace existing content with inner value instead: `*s.b = T::new()`
88+
|
89+
= note: this creates a needless allocation
90+
91+
error: creating a new box
92+
--> tests/ui/replace_box.rs:136:5
93+
|
94+
LL | q.0 = Box::new(T::new());
95+
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace existing content with inner value instead: `*q.0 = T::new()`
96+
|
97+
= note: this creates a needless allocation
98+
99+
error: aborting due to 12 previous errors
52100

0 commit comments

Comments
 (0)