mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #12059 - roife:fix/issue-11223, r=Alexendoo

Browse Source 
Fix issue 11223: add check for identical guards in lint `match_same_arms`

fixes #11223

In the current `match_same_arms` implementation, if arms have guards, they are considered different. This commit adds equality checking for guards: arms are now considered equivalent only when they either both have no guards or their guards are identical.

The portion responsible for checking guard equality is refactored to reuse the existing code for checking body equality. This is abstracted into a function called `check_eq_with_pat`. To optimize performance, `check_same_guard` and `check_same_body` here use closures for lazy evaluation, ensuring that the calculation is only performed when `!(backwards_blocking_idxs...)` is true.

changelog: [`match_same_arms`]: Add check for identical guards
This commit is contained in:
bors 2024-02-19 14:03:56 +00:00
commit fa2a3c5208
4 changed files with 87 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
clippy_lints/src/casts/cast_possible_truncation.rs
View File

@ -131,8 +131,7 @@ pub(super) fn check(
let cast_from_ptr_size = def.repr().int.map_or(true, |ty| matches!(ty, IntegerType::Pointer(_),));
let suffix = match (cast_from_ptr_size, is_isize_or_usize(cast_to)) {
(false, false) if from_nbits > to_nbits => "",
(true, false) if from_nbits > to_nbits => "",
(_, false) if from_nbits > to_nbits => "",
(false, true) if from_nbits > 64 => "",
(false, true) if from_nbits > 32 => " on targets with 32-bit wide pointers",
_ => return,

66
clippy_lints/src/matches/match_same_arms.rs
View File

@ -64,38 +64,50 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) {
let min_index = usize::min(lindex, rindex);
let max_index = usize::max(lindex, rindex);
let mut local_map: HirIdMap<HirId> = HirIdMap::default();
let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
if let Some(a_id) = path_to_local(a)
&& let Some(b_id) = path_to_local(b)
&& let entry = match local_map.entry(a_id) {
HirIdMapEntry::Vacant(entry) => entry,
// check if using the same bindings as before
HirIdMapEntry::Occupied(entry) => return *entry.get() == b_id,
}
let check_eq_with_pat = |expr_a: &Expr<'_>, expr_b: &Expr<'_>| {
let mut local_map: HirIdMap<HirId> = HirIdMap::default();
let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
if let Some(a_id) = path_to_local(a)
&& let Some(b_id) = path_to_local(b)
&& let entry = match local_map.entry(a_id) {
HirIdMapEntry::Vacant(entry) => entry,
// check if using the same bindings as before
HirIdMapEntry::Occupied(entry) => return *entry.get() == b_id,
}
// the names technically don't have to match; this makes the lint more conservative
&& cx.tcx.hir().name(a_id) == cx.tcx.hir().name(b_id)
&& cx.typeck_results().expr_ty(a) == cx.typeck_results().expr_ty(b)
&& pat_contains_local(lhs.pat, a_id)
&& pat_contains_local(rhs.pat, b_id)
{
entry.insert(b_id);
true
} else {
false
}
};
// Arms with a guard are ignored, those can’t always be merged together
// If both arms overlap with an arm in between then these can't be merged either.
!(backwards_blocking_idxs[max_index] > min_index && forwards_blocking_idxs[min_index] < max_index)
&& lhs.guard.is_none()
&& rhs.guard.is_none()
&& SpanlessEq::new(cx)
.expr_fallback(eq_fallback)
.eq_expr(lhs.body, rhs.body)
&& cx.typeck_results().expr_ty(a) == cx.typeck_results().expr_ty(b)
&& pat_contains_local(lhs.pat, a_id)
&& pat_contains_local(rhs.pat, b_id)
{
entry.insert(b_id);
true
} else {
false
}
};
SpanlessEq::new(cx)
.expr_fallback(eq_fallback)
.eq_expr(expr_a, expr_b)
// these checks could be removed to allow unused bindings
&& bindings_eq(lhs.pat, local_map.keys().copied().collect())
&& bindings_eq(rhs.pat, local_map.values().copied().collect())
};
let check_same_guard = || match (&lhs.guard, &rhs.guard) {
(None, None) => true,
(Some(lhs_guard), Some(rhs_guard)) => check_eq_with_pat(lhs_guard, rhs_guard),
_ => false,
};
let check_same_body = || check_eq_with_pat(lhs.body, rhs.body);
// Arms with different guard are ignored, those can’t always be merged together
// If both arms overlap with an arm in between then these can't be merged either.
!(backwards_blocking_idxs[max_index] > min_index && forwards_blocking_idxs[min_index] < max_index)
&& check_same_guard()
&& check_same_body()
};
let indexed_arms: Vec<(usize, &Arm<'_>)> = arms.iter().enumerate().collect();

14
tests/ui/match_same_arms2.rs
View File

@ -67,6 +67,20 @@ fn match_same_arms() {
_ => (),
}
// No warning because guards are different
let _ = match Some(42) {
Some(a) if a == 42 => a,
Some(a) if a == 24 => a,
Some(_) => 24,
None => 0,
};
let _ = match (Some(42), Some(42)) {
(Some(a), None) if a == 42 => a,
(None, Some(a)) if a == 42 => a, //~ ERROR: this match arm has an identical body to another arm
_ => 0,
};
match (Some(42), Some(42)) {
(Some(a), ..) => bar(a), //~ ERROR: this match arm has an identical body to another arm
(.., Some(a)) => bar(a),

51
tests/ui/match_same_arms2.stderr
View File

@ -71,7 +71,22 @@ LL | (Some(a), None) => bar(a),
| ^^^^^^^^^^^^^^^^^^^^^^^^^
error: this match arm has an identical body to another arm
--> tests/ui/match_same_arms2.rs:71:9
--> tests/ui/match_same_arms2.rs:80:9
|
LL | (None, Some(a)) if a == 42 => a,
| ---------------^^^^^^^^^^^^^^^^
| |
| help: try merging the arm patterns: `(None, Some(a)) | (Some(a), None)`
|
= help: or try changing either arm body
note: other arm here
--> tests/ui/match_same_arms2.rs:79:9
|
LL | (Some(a), None) if a == 42 => a,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: this match arm has an identical body to another arm
--> tests/ui/match_same_arms2.rs:85:9
|
LL | (Some(a), ..) => bar(a),
| -------------^^^^^^^^^^
@ -80,13 +95,13 @@ LL | (Some(a), ..) => bar(a),
|
= help: or try changing either arm body
note: other arm here
--> tests/ui/match_same_arms2.rs:72:9
--> tests/ui/match_same_arms2.rs:86:9
|
LL | (.., Some(a)) => bar(a),
| ^^^^^^^^^^^^^^^^^^^^^^^
error: this match arm has an identical body to another arm
--> tests/ui/match_same_arms2.rs:105:9
--> tests/ui/match_same_arms2.rs:119:9
|
LL | (Ok(x), Some(_)) => println!("ok {}", x),
| ----------------^^^^^^^^^^^^^^^^^^^^^^^^
@ -95,13 +110,13 @@ LL | (Ok(x), Some(_)) => println!("ok {}", x),
|
= help: or try changing either arm body
note: other arm here
--> tests/ui/match_same_arms2.rs:106:9
--> tests/ui/match_same_arms2.rs:120:9
|
LL | (Ok(_), Some(x)) => println!("ok {}", x),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: this match arm has an identical body to another arm
--> tests/ui/match_same_arms2.rs:121:9
--> tests/ui/match_same_arms2.rs:135:9
|
LL | Ok(_) => println!("ok"),
| -----^^^^^^^^^^^^^^^^^^
@ -110,13 +125,13 @@ LL | Ok(_) => println!("ok"),
|
= help: or try changing either arm body
note: other arm here
--> tests/ui/match_same_arms2.rs:120:9
--> tests/ui/match_same_arms2.rs:134:9
|
LL | Ok(3) => println!("ok"),
| ^^^^^^^^^^^^^^^^^^^^^^^
error: this match arm has an identical body to another arm
--> tests/ui/match_same_arms2.rs:148:9
--> tests/ui/match_same_arms2.rs:162:9
|
LL | 1 => {
| ^ help: try merging the arm patterns: `1 | 0`
@ -128,7 +143,7 @@ LL | | },
|
= help: or try changing either arm body
note: other arm here
--> tests/ui/match_same_arms2.rs:145:9
--> tests/ui/match_same_arms2.rs:159:9
|
LL | / 0 => {
LL | | empty!(0);
@ -136,7 +151,7 @@ LL | | },
| |_________^
error: match expression looks like `matches!` macro
--> tests/ui/match_same_arms2.rs:167:16
--> tests/ui/match_same_arms2.rs:181:16
|
LL | let _ans = match x {
| ________________^
@ -150,7 +165,7 @@ LL | | };
= help: to override `-D warnings` add `#[allow(clippy::match_like_matches_macro)]`
error: this match arm has an identical body to another arm
--> tests/ui/match_same_arms2.rs:199:9
--> tests/ui/match_same_arms2.rs:213:9
|
LL | Foo::X(0) => 1,
| ---------^^^^^
@ -159,13 +174,13 @@ LL | Foo::X(0) => 1,
|
= help: or try changing either arm body
note: other arm here
--> tests/ui/match_same_arms2.rs:201:9
--> tests/ui/match_same_arms2.rs:215:9
|
LL | Foo::Z(_) => 1,
| ^^^^^^^^^^^^^^
error: this match arm has an identical body to another arm
--> tests/ui/match_same_arms2.rs:209:9
--> tests/ui/match_same_arms2.rs:223:9
|
LL | Foo::Z(_) => 1,
| ---------^^^^^
@ -174,13 +189,13 @@ LL | Foo::Z(_) => 1,
|
= help: or try changing either arm body
note: other arm here
--> tests/ui/match_same_arms2.rs:207:9
--> tests/ui/match_same_arms2.rs:221:9
|
LL | Foo::X(0) => 1,
| ^^^^^^^^^^^^^^
error: this match arm has an identical body to another arm
--> tests/ui/match_same_arms2.rs:232:9
--> tests/ui/match_same_arms2.rs:246:9
|
LL | Some(Bar { y: 0, x: 5, .. }) => 1,
| ----------------------------^^^^^
@ -189,13 +204,13 @@ LL | Some(Bar { y: 0, x: 5, .. }) => 1,
|
= help: or try changing either arm body
note: other arm here
--> tests/ui/match_same_arms2.rs:229:9
--> tests/ui/match_same_arms2.rs:243:9
|
LL | Some(Bar { x: 0, y: 5, .. }) => 1,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: this match arm has an identical body to another arm
--> tests/ui/match_same_arms2.rs:246:9
--> tests/ui/match_same_arms2.rs:260:9
|
LL | 1 => cfg!(not_enable),
| -^^^^^^^^^^^^^^^^^^^^
@ -204,10 +219,10 @@ LL | 1 => cfg!(not_enable),
|
= help: or try changing either arm body
note: other arm here
--> tests/ui/match_same_arms2.rs:245:9
--> tests/ui/match_same_arms2.rs:259:9
|
LL | 0 => cfg!(not_enable),
| ^^^^^^^^^^^^^^^^^^^^^
error: aborting due to 13 previous errors
error: aborting due to 14 previous errors