bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Rollup merge of #120214 - Nadrieril:fix-120210, r=pnkfelix

Browse source 
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 https://github.com/rust-lang/rust/issues/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 https://github.com/rust-lang/rust/issues/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 https://github.com/rust-lang/rust/issues/120210

r? ```````@oli-obk``````` since you merged the original fix to https://github.com/rust-lang/rust/issues/69971
cc ```````@matthewjasper```````
This commit is contained in:
Matthias Krüger 2024-02-08 09:06:33 +01:00 • committed by GitHub
commit 7fb36f2d3b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
18 changed files with 317 additions and 263 deletions
Download patch file Download diff file Expand all files Collapse all files

107
compiler/rustc_mir_build/src/build/matches/simplify.rs
View file

@ -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):
//
// 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).
// 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;
// }
//
// 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;
@ -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,