Skip to content

Commit 8789f4e

Browse files
borsbors
committed
Auto merge of rust-lang#8985 - botahamec:single-match-option, r=llogiq
Lint `[single_match]` on `Option` matches fixes rust-lang#8928 changelog: did some cleanup of the logic for ``[`single_match`]`` and ``[`single_match_else`]`` which fixes the bug where `Option` matches were not linted unless a wildcard was used for one of the arms.
2 parents 93ebd0e + ded2bb5 commit 8789f4e

File tree

4 files changed

+142
-55
lines changed

4 files changed

+142
-55
lines changed

‎clippy_lints/src/matches/single_match.rs

Lines changed: 32 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -140,70 +140,45 @@ fn check_opt_like<'a>(
140140
ty: Ty<'a>,
141141
els: Option<&Expr<'_>>,
142142
) {
143-
// list of candidate `Enum`s we know will never get any more members
144-
let candidates = &[
145-
(&paths::COW, "Borrowed"),
146-
(&paths::COW, "Cow::Borrowed"),
147-
(&paths::COW, "Cow::Owned"),
148-
(&paths::COW, "Owned"),
149-
(&paths::OPTION, "None"),
150-
(&paths::RESULT, "Err"),
151-
(&paths::RESULT, "Ok"),
152-
];
153-
154-
// We want to suggest to exclude an arm that contains only wildcards or forms the exhaustive
155-
// match with the second branch, without enum variants in matches.
156-
if !contains_only_wilds(arms[1].pat) && !form_exhaustive_matches(arms[0].pat, arms[1].pat) {
157-
return;
143+
// We don't want to lint if the second arm contains an enum which could
144+
// have more variants in the future.
145+
if form_exhaustive_matches(cx, ty, arms[0].pat, arms[1].pat) {
146+
report_single_pattern(cx, ex, arms, expr, els);
158147
}
148+
}
159149

150+
/// Returns `true` if all of the types in the pattern are enums which we know
151+
/// won't be expanded in the future
152+
fn pat_in_candidate_enum<'a>(cx: &LateContext<'a>, ty: Ty<'a>, pat: &Pat<'_>) -> bool {
160153
let mut paths_and_types = Vec::new();
161-
if !collect_pat_paths(&mut paths_and_types, cx, arms[1].pat, ty) {
162-
return;
163-
}
154+
collect_pat_paths(&mut paths_and_types, cx, pat, ty);
155+
paths_and_types.iter().all(|ty| in_candidate_enum(cx, *ty))
156+
}
164157

165-
let in_candidate_enum = |path_info: &(String, Ty<'_>)| -> bool {
166-
let (path, ty) = path_info;
167-
for &(ty_path, pat_path) in candidates {
168-
if path == pat_path && match_type(cx, *ty, ty_path) {
169-
return true;
170-
}
158+
/// Returns `true` if the given type is an enum we know won't be expanded in the future
159+
fn in_candidate_enum<'a>(cx: &LateContext<'a>, ty: Ty<'_>) -> bool {
160+
// list of candidate `Enum`s we know will never get any more members
161+
let candidates = [&paths::COW, &paths::OPTION, &paths::RESULT];
162+
163+
for candidate_ty in candidates {
164+
if match_type(cx, ty, candidate_ty) {
165+
return true;
171166
}
172-
false
173-
};
174-
if paths_and_types.iter().all(in_candidate_enum) {
175-
report_single_pattern(cx, ex, arms, expr, els);
176167
}
168+
false
177169
}
178170

179-
/// Collects paths and their types from the given patterns. Returns true if the given pattern could
180-
/// be simplified, false otherwise.
181-
fn collect_pat_paths<'a>(acc: &mut Vec<(String, Ty<'a>)>, cx: &LateContext<'a>, pat: &Pat<'_>, ty: Ty<'a>) -> bool {
171+
/// Collects types from the given pattern
172+
fn collect_pat_paths<'a>(acc: &mut Vec<Ty<'a>>, cx: &LateContext<'a>, pat: &Pat<'_>, ty: Ty<'a>) {
182173
match pat.kind {
183-
PatKind::Wild => true,
184-
PatKind::Tuple(inner, _) => inner.iter().all(|p| {
174+
PatKind::Tuple(inner, _) => inner.iter().for_each(|p| {
185175
let p_ty = cx.typeck_results().pat_ty(p);
186-
collect_pat_paths(acc, cx, p, p_ty)
176+
collect_pat_paths(acc, cx, p, p_ty);
187177
}),
188-
PatKind::TupleStruct(ref path, ..) => {
189-
let path = rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| {
190-
s.print_qpath(path, false);
191-
});
192-
acc.push((path, ty));
193-
true
194-
},
195-
PatKind::Binding(BindingAnnotation::Unannotated, .., ident, None) => {
196-
acc.push((ident.to_string(), ty));
197-
true
178+
PatKind::TupleStruct(..) | PatKind::Binding(BindingAnnotation::Unannotated, .., None) | PatKind::Path(_) => {
179+
acc.push(ty);
198180
},
199-
PatKind::Path(ref path) => {
200-
let path = rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| {
201-
s.print_qpath(path, false);
202-
});
203-
acc.push((path, ty));
204-
true
205-
},
206-
_ => false,
181+
_ => {},
207182
}
208183
}
209184

