Skip to content

Commit dc93a16

Browse files
fzlzjerryfzlzjerry
committed
Handle non-adjacent boolean match arms
1 parent 034e59c commit dc93a16

7 files changed

Lines changed: 332 additions & 162 deletions

File tree

‎clippy_lints/src/matches/match_like_matches.rs‎

Lines changed: 108 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -69,133 +69,126 @@ pub(super) fn check_match<'tcx>(
6969
scrutinee: &'tcx Expr<'_>,
7070
arms: &'tcx [Arm<'tcx>],
7171
) -> bool {
72-
if let Some((last_arm, arms_without_last)) = arms.split_last()
73-
&& let Some((first_arm, middle_arms)) = arms_without_last.split_first()
74-
&& !span_contains_comment(cx, e.span)
75-
&& cx.typeck_results().expr_ty(e).is_bool()
76-
&& let Some(b0) = find_bool_lit(first_arm.body)
77-
&& let Some(b1) = find_bool_lit(last_arm.body)
78-
&& b0 != b1
79-
// We handle two cases:
80-
&& (
81-
// - There are no middle arms, i.e., 2 arms in total
82-
//
83-
// In that case, the first arm may or may not have a guard, because this:
84-
// ```rs
85-
// match e {
86-
// Either::Left $(if $guard)|+ => true, // or `false`, but then we'll need `!matches!(..)`
87-
// _ => false,
88-
// }
89-
// ```
90-
// can always become this:
91-
// ```rs
92-
// matches!(e, Either::Left $(if $guard)|+)
93-
// ```
94-
//
95-
// But if the guard _is_ present, it may not be an `if-let` guard, as `matches!` doesn't
96-
// support these (currently?)
97-
(middle_arms.is_empty() && first_arm.guard.is_none_or(|g| !has_let_expr(g)))
98-
99-
// - (added in #6216) There are middle arms
100-
//
101-
// In that case, neither they nor the first arm may have guards
102-
// -- otherwise, they couldn't be combined into an or-pattern in `matches!`
103-
//
104-
// This:
105-
// ```rs
106-
// match e {
107-
// Either3::First => true,
108-
// Either3::Second => true,
109-
// _ /* matches `Either3::Third` */ => false,
110-
// }
111-
// ```
112-
// can become this:
113-
// ```rs
114-
// matches!(e, Either3::First | Either3::Second)
115-
// ```
116-
//
117-
// But this:
118-
// ```rs
119-
// match e {
120-
// Either3::First if X => true,
121-
// Either3::Second => true,
122-
// _ => false,
123-
// }
124-
// ```
125-
// cannot be transformed.
126-
//
127-
// We set an additional constraint of all of them needing to return the same bool,
128-
// so we don't lint things like:
129-
// ```rs
130-
// match e {
131-
// Either3::First => true,
132-
// Either3::Second => false,
133-
// _ => false,
134-
// }
135-
// ```
136-
// This is not *strictly* necessary, but it simplifies the logic a bit
137-
|| arms_without_last.iter().all(|arm| {
138-
cx.tcx.hir_attrs(arm.hir_id).is_empty() && arm.guard.is_none() && find_bool_lit(arm.body) == Some(b0)
139-
})
140-
)
141-
{
142-
if !is_wild(last_arm.pat) {
72+
if arms.len() < 2 || span_contains_comment(cx, e.span) || !cx.typeck_results().expr_ty(e).is_bool() {
73+
return false;
74+
}
75+
76+
let Some(arm_values) = arms
77+
.iter()
78+
.map(|arm| find_bool_lit(arm.body))
79+
.collect::<Option<Vec<_>>>()
80+
else {
81+
return false;
82+
};
83+
84+
let (selected_value, selected_arms, guard) = if let [first_arm, last_arm] = arms {
85+
// A two-arm match may preserve a non-let guard on its first arm.
86+
if arm_values[0] == arm_values[1] || !is_wild(last_arm.pat) || first_arm.guard.is_some_and(has_let_expr) {
87+
return false;
88+
}
89+
90+
(arm_values[0], vec![first_arm], first_arm.guard)
91+
} else {
92+
// Longer matches combine guard-free arms with the selected result into an or-pattern.
93+
// Guards can't be combined into an or-pattern. Attributes may also remove an arm before
94+
// linting, which could change which result should be represented by the pattern.
95+
if arms
96+
.iter()
97+
.any(|arm| !cx.tcx.hir_attrs(arm.hir_id).is_empty() || arm.guard.is_some())
98+
{
14399
return false;
144100
}
145101

146-
for arm in arms_without_last {
147-
let pat = arm.pat;
148-
if !is_lint_allowed(cx, REDUNDANT_PATTERN_MATCHING, pat.hir_id) && is_some_wild(pat.kind) {
102+
// A wildcard can't be part of the suggested pattern, so represent the opposite result. If
103+
// there isn't one, represent the `true` arms directly.
104+
let selected_value = arms
105+
.iter()
106+
.position(|arm| is_wild(arm.pat))
107+
.is_none_or(|index| !arm_values[index]);
108+
109+
if !arm_values.contains(&selected_value) || !arm_values.contains(&!selected_value) {
110+
return false;
111+
}
112+
113+
let selected_arms: Vec<_> = arms
114+
.iter()
115+
.zip(&arm_values)
116+
.filter(|&(_, &value)| value == selected_value)
117+
.map(|(arm, _)| arm)
118+
.collect();
119+
120+
// An earlier arm returning the opposite value takes precedence over a later selected arm.
121+
// Moving that selected pattern into `matches!` is only sound when the two cannot overlap.
122+
for (index, (arm, &value)) in arms.iter().zip(&arm_values).enumerate() {
123+
if value == selected_value
124+
&& arms[..index]
125+
.iter()
126+
.zip(&arm_values[..index])
127+
.any(|(earlier_arm, &earlier_value)| {
128+
earlier_value != selected_value
129+
&& super::pat_overlap::patterns_overlap(cx, earlier_arm.pat, arm.pat)
130+
})
131+
{
149132
return false;
150133
}
151134
}
152135

153-
// The suggestion may be incorrect, because some arms can have `cfg` attributes
154-
// evaluated into `false` and so such arms will be stripped before.
155-
let mut applicability = Applicability::MaybeIncorrect;
156-
let pat = {
157-
use itertools::Itertools as _;
158-
arms_without_last
159-
.iter()
160-
.map(|arm| snippet_with_applicability(cx, arm.pat.span, "..", &mut applicability))
161-
.join(" | ")
162-
};
163-
let pat_and_guard = if let Some(g) = first_arm.guard {
164-
format!(
165-
"{pat} if {}",
166-
snippet_with_applicability(cx, g.span, "..", &mut applicability)
167-
)
168-
} else {
169-
pat
170-
};
136+
(selected_value, selected_arms, None)
137+
};
171138

172-
// strip potential borrows (#6503), but only if the type is a reference
173-
let mut ex_new = scrutinee;
174-
if let ExprKind::AddrOf(BorrowKind::Ref, .., ex_inner) = scrutinee.kind
175-
&& let ty::Ref(..) = cx.typeck_results().expr_ty(ex_inner).kind()
176-
{
177-
ex_new = ex_inner;
139+
for arm in &selected_arms {
140+
let pat = arm.pat;
141+
if !is_lint_allowed(cx, REDUNDANT_PATTERN_MATCHING, pat.hir_id) && is_some_wild(pat.kind) {
142+
return false;
178143
}
144+
}
179145

180-
let (snippet, _) = snippet_with_context(cx, ex_new.span, e.span.ctxt(), "..", &mut applicability);
181-
span_lint_and_then(
182-
cx,
183-
MATCH_LIKE_MATCHES_MACRO,
184-
e.span,
185-
"match expression looks like `matches!` macro",
186-
|diag| {
187-
diag.span_suggestion_verbose(
188-
e.span,
189-
"use `matches!` directly",
190-
format!("{}matches!({snippet}, {pat_and_guard})", if b0 { "" } else { "!" }),
191-
applicability,
192-
);
193-
},
194-
);
195-
true
146+
// The suggestion may be incorrect, because some arms can have `cfg` attributes evaluated into
147+
// `false` and so such arms will be stripped before.
148+
let mut applicability = Applicability::MaybeIncorrect;
149+
let pat = {
150+
use itertools::Itertools as _;
151+
selected_arms
152+
.iter()
153+
.map(|arm| snippet_with_applicability(cx, arm.pat.span, "..", &mut applicability))
154+
.join(" | ")
155+
};
156+
let pat_and_guard = if let Some(g) = guard {
157+
format!(
158+
"{pat} if {}",
159+
snippet_with_applicability(cx, g.span, "..", &mut applicability)
160+
)
196161
} else {
197-
false
162+
pat
163+
};
164+
165+
// strip potential borrows (#6503), but only if the type is a reference
166+
let mut ex_new = scrutinee;
167+
if let ExprKind::AddrOf(BorrowKind::Ref, .., ex_inner) = scrutinee.kind
168+
&& let ty::Ref(..) = cx.typeck_results().expr_ty(ex_inner).kind()
169+
{
170+
ex_new = ex_inner;
198171
}
172+
173+
let (snippet, _) = snippet_with_context(cx, ex_new.span, e.span.ctxt(), "..", &mut applicability);
174+
span_lint_and_then(
175+
cx,
176+
MATCH_LIKE_MATCHES_MACRO,
177+
e.span,
178+
"match expression looks like `matches!` macro",
179+
|diag| {
180+
diag.span_suggestion_verbose(
181+
e.span,
182+
"use `matches!` directly",
183+
format!(
184+
"{}matches!({snippet}, {pat_and_guard})",
185+
if selected_value { "" } else { "!" }
186+
),
187+
applicability,
188+
);
189+
},
190+
);
191+
true
199192
}
200193

201194
/// Extract a `bool` or `{ bool }`

‎clippy_lints/src/matches/pat_overlap.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,11 @@ use rustc_lint::LateContext;
88
use rustc_middle::ty;
99
use rustc_span::{ByteSymbol, ErrorGuaranteed, Symbol};
1010

11+
pub(super) fn patterns_overlap(cx: &LateContext<'_>, lhs: &Pat<'_>, rhs: &Pat<'_>) -> bool {
12+
let arena = DroplessArena::default();
13+
NormalizedPat::from_pat(cx, &arena, lhs).has_overlapping_values(&NormalizedPat::from_pat(cx, &arena, rhs))
14+
}
15+
1116
#[derive(Clone, Copy)]
1217
pub(super) enum NormalizedPat<'a> {
1318
Wild,

‎clippy_lints/src/operators/const_comparisons.rs‎

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -138,24 +138,18 @@ fn left_side_is_useless(left_cmp_op: CmpOp, ordering: Ordering) -> bool {
138138
CmpOp::Le | CmpOp::Ge => true,
139139
}
140140
} else {
141-
match (left_cmp_op.direction(), ordering) {
142-
(CmpOpDirection::Lesser, Ordering::Less) => false,
143-
(CmpOpDirection::Lesser, Ordering::Equal) => false,
144-
(CmpOpDirection::Lesser, Ordering::Greater) => true,
145-
(CmpOpDirection::Greater, Ordering::Less) => true,
146-
(CmpOpDirection::Greater, Ordering::Equal) => false,
147-
(CmpOpDirection::Greater, Ordering::Greater) => false,
148-
}
141+
matches!(
142+
(left_cmp_op.direction(), ordering),
143+
(CmpOpDirection::Lesser, Ordering::Greater) | (CmpOpDirection::Greater, Ordering::Less)
144+
)
149145
}
150146
}
151147

152148
fn comparison_is_possible(left_cmp_direction: CmpOpDirection, ordering: Ordering) -> bool {
153-
match (left_cmp_direction, ordering) {
154-
(CmpOpDirection::Lesser, Ordering::Less | Ordering::Equal) => false,
155-
(CmpOpDirection::Lesser, Ordering::Greater) => true,
156-
(CmpOpDirection::Greater, Ordering::Greater | Ordering::Equal) => false,
157-
(CmpOpDirection::Greater, Ordering::Less) => true,
158-
}
149+
matches!(
150+
(left_cmp_direction, ordering),
151+
(CmpOpDirection::Lesser, Ordering::Greater) | (CmpOpDirection::Greater, Ordering::Less)
152+
)
159153
}
160154

161155
#[derive(PartialEq, Eq, Clone, Copy)]

‎clippy_utils/src/higher.rs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,11 @@ pub enum RangeTy {
297297
RangeToInclusive,
298298
}
299299

300-
#[expect(clippy::match_same_arms, reason = "regularity over density")]
300+
#[expect(
301+
clippy::match_like_matches_macro,
302+
clippy::match_same_arms,
303+
reason = "regularity over density"
304+
)]
301305
impl RangeTy {
302306
/// Returns whether this type implements [`IntoIterator`] — that is, whether it is iterable —
303307
/// presuming that its element type implements the `Step` trait.

‎tests/ui/match_like_matches_macro.fixed‎

Lines changed: 38 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,11 @@ fn main() {
1717
let _z = x.is_none();
1818
//~^^^^ redundant_pattern_matching
1919

20+
// Lint when the more specific lint is explicitly allowed.
21+
#[allow(clippy::redundant_pattern_matching)]
22+
let _allowed = matches!(x, Some(_));
23+
//~^^^^ match_like_matches_macro
24+
2025
// Lint
2126
let _zz = !matches!(x, Some(r) if r == 0);
2227
//~^^^^ match_like_matches_macro
@@ -65,20 +70,20 @@ fn main() {
6570
{
6671
// no lint
6772
let _ans = match x {
68-
E::A(_) => false,
69-
E::B(_) => false,
70-
E::C => true,
73+
E::A(_) => true,
74+
E::B(_) => true,
7175
_ => true,
7276
};
7377
}
7478
{
75-
// no lint
76-
let _ans = match x {
77-
E::A(_) => true,
78-
E::B(_) => false,
79-
E::C => false,
80-
_ => true,
81-
};
79+
// lint
80+
let _ans = !matches!(x, E::A(_) | E::B(_));
81+
//~^^^^^^ match_like_matches_macro
82+
}
83+
{
84+
// lint
85+
let _ans = !matches!(x, E::B(_) | E::C);
86+
//~^^^^^^ match_like_matches_macro
8287
}
8388
{
8489
// no lint
@@ -105,11 +110,29 @@ fn main() {
105110
};
106111
}
107112
{
108-
// no lint
109-
let _ans = match x {
110-
E::A(_) => false,
111-
E::B(_) => true,
112-
_ => false,
113+
// lint
114+
let _ans = matches!(x, E::B(_));
115+
//~^^^^^ match_like_matches_macro
116+
}
117+
{
118+
enum MatchType {
119+
A(u32),
120+
B(u32, u32),
121+
C,
122+
}
123+
124+
// lint: the matching result is split by an arm returning `false`
125+
let _ans = matches!(MatchType::A(0), MatchType::A(_) | MatchType::C);
126+
//~^^^^^ match_like_matches_macro
127+
}
128+
{
129+
#![allow(clippy::match_overlapping_arm)]
130+
131+
// no lint: moving the second arm into an or-pattern would change the result for 5..=10
132+
let _ans = match 7 {
133+
0..=10 => true,
134+
5..=15 => false,
135+
_ => true,
113136
};
114137
}
115138

0 commit comments

Comments
 (0)