Skip to content

Commit fa9f744

Browse files
author
Vincent Dal Maso
committed
Add the common case search
Changes: - Refactor the common case search into a function. - Fix the useless Option around the vec in the search_same_list.
1 parent bfb2303 commit fa9f744

File tree

3 files changed

+172
-97
lines changed

3 files changed

+172
-97
lines changed

‎clippy_lints/src/copies.rs‎

Lines changed: 52 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ fn lint_same_cond(cx: &LateContext<'_, '_>, conds: &[&Expr]) {
152152
let eq: &dyn Fn(&&Expr, &&Expr) -> bool =
153153
&|&lhs, &rhs| -> bool { SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, rhs) };
154154

155-
if let Some((i, j)) = search_same(conds, hash, eq) {
155+
for (i, j) in search_same(conds, hash, eq) {
156156
span_note_and_lint(
157157
cx,
158158
IFS_SAME_COND,
@@ -185,48 +185,47 @@ fn lint_match_arms(cx: &LateContext<'_, '_>, expr: &Expr) {
185185
};
186186

187187
let indexed_arms: Vec<(usize, &Arm)> = arms.iter().enumerate().collect();
188-
search_same_list(&indexed_arms, hash, eq).map(|item| {
189-
for match_expr in item {
190-
let (&(_, i), &(_, j)) = match_expr;
191-
192-
span_lint_and_then(
193-
cx,
194-
MATCH_SAME_ARMS,
195-
j.body.span,
196-
"this `match` has identical arm bodies",
197-
|db| {
198-
db.span_note(i.body.span, "same as this");
199-
200-
// Note: this does not use `span_suggestion` on purpose:
201-
// there is no clean way
202-
// to remove the other arm. Building a span and suggest to replace it to ""
203-
// makes an even more confusing error message. Also in order not to make up a
204-
// span for the whole pattern, the suggestion is only shown when there is only
205-
// one pattern. The user should know about `|` if they are already using it…
206-
207-
if i.pats.len() == 1 && j.pats.len() == 1 {
208-
let lhs = snippet(cx, i.pats[0].span, "<pat1>");
209-
let rhs = snippet(cx, j.pats[0].span, "<pat2>");
210-
211-
if let PatKind::Wild = j.pats[0].node {
212-
// if the last arm is _, then i could be integrated into _
213-
// note that i.pats[0] cannot be _, because that would mean that we're
214-
// hiding all the subsequent arms, and rust won't compile
215-
db.span_note(
216-
i.body.span,
217-
&format!(
218-
"`{}` has the same arm body as the `_` wildcard, consider removing it`",
219-
lhs
220-
),
221-
);
222-
} else {
223-
db.span_note(i.body.span, &format!("consider refactoring into `{} | {}`", lhs, rhs));
224-
}
188+
for (&(_, i), &(_, j)) in search_same(&indexed_arms, hash, eq) {
189+
span_lint_and_then(
190+
cx,
191+
MATCH_SAME_ARMS,
192+
j.body.span,
193+
"this `match` has identical arm bodies",
194+
|db| {
195+
db.span_note(i.body.span, "same as this");
196+
197+
// Note: this does not use `span_suggestion` on purpose:
198+
// there is no clean way
199+
// to remove the other arm. Building a span and suggest to replace it to ""
200+
// makes an even more confusing error message. Also in order not to make up a
201+
// span for the whole pattern, the suggestion is only shown when there is only
202+
// one pattern. The user should know about `|` if they are already using it…
203+
204+
if i.pats.len() == 1 && j.pats.len() == 1 {
205+
let lhs = snippet(cx, i.pats[0].span, "<pat1>");
206+
let rhs = snippet(cx, j.pats[0].span, "<pat2>");
207+
208+
if let PatKind::Wild = j.pats[0].node {
209+
// if the last arm is _, then i could be integrated into _
210+
// note that i.pats[0] cannot be _, because that would mean that we're
211+
// hiding all the subsequent arms, and rust won't compile
212+
db.span_note(
213+
i.body.span,
214+
&format!(
215+
"`{}` has the same arm body as the `_` wildcard, consider removing it`",
216+
lhs
217+
),
218+
);
219+
} else {
220+
db.span_help(
221+
i.pats[0].span,
222+
&format!("consider refactoring into `{} | {}`", lhs, rhs),
223+
);
225224
}
226-
},
227-
);
228-
}
229-
});
225+
}
226+
},
227+
);
228+
}
230229
}
231230
}
232231

@@ -327,49 +326,32 @@ where
327326
None
328327
}
329328

330-
fn search_same<T, Hash, Eq>(exprs: &[T], hash: Hash, eq: Eq) -> Option<(&T, &T)>
329+
fn search_common_cases<'a, T, Eq>(exprs: &'a [T], eq: &Eq) -> Option<(&'a T, &'a T)>
331330
where
332-
Hash: Fn(&T) -> u64,
333331
Eq: Fn(&T, &T) -> bool,
334332
{
335-
// common cases
336333
if exprs.len() < 2 {
337-
return None;
334+
None
338335
} else if exprs.len() == 2 {
339-
return if eq(&exprs[0], &exprs[1]) {
336+
if eq(&exprs[0], &exprs[1]) {
340337
Some((&exprs[0], &exprs[1]))
341338
} else {
342339
None
343-
};
344-
}
345-
346-
let mut map: FxHashMap<_, Vec<&_>> =
347-
FxHashMap::with_capacity_and_hasher(exprs.len(), BuildHasherDefault::default());
348-
349-
for expr in exprs {
350-
match map.entry(hash(expr)) {
351-
Entry::Occupied(mut o) => {
352-
for o in o.get() {
353-
if eq(o, expr) {
354-
return Some((o, expr));
355-
}
356-
}
357-
o.get_mut().push(expr);
358-
},
359-
Entry::Vacant(v) => {
360-
v.insert(vec![expr]);
361-
},
362340
}
341+
} else {
342+
None
363343
}
364-
365-
None
366344
}
367345

368-
fn search_same_list<T, Hash, Eq>(exprs: &[T], hash: Hash, eq: Eq) -> Option<Vec<(&T, &T)>>
346+
fn search_same<T, Hash, Eq>(exprs: &[T], hash: Hash, eq: Eq) -> Vec<(&T, &T)>
369347
where
370348
Hash: Fn(&T) -> u64,
371349
Eq: Fn(&T, &T) -> bool,
372350
{
351+
if let Some(expr) = search_common_cases(&exprs, &eq) {
352+
return vec![expr];
353+
}
354+
373355
let mut match_expr_list: Vec<(&T, &T)> = Vec::new();
374356

375357
let mut map: FxHashMap<_, Vec<&_>> =
@@ -391,9 +373,5 @@ where
391373
}
392374
}
393375

