Add lint for 'field_reassign_with_default` #568

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

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:
 

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 <binding> = 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 <binding> ...`
+                    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::<Vec<_>>();
+
+        // 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),
+                            );
+                        }
+                    }
+                }
+            }
+        }
+    }
+}

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),

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",

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;
+}

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
+