fix: replace_box FP when the box is moved
This commit is contained in:
Linshu Yang
parent
d05f74e5f7
commit
098ded3160
5 changed files with 286 additions and 7 deletions
|
|
@ -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`
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue