Skip to content

Commit 87e4c91

Browse files
Fixes a case where collapsible_match suggested a transformation that changes runtime behavior. (#16878)
Closes #16875 ### What was wrong The lint suggested collapsing `pattern => { if test { body } }` into `pattern if test => { body }`. These are not equivalent. In the original, once `pattern` matches, the match exits regardless of `test`. In the suggestion, a failing guard allows fall-through to subsequent arms, which can match values that previously never reached them. ```rust // Before (original) match a { Some(_) => { if b == 0 { res = 1 } } _ if b == 1 => res = 2, // unreachable when a is Some(_) _ => {} } // After (suggestion — WRONG, changes behavior) match a { Some(_) if b == 0 => res = 1, _ if b == 1 => res = 2, // now reachable when a is Some(_) and b != 0 _ => {} } ``` ### Fix The `if`-guard collapsing is only safe when there are no non-wildcard arms between the current arm and the wildcard arm. The fix adds a check that all arms after the current one are "wild-like" (a bare `_`, a binding, or `None`, all without guards) before suggesting the transformation. As a side effect, three `#[expect(clippy::collapsible_match)]` suppressions in `clippy_lints/src/methods/mod.rs` are removed. They had been added to suppress this exact false positive and are no longer needed. ### Still lints correctly When only wildcard arms follow, the transformation is safe and the lint still fires: ```rust // This still lints — only `_ => {}` follows, fall-through is harmless match a { Some(_) => { if b == 0 { res = 1 } // triggers collapsible_match } _ => {} } ``` changelog: [`collapsible_match`]: no longer suggests collapsing `pattern => { if test { body } }` into a match guard when non-wildcard arms follow, as this changes the semantics of the match. <!-- TRIAGEBOT_START --> <!-- TRIAGEBOT_SUMMARY_START --> ### Summary Notes - [Beta-nomination](#16878 (review)) by [samueltardieu](https://github.com/samueltardieu) *Managed by `@rustbot`—see [help](https://forge.rust-lang.org/triagebot/note.html) for details* <!-- TRIAGEBOT_SUMMARY_END --> <!-- TRIAGEBOT_END -->
2 parents 31a89ea + c7cadb6 commit 87e4c91

6 files changed

Lines changed: 89 additions & 11 deletions

File tree

clippy_lints/src/matches/collapsible_match.rs

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,20 @@ use super::{COLLAPSIBLE_MATCH, pat_contains_disallowed_or};
2323

2424
pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, arms: &'tcx [Arm<'_>], msrv: Msrv) {
2525
if let Some(els_arm) = arms.iter().rfind(|arm| arm_is_wild_like(cx, arm)) {
26-
for arm in arms {
27-
check_arm(cx, true, arm.pat, expr, arm.body, arm.guard, Some(els_arm.body), msrv);
26+
let last_non_wildcard = arms.iter().rposition(|arm| !arm_is_wild_like(cx, arm));
27+
for (idx, arm) in arms.iter().enumerate() {
28+
let only_wildcards_after = last_non_wildcard.is_none_or(|lnw| idx >= lnw);
29+
check_arm(
30+
cx,
31+
true,
32+
arm.pat,
33+
expr,
34+
arm.body,
35+
arm.guard,
36+
Some(els_arm.body),
37+
msrv,
38+
only_wildcards_after,
39+
);
2840
}
2941
}
3042
}
@@ -37,7 +49,7 @@ pub(super) fn check_if_let<'tcx>(
3749
let_expr: &'tcx Expr<'_>,
3850
msrv: Msrv,
3951
) {
40-
check_arm(cx, false, pat, let_expr, body, None, else_expr, msrv);
52+
check_arm(cx, false, pat, let_expr, body, None, else_expr, msrv, false);
4153
}
4254

