Skip to content

Commit

Permalink
Catch more than one assignment in field_reassign_with_default
Browse files Browse the repository at this point in the history
  • Loading branch information
Henri Lunnikivi committed Aug 23, 2020
1 parent 2371879 commit 75f2c69
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 36 deletions.
101 changes: 67 additions & 34 deletions clippy_lints/src/field_reassign_with_default.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::utils::{snippet, span_lint_and_note, match_path_ast, paths};
use if_chain::if_chain;
use rustc_ast::ast::{BindingMode, Block, ExprKind, Mutability, PatKind, StmtKind};
use rustc_ast::ast::{Block, ExprKind, PatKind, StmtKind};
use rustc_lint::{EarlyContext, EarlyLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::symbol::Symbol;
Expand Down Expand Up @@ -50,8 +50,8 @@ impl EarlyLintPass for FieldReassignWithDefault {
if_chain! {
// only take `let ...` statements
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 take bindings to identifiers
if let PatKind::Ident(_, 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;
Expand All @@ -68,48 +68,81 @@ impl EarlyLintPass for FieldReassignWithDefault {
})
.collect::<Vec<_>>();

// look at all the following statements for the binding statements and see if they reassign
// the fields of the binding
// start from the `let mut binding = Default::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
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( _, ref 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_snippet = snippet(cx, later_rhs.span, "..");
if let Some(ty) = &preceding_local.ty {
let ty_snippet = snippet(cx, ty.span, "_");
// find all "later statement"'s where the fields of the binding set as
// Default::default() get reassigned
let mut first_assign = None;
let mut assigned_fields = vec![];
for post_defassign_stmt 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::Ident(_, id, _) = local.pat.kind {
if id.name == 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 path ) = binding.kind;
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));

span_lint_and_note(
cx,
FIELD_REASSIGN_WITH_DEFAULT,
later_stmt.span,
"field assignment outside of initializer for an instance created with Default::default()",
Some(preceding_local.span),
&format!("consider initializing the variable immutably with `{} {{ {}: {}, ..Default::default() }}`", ty_snippet, field, value_snippet),
);
// also set first instance of error for help message
if first_assign.is_none() {
first_assign = Some(post_defassign_stmt);
}
}
}
}
}

// 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
let stmt = &block.stmts[stmt_idx];
if let StmtKind::Local(preceding_local) = &stmt.kind {
if let Some(ty) = &preceding_local.ty {
let ty_snippet = snippet(cx, ty.span, "_");

let field_list = assigned_fields.into_iter().map(|(field, value)| format!("{}: {}", field, value)).collect::<Vec<String>>().join(", ");

let sugg = format!("{} {{ {}, ..Default::default() }}", ty_snippet, field_list);

// span lint once per statement that binds default
span_lint_and_note(
cx,
FIELD_REASSIGN_WITH_DEFAULT,
first_assign.unwrap_or_else(|| unreachable!()).span,
"field assignment outside of initializer for an instance created with Default::default()",
Some(preceding_local.span),
&format!("consider initializing the variable immutably with `{}`", sugg),
);
}
}
}
}
}
}
7 changes: 6 additions & 1 deletion tests/ui/field_reassign_with_default.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ struct B {
}

fn main() {
// wrong
// wrong, produces first error in stderr
let mut a: A = Default::default();
a.i = 42;

Expand Down Expand Up @@ -51,4 +51,9 @@ fn main() {
let mut b = B { i: 15, j: 16 };
let mut a: A = Default::default();
b.i = 2;

// wrong, produces second error in stderr
let mut a: A = Default::default();
a.i = 42;
a.j = 43;
}
14 changes: 13 additions & 1 deletion tests/ui/field_reassign_with_default.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,17 @@ note: consider initializing the variable immutably with `A { i: 42, ..Default::d
LL | let mut a: A = Default::default();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error
error: field assignment outside of initializer for an instance created with Default::default()
--> $DIR/field_reassign_with_default.rs:57:5
|
LL | a.i = 42;
| ^^^^^^^^^
|
note: consider initializing the variable immutably with `A { i: 42, j: 43, ..Default::default() }`
--> $DIR/field_reassign_with_default.rs:56:5
|
LL | let mut a: A = Default::default();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

0 comments on commit 75f2c69

Please sign in to comment.