Skip to content

Commit c4896ee

Browse files
estebankestebank
committed
Extend Default lint to tuple-structs
1 parent 7eebd59 commit c4896ee

4 files changed

+183
-140
lines changed

‎compiler/rustc_lint/src/default_could_be_derived.rs

+125-104
Original file line numberDiff line numberDiff line change
@@ -213,110 +213,6 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived {
213213
// - `val` is `Default::default()`
214214
// - `val` is `0`
215215
// - `val` is `false`
216-
fn check_expr(tcx: TyCtxt<'_>, kind: hir::ExprKind<'_>) -> bool {
217-
let Some(default_def_id) = tcx.get_diagnostic_item(sym::Default) else {
218-
return false;
219-
};
220-
match kind {
221-
hir::ExprKind::Lit(spanned_lit) => match spanned_lit.node {
222-
LitKind::Int(val, _) if val == 0 => true, // field: 0,
223-
LitKind::Bool(false) => true, // field: false,
224-
_ => false,
225-
},
226-
hir::ExprKind::Call(expr, [])
227-
if let hir::ExprKind::Path(hir::QPath::Resolved(_, path)) =
228-
expr.kind
229-
&& let Some(def_id) = path.res.opt_def_id()
230-
&& tcx.is_diagnostic_item(sym::default_fn, def_id) =>
231-
{
232-
// field: Default::default(),
233-
true
234-
}
235-
hir::ExprKind::Path(hir::QPath::Resolved(_, path))
236-
if let Res::Def(
237-
DefKind::Ctor(CtorOf::Variant, CtorKind::Const),
238-
ctor_def_id,
239-
) = path.res =>
240-
{
241-
// FIXME: We should use a better check where we explore existing
242-
// `impl Default for def_id` of the found type when `def_id` is not
243-
// local and see compare them against what we have here. For now,
244-
// we special case `Option::None` and only check unit variants of
245-
// local `Default` impls.
246-
let var_def_id = tcx.parent(ctor_def_id); // From Ctor to variant
247-
248-
// We explicitly check for `Option::<T>::None`. If `Option` was
249-
// local, it would be accounted by the logic further down, but
250-
// because the analysis uses purely the HIR, that doesn't work
251-
// accross crates.
252-
//
253-
// field: None,
254-
let mut found =
255-
tcx.is_lang_item(var_def_id, hir::LangItem::OptionNone);
256-
257-
// Look at the local `impl Default for ty` of the field's `ty`.
258-
let ty_def_id = tcx.parent(var_def_id); // From variant to enum
259-
let ty = tcx.type_of(ty_def_id).instantiate_identity();
260-
tcx.for_each_relevant_impl(default_def_id, ty, |impl_did| {
261-
let hir = tcx.hir();
262-
let Some(hir::Node::Item(impl_item)) =
263-
hir.get_if_local(impl_did)
264-
else {
265-
return;
266-
};
267-
let hir::ItemKind::Impl(impl_item) = impl_item.kind else {
268-
return;
269-
};
270-
for assoc in impl_item.items {
271-
let hir::AssocItemKind::Fn { has_self: false } = assoc.kind
272-
else {
273-
continue;
274-
};
275-
if assoc.ident.name != kw::Default {
276-
continue;
277-
}
278-
let assoc = hir.impl_item(assoc.id);
279-
let hir::ImplItemKind::Fn(_ty, body) = assoc.kind else {
280-
continue;
281-
};
282-
let body = hir.body(body);
283-
let hir::ExprKind::Block(
284-
hir::Block { stmts: [], expr: Some(expr), .. },
285-
None,
286-
) = body.value.kind
287-
else {
288-
continue;
289-
};
290-
// Look at a specific implementation of `Default::default()`
291-
// for their content and see if they are requivalent to what
292-
// the user wrote in their manual `impl` for a given field.
293-
match expr.kind {
294-
hir::ExprKind::Path(hir::QPath::Resolved(_, path))
295-
if let Res::Def(
296-
DefKind::Ctor(CtorOf::Variant, CtorKind::Const),
297-
orig_def_id,
298-
) = path.res =>
299-
{
300-
// We found
301-
//
302-
// field: Foo::Unit,
303-
//
304-
// and
305-
//
306-
// impl Default for Foo {
307-
// fn default() -> Foo { Foo::Unit }
308-
// }
309-
found |= orig_def_id == ctor_def_id
310-
}
311-
_ => {}
312-
}
313-
}
314-
});
315-
found
316-
}
317-
_ => false,
318-
}
319-
}
320216
if fields.iter().all(|f| check_expr(cx.tcx, f.expr.kind)) {
321217
cx.tcx.node_span_lint(
322218
DEFAULT_COULD_BE_DERIVED,
@@ -339,8 +235,133 @@ impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived {
339235
);
340236
}
341237
}
238+
hir::ExprKind::Call(expr, args) => {
239+
if let hir::ExprKind::Path(hir::QPath::Resolved(_, path)) = expr.kind
240+
&& let Res::Def(DefKind::Ctor(CtorOf::Struct, CtorKind::Fn), ctor_def_id) =
241+
path.res
242+
{
243+
let type_def_id = cx.tcx.parent(ctor_def_id); // From Ctor to struct
244+
if args.iter().all(|expr| check_expr(cx.tcx, expr.kind)) {
245+
cx.tcx.node_span_lint(
246+
DEFAULT_COULD_BE_DERIVED,
247+
item.hir_id(),
248+
item.span,
249+
|diag| {
250+
diag.primary_message("`impl Default` that could be derived");
251+
diag.multipart_suggestion_verbose(
252+
"you don't need to manually `impl Default`, you can derive it",
253+
vec![
254+
(
255+
cx.tcx.def_span(type_def_id).shrink_to_lo(),
256+
"#[derive(Default)] ".to_string(),
257+
),
258+
(item.span, String::new()),
259+
],
260+
Applicability::MachineApplicable,
261+
);
262+
},
263+
);
264+
}
265+
}
266+
}
342267
_ => {}
343268
}
344269
}
345270
}
346271
}
272+
273+
fn check_expr(tcx: TyCtxt<'_>, kind: hir::ExprKind<'_>) -> bool {
274+
let Some(default_def_id) = tcx.get_diagnostic_item(sym::Default) else {
275+
return false;
276+
};
277+
match kind {
278+
hir::ExprKind::Lit(spanned_lit) => match spanned_lit.node {
279+
LitKind::Int(val, _) if val == 0 => true, // field: 0,
280+
LitKind::Bool(false) => true, // field: false,
281+
_ => false,
282+
},
283+
hir::ExprKind::Call(expr, [])
284+
if let hir::ExprKind::Path(hir::QPath::Resolved(_, path)) = expr.kind
285+
&& let Some(def_id) = path.res.opt_def_id()
286+
&& tcx.is_diagnostic_item(sym::default_fn, def_id) =>
287+
{
288+
// field: Default::default(),
289+
true
290+
}
291+
hir::ExprKind::Path(hir::QPath::Resolved(_, path))
292+
if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), ctor_def_id) =
293+
path.res =>
294+
{
295+
// FIXME: We should use a better check where we explore existing
296+
// `impl Default for def_id` of the found type when `def_id` is not
297+
// local and see compare them against what we have here. For now,
298+
// we special case `Option::None` and only check unit variants of
299+
// local `Default` impls.
300+
let var_def_id = tcx.parent(ctor_def_id); // From Ctor to variant
301+
302+
// We explicitly check for `Option::<T>::None`. If `Option` was
303+
// local, it would be accounted by the logic further down, but
304+
// because the analysis uses purely the HIR, that doesn't work
305+
// accross crates.
306+
//
307+
// field: None,
308+
let mut found = tcx.is_lang_item(var_def_id, hir::LangItem::OptionNone);
309+
310+
// Look at the local `impl Default for ty` of the field's `ty`.
311+
let ty_def_id = tcx.parent(var_def_id); // From variant to enum
312+
let ty = tcx.type_of(ty_def_id).instantiate_identity();
313+
tcx.for_each_relevant_impl(default_def_id, ty, |impl_did| {
314+
let hir = tcx.hir();
315+
let Some(hir::Node::Item(impl_item)) = hir.get_if_local(impl_did) else {
316+
return;
317+
};
318+
let hir::ItemKind::Impl(impl_item) = impl_item.kind else {
319+
return;
320+
};
321+
for assoc in impl_item.items {
322+
let hir::AssocItemKind::Fn { has_self: false } = assoc.kind else {
323+
continue;
324+
};
325+
if assoc.ident.name != kw::Default {
326+
continue;
327+
}
328+
let assoc = hir.impl_item(assoc.id);
329+
let hir::ImplItemKind::Fn(_ty, body) = assoc.kind else {
330+
continue;
331+
};
332+
let body = hir.body(body);
333+
let hir::ExprKind::Block(hir::Block { stmts: [], expr: Some(expr), .. }, None) =
334+
body.value.kind
335+
else {
336+
continue;
337+
};
338+
// Look at a specific implementation of `Default::default()`
339+
// for their content and see if they are requivalent to what
340+
// the user wrote in their manual `impl` for a given field.
341+
match expr.kind {
342+
hir::ExprKind::Path(hir::QPath::Resolved(_, path))
343+
if let Res::Def(
344+
DefKind::Ctor(CtorOf::Variant, CtorKind::Const),
345+
orig_def_id,
346+
) = path.res =>
347+
{
348+
// We found
349+
//
350+
// field: Foo::Unit,
351+
//
352+
// and
353+
//
354+
// impl Default for Foo {
355+
// fn default() -> Foo { Foo::Unit }
356+
// }
357+
found |= orig_def_id == ctor_def_id
358+
}
359+
_ => {}
360+
}
361+
}
362+
});
363+
found
364+
}
365+
_ => false,
366+
}
367+
}

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

