From 247e0b5cd3bf84d06769e291518746504d1cfa81 Mon Sep 17 00:00:00 2001 From: Henri Lunnikivi Date: Thu, 31 Oct 2019 18:02:02 +0200 Subject: [PATCH 1/8] Add UI test for field_reassign_with_default --- tests/ui/field_reassign_with_default.rs | 38 +++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 tests/ui/field_reassign_with_default.rs diff --git a/tests/ui/field_reassign_with_default.rs b/tests/ui/field_reassign_with_default.rs new file mode 100644 index 000000000000..e6d549fdb45b --- /dev/null +++ b/tests/ui/field_reassign_with_default.rs @@ -0,0 +1,38 @@ +#![warn(clippy::field_reassign_with_default)] + +#[derive(Default)] +struct A { + i: i32, + j: i64, +} + +struct B { + i: i32, + j: i64, +} + +#[derive(Default)] +struct C { + i: i32, +} + +fn main() { + // wrong + let mut a: A = Default::default(); + a.i = 42; + + // right + let mut a: A = Default::default(); + + // right + let a = A { + i: 42, + ..Default::default() + }; + + // right + let b = B { i: 42, j: 24 }; + + // right + let c: C = Default::default(); +} From d3e6fef5dd7d50ee88795c9f486f56e0309e94f0 Mon Sep 17 00:00:00 2001 From: Henri Lunnikivi Date: Thu, 31 Oct 2019 18:30:31 +0200 Subject: [PATCH 2/8] Add lint declaration for field_reassign_with_default --- clippy_lints/src/field_reassign_with_default.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 clippy_lints/src/field_reassign_with_default.rs diff --git a/clippy_lints/src/field_reassign_with_default.rs b/clippy_lints/src/field_reassign_with_default.rs new file mode 100644 index 000000000000..a9a0c85ba567 --- /dev/null +++ b/clippy_lints/src/field_reassign_with_default.rs @@ -0,0 +1,12 @@ +use rustc::lint::{EarlyLintPass, LintArray, LintPass}; +use rustc::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + pub FIELD_REASSIGN_WITH_DEFAULT, + pedantic, + "instance initialized with Default should have its fields set in the initializer" +} + +declare_lint_pass!(FieldReassignWithDefault => [FIELD_REASSIGN_WITH_DEFAULT]); + +impl EarlyLintPass for FieldReassignWithDefault {} From 5a557f630e5ff2fcd81dad81bf046a181cd537df Mon Sep 17 00:00:00 2001 From: Henri Lunnikivi Date: Thu, 31 Oct 2019 18:34:33 +0200 Subject: [PATCH 3/8] Run update_lints --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/lib.rs | 3 +++ src/lintlist/mod.rs | 9 ++++++++- 4 files changed, 13 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f527a407db7..944a769663fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1142,6 +1142,7 @@ Released 2018-09-13 [`extra_unused_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#extra_unused_lifetimes [`fallible_impl_from`]: https://rust-lang.github.io/rust-clippy/master/index.html#fallible_impl_from [`filetype_is_file`]: https://rust-lang.github.io/rust-clippy/master/index.html#filetype_is_file +[`field_reassign_with_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#field_reassign_with_default [`filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map [`filter_map_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_next [`filter_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_next diff --git a/README.md b/README.md index 39cc9310f512..145d203401a9 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 355 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 356 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a641ff777871..90dc06380cf9 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -207,6 +207,7 @@ pub mod excessive_precision; pub mod exit; pub mod explicit_write; pub mod fallible_impl_from; +pub mod field_reassign_with_default; pub mod format; pub mod formatting; pub mod functions; @@ -536,6 +537,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &exit::EXIT, &explicit_write::EXPLICIT_WRITE, &fallible_impl_from::FALLIBLE_IMPL_FROM, + &field_reassign_with_default::FIELD_REASSIGN_WITH_DEFAULT, &format::USELESS_FORMAT, &formatting::POSSIBLE_MISSING_COMMA, &formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING, @@ -1062,6 +1064,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&eta_reduction::REDUNDANT_CLOSURE_FOR_METHOD_CALLS), LintId::of(&excessive_bools::FN_PARAMS_EXCESSIVE_BOOLS), LintId::of(&excessive_bools::STRUCT_EXCESSIVE_BOOLS), + LintId::of(&field_reassign_with_default::FIELD_REASSIGN_WITH_DEFAULT), LintId::of(&functions::MUST_USE_CANDIDATE), LintId::of(&functions::TOO_MANY_LINES), LintId::of(&if_not_else::IF_NOT_ELSE), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 06a1fca4fdd8..6bbfe0578324 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 355] = [ +pub const ALL_LINTS: [Lint; 356] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -567,6 +567,13 @@ pub const ALL_LINTS: [Lint; 355] = [ deprecation: None, module: "methods", }, + Lint { + name: "field_reassign_with_default", + group: "pedantic", + desc: "instance initialized with Default should have its fields set in the initializer", + deprecation: None, + module: "field_reassign_with_default", + }, Lint { name: "filter_map", group: "pedantic", From 989c5d73b792408490c9be69ff9eaff9b616287b Mon Sep 17 00:00:00 2001 From: Henri Lunnikivi Date: Thu, 31 Oct 2019 18:34:49 +0200 Subject: [PATCH 4/8] Register field_reassign_with_default to early pass --- clippy_lints/src/field_reassign_with_default.rs | 4 ++-- clippy_lints/src/lib.rs | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/field_reassign_with_default.rs b/clippy_lints/src/field_reassign_with_default.rs index a9a0c85ba567..97e3fd2f8dcf 100644 --- a/clippy_lints/src/field_reassign_with_default.rs +++ b/clippy_lints/src/field_reassign_with_default.rs @@ -1,4 +1,4 @@ -use rustc::lint::{EarlyLintPass, LintArray, LintPass}; +use rustc::lint::{LateLintPass, LintArray, LintPass}; use rustc::{declare_lint_pass, declare_tool_lint}; declare_clippy_lint! { @@ -9,4 +9,4 @@ declare_clippy_lint! { declare_lint_pass!(FieldReassignWithDefault => [FIELD_REASSIGN_WITH_DEFAULT]); -impl EarlyLintPass for FieldReassignWithDefault {} +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for FieldReassignWithDefault {} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 90dc06380cf9..fa9ec788a867 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -993,6 +993,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: let enum_variant_name_threshold = conf.enum_variant_name_threshold; store.register_early_pass(move || box enum_variants::EnumVariantNames::new(enum_variant_name_threshold)); store.register_early_pass(|| box tabs_in_doc_comments::TabsInDocComments); + store.register_early_pass(|| box field_reassign_with_default::FieldReassignWithDefault); store.register_late_pass(|| box unused_self::UnusedSelf); store.register_late_pass(|| box mutable_debug_assertion::DebugAssertWithMutCall); store.register_late_pass(|| box exit::Exit); From 22e9bc86312254d64f8ab1c3a5ad006d7329f649 Mon Sep 17 00:00:00 2001 From: Henri Lunnikivi Date: Fri, 1 Nov 2019 02:38:15 +0200 Subject: [PATCH 5/8] Improve UI test cases --- tests/ui/field_reassign_with_default.rs | 30 +++++++++++++++++++------ 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/tests/ui/field_reassign_with_default.rs b/tests/ui/field_reassign_with_default.rs index e6d549fdb45b..83599f2ed9d0 100644 --- a/tests/ui/field_reassign_with_default.rs +++ b/tests/ui/field_reassign_with_default.rs @@ -11,11 +11,6 @@ struct B { j: i64, } -#[derive(Default)] -struct C { - i: i32, -} - fn main() { // wrong let mut a: A = Default::default(); @@ -31,8 +26,29 @@ fn main() { }; // right - let b = B { i: 42, j: 24 }; + let mut a: A = Default::default(); + if a.i == 0 { + a.j = 12; + } + + // right + let mut a: A = Default::default(); + let b = 5; + + // right + let mut b = 32; + let mut a: A = Default::default(); + b = 2; + + // right + let b: B = B { i: 42, j: 24 }; + + // right + let mut b: B = B { i: 42, j: 24 }; + b.i = 52; // right - let c: C = Default::default(); + let mut b = B { i: 15, j: 16 }; + let mut a: A = Default::default(); + b.i = 2; } From 5ad7dcccf58275f2b167e0d2a4bfa15a15f8a93c Mon Sep 17 00:00:00 2001 From: Henri Lunnikivi Date: Fri, 1 Nov 2019 02:39:15 +0200 Subject: [PATCH 6/8] Implement lint for field_reassign_with_default --- .../src/field_reassign_with_default.rs | 88 ++++++++++++++++++- src/lintlist/mod.rs | 2 +- 2 files changed, 85 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/field_reassign_with_default.rs b/clippy_lints/src/field_reassign_with_default.rs index 97e3fd2f8dcf..bd230bad1e6b 100644 --- a/clippy_lints/src/field_reassign_with_default.rs +++ b/clippy_lints/src/field_reassign_with_default.rs @@ -1,12 +1,92 @@ -use rustc::lint::{LateLintPass, LintArray, LintPass}; -use rustc::{declare_lint_pass, declare_tool_lint}; +use crate::utils::span_lint_and_note; +use if_chain::if_chain; +use rustc_ast_pretty::pprust::{expr_to_string, ty_to_string}; +use rustc_lint::{EarlyContext, EarlyLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::symbol::Symbol; +use syntax::ast::{BindingMode, Block, ExprKind, Mutability, PatKind, StmtKind}; declare_clippy_lint! { pub FIELD_REASSIGN_WITH_DEFAULT, pedantic, - "instance initialized with Default should have its fields set in the initializer" + "binding initialized with Default should have its fields set in the initializer" } declare_lint_pass!(FieldReassignWithDefault => [FIELD_REASSIGN_WITH_DEFAULT]); -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for FieldReassignWithDefault {} +impl EarlyLintPass for FieldReassignWithDefault { + fn check_block(&mut self, cx: &EarlyContext<'_>, block: &Block) { + // store statement index and name of binding for all statements like + // `let mut = Default::default();` + let binding_statements_using_default: Vec<(usize, Symbol)> = block + .stmts + .iter() + .enumerate() + .filter_map(|(idx, stmt)| { + if_chain! { + // only take `let ...` + if let StmtKind::Local(ref local) = stmt.kind; + // only take `... mut ...` + if let PatKind::Ident(BindingMode::ByValue(Mutability::Mut), binding, _) = local.pat.kind; + // only when assigning `... = Default::default()` + if let Some(ref expr) = local.init; + if let ExprKind::Call(ref fn_expr, _) = &expr.kind; + if let ExprKind::Path(_, path) = &fn_expr.kind; + if path.segments.len() >= 2; + if path.segments[path.segments.len()-2].ident.as_str() == "Default"; + if path.segments.last().unwrap().ident.as_str() == "default"; + then { + Some((idx, binding.name)) + } + else { + None + } + } + }) + .collect::>(); + + // look at all the following statements for the binding statements and see if they reassign + // the fields of the binding + for (stmt_idx, binding_name) in binding_statements_using_default { + // last statement of block cannot trigger the lint + if stmt_idx == block.stmts.len() - 1 { + break; + } + + // find "later statement"'s where the fields of the binding set by Default::default() + // get reassigned + let later_stmt = &block.stmts[stmt_idx + 1]; + if_chain! { + // only take assignments + if let StmtKind::Semi(ref later_expr) = later_stmt.kind; + if let ExprKind::Assign(ref later_lhs, ref later_rhs, _) = later_expr.kind; + // only take assignments to fields where the field refers to the same binding as the previous statement + if let ExprKind::Field(ref binding_name_candidate, later_field_ident) = later_lhs.kind; + if let ExprKind::Path(_, path) = &binding_name_candidate.kind; + if let Some(second_binding_name) = path.segments.last(); + if second_binding_name.ident.name == binding_name; + then { + // take the preceding statement as span + let stmt = &block.stmts[stmt_idx]; + if let StmtKind::Local(preceding_local) = &stmt.kind { + // reorganize the latter assigment statement into `field` and `value` for the lint + let field = later_field_ident.name.as_str(); + let value = expr_to_string(later_rhs); + if let Some(ty) = &preceding_local.ty { + let ty = ty_to_string(ty); + + span_lint_and_note( + cx, + FIELD_REASSIGN_WITH_DEFAULT, + later_stmt.span, + "field assignment outside of initializer for an instance created with Default::default()", + preceding_local.span, + &format!("consider initializing the variable immutably with `{} {{ {}: {}, ..Default::default() }}`", ty, field, value), + ); + } + } + } + } + } + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 6bbfe0578324..82467c1e4085 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -570,7 +570,7 @@ pub const ALL_LINTS: [Lint; 356] = [ Lint { name: "field_reassign_with_default", group: "pedantic", - desc: "instance initialized with Default should have its fields set in the initializer", + desc: "binding initialized with Default should have its fields set in the initializer", deprecation: None, module: "field_reassign_with_default", }, From 78edc2554c25ec454b3b6feb81415902e4314da5 Mon Sep 17 00:00:00 2001 From: Henri Lunnikivi Date: Fri, 1 Nov 2019 02:40:32 +0200 Subject: [PATCH 7/8] Freeze the lint output for field_reassign_with_default --- tests/ui/field_reassign_with_default.stderr | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 tests/ui/field_reassign_with_default.stderr diff --git a/tests/ui/field_reassign_with_default.stderr b/tests/ui/field_reassign_with_default.stderr new file mode 100644 index 000000000000..3dad6bededa6 --- /dev/null +++ b/tests/ui/field_reassign_with_default.stderr @@ -0,0 +1,15 @@ +error: field assignment outside of initializer for an instance created with Default::default() + --> $DIR/field_reassign_with_default.rs:17:5 + | +LL | a.i = 42; + | ^^^^^^^^^ + | + = note: `-D clippy::field-reassign-with-default` implied by `-D warnings` +note: consider initializing the variable immutably with `A { i: 42, ..Default::default() }` + --> $DIR/field_reassign_with_default.rs:16:5 + | +LL | let mut a: A = Default::default(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to previous error + From 22933c6625c7ccf8476a05b7c29c5fa296904d91 Mon Sep 17 00:00:00 2001 From: Henri Lunnikivi Date: Fri, 1 Nov 2019 03:03:12 +0200 Subject: [PATCH 8/8] Add doc for field_reassign_with_default --- .../src/field_reassign_with_default.rs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/clippy_lints/src/field_reassign_with_default.rs b/clippy_lints/src/field_reassign_with_default.rs index bd230bad1e6b..1b155d960f17 100644 --- a/clippy_lints/src/field_reassign_with_default.rs +++ b/clippy_lints/src/field_reassign_with_default.rs @@ -7,6 +7,31 @@ use rustc_span::symbol::Symbol; use syntax::ast::{BindingMode, Block, ExprKind, Mutability, PatKind, StmtKind}; declare_clippy_lint! { + /// **What it does:** Checks for immediate reassignment of fields initialized + /// with Default::default(). + /// + /// **Why is this bad?** Fields should be set using + /// T { field: value, ..Default::default() } syntax instead of using a mutable binding. + /// + /// **Known problems:** The lint does not detect calls to Default::default() + /// if they are made via another struct implementing the Default trait. This + /// may be corrected with a LateLintPass. If type inference stops requiring + /// an explicit type for assignment using Default::default() this lint will + /// not trigger for cases where the type is elided. This may also be corrected + /// with a LateLintPass. + /// + /// **Example:** + /// ```ignore + /// // Bad + /// let mut a: A = Default::default(); + /// a.i = 42; + /// + /// // Good + /// let a = A { + /// i: 42, + /// .. Default::default() + /// }; + /// ``` pub FIELD_REASSIGN_WITH_DEFAULT, pedantic, "binding initialized with Default should have its fields set in the initializer"