Skip to content

Commit

Permalink
Rollup merge of rust-lang#121716 - Nadrieril:simple-binding-order, r=…
Browse files Browse the repository at this point in the history
…matthewjasper

match lowering: Lower bindings in a predictable order

After the recent refactorings, we can now lower bindings in a truly predictable order. The order in rust-lang#120214 was an improvement but not very clear. With this PR, we lower bindings from left to right, with the special case that `x @ pat` is traversed as `pat @ x` (i.e. `x` is lowered after any bindings in `pat`).

This description only applies in the absence of or-patterns. Or-patterns make everything complicated, because the binding place depends on the subpattern. Until I have a better idea I leave them to be handled in whatever weird order arises from today's code.

r? `@matthewjasper`
  • Loading branch information
Nadrieril authored Mar 2, 2024
2 parents b0ca9b5 + 00497ad commit 30976fb
Show file tree
Hide file tree
Showing 11 changed files with 118 additions and 100 deletions.
8 changes: 4 additions & 4 deletions compiler/rustc_mir_build/src/build/matches/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -701,7 +701,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
self.bind_pattern(
self.source_info(irrefutable_pat.span),
candidate,
&fake_borrow_temps,
fake_borrow_temps.as_slice(),
irrefutable_pat.span,
None,
false,
Expand Down Expand Up @@ -1938,7 +1938,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let post_guard_block = self.bind_pattern(
self.source_info(pat.span),
guard_candidate,
&fake_borrow_temps,
fake_borrow_temps.as_slice(),
expr_span,
None,
false,
Expand Down Expand Up @@ -2425,7 +2425,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let matching = this.bind_pattern(
this.source_info(pattern.span),
candidate,
&fake_borrow_temps,
fake_borrow_temps.as_slice(),
initializer_span,
None,
true,
Expand All @@ -2434,7 +2434,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let failure = this.bind_pattern(
this.source_info(else_block_span),
wildcard,
&fake_borrow_temps,
fake_borrow_temps.as_slice(),
initializer_span,
None,
true,
Expand Down
70 changes: 20 additions & 50 deletions compiler/rustc_mir_build/src/build/matches/simplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,60 +41,30 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// let y = x;
// }
//
// 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]
// bindings in iter 1: [4, 5]
// bindings in iter 2: [6, 7]
//
// final bindings: [6, 7, 4, 5, 1, 2, 3]
let mut accumulated_bindings = mem::take(candidate_bindings);
let mut simplified_match_pairs = Vec::new();
// Repeatedly simplify match pairs until we're left with only unsimplifiable ones.
loop {
for mut match_pair in mem::take(match_pairs) {
if let TestCase::Irrefutable { binding, ascription } = match_pair.test_case {
if let Some(binding) = binding {
candidate_bindings.push(binding);
}
if let Some(ascription) = ascription {
candidate_ascriptions.push(ascription);
}
// Simplifiable pattern; we replace it with its subpairs and simplify further.
match_pairs.append(&mut match_pair.subpairs);
} else {
// Unsimplifiable pattern; we recursively simplify its subpairs and don't
// process it further.
self.simplify_match_pairs(
&mut match_pair.subpairs,
candidate_bindings,
candidate_ascriptions,
);
simplified_match_pairs.push(match_pair);
// We therefore lower bindings from left-to-right, except we lower the `x` in `x @ pat`
// after any bindings in `pat`. This doesn't work for or-patterns: the current structure of
// match lowering forces us to lower bindings inside or-patterns last.
for mut match_pair in mem::take(match_pairs) {
self.simplify_match_pairs(
&mut match_pair.subpairs,
candidate_bindings,
candidate_ascriptions,
);
if let TestCase::Irrefutable { binding, ascription } = match_pair.test_case {
if let Some(binding) = binding {
candidate_bindings.push(binding);
}
}

// This does: accumulated_bindings = candidate.bindings.take() ++ accumulated_bindings
candidate_bindings.extend_from_slice(&accumulated_bindings);
mem::swap(candidate_bindings, &mut accumulated_bindings);
candidate_bindings.clear();

if match_pairs.is_empty() {
break;
if let Some(ascription) = ascription {
candidate_ascriptions.push(ascription);
}
// Simplifiable pattern; we replace it with its already simplified subpairs.
match_pairs.append(&mut match_pair.subpairs);
} else {
// Unsimplifiable pattern; we keep it.
match_pairs.push(match_pair);
}
}

// Store computed bindings back in `candidate_bindings`.
mem::swap(candidate_bindings, &mut accumulated_bindings);
// Store simplified match pairs back in `match_pairs`.
mem::swap(match_pairs, &mut simplified_match_pairs);

// 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,13 @@
- }
-
- bb3: {
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);
StorageLive(_9);
_9 = (((_3.1: std::option::Option<u32>) as Some).0: u32);
_0 = const 0_u32;
StorageDead(_8);
StorageDead(_9);
StorageDead(_8);
- goto -> bb4;
+ goto -> bb3;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,13 @@
- }
-
- bb4: {
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);
StorageLive(_10);
_10 = (((_3.1: std::option::Option<u32>) as Some).0: u32);
_0 = const 0_u32;
StorageDead(_9);
StorageDead(_10);
StorageDead(_9);
- goto -> bb6;
+ goto -> bb4;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,13 @@
- }
-
- bb3: {
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);
StorageLive(_9);
_9 = (((_3.1: std::option::Option<bool>) as Some).0: bool);
_0 = const 0_u32;
StorageDead(_8);
StorageDead(_9);
StorageDead(_8);
- goto -> bb4;
+ goto -> bb3;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,16 @@

- bb4: {
+ bb3: {
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);
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);
_0 = const 0_u32;
StorageDead(_11);
StorageDead(_12);
StorageDead(_13);
StorageDead(_12);
StorageDead(_11);
- goto -> bb5;
+ goto -> bb4;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,12 @@
}

bb6: {
StorageLive(_13);
_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);
_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);
StorageLive(_14);
StorageLive(_15);
_15 = _12;
Expand All @@ -132,18 +132,18 @@
StorageDead(_15);
_3 = ViewportPercentageLength::Vw(move _14);
StorageDead(_14);
StorageDead(_12);
StorageDead(_13);
StorageDead(_12);
goto -> bb10;
}

bb7: {
StorageLive(_18);
_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);
_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);
StorageLive(_19);
StorageLive(_20);
_20 = _17;
Expand All @@ -154,18 +154,18 @@
StorageDead(_20);
_3 = ViewportPercentageLength::Vh(move _19);
StorageDead(_19);
StorageDead(_17);
StorageDead(_18);
StorageDead(_17);
goto -> bb10;
}

