Skip to content

Commit f0a7673

Browse files
flip1995flip1995
authored
Rollup merge of #4102 - Urriel:fix/4096_match_same_arms, r=flip1995
Fix match_same_arms to fail late Changes: - Add a function search_same_list which return a list of matched expressions - Change the match_same_arms implementation behavior. It will lint each same arms found. fixes #4096 changelog: none
2 parents abe139c + 40f3665 commit f0a7673

File tree

6 files changed

+218
-55
lines changed

6 files changed

+218
-55
lines changed

‎clippy_lints/src/copies.rs

+26-11
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,7 +185,7 @@ fn lint_match_arms(cx: &LateContext<'_, '_>, expr: &Expr) {
185185
};
186186

187187
let indexed_arms: Vec<(usize, &Arm)> = arms.iter().enumerate().collect();
188-
if let Some((&(_, i), &(_, j))) = search_same(&indexed_arms, hash, eq) {
188+
for (&(_, i), &(_, j)) in search_same(&indexed_arms, hash, eq) {
189189
span_lint_and_then(
190190
cx,
191191
MATCH_SAME_ARMS,
@@ -217,7 +217,10 @@ fn lint_match_arms(cx: &LateContext<'_, '_>, expr: &Expr) {
217217
),
218218
);
219219
} else {
220-
db.span_note(i.body.span, &format!("consider refactoring into `{} | {}`", lhs, rhs));
220+
db.span_help(
221+
i.pats[0].span,
222+
&format!("consider refactoring into `{} | {}`", lhs, rhs),
223+
);
221224
}
222225
}
223226
},
@@ -323,21 +326,33 @@ where
323326
None
324327
}
325328

326-
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)>
327330
where
328-
Hash: Fn(&T) -> u64,
329331
Eq: Fn(&T, &T) -> bool,
330332
{
331-
// common cases
332333
if exprs.len() < 2 {
333-
return None;
334+
None
334335
} else if exprs.len() == 2 {
335-
return if eq(&exprs[0], &exprs[1]) {
336+
if eq(&exprs[0], &exprs[1]) {
336337
Some((&exprs[0], &exprs[1]))
337338
} else {
338339
None
339-
};
340+
}
341+
} else {
342+
None
340343
}
344+
}
345+
346+
fn search_same<T, Hash, Eq>(exprs: &[T], hash: Hash, eq: Eq) -> Vec<(&T, &T)>
347+
where
348+
Hash: Fn(&T) -> u64,
349+
Eq: Fn(&T, &T) -> bool,
350+
{
351+
if let Some(expr) = search_common_cases(&exprs, &eq) {
352+
return vec![expr];
353+
}
354+
355+
let mut match_expr_list: Vec<(&T, &T)> = Vec::new();
341356

342357
let mut map: FxHashMap<_, Vec<&_>> =
343358
FxHashMap::with_capacity_and_hasher(exprs.len(), BuildHasherDefault::default());
@@ -347,7 +362,7 @@ where
347362
Entry::Occupied(mut o) => {
348363
for o in o.get() {
349364
if eq(o, expr) {
350-
return Some((o, expr));
365+
match_expr_list.push((o, expr));
351366
}
352367
}
353368
o.get_mut().push(expr);
@@ -358,5 +373,5 @@ where
358373
}
359374
}
360375

361-
None
376+
match_expr_list
362377
}

‎tests/ui/if_same_then_else.rs

+14
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,20 @@ fn if_same_then_else() -> Result<&'static str, ()> {
232232
return Ok(&foo[0..]);
233233
}
234234

