Skip to content

Commit 7e18451

Browse files
committed
Auto merge of #10175 - koka831:fix/10171, r=giraffate
Use original variable name in the suggestion Fix #10171 --- changelog: Sugg: [`manual_let_else`]: Now suggest the correct variable name [#10175](#10175) <!-- changelog_checked -->
2 parents c490974 + 07c8c50 commit 7e18451

File tree

4 files changed

+100
-62
lines changed

4 files changed

+100
-62
lines changed

clippy_lints/src/manual_let_else.rs

Lines changed: 36 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ declare_clippy_lint! {
3838
/// Could be written:
3939
///
4040
/// ```rust
41-
/// # #![feature(let_else)]
4241
/// # fn main () {
4342
/// # let w = Some(0);
4443
/// let Some(v) = w else { return };
@@ -69,29 +68,23 @@ impl_lint_pass!(ManualLetElse => [MANUAL_LET_ELSE]);
6968

7069
impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
7170
fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &'tcx Stmt<'tcx>) {
72-
let if_let_or_match = if_chain! {
73-
if self.msrv.meets(msrvs::LET_ELSE);
74-
if !in_external_macro(cx.sess(), stmt.span);
75-
if let StmtKind::Local(local) = stmt.kind;
76-
if let Some(init) = local.init;
77-
if local.els.is_none();
78-
if local.ty.is_none();
79-
if init.span.ctxt() == stmt.span.ctxt();
80-
if let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init);
81-
then {
82-
if_let_or_match
83-
} else {
84-
return;
85-
}
86-
};
71+
if !self.msrv.meets(msrvs::LET_ELSE) || in_external_macro(cx.sess(), stmt.span) {
72+
return;
73+
}
8774

75+
if let StmtKind::Local(local) = stmt.kind &&
76+
let Some(init) = local.init &&
77+
local.els.is_none() &&
78+
local.ty.is_none() &&
79+
init.span.ctxt() == stmt.span.ctxt() &&
80+
let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init) {
8881
match if_let_or_match {
8982
IfLetOrMatch::IfLet(if_let_expr, let_pat, if_then, if_else) => if_chain! {
9083
if expr_is_simple_identity(let_pat, if_then);
9184
if let Some(if_else) = if_else;
9285
if expr_diverges(cx, if_else);
9386
then {
94-
emit_manual_let_else(cx, stmt.span, if_let_expr, let_pat, if_else);
87+
emit_manual_let_else(cx, stmt.span, if_let_expr, local.pat, let_pat, if_else);
9588
}
9689
},
9790
IfLetOrMatch::Match(match_expr, arms, source) => {
@@ -128,15 +121,23 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
128121
return;
129122
}
130123

131-
emit_manual_let_else(cx, stmt.span, match_expr, pat_arm.pat, diverging_arm.body);
124+
emit_manual_let_else(cx, stmt.span, match_expr, local.pat, pat_arm.pat, diverging_arm.body);
132125
},
133126
}
127+
};
134128
}
135129

136130
extract_msrv_attr!(LateContext);
137131
}
138132

