Skip to content

Commit 7f87020

Browse files
committed
add byref checking for the guard's local
1 parent 69fcbfd commit 7f87020

File tree

4 files changed

+176
-21
lines changed

4 files changed

+176
-21
lines changed

clippy_lints/src/matches/redundant_guards.rs

Lines changed: 57 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -34,24 +34,45 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
3434
],
3535
MatchSource::Normal,
3636
) = if_expr.kind
37+
&& let Some((binding_span, is_field, is_byref)) = get_pat_binding(cx, scrutinee, outer_arm)
3738
{
39+
if is_field && is_byref { return; }
40+
let pat_span = if let PatKind::Ref(pat, _) = arm.pat.kind {
41+
if is_byref { pat.span } else { continue; }
42+
} else {
43+
if is_byref { continue; }
44+
arm.pat.span
45+
};
46+
3847
emit_redundant_guards(
3948
cx,
4049
outer_arm,
4150
if_expr.span,
42-
scrutinee,
43-
arm.pat.span,
51+
pat_span,
52+
binding_span,
53+
is_field,
4454
arm.guard,
4555
);
4656
}
4757
// `Some(x) if let Some(2) = x`
48-
else if let Guard::IfLet(let_expr) = guard {
58+
else if let Guard::IfLet(let_expr) = guard
59+
&& let Some((binding_span, is_field, is_byref)) = get_pat_binding(cx, let_expr.init, outer_arm)
60+
{
61+
if is_field && is_byref { return; }
62+
let pat_span = if let PatKind::Ref(pat, _) = let_expr.pat.kind {
63+
if is_byref && !is_field { pat.span } else { continue; }
64+
} else {
65+
if is_byref { continue; }
66+
let_expr.pat.span
67+
};
68+
4969
emit_redundant_guards(
5070
cx,
5171
outer_arm,
5272
let_expr.span,
53-
let_expr.init,
54-
let_expr.pat.span,
73+
pat_span,
74+
binding_span,
75+
is_field,
5576
None,
5677
);
5778
}
@@ -67,31 +88,48 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
6788
//
6889
// This isn't necessary in the other two checks, as they must be a pattern already.
6990
&& cx.typeck_results().expr_ty(local) == cx.typeck_results().expr_ty(pat)
91+
&& let Some((binding_span, is_field, is_byref)) = get_pat_binding(cx, local, outer_arm)
7092
{
93+
if is_field && is_byref { return; }
94+
let pat_span = if let ExprKind::AddrOf(rustc_ast::BorrowKind::Ref, _, expr) = pat.kind {
95+
if is_byref { expr.span } else { continue; }
96+
} else {
97+
if is_byref { continue; }
98+
pat.span
99+
};
100+
71101
emit_redundant_guards(
72102
cx,
73103
outer_arm,
74104
if_expr.span,
75-
local,
76-
pat.span,
105+
pat_span,
106+
binding_span,
107+
is_field,
77108
None,
78109
);
79110
}
80111
}
81112
}
82113

