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

Inconsistent error when using bindings after @ #120210

Closed
Nadrieril opened this issue Jan 21, 2024 · 0 comments · Fixed by #120214
Closed

Inconsistent error when using bindings after @ #120210

Nadrieril opened this issue Jan 21, 2024 · 0 comments · Fixed by #120214
Labels
C-bug Category: This is a bug. F-bindings_after_at `#![feature(bindings_after_at)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Nadrieril
Copy link
Member

Nadrieril commented Jan 21, 2024

In the following, the first case compiles (playground) and the second errors (playground). The only difference is adding a variant to the enum.

enum NonCopyEnum1 {
    Variant { copy_field: u32 },
}

fn foo1(x: NonCopyEnum1) {
    match x {
        y @ NonCopyEnum1::Variant { copy_field: z } => {}
    }
}

enum NonCopyEnum2 {
    Variant { copy_field: u32 },
    None,
}

fn foo2(x: NonCopyEnum2) {
    match x {
        y @ NonCopyEnum2::Variant { copy_field: z } => {} // ERROR use of moved value: `x`
        _ => {}
    }
}

Error:

error[E0382]: use of moved value: `x`
 --> src/lib.rs:9:49
  |
7 | fn foo2(x: NonCopyEnum2) {
  |         - move occurs because `x` has type `NonCopyEnum2`, which does not implement the `Copy` trait
8 |     match x {
9 |         y @ NonCopyEnum2::Variant { copy_field: z } => {}
  |         - value moved here                      ^ value used here after move
  |
help: borrow this binding in the pattern to avoid moving the value
  |
9 |         ref y @ NonCopyEnum2::Variant { copy_field: z } => {}
  |         +++

This is an extension to #69971. I discovered this while poking at the match lowering code. Essentially, #69971 was accidentally only fixed for the case of irrefutable patterns, since patterns that require a test go through a different code path.

@Nadrieril Nadrieril added C-bug Category: This is a bug. F-bindings_after_at `#![feature(bindings_after_at)]` labels Jan 21, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 21, 2024
@fmease fmease added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 23, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue 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 issue 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 issue 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```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue 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````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue 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`````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue 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 bors closed this as completed in 7fb36f2 Feb 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue 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```````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. F-bindings_after_at `#![feature(bindings_after_at)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants