Auto merge of #143764 - dianne:primary-binding-drop-order, r=<try>

lower pattern bindings in the order they're written and base drop order on primary bindings' order

To fix #142163, this PR does two things:
- Makes match arms base their drop order on the first sub-branch instead of the last sub-branch. Together with the second change, this makes bindings' drop order correspond to the relative order of when each binding first appears (i.e. the order of the "primary" bindings).
- Lowers pattern bindings in the order they're written (still treating the right-hand side of a ``@`` as coming before the binding on the left). In each sub-branch of a match arm, this is the order that would be obtained if the or-alternatives chosen in that sub-branch were inlined into the arm's pattern. This both affects drop order (making bindings in or-patterns not be dropped first) and fixes the issue in [this test](https://github.com/rust-lang/rust/blob/2a023bf80a6fbd6a06d5460a34eb247b986286ed/tests/ui/pattern/bindings-after-at/bind-by-copy-or-pat.rs) from #121716.

My approach to the second point is admittedly a bit trickier than may be necessary. To avoid passing around a counter when building `FlatPat`s, I've instead added just enough information to recover the original structure of the pattern's bindings from a `MatchTreeSubBranch`'s path through the `Candidate` tree. Some alternatives:
- We could use a counter, then sort bindings by their ordinals when making `MatchTreeSubBranch`es.
- I'd like to experiment with always merging sub-candidates and removing `test_remaining_match_pairs_after_or`; that would require lowering bindings and guards in a different way. That makes it a bigger change too, though, so I figure it might be simplest to start here.
- For a very big change, we could track which or-alternatives succeed at runtime to base drop order on the binding order in the particular alternatives matched.

This is a breaking change. It will need a crater run, language team sign-off, and likely updates to the Reference.

This will conflict with #143376 and probably also #143028, so they shouldn't be merged at the same time.

r? `@matthewjasper` or `@Nadrieril`

Author: rust-bors[bot]

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

@@ -5,11 +5,11 @@
 //! This also includes code for pattern bindings in `let` statements and
 //! function parameters.
 
-use std::assert_matches::assert_matches;
 use std::borrow::Borrow;
 use std::mem;
 use std::sync::Arc;
 
+use itertools::{Itertools, Position};
 use rustc_abi::VariantIdx;
 use rustc_data_structures::fx::FxIndexMap;
 use rustc_data_structures::stack::ensure_sufficient_stack;
@@ -561,16 +561,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             // return: it isn't bound by move until right before enter the arm.
             // To handle this we instead unschedule it's drop after each time
             // we lower the guard.
+            // As a result, we end up with the drop order of the last sub-branch we lower. To use
+            // the drop order for the first sub-branch, we lower sub-branches in reverse (#142163).
             let target_block = self.cfg.start_new_block();
-            let mut schedule_drops = ScheduleDrops::Yes;
-            let arm = arm_match_scope.unzip().0;
-            // We keep a stack of all of the bindings and type ascriptions
-            // from the parent candidates that we visit, that also need to
-            // be bound for each candidate.
-            for sub_branch in branch.sub_branches {
-                if let Some(arm) = arm {
-                    self.clear_top_scope(arm.scope);
-                }
+            for (pos, sub_branch) in branch.sub_branches.into_iter().rev().with_position() {
+                debug_assert!(pos != Position::Only);
+                let schedule_drops =
+                    if pos == Position::Last { ScheduleDrops::Yes } else { ScheduleDrops::No };
                 let binding_end = self.bind_and_guard_matched_candidate(
                     sub_branch,
                     fake_borrow_temps,
@@ -579,9 +576,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     schedule_drops,
                     emit_storage_live,
                 );
-                if arm.is_none() {
-                    schedule_drops = ScheduleDrops::No;
-                }
                 self.cfg.goto(binding_end, outer_source_info, target_block);
             }
 
@@ -996,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>>,
@@ -1011,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.
 ///
@@ -1226,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>,
@@ -1452,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)
@@ -1490,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,
@@ -2453,11 +2511,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
             // Bindings for guards require some extra handling to automatically
             // insert implicit references/dereferences.
-            self.bind_matched_candidate_for_guard(
-                block,
-                schedule_drops,
-                sub_branch.bindings.iter(),
-            );
+            // This always schedules storage drops, so we may need to unschedule them below.
+            self.bind_matched_candidate_for_guard(block, sub_branch.bindings.iter());
             let guard_frame = GuardFrame {
                 locals: sub_branch
                     .bindings
@@ -2489,6 +2544,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     )
                 });
 
+            // If this isn't the final sub-branch being lowered, we need to unschedule drops of
+            // bindings and temporaries created for and by the guard. As a result, the drop order
+            // for the arm will correspond to the binding order of the final sub-branch lowered.
+            if matches!(schedule_drops, ScheduleDrops::No) {
+                self.clear_top_scope(arm.scope);
+            }
+
             let source_info = self.source_info(guard_span);
             let guard_end = self.source_info(tcx.sess.source_map().end_point(guard_span));
             let guard_frame = self.guard_context.pop().unwrap();
