Skip to content

Commit 303e05e

Browse files
estebankestebank
committed
Detect when user uses default unit variant in default impl
When checking if a field being defaulted to a unit variant, look at the (local) `Default` impl for that enum and if they match, consider that field defaultable for the purposes of the lint.
1 parent 0930485 commit 303e05e

File tree

4 files changed

+114
-31
lines changed

4 files changed

+114
-31
lines changed

‎compiler/rustc_lint/src/default_could_be_derived.rs‎

Lines changed: 81 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,8 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived {
4141
let hir::ItemKind::Impl(data) = item.kind else { return };
4242
let Some(trait_ref) = data.of_trait else { return };
4343
let Res::Def(DefKind::Trait, def_id) = trait_ref.path.res else { return };
44-
if Some(def_id) != cx.tcx.get_diagnostic_item(sym::Default) {
44+
let Some(default_def_id) = cx.tcx.get_diagnostic_item(sym::Default) else { return };
45+
if Some(def_id) != Some(default_def_id) {
4546
return;
4647
}
4748
if cx.tcx.has_attr(def_id, sym::automatically_derived) {
@@ -137,6 +138,9 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived {
137138
// - `val` is `0`
138139
// - `val` is `false`
139140
fn check_expr(tcx: TyCtxt<'_>, kind: hir::ExprKind<'_>) -> bool {
141+
let Some(default_def_id) = tcx.get_diagnostic_item(sym::Default) else {
142+
return false;
143+
};
140144
match kind {
141145
hir::ExprKind::Lit(spanned_lit) => match spanned_lit.node {
142146
LitKind::Int(val, _) if val == 0 => true, // field: 0,
@@ -155,15 +159,84 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived {
155159
hir::ExprKind::Path(hir::QPath::Resolved(_, path))
156160
if let Res::Def(
157161
DefKind::Ctor(CtorOf::Variant, CtorKind::Const),
158-
def_id,
159-
) = path.res
160-
&& let def_id = tcx.parent(def_id) // From Ctor to variant
161-
&& tcx.is_lang_item(def_id, hir::LangItem::OptionNone) =>
162+
ctor_def_id,
163+
) = path.res =>
162164
{
163165
// FIXME: We should use a better check where we explore existing
164-
// `impl Default for def_id` of the found type and see compare them
165-
// against what we have here. For now, special case `Option::None`.
166-
true
166+
// `impl Default for def_id` of the found type when `def_id` is not
167+
// local and see compare them against what we have here. For now,
168+
// we special case `Option::None` and only check unit variants of
169+
// local `Default` impls.
170+
let var_def_id = tcx.parent(ctor_def_id); // From Ctor to variant
171+
172+
// We explicitly check for `Option::<T>::None`. If `Option` was
173+
// local, it would be accounted by the logic further down, but
174+
// because the analysis uses purely the HIR, that doesn't work
175+
// accross crates.
176+
//
177+
// field: None,
178+
let mut found =
179+
tcx.is_lang_item(var_def_id, hir::LangItem::OptionNone);
180+
181+
// Look at the local `impl Default for ty` of the field's `ty`.
182+
let ty_def_id = tcx.parent(var_def_id); // From variant to enum
183+
let ty = tcx.type_of(ty_def_id).instantiate_identity();
184+
tcx.for_each_relevant_impl(default_def_id, ty, |impl_did| {
185+
let hir = tcx.hir();
186+
let Some(hir::Node::Item(impl_item)) =
187+
hir.get_if_local(impl_did)
188+
else {
189+
return;
190+
};
191+
let hir::ItemKind::Impl(impl_item) = impl_item.kind else {
192+
return;
193+
};
194+
for assoc in impl_item.items {
195+
let hir::AssocItemKind::Fn { has_self: false } = assoc.kind
196+
else {
197+
continue;
198+
};
199+
if assoc.ident.name != kw::Default {
200+
continue;
201+
}
202+
let assoc = hir.impl_item(assoc.id);
203+
let hir::ImplItemKind::Fn(_ty, body) = assoc.kind else {
204+
continue;
205+
};
206+
let body = hir.body(body);
207+
let hir::ExprKind::Block(
208+
hir::Block { stmts: [], expr: Some(expr), .. },
209+
None,
210+
) = body.value.kind
211+
else {
212+
continue;
213+
};
214+
// Look at a specific implementation of `Default::default()`
215+
// for their content and see if they are requivalent to what
216+
// the user wrote in their manual `impl` for a given field.
217+
match expr.kind {
218+
hir::ExprKind::Path(hir::QPath::Resolved(_, path))
219+
if let Res::Def(
220+
DefKind::Ctor(CtorOf::Variant, CtorKind::Const),
221+
orig_def_id,
222+
) = path.res =>
223+
{
224+
// We found
225+
//
226+
// field: Foo::Unit,
227+
//
228+
// and
229+
//
230+
// impl Default for Foo {
231+
// fn default() -> Foo { Foo::Unit }
232+
// }
233+
found |= orig_def_id == ctor_def_id
234+
}
235+
_ => {}
236+
}
237+
}
238+
});
239+
found
167240
}
168241
_ => false,
169242
}

‎tests/ui/structs/manual-default-impl-could-be-derived.fixed‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -47,17 +47,10 @@
4747

4848

4949
// Comparison of `impl` *fields* with their `Default::default()` bodies.
50-
// struct G {
51-
// f: F,
52-
// }
50+
#[derive(Default)] struct G {
51+
f: F,
52+
}
5353

54-
// impl Default for G {
55-
// fn default() -> Self {
56-
// G {
57-
// f: F::Unit,
58-
// }
59-
// }
60-
// }
6154

6255
fn main() {
6356
// let _ = A::default();
@@ -66,5 +59,5 @@ fn main() {
6659
let _ = D::default();
6760
let _ = E::default();
6861
let _ = F::default();
69-
// let _ = G::default();
62+
let _ = G::default();
7063
}

‎tests/ui/structs/manual-default-impl-could-be-derived.rs‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -67,17 +67,17 @@ impl Default for F { //~ ERROR
6767
}
6868

6969
// Comparison of `impl` *fields* with their `Default::default()` bodies.
70-
// struct G {
71-
// f: F,
72-
// }
70+
struct G {
71+
f: F,
72+
}
7373

74-
// impl Default for G {
75-
// fn default() -> Self {
76-
// G {
77-
// f: F::Unit,
78-
// }
79-
// }
80-
// }
74+
impl Default for G { //~ ERROR
75+
fn default() -> Self {
76+
G {
77+
f: F::Unit,
78+
}
79+
}
80+
}
8181

8282
fn main() {
8383
// let _ = A::default();
@@ -86,5 +86,5 @@ fn main() {
8686
let _ = D::default();
8787
let _ = E::default();
8888
let _ = F::default();
89-
// let _ = G::default();
89+
let _ = G::default();
9090
}

‎tests/ui/structs/manual-default-impl-could-be-derived.stderr‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,5 +53,22 @@ LL ~ #[derive(Default)] enum F {
5353
LL ~ #[default] Unit,
5454
|
5555

56-
error: aborting due to 3 previous errors
56+
error: `impl Default` that could be derived
57+
--> $DIR/manual-default-impl-could-be-derived.rs:74:1
58+
|
59+
LL | / impl Default for G {
60+
LL | | fn default() -> Self {
61+
LL | | G {
62+
LL | | f: F::Unit,
63+
LL | | }
64+
LL | | }
65+
LL | | }
66+
| |_^
67+
|
68+
help: you don't need to manually `impl Default`, you can derive it
69+
|
70+
LL ~ #[derive(Default)] struct G {
71+
|
72+
73+
error: aborting due to 4 previous errors
5774

0 commit comments

Comments
 (0)