Fix field_reassign_with_default for private fields

2 files changed, 81 insertions, 93 deletions

diff --git a/clippy_lints/src/default.rs b/clippy_lints/src/default.rs
index c3fe77f6250..b0d7c7b3baa 100644
--- a/clippy_lints/src/default.rs
+++ b/clippy_lints/src/default.rs
@@ -6,7 +6,7 @@ use rustc_errors::Applicability;
 use rustc_hir::def::Res;
 use rustc_hir::{Block, Expr, ExprKind, PatKind, QPath, Stmt, StmtKind};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty::{self, Adt, Ty};
+use rustc_middle::ty;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::symbol::{Ident, Symbol};
 use rustc_span::Span;
@@ -103,18 +103,41 @@ impl LateLintPass<'_> for Default {
     }
 
     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
-        let binding_statements_using_default = enumerate_bindings_using_default(cx, block);
-
         // start from the `let mut _ = _::default();` and look at all the following
         // statements, see if they re-assign the fields of the binding
-        for (stmt_idx, binding_name, binding_type, span) in binding_statements_using_default {
-            // the last statement of a block cannot trigger the lint
-            if stmt_idx == block.stmts.len() - 1 {
-                break;
-            }
+        let stmts_head = match block.stmts {
+            // Skip the last statement since there cannot possibly be any following statements that re-assign fields.
+            [head @ .., _] if !head.is_empty() => head,
+            _ => return,
+        };
+        for (stmt_idx, stmt) in stmts_head.iter().enumerate() {
+            // 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
+            let (local, variant, binding_name, binding_type, span) = if_chain! {
+                // only take `let ...` statements
+                if let StmtKind::Local(local) = stmt.kind;
+                if let Some(expr) = local.init;
+                // only take bindings to identifiers
+                if let PatKind::Binding(_, binding_id, ident, _) = local.pat.kind;
+                // only when assigning `... = Default::default()`
+                if is_expr_default(expr, cx);
+                let binding_type = cx.typeck_results().node_type(binding_id);
+                if let Some(adt) = binding_type.ty_adt_def();
+                if adt.is_struct();
+                let variant = adt.non_enum_variant();
+                if adt.did.is_local() || !variant.is_field_list_non_exhaustive();
+                let module_did = cx.tcx.parent_module(stmt.hir_id).to_def_id();
+                if variant
+                    .fields
+                    .iter()
+                    .all(|field| field.vis.is_accessible_from(module_did, cx.tcx));
+                then {
+                    (local, variant, ident.name, binding_type, expr.span)
+                } else {
+                    continue;
+                }
+            };
 
             // find all "later statement"'s where the fields of the binding set as
             // Default::default() get reassigned, unless the reassignment refers to the original binding
@@ -154,49 +177,45 @@ impl LateLintPass<'_> for Default {
             // if there are incorrectly assigned fields, do a span_lint_and_note to suggest
             // construction using `Ty { fields, ..Default::default() }`
             if !assigned_fields.is_empty() && !cancel_lint {
-                // take the original assignment as span
-                let stmt = &block.stmts[stmt_idx];
-
-                if let StmtKind::Local(preceding_local) = &stmt.kind {
-                    // 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.iter().any(|(a, _)| a == &field.name));
+                // if all fields of the struct are not assigned, add `.. Default::default()` to the suggestion.
+                let ext_with_default = !variant
+                    .fields
+                    .iter()
+                    .all(|field| assigned_fields.iter().any(|(a, _)| a == &field.ident.name));
 
-                    let field_list = assigned_fields
-                        .into_iter()
-                        .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 field_list = assigned_fields
+                    .into_iter()
+                    .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 {
-                        if field_list.is_empty() {
-                            format!("{}::default()", binding_type)
-                        } else {
-                            format!("{} {{ {}, ..Default::default() }}", binding_type, field_list)
-                        }
+                let sugg = if ext_with_default {
+                    if field_list.is_empty() {
+                        format!("{}::default()", binding_type)
                     } else {
-                        format!("{} {{ {} }}", binding_type, field_list)
-                    };
+                        format!("{} {{ {}, ..Default::default() }}", binding_type, field_list)
+                    }
+                } else {
+                    format!("{} {{ {} }}", binding_type, field_list)
+                };
 
-                    // span lint once per statement that binds default
-                    span_lint_and_note(
-                        cx,
-                        FIELD_REASSIGN_WITH_DEFAULT,
-                        first_assign.unwrap().span,
-                        "field assignment outside of initializer for an instance created with Default::default()",
-                        Some(preceding_local.span),
-                        &format!(
-                            "consider initializing the variable with `{}` and removing relevant reassignments",
-                            sugg
-                        ),
-                    );
-                    self.reassigned_linted.insert(span);
-                }
+                // span lint once per statement that binds default
+                span_lint_and_note(
+                    cx,
+                    FIELD_REASSIGN_WITH_DEFAULT,
+                    first_assign.unwrap().span,
+                    "field assignment outside of initializer for an instance created with Default::default()",
+                    Some(local.span),
+                    &format!(
+                        "consider initializing the variable with `{}` and removing relevant reassignments",
+                        sugg
+                    ),
+                );
+                self.reassigned_linted.insert(span);
             }
         }
     }
