diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ad1a7177eb4..e6d2db63855b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5786,6 +5786,7 @@ Released 2018-09-13 [`explicit_deref_methods`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_deref_methods [`explicit_into_iter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_into_iter_loop [`explicit_iter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_iter_loop +[`explicit_struct_update`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_struct_update [`explicit_write`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_write [`extend_from_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#extend_from_slice [`extend_with_drain`]: https://rust-lang.github.io/rust-clippy/master/index.html#extend_with_drain diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index c3f8e02b4c06..85de9336d17a 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -159,6 +159,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::exhaustive_items::EXHAUSTIVE_ENUMS_INFO, crate::exhaustive_items::EXHAUSTIVE_STRUCTS_INFO, crate::exit::EXIT_INFO, + crate::explicit_struct_update::EXPLICIT_STRUCT_UPDATE_INFO, crate::explicit_write::EXPLICIT_WRITE_INFO, crate::extra_unused_type_parameters::EXTRA_UNUSED_TYPE_PARAMETERS_INFO, crate::fallible_impl_from::FALLIBLE_IMPL_FROM_INFO, diff --git a/clippy_lints/src/explicit_struct_update.rs b/clippy_lints/src/explicit_struct_update.rs new file mode 100644 index 000000000000..9858ab3023eb --- /dev/null +++ b/clippy_lints/src/explicit_struct_update.rs @@ -0,0 +1,147 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::{snippet, snippet_indent}; +use rustc_errors::Applicability; +use rustc_hir::{self as hir, ExprKind, StructTailExpr}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// Checks for struct initializations where two or more fields are being set to the value of the same field from another struct of the same type. + /// ### Why is this bad? + /// This can be done more concisely using struct update syntax. + /// ### Example + /// ```no_run + /// struct Foo { + /// a: i32, + /// b: i32, + /// c: i32, + /// } + /// + /// let my_foo = Foo { + /// a: 1, + /// b: 2, + /// c: 3, + /// }; + /// + /// let my_new_foo = Foo { + /// a: 5, + /// b: my_foo.b, + /// c: my_foo.c, + /// }; + /// ``` + /// Use instead: + /// ```no_run + /// struct Foo { + /// a: i32, + /// b: i32, + /// c: i32, + /// } + /// + /// let my_foo = Foo { + /// a: 1, + /// b: 2, + /// c: 3, + /// }; + /// + /// let my_new_foo = Foo { + /// a: 5, + /// ..my_foo + /// }; + /// ``` + #[clippy::version = "1.89.0"] + pub EXPLICIT_STRUCT_UPDATE, + complexity, + "explicit struct updates in struct instantiations" +} +declare_lint_pass!(ExplicitStructUpdate => [EXPLICIT_STRUCT_UPDATE]); + +impl<'tcx> LateLintPass<'tcx> for ExplicitStructUpdate { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { + if hir::is_range_literal(expr) { + // range literals are lowered to structs, false positive + return; + } + + let ExprKind::Struct(path, fields, StructTailExpr::None) = expr.kind else { + return; + }; + + // the type of the struct + let ty = cx.typeck_results().expr_ty(expr); + + // collect the fields that are being initialized with the same field from another struct of the same + // type + let update_fields = fields.iter().try_fold( + Vec::new(), + |mut acc: Vec<(&rustc_hir::Expr<'_>, &rustc_hir::Expr<'_>)>, f| { + if let ExprKind::Field(base_expr, field_ident) = f.expr.kind { + if let Some(last) = acc.last() { + match (last.1.kind, base_expr.kind) { + ( + ExprKind::Path(hir::QPath::Resolved(_, hir::Path { res: res_a, .. })), + ExprKind::Path(hir::QPath::Resolved(_, hir::Path { res: res_b, .. })), + ) if res_a != res_b => return None, /* if we detect instantiation from multiple bases, we */ + // don't want to lint + _ => (), + } + } + + if cx.typeck_results().expr_ty(base_expr) == ty && f.ident == field_ident { + // accumulate the expressions mapping to the actual field expression, and the expression of the + // base struct, we do this so we can determine if the base struct is the same for all + acc.push((f.expr, base_expr)); + } + } + + Some(acc) + }, + ); + + let (update_base, update_fields): (_, Vec<_>) = match update_fields { + // we only care about the field expressions at this point + Some(fields) if fields.len() > 1 => (fields[0].1, fields.iter().map(|x| x.0.hir_id).collect()), + // no lint if there's no fields or multiple bases + _ => return, + }; + + // the field assignments we are keeping + let non_update_fields_spans: Vec<_> = fields + .iter() + .filter_map(|f| { + if update_fields.contains(&f.expr.hir_id) { + None + } else { + Some(f.span) + } + }) + .collect(); + + let struct_indent = snippet_indent(cx, expr.span).unwrap_or_default(); + let field_indent = format!("{struct_indent} "); + let struct_type = snippet(cx, path.span(), ""); + let struct_fields = non_update_fields_spans + .iter() + .fold(String::new(), |mut acc, &field_span| { + acc.push_str(&field_indent); + acc.push_str(&snippet(cx, field_span, "")); + acc.push_str(",\n"); + acc + }); + let struct_update_snip = snippet(cx, update_base.span, ""); + + let sugg = format!("{struct_type} {{\n{struct_fields}{field_indent}..{struct_update_snip}\n{struct_indent}}}"); + + let msg = "you seem to be updating a struct field with the same field from another struct of the same type"; + + span_lint_and_sugg( + cx, + EXPLICIT_STRUCT_UPDATE, + expr.span, + msg, + "consider using struct update syntax instead", + sugg, + Applicability::MachineApplicable, + ); + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 96a6dee58852..79c58384a40f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -138,6 +138,7 @@ mod excessive_bools; mod excessive_nesting; mod exhaustive_items; mod exit; +mod explicit_struct_update; mod explicit_write; mod extra_unused_type_parameters; mod fallible_impl_from; @@ -830,5 +831,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(cloned_ref_to_slice_refs::ClonedRefToSliceRefs::new(conf))); store.register_late_pass(|_| Box::new(infallible_try_from::InfallibleTryFrom)); store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny)); + store.register_late_pass(|_| Box::new(explicit_struct_update::ExplicitStructUpdate)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/manual_clamp.rs b/clippy_lints/src/manual_clamp.rs index 42fe386d2c3c..0b20baccd387 100644 --- a/clippy_lints/src/manual_clamp.rs +++ b/clippy_lints/src/manual_clamp.rs @@ -451,12 +451,7 @@ fn is_match_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Opt Some((*first_hir_id, *second_hir_id)), ) { return Some(ClampSuggestion { - params: InputMinMax { - input: value, - min: params.min, - max: params.max, - is_float: params.is_float, - }, + params: InputMinMax { input: value, ..params }, span: expr.span, make_assignment: None, hir_with_ignore_attr: None, @@ -510,9 +505,7 @@ fn is_two_if_pattern<'tcx>(cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) -> Some(ClampSuggestion { params: InputMinMax { input: maybe_input_first_path, - min: input_min_max.min, - max: input_min_max.max, - is_float: input_min_max.is_float, + ..input_min_max }, span: first_expr.span.to(second_expr.span), make_assignment: Some(maybe_input_first_path), diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs index 7f2bf99daff2..f263d6b0adc6 100644 --- a/clippy_utils/src/source.rs +++ b/clippy_utils/src/source.rs @@ -733,8 +733,7 @@ pub fn trim_span(sm: &SourceMap, span: Span) -> Span { SpanData { lo: data.lo + BytePos::from_usize(trim_start), hi: data.hi - BytePos::from_usize(trim_end), - ctxt: data.ctxt, - parent: data.parent, + ..data } .span() } diff --git a/tests/ui/explicit_struct_update.fixed b/tests/ui/explicit_struct_update.fixed new file mode 100644 index 000000000000..5443af6edc9d --- /dev/null +++ b/tests/ui/explicit_struct_update.fixed @@ -0,0 +1,74 @@ +#![warn(clippy::explicit_struct_update)] + +struct A { + a: i32, + b: i32, + c: i32, + d: i32, +} + +struct B; + +struct C { + a: i32, + b: i32, +} + +fn main() { + // should not lint, no explicit struct update + let a = A { a: 1, b: 2, c: 3, d: 4 }; + + let b = A { + ..a + }; + //~^^^^^^explicit_struct_update + + let c = A { + c: 4, + d: 5, + ..a + }; + //~^^^^^^explicit_struct_update + + let d = A { + d: 5, + ..a + }; + //~^^^^^^explicit_struct_update + + // should not lint, only one field is updated + let e = A { + a: a.a, + b: 5, + c: 6, + d: 7, + }; + + // should not lint, we already have update syntax + let f = A { ..a }; + + // should not lint, we already have update syntax + let g = A { a: a.a, b: a.b, ..a }; + + // should not lint, multiple bases + let h = A { + a: a.a, + b: d.b, + c: d.c, + d: 5, + }; + + // should not lint, no fields + let i = B {}; + + // should not lint, no explicit struct update + let j = C { a: 1, b: 2 }; + + // should not lint, fields filled from different type + let k = A { + a: j.a, + b: j.b, + c: 3, + d: 4, + }; +} diff --git a/tests/ui/explicit_struct_update.rs b/tests/ui/explicit_struct_update.rs new file mode 100644 index 000000000000..fa70e82cb5b6 --- /dev/null +++ b/tests/ui/explicit_struct_update.rs @@ -0,0 +1,80 @@ +#![warn(clippy::explicit_struct_update)] + +struct A { + a: i32, + b: i32, + c: i32, + d: i32, +} + +struct B; + +struct C { + a: i32, + b: i32, +} + +fn main() { + // should not lint, no explicit struct update + let a = A { a: 1, b: 2, c: 3, d: 4 }; + + let b = A { + a: a.a, + b: a.b, + c: a.c, + d: a.d, + }; + //~^^^^^^explicit_struct_update + + let c = A { + a: a.a, + b: a.b, + c: 4, + d: 5, + }; + //~^^^^^^explicit_struct_update + + let d = A { + a: a.a, + b: a.b, + c: a.c, + d: 5, + }; + //~^^^^^^explicit_struct_update + + // should not lint, only one field is updated + let e = A { + a: a.a, + b: 5, + c: 6, + d: 7, + }; + + // should not lint, we already have update syntax + let f = A { ..a }; + + // should not lint, we already have update syntax + let g = A { a: a.a, b: a.b, ..a }; + + // should not lint, multiple bases + let h = A { + a: a.a, + b: d.b, + c: d.c, + d: 5, + }; + + // should not lint, no fields + let i = B {}; + + // should not lint, no explicit struct update + let j = C { a: 1, b: 2 }; + + // should not lint, fields filled from different type + let k = A { + a: j.a, + b: j.b, + c: 3, + d: 4, + }; +} diff --git a/tests/ui/explicit_struct_update.stderr b/tests/ui/explicit_struct_update.stderr new file mode 100644 index 000000000000..51d24d7fbfff --- /dev/null +++ b/tests/ui/explicit_struct_update.stderr @@ -0,0 +1,64 @@ +error: you seem to be updating a struct field with the same field from another struct of the same type + --> tests/ui/explicit_struct_update.rs:21:13 + | +LL | let b = A { + | _____________^ +LL | | a: a.a, +LL | | b: a.b, +LL | | c: a.c, +LL | | d: a.d, +LL | | }; + | |_____^ + | + = note: `-D clippy::explicit-struct-update` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::explicit_struct_update)]` +help: consider using struct update syntax instead + | +LL ~ let b = A { +LL + ..a +LL ~ }; + | + +error: you seem to be updating a struct field with the same field from another struct of the same type + --> tests/ui/explicit_struct_update.rs:29:13 + | +LL | let c = A { + | _____________^ +LL | | a: a.a, +LL | | b: a.b, +LL | | c: 4, +LL | | d: 5, +LL | | }; + | |_____^ + | +help: consider using struct update syntax instead + | +LL ~ let c = A { +LL + c: 4, +LL + d: 5, +LL + ..a +LL ~ }; + | + +error: you seem to be updating a struct field with the same field from another struct of the same type + --> tests/ui/explicit_struct_update.rs:37:13 + | +LL | let d = A { + | _____________^ +LL | | a: a.a, +LL | | b: a.b, +LL | | c: a.c, +LL | | d: 5, +LL | | }; + | |_____^ + | +help: consider using struct update syntax instead + | +LL ~ let d = A { +LL + d: 5, +LL + ..a +LL ~ }; + | + +error: aborting due to 3 previous errors + diff --git a/tests/ui/unnecessary_struct_initialization.fixed b/tests/ui/unnecessary_struct_initialization.fixed index 07dc90acfa30..755a50f46731 100644 --- a/tests/ui/unnecessary_struct_initialization.fixed +++ b/tests/ui/unnecessary_struct_initialization.fixed @@ -1,4 +1,4 @@ -#![allow(clippy::non_canonical_clone_impl, unused)] +#![allow(clippy::non_canonical_clone_impl, clippy::explicit_struct_update, unused)] #![warn(clippy::unnecessary_struct_initialization)] struct S { diff --git a/tests/ui/unnecessary_struct_initialization.rs b/tests/ui/unnecessary_struct_initialization.rs index 12a444fc878b..a113c90a3d71 100644 --- a/tests/ui/unnecessary_struct_initialization.rs +++ b/tests/ui/unnecessary_struct_initialization.rs @@ -1,4 +1,4 @@ -#![allow(clippy::non_canonical_clone_impl, unused)] +#![allow(clippy::non_canonical_clone_impl, clippy::explicit_struct_update, unused)] #![warn(clippy::unnecessary_struct_initialization)] struct S {