Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit ed09cd9

Browse files
committed
Auto merge of #143764 - dianne:primary-binding-drop-order, r=Nadrieril,traviscross
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`
2 parents 61cb1e9 + b7de539 commit ed09cd9

10 files changed

+197
-139
lines changed

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

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -124,9 +124,19 @@ impl<'tcx> MatchPairTree<'tcx> {
124124
let test_case = match pattern.kind {
125125
PatKind::Missing | PatKind::Wild | PatKind::Error(_) => None,
126126

127-
PatKind::Or { ref pats } => Some(TestCase::Or {
128-
pats: pats.iter().map(|pat| FlatPat::new(place_builder.clone(), pat, cx)).collect(),
129-
}),
127+
PatKind::Or { ref pats } => {
128+
let pats: Box<[FlatPat<'tcx>]> =
129+
pats.iter().map(|pat| FlatPat::new(place_builder.clone(), pat, cx)).collect();
130+
if !pats[0].extra_data.bindings.is_empty() {
131+
// Hold a place for any bindings established in (possibly-nested) or-patterns.
132+
// By only holding a place when bindings are present, we skip over any
133+
// or-patterns that will be simplified by `merge_trivial_subcandidates`. In
134+
// other words, we can assume this expands into subcandidates.
135+
// FIXME(@dianne): this needs updating/removing if we always merge or-patterns
136+
extra_data.bindings.push(super::SubpatternBindings::FromOrPattern);
137+
}
138+
Some(TestCase::Or { pats })
139+
}
130140

131141
PatKind::Range(ref range) => {
132142
if range.is_full_range(cx.tcx) == Some(true) {
@@ -194,12 +204,12 @@ impl<'tcx> MatchPairTree<'tcx> {
194204

195205
// Then push this binding, after any bindings in the subpattern.
196206
if let Some(source) = place {
197-
extra_data.bindings.push(super::Binding {
207+
extra_data.bindings.push(super::SubpatternBindings::One(super::Binding {
198208
span: pattern.span,
199209
source,
200210
var_id: var,
201211
binding_mode: mode,
202-
});
212+
}));
203213
}
204214

205215
None

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

Lines changed: 97 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,11 @@
55
//! This also includes code for pattern bindings in `let` statements and
66
//! function parameters.
77
8-
use std::assert_matches::assert_matches;
98
use std::borrow::Borrow;
109
use std::mem;
1110
use std::sync::Arc;
1211

12+
use itertools::{Itertools, Position};
1313
use rustc_abi::VariantIdx;
1414
use rustc_data_structures::fx::FxIndexMap;
1515
use rustc_data_structures::stack::ensure_sufficient_stack;
@@ -561,16 +561,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
561561
// return: it isn't bound by move until right before enter the arm.
562562
// To handle this we instead unschedule it's drop after each time
563563
// we lower the guard.
564+
// As a result, we end up with the drop order of the last sub-branch we lower. To use
565+
// the drop order for the first sub-branch, we lower sub-branches in reverse (#142163).
564566
let target_block = self.cfg.start_new_block();
565-
let mut schedule_drops = ScheduleDrops::Yes;
566-
let arm = arm_match_scope.unzip().0;
567-
// We keep a stack of all of the bindings and type ascriptions
568-
// from the parent candidates that we visit, that also need to
569-
// be bound for each candidate.
570-
for sub_branch in branch.sub_branches {
571-
if let Some(arm) = arm {
572-
self.clear_top_scope(arm.scope);
573-
}
567+
for (pos, sub_branch) in branch.sub_branches.into_iter().rev().with_position() {
568+
debug_assert!(pos != Position::Only);
569+
let schedule_drops =
570+
if pos == Position::Last { ScheduleDrops::Yes } else { ScheduleDrops::No };
574571
let binding_end = self.bind_and_guard_matched_candidate(
575572
sub_branch,
576573
fake_borrow_temps,
@@ -579,9 +576,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
579576
schedule_drops,
580577
emit_storage_live,
581578
);
582-
if arm.is_none() {
583-
schedule_drops = ScheduleDrops::No;
584-
}
585579
self.cfg.goto(binding_end, outer_source_info, target_block);
586580
}
587581

@@ -996,7 +990,7 @@ struct PatternExtraData<'tcx> {
996990
span: Span,
997991

998992
/// Bindings that must be established.
999-
bindings: Vec<Binding<'tcx>>,
993+
bindings: Vec<SubpatternBindings<'tcx>>,
1000994

1001995
/// Types that must be asserted.
1002996
ascriptions: Vec<Ascription<'tcx>>,
@@ -1011,6 +1005,15 @@ impl<'tcx> PatternExtraData<'tcx> {
10111005
}
10121006
}
10131007

1008+
#[derive(Debug, Clone)]
1009+
enum SubpatternBindings<'tcx> {
1010+
/// A single binding.
1011+
One(Binding<'tcx>),
1012+
/// Holds the place for an or-pattern's bindings. This ensures their drops are scheduled in the
1013+
/// order the primary bindings appear. See rust-lang/rust#142163 for more information.
1014+
FromOrPattern,
1015+
}
1016+
10141017
/// A pattern in a form suitable for lowering the match tree, with all irrefutable
10151018
/// patterns simplified away.
10161019
///
@@ -1226,7 +1229,7 @@ fn traverse_candidate<'tcx, C, T, I>(
12261229
}
12271230
}
12281231

1229-
#[derive(Clone, Debug)]
1232+
#[derive(Clone, Copy,Debug)]
12301233
struct Binding<'tcx> {
12311234
span: Span,
12321235
source: Place<'tcx>,
@@ -1452,12 +1455,7 @@ impl<'tcx> MatchTreeSubBranch<'tcx> {
14521455
span: candidate.extra_data.span,
14531456
success_block: candidate.pre_binding_block.unwrap(),
14541457
otherwise_block: candidate.otherwise_block.unwrap(),
1455-
bindings: parent_data
1456-
.iter()
1457-
.flat_map(|d| &d.bindings)
1458-
.chain(&candidate.extra_data.bindings)
1459-
.cloned()
1460-
.collect(),
1458+
bindings: sub_branch_bindings(parent_data, &candidate.extra_data.bindings),
14611459
ascriptions: parent_data
14621460
.iter()
14631461
.flat_map(|d| &d.ascriptions)
@@ -1490,6 +1488,68 @@ impl<'tcx> MatchTreeBranch<'tcx> {
14901488
}
14911489
}
14921490

1491+
/// Collects the bindings for a [`MatchTreeSubBranch`], preserving the order they appear in the
1492+
/// pattern, as though the or-alternatives chosen in this sub-branch were inlined.
1493+
fn sub_branch_bindings<'tcx>(
1494+
parents: &[PatternExtraData<'tcx>],
1495+
leaf_bindings: &[SubpatternBindings<'tcx>],
1496+
) -> Vec<Binding<'tcx>> {
1497+
// In the common case, all bindings will be in leaves. Allocate to fit the leaf's bindings.
1498+
let mut all_bindings = Vec::with_capacity(leaf_bindings.len());
1499+
let mut remainder = parents
1500+
.iter()
1501+
.map(|parent| parent.bindings.as_slice())
1502+
.chain([leaf_bindings])
1503+
// Skip over unsimplified or-patterns without bindings.
1504+
.filter(|bindings| !bindings.is_empty());
1505+
if let Some(candidate_bindings) = remainder.next() {
1506+
push_sub_branch_bindings(&mut all_bindings, candidate_bindings, &mut remainder);
1507+
}
1508+
// Make sure we've included all bindings. For ill-formed patterns like `(x, _ | y)`, we may not
1509+
// have collected all bindings yet, since we only check the first alternative when determining
1510+
// whether to inline subcandidates' bindings.
1511+
// FIXME(@dianne): prevent ill-formed patterns from getting here
1512+
while let Some(candidate_bindings) = remainder.next() {
1513+
ty::tls::with(|tcx| {
1514+
tcx.dcx().delayed_bug("mismatched or-pattern bindings but no error emitted")
1515+
});
1516+
// To recover, we collect the rest in an arbitrary order.
1517+
push_sub_branch_bindings(&mut all_bindings, candidate_bindings, &mut remainder);
1518+
}
1519+
all_bindings
1520+
}
1521+
1522+
/// Helper for [`sub_branch_bindings`]. Collects bindings from `candidate_bindings` into
1523+
/// `flattened`. Bindings in or-patterns are collected recursively from `remainder`.
1524+
fn push_sub_branch_bindings<'c, 'tcx: 'c>(
1525+
flattened: &mut Vec<Binding<'tcx>>,
1526+
candidate_bindings: &'c [SubpatternBindings<'tcx>],
1527+
remainder: &mut impl Iterator<Item = &'c [SubpatternBindings<'tcx>]>,
1528+
) {
1529+
for subpat_bindings in candidate_bindings {
1530+
match subpat_bindings {
1531+
SubpatternBindings::One(binding) => flattened.push(*binding),
1532+
SubpatternBindings::FromOrPattern => {
1533+
// Inline bindings from an or-pattern. By construction, this always
1534+
// corresponds to a subcandidate and its closest descendants (i.e. those
1535+
// from nested or-patterns, but not adjacent or-patterns). To handle
1536+
// adjacent or-patterns, e.g. `(x | x, y | y)`, we update the `remainder` to
1537+
// point to the first descendant candidate from outside this or-pattern.
1538+
if let Some(subcandidate_bindings) = remainder.next() {
1539+
push_sub_branch_bindings(flattened, subcandidate_bindings, remainder);
1540+
} else {
1541+
// For ill-formed patterns like `x | _`, we may not have any subcandidates left
1542+
// to inline bindings from.
1543+
// FIXME(@dianne): prevent ill-formed patterns from getting here
1544+
ty::tls::with(|tcx| {
1545+
tcx.dcx().delayed_bug("mismatched or-pattern bindings but no error emitted")
1546+
});
1547+
};
1548+
}
1549+
}
1550+
}
1551+
}
1552+
14931553
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
14941554
pub(crate) enum HasMatchGuard {
14951555
Yes,
@@ -2453,11 +2513,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
24532513

24542514
// Bindings for guards require some extra handling to automatically
24552515
// insert implicit references/dereferences.
2456-
self.bind_matched_candidate_for_guard(
2457-
block,
2458-
schedule_drops,
2459-
sub_branch.bindings.iter(),
2460-
);
2516+
// This always schedules storage drops, so we may need to unschedule them below.
2517+
self.bind_matched_candidate_for_guard(block, sub_branch.bindings.iter());
24612518
let guard_frame = GuardFrame {
24622519
locals: sub_branch
24632520
.bindings
@@ -2489,6 +2546,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
24892546
)
24902547
});
24912548

2549+
// If this isn't the final sub-branch being lowered, we need to unschedule drops of
2550+
// bindings and temporaries created for and by the guard. As a result, the drop order
2551+
// for the arm will correspond to the binding order of the final sub-branch lowered.
2552+
if matches!(schedule_drops, ScheduleDrops::No) {
2553+
self.clear_top_scope(arm.scope);
2554+
}
2555+
24922556
let source_info = self.source_info(guard_span);
24932557
let guard_end = self.source_info(tcx.sess.source_map().end_point(guard_span));
24942558
let guard_frame = self.guard_context.pop().unwrap();
@@ -2538,14 +2602,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
25382602
let cause = FakeReadCause::ForGuardBinding;
25392603
self.cfg.push_fake_read(post_guard_block, guard_end, cause, Place::from(local_id));
25402604
}
2541-
assert_matches!(
2542-
schedule_drops,
2543-
ScheduleDrops::Yes,
2544-
"patterns with guards must schedule drops"
2545-
);
2605+
// Only schedule drops for the last sub-branch we lower.
25462606
self.bind_matched_candidate_for_arm_body(
25472607
post_guard_block,
2548-
ScheduleDrops::Yes,
2608+
schedule_drops,
25492609
by_value_bindings,
25502610
emit_storage_live,
25512611
);
@@ -2671,7 +2731,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
26712731
fn bind_matched_candidate_for_guard<'b>(
26722732
&mut self,
26732733
block: BasicBlock,
2674-
schedule_drops: ScheduleDrops,
26752734
bindings: impl IntoIterator<Item = &'b Binding<'tcx>>,
26762735
) where
26772736
'tcx: 'b,
@@ -2690,12 +2749,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
26902749
// a reference R: &T pointing to the location matched by
26912750
// the pattern, and every occurrence of P within a guard
26922751
// denotes *R.
2752+
// Drops must be scheduled to emit `StorageDead` on the guard's failure/break branches.
26932753
let ref_for_guard = self.storage_live_binding(
26942754
block,
26952755
binding.var_id,
26962756
binding.span,
26972757
RefWithinGuard,
2698-
schedule_drops,
2758+
ScheduleDrops::Yes,
26992759
);
27002760
match binding.binding_mode.0 {
27012761
ByRef::No => {
@@ -2705,13 +2765,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
27052765
self.cfg.push_assign(block, source_info, ref_for_guard, rvalue);
27062766
}
27072767
ByRef::Yes(mutbl) => {
2708-
// The arm binding will be by reference, so eagerly create it now.
2768+
// The arm binding will be by reference, so eagerly create it now. Drops must
2769+
// be scheduled to emit `StorageDead` on the guard's failure/break branches.
27092770
let value_for_arm = self.storage_live_binding(
27102771
block,
27112772
binding.var_id,
27122773
binding.span,
27132774
OutsideGuard,
2714-
schedule_drops,
2775+
ScheduleDrops::Yes,
27152776
);
27162777

27172778
let rvalue =

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,9 @@ impl<'a, 'b, 'tcx> FakeBorrowCollector<'a, 'b, 'tcx> {
138138

139139
fn visit_candidate(&mut self, candidate: &Candidate<'tcx>) {
140140
for binding in &candidate.extra_data.bindings {
141-
self.visit_binding(binding);
141+
if let super::SubpatternBindings::One(binding) = binding {
142+
self.visit_binding(binding);
143+
}
142144
}
143145
for match_pair in &candidate.match_pairs {
144146
self.visit_match_pair(match_pair);
@@ -147,7 +149,9 @@ impl<'a, 'b, 'tcx> FakeBorrowCollector<'a, 'b, 'tcx> {
147149

148150
fn visit_flat_pat(&mut self, flat_pat: &FlatPat<'tcx>) {
149151
for binding in &flat_pat.extra_data.bindings {
150-
self.visit_binding(binding);
152+
if let super::SubpatternBindings::One(binding) = binding {
153+
self.visit_binding(binding);
154+
}
151155
}
152156
for match_pair in &flat_pat.match_pairs {
153157
self.visit_match_pair(match_pair);

‎tests/mir-opt/match_arm_scopes.complicated_match.panic-abort.SimplifyCfg-initial.after-ElaborateDrops.after.diff

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -85,11 +85,11 @@
8585
_8 = &(_2.2: std::string::String);
8686
- _3 = &fake shallow (_2.0: bool);
8787
- _4 = &fake shallow (_2.1: bool);
88-
StorageLive(_12);
89-
StorageLive(_13);
90-
_13 = copy _1;
91-
- switchInt(move _13) -> [0: bb16, otherwise: bb15];
92-
+ switchInt(move _13) -> [0: bb13, otherwise: bb12];
88+
StorageLive(_9);
89+
StorageLive(_10);
90+
_10 = copy _1;
91+
- switchInt(move _10) -> [0: bb12, otherwise: bb11];
92+
+ switchInt(move _10) -> [0: bb9, otherwise: bb8];
9393
}
9494

9595
- bb9: {
@@ -100,11 +100,11 @@
100100
_8 = &(_2.2: std::string::String);
101101
- _3 = &fake shallow (_2.0: bool);
102102
- _4 = &fake shallow (_2.1: bool);
103-
StorageLive(_9);
104-
StorageLive(_10);
105-
_10 = copy _1;
106-
- switchInt(move _10) -> [0: bb12, otherwise: bb11];
107-
+ switchInt(move _10) -> [0: bb9, otherwise: bb8];
103+
StorageLive(_12);
104+
StorageLive(_13);
105+
_13 = copy _1;
106+
- switchInt(move _13) -> [0: bb16, otherwise: bb15];
107+
+ switchInt(move _13) -> [0: bb13, otherwise: bb12];
108108
}
109109

110110
- bb10: {
@@ -139,7 +139,7 @@
139139
- FakeRead(ForGuardBinding, _6);
140140
- FakeRead(ForGuardBinding, _8);
141141
StorageLive(_5);
142-
_5 = copy (_2.1: bool);
142+
_5 = copy (_2.0: bool);
143143
StorageLive(_7);
144144
_7 = move (_2.2: std::string::String);
145145
- goto -> bb10;
@@ -152,8 +152,8 @@
152152
StorageDead(_9);
153153
StorageDead(_8);
154154
StorageDead(_6);
155-
- falseEdge -> [real: bb1, imaginary: bb1];
156-
+ goto -> bb1;
155+
- falseEdge -> [real: bb3, imaginary: bb3];
156+
+ goto -> bb2;
157157
}
158158

159159
- bb15: {
@@ -181,7 +181,7 @@
181181
- FakeRead(ForGuardBinding, _6);
182182
- FakeRead(ForGuardBinding, _8);
183183
StorageLive(_5);
184-
_5 = copy (_2.0: bool);
184+
_5 = copy (_2.1: bool);
185185
StorageLive(_7);
186186
_7 = move (_2.2: std::string::String);
187187
- goto -> bb10;
@@ -194,8 +194,8 @@
194194
StorageDead(_12);
195195
StorageDead(_8);
196196
StorageDead(_6);
197-
- falseEdge -> [real: bb3, imaginary: bb3];
198-
+ goto -> bb2;
197+
- falseEdge -> [real: bb1, imaginary: bb1];
198+
+ goto -> bb1;
199199
}
200200

201201
- bb19: {

0 commit comments

Comments
(0)

AltStyle によって変換されたページ (->オリジナル) /