Skip to content

Commit 84d666c

Browse files
est31est31
committed
Also support linting for match
1 parent 05c9ecb commit 84d666c

File tree

3 files changed

+200
-26
lines changed

3 files changed

+200
-26
lines changed

‎clippy_lints/src/manual_let_else.rs

Lines changed: 67 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
use clippy_utils::diagnostics::span_lint;
2+
use clippy_utils::higher::IfLetOrMatch;
23
use clippy_utils::visitors::expr_visitor_no_bodies;
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;
56
use rustc_hir::intravisit::Visitor;
6-
use rustc_hir::{Expr, ExprKind, Pat, QPath, Stmt, StmtKind};
7+
use rustc_hir::{Expr, ExprKind, MatchSource, Pat, QPath, Stmt, StmtKind};
78
use rustc_lint::{LateContext, LateLintPass, LintContext};
89
use rustc_middle::lint::in_external_macro;
910
use rustc_semver::RustcVersion;
@@ -56,29 +57,63 @@ impl_lint_pass!(ManualLetElse => [MANUAL_LET_ELSE]);
5657

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

@@ -144,16 +179,22 @@ fn expr_diverges(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool {
144179
does_diverge
145180
}
146181

147-
/// Checks if the passed `if_then` is a simple identity
148-
fn if_then_simple_identity(let_pat: &'_ Pat<'_>, if_then: &'_ Expr<'_>) -> bool {
182+
fn pat_has_no_bindings(pat: &'_ Pat<'_>) -> bool {
183+
let mut has_no_bindings = true;
184+
pat.each_binding_or_first(&mut |_, _, _, _| has_no_bindings = false);
185+
has_no_bindings
186+
}
187+
188+
/// Checks if the passed block is a simple identity referring to bindings created by the pattern
189+
fn expr_is_simple_identity(pat: &'_ Pat<'_>, expr: &'_ Expr<'_>) -> bool {
149190
// TODO support patterns with multiple bindings and tuples, like:
150-
// let (foo, bar) = if let (Some(foo), bar) = g() { (foo, bar) } else { ... }
191+
// let ... = if let (Some(foo), bar) = g() { (foo, bar) } else { ... }
151192
if_chain! {
152-
if let ExprKind::Path(QPath::Resolved(_ty, path)) = &peel_blocks(if_then).kind;
193+
if let ExprKind::Path(QPath::Resolved(_ty, path)) = &peel_blocks(expr).kind;
153194
if let [path_seg] = path.segments;
154195
then {
155196
let mut pat_bindings = Vec::new();
156-
let_pat.each_binding(|_ann, _hir_id, _sp, ident| {
197+
pat.each_binding_or_first(&mut |_ann, _hir_id, _sp, ident| {
157198
pat_bindings.push(ident);
158199
});
159200
if let [binding] = &pat_bindings[..] {

‎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::unused_unit)]
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)