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/field_reassign_with_default.rs b/clippy_lints/src/field_reassign_with_default.rs new file mode 100644 index 000000000000..1b155d960f17 --- /dev/null +++ b/clippy_lints/src/field_reassign_with_default.rs @@ -0,0 +1,117 @@ +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! { + /// **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" +} + +declare_lint_pass!(FieldReassignWithDefault => [FIELD_REASSIGN_WITH_DEFAULT]); + +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/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a641ff777871..fa9ec788a867 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, @@ -991,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); @@ -1062,6 +1065,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..82467c1e4085 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: "binding initialized with Default should have its fields set in the initializer", + deprecation: None, + module: "field_reassign_with_default", + }, Lint { name: "filter_map", group: "pedantic", diff --git a/tests/ui/field_reassign_with_default.rs b/tests/ui/field_reassign_with_default.rs new file mode 100644 index 000000000000..83599f2ed9d0 --- /dev/null +++ b/tests/ui/field_reassign_with_default.rs @@ -0,0 +1,54 @@ +#![warn(clippy::field_reassign_with_default)] + +#[derive(Default)] +struct A { + i: i32, + j: i64, +} + +struct B { + i: i32, + j: i64, +} + +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 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 mut b = B { i: 15, j: 16 }; + let mut a: A = Default::default(); + b.i = 2; +} 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 +