Skip to content

Commit 1c42dbb

Browse files
khueykhuey
committed
Expand derivable-impls to cover enums with a default unit variant.
1 parent d5d8ef1 commit 1c42dbb

File tree

4 files changed

+167
-43
lines changed

4 files changed

+167
-43
lines changed

‎clippy_lints/src/derivable_impls.rs

Lines changed: 101 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
use clippy_utils::diagnostics::span_lint_and_then;
2+
use clippy_utils::source::indent_of;
23
use clippy_utils::{is_default_equivalent, peel_blocks};
34
use rustc_errors::Applicability;
45
use rustc_hir::{
5-
def::{DefKind, Res},
6-
Body, Expr, ExprKind, GenericArg, Impl, ImplItemKind, Item, ItemKind, Node, PathSegment, QPath, TyKind,
6+
def::{CtorKind, CtorOf, DefKind, Res},
7+
Body, Expr, ExprKind, GenericArg, Impl, ImplItemKind, Item, ItemKind, Node, PathSegment, QPath, Ty, TyKind,
78
};
89
use rustc_lint::{LateContext, LateLintPass};
10+
use rustc_middle::ty::{AdtDef, DefIdTree};
911
use rustc_session::{declare_lint_pass, declare_tool_lint};
1012
use rustc_span::sym;
1113

@@ -61,6 +63,98 @@ fn is_path_self(e: &Expr<'_>) -> bool {
6163
}
6264
}
6365

