diff --git a/CHANGELOG.md b/CHANGELOG.md index 575773580c0b..facf363e371e 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 c995be5edc25..64f3a8a0ebbb 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 000000000000..f3b8902e26f6 --- /dev/null +++ b/clippy_lints/src/manual_non_exhaustive.rs @@ -0,0 +1,173 @@ +use crate::utils::{snippet_opt, span_lint_and_then}; +use if_chain::if_chain; +use rustc_ast::ast::{Attribute, Item, ItemKind, StructField, 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}; +use rustc_span::Span; + +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! { + let mut markers = variants.iter().filter(|v| is_non_exhaustive_marker(v)); + 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 !attr::contains_name(&item.attrs, 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 is_private(field: &StructField) -> bool { + matches!(field.vis.node, VisibilityKind::Inherited) + } + + fn is_non_exhaustive_marker(field: &StructField) -> 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: &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) + } + + 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 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, + MANUAL_NON_EXHAUSTIVE, + item.span, + "this seems like a manual implementation of the non-exhaustive pattern", + |diag| { + if_chain! { + if !attr::contains_name(&item.attrs, sym!(non_exhaustive)); + let header_span = find_header_span(cx, item, data); + 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 field"); + }); + } + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 72675c25175c..c5360002fa05 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 000000000000..7a788f485207 --- /dev/null +++ b/tests/ui/manual_non_exhaustive.rs @@ -0,0 +1,137 @@ +#![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, + pub b: i32, + _c: (), + } + + // user forgot to remove the private field + #[non_exhaustive] + struct Sp { + 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 and no private fields, should be ignored + #[non_exhaustive] + struct NonExhaustive { + pub a: i32, + pub b: i32, + } +} + +mod tuple_structs { + struct T(pub i32, pub i32, ()); + + // user forgot to remove the private field + #[non_exhaustive] + struct Tp(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 and no private fields, 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 000000000000..613c5e8ca1d4 --- /dev/null +++ b/tests/ui/manual_non_exhaustive.stderr @@ -0,0 +1,103 @@ +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 + | +LL | struct S { + | ^------- + | | + | _____help: add the attribute: `#[non_exhaustive] struct S` + | | +LL | | pub a: i32, +LL | | pub b: i32, +LL | | _c: (), +LL | | } + | |_____^ + | +help: remove this field + --> $DIR/manual_non_exhaustive.rs:71:9 + | +LL | _c: (), + | ^^^^^^ + +error: this seems like a manual implementation of the non-exhaustive pattern + --> $DIR/manual_non_exhaustive.rs:76:5 + | +LL | / struct Sp { +LL | | pub a: i32, +LL | | pub b: i32, +LL | | _c: (), +LL | | } + | |_____^ + | +help: remove this field + --> $DIR/manual_non_exhaustive.rs:79:9 + | +LL | _c: (), + | ^^^^^^ + +error: this seems like a manual implementation of the non-exhaustive pattern + --> $DIR/manual_non_exhaustive.rs:117:5 + | +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 + | +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 + | +LL | struct Tp(pub i32, pub i32, ()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove this field + --> $DIR/manual_non_exhaustive.rs:121:33 + | +LL | struct Tp(pub i32, pub i32, ()); + | ^^ + +error: aborting due to 6 previous errors +