Skip to content

Commit 1e4d7de

Browse files
committed
move to matches and make it work on more structs
always return `Break` for rest pattern
1 parent 0dde3a8 commit 1e4d7de

File tree

8 files changed

+355
-292
lines changed

8 files changed

+355
-292
lines changed

clippy_lints/src/declared_lints.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
306306
crate::matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS_INFO,
307307
crate::matches::MATCH_WILD_ERR_ARM_INFO,
308308
crate::matches::NEEDLESS_MATCH_INFO,
309+
crate::matches::REDUNDANT_GUARD_INFO,
309310
crate::matches::REDUNDANT_PATTERN_MATCHING_INFO,
310311
crate::matches::REST_PAT_IN_FULLY_BOUND_STRUCTS_INFO,
311312
crate::matches::SIGNIFICANT_DROP_IN_SCRUTINEE_INFO,
@@ -556,7 +557,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
556557
crate::redundant_closure_call::REDUNDANT_CLOSURE_CALL_INFO,
557558
crate::redundant_else::REDUNDANT_ELSE_INFO,
558559
crate::redundant_field_names::REDUNDANT_FIELD_NAMES_INFO,
559-
crate::redundant_guard::REDUNDANT_GUARD_INFO,
560560
crate::redundant_pub_crate::REDUNDANT_PUB_CRATE_INFO,
561561
crate::redundant_slicing::DEREF_BY_SLICING_INFO,
562562
crate::redundant_slicing::REDUNDANT_SLICING_INFO,

clippy_lints/src/lib.rs

-1
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,6 @@ mod redundant_clone;
273273
mod redundant_closure_call;
274274
mod redundant_else;
275275
mod redundant_field_names;
276-
mod redundant_guard;
277276
mod redundant_pub_crate;
278277
mod redundant_slicing;
279278
mod redundant_static_lifetimes;

clippy_lints/src/matches/mod.rs

+32
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ mod match_wild_enum;
1616
mod match_wild_err_arm;
1717
mod needless_match;
1818
mod overlapping_arms;
19+
mod redundant_guard;
1920
mod redundant_pattern_match;
2021
mod rest_pat_in_fully_bound_struct;
2122
mod significant_drop_in_scrutinee;
@@ -936,6 +937,35 @@ declare_clippy_lint! {
936937
"reimplementation of `filter`"
937938
}
938939

