Rollup merge of #120214 - Nadrieril:fix-120210, r=pnkfelix

match lowering: consistently lower bindings deepest-first

Currently when lowering match expressions to MIR, we do a funny little dance with the order of bindings. I attempt to explain it in the third commit: we handle refutable (i.e. needing a test) patterns differently than irrefutable ones. This leads to inconsistencies, as reported in #120210. The reason we need a dance at all is for situations like:

```rust
fn foo1(x: NonCopyStruct) {
    let y @ NonCopyStruct { copy_field: z } = x;
    // the above should turn into
    let z = x.copy_field;
    let y = x;
}
```

Here the `y `````@`````` binding will move out of `x`, so we need to copy the field first.

I believe that the inconsistency came about when we fixed #69971, and didn't notice that the fix didn't extend to refutable patterns. My guess then is that ordering bindings by "deepest-first, otherwise source order" is a sound choice. This PR implements that (at least I hope, match lowering is hard to follow 🥲).

Fixes #120210

r? `````@oli-obk````` since you merged the original fix to #69971
cc `````@matthewjasper`````

Author: matthiaskrgr

compiler/rustc_mir_build/src/build/matches/simplify.rs

@@ -40,43 +40,39 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         &mut self,
         candidate: &mut Candidate<'pat, 'tcx>,
     ) -> bool {
-        // repeatedly simplify match pairs until fixed point is reached
         debug!("{candidate:#?}");
-
-        // existing_bindings and new_bindings exists to keep the semantics in order.
-        // Reversing the binding order for bindings after `@` changes the binding order in places
-        // it shouldn't be changed, for example `let (Some(a), Some(b)) = (x, y)`
+        // In order to please the borrow checker, in a pattern like `x @ pat` we must lower the
+        // bindings in `pat` before `x`. E.g. (#69971):
+        //
+        // struct NonCopyStruct {
+        //     copy_field: u32,
+        // }
+        //
+        // fn foo1(x: NonCopyStruct) {
+        //     let y @ NonCopyStruct { copy_field: z } = x;
+        //     // the above should turn into
+        //     let z = x.copy_field;
+        //     let y = x;
+        // }
         //
-        // To avoid this, the binding occurs in the following manner:
-        // * the bindings for one iteration of the following loop occurs in order (i.e. left to
-        // right)
-        // * the bindings from the previous iteration of the loop is prepended to the bindings from
-        // the current iteration (in the implementation this is done by mem::swap and extend)
-        // * after all iterations, these new bindings are then appended to the bindings that were
-        // preexisting (i.e. `candidate.binding` when the function was called).
+        // We can't just reverse the binding order, because we must preserve pattern-order
+        // otherwise, e.g. in `let (Some(a), Some(b)) = (x, y)`. Our rule then is: deepest-first,
+        // and bindings at the same depth stay in source order.
+        //
+        // To do this, every time around the loop we prepend the newly found bindings to the
+        // bindings we already had.
         //
         // example:
         // candidate.bindings = [1, 2, 3]
-        // binding in iter 1: [4, 5]
-        // binding in iter 2: [6, 7]
+        // bindings in iter 1: [4, 5]
+        // bindings in iter 2: [6, 7]
         //
-        // final binding: [1, 2, 3, 6, 7, 4, 5]
-        let mut existing_bindings = mem::take(&mut candidate.bindings);
-        let mut new_bindings = Vec::new();
+        // final bindings: [6, 7, 4, 5, 1, 2, 3]
+        let mut accumulated_bindings = mem::take(&mut candidate.bindings);
+        // Repeatedly simplify match pairs until fixed point is reached
         loop {
-            let match_pairs = mem::take(&mut candidate.match_pairs);
-
-            if let [MatchPair { pattern: Pat { kind: PatKind::Or { pats }, .. }, place }] =
-                &*match_pairs
-            {
-                existing_bindings.extend_from_slice(&new_bindings);
-                mem::swap(&mut candidate.bindings, &mut existing_bindings);
-                candidate.subcandidates = self.create_or_subcandidates(candidate, place, pats);
-                return true;
-            }
-
             let mut changed = false;
-            for match_pair in match_pairs {
+            for match_pair in mem::take(&mut candidate.match_pairs) {
                 match self.simplify_match_pair(match_pair, candidate) {
                     Ok(()) => {
                         changed = true;
@@ -86,36 +82,39 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     }
                 }
             }
-            // Avoid issue #69971: the binding order should be right to left if there are more
-            // bindings after `@` to please the borrow checker
-            // Ex
-            // struct NonCopyStruct {
-            //     copy_field: u32,
-            // }
-            //
-            // fn foo1(x: NonCopyStruct) {
-            //     let y @ NonCopyStruct { copy_field: z } = x;
-            //     // the above should turn into
-            //     let z = x.copy_field;
-            //     let y = x;
-            // }
-            candidate.bindings.extend_from_slice(&new_bindings);
-            mem::swap(&mut candidate.bindings, &mut new_bindings);
+
+            // This does: accumulated_bindings = candidate.bindings.take() ++ accumulated_bindings
+            candidate.bindings.extend_from_slice(&accumulated_bindings);
+            mem::swap(&mut candidate.bindings, &mut accumulated_bindings);
             candidate.bindings.clear();
 
             if !changed {
-                existing_bindings.extend_from_slice(&new_bindings);
-                mem::swap(&mut candidate.bindings, &mut existing_bindings);
-                // Move or-patterns to the end, because they can result in us
-                // creating additional candidates, so we want to test them as
-                // late as possible.
-                candidate
-                    .match_pairs
-                    .sort_by_key(|pair| matches!(pair.pattern.kind, PatKind::Or { .. }));
-                debug!(simplified = ?candidate, "simplify_candidate");
-                return false; // if we were not able to simplify any, done.
+                // If we were not able to simplify anymore, done.
+                break;
             }
         }
+
+        // Store computed bindings back in `candidate`.
+        mem::swap(&mut candidate.bindings, &mut accumulated_bindings);
+
+        let did_expand_or =
+            if let [MatchPair { pattern: Pat { kind: PatKind::Or { pats }, .. }, place }] =
+                &*candidate.match_pairs
+            {
+                candidate.subcandidates = self.create_or_subcandidates(candidate, place, pats);
+                candidate.match_pairs.clear();
+                true
+            } else {
+                false
+            };
+
+        // Move or-patterns to the end, because they can result in us
+        // creating additional candidates, so we want to test them as
+        // late as possible.
+        candidate.match_pairs.sort_by_key(|pair| matches!(pair.pattern.kind, PatKind::Or { .. }));
+        debug!(simplified = ?candidate, "simplify_candidate");
+
+        did_expand_or
     }
 
     /// Given `candidate` that has a single or-pattern for its match-pairs,

tests/mir-opt/early_otherwise_branch.opt1.EarlyOtherwiseBranch.diff

@@ -51,13 +51,13 @@
 -     }
 - 
 -     bb3: {
-          StorageLive(_8);
-          _8 = (((_3.0: std::option::Option<u32>) as Some).0: u32);
           StorageLive(_9);
           _9 = (((_3.1: std::option::Option<u32>) as Some).0: u32);
+          StorageLive(_8);
+          _8 = (((_3.0: std::option::Option<u32>) as Some).0: u32);
           _0 = const 0_u32;
-          StorageDead(_9);
           StorageDead(_8);
+          StorageDead(_9);
 -         goto -> bb4;
 +         goto -> bb3;
       }

