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

bors · bors · commit dc17e7317b7b · 2023-05-27T10:22:40.000Z
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.
diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs
@@ -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> {
diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs
@@ -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() {
diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr
@@ -260,25 +260,48 @@ 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,
 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
 
diff --git a/tests/ui/manual_let_else_match.rs b/tests/ui/manual_let_else_match.rs
@@ -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,
     };
 
diff --git a/tests/ui/manual_let_else_match.stderr b/tests/ui/manual_let_else_match.stderr
@@ -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
