Skip to content

Commit 0dde3a8

Browse files
Centri3Centri3
committed
check if expr can be pat for if x == Some(2) 
1 parent faebe10 commit 0dde3a8

File tree

4 files changed

+92
-23
lines changed

4 files changed

+92
-23
lines changed

‎clippy_lints/src/redundant_guard.rs‎

Lines changed: 74 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,17 @@
11
use clippy_utils::diagnostics::span_lint_and_then;
22
use clippy_utils::is_from_proc_macro;
33
use clippy_utils::source::snippet_with_applicability;
4+
use clippy_utils::visitors::for_each_expr;
45
use rustc_errors::Applicability;
5-
use rustc_hir::def::Res;
6+
use rustc_hir::def::{DefKind, Res};
67
use rustc_hir::{Arm, BinOpKind, Expr, ExprKind, Guard, HirId, MatchSource, Pat, PatKind};
78
use rustc_lint::LintContext;
89
use rustc_lint::{LateContext, LateLintPass};
910
use rustc_middle::lint::in_external_macro;
1011
use rustc_session::{declare_lint_pass, declare_tool_lint};
1112
use rustc_span::Span;
1213
use std::borrow::Cow;
14+
use std::ops::ControlFlow;
1315

1416
declare_clippy_lint! {
1517
/// ### What it does
@@ -75,10 +77,7 @@ impl<'tcx> LateLintPass<'tcx> for RedundantGuard {
7577
],
7678
MatchSource::Normal,
7779
) = if_expr.kind
78-
&& let ExprKind::Path(qpath) = scrutinee.kind
79-
&& let res = cx.qpath_res(&qpath, scrutinee.hir_id)
80-
&& local_came_from(cx, &res, outer_arm.pat.hir_id)
81-
&& let Some(pat_binding) = cx.tcx.hir().res_span(res)
80+
&& let Some(pat_binding) = get_pat_binding(cx, scrutinee, outer_arm)
8281
{
8382
emit_redundant_guard(
8483
cx,
@@ -97,10 +96,7 @@ impl<'tcx> LateLintPass<'tcx> for RedundantGuard {
9796
// `Some(x) if let Some(2) = x`
9897
if let Guard::IfLet(let_expr) = guard
9998
&& let pat = let_expr.pat
100-
&& let ExprKind::Path(qpath) = let_expr.init.kind
101-
&& let res = cx.qpath_res(&qpath, let_expr.init.hir_id)
102-
&& local_came_from(cx, &res, outer_arm.pat.hir_id)
103-
&& let Some(pat_binding) = cx.tcx.hir().res_span(res)
99+
&& let Some(pat_binding) = get_pat_binding(cx, let_expr.init, outer_arm)
104100
{
105101
emit_redundant_guard(
106102
cx,
@@ -116,21 +112,14 @@ impl<'tcx> LateLintPass<'tcx> for RedundantGuard {
116112
continue;
117113
}
118114

119-
// TODO: I'm not sure if this is always right. Can we always inline rhs? My intuition
120-
// says no, but I can't find a case where it can't be...
121-
122-
// ^ My intuition was correct, This will trigger on `input.span` and any function calls. It should
123-
// not do that
124-
125115
// `Some(x) if x == Some(2)`
126116
if let Guard::If(if_expr) = guard
127117
&& let ExprKind::Binary(bin_op, local, pat) = if_expr.kind
128118
&& matches!(bin_op.node, BinOpKind::Eq)
129-
&& let ExprKind::Path(qpath) = local.kind
130-
&& let res = cx.qpath_res(&qpath, local.hir_id)
131-
&& local_came_from(cx, &res, outer_arm.pat.hir_id)
132-
&& let Some(pat_binding) = cx.tcx.hir().res_span(res)
119+
&& let Some(pat_binding) = get_pat_binding(cx, local, outer_arm)
120+
&& expr_can_be_pat(cx, pat)
133121
{
122+
134123
emit_redundant_guard(
135124
cx,
136125
expr,
@@ -159,6 +148,17 @@ fn remove_binding(
159148
(pat_start, pat_end)
160149
}
161150

151+
fn get_pat_binding(cx: &LateContext<'_>, guard_expr: &Expr<'_>, outer_arm: &Arm<'_>) -> Option<Span> {
152+
if let ExprKind::Path(qpath) = guard_expr.kind
153+
&& let res = cx.qpath_res(&qpath, guard_expr.hir_id)
154+
&& local_came_from(cx, &res, outer_arm.pat.hir_id)
155+
{
156+
return cx.tcx.hir().res_span(res);
157+
}
158+
159+
None
160+
}
161+
162162
fn get_guard(cx: &LateContext<'_>, guard: Option<Guard<'_>>, app: &mut Applicability) -> Cow<'static, str> {
163163
guard.map_or_else(
164164
|| Cow::Borrowed(""),
@@ -220,3 +220,58 @@ fn emit_redundant_guard<'tcx>(
220220
},
221221
);
222222
}
223+
224+
/// Checks if the given `Expr` can also be represented as a `Pat`.
225+
fn expr_can_be_pat(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
226+
fn helper(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
227+
for_each_expr(expr, |expr| {
228+
let expr = expr.peel_blocks();
229+
if match expr.kind {
230+
ExprKind::ConstBlock(..) => cx.tcx.features().inline_const_pat,
231+
ExprKind::Call(c, ..) if let ExprKind::Path(qpath) = c.kind => {
232+
// Allow ctors
233+
matches!(cx.qpath_res(&qpath, c.hir_id), Res::Def(DefKind::Ctor(..), ..))
234+
},
235+
// TODO: This might be incorrect, specifically `AddrOf`
236+
ExprKind::Repeat(expr, ..) | ExprKind::AddrOf(.., expr) => helper(cx, expr),
237+
ExprKind::Array(exprs) | ExprKind::Tup(exprs) => {
238+
for expr in exprs {
239+
if !helper(cx, expr) {
240+
return ControlFlow::Break(());
241+
}
242+
}
243+
244+
true
245+
},
246+
ExprKind::Struct(_, fields, rest) => {
247+
for field in fields {
248+
if !helper(cx, field.expr) {
249+
return ControlFlow::Break(());
250+
}
251+
}
252+
253+
if let Some(rest) = rest {
254+
if !helper(cx, rest) {
255+
return ControlFlow::Break(());
256+
}
257+
}
258+
259+
true
260+
},
261+
ExprKind::Path(qpath) => {
262+
// Can't compare a local with another local in a pat
263+
!matches!(cx.qpath_res(&qpath, expr.hir_id), Res::Local(..))
264+
},
265+
ExprKind::Lit(..) => true,
266+
_ => false,
267+
} {
268+
return ControlFlow::Continue(());
269+
}
270+
271+
ControlFlow::Break(())
272+
})
273+
.is_none()
274+
}
275+
276+
helper(cx, expr)
277+
}

‎tests/ui/redundant_guard.fixed‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
#[macro_use]
88
extern crate proc_macros;
99

10+
struct A(u32);
11+
1012
fn main() {
1113
let x = Some(Some(1));
1214
match x {
@@ -27,6 +29,11 @@ fn main() {
2729
Some(x) if y == 2 => ..,
2830
_ => todo!(),
2931
};
32+
let a = A(1);
33+
match a {
34+
_ if a.0 == 1 => {},
35+
_ => todo!(),
36+
}
3037

3138
external! {
3239
let x = Some(Some(1));

‎tests/ui/redundant_guard.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
#[macro_use]
88
extern crate proc_macros;
99

10+
struct A(u32);
11+
1012
fn main() {
1113
let x = Some(Some(1));
1214
match x {
@@ -27,6 +29,11 @@ fn main() {
2729
Some(x) if y == 2 => ..,
2830
_ => todo!(),
2931
};
32+
let a = A(1);
33+
match a {
34+
_ if a.0 == 1 => {},
35+
_ => todo!(),
36+
}
3037

3138
external! {
3239
let x = Some(Some(1));

‎tests/ui/redundant_guard.stderr‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: redundant guard
2-
--> $DIR/redundant_guard.rs:13:20
2+
--> $DIR/redundant_guard.rs:15:20
33
|
44
LL | Some(x) if matches!(x, Some(1) if true) => ..,
55
| -----------^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -9,23 +9,23 @@ LL | Some(x) if matches!(x, Some(1) if true) => ..,
99
= note: `-D clippy::redundant-guard` implied by `-D warnings`
1010

1111
error: redundant guard
12-
--> $DIR/redundant_guard.rs:14:20
12+
--> $DIR/redundant_guard.rs:16:20
1313
|
1414
LL | Some(x) if matches!(x, Some(1)) => {
1515
| -----------^^^^^^^^^^^^^^^^^^^^
1616
| |
1717
| help: try: `Some(Some(x))`
1818

1919
error: redundant guard
20-
--> $DIR/redundant_guard.rs:18:20
20+
--> $DIR/redundant_guard.rs:20:20
2121
|
2222
LL | Some(x) if let Some(1) = x => ..,
2323
| -----------^^^^^^^^^^^^^^^
2424
| |
2525
| help: try: `Some(Some(1))`
2626

2727
error: redundant guard
28-
--> $DIR/redundant_guard.rs:19:20
28+
--> $DIR/redundant_guard.rs:21:20
2929
|
3030
LL | Some(x) if x == Some(2) => ..,
3131
| -----------^^^^^^^^^^^^

0 commit comments

Comments
 (0)