Skip to content

Commit

Permalink
Auto merge of #12059 - roife:fix/issue-11223, r=Alexendoo
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@bors
bors committed Feb 19, 2024
2 parents 74f611f + 087c7c8 commit fa2a3c5
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 47 deletions.
3 changes: 1 addition & 2 deletions clippy_lints/src/casts/cast_possible_truncation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
66 changes: 39 additions & 27 deletions clippy_lints/src/matches/match_same_arms.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
14 changes: 14 additions & 0 deletions tests/ui/match_same_arms2.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
51 changes: 33 additions & 18 deletions tests/ui/match_same_arms2.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
| -------------^^^^^^^^^^
Expand All @@ -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),
| ----------------^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -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"),
| -----^^^^^^^^^^^^^^^^^^
Expand All @@ -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`
Expand All @@ -128,15 +143,15 @@ 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);
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 {
| ________________^
Expand All @@ -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,
| ---------^^^^^
Expand All @@ -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,
| ---------^^^^^
Expand All @@ -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,
| ----------------------------^^^^^
Expand All @@ -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),
| -^^^^^^^^^^^^^^^^^^^^
Expand All @@ -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

0 comments on commit fa2a3c5

Please sign in to comment.