Auto merge of #10161 - khuey:default_enum_unit_variant, r=llogiq

Expand derivable-impls to cover enums with a default unit variant. *Please write a short comment explaining your change (or "none" for internal only changes)* changelog: [`derivable_impls`]: suggest deriving Default for enums with a default unit variant
rust-lang · Jan 5, 2023 · 61ff54e · 61ff54e
2 parents d5d8ef1 + 1c42dbb
commit 61ff54e
Show file tree

Hide file tree

Showing 4 changed files with 167 additions and 43 deletions.
diff --git a/clippy_lints/src/derivable_impls.rs b/clippy_lints/src/derivable_impls.rs
@@ -1,11 +1,13 @@
 use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::source::indent_of;
 use clippy_utils::{is_default_equivalent, peel_blocks};
 use rustc_errors::Applicability;
 use rustc_hir::{
-    def::{DefKind, Res},
-    Body, Expr, ExprKind, GenericArg, Impl, ImplItemKind, Item, ItemKind, Node, PathSegment, QPath, TyKind,
+    def::{CtorKind, CtorOf, DefKind, Res},
+    Body, Expr, ExprKind, GenericArg, Impl, ImplItemKind, Item, ItemKind, Node, PathSegment, QPath, Ty, TyKind,
 };
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::{AdtDef, DefIdTree};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::sym;
 
@@ -61,6 +63,98 @@ fn is_path_self(e: &Expr<'_>) -> bool {
     }
 }
 
