Skip to content

Commit e413e4c

Browse files
est31est31
committed
Make an attempt of creating suggestions
They aren't perfect but better than nothing
1 parent 36a4be7 commit e413e4c

File tree

4 files changed

+156
-63
lines changed

4 files changed

+156
-63
lines changed

‎clippy_lints/src/manual_let_else.rs

+51-27
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,19 @@
1-
use clippy_utils::diagnostics::span_lint;
1+
use clippy_utils::diagnostics::span_lint_and_then;
22
use clippy_utils::higher::IfLetOrMatch;
3+
use clippy_utils::source::snippet_opt;
34
use clippy_utils::ty::is_type_diagnostic_item;
45
use clippy_utils::visitors::{for_each_expr, Descend};
56
use clippy_utils::{meets_msrv, msrvs, peel_blocks};
67
use if_chain::if_chain;
78
use rustc_data_structures::fx::FxHashSet;
9+
use rustc_errors::Applicability;
810
use rustc_hir::{Expr, ExprKind, MatchSource, Pat, PatKind, QPath, Stmt, StmtKind};
911
use rustc_lint::{LateContext, LateLintPass, LintContext};
1012
use rustc_middle::lint::in_external_macro;
1113
use rustc_semver::RustcVersion;
1214
use rustc_session::{declare_tool_lint, impl_lint_pass};
1315
use rustc_span::symbol::sym;
16+
use rustc_span::Span;
1417
use serde::Deserialize;
1518
use std::ops::ControlFlow;
1619

@@ -80,20 +83,15 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
8083
};
8184