tests/mir-opt/early_otherwise_branch.opt2.EarlyOtherwiseBranch.diff

@@ -58,13 +58,13 @@
 - 
 -     bb4: {
 +     bb2: {
-          StorageLive(_9);
-          _9 = (((_3.0: std::option::Option<u32>) as Some).0: u32);
           StorageLive(_10);
           _10 = (((_3.1: std::option::Option<u32>) as Some).0: u32);
+          StorageLive(_9);
+          _9 = (((_3.0: std::option::Option<u32>) as Some).0: u32);
           _0 = const 0_u32;
-          StorageDead(_10);
           StorageDead(_9);
+          StorageDead(_10);
 -         goto -> bb6;
 +         goto -> bb4;
       }

tests/mir-opt/early_otherwise_branch.opt3.EarlyOtherwiseBranch.diff

@@ -51,13 +51,13 @@
 -     }
 - 
 -     bb3: {
-          StorageLive(_8);
-          _8 = (((_3.0: std::option::Option<u32>) as Some).0: u32);
           StorageLive(_9);
           _9 = (((_3.1: std::option::Option<bool>) as Some).0: bool);
+          StorageLive(_8);
+          _8 = (((_3.0: std::option::Option<u32>) as Some).0: u32);
           _0 = const 0_u32;
-          StorageDead(_9);
           StorageDead(_8);
+          StorageDead(_9);
 -         goto -> bb4;
 +         goto -> bb3;
       }