@@ -2538,14 +2600,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 let cause = FakeReadCause::ForGuardBinding;
                 self.cfg.push_fake_read(post_guard_block, guard_end, cause, Place::from(local_id));
             }
-            assert_matches!(
-                schedule_drops,
-                ScheduleDrops::Yes,
-                "patterns with guards must schedule drops"
-            );
+            // Only schedule drops for the last sub-branch we lower.
             self.bind_matched_candidate_for_arm_body(
                 post_guard_block,
-                ScheduleDrops::Yes,
+                schedule_drops,
                 by_value_bindings,
                 emit_storage_live,
             );
@@ -2671,7 +2729,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     fn bind_matched_candidate_for_guard<'b>(
         &mut self,
         block: BasicBlock,
-        schedule_drops: ScheduleDrops,
         bindings: impl IntoIterator<Item = &'b Binding<'tcx>>,
     ) where
         'tcx: 'b,
@@ -2690,12 +2747,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             // a reference R: &T pointing to the location matched by
             // the pattern, and every occurrence of P within a guard
             // denotes *R.
+            // Drops must be scheduled to emit `StorageDead` on the guard's failure/break branches.
             let ref_for_guard = self.storage_live_binding(
                 block,
                 binding.var_id,
                 binding.span,
                 RefWithinGuard,
-                schedule_drops,
+                ScheduleDrops::Yes,
             );
             match binding.binding_mode.0 {
                 ByRef::No => {
@@ -2705,13 +2763,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     self.cfg.push_assign(block, source_info, ref_for_guard, rvalue);
                 }
                 ByRef::Yes(mutbl) => {
-                    // The arm binding will be by reference, so eagerly create it now.
+                    // The arm binding will be by reference, so eagerly create it now. Drops must
+                    // be scheduled to emit `StorageDead` on the guard's failure/break branches.
                     let value_for_arm = self.storage_live_binding(
                         block,
                         binding.var_id,
                         binding.span,
                         OutsideGuard,
-                        schedule_drops,
+                        ScheduleDrops::Yes,
                     );
 
                     let rvalue =

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/mir-opt/match_arm_scopes.complicated_match.panic-unwind.SimplifyCfg-initial.after-ElaborateDrops.after.diff

@@ -85,11 +85,11 @@
           _8 = &(_2.2: std::string::String);
 -         _3 = &fake shallow (_2.0: bool);
 -         _4 = &fake shallow (_2.1: bool);
-          StorageLive(_12);
-          StorageLive(_13);
-          _13 = copy _1;
--         switchInt(move _13) -> [0: bb16, otherwise: bb15];
-+         switchInt(move _13) -> [0: bb13, otherwise: bb12];
+          StorageLive(_9);
+          StorageLive(_10);
+          _10 = copy _1;
+-         switchInt(move _10) -> [0: bb12, otherwise: bb11];
++         switchInt(move _10) -> [0: bb9, otherwise: bb8];
       }
 
 -     bb9: {
@@ -100,11 +100,11 @@
           _8 = &(_2.2: std::string::String);
 -         _3 = &fake shallow (_2.0: bool);
 -         _4 = &fake shallow (_2.1: bool);
-          StorageLive(_9);
-          StorageLive(_10);
-          _10 = copy _1;
--         switchInt(move _10) -> [0: bb12, otherwise: bb11];
-+         switchInt(move _10) -> [0: bb9, otherwise: bb8];
+          StorageLive(_12);
+          StorageLive(_13);
+          _13 = copy _1;
+-         switchInt(move _13) -> [0: bb16, otherwise: bb15];
++         switchInt(move _13) -> [0: bb13, otherwise: bb12];
       }
 
 -     bb10: {
@@ -139,7 +139,7 @@
 -         FakeRead(ForGuardBinding, _6);
 -         FakeRead(ForGuardBinding, _8);
           StorageLive(_5);
-          _5 = copy (_2.1: bool);
+          _5 = copy (_2.0: bool);
           StorageLive(_7);
           _7 = move (_2.2: std::string::String);
 -         goto -> bb10;
@@ -152,8 +152,8 @@
           StorageDead(_9);
           StorageDead(_8);
           StorageDead(_6);
--         falseEdge -> [real: bb1, imaginary: bb1];
-+         goto -> bb1;
+-         falseEdge -> [real: bb3, imaginary: bb3];
++         goto -> bb2;
       }
 
 -     bb15: {
@@ -181,7 +181,7 @@
 -         FakeRead(ForGuardBinding, _6);
 -         FakeRead(ForGuardBinding, _8);
           StorageLive(_5);
-          _5 = copy (_2.0: bool);
+          _5 = copy (_2.1: bool);
           StorageLive(_7);
           _7 = move (_2.2: std::string::String);
 -         goto -> bb10;
@@ -194,8 +194,8 @@
           StorageDead(_12);
           StorageDead(_8);
           StorageDead(_6);
--         falseEdge -> [real: bb3, imaginary: bb3];
-+         goto -> bb2;
+-         falseEdge -> [real: bb1, imaginary: bb1];
++         goto -> bb1;
       }
 
 -     bb19: {