4355
#[expect(clippy::too_many_arguments, clippy::too_many_lines)]
@@ -50,6 +62,7 @@ fn check_arm<'tcx>(
5062
outer_guard: Option<&'tcx Expr<'tcx>>,
5163
outer_else_body: Option<&'tcx Expr<'tcx>>,
5264
msrv: Msrv,
65+
only_wildcards_after: bool,
5366
) {
5467
let inner_expr = peel_blocks_with_stmt(outer_then_body);
5568
if let Some(inner) = IfLetOrMatch::parse(cx, inner_expr)
@@ -126,6 +139,7 @@ fn check_arm<'tcx>(
126139
);
127140
});
128141
} else if outer_is_match // Leave if-let to the `collapsible_if` lint
142+
&& only_wildcards_after // adding a guard allows fall-through; unsafe if other arms follow
129143
&& let Some(inner) = If::hir(inner_expr)
130144
&& outer_pat.span.eq_ctxt(inner.cond.span)
131145
&& match (outer_else_body, inner.r#else) {

clippy_lints/src/methods/mod.rs

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5221,7 +5221,6 @@ impl Methods {
52215221
format_collect::check(cx, expr, m_arg, m_ident_span);
52225222
},
52235223
Some((sym::take, take_self_arg, [take_arg], _, _)) => {
5224-
#[expect(clippy::collapsible_match)]
52255224
if self.msrv.meets(cx, msrvs::STR_REPEAT) {
52265225
manual_str_repeat::check(cx, expr, recv, take_self_arg, take_arg);
52275226
}
@@ -5546,9 +5545,7 @@ impl Methods {
55465545
(sym::open, [_]) => {
55475546
open_options::check(cx, expr, recv);
55485547
},
5549-
(sym::or_else, [arg]) =>
5550-
{
5551-
#[expect(clippy::collapsible_match)]
5548+
(sym::or_else, [arg]) => {
55525549
if !bind_instead_of_map::check_or_else_err(cx, expr, recv, arg) {
55535550
unnecessary_lazy_eval::check(cx, expr, recv, arg, "or");
55545551
}
@@ -5653,9 +5650,7 @@ impl Methods {
56535650
(sym::try_into, []) if cx.ty_based_def(expr).opt_parent(cx).is_diag_item(cx, sym::TryInto) => {
56545651
unnecessary_fallible_conversions::check_method(cx, expr);
56555652
},
5656-
(sym::to_owned, []) =>
5657-
{
5658-
#[expect(clippy::collapsible_match)]
5653+
(sym::to_owned, []) => {
56595654
if !suspicious_to_owned::check(cx, expr, span) {
56605655
implicit_clone::check(cx, name, expr, recv);
56615656
}

tests/ui/collapsible_match.rs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -390,6 +390,26 @@ fn take<T>(t: T) {}
390390

391391
fn main() {}
392392

393+
// https://github.com/rust-lang/rust-clippy/issues/16875
394+
// Adding a match guard allows fall-through to subsequent arms, which changes semantics
395+
// when non-wildcard arms follow the arm being collapsed.
396+
fn issue16875(a: Option<&str>, b: i32) -> i32 {
397+
let mut res = 0;
398+
// should NOT lint: `_ if b == 1` is not wild-like (has a guard), so collapsing
399+
// `Some(_)` into `Some(_) if b == 0` would let `_ if b == 1` match Some values
400+
// that previously fell through to the no-op arm body.
401+
match a {
402+
Some(_) => {
403+
if b == 0 {
404+
res = 1;
405+
}
406+
},
407+
_ if b == 1 => res = 2,
408+
_ => {},
409+
}
410+
res
411+
}
412+
393413
fn issue16705(x: Option<String>) {
394414
fn takes_ownership(s: String) -> bool {
395415
true

tests/ui/collapsible_match_fixable.fixed

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,3 +28,18 @@ fn issue16558() {
2828
_ => 1,
2929
};
3030
}
31+
32+
// https://github.com/rust-lang/rust-clippy/issues/16875
33+
// lint still fires when only wildcard-like arms follow (fall-through is harmless)
34+
fn issue16875(a: Option<&str>, b: i32) -> i32 {
35+
let mut res = 0;
36+
match a {
37+
Some(_)
38+
if b == 0 => {
39+
//~^ collapsible_match
40+
res = 1;
41+
},
42+
_ => {},
43+
}
44+
res
45+
}

tests/ui/collapsible_match_fixable.rs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,3 +29,19 @@ fn issue16558() {
2929
_ => 1,
3030
};
3131
}
32+
33+
// https://github.com/rust-lang/rust-clippy/issues/16875
34+
// lint still fires when only wildcard-like arms follow (fall-through is harmless)
35+
fn issue16875(a: Option<&str>, b: i32) -> i32 {
36+
let mut res = 0;
37+
match a {
38+
Some(_) => {
39+
if b == 0 {
40+
//~^ collapsible_match
41+
res = 1;
42+
}
43+
},
44+
_ => {},
45+
}
46+
res
47+
}

tests/ui/collapsible_match_fixable.stderr

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,5 +46,23 @@ LL |
4646
LL ~ ,
4747
|
4848

49-
error: aborting due to 3 previous errors
49+
error: this `if` can be collapsed into the outer `match`
50+
--> tests/ui/collapsible_match_fixable.rs:39:13
51+
|
52+
LL | / if b == 0 {
53+
LL | |
54+
LL | | res = 1;
55+
LL | | }
56+
| |_____________^
57+
|
58+
help: collapse nested if block
59+
|
60+
LL ~ Some(_)
61+
LL ~ if b == 0 => {
62+
LL |
63+
LL | res = 1;
64+
LL ~ },
65+
|
66+
67+
error: aborting due to 4 previous errors
5068

0 commit comments

Comments
 (0)