From 296dcf09fb7e744c8f7b118e2e328dbc1fd6a7a5 Mon Sep 17 00:00:00 2001 From: Philipp Gesang Date: Fri, 19 Apr 2024 09:26:52 +0100 Subject: [PATCH] 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. --- .../src/unnecessary_struct_initialization.rs | 187 ++++++++++++++---- .../unnecessary_struct_initialization.fixed | 70 +++++++ tests/ui/unnecessary_struct_initialization.rs | 70 +++++++ .../unnecessary_struct_initialization.stderr | 44 ++++- 4 files changed, 328 insertions(+), 43 deletions(-) diff --git a/clippy_lints/src/unnecessary_struct_initialization.rs b/clippy_lints/src/unnecessary_struct_initialization.rs index 2da753343448..afdd3505cdd1 100644 --- a/clippy_lints/src/unnecessary_struct_initialization.rs +++ b/clippy_lints/src/unnecessary_struct_initialization.rs @@ -2,14 +2,15 @@ use clippy_utils::diagnostics::span_lint_and_sugg; 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 @@ declare_clippy_lint! { /// 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 @@ declare_lint_pass!(UnnecessaryStruct => [UNNECESSARY_STRUCT_INITIALIZATION]); 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; - } - - 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; - } - } + let ExprKind::Struct(_, fields, base) = expr.kind else { + 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; - } + 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; - 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, - ); + 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 +} diff --git a/tests/ui/unnecessary_struct_initialization.fixed b/tests/ui/unnecessary_struct_initialization.fixed index f3cf65da2d6a..b0e8f4546353 100644 --- a/tests/ui/unnecessary_struct_initialization.fixed +++ b/tests/ui/unnecessary_struct_initialization.fixed @@ -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 }; } diff --git a/tests/ui/unnecessary_struct_initialization.rs b/tests/ui/unnecessary_struct_initialization.rs index bd5302f9d85d..b0db71af4d4c 100644 --- a/tests/ui/unnecessary_struct_initialization.rs +++ b/tests/ui/unnecessary_struct_initialization.rs @@ -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 }; } diff --git a/tests/ui/unnecessary_struct_initialization.stderr b/tests/ui/unnecessary_struct_initialization.stderr index 8bc308c7567b..56982cc0a39f 100644 --- a/tests/ui/unnecessary_struct_initialization.stderr +++ b/tests/ui/unnecessary_struct_initialization.stderr @@ -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