394-
if match_expr_list.is_empty() {
395-
None
396-
} else {
397-
Some(match_expr_list)
398-
}
376+
match_expr_list
399377
}

‎tests/ui/match_same_arms.rs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,14 @@ fn match_same_arms() {
116116
52 => 2, //~ ERROR match arms have same body
117117
_ => 0,
118118
};
119+
120+
let _ = match 42 {
121+
1 => 2,
122+
2 => 2, //~ ERROR 2rd matched arms have same body
123+
3 => 2, //~ ERROR 3rd matched arms have same body
124+
4 => 3,
125+
_ => 0,
126+
};
119127
}
120128

121129
fn main() {}

‎tests/ui/match_same_arms.stderr‎

Lines changed: 112 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,48 @@
1+
error: this `match` has identical arm bodies
2+
--> $DIR/match_same_arms.rs:37:14
3+
|
4+
LL | _ => {
5+
| ______________^
6+
LL | | //~ ERROR match arms have same body
7+
LL | | foo();
8+
LL | | let mut a = 42 + [23].len() as i32;
9+
... |
10+
LL | | a
11+
LL | | },
12+
| |_________^
13+
|
14+
= note: `-D clippy::match-same-arms` implied by `-D warnings`
15+
note: same as this
16+
--> $DIR/match_same_arms.rs:28:15
17+
|
18+
LL | 42 => {
19+
| _______________^
20+
LL | | foo();
21+
LL | | let mut a = 42 + [23].len() as i32;
22+
LL | | if true {
23+
... |
24+
LL | | a
25+
LL | | },
26+
| |_________^
27+
note: `42` has the same arm body as the `_` wildcard, consider removing it`
28+
--> $DIR/match_same_arms.rs:28:15
29+
|
30+
LL | 42 => {
31+
| _______________^
32+
LL | | foo();
33+
LL | | let mut a = 42 + [23].len() as i32;
34+
LL | | if true {
35+
... |
36+
LL | | a
37+
LL | | },
38+
| |_________^
39+
140
error: this `match` has identical arm bodies
241
--> $DIR/match_same_arms.rs:52:14
342
|
443
LL | _ => 0, //~ ERROR match arms have same body
544
| ^
645
|
7-
= note: `-D clippy::match-same-arms` implied by `-D warnings`
846
note: same as this
947
--> $DIR/match_same_arms.rs:50:19
1048
|
@@ -27,11 +65,11 @@ note: same as this
2765
|
2866
LL | 42 => foo(),
2967
| ^^^^^
30-
note: consider refactoring into `42 | 51`
31-
--> $DIR/match_same_arms.rs:56:15
68+
help: consider refactoring into `42 | 51`
69+
--> $DIR/match_same_arms.rs:56:9
3270
|
3371
LL | 42 => foo(),
34-
| ^^^^^
72+
| ^^
3573

3674
error: this `match` has identical arm bodies
3775
--> $DIR/match_same_arms.rs:63:17
@@ -44,11 +82,11 @@ note: same as this
4482
|
4583
LL | Some(_) => 24,
4684
| ^^
47-
note: consider refactoring into `Some(_) | None`
48-
--> $DIR/match_same_arms.rs:62:20
85+
help: consider refactoring into `Some(_) | None`
86+
--> $DIR/match_same_arms.rs:62:9
4987
|
5088
LL | Some(_) => 24,
51-
| ^^
89+
| ^^^^^^^
5290

5391
error: this `match` has identical arm bodies
5492
--> $DIR/match_same_arms.rs:85:28
@@ -61,11 +99,11 @@ note: same as this
6199
|
62100
LL | (Some(a), None) => bar(a),
63101
| ^^^^^^
64-
note: consider refactoring into `(Some(a), None) | (None, Some(a))`
65-
--> $DIR/match_same_arms.rs:84:28
102+
help: consider refactoring into `(Some(a), None) | (None, Some(a))`
103+
--> $DIR/match_same_arms.rs:84:9
66104
|
67105
LL | (Some(a), None) => bar(a),
68-
| ^^^^^^
106+
| ^^^^^^^^^^^^^^^
69107

70108
error: this `match` has identical arm bodies
71109
--> $DIR/match_same_arms.rs:91:26
@@ -78,11 +116,11 @@ note: same as this
78116
|
79117
LL | (Some(a), ..) => bar(a),
80118
| ^^^^^^
81-
note: consider refactoring into `(Some(a), ..) | (.., Some(a))`
82-
--> $DIR/match_same_arms.rs:90:26
119+
help: consider refactoring into `(Some(a), ..) | (.., Some(a))`
120+
--> $DIR/match_same_arms.rs:90:9
83121
|
84122
LL | (Some(a), ..) => bar(a),
85-
| ^^^^^^
123+
| ^^^^^^^^^^^^^
86124

87125
error: this `match` has identical arm bodies
88126
--> $DIR/match_same_arms.rs:97:20
@@ -95,11 +133,11 @@ note: same as this
95133
|
96134
LL | (1, .., 3) => 42,
97135
| ^^
98-
note: consider refactoring into `(1, .., 3) | (.., 3)`
99-
--> $DIR/match_same_arms.rs:96:23
136+
help: consider refactoring into `(1, .., 3) | (.., 3)`
137+
--> $DIR/match_same_arms.rs:96:9
100138
|
101139
LL | (1, .., 3) => 42,
102-
| ^^
140+
| ^^^^^^^^^^
103141

104142
error: this `match` has identical arm bodies
105143
--> $DIR/match_same_arms.rs:114:15
@@ -112,11 +150,11 @@ note: same as this
112150
|
113151
LL | 42 => 1,
114152
| ^
115-
note: consider refactoring into `42 | 51`
116-
--> $DIR/match_same_arms.rs:113:15
153+
help: consider refactoring into `42 | 51`
154+
--> $DIR/match_same_arms.rs:113:9
117155
|
118156
LL | 42 => 1,
119-
| ^
157+
| ^^
120158

121159
error: this `match` has identical arm bodies
122160
--> $DIR/match_same_arms.rs:116:15
@@ -129,11 +167,62 @@ note: same as this
129167
|
130168
LL | 41 => 2,
131169
| ^
132-
note: consider refactoring into `41 | 52`
133-
--> $DIR/match_same_arms.rs:115:15
170+
help: consider refactoring into `41 | 52`
171+
--> $DIR/match_same_arms.rs:115:9
134172
|
135173
LL | 41 => 2,
136-
| ^
174+
| ^^
175+
176+
error: this `match` has identical arm bodies
177+
--> $DIR/match_same_arms.rs:122:14
178+
|
179+
LL | 2 => 2, //~ ERROR 2rd matched arms have same body
180+
| ^
181+
|
182+
note: same as this
183+
--> $DIR/match_same_arms.rs:121:14
184+
|
185+
LL | 1 => 2,
186+
| ^
187+
help: consider refactoring into `1 | 2`
188+
--> $DIR/match_same_arms.rs:121:9
189+
|
190+
LL | 1 => 2,
191+
| ^
192+
193+
error: this `match` has identical arm bodies
194+
--> $DIR/match_same_arms.rs:123:14
195+
|
196+
LL | 3 => 2, //~ ERROR 3rd matched arms have same body
197+
| ^
198+
|
199+
note: same as this
200+
--> $DIR/match_same_arms.rs:121:14
201+
|
202+
LL | 1 => 2,
203+
| ^
204+
help: consider refactoring into `1 | 3`
205+
--> $DIR/match_same_arms.rs:121:9
206+
|
207+
LL | 1 => 2,
208+
| ^
209+
210+
error: this `match` has identical arm bodies
211+
--> $DIR/match_same_arms.rs:123:14
212+
|
213+
LL | 3 => 2, //~ ERROR 3rd matched arms have same body
214+
| ^
215+
|
216+
note: same as this
217+
--> $DIR/match_same_arms.rs:122:14
218+
|
219+
LL | 2 => 2, //~ ERROR 2rd matched arms have same body
220+
| ^
221+
help: consider refactoring into `2 | 3`
222+
--> $DIR/match_same_arms.rs:122:9
223+
|
224+
LL | 2 => 2, //~ ERROR 2rd matched arms have same body
225+
| ^
137226

138-
error: aborting due to 8 previous errors
227+
error: aborting due to 12 previous errors
139228

0 commit comments

Comments
 (0)