Skip to content

Commit b49700d

Browse files
est31est31
committed
Also support linting for match
1 parent 37d0a28 commit b49700d

File tree

5 files changed

+257
-29
lines changed

5 files changed

+257
-29
lines changed

‎clippy_lints/src/manual_let_else.rs

Lines changed: 86 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
use clippy_utils::diagnostics::span_lint;
2+
use clippy_utils::higher::IfLetOrMatch;
23
use clippy_utils::visitors::{for_each_expr, Descend};
3-
use clippy_utils::{higher, meets_msrv, msrvs, peel_blocks};
4+
use clippy_utils::{meets_msrv, msrvs, peel_blocks};
45
use if_chain::if_chain;
5-
use rustc_hir::{Expr, ExprKind, Pat, QPath, Stmt, StmtKind};
6+
use rustc_hir::{Expr, ExprKind, MatchSource, Pat, QPath, Stmt, StmtKind};
67
use rustc_lint::{LateContext, LateLintPass, LintContext};
78
use rustc_middle::lint::in_external_macro;
89
use rustc_semver::RustcVersion;
910
use rustc_session::{declare_tool_lint, impl_lint_pass};
11+
use rustc_span::Span;
1012
use std::ops::ControlFlow;
1113

1214
declare_clippy_lint! {
@@ -56,29 +58,64 @@ impl_lint_pass!(ManualLetElse => [MANUAL_LET_ELSE]);
5658

5759
impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
5860
fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &'tcx Stmt<'tcx>) {
59-
if !meets_msrv(self.msrv, msrvs::LET_ELSE) {
60-
return;
61-
}
62-
63-
if in_external_macro(cx.sess(), stmt.span) {
64-
return;
65-
}
66-
67-
if_chain! {
61+
let if_let_or_match = if_chain! {
62+
if meets_msrv(self.msrv, msrvs::LET_ELSE);
63+
if !in_external_macro(cx.sess(), stmt.span);
6864
if let StmtKind::Local(local) = stmt.kind;
6965
if let Some(init) = local.init;
70-
if let Some(higher::IfLet { let_pat, let_expr: _, if_then, if_else }) = higher::IfLet::hir(cx, init);
71-
if if_then_simple_identity(let_pat, if_then);
72-
if let Some(if_else) = if_else;
73-
if expr_diverges(cx, if_else);
66+
if !from_different_macros(init.span, stmt.span);
67+
if let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init);
7468
then {
75-
span_lint(
76-
cx,
77-
MANUAL_LET_ELSE,
78-
stmt.span,
79-
"this could be rewritten as `let else`",
80-
);
69+
if_let_or_match
70+
} else {
71+
return;
8172
}
73+
};
74+
75+
match if_let_or_match {
76+
IfLetOrMatch::IfLet(_let_expr, let_pat, if_then, if_else) => if_chain! {
77+
if expr_is_simple_identity(let_pat, if_then);
78+
if let Some(if_else) = if_else;
79+
if expr_diverges(cx, if_else);
80+
then {
81+
span_lint(
82+
cx,
83+
MANUAL_LET_ELSE,
84+
stmt.span,
85+
"this could be rewritten as `let else`",
86+
);
87+
}
88+
},
89+
IfLetOrMatch::Match(_match_expr, arms, source) => {
90+
if source != MatchSource::Normal {
91+
return;
92+
}
93+
// Any other number than two arms doesn't (neccessarily)
94+
// have a trivial mapping to let else.
95+
if arms.len() != 2 {
96+
return;
97+
}
98+
// We iterate over both arms, trying to find one that is an identity,
99+
// one that diverges. Our check needs to work regardless of the order
100+
// of both arms.
101+
let mut found_identity_arm = false;
102+
let mut found_diverging_arm = false;
103+
for arm in arms {
104+
// Guards don't give us an easy mapping to let else
105+
if arm.guard.is_some() {
106+
return;
107+
}
108+
if expr_is_simple_identity(arm.pat, arm.body) {
109+
found_identity_arm = true;
110+
} else if expr_diverges(cx, arm.body) && pat_has_no_bindings(arm.pat) {
111+
found_diverging_arm = true;
112+
}
113+
}
114+
if !(found_identity_arm && found_diverging_arm) {
115+
return;
116+
}
117+
span_lint(cx, MANUAL_LET_ELSE, stmt.span, "this could be rewritten as `let else`");
118+
},
82119
}
83120
}
84121

