Skip to content

Commit

Permalink
Auto merge of #80680 - flip1995:clippy_backport, r=Mark-Simulacrum
Browse files Browse the repository at this point in the history 
[beta] Clippy backport field_reassign_with_default fix

cc `@ebroto` `@Manishearth`

We had multiple complaints about this lint landing in stable 1.49 with a FP hit quite often in real world code. We want to backport the fix for this, so it already gets into 1.50. A stable backport is not warranted by this IMO.

r? `@Mark-Simulacrum`
  • Loading branch information
@bors
bors committed Jan 5, 2021
2 parents 25b3db3 + 70fced0 commit ff59982
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 119 deletions.
188 changes: 71 additions & 117 deletions src/tools/clippy/clippy_lints/src/default.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -103,34 +103,50 @@ 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
let mut first_assign = None;
let mut assigned_fields = Vec::new();
let mut cancel_lint = false;
for consecutive_statement in &block.stmts[stmt_idx + 1..] {
// interrupt if the statement is a let binding (`Local`) that shadows the original
// binding
if stmt_shadows_binding(consecutive_statement, binding_name) {
break;
}
// find out if and which field was set by this `consecutive_statement`
else if let Some((field_ident, assign_rhs)) =
field_reassigned_by_stmt(consecutive_statement, binding_name)
{
if let Some((field_ident, assign_rhs)) = field_reassigned_by_stmt(consecutive_statement, binding_name) {
// interrupt and cancel lint if assign_rhs references the original binding
if contains_name(binding_name, assign_rhs) {
cancel_lint = true;
Expand All @@ -152,7 +168,7 @@ impl LateLintPass<'_> for Default {
first_assign = Some(consecutive_statement);
}
}
// interrupt also if no field was assigned, since we only want to look at consecutive statements
// interrupt if no field was assigned, since we only want to look at consecutive statements
else {
break;
}
Expand All @@ -161,55 +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 {
// 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::<Vec<(Symbol, &Expr<'_>)>>();
// 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));

// 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));
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);
}
}
}
Expand All @@ -230,47 +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()
}

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<'tcx>(this: &Stmt<'tcx>, binding_name: Symbol) -> Option<(Ident, &'tcx Expr<'tcx>)> {
if_chain! {
Expand All @@ -290,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![]
}
12 changes: 12 additions & 0 deletions src/tools/clippy/tests/ui/field_reassign_with_default.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}
4 changes: 2 additions & 2 deletions src/tools/clippy/tests/ui/field_reassign_with_default.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ error: field assignment outside of initializer for an instance created with Defa
LL | a.i = Default::default();
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: consider initializing the variable with `A::default()` and removing relevant reassignments
note: consider initializing the variable with `A { i: Default::default(), ..Default::default() }` and removing relevant reassignments
--> $DIR/field_reassign_with_default.rs:90:5
|
LL | let mut a: A = Default::default();
Expand All @@ -65,7 +65,7 @@ error: field assignment outside of initializer for an instance created with Defa
LL | a.i = Default::default();
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: consider initializing the variable with `A { j: 45, ..Default::default() }` and removing relevant reassignments
note: consider initializing the variable with `A { i: Default::default(), j: 45 }` and removing relevant reassignments
--> $DIR/field_reassign_with_default.rs:94:5
|
LL | let mut a: A = Default::default();
Expand Down

0 comments on commit ff59982

Please sign in to comment.