@@ -217,38 +236,6 @@ fn is_expr_default<'tcx>(expr: &'tcx Expr<'tcx>, cx: &LateContext<'tcx>) -> bool
     }
 }
 
-/// 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<'tcx>,
-) -> Vec<(usize, Symbol, Ty<'tcx>, Span)> {
-    block
-        .stmts
-        .iter()
-        .enumerate()
-        .filter_map(|(idx, stmt)| {
-            if_chain! {
-                // only take `let ...` statements
-                if let StmtKind::Local(ref local) = stmt.kind;
-                // only take bindings to identifiers
-                if let PatKind::Binding(_, _, ident, _) = local.pat.kind;
-                // that are not tuples
-                let ty = cx.typeck_results().pat_ty(local.pat);
-                if !matches!(ty.kind(), ty::Tuple(_));
-                // only when assigning `... = Default::default()`
-                if let Some(ref expr) = local.init;
-                if is_expr_default(expr, cx);
-                then {
-                    Some((idx, ident.name, ty, expr.span))
-                } else {
-                    None
-                }
-            }
-        })
-        .collect()
-}
-
 /// Returns the reassigned field and the assigning expression (right-hand side of assign).
 fn field_reassigned_by_stmt<'tcx>(this: &Stmt<'tcx>, binding_name: Symbol) -> Option<(Ident, &'tcx Expr<'tcx>)> {
     if_chain! {
@@ -268,14 +255,3 @@ fn field_reassigned_by_stmt<'tcx>(this: &Stmt<'tcx>, binding_name: Symbol) -> Op
         }
     }
 }
-
-/// Returns the vec of fields for a struct and an empty vec for non-struct ADTs.
-fn fields_of_type(ty: Ty<'_>) -> Vec<Ident> {
-    if let Adt(adt, _) = ty.kind() {
-        if adt.is_struct() {
-            let variant = &adt.non_enum_variant();
-            return variant.fields.iter().map(|f| f.ident).collect();
-        }
-    }
-    vec![]
-}
diff --git a/tests/ui/field_reassign_with_default.rs b/tests/ui/field_reassign_with_default.rs
index 79a30c22f95..3e0921022b4 100644
--- a/tests/ui/field_reassign_with_default.rs
+++ b/tests/ui/field_reassign_with_default.rs
@@ -107,4 +107,16 @@ fn main() {
     x.i = side_effect.next();
     x.j = 2;
     x.i = side_effect.next();
+
+    // don't lint - some private fields
+    let mut x = m::F::default();
+    x.a = 1;
+}
+
+mod m {
+    #[derive(Default)]
+    pub struct F {
+        pub a: u64,
+        b: u64,
+    }
 }