Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the manual_non_exhaustive lint #5550

Merged
merged 3 commits into from
May 2, 2020
Merged

Implement the manual_non_exhaustive lint #5550

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f072ded
Implement the manual_non_exhaustive lint
ebroto Apr 30, 2020
42b0b47
Apply suggestions from PR review
ebroto May 1, 2020
350c17d
Use the only variant left instead of a wildcard
ebroto May 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 45 additions & 39 deletions clippy_lints/src/manual_non_exhaustive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -90,29 +91,30 @@ fn check_manual_non_exhaustive_enum(cx: &EarlyContext<'_>, item: &Item, variants
}

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,
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");
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");
});
}
}
Expand All @@ -123,44 +125,48 @@ fn check_manual_non_exhaustive_struct(cx: &EarlyContext<'_>, item: &Item, data:
matches!(field.vis.node, VisibilityKind::Inherited)
}

fn is_non_exhaustive_marker(name: &Option<Ident>) -> 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"),
ebroto marked this conversation as resolved.
Show resolved Hide resolved
};

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 !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,
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");
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");
});
}
}
Expand Down
39 changes: 26 additions & 13 deletions tests/ui/manual_non_exhaustive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,16 @@ mod enums {
_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,
Expand All @@ -32,21 +41,13 @@ mod enums {
_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)]
Expand All @@ -55,7 +56,7 @@ mod enums {
_C,
}

// already non_exhaustive, should be ignored
// already non_exhaustive and no markers, should be ignored
#[non_exhaustive]
enum NonExhaustive {
A,
Expand All @@ -70,6 +71,14 @@ mod structs {
_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,
Expand All @@ -96,7 +105,7 @@ mod structs {
_a: (),
}

// already non exhaustive, should be ignored
// already non exhaustive and no private fields, should be ignored
#[non_exhaustive]
struct NonExhaustive {
pub a: i32,
Expand All @@ -107,6 +116,10 @@ mod structs {
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, ());

Expand All @@ -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);
}
Expand Down
81 changes: 68 additions & 13 deletions tests/ui/manual_non_exhaustive.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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