139-
fn emit_manual_let_else(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, pat: &Pat<'_>, else_body: &Expr<'_>) {
133+
fn emit_manual_let_else(
134+
cx: &LateContext<'_>,
135+
span: Span,
136+
expr: &Expr<'_>,
137+
local: &Pat<'_>,
138+
pat: &Pat<'_>,
139+
else_body: &Expr<'_>,
140+
) {
140141
span_lint_and_then(
141142
cx,
142143
MANUAL_LET_ELSE,
@@ -145,12 +146,11 @@ fn emit_manual_let_else(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, pat:
145146
|diag| {
146147
// This is far from perfect, for example there needs to be:
147148
// * mut additions for the bindings
148-
// * renamings of the bindings
149+
// * renamings of the bindings for `PatKind::Or`
149150
// * unused binding collision detection with existing ones
150151
// * putting patterns with at the top level | inside ()
151152
// for this to be machine applicable.
152153
let mut app = Applicability::HasPlaceholders;
153-
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", &mut app);
154154
let (sn_expr, _) = snippet_with_context(cx, expr.span, span.ctxt(), "", &mut app);
155155
let (sn_else, _) = snippet_with_context(cx, else_body.span, span.ctxt(), "", &mut app);
156156

@@ -159,10 +159,21 @@ fn emit_manual_let_else(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, pat:
159159
} else {
160160
format!("{{ {sn_else} }}")
161161
};
162-
let sn_bl = if matches!(pat.kind, PatKind::Or(..)) {
163-
format!("({sn_pat})")
164-
} else {
165-
sn_pat.into_owned()
162+
let sn_bl = match pat.kind {
163+
PatKind::Or(..) => {
164+
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", &mut app);
165+
format!("({sn_pat})")
166+
},
167+
// Replace the variable name iff `TupleStruct` has one argument like `Variant(v)`.
168+
PatKind::TupleStruct(ref w, args, ..) if args.len() == 1 => {
169+
let sn_wrapper = cx.sess().source_map().span_to_snippet(w.span()).unwrap_or_default();
170+
let (sn_inner, _) = snippet_with_context(cx, local.span, span.ctxt(), "", &mut app);
171+
format!("{sn_wrapper}({sn_inner})")
172+
},
173+
_ => {
174+
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", &mut app);
175+
sn_pat.into_owned()
176+
},
166177
};
167178
let sugg = format!("let {sn_bl} = {sn_expr} else {else_bl};");
168179
diag.span_suggestion(span, "consider writing", sugg, app);

tests/ui/manual_let_else.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,12 @@
88
)]
99
#![warn(clippy::manual_let_else)]
1010

11+
enum Variant {
12+
A(usize, usize),
13+
B(usize),
14+
C,
15+
}
16+
1117
fn g() -> Option<()> {
1218
None
1319
}
@@ -135,6 +141,15 @@ fn fire() {
135141
};
136142
}
137143
create_binding_if_some!(w, g());
144+
145+
fn e() -> Variant {
146+
Variant::A(0, 0)
147+
}
148+
149+
// Should not be renamed
150+
let v = if let Variant::A(a, 0) = e() { a } else { return };
151+
// Should be renamed
152+
let v = if let Variant::B(b) = e() { b } else { return };
138153
}
139154

140155
fn not_fire() {

tests/ui/manual_let_else.stderr

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

99
error: this could be rewritten as `let...else`
10-
--> $DIR/manual_let_else.rs:19:5
10+
--> $DIR/manual_let_else.rs:25:5
1111
|
1212
LL | / let v = if let Some(v_some) = g() {
1313
LL | | v_some
@@ -18,13 +18,13 @@ LL | | };
1818
|
1919
help: consider writing
2020
|
21-
LL ~ let Some(v_some) = g() else {
21+
LL ~ let Some(v) = g() else {
2222
LL + return;
2323
LL + };
2424
|
2525

2626
error: this could be rewritten as `let...else`
27-
--> $DIR/manual_let_else.rs:25:5
27+
--> $DIR/manual_let_else.rs:31:5
2828
|
2929
LL | / let v = if let Some(v) = g() {
3030
LL | | // Blocks around the identity should have no impact
@@ -45,25 +45,25 @@ LL + };
4545
|
4646

4747
error: this could be rewritten as `let...else`
48-
--> $DIR/manual_let_else.rs:38:9
48+
--> $DIR/manual_let_else.rs:44:9
4949
|
5050
LL | let v = if let Some(v_some) = g() { v_some } else { continue };
51-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { continue };`
51+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { continue };`
5252

5353
error: this could be rewritten as `let...else`
54-
--> $DIR/manual_let_else.rs:39:9
54+
--> $DIR/manual_let_else.rs:45:9
5555
|
5656
LL | let v = if let Some(v_some) = g() { v_some } else { break };
57-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { break };`
57+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { break };`
5858

5959
error: this could be rewritten as `let...else`
60-
--> $DIR/manual_let_else.rs:43:5
60+
--> $DIR/manual_let_else.rs:49:5
6161
|
6262
LL | let v = if let Some(v_some) = g() { v_some } else { panic!() };
63-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { panic!() };`
63+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { panic!() };`
6464

6565
error: this could be rewritten as `let...else`
66-
--> $DIR/manual_let_else.rs:46:5
66+
--> $DIR/manual_let_else.rs:52:5
6767
|
6868
LL | / let v = if let Some(v_some) = g() {
6969
LL | | v_some
@@ -74,13 +74,13 @@ LL | | };
7474
|
7575
help: consider writing
7676
|
77-
LL ~ let Some(v_some) = g() else {
77+
LL ~ let Some(v) = g() else {
7878
LL + std::process::abort()
7979
LL + };
8080
|
8181

8282
error: this could be rewritten as `let...else`
83-
--> $DIR/manual_let_else.rs:53:5
83+
--> $DIR/manual_let_else.rs:59:5
8484
|
8585
LL | / let v = if let Some(v_some) = g() {
8686
LL | | v_some
@@ -91,13 +91,13 @@ LL | | };
9191
|
9292
help: consider writing
9393
|
94-
LL ~ let Some(v_some) = g() else {
94+
LL ~ let Some(v) = g() else {
9595
LL + if true { return } else { panic!() }
9696
LL + };
9797
|
9898

9999
error: this could be rewritten as `let...else`
100-
--> $DIR/manual_let_else.rs:60:5
100+
--> $DIR/manual_let_else.rs:66:5
101101
|
102102
LL | / let v = if let Some(v_some) = g() {
103103
LL | | v_some
@@ -109,14 +109,14 @@ LL | | };
109109
|
110110
help: consider writing
111111
|
112-
LL ~ let Some(v_some) = g() else {
112+
LL ~ let Some(v) = g() else {
113113
LL + if true {}
114114
LL + panic!();
115115
LL + };
116116
|
117117

118118
error: this could be rewritten as `let...else`
119-
--> $DIR/manual_let_else.rs:70:5
119+
--> $DIR/manual_let_else.rs:76:5
120120
|
121121
LL | / let v = if let Some(v_some) = g() {
122122
LL | | v_some
@@ -129,7 +129,7 @@ LL | | };
129129
|
130130
help: consider writing
131131
|
132-
LL ~ let Some(v_some) = g() else {
132+
LL ~ let Some(v) = g() else {
133133
LL + match () {
134134
LL + _ if panic!() => {},
135135
LL + _ => panic!(),
@@ -138,13 +138,13 @@ LL + };
138138
|
139139

140140
error: this could be rewritten as `let...else`
141-
--> $DIR/manual_let_else.rs:80:5
141+
--> $DIR/manual_let_else.rs:86:5
142142
|
143143
LL | let v = if let Some(v_some) = g() { v_some } else { if panic!() {} };
144-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { if panic!() {} };`
144+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { if panic!() {} };`
145145

146146
error: this could be rewritten as `let...else`
147-
--> $DIR/manual_let_else.rs:83:5
147+
--> $DIR/manual_let_else.rs:89:5
148148
|
149149
LL | / let v = if let Some(v_some) = g() {
150150
LL | | v_some
@@ -157,15 +157,15 @@ LL | | };
157157
|
158158
help: consider writing
159159
|
160-
LL ~ let Some(v_some) = g() else {
160+
LL ~ let Some(v) = g() else {
161161
LL + match panic!() {
162162
LL + _ => {},
163163
LL + }
164164
LL + };
165165
|
166166

167167
error: this could be rewritten as `let...else`
168-
--> $DIR/manual_let_else.rs:92:5
168+
--> $DIR/manual_let_else.rs:98:5
169169
|
170170
LL | / let v = if let Some(v_some) = g() {
171171
LL | | v_some
@@ -178,15 +178,15 @@ LL | | };
178178
|
179179
help: consider writing
180180
|
181-
LL ~ let Some(v_some) = g() else { if true {
181+
LL ~ let Some(v) = g() else { if true {
182182
LL + return;
183183
LL + } else {
184184
LL + panic!("diverge");
185185
LL + } };
186186
|
187187

188188
error: this could be rewritten as `let...else`
189-
--> $DIR/manual_let_else.rs:101:5
189+
--> $DIR/manual_let_else.rs:107:5
190190
|
191191
LL | / let v = if let Some(v_some) = g() {
192192
LL | | v_some
@@ -199,7 +199,7 @@ LL | | };
199199
|
200200
help: consider writing
201201
|
202-
LL ~ let Some(v_some) = g() else {
202+
LL ~ let Some(v) = g() else {
203203
LL + match (g(), g()) {
204204
LL + (Some(_), None) => return,
205205
LL + (None, Some(_)) => {
@@ -215,7 +215,7 @@ LL + };
215215
|
216216

217217
error: this could be rewritten as `let...else`
218-
--> $DIR/manual_let_else.rs:118:5
218+
--> $DIR/manual_let_else.rs:124:5
219219
|
220220
LL | / let (v, w) = if let Some(v_some) = g().map(|v| (v, 42)) {
221221
LL | | v_some
@@ -226,13 +226,13 @@ LL | | };
226226
|
227227
help: consider writing
228228
|
229-
LL ~ let Some(v_some) = g().map(|v| (v, 42)) else {
229+
LL ~ let Some((v, w)) = g().map(|v| (v, 42)) else {
230230
LL + return;
231231
LL + };
232232
|
233233

234234
error: this could be rewritten as `let...else`
235-
--> $DIR/manual_let_else.rs:125:5
235+
--> $DIR/manual_let_else.rs:131:5
236236
|
237237
LL | / let v = if let (Some(v_some), w_some) = (g(), 0) {
238238
LL | | (w_some, v_some)
@@ -249,24 +249,36 @@ LL + };
249249
|
250250

251251
error: this could be rewritten as `let...else`
252-
--> $DIR/manual_let_else.rs:134:13
252+
--> $DIR/manual_let_else.rs:140:13
253253
|
254254
LL | let $n = if let Some(v) = $e { v } else { return };
255-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return };`
255+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some($n) = g() else { return };`
256256
...
257257
LL | create_binding_if_some!(w, g());
258258
| ------------------------------- in this macro invocation
259259
|
260260
= note: this error originates in the macro `create_binding_if_some` (in Nightly builds, run with -Z macro-backtrace for more info)
261261

262262
error: this could be rewritten as `let...else`
263-
--> $DIR/manual_let_else.rs:247:5
263+
--> $DIR/manual_let_else.rs:150:5
264+
|
265+
LL | let v = if let Variant::A(a, 0) = e() { a } else { return };
266+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(a, 0) = e() else { return };`
267+
268+
error: this could be rewritten as `let...else`
269+
--> $DIR/manual_let_else.rs:152:5
270+
|
271+
LL | let v = if let Variant::B(b) = e() { b } else { return };
272+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::B(v) = e() else { return };`
273+
274+
error: this could be rewritten as `let...else`
275+
--> $DIR/manual_let_else.rs:262:5
264276
|
265277
LL | / let _ = match ff {
266278
LL | | Some(value) => value,
267279
LL | | _ => macro_call!(),
268280
LL | | };
269-
| |______^ help: consider writing: `let Some(value) = ff else { macro_call!() };`
281+
| |______^ help: consider writing: `let Some(_) = ff else { macro_call!() };`
270282

271-
error: aborting due to 18 previous errors
283+
error: aborting due to 20 previous errors
272284

0 commit comments

Comments
 (0)