From 6e20a634e7a87a8c0234397e9fdcd8d8dea4e0f4 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Wed, 6 Apr 2022 09:26:59 -0400 Subject: [PATCH] Don't lint `manual_non_exhaustive` when the enum variant is used --- clippy_lints/src/lib.rs | 3 +- clippy_lints/src/manual_non_exhaustive.rs | 177 ++++++++++++------ tests/ui/manual_non_exhaustive_enum.rs | 78 ++++++++ tests/ui/manual_non_exhaustive_enum.stderr | 41 ++++ ...ive.rs => manual_non_exhaustive_struct.rs} | 63 ------- ...rr => manual_non_exhaustive_struct.stderr} | 58 +----- 6 files changed, 246 insertions(+), 174 deletions(-) create mode 100644 tests/ui/manual_non_exhaustive_enum.rs create mode 100644 tests/ui/manual_non_exhaustive_enum.stderr rename tests/ui/{manual_non_exhaustive.rs => manual_non_exhaustive_struct.rs} (58%) rename tests/ui/{manual_non_exhaustive.stderr => manual_non_exhaustive_struct.stderr} (53%) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8dab039f24f..b4a70988286 100644 --- 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 index 33d1bb2985f..7b4b8d6bffa 100644 --- 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,82 +63,59 @@ 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, } -impl ManualNonExhaustive { +impl ManualNonExhaustiveStruct { #[must_use] pub fn new(msrv: Option) -> 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, + constructed_enum_variants: FxHashSet<(DefId, DefId)>, + potential_enums: Vec<(LocalDefId, LocalDefId, Span, Span)>, +} + +impl ManualNonExhaustiveEnum { + #[must_use] + pub fn new(msrv: Option) -> 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; - } + 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); - }, - _ => {}, + 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) - } - - let mut markers = variants.iter().filter(|v| is_non_exhaustive_marker(v)); - if_chain! { - if let Some(marker) = markers.next(); - if markers.count() == 0 && variants.len() > 1; - then { - span_lint_and_then( - cx, - MANUAL_NON_EXHAUSTIVE, - item.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 = cx.sess().source_map().span_until_char(item.span, '{'); - if let Some(snippet) = snippet_opt(cx, header_span); - then { - diag.span_suggestion( - header_span, - "add the attribute", - format!("#[non_exhaustive] {}", snippet), - Applicability::Unspecified, - ); - } - } - diag.span_help(marker.span, "remove this variant"); - }); - } - } -} - -fn check_manual_non_exhaustive_struct(cx: &EarlyContext<'_>, item: &Item, data: &VariantData) { +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) } @@ -142,11 +124,11 @@ fn check_manual_non_exhaustive_struct(cx: &EarlyContext<'_>, item: &Item, data: 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: &Item, data: &VariantData) -> Span { + fn find_header_span(cx: &EarlyContext<'_>, item: &ast::Item, data: &ast::VariantData) -> Span { let delimiter = match data { - VariantData::Struct(..) => '{', - VariantData::Tuple(..) => '(', - VariantData::Unit(_) => unreachable!("`VariantData::Unit` is already handled above"), + 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) @@ -184,3 +166,74 @@ fn check_manual_non_exhaustive_struct(cx: &EarlyContext<'_>, item: &Item, data: } } } + +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; + } + + 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 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)); + } + } + + 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, + enum_span, + "this seems like a manual implementation of the non-exhaustive pattern", + |diag| { + 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(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 new file mode 100644 index 00000000000..f23c6d69b4c --- /dev/null +++ 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 new file mode 100644 index 00000000000..317a45d2cbd --- /dev/null +++ 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.rs b/tests/ui/manual_non_exhaustive_struct.rs similarity index 58% rename from tests/ui/manual_non_exhaustive.rs rename to tests/ui/manual_non_exhaustive_struct.rs index 7a788f48520..498eee4447b 100644 --- a/tests/ui/manual_non_exhaustive.rs +++ b/tests/ui/manual_non_exhaustive_struct.rs @@ -1,69 +1,6 @@ #![warn(clippy::manual_non_exhaustive)] #![allow(unused)] -mod enums { - 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, - } -} - mod structs { struct S { pub a: i32, diff --git a/tests/ui/manual_non_exhaustive.stderr b/tests/ui/manual_non_exhaustive_struct.stderr similarity index 53% rename from tests/ui/manual_non_exhaustive.stderr rename to tests/ui/manual_non_exhaustive_struct.stderr index 613c5e8ca1d..e0766c17b75 100644 --- a/tests/ui/manual_non_exhaustive.stderr +++ b/tests/ui/manual_non_exhaustive_struct.stderr @@ -1,44 +1,5 @@ error: this seems like a manual implementation of the non-exhaustive pattern - --> $DIR/manual_non_exhaustive.rs:5:5 - | -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.rs:9:9 - | -LL | _C, - | ^^ - -error: this seems like a manual implementation of the non-exhaustive pattern - --> $DIR/manual_non_exhaustive.rs:14:5 - | -LL | / enum Ep { -LL | | A, -LL | | B, -LL | | #[doc(hidden)] -LL | | _C, -LL | | } - | |_____^ - | -help: remove this variant - --> $DIR/manual_non_exhaustive.rs:18:9 - | -LL | _C, - | ^^ - -error: this seems like a manual implementation of the non-exhaustive pattern - --> $DIR/manual_non_exhaustive.rs:68:5 + --> $DIR/manual_non_exhaustive_struct.rs:5:5 | LL | struct S { | ^------- @@ -51,14 +12,15 @@ LL | | _c: (), LL | | } | |_____^ | + = note: `-D clippy::manual-non-exhaustive` implied by `-D warnings` help: remove this field - --> $DIR/manual_non_exhaustive.rs:71:9 + --> $DIR/manual_non_exhaustive_struct.rs:8:9 | LL | _c: (), | ^^^^^^ error: this seems like a manual implementation of the non-exhaustive pattern - --> $DIR/manual_non_exhaustive.rs:76:5 + --> $DIR/manual_non_exhaustive_struct.rs:13:5 | LL | / struct Sp { LL | | pub a: i32, @@ -68,13 +30,13 @@ LL | | } | |_____^ | help: remove this field - --> $DIR/manual_non_exhaustive.rs:79:9 + --> $DIR/manual_non_exhaustive_struct.rs:16:9 | LL | _c: (), | ^^^^^^ error: this seems like a manual implementation of the non-exhaustive pattern - --> $DIR/manual_non_exhaustive.rs:117:5 + --> $DIR/manual_non_exhaustive_struct.rs:54:5 | LL | struct T(pub i32, pub i32, ()); | --------^^^^^^^^^^^^^^^^^^^^^^^ @@ -82,22 +44,22 @@ LL | struct T(pub i32, pub i32, ()); | help: add the attribute: `#[non_exhaustive] struct T` | help: remove this field - --> $DIR/manual_non_exhaustive.rs:117:32 + --> $DIR/manual_non_exhaustive_struct.rs:54:32 | LL | struct T(pub i32, pub i32, ()); | ^^ error: this seems like a manual implementation of the non-exhaustive pattern - --> $DIR/manual_non_exhaustive.rs:121:5 + --> $DIR/manual_non_exhaustive_struct.rs:58:5 | LL | struct Tp(pub i32, pub i32, ()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | help: remove this field - --> $DIR/manual_non_exhaustive.rs:121:33 + --> $DIR/manual_non_exhaustive_struct.rs:58:33 | LL | struct Tp(pub i32, pub i32, ()); | ^^ -error: aborting due to 6 previous errors +error: aborting due to 4 previous errors