940+
declare_clippy_lint! {
941+
/// ### What it does
942+
/// Checks for unnecessary guards in match expressions.
943+
///
944+
/// ### Why is this bad?
945+
/// It's more complex and much less readable.
946+
///
947+
/// ### Example
948+
/// ```rust,ignore
949+
/// match x {
950+
/// Some(x) if matches!(x, Some(1)) => ..,
951+
/// Some(x) if x == Some(2) => ..,
952+
/// _ => todo!(),
953+
/// }
954+
/// ```
955+
/// Use instead:
956+
/// ```rust,ignore
957+
/// match x {
958+
/// Some(Some(1)) => ..,
959+
/// Some(Some(2)) => ..,
960+
/// _ => todo!(),
961+
/// }
962+
/// ```
963+
#[clippy::version = "1.72.0"]
964+
pub REDUNDANT_GUARD,
965+
complexity,
966+
"checks for unnecessary guards in match expressions"
967+
}
968+
939969
#[derive(Default)]
940970
pub struct Matches {
941971
msrv: Msrv,
@@ -978,6 +1008,7 @@ impl_lint_pass!(Matches => [
9781008
TRY_ERR,
9791009
MANUAL_MAP,
9801010
MANUAL_FILTER,
1011+
REDUNDANT_GUARD,
9811012
]);
9821013

9831014
impl<'tcx> LateLintPass<'tcx> for Matches {
@@ -1025,6 +1056,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
10251056
needless_match::check_match(cx, ex, arms, expr);
10261057
match_on_vec_items::check(cx, ex);
10271058
match_str_case_mismatch::check(cx, ex, arms);
1059+
redundant_guard::check(cx, arms);
10281060

10291061
if !in_constant(cx, expr.hir_id) {
10301062
manual_unwrap_or::check(cx, expr, ex, arms);
+249
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,249 @@
1+
use clippy_utils::diagnostics::span_lint_and_then;
2+
use clippy_utils::source::snippet_with_applicability;
3+
use clippy_utils::visitors::for_each_expr;
4+
use rustc_errors::Applicability;
5+
use rustc_hir::def::{DefKind, Res};
6+
use rustc_hir::{Arm, BinOpKind, Expr, ExprKind, Guard, HirId, MatchSource, Node, Pat, PatField, PatKind};
7+
use rustc_lint::LateContext;
8+
use rustc_span::symbol::Ident;
9+
use rustc_span::Span;
10+
use std::borrow::Cow;
11+
use std::ops::ControlFlow;
12+
13+
use super::REDUNDANT_GUARD;
14+
15+
pub(super) fn check(cx: &LateContext<'_>, arms: &[Arm<'_>]) {
16+
for outer_arm in arms {
17+
let mut app = Applicability::MaybeIncorrect;
18+
let Some(guard) = outer_arm.guard else {
19+
continue;
20+
};
21+
22+
// `Some(x) if matches!(x, y)`
23+
if let Guard::If(if_expr) = guard
24+
&& let ExprKind::Match(
25+
scrutinee,
26+
[
27+
arm,
28+
Arm {
29+
pat: Pat {
30+
kind: PatKind::Wild,
31+
..
32+
},
33+
..
34+
},
35+
],
36+
MatchSource::Normal,
37+
) = if_expr.kind
38+
&& let Some((pat_binding, field_name)) = get_pat_binding(cx, scrutinee, outer_arm)
39+
{
40+
emit_redundant_guard(
41+
cx,
42+
outer_arm,
43+
if_expr.span,
44+
pat_binding,
45+
field_name,
46+
arm.pat.span,
47+
arm.guard,
48+
&mut app,
49+
);
50+
51+
continue;
52+
}
53+
54+
// `Some(x) if let Some(2) = x`
55+
if let Guard::IfLet(let_expr) = guard
56+
&& let pat = let_expr.pat
57+
&& let Some((pat_binding, field_name)) = get_pat_binding(cx, let_expr.init, outer_arm)
58+
{
59+
emit_redundant_guard(
60+
cx,
61+
outer_arm,
62+
let_expr.span,
63+
pat_binding,
64+
field_name,
65+
pat.span,
66+
None,
67+
&mut app,
68+
);
69+
70+
continue;
71+
}
72+
73+
// `Some(x) if x == Some(2)`
74+
if let Guard::If(if_expr) = guard
75+
&& let ExprKind::Binary(bin_op, local, pat) = if_expr.kind
76+
&& matches!(bin_op.node, BinOpKind::Eq)
77+
&& let Some((pat_binding, field_name)) = get_pat_binding(cx, local, outer_arm)
78+
&& expr_can_be_pat(cx, pat)
79+
{
80+
emit_redundant_guard(
81+
cx,
82+
outer_arm,
83+
if_expr.span,
84+
pat_binding,
85+
field_name,
86+
pat.span,
87+
None,
88+
&mut app,
89+
);
90+
91+
continue;
92+
}
93+
}
94+
}
95+
96+
fn remove_binding(
97+
cx: &LateContext<'_>,
98+
pat: &Pat<'_>,
99+
span: Span,
100+
app: &mut Applicability,
101+
) -> (Cow<'static, str>, Cow<'static, str>) {
102+
let pat_start = snippet_with_applicability(cx, pat.span.with_hi(span.lo()), "<start>", app);
103+
let pat_end = snippet_with_applicability(cx, pat.span.with_lo(span.hi()), "<end>", app);
104+
(pat_start, pat_end)
105+
}
106+
107+
fn get_pat_binding(cx: &LateContext<'_>, guard_expr: &Expr<'_>, outer_arm: &Arm<'_>) -> Option<(Span, Option<Ident>)> {
108+
if let ExprKind::Path(qpath) = guard_expr.kind
109+
&& let res = cx.qpath_res(&qpath, guard_expr.hir_id)
110+
&& let Res::Local(hir_id) = res
111+
&& local_came_from_and_not_used(cx, &res, outer_arm.pat.hir_id, outer_arm.body)
112+
{
113+
return cx.tcx.hir().res_span(res).map(|span| {
114+
(
115+
span,
116+
match cx.tcx.hir().get_parent(hir_id) {
117+
Node::PatField(PatField { ident, .. }) => Some(*ident),
118+
_ => None,
119+
},
120+
)
121+
});
122+
}
123+
124+
None
125+
}
126+
127+
fn get_guard(cx: &LateContext<'_>, guard: Option<Guard<'_>>, app: &mut Applicability) -> Cow<'static, str> {
128+
guard.map_or_else(
129+
|| Cow::Borrowed(""),
130+
|guard| {
131+
let (prefix, span) = match guard {
132+
Guard::If(e) => ("if", e.span),
133+
Guard::IfLet(l) => ("if let", l.span),
134+
};
135+
136+
Cow::Owned(format!(
137+
" {prefix} {}",
138+
snippet_with_applicability(cx, span, "<guard>", app),
139+
))
140+
},
141+
)
142+
}
143+
144+
fn local_came_from_and_not_used(cx: &LateContext<'_>, local: &Res, target: HirId, body: &Expr<'_>) -> bool {
145+
if let Res::Local(local) = local
146+
&& cx.tcx.hir().parent_iter(*local).any(|(p, _)| p == target)
147+
&& for_each_expr(body, |expr| {
148+
if let ExprKind::Path(qpath) = expr.kind
149+
&& let Res::Local(local_ref) = cx.qpath_res(&qpath, expr.hir_id)
150+
&& *local == local_ref
151+
{
152+
return ControlFlow::Break(());
153+
}
154+
155+
ControlFlow::Continue(())
156+
})
157+
.is_none()
158+
{
159+
return true;
160+
}
161+
162+
false
163+
}
164+
165+
#[expect(clippy::too_many_arguments)]
166+
fn emit_redundant_guard(
167+
cx: &LateContext<'_>,
168+
outer_arm: &Arm<'_>,
169+
guard_span: Span,
170+
pat_binding: Span,
171+
field_name: Option<Ident>,
172+
pat_span: Span,
173+
inner_guard: Option<Guard<'_>>,
174+
app: &mut Applicability,
175+
) {
176+
let (pat_start, pat_end) = remove_binding(cx, outer_arm.pat, pat_binding, app);
177+
let binding_replacement = snippet_with_applicability(cx, pat_span, "<binding_repl>", app);
178+
179+
span_lint_and_then(
180+
cx,
181+
REDUNDANT_GUARD,
182+
guard_span.source_callsite(),
183+
"redundant guard",
184+
|diag| {
185+
diag.span_suggestion(
186+
outer_arm.span.with_hi(guard_span.source_callsite().hi()),
187+
"try",
188+
format!(
189+
"{pat_start}{}{binding_replacement}{pat_end}{}",
190+
field_name.map_or_else(|| Cow::Borrowed(""), |i| format!("{}: ", i.as_str()).into()),
191+
get_guard(cx, inner_guard, app),
192+
),
193+
*app,
194+
);
195+
},
196+
);
197+
}
198+
199+
/// Checks if the given `Expr` can also be represented as a `Pat`.
200+
fn expr_can_be_pat(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
201+
fn helper(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
202+
for_each_expr(expr, |expr| {
203+
let expr = expr.peel_blocks();
204+
if match expr.kind {
205+
ExprKind::ConstBlock(..) => cx.tcx.features().inline_const_pat,
206+
ExprKind::Call(c, ..) if let ExprKind::Path(qpath) = c.kind => {
207+
// Allow ctors
208+
matches!(cx.qpath_res(&qpath, c.hir_id), Res::Def(DefKind::Ctor(..), ..))
209+
},
210+
ExprKind::AddrOf(.., expr) => helper(cx, expr),
211+
ExprKind::Array(exprs) | ExprKind::Tup(exprs) => {
212+
for expr in exprs {
213+
if !helper(cx, expr) {
214+
return ControlFlow::Break(());
215+
}
216+
}
217+
218+
true
219+
},
220+
ExprKind::Struct(_, fields, rest) => {
221+
for field in fields {
222+
if !helper(cx, field.expr) {
223+
return ControlFlow::Break(());
224+
}
225+
}
226+
227+
if rest.is_some() {
228+
return ControlFlow::Break(());
229+
}
230+
231+
true
232+
},
233+
ExprKind::Path(qpath) => {
234+
// Can't compare a local with another local in a pat
235+
!matches!(cx.qpath_res(&qpath, expr.hir_id), Res::Local(..))
236+
},
237+
ExprKind::Lit(..) => true,
238+
_ => false,
239+
} {
240+
return ControlFlow::Continue(());
241+
}
242+
243+
ControlFlow::Break(())
244+
})
245+
.is_none()
246+
}
247+
248+
helper(cx, expr)
249+
}

0 commit comments

Comments
 (0)