lower bindings in the order they're written

Author: dianne

compiler/rustc_mir_build/src/builder/matches/match_pair.rs

@@ -124,9 +124,19 @@ impl<'tcx> MatchPairTree<'tcx> {
         let test_case = match pattern.kind {
             PatKind::Missing | PatKind::Wild | PatKind::Error(_) => None,
 
-            PatKind::Or { ref pats } => Some(TestCase::Or {
-                pats: pats.iter().map(|pat| FlatPat::new(place_builder.clone(), pat, cx)).collect(),
-            }),
+            PatKind::Or { ref pats } => {
+                let pats: Box<[FlatPat<'tcx>]> =
+                    pats.iter().map(|pat| FlatPat::new(place_builder.clone(), pat, cx)).collect();
+                if !pats[0].extra_data.bindings.is_empty() {
+                    // Hold a place for any bindings established in (possibly-nested) or-patterns.
+                    // By only holding a place when bindings are present, we skip over any
+                    // or-patterns that will be simplified by `merge_trivial_subcandidates`. In
+                    // other words, we can assume this expands into subcandidates.
+                    // FIXME(@dianne): this needs updating/removing if we always merge or-patterns
+                    extra_data.bindings.push(super::SubpatternBindings::FromOrPattern);
+                }
+                Some(TestCase::Or { pats })
+            }
 
             PatKind::Range(ref range) => {
                 if range.is_full_range(cx.tcx) == Some(true) {
@@ -194,12 +204,12 @@ impl<'tcx> MatchPairTree<'tcx> {
 
                 // Then push this binding, after any bindings in the subpattern.
                 if let Some(source) = place {
-                    extra_data.bindings.push(super::Binding {
+                    extra_data.bindings.push(super::SubpatternBindings::One(super::Binding {
                         span: pattern.span,
                         source,
                         var_id: var,
                         binding_mode: mode,
-                    });
+                    }));
                 }
 
                 None

compiler/rustc_mir_build/src/builder/matches/mod.rs

@@ -990,7 +990,7 @@ struct PatternExtraData<'tcx> {
     span: Span,
 
     /// Bindings that must be established.
-    bindings: Vec<Binding<'tcx>>,
+    bindings: Vec<SubpatternBindings<'tcx>>,
 
     /// Types that must be asserted.
     ascriptions: Vec<Ascription<'tcx>>,
@@ -1005,6 +1005,15 @@ impl<'tcx> PatternExtraData<'tcx> {
     }
 }
 
+#[derive(Debug, Clone)]
+enum SubpatternBindings<'tcx> {
+    /// A single binding.
+    One(Binding<'tcx>),
+    /// Holds the place for an or-pattern's bindings. This ensures their drops are scheduled in the
+    /// order the primary bindings appear. See rust-lang/rust#142163 for more information.
+    FromOrPattern,
+}
+
 /// A pattern in a form suitable for lowering the match tree, with all irrefutable
 /// patterns simplified away.
 ///
@@ -1220,7 +1229,7 @@ fn traverse_candidate<'tcx, C, T, I>(
     }
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Copy, Debug)]
 struct Binding<'tcx> {
     span: Span,
     source: Place<'tcx>,
@@ -1446,12 +1455,7 @@ impl<'tcx> MatchTreeSubBranch<'tcx> {
             span: candidate.extra_data.span,
             success_block: candidate.pre_binding_block.unwrap(),
             otherwise_block: candidate.otherwise_block.unwrap(),
-            bindings: parent_data
-                .iter()
-                .flat_map(|d| &d.bindings)
-                .chain(&candidate.extra_data.bindings)
-                .cloned()
-                .collect(),
+            bindings: sub_branch_bindings(parent_data, &candidate.extra_data.bindings),
             ascriptions: parent_data
                 .iter()
                 .flat_map(|d| &d.ascriptions)
@@ -1484,6 +1488,66 @@ impl<'tcx> MatchTreeBranch<'tcx> {
     }
 }
 
+/// Collects the bindings for a [`MatchTreeSubBranch`], preserving the order they appear in the
+/// pattern, as though the or-alternatives chosen in this sub-branch were inlined.
+fn sub_branch_bindings<'tcx>(
+    parents: &[PatternExtraData<'tcx>],
+    leaf_bindings: &[SubpatternBindings<'tcx>],
+) -> Vec<Binding<'tcx>> {
+    // In the common case, all bindings will be in leaves. Allocate to fit the leaf's bindings.
+    let mut bindings = Vec::with_capacity(leaf_bindings.len());
+    let remainder = push_sub_branch_bindings(&mut bindings, Some(parents), leaf_bindings);
+    // Make sure we've included all bindings. We can end up with a non-`None` remainder if there's
+    // an unsimplifed or-pattern at the end that doesn't contain bindings.
+    if let Some(remainder) = remainder {
+        assert!(remainder.iter().all(|parent| parent.bindings.is_empty()));
+        assert!(leaf_bindings.is_empty());
+    }
+    bindings
+}
+
+/// Helper for [`sub_branch_bindings`]. Returns any bindings yet to be inlined.
+fn push_sub_branch_bindings<'c, 'tcx>(
+    flattened: &mut Vec<Binding<'tcx>>,
+    parents: Option<&'c [PatternExtraData<'tcx>]>,
+    leaf_bindings: &[SubpatternBindings<'tcx>],
+) -> Option<&'c [PatternExtraData<'tcx>]> {
+    match parents {
+        None => bug!("can't inline or-pattern bindings: already inlined all bindings"),
+        Some(mut parents) => {
+            let (bindings, mut remainder) = loop {
+                match parents {
+                    // Base case: only the leaf's bindings remain to be inlined.
+                    [] => break (leaf_bindings, None),
+                    // Otherwise, inline the first non-empty descendant.
+                    [parent, remainder @ ..] => {
+                        if parent.bindings.is_empty() {
+                            // Skip over unsimplified or-patterns without bindings.
+                            parents = remainder;
+                        } else {
+                            break (&parent.bindings[..], Some(remainder));
+                        }
+                    }
+                }
+            };
+            for subpat_bindings in bindings {
+                match subpat_bindings {
+                    SubpatternBindings::One(binding) => flattened.push(*binding),
+                    SubpatternBindings::FromOrPattern => {
+                        // Inline bindings from an or-pattern. By construction, this always
+                        // corresponds to a subcandidate and its closest descendants (i.e. those
+                        // from nested or-patterns, but not adjacent or-patterns). To handle
+                        // adjacent or-patterns, e.g. `(x | x, y | y)`, we update the `remainder` to
+                        // point to the first descendant candidate from outside this or-pattern.
+                        remainder = push_sub_branch_bindings(flattened, remainder, leaf_bindings);
+                    }
+                }
+            }
+            remainder
+        }
+    }
+}
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub(crate) enum HasMatchGuard {
     Yes,

compiler/rustc_mir_build/src/builder/matches/util.rs

@@ -138,7 +138,9 @@ impl<'a, 'b, 'tcx> FakeBorrowCollector<'a, 'b, 'tcx> {
 
     fn visit_candidate(&mut self, candidate: &Candidate<'tcx>) {
         for binding in &candidate.extra_data.bindings {
-            self.visit_binding(binding);
+            if let super::SubpatternBindings::One(binding) = binding {
+                self.visit_binding(binding);
+            }
         }
         for match_pair in &candidate.match_pairs {
             self.visit_match_pair(match_pair);
@@ -147,7 +149,9 @@ impl<'a, 'b, 'tcx> FakeBorrowCollector<'a, 'b, 'tcx> {
 
     fn visit_flat_pat(&mut self, flat_pat: &FlatPat<'tcx>) {
         for binding in &flat_pat.extra_data.bindings {
-            self.visit_binding(binding);
+            if let super::SubpatternBindings::One(binding) = binding {
+                self.visit_binding(binding);
+            }
         }
         for match_pair in &flat_pat.match_pairs {
             self.visit_match_pair(match_pair);

tests/ui/drop/or-pattern-drop-order.rs

@@ -1,6 +1,7 @@
 //@ run-pass
 //! Test drop order for different ways of declaring pattern bindings involving or-patterns.
-//! Currently, it's inconsistent between language constructs (#142163).
+//! In particular, are ordered based on the order in which the first occurrence of each binding
+//! appears (i.e. the "primary" bindings). Regression test for #142163.
 
 use std::cell::RefCell;
 use std::ops::Drop;
@@ -43,11 +44,10 @@ fn main() {
         y = LogDrop(o, 1);
     });
 
-    // When bindings are declared with `let pat = expr;`, bindings within or-patterns are seen last,
-    // thus they're dropped first.
+    // `let pat = expr;` should have the same drop order.
     assert_drop_order(1..=3, |o| {
-        // Drops are right-to-left, treating `y` as rightmost: `y`, `z`, `x`.
-        let (x, Ok(y) | Err(y), z) = (LogDrop(o, 3), Ok(LogDrop(o, 1)), LogDrop(o, 2));
+        // Drops are right-to-left: `z`, `y`, `x`.
+        let (x, Ok(y) | Err(y), z) = (LogDrop(o, 3), Ok(LogDrop(o, 2)), LogDrop(o, 1));
     });
     assert_drop_order(1..=2, |o| {
         // The first or-pattern alternative determines the bindings' drop order: `y`, `x`.
@@ -58,11 +58,10 @@ fn main() {
         let ((true, x, y) | (false, y, x)) = (false, LogDrop(o, 1), LogDrop(o, 2));
     });
 
-    // `match` treats or-patterns as last like `let pat = expr;`, but also determines drop order
-    // using the order of the bindings in the *last* or-pattern alternative.
+    // `match` should have the same drop order.
     assert_drop_order(1..=3, |o| {
-        // Drops are right-to-left, treating `y` as rightmost: `y`, `z`, `x`.
-        match (LogDrop(o, 3), Ok(LogDrop(o, 1)), LogDrop(o, 2)) { (x, Ok(y) | Err(y), z) => {} }
+        // Drops are right-to-left: `z`, `y` `x`.
+        match (LogDrop(o, 3), Ok(LogDrop(o, 2)), LogDrop(o, 1)) { (x, Ok(y) | Err(y), z) => {} }
     });
     assert_drop_order(1..=2, |o| {
         // The first or-pattern alternative determines the bindings' drop order: `y`, `x`.
@@ -74,27 +73,26 @@ fn main() {
     });
 
     // Function params are visited one-by-one, and the order of bindings within a param's pattern is
-    // the same as `let pat = expr`;
+    // the same as `let pat = expr;`
     assert_drop_order(1..=3, |o| {
         // Among separate params, the drop order is right-to-left: `z`, `y`, `x`.
         (|x, (Ok(y) | Err(y)), z| {})(LogDrop(o, 3), Ok(LogDrop(o, 2)), LogDrop(o, 1));
     });
     assert_drop_order(1..=3, |o| {
-        // Within a param's pattern, or-patterns are treated as rightmost: `y`, `z`, `x`.
-        (|(x, Ok(y) | Err(y), z)| {})((LogDrop(o, 3), Ok(LogDrop(o, 1)), LogDrop(o, 2)));
+        // Within a param's pattern, likewise: `z`, `y`, `x`.
+        (|(x, Ok(y) | Err(y), z)| {})((LogDrop(o, 3), Ok(LogDrop(o, 2)), LogDrop(o, 1)));
     });
     assert_drop_order(1..=2, |o| {
         // The first or-pattern alternative determines the bindings' drop order: `y`, `x`.
         (|((true, x, y) | (false, y, x))| {})((true, LogDrop(o, 2), LogDrop(o, 1)));
     });
 
     // `if let` and `let`-`else` see bindings in the same order as `let pat = expr;`.
-    // Vars in or-patterns are seen last (dropped first), and the first alternative's order is used.
     assert_drop_order(1..=3, |o| {
-        if let (x, Ok(y) | Err(y), z) = (LogDrop(o, 3), Ok(LogDrop(o, 1)), LogDrop(o, 2)) {}
+        if let (x, Ok(y) | Err(y), z) = (LogDrop(o, 3), Ok(LogDrop(o, 2)), LogDrop(o, 1)) {}
     });
     assert_drop_order(1..=3, |o| {
-        let (x, Ok(y) | Err(y), z) = (LogDrop(o, 3), Ok(LogDrop(o, 1)), LogDrop(o, 2)) else {
+        let (x, Ok(y) | Err(y), z) = (LogDrop(o, 3), Ok(LogDrop(o, 2)), LogDrop(o, 1)) else {
             unreachable!();
         };
     });
@@ -106,4 +104,21 @@ fn main() {
             unreachable!();
         };
     });
+
+    // Test nested and adjacent or-patterns, including or-patterns without bindings under a guard.
+    assert_drop_order(1..=6, |o| {
+        // The `LogDrop`s that aren't moved into bindings are dropped last.
+        match [
+            [LogDrop(o, 6), LogDrop(o, 4)],
+            [LogDrop(o, 3), LogDrop(o, 2)],
+            [LogDrop(o, 1), LogDrop(o, 5)],
+        ] {
+            [
+                [_ | _, w | w] | [w | w, _ | _],
+                [x | x, y | y] | [y | y, x | x],
+                [z | z, _ | _] | [_ | _, z | z],
+            ] if true => {}
+            _ => unreachable!(),
+        }
+    });
 }

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

@@ -1,16 +1,15 @@
-//@ known-bug: unknown
+//@ run-pass
 #![allow(unused)]
 
 struct A(u32);
 
 pub fn main() {
-    // The or-pattern bindings are lowered after `x`, which triggers the error.
+    // Bindings are lowered in the order they appear syntactically, so this works.
     let x @ (A(a) | A(a)) = A(10);
-    // ERROR: use of moved value
     assert!(x.0 == 10);
     assert!(a == 10);
 
-    // This works.
+    // This also works.
     let (x @ A(a) | x @ A(a)) = A(10);
     assert!(x.0 == 10);
     assert!(a == 10);