From f072ded3bf6286668ff8eade5b58e471dbe66f2a Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Fri, 1 May 2020 01:44:17 +0200 Subject: [PATCH] Implement the manual_non_exhaustive lint --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 5 + clippy_lints/src/manual_non_exhaustive.rs | 167 ++++++++++++++++++++++ src/lintlist/mod.rs | 7 + tests/ui/manual_non_exhaustive.rs | 124 ++++++++++++++++ tests/ui/manual_non_exhaustive.stderr | 48 +++++++ 6 files changed, 352 insertions(+) create mode 100644 clippy_lints/src/manual_non_exhaustive.rs create mode 100644 tests/ui/manual_non_exhaustive.rs create mode 100644 tests/ui/manual_non_exhaustive.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 575773580..facf363e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1423,6 +1423,7 @@ Released 2018-09-13 [`macro_use_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#macro_use_imports [`main_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#main_recursion [`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy +[`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive [`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic [`manual_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_swap [`many_single_char_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#many_single_char_names diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c995be5ed..64f3a8a0e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -247,6 +247,7 @@ mod literal_representation; mod loops; mod macro_use; mod main_recursion; +mod manual_non_exhaustive; mod map_clone; mod map_unit_fn; mod match_on_vec_items; @@ -628,6 +629,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &loops::WHILE_LET_ON_ITERATOR, ¯o_use::MACRO_USE_IMPORTS, &main_recursion::MAIN_RECURSION, + &manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE, &map_clone::MAP_CLONE, &map_unit_fn::OPTION_MAP_UNIT_FN, &map_unit_fn::RESULT_MAP_UNIT_FN, @@ -1064,6 +1066,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box utils::internal_lints::CollapsibleCalls); store.register_late_pass(|| box if_let_mutex::IfLetMutex); store.register_late_pass(|| box match_on_vec_items::MatchOnVecItems); + store.register_early_pass(|| box manual_non_exhaustive::ManualNonExhaustive); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1280,6 +1283,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&loops::WHILE_LET_LOOP), LintId::of(&loops::WHILE_LET_ON_ITERATOR), LintId::of(&main_recursion::MAIN_RECURSION), + LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(&map_clone::MAP_CLONE), LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN), @@ -1474,6 +1478,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&loops::NEEDLESS_RANGE_LOOP), LintId::of(&loops::WHILE_LET_ON_ITERATOR), LintId::of(&main_recursion::MAIN_RECURSION), + LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(&map_clone::MAP_CLONE), LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(&matches::MATCH_OVERLAPPING_ARM), diff --git a/clippy_lints/src/manual_non_exhaustive.rs b/clippy_lints/src/manual_non_exhaustive.rs new file mode 100644 index 000000000..ca2a2cf2e --- /dev/null +++ b/clippy_lints/src/manual_non_exhaustive.rs @@ -0,0 +1,167 @@ +use crate::utils::{snippet_opt, span_lint_and_then}; +use if_chain::if_chain; +use rustc_ast::ast::{Attribute, Ident, Item, ItemKind, StructField, TyKind, Variant, VariantData, VisibilityKind}; +use rustc_attr as attr; +use rustc_errors::Applicability; +use rustc_lint::{EarlyContext, EarlyLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// **What it does:** Checks for manual implementations of the non-exhaustive pattern. + /// + /// **Why is this bad?** Using the #[non_exhaustive] attribute expresses better the intent + /// and allows possible optimizations when applied to enums. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// struct S { + /// pub a: i32, + /// pub b: i32, + /// _c: (), + /// } + /// + /// enum E { + /// A, + /// B, + /// #[doc(hidden)] + /// _C, + /// } + /// + /// struct T(pub i32, pub i32, ()); + /// ``` + /// Use instead: + /// ```rust + /// #[non_exhaustive] + /// struct S { + /// pub a: i32, + /// pub b: i32, + /// } + /// + /// #[non_exhaustive] + /// enum E { + /// A, + /// B, + /// } + /// + /// #[non_exhaustive] + /// struct T(pub i32, pub i32); + /// ``` + pub MANUAL_NON_EXHAUSTIVE, + style, + "manual implementations of the non-exhaustive pattern can be simplified using #[non_exhaustive]" +} + +declare_lint_pass!(ManualNonExhaustive => [MANUAL_NON_EXHAUSTIVE]); + +impl EarlyLintPass for ManualNonExhaustive { + fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { + 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); + }, + _ => {}, + } + } +} + +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('_') + && variant.attrs.iter().any(|a| is_doc_hidden(a)) + } + + fn is_doc_hidden(attr: &Attribute) -> bool { + attr.check_name(sym!(doc)) + && match attr.meta_item_list() { + Some(l) => attr::list_contains_name(&l, sym!(hidden)), + None => false, + } + } + + if_chain! { + if !attr::contains_name(&item.attrs, sym!(non_exhaustive)); + let markers = variants.iter().filter(|v| is_non_exhaustive_marker(v)).count(); + if markers == 1 && variants.len() > markers; + if let Some(variant) = variants.last(); + if is_non_exhaustive_marker(variant); + then { + span_lint_and_then( + cx, + MANUAL_NON_EXHAUSTIVE, + item.span, + "this seems like a manual implementation of the non-exhaustive pattern", + |diag| { + let header_span = cx.sess.source_map().span_until_char(item.span, '{'); + + if let Some(snippet) = snippet_opt(cx, header_span) { + diag.span_suggestion( + item.span, + "add the attribute", + format!("#[non_exhaustive] {}", snippet), + Applicability::Unspecified, + ); + diag.span_help(variant.span, "and remove this variant"); + } + }); + } + } +} + +fn check_manual_non_exhaustive_struct(cx: &EarlyContext<'_>, item: &Item, data: &VariantData) { + fn is_private(field: &StructField) -> bool { + matches!(field.vis.node, VisibilityKind::Inherited) + } + + fn is_non_exhaustive_marker(name: &Option) -> bool { + name.map(|n| n.as_str().starts_with('_')).unwrap_or(true) + } + + let fields = data.fields(); + let private_fields = fields.iter().filter(|f| is_private(f)).count(); + let public_fields = fields.iter().filter(|f| f.vis.node.is_pub()).count(); + + if_chain! { + if !attr::contains_name(&item.attrs, sym!(non_exhaustive)); + if private_fields == 1 && public_fields >= private_fields && public_fields == fields.len() - 1; + if let Some(field) = fields.iter().find(|f| is_private(f)); + if is_non_exhaustive_marker(&field.ident); + if let TyKind::Tup(tup_fields) = &field.ty.kind; + if tup_fields.is_empty(); + then { + span_lint_and_then( + cx, + MANUAL_NON_EXHAUSTIVE, + item.span, + "this seems like a manual implementation of the non-exhaustive pattern", + |diag| { + let delimiter = match data { + VariantData::Struct(..) => '{', + VariantData::Tuple(..) => '(', + _ => unreachable!(), + }; + let header_span = cx.sess.source_map().span_until_char(item.span, delimiter); + + if let Some(snippet) = snippet_opt(cx, header_span) { + diag.span_suggestion( + item.span, + "add the attribute", + format!("#[non_exhaustive] {}", snippet), + Applicability::Unspecified, + ); + diag.span_help(field.span, "and remove this field"); + } + }); + } + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 72675c251..c5360002f 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1088,6 +1088,13 @@ pub static ref ALL_LINTS: Vec = vec![ deprecation: None, module: "loops", }, + Lint { + name: "manual_non_exhaustive", + group: "style", + desc: "manual implementations of the non-exhaustive pattern can be simplified using #[non_exhaustive]", + deprecation: None, + module: "manual_non_exhaustive", + }, Lint { name: "manual_saturating_arithmetic", group: "style", diff --git a/tests/ui/manual_non_exhaustive.rs b/tests/ui/manual_non_exhaustive.rs new file mode 100644 index 000000000..9c239db6e --- /dev/null +++ b/tests/ui/manual_non_exhaustive.rs @@ -0,0 +1,124 @@ +#![warn(clippy::manual_non_exhaustive)] +#![allow(unused)] + +mod enums { + enum E { + A, + B, + #[doc(hidden)] + _C, + } + + // last 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 not the last one, should be ignored + enum NotLast { + A, + #[doc(hidden)] + _B, + C, + } + + // variant with doc hidden is the only one, should be ignored + enum OnlyMarker { + #[doc(hidden)] + _A, + } + + // variant with multiple non-exhaustive "markers", should be ignored + enum MultipleMarkers { + A, + #[doc(hidden)] + _B, + #[doc(hidden)] + _C, + } + + // already non_exhaustive, should be ignored + #[non_exhaustive] + enum NonExhaustive { + A, + B, + } +} + +mod structs { + struct S { + pub a: i32, + pub b: i32, + _c: (), + } + + // some other fields are private, should be ignored + struct PrivateFields { + a: i32, + pub b: i32, + _c: (), + } + + // private field name does not start with underscore, should be ignored + struct NoUnderscore { + pub a: i32, + pub b: i32, + c: (), + } + + // private field is not unit type, should be ignored + struct NotUnit { + pub a: i32, + pub b: i32, + _c: i32, + } + + // private field is the only field, should be ignored + struct OnlyMarker { + _a: (), + } + + // already non exhaustive, should be ignored + #[non_exhaustive] + struct NonExhaustive { + pub a: i32, + pub b: i32, + } +} + +mod tuple_structs { + struct T(pub i32, pub i32, ()); + + // some other fields are private, should be ignored + struct PrivateFields(pub i32, i32, ()); + + // private field is not unit type, should be ignored + struct NotUnit(pub i32, pub i32, i32); + + // private field is the only field, should be ignored + struct OnlyMarker(()); + + // already non exhaustive, should be ignored + #[non_exhaustive] + struct NonExhaustive(pub i32, pub i32); +} + +fn main() {} diff --git a/tests/ui/manual_non_exhaustive.stderr b/tests/ui/manual_non_exhaustive.stderr new file mode 100644 index 000000000..d6719bca0 --- /dev/null +++ b/tests/ui/manual_non_exhaustive.stderr @@ -0,0 +1,48 @@ +error: this seems like a manual implementation of the non-exhaustive pattern + --> $DIR/manual_non_exhaustive.rs:5:5 + | +LL | / enum E { +LL | | A, +LL | | B, +LL | | #[doc(hidden)] +LL | | _C, +LL | | } + | |_____^ help: add the attribute: `#[non_exhaustive] enum E` + | + = note: `-D clippy::manual-non-exhaustive` implied by `-D warnings` +help: and 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:67:5 + | +LL | / struct S { +LL | | pub a: i32, +LL | | pub b: i32, +LL | | _c: (), +LL | | } + | |_____^ help: add the attribute: `#[non_exhaustive] struct S` + | +help: and remove this field + --> $DIR/manual_non_exhaustive.rs:70:9 + | +LL | _c: (), + | ^^^^^^ + +error: this seems like a manual implementation of the non-exhaustive pattern + --> $DIR/manual_non_exhaustive.rs:108:5 + | +LL | struct T(pub i32, pub i32, ()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add the attribute: `#[non_exhaustive] struct T` + | +help: and remove this field + --> $DIR/manual_non_exhaustive.rs:108:32 + | +LL | struct T(pub i32, pub i32, ()); + | ^^ + +error: aborting due to 3 previous errors +