66+
fn check_struct<'tcx>(
67+
cx: &LateContext<'tcx>,
68+
item: &'tcx Item<'_>,
69+
self_ty: &Ty<'_>,
70+
func_expr: &Expr<'_>,
71+
adt_def: AdtDef<'_>,
72+
) {
73+
if let TyKind::Path(QPath::Resolved(_, p)) = self_ty.kind {
74+
if let Some(PathSegment { args: Some(a), .. }) = p.segments.last() {
75+
for arg in a.args {
76+
if !matches!(arg, GenericArg::Lifetime(_)) {
77+
return;
78+
}
79+
}
80+
}
81+
}
82+
let should_emit = match peel_blocks(func_expr).kind {
83+
ExprKind::Tup(fields) => fields.iter().all(|e| is_default_equivalent(cx, e)),
84+
ExprKind::Call(callee, args) if is_path_self(callee) => args.iter().all(|e| is_default_equivalent(cx, e)),
85+
ExprKind::Struct(_, fields, _) => fields.iter().all(|ef| is_default_equivalent(cx, ef.expr)),
86+
_ => false,
87+
};
88+
89+
if should_emit {
90+
let struct_span = cx.tcx.def_span(adt_def.did());
91+
span_lint_and_then(cx, DERIVABLE_IMPLS, item.span, "this `impl` can be derived", |diag| {
92+
diag.span_suggestion_hidden(
93+
item.span,
94+
"remove the manual implementation...",
95+
String::new(),
96+
Applicability::MachineApplicable,
97+
);
98+
diag.span_suggestion(
99+
struct_span.shrink_to_lo(),
100+
"...and instead derive it",
101+
"#[derive(Default)]\n".to_string(),
102+
Applicability::MachineApplicable,
103+
);
104+
});
105+
}
106+
}
107+
108+
fn check_enum<'tcx>(cx: &LateContext<'tcx>, item: &'tcx Item<'_>, func_expr: &Expr<'_>, adt_def: AdtDef<'_>) {
109+
if_chain! {
110+
if let ExprKind::Path(QPath::Resolved(None, p)) = &peel_blocks(func_expr).kind;
111+
if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), id) = p.res;
112+
if let variant_id = cx.tcx.parent(id);
113+
if let Some(variant_def) = adt_def.variants().iter().find(|v| v.def_id == variant_id);
114+
if variant_def.fields.is_empty();
115+
if !variant_def.is_field_list_non_exhaustive();
116+
117+
then {
118+
let enum_span = cx.tcx.def_span(adt_def.did());
119+
let indent_enum = indent_of(cx, enum_span).unwrap_or(0);
120+
let variant_span = cx.tcx.def_span(variant_def.def_id);
121+
let indent_variant = indent_of(cx, variant_span).unwrap_or(0);
122+
span_lint_and_then(
123+
cx,
124+
DERIVABLE_IMPLS,
125+
item.span,
126+
"this `impl` can be derived",
127+
|diag| {
128+
diag.span_suggestion_hidden(
129+
item.span,
130+
"remove the manual implementation...",
131+
String::new(),
132+
Applicability::MachineApplicable
133+
);
134+
diag.span_suggestion(
135+
enum_span.shrink_to_lo(),
136+
"...and instead derive it...",
137+
format!(
138+
"#[derive(Default)]\n{indent}",
139+
indent = " ".repeat(indent_enum),
140+
),
141+
Applicability::MachineApplicable
142+
);
143+
diag.span_suggestion(
144+
variant_span.shrink_to_lo(),
145+
"...and mark the default variant",
146+
format!(
147+
"#[default]\n{indent}",
148+
indent = " ".repeat(indent_variant),
149+
),
150+
Applicability::MachineApplicable
151+
);
152+
}
153+
);
154+
}
155+
}
156+
}
157+
64158
impl<'tcx> LateLintPass<'tcx> for DerivableImpls {
65159
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
66160
if_chain! {
@@ -83,47 +177,12 @@ impl<'tcx> LateLintPass<'tcx> for DerivableImpls {
83177
if !attrs.iter().any(|attr| attr.doc_str().is_some());
84178
if let child_attrs = cx.tcx.hir().attrs(impl_item_hir);
85179
if !child_attrs.iter().any(|attr| attr.doc_str().is_some());
86-
if adt_def.is_struct();
87-
then {
88-
if let TyKind::Path(QPath::Resolved(_, p)) = self_ty.kind {
89-
if let Some(PathSegment { args: Some(a), .. }) = p.segments.last() {
90-
for arg in a.args {
91-
if !matches!(arg, GenericArg::Lifetime(_)) {
92-
return;
93-
}
94-
}
95-
}
96-
}
97-
let should_emit = match peel_blocks(func_expr).kind {
98-
ExprKind::Tup(fields) => fields.iter().all(|e| is_default_equivalent(cx, e)),
99-
ExprKind::Call(callee, args)
100-
if is_path_self(callee) => args.iter().all(|e| is_default_equivalent(cx, e)),
101-
ExprKind::Struct(_, fields, _) => fields.iter().all(|ef| is_default_equivalent(cx, ef.expr)),
102-
_ => false,
103-
};
104180

105-
if should_emit {
106-
let struct_span = cx.tcx.def_span(adt_def.did());
107-
span_lint_and_then(
108-
cx,
109-
DERIVABLE_IMPLS,
110-
item.span,
111-
"this `impl` can be derived",
112-
|diag| {
113-
diag.span_suggestion_hidden(
114-
item.span,
115-
"remove the manual implementation...",
116-
String::new(),
117-
Applicability::MachineApplicable
118-
);
119-
diag.span_suggestion(
120-
struct_span.shrink_to_lo(),
121-
"...and instead derive it",
122-
"#[derive(Default)]\n".to_string(),
123-
Applicability::MachineApplicable
124-
);
125-
}
126-
);
181+
then {
182+
if adt_def.is_struct() {
183+
check_struct(cx, item, self_ty, func_expr, adt_def);
184+
} else if adt_def.is_enum() {
185+
check_enum(cx, item, func_expr, adt_def);
127186
}
128187
}
129188
}

‎tests/ui/derivable_impls.fixed

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,4 +210,25 @@ impl Default for IntOrString {
210210
}
211211
}
212212

213+
#[derive(Default)]
214+
pub enum SimpleEnum {
215+
Foo,
216+
#[default]
217+
Bar,
218+
}
219+
220+
221+
222+
pub enum NonExhaustiveEnum {
223+
Foo,
224+
#[non_exhaustive]
225+
Bar,
226+
}
227+
228+
impl Default for NonExhaustiveEnum {
229+
fn default() -> Self {
230+
NonExhaustiveEnum::Bar
231+
}
232+
}
233+
213234
fn main() {}

‎tests/ui/derivable_impls.rs

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,4 +244,27 @@ impl Default for IntOrString {
244244
}
245245
}
246246

247+
pub enum SimpleEnum {
248+
Foo,
249+
Bar,
250+
}
251+
252+
impl Default for SimpleEnum {
253+
fn default() -> Self {
254+
SimpleEnum::Bar
255+
}
256+
}
257+
258+
pub enum NonExhaustiveEnum {
259+
Foo,
260+
#[non_exhaustive]
261+
Bar,
262+
}
263+
264+
impl Default for NonExhaustiveEnum {
265+
fn default() -> Self {
266+
NonExhaustiveEnum::Bar
267+
}
268+
}
269+
247270
fn main() {}

‎tests/ui/derivable_impls.stderr

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,5 +113,26 @@ help: ...and instead derive it
113113
LL | #[derive(Default)]
114114
|
115115

116-
error: aborting due to 7 previous errors
116+
error: this `impl` can be derived
117+
--> $DIR/derivable_impls.rs:252:1
118+
|
119+
LL | / impl Default for SimpleEnum {
120+
LL | | fn default() -> Self {
121+
LL | | SimpleEnum::Bar
122+
LL | | }
123+
LL | | }
124+
| |_^
125+
|
126+
= help: remove the manual implementation...
127+
help: ...and instead derive it...
128+
|
129+
LL | #[derive(Default)]
130+
|
131+
help: ...and mark the default variant
132+
|
133+
LL ~ #[default]
134+
LL ~ Bar,
135+
|
136+
137+
error: aborting due to 8 previous errors
117138

0 commit comments

Comments
 (0)