Skip to content

Commit

Permalink
Rollup merge of rust-lang#4102 - Urriel:fix/4096_match_same_arms, r=f…
Browse files Browse the repository at this point in the history 
…lip1995

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 rust-lang#4096

changelog: none
  • Loading branch information
@flip1995
flip1995 authored May 27, 2019
2 parents abe139c + 40f3665 commit f0a7673
Show file tree
Hide file tree
Showing 6 changed files with 218 additions and 55 deletions.
37 changes: 26 additions & 11 deletions clippy_lints/src/copies.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ fn lint_same_cond(cx: &LateContext<'_, '_>, conds: &[&Expr]) {
let eq: &dyn Fn(&&Expr, &&Expr) -> bool =
&|&lhs, &rhs| -> bool { SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, rhs) };

if let Some((i, j)) = search_same(conds, hash, eq) {
for (i, j) in search_same(conds, hash, eq) {
span_note_and_lint(
cx,
IFS_SAME_COND,
Expand Down Expand Up @@ -185,7 +185,7 @@ fn lint_match_arms(cx: &LateContext<'_, '_>, expr: &Expr) {
};

let indexed_arms: Vec<(usize, &Arm)> = arms.iter().enumerate().collect();
if let Some((&(_, i), &(_, j))) = search_same(&indexed_arms, hash, eq) {
for (&(_, i), &(_, j)) in search_same(&indexed_arms, hash, eq) {
span_lint_and_then(
cx,
MATCH_SAME_ARMS,
Expand Down Expand Up @@ -217,7 +217,10 @@ fn lint_match_arms(cx: &LateContext<'_, '_>, expr: &Expr) {
),
);
} else {
db.span_note(i.body.span, &format!("consider refactoring into `{} | {}`", lhs, rhs));
db.span_help(
i.pats[0].span,
&format!("consider refactoring into `{} | {}`", lhs, rhs),
);
}
}
},
Expand Down Expand Up @@ -323,21 +326,33 @@ where
None
}

fn search_same<T, Hash, Eq>(exprs: &[T], hash: Hash, eq: Eq) -> Option<(&T, &T)>
fn search_common_cases<'a, T, Eq>(exprs: &'a [T], eq: &Eq) -> Option<(&'a T, &'a T)>
where
Hash: Fn(&T) -> u64,
Eq: Fn(&T, &T) -> bool,
{
// common cases
if exprs.len() < 2 {
return None;
None
} else if exprs.len() == 2 {
return if eq(&exprs[0], &exprs[1]) {
if eq(&exprs[0], &exprs[1]) {
Some((&exprs[0], &exprs[1]))
} else {
None
};
}
} else {
None
}
}

fn search_same<T, Hash, Eq>(exprs: &[T], hash: Hash, eq: Eq) -> Vec<(&T, &T)>
where
Hash: Fn(&T) -> u64,
Eq: Fn(&T, &T) -> bool,
{
if let Some(expr) = search_common_cases(&exprs, &eq) {
return vec![expr];
}

let mut match_expr_list: Vec<(&T, &T)> = Vec::new();

let mut map: FxHashMap<_, Vec<&_>> =
FxHashMap::with_capacity_and_hasher(exprs.len(), BuildHasherDefault::default());
Expand All @@ -347,7 +362,7 @@ where
Entry::Occupied(mut o) => {
for o in o.get() {
if eq(o, expr) {
return Some((o, expr));
match_expr_list.push((o, expr));
}
}
o.get_mut().push(expr);
Expand All @@ -358,5 +373,5 @@ where
}
}

None
match_expr_list
}
14 changes: 14 additions & 0 deletions tests/ui/if_same_then_else.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,20 @@ fn if_same_then_else() -> Result<&'static str, ()> {
return Ok(&foo[0..]);
}

if true {
let foo = "";
return Ok(&foo[0..]);
} else if false {
let foo = "bar";
return Ok(&foo[0..]);
} else if true {
let foo = "";
return Ok(&foo[0..]);
} else {
let foo = "";
return Ok(&foo[0..]);
}

