Skip to content

Commit ef38662

Browse files
est31est31
committed
Some improvements to the manual_let_else lint suggestions
* Replace variables inside | patterns in the if let: let v = if let V::A(v) | V::B(v) = v { v } else ... * Support nested patterns: let v = if let Ok(Ok(Ok(v))) = v { v } else ... * Support tuple structs with more than one arg: let v = V::W(v, _) = v { v } else ... * Correctly handle .. in tuple struct patterns: let v = V::X(v, ..) = v { v } else ...
1 parent 83a4b09 commit ef38662

File tree

5 files changed

+97
-31
lines changed

5 files changed

+97
-31
lines changed

‎clippy_lints/src/manual_let_else.rs

+52-19
Original file line numberDiff line numberDiff line change
@@ -146,10 +146,9 @@ fn emit_manual_let_else(
146146
"this could be rewritten as `let...else`",
147147
|diag| {
148148
// This is far from perfect, for example there needs to be:
149-
// * mut additions for the bindings
150-
// * renamings of the bindings for `PatKind::Or`
149+
// * tracking for multi-binding cases: let (foo, bar) = if let (Some(foo), Ok(bar)) = ...
150+
// * renamings of the bindings for many `PatKind`s like structs, slices, etc.
151151
// * unused binding collision detection with existing ones
152-
// * putting patterns with at the top level | inside ()
153152
// for this to be machine applicable.
154153
let mut app = Applicability::HasPlaceholders;
155154
let (sn_expr, _) = snippet_with_context(cx, expr.span, span.ctxt(), "", &mut app);
@@ -160,28 +159,62 @@ fn emit_manual_let_else(
160159
} else {
161160
format!("{{ {sn_else} }}")
162161
};
163-
let sn_bl = match pat.kind {
164-
PatKind::Or(..) => {
165-
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", &mut app);
166-
format!("({sn_pat})")
167-
},
168-
// Replace the variable name iff `TupleStruct` has one argument like `Variant(v)`.
169-
PatKind::TupleStruct(ref w, args, ..) if args.len() == 1 => {
170-
let sn_wrapper = cx.sess().source_map().span_to_snippet(w.span()).unwrap_or_default();
171-
let (sn_inner, _) = snippet_with_context(cx, local.span, span.ctxt(), "", &mut app);
172-
format!("{sn_wrapper}({sn_inner})")
173-
},
174-
_ => {
175-
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", &mut app);
176-
sn_pat.into_owned()
177-
},
178-
};
162+
let sn_bl = replace_in_pattern(cx, span, local, pat, &mut app);
179163
let sugg = format!("let {sn_bl} = {sn_expr} else {else_bl};");
180164
diag.span_suggestion(span, "consider writing", sugg, app);
181165
},
182166
);
183167
}
184168

169+
// replaces the locals in the pattern
170+
fn replace_in_pattern(
171+
cx: &LateContext<'_>,
172+
span: Span,
173+
local: &Pat<'_>,
174+
pat: &Pat<'_>,
175+
app: &mut Applicability,
176+
) -> String {
177+
let mut bindings_count = 0;
178+
pat.each_binding_or_first(&mut |_, _, _, _| bindings_count += 1);
179+
// If the pattern creates multiple bindings, exit early,
180+
// as otherwise we might paste the pattern to the positions of multiple bindings.
181+
if bindings_count > 1 {
182+
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", app);
183+
return sn_pat.into_owned();
184+
}
185+
186+
match pat.kind {
187+
PatKind::Binding(..) => {
188+
let (sn_bdg, _) = snippet_with_context(cx, local.span, span.ctxt(), "", app);
189+
return sn_bdg.to_string();
190+
},
191+
PatKind::Or(pats) => {
192+
let patterns = pats
193+
.iter()
194+
.map(|pat| replace_in_pattern(cx, span, local, pat, app))
195+
.collect::<Vec<_>>();
196+
let or_pat = patterns.join(" | ");
197+
return format!("({or_pat})");
198+
},
199+
// Replace the variable name iff `TupleStruct` has one argument like `Variant(v)`.
200+
PatKind::TupleStruct(ref w, args, dot_dot_pos) => {
201+
let mut args = args
202+
.iter()
203+
.map(|pat| replace_in_pattern(cx, span, local, pat, app))
204+
.collect::<Vec<_>>();
205+
if let Some(pos) = dot_dot_pos.as_opt_usize() {
206+
args.insert(pos, "..".to_owned());
207+
}
208+
let args = args.join(", ");
209+
let sn_wrapper = cx.sess().source_map().span_to_snippet(w.span()).unwrap_or_default();
210+
return format!("{sn_wrapper}({args})");
211+
},
212+
_ => {},
213+
}
214+
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", app);
215+
sn_pat.into_owned()
216+
}
217+
185218
/// Check whether an expression is divergent. May give false negatives.
186219
fn expr_diverges(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
187220
struct V<'cx, 'tcx> {

‎tests/ui/manual_let_else.rs

+13-3
Original file line numberDiff line numberDiff line change
@@ -146,10 +146,20 @@ fn fire() {
146146
Variant::A(0, 0)
147147
}
148148

149-
// Should not be renamed
150149
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 };
150+
151+
// `mut v` is inserted into the pattern
152+
let mut v = if let Variant::B(b) = e() { b } else { return };
153+
154+
// Nesting works
155+
let nested = Ok(Some(e()));
156+
let v = if let Ok(Some(Variant::B(b))) | Err(Some(Variant::A(b, _))) = nested {
157+
b
158+
} else {
159+
return;
160+
};
161+
// dot dot works
162+
let v = if let Variant::A(.., a) = e() { a } else { return };
153163
}
154164

