Auto merge of #10866 - est31:manual_let_else_pattern, r=Manishearth

manual_let_else: support struct patterns

This adds upon the improvements of #10797 and:

* Only prints `()` around `Or` patterns at the top level (fixing a regression of #10797)
* Supports multi-binding patterns: `let (u, v) = if let (Some(u_i), Ok(v_i)) = ex { (u_i, v_i) } else ...`
* Traverses through tuple patterns: `let v = if let (Some(v), None) = ex { v } else ...`
* Supports struct patterns: `let v = if let S { v, w, } = ex { (v, w) } else ...`

```
changelog: [`manual_let_else`]: improve pattern printing to support struct patterns
```

fixes #10708
fixes #10424

clippy_lints/src/manual_let_else.rs

@@ -6,17 +6,18 @@ use clippy_utils::source::snippet_with_context;
 use clippy_utils::ty::is_type_diagnostic_item;
 use clippy_utils::visitors::{Descend, Visitable};
 use if_chain::if_chain;
-use rustc_data_structures::fx::FxHashSet;
+use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::{walk_expr, Visitor};
 use rustc_hir::{Expr, ExprKind, HirId, ItemId, Local, MatchSource, Pat, PatKind, QPath, Stmt, StmtKind, Ty};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
-use rustc_span::symbol::sym;
+use rustc_span::symbol::{sym, Symbol};
 use rustc_span::Span;
 use serde::Deserialize;
 use std::ops::ControlFlow;
+use std::slice;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -81,11 +82,11 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
         {
             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(ident_map) = expr_simple_identity_map(local.pat, 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);
+                        emit_manual_let_else(cx, stmt.span, if_let_expr, &ident_map, let_pat, if_else);
                     }
                 },
                 IfLetOrMatch::Match(match_expr, arms, source) => {
@@ -118,11 +119,11 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
                         return;
                     }
                     let pat_arm = &arms[1 - idx];
-                    if !expr_is_simple_identity(pat_arm.pat, pat_arm.body) {
+                    let Some(ident_map) = expr_simple_identity_map(local.pat, pat_arm.pat, pat_arm.body) else {
-                        return;
+                        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, &ident_map, pat_arm.pat, diverging_arm.body);
                 },
             }
         };
