unnecessary_struct_initialization: extend to assignments moving all fields
This lint makes Clippy warn about situations where an owned struct is essentially recreated by moving all its fields into a new instance of the struct. Until now this lint only triggered for structs recreated from a base struct. NB: The new functionality too will cause false positives for the situation where a non-copy struct consisting of all copy members is touched again in subsequent code.
This commit is contained in:
Philipp Gesang
parent
87f8a5b374
commit
296dcf09fb
@ -2,14 +2,15 @@
|
||||
use clippy_utils::source::snippet;
|
||||
use clippy_utils::ty::is_copy;
|
||||
use clippy_utils::{get_parent_expr, path_to_local};
|
||||
use rustc_hir::{BindingMode, Expr, ExprKind, Node, PatKind, UnOp};
|
||||
use rustc_hir::{BindingMode, Expr, ExprField, ExprKind, Node, PatKind, Path, QPath, UnOp};
|
||||
use rustc_lint::{LateContext, LateLintPass};
|
||||
use rustc_session::declare_lint_pass;
|
||||
|
||||
declare_clippy_lint! {
|
||||
/// ### What it does
|
||||
/// Checks for initialization of a `struct` by copying a base without setting
|
||||
/// any field.
|
||||
/// Checks for initialization of an identical `struct` from another instance
|
||||
/// of the type, either by copying a base without setting any field or by
|
||||
/// moving all fields individually.
|
||||
///
|
||||
/// ### Why is this bad?
|
||||
/// Readability suffers from unnecessary struct building.
|
||||
@ -29,9 +30,14 @@
|
||||
/// let b = a;
|
||||
/// ```
|
||||
///
|
||||
/// The struct literal ``S { ..a }`` in the assignment to ``b`` could be replaced
|
||||
/// with just ``a``.
|
||||
///
|
||||
/// ### Known Problems
|
||||
/// Has false positives when the base is a place expression that cannot be
|
||||
/// moved out of, see [#10547](https://github.com/rust-lang/rust-clippy/issues/10547).
|
||||
///
|
||||
/// Empty structs are ignored by the lint.
|
||||
#[clippy::version = "1.70.0"]
|
||||
pub UNNECESSARY_STRUCT_INITIALIZATION,
|
||||
nursery,
|
||||
@ -41,42 +47,111 @@
|
||||
|
||||
impl LateLintPass<'_> for UnnecessaryStruct {
|
||||
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
|
||||
if let ExprKind::Struct(_, &[], Some(base)) = expr.kind {
|
||||
if let Some(parent) = get_parent_expr(cx, expr)
|
||||
&& let parent_ty = cx.typeck_results().expr_ty_adjusted(parent)
|
||||
&& parent_ty.is_any_ptr()
|
||||
{
|
||||
if is_copy(cx, cx.typeck_results().expr_ty(expr)) && path_to_local(base).is_some() {
|
||||
// When the type implements `Copy`, a reference to the new struct works on the
|
||||
// copy. Using the original would borrow it.
|
||||
return;
|
||||
}
|
||||
let ExprKind::Struct(_, fields, base) = expr.kind else {
|
||||
return;
|
||||
};
|
||||
|
||||
if parent_ty.is_mutable_ptr() && !is_mutable(cx, base) {
|
||||
// The original can be used in a mutable reference context only if it is mutable.
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
||||
// TODO: do not propose to replace *XX if XX is not Copy
|
||||
if let ExprKind::Unary(UnOp::Deref, target) = base.kind
|
||||
&& matches!(target.kind, ExprKind::Path(..))
|
||||
&& !is_copy(cx, cx.typeck_results().expr_ty(expr))
|
||||
{
|
||||
// `*base` cannot be used instead of the struct in the general case if it is not Copy.
|
||||
return;
|
||||
}
|
||||
|
||||
span_lint_and_sugg(
|
||||
cx,
|
||||
UNNECESSARY_STRUCT_INITIALIZATION,
|
||||
expr.span,
|
||||
"unnecessary struct building",
|
||||
"replace with",
|
||||
snippet(cx, base.span, "..").into_owned(),
|
||||
rustc_errors::Applicability::MachineApplicable,
|
||||
);
|
||||
if expr.span.from_expansion() {
|
||||
// Prevent lint from hitting inside macro code
|
||||
return;
|
||||
}
|
||||
|
||||
let field_path = same_path_in_all_fields(cx, expr, fields);
|
||||
|
||||
let sugg = match (field_path, base) {
|
||||
(Some(&path), None) => {
|
||||
// all fields match, no base given
|
||||
path.span
|
||||
},
|
||||
(Some(path), Some(base)) if base_is_suitable(cx, expr, base) && path_matches_base(path, base) => {
|
||||
// all fields match, has base: ensure that the path of the base matches
|
||||
base.span
|
||||
},
|
||||
(None, Some(base)) if fields.is_empty() && base_is_suitable(cx, expr, base) => {
|
||||
// just the base, no explicit fields
|
||||
base.span
|
||||
},
|
||||
_ => return,
|
||||
};
|
||||
|
||||
span_lint_and_sugg(
|
||||
cx,
|
||||
UNNECESSARY_STRUCT_INITIALIZATION,
|
||||
expr.span,
|
||||
"unnecessary struct building",
|
||||
"replace with",
|
||||
snippet(cx, sugg, "..").into_owned(),
|
||||
rustc_errors::Applicability::MachineApplicable,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
fn base_is_suitable(cx: &LateContext<'_>, expr: &Expr<'_>, base: &Expr<'_>) -> bool {
|
||||
if !check_references(cx, expr, base) {
|
||||
return false;
|
||||
}
|
||||
|
||||
// TODO: do not propose to replace *XX if XX is not Copy
|
||||
if let ExprKind::Unary(UnOp::Deref, target) = base.kind
|
||||
&& matches!(target.kind, ExprKind::Path(..))
|
||||
&& !is_copy(cx, cx.typeck_results().expr_ty(expr))
|
||||
{
|
||||
// `*base` cannot be used instead of the struct in the general case if it is not Copy.
|
||||
return false;
|
||||
}
|
||||
true
|
||||
}
|
||||
|
||||
/// Check whether all fields of a struct assignment match.
|
||||
/// Returns a [Path] item that one can obtain a span from for the lint suggestion.
|
||||
///
|
||||
/// Conditions that must be satisfied to trigger this variant of the lint:
|
||||
///
|
||||
/// - source struct of the assignment must be of same type as the destination
|
||||
/// - names of destination struct fields must match the field names of the source
|
||||
///
|
||||
/// We don’t check here if all struct fields are assigned as the remainder may
|
||||
/// be filled in from a base struct.
|
||||
fn same_path_in_all_fields<'tcx>(
|
||||
cx: &LateContext<'_>,
|
||||
expr: &Expr<'_>,
|
||||
fields: &[ExprField<'tcx>],
|
||||
) -> Option<&'tcx Path<'tcx>> {
|
||||
let ty = cx.typeck_results().expr_ty(expr);
|
||||
|
||||
let mut found = None;
|
||||
|
||||
for f in fields {
|
||||
// fields are assigned from expression
|
||||
if let ExprKind::Field(src_expr, ident) = f.expr.kind
|
||||
// expression type matches
|
||||
&& ty == cx.typeck_results().expr_ty(src_expr)
|
||||
// field name matches
|
||||
&& f.ident == ident
|
||||
// assigned from a path expression
|
||||
&& let ExprKind::Path(QPath::Resolved(None, src_path)) = src_expr.kind
|
||||
{
|
||||
let Some((_, p)) = found else {
|
||||
// this is the first field assignment in the list
|
||||
found = Some((src_expr, src_path));
|
||||
continue;
|
||||
};
|
||||
|
||||
if p.res == src_path.res {
|
||||
// subsequent field assignment with same origin struct as before
|
||||
continue;
|
||||
}
|
||||
}
|
||||
// source of field assignment doesn’t qualify
|
||||
return None;
|
||||
}
|
||||
|
||||
if let Some((src_expr, src_path)) = found
|
||||
&& check_references(cx, expr, src_expr)
|
||||
{
|
||||
Some(src_path)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
@ -89,3 +164,43 @@ fn is_mutable(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
|
||||
true
|
||||
}
|
||||
}
|
||||
|
||||
fn check_references(cx: &LateContext<'_>, expr_a: &Expr<'_>, expr_b: &Expr<'_>) -> bool {
|
||||
if let Some(parent) = get_parent_expr(cx, expr_a)
|
||||
&& let parent_ty = cx.typeck_results().expr_ty_adjusted(parent)
|
||||
&& parent_ty.is_any_ptr()
|
||||
{
|
||||
if is_copy(cx, cx.typeck_results().expr_ty(expr_a)) && path_to_local(expr_b).is_some() {
|
||||
// When the type implements `Copy`, a reference to the new struct works on the
|
||||
// copy. Using the original would borrow it.
|
||||
return false;
|
||||
}
|
||||
|
||||
if parent_ty.is_mutable_ptr() && !is_mutable(cx, expr_b) {
|
||||
// The original can be used in a mutable reference context only if it is mutable.
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
true
|
||||
}
|
||||
|
||||
/// When some fields are assigned from a base struct and others individually
|
||||
/// the lint applies only if the source of the field is the same as the base.
|
||||
/// This is enforced here by comparing the path of the base expression;
|
||||
/// needless to say the lint only applies if it (or whatever expression it is
|
||||
/// a reference of) actually has a path.
|
||||
fn path_matches_base(path: &Path<'_>, base: &Expr<'_>) -> bool {
|
||||
let base_path = match base.kind {
|
||||
ExprKind::Unary(UnOp::Deref, base_expr) => {
|
||||
if let ExprKind::Path(QPath::Resolved(_, base_path)) = base_expr.kind {
|
||||
base_path
|
||||
} else {
|
||||
return false;
|
||||
}
|
||||
},
|
||||
ExprKind::Path(QPath::Resolved(_, base_path)) => base_path,
|
||||
_ => return false,
|
||||
};
|
||||
path.res == base_path.res
|
||||
}
|
||||
|
||||
@ -26,6 +26,11 @@ struct V {
|
||||
f: u32,
|
||||
}
|
||||
|
||||
struct W {
|
||||
f1: u32,
|
||||
f2: u32,
|
||||
}
|
||||
|
||||
impl Clone for V {
|
||||
fn clone(&self) -> Self {
|
||||
// Lint: `Self` implements `Copy`
|
||||
@ -68,4 +73,69 @@ fn main() {
|
||||
|
||||
// Should lint: the result of an expression is mutable and temporary
|
||||
let p = &mut *Box::new(T { f: 5 });
|
||||
|
||||
// Should lint: all fields of `q` would be consumed anyway
|
||||
let q = W { f1: 42, f2: 1337 };
|
||||
let r = q;
|
||||
|
||||
// Should not lint: not all fields of `t` from same source
|
||||
let s = W { f1: 1337, f2: 42 };
|
||||
let t = W { f1: s.f1, f2: r.f2 };
|
||||
|
||||
// Should not lint: different fields of `s` assigned
|
||||
let u = W { f1: s.f2, f2: s.f1 };
|
||||
|
||||
// Should lint: all fields of `v` would be consumed anyway
|
||||
let v = W { f1: 42, f2: 1337 };
|
||||
let w = v;
|
||||
|
||||
// Should not lint: source differs between fields and base
|
||||
let x = W { f1: 42, f2: 1337 };
|
||||
let y = W { f1: w.f1, ..x };
|
||||
|
||||
// Should lint: range desugars to struct
|
||||
let r1 = 0..5;
|
||||
let r2 = r1;
|
||||
|
||||
references();
|
||||
shorthand();
|
||||
}
|
||||
|
||||
fn references() {
|
||||
// Should not lint as `a` is not mutable
|
||||
let a = W { f1: 42, f2: 1337 };
|
||||
let b = &mut W { f1: a.f1, f2: a.f2 };
|
||||
|
||||
// Should lint as `d` is a shared reference
|
||||
let c = W { f1: 42, f2: 1337 };
|
||||
let d = &c;
|
||||
|
||||
// Should not lint as `e` is not mutable
|
||||
let e = W { f1: 42, f2: 1337 };
|
||||
let f = &mut W { f1: e.f1, ..e };
|
||||
|
||||
// Should lint as `h` is a shared reference
|
||||
let g = W { f1: 42, f2: 1337 };
|
||||
let h = &g;
|
||||
|
||||
// Should not lint as `j` is copy
|
||||
let i = V { f: 0x1701d };
|
||||
let j = &V { ..i };
|
||||
|
||||
// Should not lint as `k` is copy
|
||||
let k = V { f: 0x1701d };
|
||||
let l = &V { f: k.f };
|
||||
}
|
||||
|
||||
fn shorthand() {
|
||||
struct S1 {
|
||||
a: i32,
|
||||
b: i32,
|
||||
}
|
||||
|
||||
let a = 42;
|
||||
let s = S1 { a: 3, b: 4 };
|
||||
|
||||
// Should not lint: `a` is not from `s`
|
||||
let s = S1 { a, b: s.b };
|
||||
}
|
||||
|
||||
@ -26,6 +26,11 @@ struct V {
|
||||
f: u32,
|
||||
}
|
||||
|
||||
struct W {
|
||||
f1: u32,
|
||||
f2: u32,
|
||||
}
|
||||
|
||||
impl Clone for V {
|
||||
fn clone(&self) -> Self {
|
||||
// Lint: `Self` implements `Copy`
|
||||
@ -72,4 +77,69 @@ fn main() {
|
||||
let p = &mut T {
|
||||
..*Box::new(T { f: 5 })
|
||||
};
|
||||
|
||||
// Should lint: all fields of `q` would be consumed anyway
|
||||
let q = W { f1: 42, f2: 1337 };
|
||||
let r = W { f1: q.f1, f2: q.f2 };
|
||||
|
||||
// Should not lint: not all fields of `t` from same source
|
||||
let s = W { f1: 1337, f2: 42 };
|
||||
let t = W { f1: s.f1, f2: r.f2 };
|
||||
|
||||
// Should not lint: different fields of `s` assigned
|
||||
let u = W { f1: s.f2, f2: s.f1 };
|
||||
|
||||
// Should lint: all fields of `v` would be consumed anyway
|
||||
let v = W { f1: 42, f2: 1337 };
|
||||
let w = W { f1: v.f1, ..v };
|
||||
|
||||
// Should not lint: source differs between fields and base
|
||||
let x = W { f1: 42, f2: 1337 };
|
||||
let y = W { f1: w.f1, ..x };
|
||||
|
||||
// Should lint: range desugars to struct
|
||||
let r1 = 0..5;
|
||||
let r2 = r1.start..r1.end;
|
||||
|
||||
references();
|
||||
shorthand();
|
||||
}
|
||||
|
||||
fn references() {
|
||||
// Should not lint as `a` is not mutable
|
||||
let a = W { f1: 42, f2: 1337 };
|
||||
let b = &mut W { f1: a.f1, f2: a.f2 };
|
||||
|
||||
// Should lint as `d` is a shared reference
|
||||
let c = W { f1: 42, f2: 1337 };
|
||||
let d = &W { f1: c.f1, f2: c.f2 };
|
||||
|
||||
// Should not lint as `e` is not mutable
|
||||
let e = W { f1: 42, f2: 1337 };
|
||||
let f = &mut W { f1: e.f1, ..e };
|
||||
|
||||
// Should lint as `h` is a shared reference
|
||||
let g = W { f1: 42, f2: 1337 };
|
||||
let h = &W { f1: g.f1, ..g };
|
||||
|
||||
// Should not lint as `j` is copy
|
||||
let i = V { f: 0x1701d };
|
||||
let j = &V { ..i };
|
||||
|
||||
// Should not lint as `k` is copy
|
||||
let k = V { f: 0x1701d };
|
||||
let l = &V { f: k.f };
|
||||
}
|
||||
|
||||
fn shorthand() {
|
||||
struct S1 {
|
||||
a: i32,
|
||||
b: i32,
|
||||
}
|
||||
|
||||
let a = 42;
|
||||
let s = S1 { a: 3, b: 4 };
|
||||
|
||||
// Should not lint: `a` is not from `s`
|
||||
let s = S1 { a, b: s.b };
|
||||
}
|
||||
|
||||
@ -1,5 +1,5 @@
|
||||
error: unnecessary struct building
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:32:9
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:37:9
|
||||
|
|
||||
LL | Self { ..*self }
|
||||
| ^^^^^^^^^^^^^^^^ help: replace with: `*self`
|
||||
@ -8,25 +8,25 @@ LL | Self { ..*self }
|
||||
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_struct_initialization)]`
|
||||
|
||||
error: unnecessary struct building
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:39:17
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:44:17
|
||||
|
|
||||
LL | let mut b = S { ..a };
|
||||
| ^^^^^^^^^ help: replace with: `a`
|
||||
|
||||
error: unnecessary struct building
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:42:18
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:47:18
|
||||
|
|
||||
LL | let c = &mut S { ..b };
|
||||
| ^^^^^^^^^ help: replace with: `b`
|
||||
|
||||
error: unnecessary struct building
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:50:14
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:55:14
|
||||
|
|
||||
LL | let g = &S { ..f };
|
||||
| ^^^^^^^^^ help: replace with: `f`
|
||||
|
||||
error: unnecessary struct building
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:53:18
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:58:18
|
||||
|
|
||||
LL | let h = &mut S {
|
||||
| __________________^
|
||||
@ -35,7 +35,7 @@ LL | | };
|
||||
| |_____^ help: replace with: `*Box::new(S { f: String::from("foo") })`
|
||||
|
||||
error: unnecessary struct building
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:72:18
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:77:18
|
||||
|
|
||||
LL | let p = &mut T {
|
||||
| __________________^
|
||||
@ -43,5 +43,35 @@ LL | | ..*Box::new(T { f: 5 })
|
||||
LL | | };
|
||||
| |_____^ help: replace with: `*Box::new(T { f: 5 })`
|
||||
|
||||
error: aborting due to 6 previous errors
|
||||
error: unnecessary struct building
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:83:13
|
||||
|
|
||||
LL | let r = W { f1: q.f1, f2: q.f2 };
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `q`
|
||||
|
||||
error: unnecessary struct building
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:94:13
|
||||
|
|
||||
LL | let w = W { f1: v.f1, ..v };
|
||||
| ^^^^^^^^^^^^^^^^^^^ help: replace with: `v`
|
||||
|
||||
error: unnecessary struct building
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:102:14
|
||||
|
|
||||
LL | let r2 = r1.start..r1.end;
|
||||
| ^^^^^^^^^^^^^^^^ help: replace with: `r1`
|
||||
|
||||
error: unnecessary struct building
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:115:14
|
||||
|
|
||||
LL | let d = &W { f1: c.f1, f2: c.f2 };
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `c`
|
||||
|
||||
error: unnecessary struct building
|
||||
--> tests/ui/unnecessary_struct_initialization.rs:123:14
|
||||
|
|
||||
LL | let h = &W { f1: g.f1, ..g };
|
||||
| ^^^^^^^^^^^^^^^^^^^ help: replace with: `g`
|
||||
|
||||
error: aborting due to 11 previous errors
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user