Skip to content

Commit

Permalink
Implement default_could_be_derived and `default_overrides_default_f…
Browse files Browse the repository at this point in the history
…ields` lints

Detect when a manual `Default` implementation isn't using the existing default field values and suggest using `..` instead:

```
error: `Default` impl doesn't use the declared default field values
  --> $DIR/manual-default-impl-could-be-derived.rs:13:1
   |
LL | / impl Default for A {
LL | |     fn default() -> Self {
LL | |         A {
LL | |             x: S,
LL | |             y: 0,
   | |                - this field has a default value
...  |
LL | | }
   | |_^
   |
note: the lint level is defined here
  --> $DIR/manual-default-impl-could-be-derived.rs:4:35
   |
LL | #![deny(default_could_be_derived, default_overrides_default_fields)]
   |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: use the default values in the `impl` to avoid them diverging over time
   |
LL -             x: S,
LL -             y: 0,
LL +             x: S, ..
   |
```

Detect when a manual `Default` implementation for a type containing at least one default field value has *all* fields that could be derived and suggest `#[derive(Default)]`:

```
error: `Default` impl that could be derived
  --> $DIR/manual-default-impl-could-be-derived.rs:27:1
   |
LL |   struct B {
   |   -------- all the fields in this struct have default values
LL |       x: S = S,
   |              - default value
LL |       y: i32 = 1,
   |                - default value
...
LL | / impl Default for B {
LL | |     fn default() -> Self {
LL | |         B {
LL | |             x: S,
...  |
LL | | }
   | |_^
   |
note: the lint level is defined here
  --> $DIR/manual-default-impl-could-be-derived.rs:4:9
   |
LL | #![deny(default_could_be_derived, default_overrides_default_fields)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^
help: to avoid divergence in behavior between `Struct { .. }` and `<Struct as Default>::default()`, derive the `Default`
   |
LL ~ #[derive(Default)] struct B {
   |
```

Store a mapping between the `DefId` for an `impl Default for Ty {}` and the `DefId` of either a Unit variant/struct or an fn with no arguments that is called within `<Ty as Default>::default()`.

When linting `impl`s, if it is for `Default`, we evaluate the contents of their `fn default()`. If it is *only* an ADT literal for `Self` and every field is either a "known to be defaulted" value (`0` or `false`), an explicit `Default::default()` call or a call or path to the same "equivalent" `DefId` from that field's type's `Default::default()` implementation.
  • Loading branch information
estebank committed Dec 23, 2024
1 parent e108481 commit be8b02f
Show file tree
Hide file tree
Showing 30 changed files with 1,237 additions and 40 deletions.
4 changes: 2 additions & 2 deletions compiler/rustc_ast/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1455,8 +1455,8 @@ pub enum StructRest {
Base(P<Expr>),
/// `..`.
Rest(Span),
/// No trailing `..` or expression.
None,
/// No trailing `..` or expression. The `Span` points at the trailing `,` or spot before `}`.
None(Span),
}

