Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

match lowering: consistently lower bindings deepest-first #120214

Merged
merged 5 commits into from
Feb 8, 2024

Conversation

Nadrieril
Copy link
Member

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 #120210. The reason we need a dance at all is for situations like:

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 #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 #120210

r? @oli-obk since you merged the original fix to #69971
cc @matthewjasper

@Nadrieril Nadrieril added the F-bindings_after_at `#![feature(bindings_after_at)]` label Jan 21, 2024
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 21, 2024
@rust-log-analyzer

This comment has been minimized.

@Nadrieril
Copy link
Member Author

🤦 forgot the mir_opt tests. I'll fix that tmr

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 21, 2024
@Nadrieril
Copy link
Member Author

Nadrieril commented Jan 22, 2024

Having slept on it, this actually lowers (a, Some(b)) backwards. But (a, (b,)) is lowered backwards on current rust already if I follow correctly? What are the rules and requirements here?

EDIT: asked on Zulip

@oli-obk
Copy link
Contributor

oli-obk commented Jan 24, 2024

r? @pnkfelix

@rustbot rustbot assigned pnkfelix and unassigned oli-obk Jan 24, 2024
@Nadrieril
Copy link
Member Author

My conclusion from the Zulip discussion is that we don't really have guarantees here, so this seems fine.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 25, 2024
@pnkfelix
Copy link
Member

pnkfelix commented Feb 2, 2024

Hmm. I'll want to think about this.

I believe I understand the motivation for "deepest-first", but I also have to admit that I was hoping for us to be able to work towards an evaluation order that doesn't require a human to count levels of depth.

In other words, moving to deepest first here might inadvertantly commit us to using it forevermore... and as I mentioned in Zulip, I'm specifically concerned about pattern match order-of-eval when one is dealing with unions-of-structs.

@Nadrieril
Copy link
Member Author

Give the wild order we already have, I was hoping this wouldn't be much worse. My medium-term plan is to refactor march lowering so we get actual control over this order instead of mixing it up with the rest of lowering. This PR is the first step towards that

@pnkfelix
Copy link
Member

pnkfelix commented Feb 2, 2024

Okay, I think I can agree with @Nadrieril that this PR doesn't make things worse.

I'm still nervous about any kind of commitment to "deepest-first", but I'm willing to land this in the hopes that the overall order-of-evaluation for patterns becomes easier to deal with after these sorts of refactorings to the code.

@pnkfelix
Copy link
Member

pnkfelix commented Feb 2, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Feb 2, 2024

📌 Commit 0825ad3 has been approved by pnkfelix

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 2, 2024

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2024
@Nadrieril
Copy link
Member Author