@@ -135,7 +136,7 @@ fn emit_manual_let_else(
     cx: &LateContext<'_>,
     span: Span,
     expr: &Expr<'_>,
-    local: &Pat<'_>,
+    ident_map: &FxHashMap<Symbol, &Pat<'_>>,
     pat: &Pat<'_>,
     else_body: &Expr<'_>,
 ) {
@@ -146,8 +147,8 @@ fn emit_manual_let_else(
         "this could be rewritten as `let...else`",
         |diag| {
             // This is far from perfect, for example there needs to be:
-            // * tracking for multi-binding cases: let (foo, bar) = if let (Some(foo), Ok(bar)) = ...
+            // * renamings of the bindings for many `PatKind`s like slices, etc.
-            // * renamings of the bindings for many `PatKind`s like structs, slices, etc.
+            // * limitations in the existing replacement algorithms
             // * unused binding collision detection with existing ones
             // for this to be machine applicable.
             let mut app = Applicability::HasPlaceholders;
@@ -159,57 +160,126 @@ fn emit_manual_let_else(
             } else {
                 format!("{{ {sn_else} }}")
             };
-            let sn_bl = replace_in_pattern(cx, span, local, pat, &mut app);
+            let sn_bl = replace_in_pattern(cx, span, ident_map, pat, &mut app, true);
             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
+/// Replaces the locals in the pattern
+///
+/// For this example:
+///
+/// ```ignore
+/// let (a, FooBar { b, c }) = if let Bar { Some(a_i), b_i } = ex { (a_i, b_i) } else { return };
+/// ```
+///
+/// We have:
+///
+/// ```ignore
+/// pat: Bar { Some(a_i), b_i }
+/// ident_map: (a_i) -> (a), (b_i) -> (FooBar { b, c })
+/// ```
+///
+/// We return:
+///
+/// ```ignore
+/// Bar { Some(a), b_i: FooBar { b, c } }
+/// ```
 fn replace_in_pattern(
     cx: &LateContext<'_>,
     span: Span,
-    local: &Pat<'_>,
+    ident_map: &FxHashMap<Symbol, &Pat<'_>>,
     pat: &Pat<'_>,
     app: &mut Applicability,
+    top_level: bool,
 ) -> String {
-    let mut bindings_count = 0;
+    // We put a labeled block here so that we can implement the fallback in this function.
-    pat.each_binding_or_first(&mut |_, _, _, _| bindings_count += 1);
+    // As the function has multiple call sites, implementing the fallback via an Option<T>
-    // If the pattern creates multiple bindings, exit early,
+    // return type and unwrap_or_else would cause repetition. Similarly, the function also
-    // as otherwise we might paste the pattern to the positions of multiple bindings.
+    // invokes the fall back multiple times.
-    if bindings_count > 1 {
+    'a: {
-        let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", app);
+        // If the ident map is empty, there is no replacement to do.
-        return sn_pat.into_owned();
+        // The code following this if assumes a non-empty ident_map.
-    }
+        if ident_map.is_empty() {
+            break 'a;
+        }
 
-    match pat.kind {
+        match pat.kind {
-        PatKind::Binding(..) => {
+            PatKind::Binding(_ann, _id, binding_name, opt_subpt) => {
-            let (sn_bdg, _) = snippet_with_context(cx, local.span, span.ctxt(), "", app);
+                let Some(pat_to_put) = ident_map.get(&binding_name.name) else { break 'a };
-            return sn_bdg.to_string();
+                let (sn_ptp, _) = snippet_with_context(cx, pat_to_put.span, span.ctxt(), "", app);
-        },
+                if let Some(subpt) = opt_subpt {
-        PatKind::Or(pats) => {
+                    let subpt = replace_in_pattern(cx, span, ident_map, subpt, app, false);
-            let patterns = pats
+                    return format!("{sn_ptp} @ {subpt}");
-                .iter()
+                }
-                .map(|pat| replace_in_pattern(cx, span, local, pat, app))
+                return sn_ptp.to_string();
-                .collect::<Vec<_>>();
+            },
-            let or_pat = patterns.join(" | ");
+            PatKind::Or(pats) => {
-            return format!("({or_pat})");
+                let patterns = pats
-        },
+                    .iter()
-        // Replace the variable name iff `TupleStruct` has one argument like `Variant(v)`.
+                    .map(|pat| replace_in_pattern(cx, span, ident_map, pat, app, false))
-        PatKind::TupleStruct(ref w, args, dot_dot_pos) => {
+                    .collect::<Vec<_>>();
-            let mut args = args
+                let or_pat = patterns.join(" | ");
-                .iter()
+                if top_level {
-                .map(|pat| replace_in_pattern(cx, span, local, pat, app))
+                    return format!("({or_pat})");
-                .collect::<Vec<_>>();
+                }
-            if let Some(pos) = dot_dot_pos.as_opt_usize() {
+                return or_pat;
-                args.insert(pos, "..".to_owned());
+            },
-            }
+            PatKind::Struct(path, fields, has_dot_dot) => {
-            let args = args.join(", ");
+                let fields = fields
-            let sn_wrapper = cx.sess().source_map().span_to_snippet(w.span()).unwrap_or_default();
+                    .iter()
-            return format!("{sn_wrapper}({args})");
+                    .map(|fld| {
-        },
+                        if let PatKind::Binding(_, _, name, None) = fld.pat.kind &&
-        _ => {},
+                            let Some(pat_to_put) = ident_map.get(&name.name)
+                        {
+                            let (sn_fld_name, _) = snippet_with_context(cx, fld.ident.span, span.ctxt(), "", app);
+                            let (sn_ptp, _) = snippet_with_context(cx, pat_to_put.span, span.ctxt(), "", app);
+                            // TODO: this is a bit of a hack, but it does its job. Ideally, we'd check if pat_to_put is
+                            // a PatKind::Binding but that is also hard to get right.
+                            if sn_fld_name == sn_ptp {
+                                // Field init shorthand
+                                return format!("{sn_fld_name}");
+                            }
+                            return format!("{sn_fld_name}: {sn_ptp}");
+                        }
+                        let (sn_fld, _) = snippet_with_context(cx, fld.span, span.ctxt(), "", app);
+                        sn_fld.into_owned()
+                    })
+                    .collect::<Vec<_>>();
+                let fields_string = fields.join(", ");
+
+                let dot_dot_str = if has_dot_dot { " .." } else { "" };
+                let (sn_pth, _) = snippet_with_context(cx, path.span(), span.ctxt(), "", app);
+                return format!("{sn_pth} {{ {fields_string}{dot_dot_str} }}");
+            },
+            // 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, ident_map, pat, app, false))
+                    .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})");
+            },
+            PatKind::Tuple(args, dot_dot_pos) => {
+                let mut args = args
+                    .iter()
+                    .map(|pat| replace_in_pattern(cx, span, ident_map, pat, app, false))
+                    .collect::<Vec<_>>();
+                if let Some(pos) = dot_dot_pos.as_opt_usize() {
+                    args.insert(pos, "..".to_owned());
+                }
+                let args = args.join(", ");
+                return format!("({args})");
+            },
+            _ => {},
+        }
     }
     let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", app);
     sn_pat.into_owned()
@@ -353,37 +423,74 @@ fn pat_allowed_for_else(cx: &LateContext<'_>, pat: &'_ Pat<'_>, check_types: boo
     !has_disallowed
 }
 
-/// Checks if the passed block is a simple identity referring to bindings created by the pattern
+/// Checks if the passed block is a simple identity referring to bindings created by the pattern,
-fn expr_is_simple_identity(pat: &'_ Pat<'_>, expr: &'_ Expr<'_>) -> bool {
+/// and if yes, returns a mapping between the relevant sub-pattern and the identifier it corresponds
-    // We support patterns with multiple bindings and tuples, like:
+/// to.
-    //   let ... = if let (Some(foo), bar) = g() { (foo, bar) } else { ... }
+///
+/// We support patterns with multiple bindings and tuples, e.g.:
+///
+/// ```ignore
+/// let (foo_o, bar_o) = if let (Some(foo), bar) = g() { (foo, bar) } else { ... }
+/// ```
+///
+/// The expected params would be:
+///
+/// ```ignore
+/// local_pat: (foo_o, bar_o)
+/// let_pat: (Some(foo), bar)
+/// expr: (foo, bar)
+/// ```
+///
+/// We build internal `sub_pats` so that it looks like `[foo_o, bar_o]` and `paths` so that it looks
+/// like `[foo, bar]`. Then we turn that into `FxHashMap [(foo) -> (foo_o), (bar) -> (bar_o)]` which
+/// we return.
+fn expr_simple_identity_map<'a, 'hir>(
+    local_pat: &'a Pat<'hir>,
+    let_pat: &'_ Pat<'hir>,
+    expr: &'_ Expr<'hir>,
+) -> Option<FxHashMap<Symbol, &'a Pat<'hir>>> {
     let peeled = peel_blocks(expr);
-    let paths = match peeled.kind {
+    let (sub_pats, paths) = match (local_pat.kind, peeled.kind) {
-        ExprKind::Tup(exprs) | ExprKind::Array(exprs) => exprs,
+        (PatKind::Tuple(pats, _), ExprKind::Tup(exprs)) | (PatKind::Slice(pats, ..), ExprKind::Array(exprs)) => {
-        ExprKind::Path(_) => std::slice::from_ref(peeled),
+            (pats, exprs)
-        _ => return false,
+        },
+        (_, ExprKind::Path(_)) => (slice::from_ref(local_pat), slice::from_ref(peeled)),
+        _ => return None,
     };
+
+    // There is some length mismatch, which indicates usage of .. in the patterns above e.g.:
+    // let (a, ..) = if let [a, b, _c] = ex { (a, b) } else { ... };
+    // We bail in these cases as they should be rare.
+    if paths.len() != sub_pats.len() {
+        return None;
+    }
+
     let mut pat_bindings = FxHashSet::default();
-    pat.each_binding_or_first(&mut |_ann, _hir_id, _sp, ident| {
+    let_pat.each_binding_or_first(&mut |_ann, _hir_id, _sp, ident| {
         pat_bindings.insert(ident);
     });
     if pat_bindings.len() < paths.len() {
-        return false;
+        // This rebinds some bindings from the outer scope, or it repeats some copy-able bindings multiple
+        // times. We don't support these cases so we bail here. E.g.:
+        // let foo = 0;
+        // let (new_foo, bar, bar_copied) = if let Some(bar) = Some(0) { (foo, bar, bar) } else { .. };
+        return None;
     }
-    for path in paths {
+    let mut ident_map = FxHashMap::default();
-        if_chain! {
+    for (sub_pat, path) in sub_pats.iter().zip(paths.iter()) {
-            if let ExprKind::Path(QPath::Resolved(_ty, path)) = path.kind;
+        if let ExprKind::Path(QPath::Resolved(_ty, path)) = path.kind &&
-            if let [path_seg] = path.segments;
+            let [path_seg] = path.segments
-            then {
+        {
-                if !pat_bindings.remove(&path_seg.ident) {
+            let ident = path_seg.ident;
-                    return false;
+            if !pat_bindings.remove(&ident) {
-                }
+                return None;
-            } else {
-                return false;
             }
+            ident_map.insert(ident.name, sub_pat);
+        } else {
+            return None;
         }
     }
-    true
+    Some(ident_map)
 }
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Deserialize)]

tests/ui/manual_let_else.rs

@@ -127,8 +127,8 @@ fn fire() {
         return;
     };
 
-    // Tuples supported for the identity block and pattern
+    // Tuples supported with multiple bindings
-    let v = if let (Some(v_some), w_some) = (g(), 0) {
+    let (w, S { v }) = if let (Some(v_some), w_some) = (g().map(|_| S { v: 0 }), 0) {
         (w_some, v_some)
     } else {
         return;
@@ -160,6 +160,30 @@ fn fire() {
     };
     // dot dot works
     let v = if let Variant::A(.., a) = e() { a } else { return };
+
+    // () is preserved: a bit of an edge case but make sure it stays around
+    let w = if let (Some(v), ()) = (g(), ()) { v } else { return };
+
+    // Tuple structs work
+    let w = if let Some(S { v: x }) = Some(S { v: 0 }) {
+        x
+    } else {
+        return;
+    };
+
+    // Field init shorthand is suggested
+    let v = if let Some(S { v: x }) = Some(S { v: 0 }) {
+        x
+    } else {
+        return;
+    };
+
+    // Multi-field structs also work
+    let (x, S { v }, w) = if let Some(U { v, w, x }) = None::<U<S<()>>> {
+        (x, v, w)
+    } else {
+        return;
+    };
 }
 
 fn not_fire() {
@@ -284,4 +308,23 @@ fn not_fire() {
         };
         1
     };
+
+    // This would require creation of a suggestion of the form
+    // let v @ (Some(_), _) = (...) else { return };
+    // Which is too advanced for our code, so we just bail.
+    let v = if let (Some(v_some), w_some) = (g(), 0) {
+        (w_some, v_some)
+    } else {
+        return;
+    };
+}
+
+struct S<T> {
+    v: T,
+}
+
+struct U<T> {
+    v: T,
+    w: T,
+    x: T,
 }

tests/ui/manual_let_else.stderr

@@ -234,7 +234,7 @@ LL +     };
 error: this could be rewritten as `let...else`
   --> $DIR/manual_let_else.rs:131:5
    |
-LL | /     let v = if let (Some(v_some), w_some) = (g(), 0) {
+LL | /     let (w, S { v }) = if let (Some(v_some), w_some) = (g().map(|_| S { v: 0 }), 0) {
 LL | |         (w_some, v_some)
 LL | |     } else {
 LL | |         return;
@@ -243,7 +243,7 @@ LL | |     };
    |
 help: consider writing
    |
-LL ~     let (Some(v_some), w_some) = (g(), 0) else {
+LL ~     let (Some(S { v }), w) = (g().map(|_| S { v: 0 }), 0) else {
 LL +         return;
 LL +     };
    |
@@ -295,7 +295,64 @@ 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
+  --> $DIR/manual_let_else.rs:165:5
+   |
+LL |     let w = if let (Some(v), ()) = (g(), ()) { v } else { return };
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let (Some(w), ()) = (g(), ()) else { return };`
+
+error: this could be rewritten as `let...else`
+  --> $DIR/manual_let_else.rs:168:5
+   |
+LL | /     let w = if let Some(S { v: x }) = Some(S { v: 0 }) {
+LL | |         x
+LL | |     } else {
+LL | |         return;
+LL | |     };
+   | |______^
+   |
+help: consider writing
+   |
+LL ~     let Some(S { v: w }) = Some(S { v: 0 }) else {
+LL +         return;
+LL +     };
+   |
+
+error: this could be rewritten as `let...else`
+  --> $DIR/manual_let_else.rs:175:5
+   |
+LL | /     let v = if let Some(S { v: x }) = Some(S { v: 0 }) {
+LL | |         x
+LL | |     } else {
+LL | |         return;
+LL | |     };
+   | |______^
+   |
+help: consider writing
+   |
+LL ~     let Some(S { v }) = Some(S { v: 0 }) else {
+LL +         return;
+LL +     };
+   |
+
+error: this could be rewritten as `let...else`
+  --> $DIR/manual_let_else.rs:182:5
+   |
+LL | /     let (x, S { v }, w) = if let Some(U { v, w, x }) = None::<U<S<()>>> {
+LL | |         (x, v, w)
+LL | |     } else {
+LL | |         return;
+LL | |     };
+   | |______^
+   |
+help: consider writing
+   |
+LL ~     let Some(U { v: S { v }, w, x }) = None::<U<S<()>>> else {
+LL +         return;
+LL +     };
+   |
+
+error: this could be rewritten as `let...else`
+  --> $DIR/manual_let_else.rs:296:5
    |
 LL | /     let _ = match ff {
 LL | |         Some(value) => value,
@@ -303,5 +360,5 @@ LL | |         _ => macro_call!(),
 LL | |     };
    | |______^ help: consider writing: `let Some(_) = ff else { macro_call!() };`
 
-error: aborting due to 22 previous errors
+error: aborting due to 26 previous errors
 

tests/ui/manual_let_else_match.rs

@@ -72,6 +72,11 @@ fn fire() {
         _ => return,
     };
 
+    let _value = match Some(build_enum()) {
+        Some(Variant::Bar(v) | Variant::Baz(v)) => v,
+        _ => return,
+    };
+
     let data = [1_u8, 2, 3, 4, 0, 0, 0, 0];
     let data = match data.as_slice() {
         [data @ .., 0, 0, 0, 0] | [data @ .., 0, 0] | [data @ .., 0] => data,

tests/ui/manual_let_else_match.stderr

@@ -64,7 +64,16 @@ LL | |     };
    | |______^ 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
+  --> $DIR/manual_let_else_match.rs:75:5
+   |
+LL | /     let _value = match Some(build_enum()) {
+LL | |         Some(Variant::Bar(v) | Variant::Baz(v)) => v,
+LL | |         _ => return,
+LL | |     };
+   | |______^ help: consider writing: `let Some(Variant::Bar(_value) | Variant::Baz(_value)) = Some(build_enum()) else { return };`
+
+error: this could be rewritten as `let...else`
+  --> $DIR/manual_let_else_match.rs:81:5
    |
 LL | /     let data = match data.as_slice() {
 LL | |         [data @ .., 0, 0, 0, 0] | [data @ .., 0, 0] | [data @ .., 0] => data,
@@ -72,5 +81,5 @@ LL | |         _ => return,
 LL | |     };
    | |______^ help: consider writing: `let ([data @ .., 0, 0, 0, 0] | [data @ .., 0, 0] | [data @ .., 0]) = data.as_slice() else { return };`
 
-error: aborting due to 8 previous errors
+error: aborting due to 9 previous errors