8285
match if_let_or_match {
83-
IfLetOrMatch::IfLet(_let_expr, let_pat, if_then, if_else) => if_chain! {
86+
IfLetOrMatch::IfLet(if_let_expr, let_pat, if_then, if_else) => if_chain! {
8487
if expr_is_simple_identity(let_pat, if_then);
8588
if let Some(if_else) = if_else;
8689
if expr_diverges(cx, if_else);
8790
then {
88-
span_lint(
89-
cx,
90-
MANUAL_LET_ELSE,
91-
stmt.span,
92-
"this could be rewritten as `let else`",
93-
);
91+
emit_manual_let_else(cx, stmt.span, if_let_expr, let_pat, if_else);
9492
}
9593
},
96-
IfLetOrMatch::Match(_match_expr, arms, source) => {
94+
IfLetOrMatch::Match(match_expr, arms, source) => {
9795
if self.matches_behaviour == MatchLintBehaviour::Never {
9896
return;
9997
}
@@ -105,34 +103,60 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
105103
if arms.len() != 2 {
106104
return;
107105
}
108-
let check_types = self.matches_behaviour == MatchLintBehaviour::WellKnownTypes;
109-
// We iterate over both arms, trying to find one that is an identity,
110-
// one that diverges. Our check needs to work regardless of the order
111-
// of both arms.
112-
let mut found_identity_arm = false;
113-
let mut found_diverging_arm = false;
114-
for arm in arms {
115-
// Guards don't give us an easy mapping to let else
116-
if arm.guard.is_some() {
117-
return;
118-
}
119-
if expr_is_simple_identity(arm.pat, arm.body) {
120-
found_identity_arm = true;
121-
} else if expr_diverges(cx, arm.body) && pat_allowed_for_else(cx, arm.pat, check_types) {
122-
found_diverging_arm = true;
123-
}
106+
// Guards don't give us an easy mapping either
107+
if arms.iter().any(|arm| arm.guard.is_some()) {
108+
return;
124109
}
125-
if !(found_identity_arm && found_diverging_arm) {
110+
let check_types = self.matches_behaviour == MatchLintBehaviour::WellKnownTypes;
111+
let diverging_arm_opt = arms
112+
.iter()
113+
.enumerate()
114+
.find(|(_, arm)| expr_diverges(cx, arm.body) && pat_allowed_for_else(cx, arm.pat, check_types));
115+
let Some((idx, diverging_arm)) = diverging_arm_opt else { return; };
116+
let pat_arm = &arms[1 - idx];
117+
if !expr_is_simple_identity(pat_arm.pat, pat_arm.body) {
126118
return;
127119
}
128-
span_lint(cx, MANUAL_LET_ELSE, stmt.span, "this could be rewritten as `let else`");
120+
121+
emit_manual_let_else(cx, stmt.span, match_expr, pat_arm.pat, diverging_arm.body);
129122
},
130123
}
131124
}
132125

133126
extract_msrv_attr!(LateContext);
134127
}
135128

129+
fn emit_manual_let_else(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, pat: &Pat<'_>, else_body: &Expr<'_>) {
130+
span_lint_and_then(
131+
cx,
132+
MANUAL_LET_ELSE,
133+
span,
134+
"this could be rewritten as `let...else`",
135+
|diag| {
136+
// This is far from perfect, for example there needs to be:
137+
// * mut additions for the bindings
138+
// * renamings of the bindings
139+
// * unused binding collision detection with existing ones
140+
// * putting patterns with at the top level | inside ()
141+
// for this to be machine applicable.
142+
let app = Applicability::HasPlaceholders;
143+
144+
if let Some(sn_pat) = snippet_opt(cx, pat.span) &&
145+
let Some(sn_expr) = snippet_opt(cx, expr.span) &&
146+
let Some(sn_else) = snippet_opt(cx, else_body.span)
147+
{
148+
let else_bl = if matches!(else_body.kind, ExprKind::Block(..)) {
149+
sn_else
150+
} else {
151+
format!("{{ {sn_else} }}")
152+
};
153+
let sugg = format!("let {sn_pat} = {sn_expr} else {else_bl};");
154+
diag.span_suggestion(span, "consider writing", sugg, app);
155+
}
156+
},
157+
);
158+
}
159+
136160
fn expr_diverges(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool {
137161
fn is_never(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool {
138162
if let Some(ty) = cx.typeck_results().expr_ty_opt(expr) {

‎tests/ui/manual_let_else.rs

+5-6
Original file line numberDiff line numberDiff line change
@@ -158,14 +158,13 @@ fn not_fire() {
158158
}
159159
};
160160

161-
162-
let v = if let Some(v_some) = g() {
161+
enum Uninhabited {}
162+
fn un() -> Uninhabited {
163+
panic!()
164+
}
165+
let v = if let Some(v_some) = None {
163166
v_some
164167
} else {
165-
enum Uninhabited {}
166-
fn un() -> Uninhabited {
167-
panic!()
168-
}
169168
// Don't lint if the type is uninhabited but not !
170169
un()
171170
};

‎tests/ui/manual_let_else.stderr

+88-18
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
1-
error: this could be rewritten as `let else`
1+
error: this could be rewritten as `let...else`
22
--> $DIR/manual_let_else.rs:17:5
33
|
44
LL | let v = if let Some(v_some) = g() { v_some } else { return };
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { return };`
66
|
77
= note: `-D clippy::manual-let-else` implied by `-D warnings`
88

9-
error: this could be rewritten as `let else`
9+
error: this could be rewritten as `let...else`
1010
--> $DIR/manual_let_else.rs:18:5
1111
|
1212
LL | / let v = if let Some(v_some) = g() {
@@ -15,8 +15,15 @@ LL | | } else {
1515
LL | | return;
1616
LL | | };
1717
| |______^
18+
|
19+
help: consider writing
20+
|
21+
LL ~ let Some(v_some) = g() else {
22+
LL + return;
23+
LL + };
24+
|
1825

19-
error: this could be rewritten as `let else`
26+
error: this could be rewritten as `let...else`
2027
--> $DIR/manual_let_else.rs:24:5
2128
|
2229
LL | / let v = if let Some(v) = g() {
@@ -27,26 +34,35 @@ LL | | { v }
2734
LL | | return;
2835
LL | | };
2936
| |______^
37+
|
38+
help: consider writing
39+
|
40+
LL ~ let Some(v) = g() else {
41+
LL + // Some computation should still make it fire
42+
LL + g();
43+
LL + return;
44+
LL + };
45+
|
3046

31-
error: this could be rewritten as `let else`
47+
error: this could be rewritten as `let...else`
3248
--> $DIR/manual_let_else.rs:37:9
3349
|
3450
LL | let v = if let Some(v_some) = g() { v_some } else { continue };
35-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
51+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { continue };`
3652

37-
error: this could be rewritten as `let else`
53+
error: this could be rewritten as `let...else`
3854
--> $DIR/manual_let_else.rs:38:9
3955
|
4056
LL | let v = if let Some(v_some) = g() { v_some } else { break };
41-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
57+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { break };`
4258

43-
error: this could be rewritten as `let else`
59+
error: this could be rewritten as `let...else`
4460
--> $DIR/manual_let_else.rs:42:5
4561
|
4662
LL | let v = if let Some(v_some) = g() { v_some } else { panic!() };
47-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
63+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { panic!() };`
4864

49-
error: this could be rewritten as `let else`
65+
error: this could be rewritten as `let...else`
5066
--> $DIR/manual_let_else.rs:45:5
5167
|
5268
LL | / let v = if let Some(v_some) = g() {
@@ -55,8 +71,15 @@ LL | | } else {
5571
LL | | std::process::abort()
5672
LL | | };
5773
| |______^
74+
|
75+
help: consider writing
76+
|
77+
LL ~ let Some(v_some) = g() else {
78+
LL + std::process::abort()
79+
LL + };
80+
|
5881

59-
error: this could be rewritten as `let else`
82+
error: this could be rewritten as `let...else`
6083
--> $DIR/manual_let_else.rs:52:5
6184
|
6285
LL | / let v = if let Some(v_some) = g() {
@@ -65,8 +88,15 @@ LL | | } else {
6588
LL | | if true { return } else { panic!() }
6689
LL | | };
6790
| |______^
91+
|
92+
help: consider writing
93+
|
94+
LL ~ let Some(v_some) = g() else {
95+
LL + if true { return } else { panic!() }
96+
LL + };
97+
|
6898

69-
error: this could be rewritten as `let else`
99+
error: this could be rewritten as `let...else`
70100
--> $DIR/manual_let_else.rs:59:5
71101
|
72102
LL | / let v = if let Some(v_some) = g() {
@@ -77,8 +107,17 @@ LL | | } else {
77107
LL | | panic!("diverge");
78108
LL | | };
79109
| |______^
110+
|
111+
help: consider writing
112+
|
113+
LL ~ let Some(v_some) = g() else { if true {
114+
LL + return;
115+
LL + } else {
116+
LL + panic!("diverge");
117+
LL + } };
118+
|
80119

81-
error: this could be rewritten as `let else`
120+
error: this could be rewritten as `let...else`
82121
--> $DIR/manual_let_else.rs:68:5
83122
|
84123
LL | / let v = if let Some(v_some) = g() {
@@ -89,8 +128,25 @@ LL | | match (g(), g()) {
89128
LL | | }
90129
LL | | };
91130
| |______^
131+
|
132+
help: consider writing
133+
|
134+
LL ~ let Some(v_some) = g() else {
135+
LL + match (g(), g()) {
136+
LL + (Some(_), None) => return,
137+
LL + (None, Some(_)) => {
138+
LL + if true {
139+
LL + return;
140+
LL + } else {
141+
LL + panic!();
142+
LL + }
143+
LL + },
144+
LL + _ => return,
145+
LL + }
146+
LL + };
147+
|
92148

93-
error: this could be rewritten as `let else`
149+
error: this could be rewritten as `let...else`
94150
--> $DIR/manual_let_else.rs:85:5
95151
|
96152
LL | / let (v, w) = if let Some(v_some) = g().map(|v| (v, 42)) {
@@ -99,8 +155,15 @@ LL | | } else {
99155
LL | | return;
100156
LL | | };
101157
| |______^
158+
|
159+
help: consider writing
160+
|
161+
LL ~ let Some(v_some) = g().map(|v| (v, 42)) else {
162+
LL + return;
163+
LL + };
164+
|
102165

103-
error: this could be rewritten as `let else`
166+
error: this could be rewritten as `let...else`
104167
--> $DIR/manual_let_else.rs:92:5
105168
|
106169
LL | / let v = if let (Some(v_some), w_some) = (g(), 0) {
@@ -109,12 +172,19 @@ LL | | } else {
109172
LL | | return;
110173
LL | | };
111174
| |______^
175+
|
176+
help: consider writing
177+
|
178+
LL ~ let (Some(v_some), w_some) = (g(), 0) else {
179+
LL + return;
180+
LL + };
181+
|
112182

113-
error: this could be rewritten as `let else`
183+
error: this could be rewritten as `let...else`
114184
--> $DIR/manual_let_else.rs:101:13
115185
|
116186
LL | let $n = if let Some(v) = $e { v } else { return };
117-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
187+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return };`
118188
...
119189
LL | create_binding_if_some!(w, g());
120190
| ------------------------------- in this macro invocation

0 commit comments

Comments
 (0)