Skip to content

Commit 3323442

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

File tree

5 files changed

+264
-7
lines changed

5 files changed

+264
-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: 82 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,15 @@ 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;
67
use rustc_errors::Applicability;
7-
use rustc_hir::{Expr, ExprKind, LangItem, QPath};
8+
use rustc_hir::{Body, Expr, ExprKind, HirId, LangItem, Node, QPath};
9+
use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
810
use rustc_lint::{LateContext, LateLintPass};
9-
use rustc_session::declare_lint_pass;
10-
use rustc_span::sym;
11+
use rustc_middle::mir::FakeReadCause;
12+
use rustc_middle::ty;
13+
use rustc_session::impl_lint_pass;
14+
use rustc_span::{Symbol, sym};
1115

1216
declare_clippy_lint! {
1317
/// ### What it does
@@ -33,17 +37,54 @@ declare_clippy_lint! {
3337
perf,
3438
"assigning a newly created box to `Box<T>` is inefficient"
3539
}
36-
declare_lint_pass!(ReplaceBox => [REPLACE_BOX]);
40+
41+
/// A local place followed by optional fields
42+
type IdFields = (HirId, Vec<Symbol>);
43+
44+
#[derive(Default)]
45+
pub struct ReplaceBox {
46+
// Stack of caches for moved vars. The latest entry is the
47+
// body being currently visited.
48+
moved_vars_caches: Vec<Option<FxHashSet<IdFields>>>,
49+
}
50+
51+
impl ReplaceBox {
52+
fn current_moved_vars_cache(&mut self) -> &mut Option<FxHashSet<IdFields>> {
53+
self.moved_vars_caches.last_mut().unwrap()
54+
}
55+
}
56+
57+
impl_lint_pass!(ReplaceBox => [REPLACE_BOX]);
3758

3859
impl LateLintPass<'_> for ReplaceBox {
60+
fn check_body(&mut self, _: &LateContext<'_>, _: &Body<'_>) {
61+
self.moved_vars_caches.push(None);
62+
}
63+
64+
fn check_body_post(&mut self, _: &LateContext<'_>, _: &Body<'_>) {
65+
_ = self.moved_vars_caches.pop();
66+
}
67+
3968
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) {
4069
if let ExprKind::Assign(lhs, rhs, _) = &expr.kind
4170
&& !lhs.span.from_expansion()
4271
&& !rhs.span.from_expansion()
4372
&& let lhs_ty = cx.typeck_results().expr_ty(lhs)
73+
&& let Some(inner_ty) = lhs_ty.boxed_ty()
4474
// No diagnostic for late-initialized locals
4575
&& lhs.res_local_id().is_none_or(|local| local_is_initialized(cx, local))
46-
&& let Some(inner_ty) = lhs_ty.boxed_ty()
76+
// No diagnostic if this is a local that has been moved, or the field
77+
// of a local that has been moved, or several chained field accesses of a local
78+
&& local_base(lhs).is_none_or(|base_id| {
79+
!self.current_moved_vars_cache().get_or_insert_with(|| {
80+
let body_id = cx.enclosing_body.unwrap();
81+
let mut ctx = MovedVariablesCtxt { cx, moved_vars: FxHashSet::default() };
82+
ExprUseVisitor::for_clippy(cx, cx.tcx.hir_body_owner_def_id(body_id), &mut ctx)
83+
.consume_body(cx.tcx.hir_body(body_id))
84+
.into_ok();
85+
ctx.moved_vars
86+
}).contains(&base_id)
87+
})
4788
{
4889
if let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default)
4990
&& implements_trait(cx, inner_ty, default_trait_id, &[])
@@ -109,3 +150,39 @@ fn get_box_new_payload<'tcx>(cx: &LateContext<'_>, expr: &Expr<'tcx>) -> Option<
109150
None
110151
}
111152
}
153+
154+
struct MovedVariablesCtxt<'a, 'tcx> {
155+
cx: &'a LateContext<'tcx>,
156+
moved_vars: FxHashSet<IdFields>,
157+
}
158+
159+
impl<'tcx> Delegate<'tcx> for MovedVariablesCtxt<'_, 'tcx> {
160+
fn consume(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId) {
161+
if let PlaceBase::Local(_) = cmt.place.base
162+
&& let Node::Expr(expr) = self.cx.tcx.hir_node(cmt.hir_id)
163+
&& let Some(id_fields) = local_base(expr)
164+
{
165+
self.moved_vars.insert(id_fields);
166+
}
167+
}
168+
169+
fn use_cloned(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
170+
171+
fn borrow(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {}
172+
173+
fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
174+
175+
fn fake_read(&mut self, _: &PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
176+
}
177+
178+
/// If `expr` is a local variable with optional field accesses, return it.
179+
fn local_base(expr: &Expr<'_>) -> Option<IdFields> {
180+
match expr.kind {
181+
ExprKind::Path(qpath) => qpath.res_local_id().map(|id| (id, Vec::new())),
182+
ExprKind::Field(expr, field) => local_base(expr).map(|(id, mut fields)| {
183+
fields.push(field.name);
184+
(id, fields)
185+
}),
186+
_ => None,
187+
}
188+
}

‎tests/ui/replace_box.fixed‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,3 +70,69 @@ 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+
}

‎tests/ui/replace_box.rs‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,3 +70,69 @@ 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+
}

‎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)