tests/mir-opt/early_otherwise_branch_3_element_tuple.opt1.EarlyOtherwiseBranch.diff

@@ -69,16 +69,16 @@
 
 -     bb4: {
 +     bb3: {
-          StorageLive(_11);
-          _11 = (((_4.0: std::option::Option<u32>) as Some).0: u32);
-          StorageLive(_12);
-          _12 = (((_4.1: std::option::Option<u32>) as Some).0: u32);
           StorageLive(_13);
           _13 = (((_4.2: std::option::Option<u32>) as Some).0: u32);
+          StorageLive(_12);
+          _12 = (((_4.1: std::option::Option<u32>) as Some).0: u32);
+          StorageLive(_11);
+          _11 = (((_4.0: std::option::Option<u32>) as Some).0: u32);
           _0 = const 0_u32;
-          StorageDead(_13);
-          StorageDead(_12);
           StorageDead(_11);
+          StorageDead(_12);
+          StorageDead(_13);
 -         goto -> bb5;
 +         goto -> bb4;
       }

tests/mir-opt/early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.diff

@@ -116,12 +116,12 @@
       }
 
       bb6: {
-          StorageLive(_12);
-          _39 = deref_copy (_4.0: &ViewportPercentageLength);
-          _12 = (((*_39) as Vw).0: f32);
           StorageLive(_13);
-          _40 = deref_copy (_4.1: &ViewportPercentageLength);
-          _13 = (((*_40) as Vw).0: f32);
+          _39 = deref_copy (_4.1: &ViewportPercentageLength);
+          _13 = (((*_39) as Vw).0: f32);
+          StorageLive(_12);
+          _40 = deref_copy (_4.0: &ViewportPercentageLength);
+          _12 = (((*_40) as Vw).0: f32);
           StorageLive(_14);
           StorageLive(_15);
           _15 = _12;
@@ -132,18 +132,18 @@
           StorageDead(_15);
           _3 = ViewportPercentageLength::Vw(move _14);
           StorageDead(_14);
-          StorageDead(_13);
           StorageDead(_12);
+          StorageDead(_13);
           goto -> bb10;
       }
 
       bb7: {
-          StorageLive(_17);
-          _41 = deref_copy (_4.0: &ViewportPercentageLength);
-          _17 = (((*_41) as Vh).0: f32);
           StorageLive(_18);
-          _42 = deref_copy (_4.1: &ViewportPercentageLength);
-          _18 = (((*_42) as Vh).0: f32);
+          _41 = deref_copy (_4.1: &ViewportPercentageLength);
+          _18 = (((*_41) as Vh).0: f32);
+          StorageLive(_17);
+          _42 = deref_copy (_4.0: &ViewportPercentageLength);
+          _17 = (((*_42) as Vh).0: f32);
           StorageLive(_19);
           StorageLive(_20);
           _20 = _17;
@@ -154,18 +154,18 @@
           StorageDead(_20);
           _3 = ViewportPercentageLength::Vh(move _19);
           StorageDead(_19);
-          StorageDead(_18);
           StorageDead(_17);
+          StorageDead(_18);
           goto -> bb10;
       }
 
       bb8: {
-          StorageLive(_22);
-          _43 = deref_copy (_4.0: &ViewportPercentageLength);
-          _22 = (((*_43) as Vmin).0: f32);
           StorageLive(_23);
-          _44 = deref_copy (_4.1: &ViewportPercentageLength);
-          _23 = (((*_44) as Vmin).0: f32);
+          _43 = deref_copy (_4.1: &ViewportPercentageLength);
+          _23 = (((*_43) as Vmin).0: f32);
+          StorageLive(_22);
+          _44 = deref_copy (_4.0: &ViewportPercentageLength);
+          _22 = (((*_44) as Vmin).0: f32);
           StorageLive(_24);
           StorageLive(_25);
           _25 = _22;
@@ -176,18 +176,18 @@
           StorageDead(_25);
           _3 = ViewportPercentageLength::Vmin(move _24);
           StorageDead(_24);
-          StorageDead(_23);
           StorageDead(_22);
+          StorageDead(_23);
           goto -> bb10;
       }
 
       bb9: {
-          StorageLive(_27);
-          _45 = deref_copy (_4.0: &ViewportPercentageLength);
-          _27 = (((*_45) as Vmax).0: f32);
           StorageLive(_28);
-          _46 = deref_copy (_4.1: &ViewportPercentageLength);
-          _28 = (((*_46) as Vmax).0: f32);
+          _45 = deref_copy (_4.1: &ViewportPercentageLength);
+          _28 = (((*_45) as Vmax).0: f32);
+          StorageLive(_27);
+          _46 = deref_copy (_4.0: &ViewportPercentageLength);
+          _27 = (((*_46) as Vmax).0: f32);
           StorageLive(_29);
           StorageLive(_30);
           _30 = _27;
@@ -198,8 +198,8 @@
           StorageDead(_30);
           _3 = ViewportPercentageLength::Vmax(move _29);
           StorageDead(_29);
-          StorageDead(_28);
           StorageDead(_27);
+          StorageDead(_28);
           goto -> bb10;
       }
 

