Do not add fields assigned with default to suggestion

Add test case.

Author: Henri Lunnikivi

clippy_lints/src/field_reassign_with_default.rs

@@ -40,7 +40,7 @@ declare_clippy_lint! {
 declare_lint_pass!(FieldReassignWithDefault => [FIELD_REASSIGN_WITH_DEFAULT]);
 
 impl LateLintPass<'_> for FieldReassignWithDefault {
-    fn check_block(&mut self, cx: &LateContext<'_>, block: &Block<'_>) {
+    fn check_block<'tcx>(&mut self, cx: &LateContext<'tcx>, block: &Block<'tcx>) {
         // find all binding statements like `let mut _ = T::default()` where `T::default()` is the
         // `default` method of the `Default` trait, and store statement index in current block being
         // checked and the name of the bound variable
@@ -68,11 +68,8 @@ impl LateLintPass<'_> for FieldReassignWithDefault {
                 else if let Some((field_ident, assign_rhs)) =
                     field_reassigned_by_stmt(consecutive_statement, binding_name)
                 {
-                    // extract and store the assigned value for help message
-                    let value_snippet = snippet(cx, assign_rhs.span, "..");
-
                     // always re-insert set value, this way the latest value is stored for output snippet
-                    assigned_fields.insert(field_ident.name, value_snippet);
+                    assigned_fields.insert(field_ident.name, assign_rhs);
 
                     // also set first instance of error for help message
                     if first_assign.is_none() {
@@ -92,19 +89,33 @@ impl LateLintPass<'_> for FieldReassignWithDefault {
                 let stmt = &block.stmts[stmt_idx];
 
                 if let StmtKind::Local(preceding_local) = &stmt.kind {
+                    // filter out fields like `= Default::default()`, because the FRU already covers them
+                    let assigned_fields = assigned_fields
+                        .into_iter()
+                        .filter(|(_, rhs)| !is_expr_default(rhs, cx))
+                        .collect::<FxHashMap<Symbol, &Expr<'_>>>();
+
                     // if all fields of the struct are not assigned, add `.. Default::default()` to the suggestion.
                     let ext_with_default = !fields_of_type(binding_type)
                         .iter()
                         .all(|field| assigned_fields.contains_key(&field.name));
 
                     let field_list = assigned_fields
                         .into_iter()
-                        .map(|(field, value)| format!("{}: {}", field, value))
+                        .map(|(field, rhs)| {
+                            // extract and store the assigned value for help message
+                            let value_snippet = snippet(cx, rhs.span, "..");
+                            format!("{}: {}", field, value_snippet)
+                        })
                         .collect::<Vec<String>>()
                         .join(", ");
 
                     let sugg = if ext_with_default {
-                        format!("{} {{ {}, ..Default::default() }}", binding_type, field_list)
+                        if field_list.is_empty() {
+                            format!("{}::default()", binding_type)
+                        } else {
+                            format!("{} {{ {}, ..Default::default() }}", binding_type, field_list)
+                        }
                     } else {
                         format!("{} {{ {} }}", binding_type, field_list)
                     };
@@ -124,9 +135,28 @@ impl LateLintPass<'_> for FieldReassignWithDefault {
     }
 }
 
+/// Checks if the given expression is the `default` method belonging to the `Default` trait.
+fn is_expr_default<'tcx>(expr: &'tcx Expr<'tcx>, cx: &LateContext<'tcx>) -> bool {
+    if_chain! {
+        if let ExprKind::Call(ref fn_expr, _) = &expr.kind;
+        if let ExprKind::Path(qpath) = &fn_expr.kind;
+        if let Res::Def(_, def_id) = qpath_res(cx, qpath, fn_expr.hir_id);
+        // right hand side of assignment is `Default::default`
+        if match_def_path(cx, def_id, &paths::DEFAULT_TRAIT_METHOD);
+        then {
+            true
+        } else {
+            false
+        }
+    }
+}
+
 /// Returns the block indices, identifiers and types of bindings set as `Default::default()`, except
 /// for when the pattern type is a tuple.
-fn enumerate_bindings_using_default<'tcx>(cx: &LateContext<'tcx>, block: &Block<'_>) -> Vec<(usize, Symbol, Ty<'tcx>)> {
+fn enumerate_bindings_using_default<'tcx>(
+    cx: &LateContext<'tcx>,
+    block: &Block<'tcx>,
+) -> Vec<(usize, Symbol, Ty<'tcx>)> {
     block
         .stmts
         .iter()
@@ -142,11 +172,7 @@ fn enumerate_bindings_using_default<'tcx>(cx: &LateContext<'tcx>, block: &Block<
                 if !matches!(ty.kind(), ty::Tuple(_));
                 // 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(qpath) = &fn_expr.kind;
-                if let Res::Def(_, def_id) = qpath_res(cx, qpath, fn_expr.hir_id);
-                // right hand side of assignment is `Default::default`
-                if match_def_path(cx, def_id, &paths::DEFAULT_TRAIT_METHOD);
+                if is_expr_default(expr, cx);
                 then {
                     Some((idx, ident.name, ty))
                 } else {

tests/ui/field_reassign_with_default.rs

@@ -72,4 +72,13 @@ fn main() {
     let mut c: (i32, i32) = Default::default();
     c.0 = 42;
     c.1 = 21;
+
+    // wrong, produces the fifth error in stderr
+    let mut a: A = Default::default();
+    a.i = Default::default();
+
+    // wrong, produces the sixth error in stderr
+    let mut a: A = Default::default();
+    a.i = Default::default();
+    a.j = 45;
 }

tests/ui/field_reassign_with_default.stderr

@@ -47,5 +47,29 @@ note: consider initializing the variable with `A { i: 42, ..Default::default() }
 LL |     let mut a = A::default();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 4 previous errors
+error: field assignment outside of initializer for an instance created with Default::default()
+  --> $DIR/field_reassign_with_default.rs:78:5
+   |
+LL |     a.i = Default::default();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+note: consider initializing the variable with `A::default()`
+  --> $DIR/field_reassign_with_default.rs:77:5
+   |
+LL |     let mut a: A = Default::default();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: field assignment outside of initializer for an instance created with Default::default()
+  --> $DIR/field_reassign_with_default.rs:82:5
+   |
+LL |     a.i = Default::default();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+note: consider initializing the variable with `A { j: 45, ..Default::default() }`
+  --> $DIR/field_reassign_with_default.rs:81:5
+   |
+LL |     let mut a: A = Default::default();
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 6 previous errors