diff --git a/clippy_lints/src/field_reassign_with_default.rs b/clippy_lints/src/field_reassign_with_default.rs index e75c1bef252b..bb50631d94d5 100644 --- a/clippy_lints/src/field_reassign_with_default.rs +++ b/clippy_lints/src/field_reassign_with_default.rs @@ -1,8 +1,8 @@ use crate::utils::{match_def_path, paths, qpath_res, snippet, span_lint_and_note}; use if_chain::if_chain; use rustc_hir::def::Res; -use rustc_hir::{Block, ExprKind, PatKind, QPath, StmtKind}; -use rustc_span::symbol::Symbol; +use rustc_hir::{Block, Expr, ExprKind, PatKind, QPath, Stmt, StmtKind}; +use rustc_span::symbol::{Ident, Symbol}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -11,7 +11,7 @@ declare_clippy_lint! { /// **What it does:** Checks for immediate reassignment of fields initialized /// with Default::default(). /// - /// **Why is this bad?**It's more idiomatic to use [functional update syntax](https://doc.rust-lang.org/reference/expressions/struct-expr.html#functional-update-syntax). + /// **Why is this bad?**It's more idiomatic to use the [functional update syntax](https://doc.rust-lang.org/reference/expressions/struct-expr.html#functional-update-syntax). /// /// **Known problems:** None. /// @@ -39,36 +39,13 @@ declare_lint_pass!(FieldReassignWithDefault => [FIELD_REASSIGN_WITH_DEFAULT]); impl LateLintPass<'_> for FieldReassignWithDefault { fn check_block(&mut self, cx: &LateContext<'_>, 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 ...` statements - if let StmtKind::Local(ref local) = stmt.kind; - // only take bindings to identifiers - if let PatKind::Binding(_, _, ident, _) = 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(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 { - Some((idx, ident.name)) - } - else { - None - } - } - }) - .collect::>(); - // start from the `let mut binding = Default::default();` and look at all the following + // 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: Vec<(usize, Symbol)> = 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) in binding_statements_using_default { // last statement of block cannot trigger the lint @@ -80,40 +57,24 @@ impl LateLintPass<'_> for FieldReassignWithDefault { // Default::default() get reassigned let mut first_assign = None; let mut assigned_fields = vec![]; - for post_defassign_stmt in &block.stmts[stmt_idx + 1..] { + for consequtive_statement in &block.stmts[stmt_idx + 1..] { // interrupt if the statement is a let binding (`Local`) that shadows the original // binding - if let StmtKind::Local(local) = &post_defassign_stmt.kind { - if let PatKind::Binding(_, _, ident, _) = local.pat.kind { - if ident.name == binding_name { - break; - } - } + if stmt_shadows_binding(consequtive_statement, &binding_name) { + break; } // statement kinds other than `StmtKind::Local` are valid, because they cannot // shadow a binding else { - if_chain! { - // only take assignments - if let StmtKind::Semi(ref later_expr) = post_defassign_stmt.kind; - if let ExprKind::Assign(ref assign_lhs, ref assign_rhs, _) = later_expr.kind; - // only take assignments to fields where the left-hand side field is a field of - // the same binding as the previous statement - if let ExprKind::Field(ref binding, later_field_ident) = assign_lhs.kind; - if let ExprKind::Path(ref qpath) = binding.kind; - if let QPath::Resolved(_, path) = qpath; - if let Some(second_binding_name) = path.segments.last(); - if second_binding_name.ident.name == binding_name; - then { - // extract and store the name of the field and the assigned value for help message - let field = later_field_ident.name; - let value_snippet = snippet(cx, assign_rhs.span, ".."); - assigned_fields.push((field, value_snippet)); + // find out if and which field was set by this `consequtive_statement` + if let Some((field_ident, assign_rhs)) = field_reassigned_by_stmt(consequtive_statement, &binding_name) { + // extract and store the assigned value for help message + let value_snippet = snippet(cx, assign_rhs.span, ".."); + assigned_fields.push((field_ident.name, value_snippet)); - // also set first instance of error for help message - if first_assign.is_none() { - first_assign = Some(post_defassign_stmt); - } + // also set first instance of error for help message + if first_assign.is_none() { + first_assign = Some(consequtive_statement); } } } @@ -122,7 +83,7 @@ impl LateLintPass<'_> for FieldReassignWithDefault { // if there are incorrectly assigned fields, do a span_lint_and_note to suggest // immutable construction using `Ty { fields, ..Default::default() }` if !assigned_fields.is_empty() { - // take the preceding statement as span + // take the original assignment as span let stmt = &block.stmts[stmt_idx]; if let StmtKind::Local(preceding_local) = &stmt.kind { if let Some(ty) = &preceding_local.ty { @@ -151,3 +112,61 @@ impl LateLintPass<'_> for FieldReassignWithDefault { } } } + +fn enumerate_bindings_using_default(cx: &LateContext<'_>, block: &Block<'_>) -> Vec<(usize, Symbol)> { + 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; + // 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); + then { + Some((idx, ident.name)) + } + else { + None + } + } + }).collect() +} + +fn stmt_shadows_binding(this: &Stmt<'_>, shadowed: &Symbol) -> bool { + if let StmtKind::Local(local) = &this.kind { + if let PatKind::Binding(_, _, ident, _) = local.pat.kind { + return &ident.name == shadowed; + } + } + false +} + +/// Returns the reassigned field and the assigning expression (right-hand side of assign). +fn field_reassigned_by_stmt<'hir>(this: &Stmt<'hir>, binding_name: &Symbol) -> Option<(Ident, &'hir Expr<'hir>)> { + if_chain! { + // only take assignments + if let StmtKind::Semi(ref later_expr) = this.kind; + if let ExprKind::Assign(ref assign_lhs, ref assign_rhs, _) = later_expr.kind; + // only take assignments to fields where the left-hand side field is a field of + // the same binding as the previous statement + if let ExprKind::Field(ref binding, field_ident) = assign_lhs.kind; + if let ExprKind::Path(ref qpath) = binding.kind; + if let QPath::Resolved(_, path) = qpath; + if let Some(second_binding_name) = path.segments.last(); + if &second_binding_name.ident.name == binding_name; + then { + Some((field_ident, assign_rhs)) + } else { + None + } + } +}