@@ -218,7 +193,7 @@ fn contains_only_wilds(pat: &Pat<'_>) -> bool {
218193

219194
/// Returns true if the given patterns forms only exhaustive matches that don't contain enum
220195
/// patterns without a wildcard.
221-
fn form_exhaustive_matches(left: &Pat<'_>, right: &Pat<'_>) -> bool {
196+
fn form_exhaustive_matches<'a>(cx: &LateContext<'a>, ty: Ty<'a>, left: &Pat<'_>, right: &Pat<'_>) -> bool {
222197
match (&left.kind, &right.kind) {
223198
(PatKind::Wild, _) | (_, PatKind::Wild) => true,
224199
(PatKind::Tuple(left_in, left_pos), PatKind::Tuple(right_in, right_pos)) => {
@@ -264,6 +239,10 @@ fn form_exhaustive_matches(left: &Pat<'_>, right: &Pat<'_>) -> bool {
264239
}
265240
true
266241
},
242+
(PatKind::TupleStruct(..), PatKind::Path(_)) => pat_in_candidate_enum(cx, ty, right),
243+
(PatKind::TupleStruct(..), PatKind::TupleStruct(_, inner, _)) => {
244+
pat_in_candidate_enum(cx, ty, right) && inner.iter().all(contains_only_wilds)
245+
},
267246
_ => false,
268247
}
269248
}

‎tests/ui/single_match.stderr

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,15 @@ LL | | _ => {},
3838
LL | | };
3939
| |_____^ help: try this: `if let (2..=3, 7..=9) = z { dummy() }`
4040

41+
error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
42+
--> $DIR/single_match.rs:54:5
43+
|
44+
LL | / match x {
45+
LL | | Some(y) => dummy(),
46+
LL | | None => (),
47+
LL | | };
48+
| |_____^ help: try this: `if let Some(y) = x { dummy() }`
49+
4150
error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
4251
--> $DIR/single_match.rs:59:5
4352
|
@@ -146,5 +155,5 @@ LL | | (..) => {},
146155
LL | | }
147156
| |_____^ help: try this: `if let (.., Some(E::V), _) = (Some(42), Some(E::V), Some(42)) {}`
148157

149-
error: aborting due to 15 previous errors
158+
error: aborting due to 16 previous errors
150159

‎tests/ui/single_match_else.rs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,4 +97,23 @@ fn main() {
9797
return;
9898
},
9999
}
100+
101+
// lint here
102+
use std::convert::Infallible;
103+
match Result::<i32, Infallible>::Ok(1) {
104+
Ok(a) => println!("${:?}", a),
105+
Err(_) => {
106+
println!("else block");
107+
return;
108+
}
109+
}
110+
111+
use std::borrow::Cow;
112+
match Cow::from("moo") {
113+
Cow::Owned(a) => println!("${:?}", a),
114+
Cow::Borrowed(_) => {
115+
println!("else block");
116+
return;
117+
}
118+
}
100119
}

‎tests/ui/single_match_else.stderr

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,5 +20,85 @@ LL + None
2020
LL ~ };
2121
|
2222

23-
error: aborting due to previous error
23+
error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
24+
--> $DIR/single_match_else.rs:84:5
25+
|
26+
LL | / match Some(1) {
27+
LL | | Some(a) => println!("${:?}", a),
28+
LL | | None => {
29+
LL | | println!("else block");
30+
LL | | return
31+
LL | | },
32+
LL | | }
33+
| |_____^
34+
|
35+
help: try this
36+
|
37+
LL ~ if let Some(a) = Some(1) { println!("${:?}", a) } else {
38+
LL + println!("else block");
39+
LL + return
40+
LL + }
41+
|
42+
43+
error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
44+
--> $DIR/single_match_else.rs:93:5
45+
|
46+
LL | / match Some(1) {
47+
LL | | Some(a) => println!("${:?}", a),
48+
LL | | None => {
49+
LL | | println!("else block");
50+
LL | | return;
51+
LL | | },
52+
LL | | }
53+
| |_____^
54+
|
55+
help: try this
56+
|
57+
LL ~ if let Some(a) = Some(1) { println!("${:?}", a) } else {
58+
LL + println!("else block");
59+
LL + return;
60+
LL + }
61+
|
62+
63+
error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
64+
--> $DIR/single_match_else.rs:103:5
65+
|
66+
LL | / match Result::<i32, Infallible>::Ok(1) {
67+
LL | | Ok(a) => println!("${:?}", a),
68+
LL | | Err(_) => {
69+
LL | | println!("else block");
70+
LL | | return;
71+
LL | | }
72+
LL | | }
73+
| |_____^
74+
|
75+
help: try this
76+
|
77+
LL ~ if let Ok(a) = Result::<i32, Infallible>::Ok(1) { println!("${:?}", a) } else {
78+
LL + println!("else block");
79+
LL + return;
80+
LL + }
81+
|
82+
83+
error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
84+
--> $DIR/single_match_else.rs:112:5
85+
|
86+
LL | / match Cow::from("moo") {
87+
LL | | Cow::Owned(a) => println!("${:?}", a),
88+
LL | | Cow::Borrowed(_) => {
89+
LL | | println!("else block");
90+
LL | | return;
91+
LL | | }
92+
LL | | }
93+
| |_____^
94+
|
95+
help: try this
96+
|
97+
LL ~ if let Cow::Owned(a) = Cow::from("moo") { println!("${:?}", a) } else {
98+
LL + println!("else block");
99+
LL + return;
100+
LL + }
101+
|
102+
103+
error: aborting due to 5 previous errors
24104

0 commit comments

Comments
 (0)