Skip to content

add lint for recreation of an entire struct #12772

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 21, 2024
Merged

add lint for recreation of an entire struct #12772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
187 changes: 151 additions & 36 deletions clippy_lints/src/unnecessary_struct_initialization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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,
Expand All @@ -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
}
}

Expand All @@ -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;
Comment on lines +173 to +176
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is pre-existing code (+ a nursery lint) so we can just keep it as is and you can resolve the comment, but this logic doesn't seem entirely correct I think 🤔.
Only the fields need to be Copy for S { ..base }, not the type itself:

struct S1 { a: i32, b: i32 }

let mut s = S1 { a: 0, b: 0 };
(&mut S1 { ..s }).b = 1; // incorrect lint, changing to `&mut s` as suggested fails the assertion
assert_eq!(s.b, 0);

}

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
}
70 changes: 70 additions & 0 deletions tests/ui/unnecessary_struct_initialization.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down Expand Up @@ -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 };
}
70 changes: 70 additions & 0 deletions tests/ui/unnecessary_struct_initialization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down Expand Up @@ -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 };
}
Loading