+8-14
Original file line numberDiff line numberDiff line change
@@ -11,20 +11,14 @@
1111
// fn default() -> Self { A }
1212
// }
1313
//
14-
// #[derive(Debug)]
15-
// struct B(Option<i32>);
16-
//
17-
// impl Default for B {
18-
// fn default() -> Self { B(Default::default()) }
19-
// }
20-
//
21-
// #[derive(Debug)]
22-
// struct C(Option<i32>);
23-
//
24-
// impl Default for C {
25-
// fn default() -> Self { C(None) }
26-
// }
27-
//
14+
#[derive(Debug)]
15+
#[derive(Default)] struct B(Option<i32>);
16+
17+
18+
#[derive(Debug)]
19+
#[derive(Default)] struct C(Option<i32>);
20+
21+
2822

2923
// Explicit check against numeric literals and `Default::default()` calls.
3024
#[derive(Default)] struct D {

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

+16-16
Original file line numberDiff line numberDiff line change
@@ -11,20 +11,20 @@
1111
// fn default() -> Self { A }
1212
// }
1313
//
14-
// #[derive(Debug)]
15-
// struct B(Option<i32>);
16-
//
17-
// impl Default for B {
18-
// fn default() -> Self { B(Default::default()) }
19-
// }
20-
//
21-
// #[derive(Debug)]
22-
// struct C(Option<i32>);
23-
//
24-
// impl Default for C {
25-
// fn default() -> Self { C(None) }
26-
// }
27-
//
14+
#[derive(Debug)]
15+
struct B(Option<i32>);
16+
17+
impl Default for B { //~ ERROR
18+
fn default() -> Self { B(Default::default()) }
19+
}
20+
21+
#[derive(Debug)]
22+
struct C(Option<i32>);
23+
24+
impl Default for C { //~ ERROR
25+
fn default() -> Self { C(None) }
26+
}
27+
2828