155165
fn not_fire() {

‎tests/ui/manual_let_else.stderr

+29-6
Original file line numberDiff line numberDiff line change
@@ -260,25 +260,48 @@ LL | create_binding_if_some!(w, g());
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:150:5
263+
--> $DIR/manual_let_else.rs:149:5
264264
|
265265
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 };`
266+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(v, 0) = e() else { return };`
267267

268268
error: this could be rewritten as `let...else`
269269
--> $DIR/manual_let_else.rs:152:5
270270
|
271-
LL | let v = if let Variant::B(b) = e() { b } else { return };
272-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::B(v) = e() else { return };`
271+
LL | let mut v = if let Variant::B(b) = e() { b } else { return };
272+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::B(mut v) = e() else { return };`
273273

274274
error: this could be rewritten as `let...else`
275-
--> $DIR/manual_let_else.rs:262:5
275+
--> $DIR/manual_let_else.rs:156:5
276+
|
277+
LL | / let v = if let Ok(Some(Variant::B(b))) | Err(Some(Variant::A(b, _))) = nested {
278+
LL | | b
279+
LL | | } else {
280+
LL | | return;
281+
LL | | };
282+
| |______^
283+
|
284+
help: consider writing
285+
|
286+
LL ~ let (Ok(Some(Variant::B(v))) | Err(Some(Variant::A(v, _)))) = nested else {
287+
LL + return;
288+
LL + };
289+
|
290+
291+
error: this could be rewritten as `let...else`
292+
--> $DIR/manual_let_else.rs:162:5
293+
|
294+
LL | let v = if let Variant::A(.., a) = e() { a } else { return };
295+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(.., v) = e() else { return };`
296+
297+
error: this could be rewritten as `let...else`
298+
--> $DIR/manual_let_else.rs:272:5
276299
|
277300
LL | / let _ = match ff {
278301
LL | | Some(value) => value,
279302
LL | | _ => macro_call!(),
280303
LL | | };
281304
| |______^ help: consider writing: `let Some(_) = ff else { macro_call!() };`
282305

283-
error: aborting due to 20 previous errors
306+
error: aborting due to 22 previous errors
284307

‎tests/ui/manual_let_else_match.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ fn fire() {
6868
let f = Variant::Bar(1);
6969

7070
let _value = match f {
71-
Variant::Bar(_) | Variant::Baz(_) => (),
71+
Variant::Bar(v) | Variant::Baz(v) => v,
7272
_ => return,
7373
};
7474

‎tests/ui/manual_let_else_match.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,10 @@ error: this could be rewritten as `let...else`
5858
--> $DIR/manual_let_else_match.rs:70:5
5959
|
6060
LL | / let _value = match f {
61-
LL | | Variant::Bar(_) | Variant::Baz(_) => (),
61+
LL | | Variant::Bar(v) | Variant::Baz(v) => v,
6262
LL | | _ => return,
6363
LL | | };
64-
| |______^ help: consider writing: `let (Variant::Bar(_) | Variant::Baz(_)) = f else { return };`
64+
| |______^ help: consider writing: `let (Variant::Bar(_value) | Variant::Baz(_value)) = f else { return };`
6565

6666
error: this could be rewritten as `let...else`
6767
--> $DIR/manual_let_else_match.rs:76:5

0 commit comments

Comments
 (0)