@@ -139,16 +176,39 @@ fn expr_diverges(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool {
139176
.is_some()
140177
}
141178

142-
/// Checks if the passed `if_then` is a simple identity
143-
fn if_then_simple_identity(let_pat: &'_ Pat<'_>, if_then: &'_ Expr<'_>) -> bool {
179+
/// Returns true if the two spans come from different macro sites,
180+
/// or one comes from an invocation and the other is not from a macro at all.
181+
fn from_different_macros(span_a: Span, span_b: Span) -> bool {
182+
// This pre-check is a speed up so that we don't build outer_expn_data unless needed.
183+
match (span_a.from_expansion(), span_b.from_expansion()) {
184+
(false, false) => return false,
185+
(true, false) | (false, true) => return true,
186+
// We need to determine if both are from the same macro
187+
(true, true) => (),
188+
}
189+
let data_for_comparison = |sp: Span| {
190+
let expn_data = sp.ctxt().outer_expn_data();
191+
(expn_data.kind, expn_data.call_site)
192+
};
193+
data_for_comparison(span_a) != data_for_comparison(span_b)
194+
}
195+
196+
fn pat_has_no_bindings(pat: &'_ Pat<'_>) -> bool {
197+
let mut has_no_bindings = true;
198+
pat.each_binding_or_first(&mut |_, _, _, _| has_no_bindings = false);
199+
has_no_bindings
200+
}
201+
202+
/// Checks if the passed block is a simple identity referring to bindings created by the pattern
203+
fn expr_is_simple_identity(pat: &'_ Pat<'_>, expr: &'_ Expr<'_>) -> bool {
144204
// TODO support patterns with multiple bindings and tuples, like:
145-
// let (foo, bar) = if let (Some(foo), bar) = g() { (foo, bar) } else { ... }
205+
// let ... = if let (Some(foo), bar) = g() { (foo, bar) } else { ... }
146206
if_chain! {
147-
if let ExprKind::Path(QPath::Resolved(_ty, path)) = &peel_blocks(if_then).kind;
207+
if let ExprKind::Path(QPath::Resolved(_ty, path)) = &peel_blocks(expr).kind;
148208
if let [path_seg] = path.segments;
149209
then {
150210
let mut pat_bindings = Vec::new();
151-
let_pat.each_binding(|_ann, _hir_id, _sp, ident| {
211+
pat.each_binding_or_first(&mut |_ann, _hir_id, _sp, ident| {
152212
pat_bindings.push(ident);
153213
});
154214
if let [binding] = &pat_bindings[..] {

‎tests/ui/manual_let_else.rs

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@
22
#![allow(
33
clippy::collapsible_else_if,
44
clippy::unused_unit,
5-
clippy::never_loop,
6-
clippy::let_unit_value
5+
clippy::let_unit_value,
6+
clippy::never_loop
77
)]
88
#![warn(clippy::manual_let_else)]
99

@@ -87,6 +87,14 @@ fn fire() {
8787
} else {
8888
return;
8989
};
90+
91+
// entirely inside macro lints
92+
macro_rules! create_binding_if_some {
93+
($n:ident, $e:expr) => {
94+
let $n = if let Some(v) = $e { v } else { return };
95+
};
96+
}
97+
create_binding_if_some!(w, g());
9098
}
9199

92100
fn not_fire() {
@@ -164,4 +172,20 @@ fn not_fire() {
164172
};
165173
Some(v)
166174
}
175+
176+
// Macro boundary inside let
177+
macro_rules! some_or_return {
178+
($e:expr) => {
179+
if let Some(v) = $e { v } else { return }
180+
};
181+
}
182+
let v = some_or_return!(g());
183+
184+
// Also macro boundary inside let, but inside a macro
185+
macro_rules! create_binding_if_some_nf {
186+
($n:ident, $e:expr) => {
187+
let $n = some_or_return!($e);
188+
};
189+
}
190+
create_binding_if_some_nf!(v, g());
167191
}

‎tests/ui/manual_let_else.stderr

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,5 +100,16 @@ LL | | return;
100100
LL | | };
101101
| |______^
102102

103-
error: aborting due to 11 previous errors
103+
error: this could be rewritten as `let else`
104+
--> $DIR/manual_let_else.rs:94:13
105+
|
106+
LL | let $n = if let Some(v) = $e { v } else { return };
107+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
108+
...
109+
LL | create_binding_if_some!(w, g());
110+
| ------------------------------- in this macro invocation
111+
|
112+
= note: this error originates in the macro `create_binding_if_some` (in Nightly builds, run with -Z macro-backtrace for more info)
113+
114+
error: aborting due to 12 previous errors
104115

‎tests/ui/manual_let_else_match.rs

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
#![allow(unused_braces, unused_variables, dead_code)]
2+
#![allow(clippy::collapsible_else_if, clippy::let_unit_value)]
3+
#![warn(clippy::manual_let_else)]
4+
// Ensure that we don't conflict with match -> if let lints
5+
#![warn(clippy::single_match_else, clippy::single_match)]
6+
7+
enum Variant {
8+
Foo,
9+
Bar(u32),
10+
Baz(u32),
11+
}
12+
13+
fn f() -> Result<u32, u32> {
14+
Ok(0)
15+
}
16+
17+
fn g() -> Option<()> {
18+
None
19+
}
20+
21+
fn h() -> Variant {
22+
Variant::Foo
23+
}
24+
25+
fn main() {}
26+
27+
fn fire() {
28+
let v = match g() {
29+
Some(v_some) => v_some,
30+
None => return,
31+
};
32+
33+
let v = match g() {
34+
Some(v_some) => v_some,
35+
_ => return,
36+
};
37+
38+
loop {
39+
// More complex pattern for the identity arm
40+
let v = match h() {
41+
Variant::Foo => continue,
42+
Variant::Bar(v) | Variant::Baz(v) => v,
43+
};
44+
}
45+
46+
// There is a _ in the diverging arm
47+
// TODO also support unused bindings aka _v
48+
let v = match f() {
49+
Ok(v) => v,
50+
Err(_) => return,
51+
};
52+
}
53+
54+
fn not_fire() {
55+
// Multiple diverging arms
56+
let v = match h() {
57+
Variant::Foo => panic!(),
58+
Variant::Bar(_v) => return,
59+
Variant::Baz(v) => v,
60+
};
61+
62+
// Multiple identity arms
63+
let v = match h() {
64+
Variant::Foo => panic!(),
65+
Variant::Bar(v) => v,
66+
Variant::Baz(v) => v,
67+
};
68+
69+
// No diverging arm at all, only identity arms.
70+
// This is no case for let else, but destructuring assignment.
71+
let v = match f() {
72+
Ok(v) => v,
73+
Err(e) => e,
74+
};
75+
76+
// The identity arm has a guard
77+
let v = match h() {
78+
Variant::Bar(v) if g().is_none() => v,
79+
_ => return,
80+
};
81+
82+
// The diverging arm has a guard
83+
let v = match f() {
84+
Err(v) if v > 0 => panic!(),
85+
Ok(v) | Err(v) => v,
86+
};
87+
88+
// The diverging arm creates a binding
89+
let v = match f() {
90+
Ok(v) => v,
91+
Err(e) => panic!("error: {e}"),
92+
};
93+
}

‎tests/ui/manual_let_else_match.stderr

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
error: this could be rewritten as `let else`
2+
--> $DIR/manual_let_else_match.rs:28:5
3+
|
4+
LL | / let v = match g() {
5+
LL | | Some(v_some) => v_some,
6+
LL | | None => return,
7+
LL | | };
8+
| |______^
9+
|
10+
= note: `-D clippy::manual-let-else` implied by `-D warnings`
11+
12+
error: this could be rewritten as `let else`
13+
--> $DIR/manual_let_else_match.rs:33:5
14+
|
15+
LL | / let v = match g() {
16+
LL | | Some(v_some) => v_some,
17+
LL | | _ => return,
18+
LL | | };
19+
| |______^
20+
21+
error: this could be rewritten as `let else`
22+
--> $DIR/manual_let_else_match.rs:40:9
23+
|
24+
LL | / let v = match h() {
25+
LL | | Variant::Foo => continue,
26+
LL | | Variant::Bar(v) | Variant::Baz(v) => v,
27+
LL | | };
28+
| |__________^
29+
30+
error: this could be rewritten as `let else`
31+
--> $DIR/manual_let_else_match.rs:48:5
32+
|
33+
LL | / let v = match f() {
34+
LL | | Ok(v) => v,
35+
LL | | Err(_) => return,
36+
LL | | };
37+
| |______^
38+
39+
error: aborting due to 4 previous errors
40+

0 commit comments

Comments
 (0)