Skip to content

Commit 42b0b47

Browse files
ebrotoebroto
committed
Apply suggestions from PR review
1 parent f072ded commit 42b0b47

File tree

3 files changed

+139
-65
lines changed

3 files changed

+139
-65
lines changed

‎clippy_lints/src/manual_non_exhaustive.rs

+45-39
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
use crate::utils::{snippet_opt, span_lint_and_then};
22
use if_chain::if_chain;
3-
use rustc_ast::ast::{Attribute, Ident, Item, ItemKind, StructField, TyKind, Variant, VariantData, VisibilityKind};
3+
use rustc_ast::ast::{Attribute, Item, ItemKind, StructField, Variant, VariantData, VisibilityKind};
44
use rustc_attr as attr;
55
use rustc_errors::Applicability;
66
use rustc_lint::{EarlyContext, EarlyLintPass};
77
use rustc_session::{declare_lint_pass, declare_tool_lint};
8+
use rustc_span::Span;
89

910
declare_clippy_lint! {
1011
/// **What it does:** Checks for manual implementations of the non-exhaustive pattern.
@@ -90,29 +91,30 @@ fn check_manual_non_exhaustive_enum(cx: &EarlyContext<'_>, item: &Item, variants
9091
}
9192

9293
if_chain! {
93-
if !attr::contains_name(&item.attrs, sym!(non_exhaustive));
94-
let markers = variants.iter().filter(|v| is_non_exhaustive_marker(v)).count();
95-
if markers == 1 && variants.len() > markers;
96-
if let Some(variant) = variants.last();
97-
if is_non_exhaustive_marker(variant);
94+
let mut markers = variants.iter().filter(|v| is_non_exhaustive_marker(v));
95+
if let Some(marker) = markers.next();
96+
if markers.count() == 0 && variants.len() > 1;
9897
then {
9998
span_lint_and_then(
10099
cx,
101100
MANUAL_NON_EXHAUSTIVE,
102101
item.span,
103102
"this seems like a manual implementation of the non-exhaustive pattern",
104103
|diag| {
105-
let header_span = cx.sess.source_map().span_until_char(item.span, '{');
106-
107-
if let Some(snippet) = snippet_opt(cx, header_span) {
108-
diag.span_suggestion(
109-
item.span,
110-
"add the attribute",
111-
format!("#[non_exhaustive] {}", snippet),
112-
Applicability::Unspecified,
113-
);
114-
diag.span_help(variant.span, "and remove this variant");
104+
if_chain! {
105+
if !attr::contains_name(&item.attrs, sym!(non_exhaustive));
106+
let header_span = cx.sess.source_map().span_until_char(item.span, '{');
107+
if let Some(snippet) = snippet_opt(cx, header_span);
108+
then {
109+
diag.span_suggestion(
110+
header_span,
111+
"add the attribute",
112+
format!("#[non_exhaustive] {}", snippet),
113+
Applicability::Unspecified,
114+
);
115+
}
115116
}
117+
diag.span_help(marker.span, "remove this variant");
116118
});
117119
}
118120
}
@@ -123,44 +125,48 @@ fn check_manual_non_exhaustive_struct(cx: &EarlyContext<'_>, item: &Item, data:
123125
matches!(field.vis.node, VisibilityKind::Inherited)
124126
}
125127

126-
fn is_non_exhaustive_marker(name: &Option<Ident>) -> bool {
127-
name.map(|n| n.as_str().starts_with('_')).unwrap_or(true)
128+
fn is_non_exhaustive_marker(field: &StructField) -> bool {
129+
is_private(field) && field.ty.kind.is_unit() && field.ident.map_or(true, |n| n.as_str().starts_with('_'))
130+
}
131+
132+
fn find_header_span(cx: &EarlyContext<'_>, item: &Item, data: &VariantData) -> Span {
133+
let delimiter = match data {
134+
VariantData::Struct(..) => '{',
135+
VariantData::Tuple(..) => '(',
136+
_ => unreachable!("`VariantData::Unit` is already handled above"),
137+
};
138+
139+
cx.sess.source_map().span_until_char(item.span, delimiter)
128140
}
129141

130142
let fields = data.fields();
131143
let private_fields = fields.iter().filter(|f| is_private(f)).count();
132144
let public_fields = fields.iter().filter(|f| f.vis.node.is_pub()).count();
133145

134146
if_chain! {
135-
if !attr::contains_name(&item.attrs, sym!(non_exhaustive));
136-
if private_fields == 1 && public_fields >= private_fields && public_fields == fields.len() - 1;
137-
if let Some(field) = fields.iter().find(|f| is_private(f));
138-
if is_non_exhaustive_marker(&field.ident);
139-
if let TyKind::Tup(tup_fields) = &field.ty.kind;
140-
if tup_fields.is_empty();
147+
if private_fields == 1 && public_fields >= 1 && public_fields == fields.len() - 1;
148+
if let Some(marker) = fields.iter().find(|f| is_non_exhaustive_marker(f));
141149
then {
142150
span_lint_and_then(
143151
cx,
144152
MANUAL_NON_EXHAUSTIVE,
145153
item.span,
146154
"this seems like a manual implementation of the non-exhaustive pattern",
147155
|diag| {
148-
let delimiter = match data {
149-
VariantData::Struct(..) => '{',
150-
VariantData::Tuple(..) => '(',
151-
_ => unreachable!(),
152-
};
153-
let header_span = cx.sess.source_map().span_until_char(item.span, delimiter);
154-
155-
if let Some(snippet) = snippet_opt(cx, header_span) {
156-
diag.span_suggestion(
157-
item.span,
158-
"add the attribute",
159-
format!("#[non_exhaustive] {}", snippet),
160-
Applicability::Unspecified,
161-
);
162-
diag.span_help(field.span, "and remove this field");
156+
if_chain! {
157+
if !attr::contains_name(&item.attrs, sym!(non_exhaustive));
158+
let header_span = find_header_span(cx, item, data);
159+
if let Some(snippet) = snippet_opt(cx, header_span);
160+
then {
161+
diag.span_suggestion(
162+
header_span,
163+
"add the attribute",
164+
format!("#[non_exhaustive] {}", snippet),
165+
Applicability::Unspecified,
166+
);
167+
}
163168
}
169+
diag.span_help(marker.span, "remove this field");
164170
});
165171
}
166172
}

‎tests/ui/manual_non_exhaustive.rs

+26-13
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,16 @@ mod enums {
99
_C,
1010
}
1111

12-
// last variant does not have doc hidden attribute, should be ignored
12+
// user forgot to remove the marker
13+
#[non_exhaustive]
14+
enum Ep {
15+
A,
16+
B,
17+
#[doc(hidden)]
18+
_C,
19+
}
20+
21+
// marker variant does not have doc hidden attribute, should be ignored
1322
enum NoDocHidden {
1423
A,
1524
B,
@@ -32,21 +41,13 @@ mod enums {
3241
_C(bool),
3342
}
3443

35-
// variant with doc hidden is not the last one, should be ignored
36-
enum NotLast {
37-
A,
38-
#[doc(hidden)]
39-
_B,
40-
C,
41-
}
42-
4344
// variant with doc hidden is the only one, should be ignored
4445
enum OnlyMarker {
4546
#[doc(hidden)]
4647
_A,
4748
}
4849

49-
// variant with multiple non-exhaustive "markers", should be ignored
50+
// variant with multiple markers, should be ignored
5051
enum MultipleMarkers {
5152
A,
5253
#[doc(hidden)]
@@ -55,7 +56,7 @@ mod enums {
5556
_C,
5657
}
5758

58-
// already non_exhaustive, should be ignored
59+
// already non_exhaustive and no markers, should be ignored
5960
#[non_exhaustive]
6061
enum NonExhaustive {
6162
A,
@@ -70,6 +71,14 @@ mod structs {
7071
_c: (),
7172
}
7273

74+
// user forgot to remove the private field
75+
#[non_exhaustive]
76+
struct Sp {
77+
pub a: i32,
78+
pub b: i32,
79+
_c: (),
80+
}
81+
7382
// some other fields are private, should be ignored
7483
struct PrivateFields {
7584
a: i32,
@@ -96,7 +105,7 @@ mod structs {
96105
_a: (),
97106
}
98107

99-
// already non exhaustive, should be ignored
108+
// already non exhaustive and no private fields, should be ignored
100109
#[non_exhaustive]
101110
struct NonExhaustive {
102111
pub a: i32,
@@ -107,6 +116,10 @@ mod structs {
107116
mod tuple_structs {
108117
struct T(pub i32, pub i32, ());
109118

119+
// user forgot to remove the private field
120+
#[non_exhaustive]
121+
struct Tp(pub i32, pub i32, ());
122+
110123
// some other fields are private, should be ignored
111124
struct PrivateFields(pub i32, i32, ());
112125

@@ -116,7 +129,7 @@ mod tuple_structs {
116129
// private field is the only field, should be ignored
117130
struct OnlyMarker(());
118131

119-
// already non exhaustive, should be ignored
132+
// already non exhaustive and no private fields, should be ignored
120133
#[non_exhaustive]
121134
struct NonExhaustive(pub i32, pub i32);
122135
}

‎tests/ui/manual_non_exhaustive.stderr

+68-13
Original file line numberDiff line numberDiff line change
@@ -1,48 +1,103 @@
11
error: this seems like a manual implementation of the non-exhaustive pattern
22
--> $DIR/manual_non_exhaustive.rs:5:5
33
|
4-
LL | / enum E {
4+
LL | enum E {
5+
| ^-----
6+
| |
7+
| _____help: add the attribute: `#[non_exhaustive] enum E`
8+
| |
59
LL | | A,
610
LL | | B,
711
LL | | #[doc(hidden)]
812
LL | | _C,
913
LL | | }
10-
| |_____^ help: add the attribute: `#[non_exhaustive] enum E`
14+
| |_____^
1115
|
1216
= note: `-D clippy::manual-non-exhaustive` implied by `-D warnings`
13-
help: and remove this variant
17+
help: remove this variant
1418
--> $DIR/manual_non_exhaustive.rs:9:9
1519
|
1620
LL | _C,
1721
| ^^
1822

1923
error: this seems like a manual implementation of the non-exhaustive pattern
20-
--> $DIR/manual_non_exhaustive.rs:67:5
24+
--> $DIR/manual_non_exhaustive.rs:14:5
2125
|
22-
LL | / struct S {
26+
LL | / enum Ep {
27+
LL | | A,
28+
LL | | B,
29+
LL | | #[doc(hidden)]
30+
LL | | _C,
31+
LL | | }
32+
| |_____^
33+
|
34+
help: remove this variant
35+
--> $DIR/manual_non_exhaustive.rs:18:9
36+
|
37+
LL | _C,
38+
| ^^
39+
40+
error: this seems like a manual implementation of the non-exhaustive pattern
41+
--> $DIR/manual_non_exhaustive.rs:68:5
42+
|
43+
LL | struct S {
44+
| ^-------
45+
| |
46+
| _____help: add the attribute: `#[non_exhaustive] struct S`
47+
| |
48+
LL | | pub a: i32,
49+
LL | | pub b: i32,
50+
LL | | _c: (),
51+
LL | | }
52+
| |_____^
53+
|
54+
help: remove this field
55+
--> $DIR/manual_non_exhaustive.rs:71:9
56+
|
57+
LL | _c: (),
58+
| ^^^^^^
59+
60+
error: this seems like a manual implementation of the non-exhaustive pattern
61+
--> $DIR/manual_non_exhaustive.rs:76:5
62+
|
63+
LL | / struct Sp {
2364
LL | | pub a: i32,
2465
LL | | pub b: i32,
2566
LL | | _c: (),
2667
LL | | }
27-
| |_____^ help: add the attribute: `#[non_exhaustive] struct S`
68+
| |_____^
2869
|
29-
help: and remove this field
30-
--> $DIR/manual_non_exhaustive.rs:70:9
70+
help: remove this field
71+
--> $DIR/manual_non_exhaustive.rs:79:9
3172
|
3273
LL | _c: (),
3374
| ^^^^^^
3475

3576
error: this seems like a manual implementation of the non-exhaustive pattern
36-
--> $DIR/manual_non_exhaustive.rs:108:5
77+
--> $DIR/manual_non_exhaustive.rs:117:5
3778
|
3879
LL | struct T(pub i32, pub i32, ());
39-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add the attribute: `#[non_exhaustive] struct T`
80+
| --------^^^^^^^^^^^^^^^^^^^^^^^
81+
| |
82+
| help: add the attribute: `#[non_exhaustive] struct T`
4083
|
41-
help: and remove this field
42-
--> $DIR/manual_non_exhaustive.rs:108:32
84+
help: remove this field
85+
--> $DIR/manual_non_exhaustive.rs:117:32
4386
|
4487
LL | struct T(pub i32, pub i32, ());
4588
| ^^
4689

47-
error: aborting due to 3 previous errors
90+
error: this seems like a manual implementation of the non-exhaustive pattern
91+
--> $DIR/manual_non_exhaustive.rs:121:5
92+
|
93+
LL | struct Tp(pub i32, pub i32, ());
94+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
95+
|
96+
help: remove this field
97+
--> $DIR/manual_non_exhaustive.rs:121:33
98+
|
99+
LL | struct Tp(pub i32, pub i32, ());
100+
| ^^
101+
102+
error: aborting due to 6 previous errors
48103

0 commit comments

Comments
 (0)