Don't lint manual_non_exhaustive when the enum variant is used

Jarcho · Jarcho · commit 6e20a634e7a8 · 2022-04-06T10:12:30.000-04:00
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -575,7 +575,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(move || Box::new(approx_const::ApproxConstant::new(msrv)));
     store.register_late_pass(move || Box::new(methods::Methods::new(avoid_breaking_exported_api, msrv)));
     store.register_late_pass(move || Box::new(matches::Matches::new(msrv)));
-    store.register_early_pass(move || Box::new(manual_non_exhaustive::ManualNonExhaustive::new(msrv)));
+    store.register_early_pass(move || Box::new(manual_non_exhaustive::ManualNonExhaustiveStruct::new(msrv)));
+    store.register_late_pass(move || Box::new(manual_non_exhaustive::ManualNonExhaustiveEnum::new(msrv)));
     store.register_late_pass(move || Box::new(manual_strip::ManualStrip::new(msrv)));
     store.register_early_pass(move || Box::new(redundant_static_lifetimes::RedundantStaticLifetimes::new(msrv)));
     store.register_early_pass(move || Box::new(redundant_field_names::RedundantFieldNames::new(msrv)));
diff --git a/clippy_lints/src/manual_non_exhaustive.rs b/clippy_lints/src/manual_non_exhaustive.rs
@@ -1,13 +1,18 @@
 use clippy_utils::attrs::is_doc_hidden;
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::source::snippet_opt;
-use clippy_utils::{meets_msrv, msrvs};
+use clippy_utils::{is_lint_allowed, meets_msrv, msrvs};
 use if_chain::if_chain;