#[derive(Clone, Encodable, Decodable, Debug)]
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast/src/mut_visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1771,7 +1771,7 @@ pub fn walk_expr<T: MutVisitor>(vis: &mut T, Expr { kind, id, span, attrs, token
match rest {
StructRest::Base(expr) => vis.visit_expr(expr),
StructRest::Rest(_span) => {}
StructRest::None => {}
StructRest::None(_span) => {}
}
}
ExprKind::Paren(expr) => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast/src/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1094,7 +1094,7 @@ pub fn walk_expr<'a, V: Visitor<'a>>(visitor: &mut V, expression: &'a Expr) -> V
match rest {
StructRest::Base(expr) => try_visit!(visitor.visit_expr(expr)),
StructRest::Rest(_span) => {}
StructRest::None => {}
StructRest::None(_span) => {}
}
}
ExprKind::Tup(subexpressions) => {
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_ast_lowering/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
let rest = match &se.rest {
StructRest::Base(e) => hir::StructTailExpr::Base(self.lower_expr(e)),
StructRest::Rest(sp) => hir::StructTailExpr::DefaultFields(*sp),
StructRest::None => hir::StructTailExpr::None,
StructRest::None(sp) => hir::StructTailExpr::None(*sp),
};
hir::ExprKind::Struct(
self.arena.alloc(self.lower_qpath(
Expand Down Expand Up @@ -1419,7 +1419,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
true
}
StructRest::Rest(_) => true,
StructRest::None => false,
StructRest::None(_) => false,
};
let struct_pat = hir::PatKind::Struct(qpath, field_pats, fields_omitted);
return self.pat_without_dbm(lhs.span, struct_pat);
Expand Down Expand Up @@ -1530,7 +1530,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
hir::ExprKind::Struct(
self.arena.alloc(hir::QPath::LangItem(lang_item, self.lower_span(span))),
fields,
hir::StructTailExpr::None,
hir::StructTailExpr::None(DUMMY_SP),
)
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast_pretty/src/pprust/state/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ impl<'a> State<'a> {
self.word("{");
let has_rest = match rest {
ast::StructRest::Base(_) | ast::StructRest::Rest(_) => true,
ast::StructRest::None => false,
ast::StructRest::None(_) => false,
};
if fields.is_empty() && !has_rest {
self.word("}");
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_expand/src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ impl<'a> ExtCtxt<'a> {
qself: None,
path,
fields,
rest: ast::StructRest::None,
rest: ast::StructRest::None(DUMMY_SP),
})),
)
}
Expand Down
20 changes: 10 additions & 10 deletions compiler/rustc_hir/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2116,7 +2116,7 @@ impl Expr<'_> {
ExprKind::Struct(_, fields, init) => {
let init_side_effects = match init {
StructTailExpr::Base(init) => init.can_have_side_effects(),
StructTailExpr::DefaultFields(_) | StructTailExpr::None => false,
StructTailExpr::DefaultFields(_) | StructTailExpr::None(_) => false,
};
fields.iter().map(|field| field.expr).any(|e| e.can_have_side_effects())
|| init_side_effects
Expand Down Expand Up @@ -2191,48 +2191,48 @@ impl Expr<'_> {
ExprKind::Struct(
QPath::LangItem(LangItem::RangeTo, _),
[val1],
StructTailExpr::None,
StructTailExpr::None(_),
),
ExprKind::Struct(
QPath::LangItem(LangItem::RangeTo, _),
[val2],
StructTailExpr::None,
StructTailExpr::None(_),
),
)
| (
ExprKind::Struct(
QPath::LangItem(LangItem::RangeToInclusive, _),
[val1],
StructTailExpr::None,
StructTailExpr::None(_),
),
ExprKind::Struct(
QPath::LangItem(LangItem::RangeToInclusive, _),
[val2],
StructTailExpr::None,
StructTailExpr::None(_),
),
)
| (
ExprKind::Struct(
QPath::LangItem(LangItem::RangeFrom, _),
[val1],
StructTailExpr::None,
StructTailExpr::None(_),
),
ExprKind::Struct(
QPath::LangItem(LangItem::RangeFrom, _),
[val2],
StructTailExpr::None,
StructTailExpr::None(_),
),
) => val1.expr.equivalent_for_indexing(val2.expr),
(
ExprKind::Struct(
QPath::LangItem(LangItem::Range, _),
[val1, val3],
StructTailExpr::None,
StructTailExpr::None(_),
),
ExprKind::Struct(
QPath::LangItem(LangItem::Range, _),
[val2, val4],
StructTailExpr::None,
StructTailExpr::None(_),
),
) => {
val1.expr.equivalent_for_indexing(val2.expr)
Expand Down Expand Up @@ -2428,7 +2428,7 @@ pub enum ExprKind<'hir> {
#[derive(Debug, Clone, Copy, HashStable_Generic)]
pub enum StructTailExpr<'hir> {
/// A struct expression where all the fields are explicitly enumerated: `Foo { a, b }`.
None,
None(Span),
/// A struct expression with a "base", an expression of the same type as the outer struct that
/// will be used to populate any fields not explicitly mentioned: `Foo { ..base }`
Base(&'hir Expr<'hir>),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir/src/intravisit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -749,7 +749,7 @@ pub fn walk_expr<'v, V: Visitor<'v>>(visitor: &mut V, expression: &'v Expr<'v>)
walk_list!(visitor, visit_expr_field, fields);
match optional_base {
StructTailExpr::Base(base) => try_visit!(visitor.visit_expr(base)),
StructTailExpr::None | StructTailExpr::DefaultFields(_) => {}
StructTailExpr::None(_) | StructTailExpr::DefaultFields(_) => {}
}
}
ExprKind::Tup(subexpressions) => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_pretty/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1226,7 +1226,7 @@ impl<'a> State<'a> {
self.word("..");
self.end();
}
hir::StructTailExpr::None => {
hir::StructTailExpr::None(_) => {
if !fields.is_empty() {
self.word(",");
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_typeck/src/expr_use_visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -707,7 +707,7 @@ impl<'tcx, Cx: TypeInformationCtxt<'tcx>, D: Delegate<'tcx>> ExprUseVisitor<'tcx

let with_expr = match *opt_with {
hir::StructTailExpr::Base(w) => &*w,
hir::StructTailExpr::DefaultFields(_) | hir::StructTailExpr::None => {
hir::StructTailExpr::DefaultFields(_) | hir::StructTailExpr::None(_) => {
return Ok(());
}
};
Expand Down
Loading

0 comments on commit be8b02f

Please sign in to comment.