2929
// Explicit check against numeric literals and `Default::default()` calls.
3030
struct D {
@@ -112,8 +112,8 @@ impl Default for I { //~ ERROR
112112

113113
fn main() {
114114
// let _ = A::default();
115-
// let _ = B::default();
116-
// let _ = C::default();
115+
let _ = B::default();
116+
let _ = C::default();
117117
let _ = D::default();
118118
let _ = E::default();
119119
let _ = F::<i32>::default();

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

+34-6
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,36 @@
1+
error: `impl Default` that could be derived
2+
--> $DIR/manual-default-impl-could-be-derived.rs:17:1
3+
|
4+
LL | / impl Default for B {
5+
LL | | fn default() -> Self { B(Default::default()) }
6+
LL | | }
7+
| |_^
8+
|
9+
note: the lint level is defined here
10+
--> $DIR/manual-default-impl-could-be-derived.rs:4:9
11+
|
12+
LL | #![deny(default_could_be_derived)]
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^
14+
help: you don't need to manually `impl Default`, you can derive it
15+
|
16+
LL - struct B(Option<i32>);
17+
LL + #[derive(Default)] struct B(Option<i32>);
18+
|
19+
20+
error: `impl Default` that could be derived
21+
--> $DIR/manual-default-impl-could-be-derived.rs:24:1
22+
|
23+
LL | / impl Default for C {
24+
LL | | fn default() -> Self { C(None) }
25+
LL | | }
26+
| |_^
27+
|
28+
help: you don't need to manually `impl Default`, you can derive it
29+
|
30+
LL - struct C(Option<i32>);
31+
LL + #[derive(Default)] struct C(Option<i32>);
32+
|
33+
134
error: `impl Default` that could be derived
235
--> $DIR/manual-default-impl-could-be-derived.rs:35:1
336
|
@@ -10,11 +43,6 @@ LL | | }
1043
LL | | }
1144
| |_^
1245
|
13-
note: the lint level is defined here
14-
--> $DIR/manual-default-impl-could-be-derived.rs:4:9
15-
|
16-
LL | #![deny(default_could_be_derived)]
17-
| ^^^^^^^^^^^^^^^^^^^^^^^^
1846
help: you don't need to manually `impl Default`, you can derive it
1947
|
2048
LL ~ #[derive(Default)] struct D {
@@ -116,5 +144,5 @@ help: to avoid divergence in behavior between `Struct { .. }` and `<Struct as De
116144
LL ~ #[derive(Default)] struct I {
117145
|
118146

119-
error: aborting due to 6 previous errors
147+
error: aborting due to 8 previous errors
120148

0 commit comments

Comments
 (0)