Skip to content

Commit

Permalink
add lint for recreation of an entire struct
Browse files Browse the repository at this point in the history
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.

Clippy will suggest to simply use the original struct. The lint
is not machine-applicable because the source struct may have been
partially moved.
  • Loading branch information
phi-gamma committed May 7, 2024
1 parent befb659 commit b8f8654
Show file tree
Hide file tree
Showing 6 changed files with 228 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5690,6 +5690,7 @@ Released 2018-09-13
[`redundant_field_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_field_names
[`redundant_guards`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_guards
[`redundant_locals`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_locals
[`redundant_owned_struct_recreation`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_owned_struct_recreation
[`redundant_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern
[`redundant_pattern_matching`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern_matching
[`redundant_pub_crate`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pub_crate
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::redundant_else::REDUNDANT_ELSE_INFO,
crate::redundant_field_names::REDUNDANT_FIELD_NAMES_INFO,
crate::redundant_locals::REDUNDANT_LOCALS_INFO,
crate::redundant_owned_struct_recreation::REDUNDANT_OWNED_STRUCT_RECREATION_INFO,
crate::redundant_pub_crate::REDUNDANT_PUB_CRATE_INFO,
crate::redundant_slicing::DEREF_BY_SLICING_INFO,
crate::redundant_slicing::REDUNDANT_SLICING_INFO,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,7 @@ mod redundant_closure_call;
mod redundant_else;
mod redundant_field_names;
mod redundant_locals;
mod redundant_owned_struct_recreation;
mod redundant_pub_crate;
mod redundant_slicing;
mod redundant_static_lifetimes;
Expand Down Expand Up @@ -701,6 +702,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
store.register_late_pass(|_| Box::new(default_numeric_fallback::DefaultNumericFallback));
store.register_late_pass(|_| Box::new(inconsistent_struct_constructor::InconsistentStructConstructor));
store.register_late_pass(|_| Box::new(non_octal_unix_permissions::NonOctalUnixPermissions));
store.register_late_pass(|_| Box::new(redundant_owned_struct_recreation::RedundantOwnedStructRecreation));
store.register_early_pass(|| Box::new(unnecessary_self_imports::UnnecessarySelfImports));
store.register_late_pass(move |_| Box::new(approx_const::ApproxConstant::new(msrv())));
let format_args = format_args_storage.clone();
Expand Down
115 changes: 115 additions & 0 deletions clippy_lints/src/redundant_owned_struct_recreation.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
#![allow(unused)]
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::source::snippet;
use rustc_data_structures::fx::FxHashMap;
use rustc_errors::Applicability;

use rustc_hir::{self as hir, ExprKind, Path, PathSegment, QPath};

use rustc_lint::{LateContext, LateLintPass};
use rustc_session::declare_lint_pass;
use std::fmt::{self, Write as _};

declare_clippy_lint! {
/// ### What it does
/// Checks for struct literals that recreate a struct we already have an owned
/// version of.
///
/// ### Why is this bad?
/// This is essentially a no-op as all members are being moved into the new
/// struct and the old one becomes inaccessible.
///
/// ### Example
/// ```no_run
/// fn redundant () {
/// struct Foo { a: i32, b: String }
///
/// let foo = Foo { a: 42, b: "Hello, Foo!".into() };
///
/// let bar = Foo { a: foo.a, b: foo.b };
///
/// println!("redundant: bar = Foo {{ a: {}, b: \"{}\" }}", bar.a, bar.b);
/// }
/// ```
///
/// The struct literal in the assignment to ``bar`` could be replaced with
/// ``let bar = foo``.
///
/// Empty structs are ignored by the lint.
#[clippy::version = "pre 1.29.0"]
pub REDUNDANT_OWNED_STRUCT_RECREATION,
//pedantic,
correctness,
"recreation of owned structs from identical parts"
}

declare_lint_pass!(RedundantOwnedStructRecreation => [REDUNDANT_OWNED_STRUCT_RECREATION]);

impl<'tcx> LateLintPass<'tcx> for RedundantOwnedStructRecreation {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
if !expr.span.from_expansion()
&& let ExprKind::Struct(qpath, fields, base) = expr.kind
{
let ty = cx.typeck_results().expr_ty(expr);

// Conditions that must be satisfied to trigger the lint:
// - source of the assignment must be a struct
// - type of struct in expr must match
// - names of assignee struct fields (lhs) must match the names of the assigned on (rhs)
// - all fields musts be assigned (no Default)

if base.is_some() {
// one or more fields assigned from `..Default`
return;
}

let matching = fields.iter().filter(|f| !f.is_shorthand).try_fold(
None,
|seen_path, f| -> Result<Option<&'tcx Path<'tcx>>, ()> {
// fields are assigned from expression
if let ExprKind::Field(expr, ident) = f.expr.kind
// expression type matches
&& ty == cx.typeck_results().expr_ty(expr)
// field name matches
&& f.ident == ident
&& let ExprKind::Path(QPath::Resolved(None, path)) = expr.kind
{
match seen_path {
None => {
// first field assignment
Ok(Some(path))
},
Some(p) if p.res == path.res => {
// subsequent field assignment, same origin struct as before
Ok(seen_path)
},
Some(_) => {
// subsequent field assignment, different origin struct as before
Err(())
},
}
} else {
// source of field assignment not an expression
Err(())
}
},
);

let Ok(Some(path)) = matching else { return };

let sugg = format!("{}", snippet(cx, path.span, ".."),);

span_lint_and_sugg(
cx,
REDUNDANT_OWNED_STRUCT_RECREATION,
expr.span,
"recreation of owned struct from identical parts",
"try replacing it with",
sugg,
// the lint may be incomplete due to partial moves
// of struct fields
Applicability::MaybeIncorrect,
);
}
}
}
68 changes: 68 additions & 0 deletions tests/ui/redundant_owned_struct_recreation.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
//@no-rustfix
#![warn(clippy::redundant_owned_struct_recreation)]
#![allow(dead_code, unused_variables)]

fn recreate() {
struct Recreated<'s> {
a: i32,
b: &'s str,
}

fn recreate_immediate(r: Recreated) -> Recreated {
Recreated { a: r.a, b: r.b }
}

fn recreate_through_binding(r: Recreated) -> Recreated {
let b = r.b;
Recreated { a: r.a, b }
}

let b = String::from("Answer");
let r = recreate_immediate(Recreated { a: 42, b: &b });
let r = recreate_through_binding(Recreated { a: 42, b: &b });
}

struct One;
struct Two;
struct Three {
one: One,
two: Two,
}

fn recreate_immediate(xy: Three) -> Three {
Three {
one: xy.one,
two: xy.two,
}
}

fn recreate_through_binding(xy: Three) -> Three {
let two = xy.two;
// Here ``xy`` is subject to a partial move so we cannot
// emit a machine applicable fixup suggestion.
Three { one: xy.one, two }
}

impl Three {
fn recreate_through_member(self) -> Self {
Self {
one: self.one,
two: self.two,
}
}
}

fn ignore_struct_with_default() {
#[derive(Default)]
struct WithDefault {
a: usize,
b: u8,
}

let one = WithDefault { a: 1337usize, b: 42u8 };
// This *must not* trigger the lint.
let two = WithDefault {
a: one.a,
..Default::default()
};
}
41 changes: 41 additions & 0 deletions tests/ui/redundant_owned_struct_recreation.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
error: recreation of owned struct from identical parts
--> tests/ui/redundant_owned_struct_recreation.rs:12:9
|
LL | Recreated { a: r.a, b: r.b }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try replacing it with: `r`
|
= note: `-D clippy::redundant-owned-struct-recreation` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::redundant_owned_struct_recreation)]`

error: recreation of owned struct from identical parts
--> tests/ui/redundant_owned_struct_recreation.rs:17:9
|
LL | Recreated { a: r.a, b }
| ^^^^^^^^^^^^^^^^^^^^^^^ help: try replacing it with: `r`

error: recreation of owned struct from identical parts
--> tests/ui/redundant_owned_struct_recreation.rs:33:5
|
LL | / Three {
LL | | one: xy.one,
LL | | two: xy.two,
LL | | }
| |_____^ help: try replacing it with: `xy`

error: recreation of owned struct from identical parts
--> tests/ui/redundant_owned_struct_recreation.rs:43:5
|
LL | Three { one: xy.one, two }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try replacing it with: `xy`

error: recreation of owned struct from identical parts
--> tests/ui/redundant_owned_struct_recreation.rs:48:9
|
LL | / Self {
LL | | one: self.one,
LL | | two: self.two,
LL | | }
| |_________^ help: try replacing it with: `self`

error: aborting due to 5 previous errors

0 comments on commit b8f8654

Please sign in to comment.