+fn check_struct<'tcx>(
+    cx: &LateContext<'tcx>,
+    item: &'tcx Item<'_>,
+    self_ty: &Ty<'_>,
+    func_expr: &Expr<'_>,
+    adt_def: AdtDef<'_>,
+) {
+    if let TyKind::Path(QPath::Resolved(_, p)) = self_ty.kind {
+        if let Some(PathSegment { args: Some(a), .. }) = p.segments.last() {
+            for arg in a.args {
+                if !matches!(arg, GenericArg::Lifetime(_)) {
+                    return;
+                }
+            }
+        }
+    }
+    let should_emit = match peel_blocks(func_expr).kind {
+        ExprKind::Tup(fields) => fields.iter().all(|e| is_default_equivalent(cx, e)),
+        ExprKind::Call(callee, args) if is_path_self(callee) => args.iter().all(|e| is_default_equivalent(cx, e)),
+        ExprKind::Struct(_, fields, _) => fields.iter().all(|ef| is_default_equivalent(cx, ef.expr)),
+        _ => false,
+    };
+
+    if should_emit {
+        let struct_span = cx.tcx.def_span(adt_def.did());
+        span_lint_and_then(cx, DERIVABLE_IMPLS, item.span, "this `impl` can be derived", |diag| {
+            diag.span_suggestion_hidden(
+                item.span,
+                "remove the manual implementation...",
+                String::new(),
+                Applicability::MachineApplicable,
+            );
+            diag.span_suggestion(
+                struct_span.shrink_to_lo(),
+                "...and instead derive it",
+                "#[derive(Default)]\n".to_string(),
+                Applicability::MachineApplicable,
+            );
+        });
+    }
+}
+
+fn check_enum<'tcx>(cx: &LateContext<'tcx>, item: &'tcx Item<'_>, func_expr: &Expr<'_>, adt_def: AdtDef<'_>) {
+    if_chain! {
+        if let ExprKind::Path(QPath::Resolved(None, p)) = &peel_blocks(func_expr).kind;
+        if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), id) = p.res;
+        if let variant_id = cx.tcx.parent(id);
+        if let Some(variant_def) = adt_def.variants().iter().find(|v| v.def_id == variant_id);
+        if variant_def.fields.is_empty();
+        if !variant_def.is_field_list_non_exhaustive();
+
+        then {
+            let enum_span = cx.tcx.def_span(adt_def.did());
+            let indent_enum = indent_of(cx, enum_span).unwrap_or(0);
+            let variant_span = cx.tcx.def_span(variant_def.def_id);
+            let indent_variant = indent_of(cx, variant_span).unwrap_or(0);
+            span_lint_and_then(
+                cx,
+                DERIVABLE_IMPLS,
+                item.span,
+                "this `impl` can be derived",
+                |diag| {
+                    diag.span_suggestion_hidden(
+                        item.span,
+                        "remove the manual implementation...",
+                        String::new(),
+                        Applicability::MachineApplicable
+                    );
+                    diag.span_suggestion(
+                        enum_span.shrink_to_lo(),
+                        "...and instead derive it...",
+                        format!(
+                            "#[derive(Default)]\n{indent}",
+                            indent = " ".repeat(indent_enum),
+                        ),
+                        Applicability::MachineApplicable
+                    );
+                    diag.span_suggestion(
+                        variant_span.shrink_to_lo(),
+                        "...and mark the default variant",
+                        format!(
+                            "#[default]\n{indent}",
+                            indent = " ".repeat(indent_variant),
+                        ),
+                        Applicability::MachineApplicable
+                    );
+                }
+            );
+        }
+    }
+}
+
 impl<'tcx> LateLintPass<'tcx> for DerivableImpls {
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
         if_chain! {
@@ -83,47 +177,12 @@ impl<'tcx> LateLintPass<'tcx> for DerivableImpls {
             if !attrs.iter().any(|attr| attr.doc_str().is_some());
             if let child_attrs = cx.tcx.hir().attrs(impl_item_hir);
             if !child_attrs.iter().any(|attr| attr.doc_str().is_some());
-            if adt_def.is_struct();
-            then {
-                if let TyKind::Path(QPath::Resolved(_, p)) = self_ty.kind {
-                    if let Some(PathSegment { args: Some(a), .. }) = p.segments.last() {
-                        for arg in a.args {
-                            if !matches!(arg, GenericArg::Lifetime(_)) {
-                                return;
-                            }
-                        }
-                    }
-                }
-                let should_emit = match peel_blocks(func_expr).kind {
-                    ExprKind::Tup(fields) => fields.iter().all(|e| is_default_equivalent(cx, e)),
-                    ExprKind::Call(callee, args)
-                        if is_path_self(callee) => args.iter().all(|e| is_default_equivalent(cx, e)),
-                    ExprKind::Struct(_, fields, _) => fields.iter().all(|ef| is_default_equivalent(cx, ef.expr)),
-                    _ => false,
-                };
 
-                if should_emit {
-                    let struct_span = cx.tcx.def_span(adt_def.did());
-                    span_lint_and_then(
-                        cx,
-                        DERIVABLE_IMPLS,
-                        item.span,
-                        "this `impl` can be derived",
-                        |diag| {
-                            diag.span_suggestion_hidden(
-                                item.span,
-                                "remove the manual implementation...",
-                                String::new(),
-                                Applicability::MachineApplicable
-                            );
-                            diag.span_suggestion(
-                                struct_span.shrink_to_lo(),
-                                "...and instead derive it",
-                                "#[derive(Default)]\n".to_string(),
-                                Applicability::MachineApplicable
-                            );
-                        }
-                    );
+            then {
+                if adt_def.is_struct() {
+                    check_struct(cx, item, self_ty, func_expr, adt_def);
+                } else if adt_def.is_enum() {
+                    check_enum(cx, item, func_expr, adt_def);
                 }
             }
         }

diff --git a/tests/ui/derivable_impls.fixed b/tests/ui/derivable_impls.fixed
@@ -210,4 +210,25 @@ impl Default for IntOrString {
     }
 }
 
+#[derive(Default)]
+pub enum SimpleEnum {
+    Foo,
+    #[default]
+    Bar,
+}
+
+
+
+pub enum NonExhaustiveEnum {
+    Foo,
+    #[non_exhaustive]
+    Bar,
+}
+
+impl Default for NonExhaustiveEnum {
+    fn default() -> Self {
+        NonExhaustiveEnum::Bar
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/derivable_impls.rs b/tests/ui/derivable_impls.rs
@@ -244,4 +244,27 @@ impl Default for IntOrString {
     }
 }
 
+pub enum SimpleEnum {
+    Foo,
+    Bar,
+}
+
+impl Default for SimpleEnum {
+    fn default() -> Self {
+        SimpleEnum::Bar
+    }
+}
+
+pub enum NonExhaustiveEnum {
+    Foo,
+    #[non_exhaustive]
+    Bar,
+}
+
+impl Default for NonExhaustiveEnum {
+    fn default() -> Self {
+        NonExhaustiveEnum::Bar
+    }
+}
+
 fn main() {}
diff --git a/tests/ui/derivable_impls.stderr b/tests/ui/derivable_impls.stderr
@@ -113,5 +113,26 @@ help: ...and instead derive it
 LL | #[derive(Default)]
    |
 
-error: aborting due to 7 previous errors
+error: this `impl` can be derived
+  --> $DIR/derivable_impls.rs:252:1
+   |
+LL | / impl Default for SimpleEnum {
+LL | |     fn default() -> Self {
+LL | |         SimpleEnum::Bar
+LL | |     }
+LL | | }
+   | |_^
+   |
+   = help: remove the manual implementation...
+help: ...and instead derive it...
+   |
+LL | #[derive(Default)]
+   |
+help: ...and mark the default variant
+   |
+LL ~     #[default]
+LL ~     Bar,
+   |
+
+error: aborting due to 8 previous errors