Thank you!

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 3, 2024
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`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 3, 2024
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``
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#116284 (make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern)
 - rust-lang#120060 (Use the same mir-opt bless targets on all platforms)
 - rust-lang#120214 (match lowering: consistently lower bindings deepest-first)
 - rust-lang#120326 (Actually abort in -Zpanic-abort-tests)
 - rust-lang#120396 (Account for unbounded type param receiver in suggestions)
 - rust-lang#120435 (Suggest name value cfg when only value is used for check-cfg)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
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```
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#113833 (`std::error::Error` -> Trait Implementations: lifetimes consistency improvement)
 - rust-lang#115386 (PartialEq, PartialOrd: update and synchronize handling of transitive chains)
 - rust-lang#116284 (make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern)
 - rust-lang#118960 (Add LocalWaker and ContextBuilder types to core, and LocalWake trait to alloc.)
 - rust-lang#120060 (Use the same mir-opt bless targets on all platforms)
 - rust-lang#120214 (match lowering: consistently lower bindings deepest-first)
 - rust-lang#120384 (Use `<T, U>` for array/slice equality `impl`s)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
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````
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#120023 (tidy: reduce allocs)
 - rust-lang#120214 (match lowering: consistently lower bindings deepest-first)
 - rust-lang#120396 (Account for unbounded type param receiver in suggestions)
 - rust-lang#120423 (update indirect structural match lints to match RFC and to show up for dependencies)
 - rust-lang#120435 (Suggest name value cfg when only value is used for check-cfg)
 - rust-lang#120507 (Account for non-overlapping unmet trait bounds in suggestion)
 - rust-lang#120521 (Make `NonZero` constructors generic.)
 - rust-lang#120527 (Switch OwnedStore handle count to AtomicU32)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 7, 2024
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`````
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#119592 (resolve: Unload speculatively resolved crates before freezing cstore)
 - rust-lang#120103 (Make it so that async-fn-in-trait is compatible with a concrete future in implementation)
 - rust-lang#120206 (hir: Make sure all `HirId`s have corresponding HIR `Node`s)
 - rust-lang#120214 (match lowering: consistently lower bindings deepest-first)
 - rust-lang#120596 ([rustdoc] Correctly generate path for non-local items in source code pages)
 - rust-lang#120688 (GVN: also turn moves into copies with projections)
 - rust-lang#120702 (docs: also check the inline stmt during redundant link check)
 - rust-lang#120739 (improve pretty printing for associated items in trait objects)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 8, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#119592 (resolve: Unload speculatively resolved crates before freezing cstore)
 - rust-lang#120103 (Make it so that async-fn-in-trait is compatible with a concrete future in implementation)
 - rust-lang#120206 (hir: Make sure all `HirId`s have corresponding HIR `Node`s)
 - rust-lang#120214 (match lowering: consistently lower bindings deepest-first)
 - rust-lang#120596 ([rustdoc] Correctly generate path for non-local items in source code pages)
 - rust-lang#120688 (GVN: also turn moves into copies with projections)
 - rust-lang#120702 (docs: also check the inline stmt during redundant link check)
 - rust-lang#120739 (improve pretty printing for associated items in trait objects)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 8, 2024
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``````
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 8, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119592 (resolve: Unload speculatively resolved crates before freezing cstore)
 - rust-lang#120103 (Make it so that async-fn-in-trait is compatible with a concrete future in implementation)
 - rust-lang#120206 (hir: Make sure all `HirId`s have corresponding HIR `Node`s)
 - rust-lang#120214 (match lowering: consistently lower bindings deepest-first)
 - rust-lang#120596 ([rustdoc] Correctly generate path for non-local items in source code pages)
 - rust-lang#120688 (GVN: also turn moves into copies with projections)
 - rust-lang#120727 (exhaustiveness: Prefer "`0..MAX` not covered" to "`_` not covered")
 - rust-lang#120734 (Add `SubdiagnosticMessageOp` as a trait alias.)
 - rust-lang#120739 (improve pretty printing for associated items in trait objects)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 8, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119592 (resolve: Unload speculatively resolved crates before freezing cstore)
 - rust-lang#120103 (Make it so that async-fn-in-trait is compatible with a concrete future in implementation)
 - rust-lang#120206 (hir: Make sure all `HirId`s have corresponding HIR `Node`s)
 - rust-lang#120214 (match lowering: consistently lower bindings deepest-first)
 - rust-lang#120688 (GVN: also turn moves into copies with projections)
 - rust-lang#120702 (docs: also check the inline stmt during redundant link check)
 - rust-lang#120727 (exhaustiveness: Prefer "`0..MAX` not covered" to "`_` not covered")
 - rust-lang#120734 (Add `SubdiagnosticMessageOp` as a trait alias.)
 - rust-lang#120739 (improve pretty printing for associated items in trait objects)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7fb36f2 into rust-lang:master Feb 8, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 8, 2024
Rollup merge of rust-lang#120214 - Nadrieril:fix-120210, r=pnkfelix

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```````
@Nadrieril Nadrieril deleted the fix-120210 branch February 8, 2024 14:23
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 26, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119592 (resolve: Unload speculatively resolved crates before freezing cstore)
 - rust-lang#120103 (Make it so that async-fn-in-trait is compatible with a concrete future in implementation)
 - rust-lang#120206 (hir: Make sure all `HirId`s have corresponding HIR `Node`s)
 - rust-lang#120214 (match lowering: consistently lower bindings deepest-first)
 - rust-lang#120688 (GVN: also turn moves into copies with projections)
 - rust-lang#120702 (docs: also check the inline stmt during redundant link check)
 - rust-lang#120727 (exhaustiveness: Prefer "`0..MAX` not covered" to "`_` not covered")
 - rust-lang#120734 (Add `SubdiagnosticMessageOp` as a trait alias.)
 - rust-lang#120739 (improve pretty printing for associated items in trait objects)

r? `@ghost`
`@rustbot` modify labels: rollup
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Mar 2, 2024
…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`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 3, 2024
Rollup merge of rust-lang#121716 - Nadrieril:simple-binding-order, r=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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-bindings_after_at `#![feature(bindings_after_at)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
6 participants