83-
fn get_pat_binding<'tcx>(cx: &LateContext<'tcx>, guard_expr: &Expr<'_>, outer_arm: &Arm<'tcx>) -> Option<(Span, bool)> {
114+
fn get_pat_binding<'tcx>(
115+
cx: &LateContext<'tcx>,
116+
guard_expr: &Expr<'_>,
117+
outer_arm: &Arm<'tcx>,
118+
) -> Option<(Span, bool, bool)> {
84119
if let Some(local) = path_to_local(guard_expr) && !is_local_used(cx, outer_arm.body, local) {
85120
let mut span = None;
86121
let mut multiple_bindings = false;
122+
let mut is_byref = false;
87123
// `each_binding` gives the `HirId` of the `Pat` itself, not the binding
88124
outer_arm.pat.walk(|pat| {
89-
if let PatKind::Binding(_, hir_id, _, _) = pat.kind
125+
if let PatKind::Binding(bind_annot, hir_id, _, _) = pat.kind
90126
&& hir_id == local
91-
&& span.replace(pat.span).is_some()
92127
{
93-
multiple_bindings = true;
94-
return false;
128+
is_byref = matches!(bind_annot.0, rustc_ast::ByRef::Yes);
129+
if span.replace(pat.span).is_some() {
130+
multiple_bindings = true;
131+
return false;
132+
}
95133
}
96134

97135
true
@@ -102,7 +140,8 @@ fn get_pat_binding<'tcx>(cx: &LateContext<'tcx>, guard_expr: &Expr<'_>, outer_ar
102140
return span.map(|span| {
103141
(
104142
span,
105-
!matches!(cx.tcx.hir().get_parent(local), Node::PatField(_)),
143+
matches!(cx.tcx.hir().get_parent(local), Node::PatField(_)),
144+
is_byref,
106145
)
107146
});
108147
}
@@ -115,14 +154,12 @@ fn emit_redundant_guards<'tcx>(
115154
cx: &LateContext<'tcx>,
116155
outer_arm: &Arm<'tcx>,
117156
guard_span: Span,
118-
local: &Expr<'_>,
119157
pat_span: Span,
158+
binding_span: Span,
159+
field_binding: bool,
120160
inner_guard: Option<Guard<'_>>,
121161
) {
122162
let mut app = Applicability::MaybeIncorrect;
123-
let Some((pat_binding, can_use_shorthand)) = get_pat_binding(cx, local, outer_arm) else {
124-
return;
125-
};
126163

127164
span_lint_and_then(
128165
cx,
@@ -134,10 +171,10 @@ fn emit_redundant_guards<'tcx>(
134171
diag.multipart_suggestion_verbose(
135172
"try",
136173
vec![
137-
if can_use_shorthand {
138-
(pat_binding, binding_replacement.into_owned())
174+
if field_binding {
175+
(binding_span.shrink_to_hi(), format!(": {binding_replacement}"))
139176
} else {
140-
(pat_binding.shrink_to_hi(), format!(": {binding_replacement}"))
177+
(binding_span, binding_replacement.into_owned())
141178
},
142179
(
143180
guard_span.source_callsite().with_lo(outer_arm.pat.span.hi()),

tests/ui/redundant_guards.fixed

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,3 +143,44 @@ fn g(opt_s: Option<S>) {
143143
_ => {},
144144
}
145145
}
146+
147+
mod issue11465 {
148+
enum A {
149+
Foo([u8; 3]),
150+
}
151+
152+
struct B {
153+
b: String,
154+
c: i32,
155+
}
156+
157+
fn issue11465() {
158+
let c = Some(1);
159+
match c {
160+
Some(1) => {},
161+
Some(2) => {},
162+
Some(3) => {},
163+
_ => {},
164+
};
165+
166+
let enum_a = A::Foo([98, 97, 114]);
167+
match enum_a {
168+
A::Foo(ref arr) if arr == b"foo" => {},
169+
A::Foo(ref arr) if let b"bar" = arr => {},
170+
A::Foo(ref arr) if matches!(arr, b"baz") => {},
171+
_ => {},
172+
};
173+
174+
let struct_b = B {
175+
b: "bar".to_string(),
176+
c: 42,
177+
};
178+
match struct_b {
179+
B { ref b, .. } if b == "bar" => {},
180+
B { ref c, .. } if c == &1 => {},
181+
B { ref c, .. } if let &1 = c => {},
182+
B { ref c, .. } if matches!(c, &1) => {},
183+
_ => {},
184+
}
185+
}
186+
}

tests/ui/redundant_guards.rs

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,3 +143,44 @@ fn g(opt_s: Option<S>) {
143143
_ => {},
144144
}
145145
}
146+
147+
mod issue11465 {
148+
enum A {
149+
Foo([u8; 3]),
150+
}
151+
152+
struct B {
153+
b: String,
154+
c: i32,
155+
}
156+
157+
fn issue11465() {
158+
let c = Some(1);
159+
match c {
160+
Some(ref x) if x == &1 => {},
161+
Some(ref x) if let &2 = x => {},
162+
Some(ref x) if matches!(x, &3) => {},
163+
_ => {},
164+
};
165+
166+
let enum_a = A::Foo([98, 97, 114]);
167+
match enum_a {
168+
A::Foo(ref arr) if arr == b"foo" => {},
169+
A::Foo(ref arr) if let b"bar" = arr => {},
170+
A::Foo(ref arr) if matches!(arr, b"baz") => {},
171+
_ => {},
172+
};
173+
174+
let struct_b = B {
175+
b: "bar".to_string(),
176+
c: 42,
177+
};
178+
match struct_b {
179+
B { ref b, .. } if b == "bar" => {},
180+
B { ref c, .. } if c == &1 => {},
181+
B { ref c, .. } if let &1 = c => {},
182+
B { ref c, .. } if matches!(c, &1) => {},
183+
_ => {},
184+
}
185+
}
186+
}

tests/ui/redundant_guards.stderr

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,5 +94,41 @@ LL - x if matches!(x, Some(0)) => ..,
9494
LL + Some(0) => ..,
9595
|
9696

97-
error: aborting due to 8 previous errors
97+
error: redundant guard
98+
--> $DIR/redundant_guards.rs:160:28
99+
|
100+
LL | Some(ref x) if x == &1 => {},
101+
| ^^^^^^^
102+
|
103+
help: try
104+
|
105+
LL - Some(ref x) if x == &1 => {},
106+
LL + Some(1) => {},
107+
|
108+
109+
error: redundant guard
110+
--> $DIR/redundant_guards.rs:161:28
111+
|
112+
LL | Some(ref x) if let &2 = x => {},
113+
| ^^^^^^^^^^
114+
|
115+
help: try
116+
|
117+
LL - Some(ref x) if let &2 = x => {},
118+
LL + Some(2) => {},
119+
|
120+
121+
error: redundant guard
122+
--> $DIR/redundant_guards.rs:162:28
123+
|
124+
LL | Some(ref x) if matches!(x, &3) => {},
125+
| ^^^^^^^^^^^^^^^
126+
|
127+
help: try
128+
|
129+
LL - Some(ref x) if matches!(x, &3) => {},
130+
LL + Some(3) => {},
131+
|
132+
133+
error: aborting due to 11 previous errors
98134

0 commit comments

Comments
 (0)