Skip to content

Commit 2c4620b

Browse files
committed
Don't lint if local came from an or pattern
Don't lint if local came from an or pattern
1 parent 5c5819e commit 2c4620b

File tree

4 files changed

+63
-53
lines changed

4 files changed

+63
-53
lines changed

clippy_lints/src/matches/redundant_guards.rs

Lines changed: 37 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ use super::REDUNDANT_GUARDS;
1313

1414
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
1515
for outer_arm in arms {
16-
let mut app = Applicability::MaybeIncorrect;
1716
let Some(guard) = outer_arm.guard else {
1817
continue;
1918
};
@@ -34,40 +33,31 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
3433
],
3534
MatchSource::Normal,
3635
) = if_expr.kind
37-
&& let Some((pat_binding, can_use_shorthand)) = get_pat_binding(cx, scrutinee, outer_arm)
3836
{
3937
emit_redundant_guards(
4038
cx,
4139
outer_arm,
4240
if_expr.span,
43-
pat_binding,
44-
can_use_shorthand,
41+
scrutinee,
4542
arm.pat.span,
4643
arm.guard,
47-
&mut app,
4844
);
4945
}
5046
// `Some(x) if let Some(2) = x`
51-
else if let Guard::IfLet(let_expr) = guard
52-
&& let pat = let_expr.pat
53-
&& let Some((pat_binding, can_use_shorthand)) = get_pat_binding(cx, let_expr.init, outer_arm)
54-
{
47+
else if let Guard::IfLet(let_expr) = guard {
5548
emit_redundant_guards(
5649
cx,
5750
outer_arm,
5851
let_expr.span,
59-
pat_binding,
60-
can_use_shorthand,
61-
pat.span,
52+
let_expr.init,
53+
let_expr.pat.span,
6254
None,
63-
&mut app,
6455
);
6556
}
6657
// `Some(x) if x == Some(2)`
6758
else if let Guard::If(if_expr) = guard
6859
&& let ExprKind::Binary(bin_op, local, pat) = if_expr.kind
6960
&& matches!(bin_op.node, BinOpKind::Eq)
70-
&& let Some((pat_binding, can_use_shorthand)) = get_pat_binding(cx, local, outer_arm)
7161
&& expr_can_be_pat(cx, pat)
7262
// Ensure they have the same type. If they don't, we'd need deref coercion which isn't
7363
// possible (currently) in a pattern. In some cases, you can use something like
@@ -81,11 +71,9 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
8171
cx,
8272
outer_arm,
8373
if_expr.span,
84-
pat_binding,
85-
can_use_shorthand,
74+
local,
8675
pat.span,
8776
None,
88-
&mut app,
8977
);
9078
}
9179
}
@@ -94,14 +82,21 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
9482
fn get_pat_binding<'tcx>(cx: &LateContext<'tcx>, guard_expr: &Expr<'_>, outer_arm: &Arm<'tcx>) -> Option<(Span, bool)> {
9583
if let Some(local) = path_to_local(guard_expr) && !is_local_used(cx, outer_arm.body, local) {
9684
let mut span = None;
97-
// FIXME: hir_id == local is only true for the first or, this will always be false
9885
let mut multiple_bindings = false;
99-
outer_arm.pat.each_binding(|_, hir_id, binding_span, _| {
100-
if hir_id == local && span.replace(binding_span).is_some() {
86+
// `each_binding` gives the `HirId` of the `Pat` itself, not the binding
87+
outer_arm.pat.walk(|pat| {
88+
if let PatKind::Binding(_, hir_id, _, _) = pat.kind
89+
&& hir_id == local
90+
&& span.replace(pat.span).is_some()
91+
{
10192
multiple_bindings = true;
93+
return false;
10294
}
95+
96+
true
10397
});
10498

99+
// Ignore bindings from or patterns, like `First(x) | Second(x, _) | Third(x, _, _)`
105100
if !multiple_bindings {
106101
return span.map(|span| {
107102
(
@@ -115,35 +110,26 @@ fn get_pat_binding<'tcx>(cx: &LateContext<'tcx>, guard_expr: &Expr<'_>, outer_ar
115110
None
116111
}
117112

118-
fn get_guard(cx: &LateContext<'_>, guard: Option<Guard<'_>>, app: &mut Applicability) -> String {
119-
guard.map_or_else(String::new, |guard| {
120-
let (prefix, span) = match guard {
121-
Guard::If(e) => ("if", e.span),
122-
Guard::IfLet(l) => ("if let", l.span),
123-
};
124-
125-
format!(" {prefix} {}", snippet_with_applicability(cx, span, "<guard>", app))
126-
})
127-
}
128-
129-
#[expect(clippy::too_many_arguments)]
130-
fn emit_redundant_guards(
131-
cx: &LateContext<'_>,
132-
outer_arm: &Arm<'_>,
113+
fn emit_redundant_guards<'tcx>(
114+
cx: &LateContext<'tcx>,
115+
outer_arm: &Arm<'tcx>,
133116
guard_span: Span,
134-
pat_binding: Span,
135-
can_use_shorthand: bool,
117+
local: &Expr<'_>,
136118
pat_span: Span,
137119
inner_guard: Option<Guard<'_>>,
138-
app: &mut Applicability,
139120
) {
121+
let mut app = Applicability::MaybeIncorrect;
122+
let Some((pat_binding, can_use_shorthand)) = get_pat_binding(cx, local, outer_arm) else {
123+
return;
124+
};
125+
140126
span_lint_and_then(
141127
cx,
142128
REDUNDANT_GUARDS,
143129
guard_span.source_callsite(),
144130
"redundant guard",
145131
|diag| {
146-
let binding_replacement = snippet_with_applicability(cx, pat_span, "<binding_repl>", app);
132+
let binding_replacement = snippet_with_applicability(cx, pat_span, "<binding_repl>", &mut app);
147133
diag.multipart_suggestion_verbose(
148134
"try",
149135
vec![
@@ -154,10 +140,20 @@ fn emit_redundant_guards(
154140
},
155141
(
156142
guard_span.source_callsite().with_lo(outer_arm.pat.span.hi()),
157-
get_guard(cx, inner_guard, app),
143+
inner_guard.map_or_else(String::new, |guard| {
144+
let (prefix, span) = match guard {
145+
Guard::If(e) => ("if", e.span),
146+
Guard::IfLet(l) => ("if let", l.span),
147+
};
148+
149+
format!(
150+
" {prefix} {}",
151+
snippet_with_applicability(cx, span, "<guard>", &mut app),
152+
)
153+
}),
158154
),
159155
],
160-
*app,
156+
app,
161157
);
162158
},
163159
);

tests/ui/redundant_guards.fixed

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
//@run-rustfix
2-
//@aux-build:proc_macros.rs
2+
//@aux-build:proc_macros.rs:proc-macro
33
#![feature(if_let_guard)]
44
#![allow(clippy::no_effect, unused)]
55
#![warn(clippy::redundant_guards)]
@@ -80,22 +80,23 @@ fn main() {
8080
}
8181
}
8282

83+
// Do not lint
84+
8385
enum E {
8486
A(&'static str),
8587
B(&'static str),
8688
C(&'static str),
8789
}
8890

89-
#[allow(clippy::redundant_guards)] // FIXME: hir_id == local is only true for the first or
9091
fn i() {
9192
match E::A("") {
9293
E::A(x) | E::B(x) | E::C(x) if x == "from an or pattern" => {},
94+
// Ok well, this one is fine. Lint it
95+
E::A("not from an or pattern") => {},
9396
_ => {},
94-
}
97+
};
9598
}
9699

97-
// Do not lint
98-
99100
fn f(s: Option<std::ffi::OsString>) {
100101
match s {
101102
Some(x) if x == "a" => {},

tests/ui/redundant_guards.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
//@run-rustfix
2-
//@aux-build:proc_macros.rs
2+
//@aux-build:proc_macros.rs:proc-macro
33
#![feature(if_let_guard)]
44
#![allow(clippy::no_effect, unused)]
55
#![warn(clippy::redundant_guards)]
@@ -80,22 +80,23 @@ fn main() {
8080
}
8181
}
8282

83+
// Do not lint
84+
8385
enum E {
8486
A(&'static str),
8587
B(&'static str),
8688
C(&'static str),
8789
}
8890

89-
#[allow(clippy::redundant_guards)] // FIXME: hir_id == local is only true for the first or
9091
fn i() {
9192
match E::A("") {
9293
E::A(x) | E::B(x) | E::C(x) if x == "from an or pattern" => {},
94+
// Ok well, this one is fine. Lint it
95+
E::A(y) if y == "not from an or pattern" => {},
9396
_ => {},
94-
}
97+
};
9598
}
9699

97-
// Do not lint
98-
99100
fn f(s: Option<std::ffi::OsString>) {
100101
match s {
101102
Some(x) if x == "a" => {},

tests/ui/redundant_guards.stderr

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,19 @@ LL + B { e: Some(A(2)) } => ..,
7171
|
7272

7373
error: redundant guard
74-
--> $DIR/redundant_guards.rs:108:14
74+
--> $DIR/redundant_guards.rs:95:20
75+
|
76+
LL | E::A(y) if y == "not from an or pattern" => {},
77+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
78+
|
79+
help: try
80+
|
81+
LL - E::A(y) if y == "not from an or pattern" => {},
82+
LL + E::A("not from an or pattern") => {},
83+
|
84+
85+
error: redundant guard
86+
--> $DIR/redundant_guards.rs:109:14
7587
|
7688
LL | x if matches!(x, Some(0)) => ..,
7789
| ^^^^^^^^^^^^^^^^^^^^
@@ -82,5 +94,5 @@ LL - x if matches!(x, Some(0)) => ..,
8294
LL + Some(0) => ..,
8395
|
8496

85-
error: aborting due to 7 previous errors
97+
error: aborting due to 8 previous errors
8698

0 commit comments

Comments
 (0)