bb8: {
StorageLive(_23);
_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);
_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);
StorageLive(_24);
StorageLive(_25);
_25 = _22;
Expand All @@ -176,18 +176,18 @@
StorageDead(_25);
_3 = ViewportPercentageLength::Vmin(move _24);
StorageDead(_24);
StorageDead(_22);
StorageDead(_23);
StorageDead(_22);
goto -> bb10;
}

bb9: {
StorageLive(_28);
_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);
_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);
StorageLive(_29);
StorageLive(_30);
_30 = _27;
Expand All @@ -198,8 +198,8 @@
StorageDead(_30);
_3 = ViewportPercentageLength::Vmax(move _29);
StorageDead(_29);
StorageDead(_27);
StorageDead(_28);
StorageDead(_27);
goto -> bb10;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,13 @@
}

bb5: {
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);
StorageLive(_10);
_10 = (((_3.1: std::option::Option<u32>) as Some).0: u32);
_0 = const 0_u32;
StorageDead(_9);
StorageDead(_10);
StorageDead(_9);
goto -> bb8;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ fn match_tuple(_1: (u32, bool, Option<i32>, u32)) -> u32 {

bb0: {
PlaceMention(_1);
_2 = discriminant((_1.2: std::option::Option<i32>));
switchInt(move _2) -> [0: bb3, 1: bb2, otherwise: bb1];
switchInt((_1.0: u32)) -> [1: bb2, 4: bb2, otherwise: bb1];
}

bb1: {
Expand All @@ -29,11 +28,12 @@ fn match_tuple(_1: (u32, bool, Option<i32>, u32)) -> u32 {
}

bb2: {
switchInt((((_1.2: std::option::Option<i32>) as Some).0: i32)) -> [1: bb3, 8: bb3, otherwise: bb1];
_2 = discriminant((_1.2: std::option::Option<i32>));
switchInt(move _2) -> [0: bb4, 1: bb3, otherwise: bb1];
}

bb3: {
switchInt((_1.0: u32)) -> [1: bb4, 4: bb4, otherwise: bb1];
switchInt((((_1.2: std::option::Option<i32>) as Some).0: i32)) -> [1: bb4, 8: bb4, otherwise: bb1];
}

bb4: {
Expand Down
17 changes: 17 additions & 0 deletions tests/ui/pattern/bindings-after-at/bind-by-copy-or-pat.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//@ known-bug: unknown
#![allow(unused)]

struct A(u32);

pub fn main() {
// The or-pattern bindings are lowered after `x`, which triggers the error.
let x @ (A(a) | A(a)) = A(10);
// ERROR: use of moved value
assert!(x.0 == 10);
assert!(a == 10);

// This works.
let (x @ A(a) | x @ A(a)) = A(10);
assert!(x.0 == 10);
assert!(a == 10);
}
31 changes: 31 additions & 0 deletions tests/ui/pattern/bindings-after-at/bind-by-copy-or-pat.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
error[E0382]: use of moved value
--> $DIR/bind-by-copy-or-pat.rs:8:16
|
LL | let x @ (A(a) | A(a)) = A(10);
| - ^ ----- move occurs because value has type `A`, which does not implement the `Copy` trait
| | |
| | value used here after move
| value moved here
|
help: borrow this binding in the pattern to avoid moving the value
|
LL | let ref x @ (A(a) | A(a)) = A(10);
| +++

error[E0382]: use of moved value
--> $DIR/bind-by-copy-or-pat.rs:8:23
|
LL | let x @ (A(a) | A(a)) = A(10);
| - ^ ----- move occurs because value has type `A`, which does not implement the `Copy` trait
| | |
| | value used here after move
| value moved here
|
help: borrow this binding in the pattern to avoid moving the value
|
LL | let ref x @ (A(a) | A(a)) = A(10);
| +++

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0382`.

0 comments on commit 30976fb

Please sign in to comment.