Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2025-02-17 06:28:42 +00:00
Code Issues Projects Releases Packages Wiki Activity

Auto merge of #10797 - est31:manual_let_else_pattern, r=llogiq

Browse source 
Improve pattern printing for manual_let_else

* Address a formatting issue pointed out in https://github.com/rust-lang/rust-clippy/pull/10175/files#r1137091002
* 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 ...`; note that more than one *capture* is still not supported, so it bails for `let (v, w) = if let E::F(vi, wi) = x { (vi, wi)}`
* Correctly handle .. in tuple struct patterns: `let v = V::X(v, ..) = v { v } else ...`

- \[ ] Followed [lint naming conventions][lint_naming]
- \[x] Added passing UI tests (including committed `.stderr` file)
- \[x] `cargo test` passes locally
- \[ ] Executed `cargo dev update_lints`
- \[ ] Added lint documentation
- \[x] Run `cargo dev fmt`

[lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints

---

changelog: [`manual_let_else`]: improve variable name in suggestions

Closes #10431 as this PR is adding a test for the `mut` case.
This commit is contained in:
bors 2023-05-27 10:22:40 +00:00
commit dc17e7317b
5 changed files with 144 additions and 77 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

164
clippy_lints/src/manual_let_else.rs
View file

@ -77,53 +77,54 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
local.els.is_none() &&
local.ty.is_none() &&
init.span.ctxt() == stmt.span.ctxt() &&
let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init) {
match if_let_or_match {
IfLetOrMatch::IfLet(if_let_expr, let_pat, if_then, if_else) => if_chain! {
if expr_is_simple_identity(let_pat, if_then);
if let Some(if_else) = if_else;
if expr_diverges(cx, if_else);
then {
emit_manual_let_else(cx, stmt.span, if_let_expr, local.pat, let_pat, if_else);
}
},
IfLetOrMatch::Match(match_expr, arms, source) => {
if self.matches_behaviour == MatchLintBehaviour::Never {
return;
}
if source != MatchSource::Normal {
return;
}
// Any other number than two arms doesn't (necessarily)
// have a trivial mapping to let else.
if arms.len() != 2 {
return;
}
// Guards don't give us an easy mapping either
if arms.iter().any(|arm| arm.guard.is_some()) {
return;
}
let check_types = self.matches_behaviour == MatchLintBehaviour::WellKnownTypes;
let diverging_arm_opt = arms
.iter()
.enumerate()
.find(|(_, arm)| expr_diverges(cx, arm.body) && pat_allowed_for_else(cx, arm.pat, check_types));
let Some((idx, diverging_arm)) = diverging_arm_opt else { return; };
// If the non-diverging arm is the first one, its pattern can be reused in a let/else statement.
// However, if it arrives in second position, its pattern may cover some cases already covered
// by the diverging one.
// TODO: accept the non-diverging arm as a second position if patterns are disjointed.
if idx == 0 {
return;
}
let pat_arm = &arms[1 - idx];
if !expr_is_simple_identity(pat_arm.pat, pat_arm.body) {
return;
}
let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init)
{
match if_let_or_match {
IfLetOrMatch::IfLet(if_let_expr, let_pat, if_then, if_else) => if_chain! {
if expr_is_simple_identity(let_pat, if_then);
if let Some(if_else) = if_else;
if expr_diverges(cx, if_else);
then {
emit_manual_let_else(cx, stmt.span, if_let_expr, local.pat, let_pat, if_else);
}
},
IfLetOrMatch::Match(match_expr, arms, source) => {
if self.matches_behaviour == MatchLintBehaviour::Never {
return;
}
if source != MatchSource::Normal {
return;
}
// Any other number than two arms doesn't (necessarily)
// have a trivial mapping to let else.
if arms.len() != 2 {
return;
}
// Guards don't give us an easy mapping either
if arms.iter().any(|arm| arm.guard.is_some()) {
return;
}
let check_types = self.matches_behaviour == MatchLintBehaviour::WellKnownTypes;
let diverging_arm_opt = arms
.iter()
.enumerate()
.find(|(_, arm)| expr_diverges(cx, arm.body) && pat_allowed_for_else(cx, arm.pat, check_types));
let Some((idx, diverging_arm)) = diverging_arm_opt else { return; };
// If the non-diverging arm is the first one, its pattern can be reused in a let/else statement.
// However, if it arrives in second position, its pattern may cover some cases already covered
// by the diverging one.
// TODO: accept the non-diverging arm as a second position if patterns are disjointed.
if idx == 0 {
return;
}
let pat_arm = &arms[1 - idx];
if !expr_is_simple_identity(pat_arm.pat, pat_arm.body) {
return;
}
emit_manual_let_else(cx, stmt.span, match_expr, local.pat, pat_arm.pat, diverging_arm.body);
},
}
emit_manual_let_else(cx, stmt.span, match_expr, local.pat, pat_arm.pat, diverging_arm.body);
},
}
};
}
@ -145,10 +146,9 @@ fn emit_manual_let_else(
"this could be rewritten as `let...else`",
|diag| {
// This is far from perfect, for example there needs to be:
// * mut additions for the bindings
// * renamings of the bindings for `PatKind::Or`
// * tracking for multi-binding cases: let (foo, bar) = if let (Some(foo), Ok(bar)) = ...
// * renamings of the bindings for many `PatKind`s like structs, slices, etc.
// * unused binding collision detection with existing ones
// * putting patterns with at the top level | inside ()
// for this to be machine applicable.
let mut app = Applicability::HasPlaceholders;
let (sn_expr, _) = snippet_with_context(cx, expr.span, span.ctxt(), "", &mut app);
@ -159,28 +159,62 @@ fn emit_manual_let_else(
} else {
format!("{{ {sn_else} }}")
};
let sn_bl = match pat.kind {
PatKind::Or(..) => {
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", &mut app);
format!("({sn_pat})")
},
// Replace the variable name iff `TupleStruct` has one argument like `Variant(v)`.
PatKind::TupleStruct(ref w, args, ..) if args.len() == 1 => {
let sn_wrapper = cx.sess().source_map().span_to_snippet(w.span()).unwrap_or_default();
let (sn_inner, _) = snippet_with_context(cx, local.span, span.ctxt(), "", &mut app);
format!("{sn_wrapper}({sn_inner})")
},
_ => {
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", &mut app);
sn_pat.into_owned()
},
};
let sn_bl = replace_in_pattern(cx, span, local, pat, &mut app);
let sugg = format!("let {sn_bl} = {sn_expr} else {else_bl};");
diag.span_suggestion(span, "consider writing", sugg, app);
},
);
}
// replaces the locals in the pattern
fn replace_in_pattern(
cx: &LateContext<'_>,
span: Span,
local: &Pat<'_>,
pat: &Pat<'_>,
app: &mut Applicability,
) -> String {
let mut bindings_count = 0;
pat.each_binding_or_first(&mut |_, _, _, _| bindings_count += 1);
// If the pattern creates multiple bindings, exit early,
// as otherwise we might paste the pattern to the positions of multiple bindings.
if bindings_count > 1 {
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", app);
return sn_pat.into_owned();
}
match pat.kind {
PatKind::Binding(..) => {
let (sn_bdg, _) = snippet_with_context(cx, local.span, span.ctxt(), "", app);
return sn_bdg.to_string();
},
PatKind::Or(pats) => {
let patterns = pats
.iter()
.map(|pat| replace_in_pattern(cx, span, local, pat, app))
.collect::<Vec<_>>();
let or_pat = patterns.join(" | ");
return format!("({or_pat})");
},
// Replace the variable name iff `TupleStruct` has one argument like `Variant(v)`.
PatKind::TupleStruct(ref w, args, dot_dot_pos) => {
let mut args = args
.iter()
.map(|pat| replace_in_pattern(cx, span, local, pat, app))
.collect::<Vec<_>>();
if let Some(pos) = dot_dot_pos.as_opt_usize() {
args.insert(pos, "..".to_owned());
}
let args = args.join(", ");
let sn_wrapper = cx.sess().source_map().span_to_snippet(w.span()).unwrap_or_default();
return format!("{sn_wrapper}({args})");
},
_ => {},
}
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", app);
sn_pat.into_owned()
}
/// Check whether an expression is divergent. May give false negatives.
fn expr_diverges(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
struct V<'cx, 'tcx> {

16
tests/ui/manual_let_else.rs
View file

@ -146,10 +146,20 @@ fn fire() {
Variant::A(0, 0)
}
// Should not be renamed
let v = if let Variant::A(a, 0) = e() { a } else { return };
// Should be renamed
let v = if let Variant::B(b) = e() { b } else { return };
// `mut v` is inserted into the pattern
let mut v = if let Variant::B(b) = e() { b } else { return };
// Nesting works
let nested = Ok(Some(e()));
let v = if let Ok(Some(Variant::B(b))) | Err(Some(Variant::A(b, _))) = nested {
b
} else {
return;
};
// dot dot works
let v = if let Variant::A(.., a) = e() { a } else { return };
}
fn not_fire() {

35
tests/ui/manual_let_else.stderr
View file

@ -260,19 +260,42 @@ LL | create_binding_if_some!(w, g());
= note: this error originates in the macro `create_binding_if_some` (in Nightly builds, run with -Z macro-backtrace for more info)
error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:150:5
--> $DIR/manual_let_else.rs:149:5
|
LL | let v = if let Variant::A(a, 0) = e() { a } else { return };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(a, 0) = e() else { return };`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(v, 0) = e() else { return };`
error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:152:5
|
LL | let v = if let Variant::B(b) = e() { b } else { return };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::B(v) = e() else { return };`
LL | let mut v = if let Variant::B(b) = e() { b } else { return };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::B(mut v) = e() else { return };`
error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:262:5
--> $DIR/manual_let_else.rs:156:5
|
LL | / let v = if let Ok(Some(Variant::B(b))) | Err(Some(Variant::A(b, _))) = nested {
LL | | b
LL | | } else {
LL | | return;
LL | | };
| |______^
|
help: consider writing
|
LL ~ let (Ok(Some(Variant::B(v))) | Err(Some(Variant::A(v, _)))) = nested else {
LL + return;
LL + };
|
error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:162:5
|
LL | let v = if let Variant::A(.., a) = e() { a } else { return };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(.., v) = e() else { return };`
error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:272:5
|
LL | / let _ = match ff {
LL | | Some(value) => value,
@ -280,5 +303,5 @@ LL | | _ => macro_call!(),
LL | | };
| |______^ help: consider writing: `let Some(_) = ff else { macro_call!() };`
error: aborting due to 20 previous errors
error: aborting due to 22 previous errors

2
tests/ui/manual_let_else_match.rs
View file

@ -68,7 +68,7 @@ fn fire() {
let f = Variant::Bar(1);
let _value = match f {
Variant::Bar(_) | Variant::Baz(_) => (),
Variant::Bar(v) | Variant::Baz(v) => v,
_ => return,
};

4
tests/ui/manual_let_else_match.stderr
View file

@ -58,10 +58,10 @@ error: this could be rewritten as `let...else`
--> $DIR/manual_let_else_match.rs:70:5
|
LL | / let _value = match f {
LL | | Variant::Bar(_) | Variant::Baz(_) => (),
LL | | Variant::Bar(v) | Variant::Baz(v) => v,
LL | | _ => return,
LL | | };
| |______^ help: consider writing: `let (Variant::Bar(_) | Variant::Baz(_)) = f else { return };`
| |______^ help: consider writing: `let (Variant::Bar(_value) | Variant::Baz(_value)) = f else { return };`
error: this could be rewritten as `let...else`
--> $DIR/manual_let_else_match.rs:76:5