tests/mir-opt/early_otherwise_branch_noopt.noopt1.EarlyOtherwiseBranch.diff

@@ -59,13 +59,13 @@
       }
 
       bb5: {
-          StorageLive(_9);
-          _9 = (((_3.0: std::option::Option<u32>) as Some).0: u32);
           StorageLive(_10);
           _10 = (((_3.1: std::option::Option<u32>) as Some).0: u32);
+          StorageLive(_9);
+          _9 = (((_3.0: std::option::Option<u32>) as Some).0: u32);
           _0 = const 0_u32;
-          StorageDead(_10);
           StorageDead(_9);
+          StorageDead(_10);
           goto -> bb8;
       }
 

tests/ui/pattern/bindings-after-at/bind-by-copy.rs

@@ -1,13 +1,30 @@
 // run-pass
+#![allow(unused)]
 
 // Test copy
 
-struct A { a: i32, b: i32 }
-struct B { a: i32, b: C }
-struct D { a: i32, d: C }
-#[derive(Copy,Clone)]
-struct C { c: i32 }
+struct A {
+    a: i32,
+    b: i32,
+}
+struct B {
+    a: i32,
+    b: C,
+}
+struct D {
+    a: i32,
+    d: C,
+}
+#[derive(Copy, Clone)]
+struct C {
+    c: i32,
+}
+enum E {
+    E { a: i32, e: C },
+    NotE,
+}
 
+#[rustfmt::skip]
 pub fn main() {
     match (A {a: 10, b: 20}) {
         x@A {a, b: 20} => { assert!(x.a == 10); assert!(a == 10); }
@@ -23,6 +40,23 @@ pub fn main() {
     y.d.c = 30;
     assert_eq!(d.c, 20);
 
+    match (E::E { a: 10, e: C { c: 20 } }) {
+        x @ E::E{ a, e: C { c } } => {
+            assert!(matches!(x, E::E { a: 10, e: C { c: 20 } }));
+            assert!(a == 10);
+            assert!(c == 20);
+        }
+        _ => panic!(),
+    }
+    match (E::E { a: 10, e: C { c: 20 } }) {
+        mut x @ E::E{ a, e: C { mut c } } => {
+            x = E::NotE;
+            c += 30;
+            assert_eq!(c, 50);
+        }
+        _ => panic!(),
+    }
+
     let some_b = Some(B { a: 10, b: C { c: 20 } });
 
     // in irrefutable pattern

tests/ui/pattern/bindings-after-at/borrowck-move-and-move.rs

@@ -15,8 +15,8 @@ fn main() {
     let a @ (b, c) = (u(), u()); //~ ERROR use of partially moved value
 
     match Ok(U) {
-        a @ Ok(b) | a @ Err(b) => {} //~ ERROR use of moved value
-                                     //~^ ERROR use of moved value
+        a @ Ok(b) | a @ Err(b) => {} //~ ERROR use of partially moved value
+                                     //~^ ERROR use of partially moved value
     }
 
     fn fun(a @ b: U) {} //~ ERROR use of moved value