From 42b0b4754c881101cefb0307c489d6159c19b2f3 Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Fri, 1 May 2020 22:37:14 +0200 Subject: [PATCH] Apply suggestions from PR review --- clippy_lints/src/manual_non_exhaustive.rs | 84 ++++++++++++----------- tests/ui/manual_non_exhaustive.rs | 39 +++++++---- tests/ui/manual_non_exhaustive.stderr | 81 ++++++++++++++++++---- 3 files changed, 139 insertions(+), 65 deletions(-) diff --git a/clippy_lints/src/manual_non_exhaustive.rs b/clippy_lints/src/manual_non_exhaustive.rs index ca2a2cf2e1e..a4273da1d74 100644 --- a/clippy_lints/src/manual_non_exhaustive.rs +++ b/clippy_lints/src/manual_non_exhaustive.rs @@ -1,10 +1,11 @@ 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_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. @@ -90,11 +91,9 @@ fn is_doc_hidden(attr: &Attribute) -> bool { } 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); + 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, @@ -102,17 +101,20 @@ fn is_doc_hidden(attr: &Attribute) -> bool { 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"); + 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"); }); } } @@ -123,8 +125,18 @@ 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) + 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(..) => '(', + _ => unreachable!("`VariantData::Unit` is already handled above"), + }; + + cx.sess.source_map().span_until_char(item.span, delimiter) } let fields = data.fields(); @@ -132,12 +144,8 @@ fn is_non_exhaustive_marker(name: &Option) -> bool { 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(); + 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, @@ -145,22 +153,20 @@ fn is_non_exhaustive_marker(name: &Option) -> bool { 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"); + 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/tests/ui/manual_non_exhaustive.rs b/tests/ui/manual_non_exhaustive.rs index 9c239db6e00..7a788f48520 100644 --- a/tests/ui/manual_non_exhaustive.rs +++ b/tests/ui/manual_non_exhaustive.rs @@ -9,7 +9,16 @@ enum E { _C, } - // last variant does not have doc hidden attribute, should be ignored + // 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, @@ -32,21 +41,13 @@ enum NotUnit { _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 + // variant with multiple markers, should be ignored enum MultipleMarkers { A, #[doc(hidden)] @@ -55,7 +56,7 @@ enum MultipleMarkers { _C, } - // already non_exhaustive, should be ignored + // already non_exhaustive and no markers, should be ignored #[non_exhaustive] enum NonExhaustive { A, @@ -70,6 +71,14 @@ struct S { _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, @@ -96,7 +105,7 @@ struct OnlyMarker { _a: (), } - // already non exhaustive, should be ignored + // already non exhaustive and no private fields, should be ignored #[non_exhaustive] struct NonExhaustive { pub a: i32, @@ -107,6 +116,10 @@ struct NonExhaustive { 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, ()); @@ -116,7 +129,7 @@ mod tuple_structs { // private field is the only field, should be ignored struct OnlyMarker(()); - // already non exhaustive, should be ignored + // already non exhaustive and no private fields, should be ignored #[non_exhaustive] struct NonExhaustive(pub i32, pub i32); } diff --git a/tests/ui/manual_non_exhaustive.stderr b/tests/ui/manual_non_exhaustive.stderr index d6719bca0d4..613c5e8ca1d 100644 --- a/tests/ui/manual_non_exhaustive.stderr +++ b/tests/ui/manual_non_exhaustive.stderr @@ -1,48 +1,103 @@ error: this seems like a manual implementation of the non-exhaustive pattern --> $DIR/manual_non_exhaustive.rs:5:5 | -LL | / enum E { +LL | enum E { + | ^----- + | | + | _____help: add the attribute: `#[non_exhaustive] 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 +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:67:5 + --> $DIR/manual_non_exhaustive.rs:14:5 | -LL | / struct S { +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: add the attribute: `#[non_exhaustive] struct S` + | |_____^ | -help: and remove this field - --> $DIR/manual_non_exhaustive.rs:70:9 +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:108:5 + --> $DIR/manual_non_exhaustive.rs:117:5 | LL | struct T(pub i32, pub i32, ()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add the attribute: `#[non_exhaustive] struct T` + | --------^^^^^^^^^^^^^^^^^^^^^^^ + | | + | help: add the attribute: `#[non_exhaustive] struct T` | -help: and remove this field - --> $DIR/manual_non_exhaustive.rs:108:32 +help: remove this field + --> $DIR/manual_non_exhaustive.rs:117:32 | LL | struct T(pub i32, pub i32, ()); | ^^ -error: aborting due to 3 previous errors +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 -- 2.44.0