235+
if true {
236+
let foo = "";
237+
return Ok(&foo[0..]);
238+
} else if false {
239+
let foo = "bar";
240+
return Ok(&foo[0..]);
241+
} else if true {
242+
let foo = "";
243+
return Ok(&foo[0..]);
244+
} else {
245+
let foo = "";
246+
return Ok(&foo[0..]);
247+
}
248+
235249
// False positive `if_same_then_else`: `let (x, y)` vs. `let (y, x)`; see issue #3559.
236250
if true {
237251
let foo = "";

‎tests/ui/if_same_then_else.stderr

+34-1
Original file line numberDiff line numberDiff line change
@@ -210,5 +210,38 @@ LL | | try!(Ok("foo"));
210210
LL | | } else {
211211
| |_____^
212212

213-
error: aborting due to 10 previous errors
213+
error: this `if` has identical blocks
214+
--> $DIR/if_same_then_else.rs:244:12
215+
|
216+
LL | } else {
217+
| ____________^
218+
LL | | let foo = "";
219+
LL | | return Ok(&foo[0..]);
220+
LL | | }
221+
| |_____^
222+
|
223+
note: same as this
224+
--> $DIR/if_same_then_else.rs:241:20
225+
|
226+
LL | } else if true {
227+
| ____________________^
228+
LL | | let foo = "";
229+
LL | | return Ok(&foo[0..]);
230+
LL | | } else {
231+
| |_____^
232+
233+
error: this `if` has the same condition as a previous if
234+
--> $DIR/if_same_then_else.rs:241:15
235+
|
236+
LL | } else if true {
237+
| ^^^^
238+
|
239+
= note: #[deny(clippy::ifs_same_cond)] on by default
240+
note: same as this
241+
--> $DIR/if_same_then_else.rs:235:8
242+
|
243+
LL | if true {
244+
| ^^^^
245+
246+
error: aborting due to 12 previous errors
214247

‎tests/ui/match_same_arms.rs

+16
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,22 @@ fn match_same_arms() {
108108
(None, Some(a)) => bar(a), // bindings have different types
109109
_ => (),
110110
}
111+
112+
let _ = match 42 {
113+
42 => 1,
114+
51 => 1, //~ ERROR match arms have same body
115+
41 => 2,
116+
52 => 2, //~ ERROR match arms have same body
117+
_ => 0,
118+
};
119+
120+
let _ = match 42 {
121+
1 => 2,
122+
2 => 2, //~ ERROR 2nd matched arms have same body
123+
3 => 2, //~ ERROR 3rd matched arms have same body
124+
4 => 3,
125+
_ => 0,
126+
};
111127
}
112128

113129
fn main() {}

‎tests/ui/match_same_arms.stderr

+101-16
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,11 @@ note: same as this
6565
|
6666
LL | 42 => foo(),
6767
| ^^^^^
68-
note: consider refactoring into `42 | 51`
69-
--> $DIR/match_same_arms.rs:56:15
68+
help: consider refactoring into `42 | 51`
69+
--> $DIR/match_same_arms.rs:56:9
7070
|
7171
LL | 42 => foo(),
72-
| ^^^^^
72+
| ^^
7373

7474
error: this `match` has identical arm bodies
7575
--> $DIR/match_same_arms.rs:63:17
@@ -82,11 +82,11 @@ note: same as this
8282
|
8383
LL | Some(_) => 24,
8484
| ^^
85-
note: consider refactoring into `Some(_) | None`
86-
--> $DIR/match_same_arms.rs:62:20
85+
help: consider refactoring into `Some(_) | None`
86+
--> $DIR/match_same_arms.rs:62:9
8787
|
8888
LL | Some(_) => 24,
89-
| ^^
89+
| ^^^^^^^
9090

9191
error: this `match` has identical arm bodies
9292
--> $DIR/match_same_arms.rs:85:28
@@ -99,11 +99,11 @@ note: same as this
9999
|
100100
LL | (Some(a), None) => bar(a),
101101
| ^^^^^^
102-
note: consider refactoring into `(Some(a), None) | (None, Some(a))`
103-
--> $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
104104
|
105105
LL | (Some(a), None) => bar(a),
106-
| ^^^^^^
106+
| ^^^^^^^^^^^^^^^
107107

108108
error: this `match` has identical arm bodies
109109
--> $DIR/match_same_arms.rs:91:26
@@ -116,11 +116,11 @@ note: same as this
116116
|
117117
LL | (Some(a), ..) => bar(a),
118118
| ^^^^^^
119-
note: consider refactoring into `(Some(a), ..) | (.., Some(a))`
120-
--> $DIR/match_same_arms.rs:90:26
119+
help: consider refactoring into `(Some(a), ..) | (.., Some(a))`
120+
--> $DIR/match_same_arms.rs:90:9
121121
|
122122
LL | (Some(a), ..) => bar(a),
123-
| ^^^^^^
123+
| ^^^^^^^^^^^^^
124124

125125
error: this `match` has identical arm bodies
126126
--> $DIR/match_same_arms.rs:97:20
@@ -133,11 +133,96 @@ note: same as this
133133
|
134134
LL | (1, .., 3) => 42,
135135
| ^^
136-
note: consider refactoring into `(1, .., 3) | (.., 3)`
137-
--> $DIR/match_same_arms.rs:96:23
136+
help: consider refactoring into `(1, .., 3) | (.., 3)`
137+
--> $DIR/match_same_arms.rs:96:9
138138
|
139139
LL | (1, .., 3) => 42,
140-
| ^^
140+
| ^^^^^^^^^^
141+
142+
error: this `match` has identical arm bodies
143+
--> $DIR/match_same_arms.rs:114:15
144+
|
145+
LL | 51 => 1, //~ ERROR match arms have same body
146+
| ^
147+
|
148+
note: same as this
149+
--> $DIR/match_same_arms.rs:113:15
150+
|
151+
LL | 42 => 1,
152+
| ^
153+
help: consider refactoring into `42 | 51`
154+
--> $DIR/match_same_arms.rs:113:9
155+
|
156+
LL | 42 => 1,
157+
| ^^
158+
159+
error: this `match` has identical arm bodies
160+
--> $DIR/match_same_arms.rs:116:15
161+
|
162+
LL | 52 => 2, //~ ERROR match arms have same body
163+
| ^
164+
|
165+
note: same as this
166+
--> $DIR/match_same_arms.rs:115:15
167+
|
168+
LL | 41 => 2,
169+
| ^
170+
help: consider refactoring into `41 | 52`
171+
--> $DIR/match_same_arms.rs:115:9
172+
|
173+
LL | 41 => 2,
174+
| ^^
175+
176+
error: this `match` has identical arm bodies
177+
--> $DIR/match_same_arms.rs:122:14
178+
|
179+
LL | 2 => 2, //~ ERROR 2nd 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 2nd 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 2nd matched arms have same body
225+
| ^
141226

142-
error: aborting due to 7 previous errors
227+
error: aborting due to 12 previous errors
143228

0 commit comments

Comments
 (0)