Skip to content

Commit 2117e62

Browse files
est31est31
committed
Also support linting for match
1 parent 1388864 commit 2117e62

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;
@@ -55,29 +56,63 @@ impl_lint_pass!(ManualLetElse => [MANUAL_LET_ELSE]);
5556

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

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

146-
/// Checks if the passed `if_then` is a simple identity
147-
fn if_then_simple_identity(let_pat: &'_ Pat<'_>, if_then: &'_ Expr<'_>) -> bool {
181+
fn pat_has_no_bindings(pat: &'_ Pat<'_>) -> bool {
182+
let mut has_no_bindings = true;
183+
pat.each_binding_or_first(&mut |_, _, _, _| has_no_bindings = false);
184+
has_no_bindings
185+
}
186+
187+
/// Checks if the passed block is a simple identity referring to bindings created by the pattern
188+
fn expr_is_simple_identity(pat: &'_ Pat<'_>, expr: &'_ Expr<'_>) -> bool {
148189
// TODO support patterns with multiple bindings and tuples, like:
149-
// let (foo, bar) = if let (Some(foo), bar) = g() { (foo, bar) } else { ... }
190+
// let ... = if let (Some(foo), bar) = g() { (foo, bar) } else { ... }
150191
if_chain! {
151-
if let ExprKind::Path(QPath::Resolved(_ty, path)) = &peel_blocks(if_then).kind;
192+
if let ExprKind::Path(QPath::Resolved(_ty, path)) = &peel_blocks(expr).kind;
152193
if let [path_seg] = path.segments;
153194
then {
154195
let mut pat_bindings = Vec::new();
155-
let_pat.each_binding(|_ann, _hir_id, _sp, ident| {
196+
pat.each_binding_or_first(&mut |_ann, _hir_id, _sp, ident| {
156197
pat_bindings.push(ident);
157198
});
158199
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)