Skip to content

Commit

Permalink
Rollup merge of rust-lang#120214 - Nadrieril:fix-120210, r=pnkfelix
Browse files Browse the repository at this point in the history
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 rust-lang#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 rust-lang#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 rust-lang#120210

r? `@oli-obk` since you merged the original fix to rust-lang#69971
cc `@matthewjasper`
  • Loading branch information
matthiaskrgr authored Feb 3, 2024
2 parents 6e5c565 + 0825ad3 commit 4790a1f
Show file tree
Hide file tree
Showing 18 changed files with 316 additions and 262 deletions.
107 changes: 53 additions & 54 deletions compiler/rustc_mir_build/src/build/matches/simplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -198,8 +198,8 @@
StorageDead(_30);
_3 = ViewportPercentageLength::Vmax(move _29);
StorageDead(_29);
StorageDead(_28);
StorageDead(_27);
StorageDead(_28);
goto -> bb10;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
44 changes: 39 additions & 5 deletions tests/ui/pattern/bindings-after-at/bind-by-copy.rs
Original file line number Diff line number Diff line change
@@ -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); }
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/pattern/bindings-after-at/borrowck-move-and-move.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit 4790a1f

Please sign in to comment.