// False positive `if_same_then_else`: `let (x, y)` vs. `let (y, x)`; see issue #3559.
if true {
let foo = "";
Expand Down
35 changes: 34 additions & 1 deletion tests/ui/if_same_then_else.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,5 +210,38 @@ LL | | try!(Ok("foo"));
LL | | } else {
| |_____^

error: aborting due to 10 previous errors
error: this `if` has identical blocks
--> $DIR/if_same_then_else.rs:244:12
|
LL | } else {
| ____________^
LL | | let foo = "";
LL | | return Ok(&foo[0..]);
LL | | }
| |_____^
|
note: same as this
--> $DIR/if_same_then_else.rs:241:20
|
LL | } else if true {
| ____________________^
LL | | let foo = "";
LL | | return Ok(&foo[0..]);
LL | | } else {
| |_____^

error: this `if` has the same condition as a previous if
--> $DIR/if_same_then_else.rs:241:15
|
LL | } else if true {
| ^^^^
|
= note: #[deny(clippy::ifs_same_cond)] on by default
note: same as this
--> $DIR/if_same_then_else.rs:235:8
|
LL | if true {
| ^^^^

error: aborting due to 12 previous errors

16 changes: 16 additions & 0 deletions tests/ui/match_same_arms.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,22 @@ fn match_same_arms() {
(None, Some(a)) => bar(a), // bindings have different types
_ => (),
}

let _ = match 42 {
42 => 1,
51 => 1, //~ ERROR match arms have same body
41 => 2,
52 => 2, //~ ERROR match arms have same body
_ => 0,
};

let _ = match 42 {
1 => 2,
2 => 2, //~ ERROR 2nd matched arms have same body
3 => 2, //~ ERROR 3rd matched arms have same body
4 => 3,
_ => 0,
};
}

fn main() {}
117 changes: 101 additions & 16 deletions tests/ui/match_same_arms.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,11 @@ note: same as this
|
LL | 42 => foo(),
| ^^^^^
note: consider refactoring into `42 | 51`
--> $DIR/match_same_arms.rs:56:15
help: consider refactoring into `42 | 51`
--> $DIR/match_same_arms.rs:56:9
|
LL | 42 => foo(),
| ^^^^^
| ^^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:63:17
Expand All @@ -82,11 +82,11 @@ note: same as this
|
LL | Some(_) => 24,
| ^^
note: consider refactoring into `Some(_) | None`
--> $DIR/match_same_arms.rs:62:20
help: consider refactoring into `Some(_) | None`
--> $DIR/match_same_arms.rs:62:9
|
LL | Some(_) => 24,
| ^^
| ^^^^^^^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:85:28
Expand All @@ -99,11 +99,11 @@ note: same as this
|
LL | (Some(a), None) => bar(a),
| ^^^^^^
note: consider refactoring into `(Some(a), None) | (None, Some(a))`
--> $DIR/match_same_arms.rs:84:28
help: consider refactoring into `(Some(a), None) | (None, Some(a))`
--> $DIR/match_same_arms.rs:84:9
|
LL | (Some(a), None) => bar(a),
| ^^^^^^
| ^^^^^^^^^^^^^^^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:91:26
Expand All @@ -116,11 +116,11 @@ note: same as this
|
LL | (Some(a), ..) => bar(a),
| ^^^^^^
note: consider refactoring into `(Some(a), ..) | (.., Some(a))`
--> $DIR/match_same_arms.rs:90:26
help: consider refactoring into `(Some(a), ..) | (.., Some(a))`
--> $DIR/match_same_arms.rs:90:9
|
LL | (Some(a), ..) => bar(a),
| ^^^^^^
| ^^^^^^^^^^^^^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:97:20
Expand All @@ -133,11 +133,96 @@ note: same as this
|
LL | (1, .., 3) => 42,
| ^^
note: consider refactoring into `(1, .., 3) | (.., 3)`
--> $DIR/match_same_arms.rs:96:23
help: consider refactoring into `(1, .., 3) | (.., 3)`
--> $DIR/match_same_arms.rs:96:9
|
LL | (1, .., 3) => 42,
| ^^
| ^^^^^^^^^^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:114:15
|
LL | 51 => 1, //~ ERROR match arms have same body
| ^
|
note: same as this
--> $DIR/match_same_arms.rs:113:15
|
LL | 42 => 1,
| ^
help: consider refactoring into `42 | 51`
--> $DIR/match_same_arms.rs:113:9
|
LL | 42 => 1,
| ^^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:116:15
|
LL | 52 => 2, //~ ERROR match arms have same body
| ^
|
note: same as this
--> $DIR/match_same_arms.rs:115:15
|
LL | 41 => 2,
| ^
help: consider refactoring into `41 | 52`
--> $DIR/match_same_arms.rs:115:9
|
LL | 41 => 2,
| ^^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:122:14
|
LL | 2 => 2, //~ ERROR 2nd matched arms have same body
| ^
|
note: same as this
--> $DIR/match_same_arms.rs:121:14
|
LL | 1 => 2,
| ^
help: consider refactoring into `1 | 2`
--> $DIR/match_same_arms.rs:121:9
|
LL | 1 => 2,
| ^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:123:14
|
LL | 3 => 2, //~ ERROR 3rd matched arms have same body
| ^
|
note: same as this
--> $DIR/match_same_arms.rs:121:14
|
LL | 1 => 2,
| ^
help: consider refactoring into `1 | 3`
--> $DIR/match_same_arms.rs:121:9
|
LL | 1 => 2,
| ^

error: this `match` has identical arm bodies
--> $DIR/match_same_arms.rs:123:14
|
LL | 3 => 2, //~ ERROR 3rd matched arms have same body
| ^
|
note: same as this
--> $DIR/match_same_arms.rs:122:14
|
LL | 2 => 2, //~ ERROR 2nd matched arms have same body
| ^
help: consider refactoring into `2 | 3`
--> $DIR/match_same_arms.rs:122:9
|
LL | 2 => 2, //~ ERROR 2nd matched arms have same body
| ^

error: aborting due to 7 previous errors
error: aborting due to 12 previous errors

Loading

0 comments on commit f0a7673

Please sign in to comment.