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

Unnecessary 'cannot bind by-move with sub-bindings' with bindings_after_at #69971

Closed
comex opened this issue Mar 13, 2020 · 3 comments · Fixed by #78638 or #120214
Closed

Unnecessary 'cannot bind by-move with sub-bindings' with bindings_after_at #69971

comex opened this issue Mar 13, 2020 · 3 comments · Fixed by #78638 or #120214
Labels
A-borrow-checker Area: The borrow checker A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-feature-request Category: A feature request, i.e: not implemented / a PR. F-bindings_after_at `#![feature(bindings_after_at)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@comex
Copy link
Contributor

comex commented Mar 13, 2020

The following code (playground link) attempts to move a struct while using a sub-binding to copy one of its fields:

#![feature(bindings_after_at)]

struct NonCopyStruct {
    copy_field: u32,
}

fn foo(x: NonCopyStruct) {
    let y @ NonCopyStruct { copy_field: z } = x;
}

On nightly, it produces two errors:

error[E0007]: cannot bind by-move with sub-bindings
 --> src/lib.rs:8:9
  |
8 |     let y @ NonCopyStruct { copy_field: z } = x;
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ binds an already bound by-move value by moving it
error[E0382]: use of moved value: `x`
 --> src/lib.rs:8:41
  |
7 | fn foo(x: NonCopyStruct) {
  |        - move occurs because `x` has type `NonCopyStruct`, which does not implement the `Copy` trait
8 |     let y @ NonCopyStruct { copy_field: z } = x;
  |         --------------------------------^--
  |         |                               |
  |         |                               value used here after move

However, it would be sound to allow this, since it's equivalent to copying the field before or after moving the struct.

(There are no errors if the struct does implement Copy.)

@Centril Centril added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. A-borrow-checker Area: The borrow checker A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html labels Mar 13, 2020
@Centril
Copy link
Contributor

Centril commented Mar 13, 2020

So the only interaction with #![feature(bindings_after_at)] here is that you're now allowed to mention bindings after @. The feature gate does nothing beyond removing a call to check_legality_of_bindings_in_at_patterns which raises an error on any bindings in the subpattern of ident @ subpattern.

If you also enable #![feature(move_ref_pattern)], then that removes a call to check_legality_of_move_bindings from which the first error is coming. The second error comes from the borrow checker.

cc @matthewjasper

@matthewjasper
Copy link
Contributor

This is the same as

fn foo(x: NonCopyStruct) {
    let y = x;
    let NonCopyStruct { copy_field: z } = x;
}

@Centril
Copy link
Contributor

Centril commented Mar 20, 2020

I tried to change this to:

fn foo(x: NonCopyStruct) {
    let NonCopyStruct { copy_field: z } = x;
    let y = x;
}

in MIR lowering by adjusting the visiting order in two places, but that didn't change the outcome. Perhaps that's because of the treatment of _?

At any rate, guaranteeing the visiting order here seems suboptimal wrt. for example making x @ y a true intersection pattern and y before x seems contrary to LTR reading order.

@scottmcm scottmcm added the F-bindings_after_at `#![feature(bindings_after_at)]` label Oct 31, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Nov 3, 2020
… r=oli-obk

reverse binding order in matches to allow the subbinding of copyable fields in bindings after @

Fixes rust-lang#69971

### TODO

- [x] Regression tests

r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 5, 2020
…=oli-obk

reverse binding order in matches to allow the subbinding of copyable fields in bindings after @

Fixes rust-lang#69971

### TODO

- [x] Regression tests

r? `@oli-obk`
@bors bors closed this as completed in c93d25b Nov 5, 2020
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``````
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```````
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
A-borrow-checker Area: The borrow checker A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-feature-request Category: A feature request, i.e: not implemented / a PR. F-bindings_after_at `#![feature(bindings_after_at)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
4 participants