-use rustc_ast::ast::{FieldDef, Item, ItemKind, Variant, VariantData, VisibilityKind};
+use rustc_ast::ast::{self, FieldDef, VisibilityKind};
+use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::Applicability;
-use rustc_lint::{EarlyContext, EarlyLintPass, LintContext};
+use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
+use rustc_hir::{self as hir, Expr, ExprKind, QPath};
+use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintContext};
+use rustc_middle::ty::DefIdTree;
 use rustc_semver::RustcVersion;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::def_id::{DefId, LocalDefId};
 use rustc_span::{sym, Span};
 
 declare_clippy_lint! {
@@ -58,55 +63,84 @@ declare_clippy_lint! {
     "manual implementations of the non-exhaustive pattern can be simplified using #[non_exhaustive]"
 }
 
-#[derive(Clone)]
-pub struct ManualNonExhaustive {
+#[allow(clippy::module_name_repetitions)]
+pub struct ManualNonExhaustiveStruct {
     msrv: Option<RustcVersion>,
 }
 
-impl ManualNonExhaustive {
+impl ManualNonExhaustiveStruct {
     #[must_use]
     pub fn new(msrv: Option<RustcVersion>) -> Self {
         Self { msrv }
     }
 }
 
-impl_lint_pass!(ManualNonExhaustive => [MANUAL_NON_EXHAUSTIVE]);
+impl_lint_pass!(ManualNonExhaustiveStruct => [MANUAL_NON_EXHAUSTIVE]);
 
-impl EarlyLintPass for ManualNonExhaustive {
-    fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) {
+#[allow(clippy::module_name_repetitions)]
+pub struct ManualNonExhaustiveEnum {
+    msrv: Option<RustcVersion>,
+    constructed_enum_variants: FxHashSet<(DefId, DefId)>,
+    potential_enums: Vec<(LocalDefId, LocalDefId, Span, Span)>,
+}
+
+impl ManualNonExhaustiveEnum {
+    #[must_use]
+    pub fn new(msrv: Option<RustcVersion>) -> Self {
+        Self {
+            msrv,
+            constructed_enum_variants: FxHashSet::default(),
+            potential_enums: Vec::new(),
+        }
+    }
+}
+
+impl_lint_pass!(ManualNonExhaustiveEnum => [MANUAL_NON_EXHAUSTIVE]);
+
+impl EarlyLintPass for ManualNonExhaustiveStruct {
+    fn check_item(&mut self, cx: &EarlyContext<'_>, item: &ast::Item) {
         if !meets_msrv(self.msrv.as_ref(), &msrvs::NON_EXHAUSTIVE) {
             return;
         }
 
-        match &item.kind {
-            ItemKind::Enum(def, _) => {
-                check_manual_non_exhaustive_enum(cx, item, &def.variants);
-            },
-            ItemKind::Struct(variant_data, _) => {
-                if let VariantData::Unit(..) = variant_data {
-                    return;
-                }
-
-                check_manual_non_exhaustive_struct(cx, item, variant_data);
-            },
-            _ => {},
+        if let ast::ItemKind::Struct(variant_data, _) = &item.kind {
+            if let ast::VariantData::Unit(..) = variant_data {
+                return;
+            }
+
+            check_manual_non_exhaustive_struct(cx, item, variant_data);
         }
     }
 
     extract_msrv_attr!(EarlyContext);
 }
 
-fn check_manual_non_exhaustive_enum(cx: &EarlyContext<'_>, item: &Item, variants: &[Variant]) {
-    fn is_non_exhaustive_marker(variant: &Variant) -> bool {
-        matches!(variant.data, VariantData::Unit(_))
-            && variant.ident.as_str().starts_with('_')
-            && is_doc_hidden(&variant.attrs)
+fn check_manual_non_exhaustive_struct(cx: &EarlyContext<'_>, item: &ast::Item, data: &ast::VariantData) {
+    fn is_private(field: &FieldDef) -> bool {
+        matches!(field.vis.kind, VisibilityKind::Inherited)
+    }
+
+    fn is_non_exhaustive_marker(field: &FieldDef) -> bool {
+        is_private(field) && field.ty.kind.is_unit() && field.ident.map_or(true, |n| n.as_str().starts_with('_'))
+    }
+
+    fn find_header_span(cx: &EarlyContext<'_>, item: &ast::Item, data: &ast::VariantData) -> Span {
+        let delimiter = match data {
+            ast::VariantData::Struct(..) => '{',
+            ast::VariantData::Tuple(..) => '(',
+            ast::VariantData::Unit(_) => unreachable!("`VariantData::Unit` is already handled above"),
+        };
+
+        cx.sess().source_map().span_until_char(item.span, delimiter)
     }
 
-    let mut markers = variants.iter().filter(|v| is_non_exhaustive_marker(v));
+    let fields = data.fields();
+    let private_fields = fields.iter().filter(|f| is_private(f)).count();
+    let public_fields = fields.iter().filter(|f| f.vis.kind.is_pub()).count();
+
     if_chain! {
-        if let Some(marker) = markers.next();
-        if markers.count() == 0 && variants.len() > 1;
+        if private_fields == 1 && public_fields >= 1 && public_fields == fields.len() - 1;
+        if let Some(marker) = fields.iter().find(|f| is_non_exhaustive_marker(f));
         then {
             span_lint_and_then(
                 cx,
@@ -116,7 +150,7 @@ fn check_manual_non_exhaustive_enum(cx: &EarlyContext<'_>, item: &Item, variants
                 |diag| {
                     if_chain! {
                         if !item.attrs.iter().any(|attr| attr.has_name(sym::non_exhaustive));
-                        let header_span = cx.sess().source_map().span_until_char(item.span, '{');
+                        let header_span = find_header_span(cx, item, data);
                         if let Some(snippet) = snippet_opt(cx, header_span);
                         then {
                             diag.span_suggestion(
@@ -127,60 +161,79 @@ fn check_manual_non_exhaustive_enum(cx: &EarlyContext<'_>, item: &Item, variants
                             );
                         }
                     }
-                    diag.span_help(marker.span, "remove this variant");
+                    diag.span_help(marker.span, "remove this field");
                 });
         }
     }
 }
 
-fn check_manual_non_exhaustive_struct(cx: &EarlyContext<'_>, item: &Item, data: &VariantData) {
-    fn is_private(field: &FieldDef) -> bool {
-        matches!(field.vis.kind, VisibilityKind::Inherited)
-    }
+impl<'tcx> LateLintPass<'tcx> for ManualNonExhaustiveEnum {
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
+        if !meets_msrv(self.msrv.as_ref(), &msrvs::NON_EXHAUSTIVE) {
+            return;
+        }
 
-    fn is_non_exhaustive_marker(field: &FieldDef) -> bool {
-        is_private(field) && field.ty.kind.is_unit() && field.ident.map_or(true, |n| n.as_str().starts_with('_'))
+        if let hir::ItemKind::Enum(def, _) = &item.kind
+            && def.variants.len() > 1
+        {
+            let mut iter = def.variants.iter().filter_map(|v| {
+                let id = cx.tcx.hir().local_def_id(v.id);
+                (matches!(v.data, hir::VariantData::Unit(_))
+                    && v.ident.as_str().starts_with('_')
+                    && is_doc_hidden(cx.tcx.get_attrs(id.to_def_id())))
+                .then(|| (id, v.span))
+            });
+            if let Some((id, span)) = iter.next()
+                && iter.next().is_none()
+            {
+                self.potential_enums.push((item.def_id, id, item.span, span));
+            }
+        }
     }
 
-    fn find_header_span(cx: &EarlyContext<'_>, item: &Item, data: &VariantData) -> Span {
-        let delimiter = match data {
-            VariantData::Struct(..) => '{',
-            VariantData::Tuple(..) => '(',
-            VariantData::Unit(_) => unreachable!("`VariantData::Unit` is already handled above"),
-        };
-
-        cx.sess().source_map().span_until_char(item.span, delimiter)
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
+        if let ExprKind::Path(QPath::Resolved(None, p)) = &e.kind
+            && let [.., name] = p.segments
+            && let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), id) = p.res
+            && name.ident.as_str().starts_with('_')
+            && let Some(variant_id) = cx.tcx.parent(id)
+            && let Some(enum_id) = cx.tcx.parent(variant_id)
+        {
+            self.constructed_enum_variants.insert((enum_id, variant_id));
+        }
     }
 
-    let fields = data.fields();
-    let private_fields = fields.iter().filter(|f| is_private(f)).count();
-    let public_fields = fields.iter().filter(|f| f.vis.kind.is_pub()).count();
-
-    if_chain! {
-        if private_fields == 1 && public_fields >= 1 && public_fields == fields.len() - 1;
-        if let Some(marker) = fields.iter().find(|f| is_non_exhaustive_marker(f));
-        then {
+    fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
+        for &(enum_id, _, enum_span, variant_span) in
+            self.potential_enums.iter().filter(|&&(enum_id, variant_id, _, _)| {
+                !self
+                    .constructed_enum_variants
+                    .contains(&(enum_id.to_def_id(), variant_id.to_def_id()))
+                    && !is_lint_allowed(cx, MANUAL_NON_EXHAUSTIVE, cx.tcx.hir().local_def_id_to_hir_id(enum_id))
+            })
+        {
             span_lint_and_then(
                 cx,
                 MANUAL_NON_EXHAUSTIVE,
-                item.span,
+                enum_span,
                 "this seems like a manual implementation of the non-exhaustive pattern",
                 |diag| {
-                    if_chain! {
-                        if !item.attrs.iter().any(|attr| attr.has_name(sym::non_exhaustive));
-                        let header_span = find_header_span(cx, item, data);
-                        if let Some(snippet) = snippet_opt(cx, header_span);
-                        then {
+                    if !cx.tcx.adt_def(enum_id).is_variant_list_non_exhaustive()
+                        && let header_span = cx.sess().source_map().span_until_char(enum_span, '{')
+                        && let Some(snippet) = snippet_opt(cx, header_span)
+                    {
                             diag.span_suggestion(
                                 header_span,
                                 "add the attribute",
                                 format!("#[non_exhaustive] {}", snippet),
                                 Applicability::Unspecified,
                             );
-                        }
                     }
-                    diag.span_help(marker.span, "remove this field");
-                });
+                    diag.span_help(variant_span, "remove this variant");
+                },
+            );
         }
     }
+
+    extract_msrv_attr!(LateContext);
 }
diff --git a/tests/ui/manual_non_exhaustive_enum.rs b/tests/ui/manual_non_exhaustive_enum.rs
@@ -0,0 +1,78 @@
+#![warn(clippy::manual_non_exhaustive)]
+#![allow(unused)]
+
+enum E {
+    A,
+    B,
+    #[doc(hidden)]
+    _C,
+}
+
+// user forgot to remove the marker
+#[non_exhaustive]
+enum Ep {
+    A,
+    B,
+    #[doc(hidden)]
+    _C,
+}
+
+// marker variant does not have doc hidden attribute, should be ignored
+enum NoDocHidden {
+    A,
+    B,
+    _C,
+}
+
+// name of variant with doc hidden does not start with underscore, should be ignored
+enum NoUnderscore {
+    A,
+    B,
+    #[doc(hidden)]
+    C,
+}
+
+// variant with doc hidden is not unit, should be ignored
+enum NotUnit {
+    A,
+    B,
+    #[doc(hidden)]
+    _C(bool),
+}
+
+// variant with doc hidden is the only one, should be ignored
+enum OnlyMarker {
+    #[doc(hidden)]
+    _A,
+}
+
+// variant with multiple markers, should be ignored
+enum MultipleMarkers {
+    A,
+    #[doc(hidden)]
+    _B,
+    #[doc(hidden)]
+    _C,
+}
+
+// already non_exhaustive and no markers, should be ignored
+#[non_exhaustive]
+enum NonExhaustive {
+    A,
+    B,
+}
+
+// marked is used, don't lint
+enum UsedHidden {
+    #[doc(hidden)]
+    _A,
+    B,
+    C,
+}
+fn foo(x: &mut UsedHidden) {
+    if matches!(*x, UsedHidden::B) {
+        *x = UsedHidden::_A;
+    }
+}
+
+fn main() {}
diff --git a/tests/ui/manual_non_exhaustive_enum.stderr b/tests/ui/manual_non_exhaustive_enum.stderr
@@ -0,0 +1,41 @@
+error: this seems like a manual implementation of the non-exhaustive pattern
+  --> $DIR/manual_non_exhaustive_enum.rs:4:1
+   |
+LL |   enum E {
+   |   ^-----
+   |   |
+   |  _help: add the attribute: `#[non_exhaustive] enum E`
+   | |
+LL | |     A,
+LL | |     B,
+LL | |     #[doc(hidden)]
+LL | |     _C,
+LL | | }
+   | |_^
+   |
+   = note: `-D clippy::manual-non-exhaustive` implied by `-D warnings`
+help: remove this variant
+  --> $DIR/manual_non_exhaustive_enum.rs:8:5
+   |
+LL |     _C,
+   |     ^^
+
+error: this seems like a manual implementation of the non-exhaustive pattern
+  --> $DIR/manual_non_exhaustive_enum.rs:13:1
+   |
+LL | / enum Ep {
+LL | |     A,
+LL | |     B,
+LL | |     #[doc(hidden)]
+LL | |     _C,
+LL | | }
+   | |_^
+   |
+help: remove this variant
+  --> $DIR/manual_non_exhaustive_enum.rs:17:5
+   |
+LL |     _C,
+   |     ^^
+
+error: aborting due to 2 previous errors
+
diff --git a/tests/ui/manual_non_exhaustive_struct.rs b/tests/ui/manual_non_exhaustive_struct.rs
diff --git a/tests/ui/manual_non_exhaustive_struct.stderr b/tests/ui/manual_non_exhaustive_struct.stderr
