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

Tracking issue for pattern with by-move & by-ref bindings #68354

Closed
Centril opened this issue Jan 18, 2020 · 25 comments
Closed

Tracking issue for pattern with by-move & by-ref bindings #68354

Centril opened this issue Jan 18, 2020 · 25 comments
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) A-patterns Relating to patterns and pattern matching B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. F-move_ref_pattern `#![feature(move_ref_pattern)]` NLL-complete Working towards the "valid code works" goal T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Jan 18, 2020

This is the tracking issue for #![feature(move_ref_pattern)], which allows patterns containing both by-ref and by-move bindings at the same time. For example: (ref x, y, ref mut z).

Implementation history

@Centril Centril added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC F-move_ref_pattern `#![feature(move_ref_pattern)]` labels Jan 18, 2020
@Centril Centril changed the title Tracking issue for pattern with by-move & by-ref bindings (#![feature(move_ref_pattern)]) Tracking issue for pattern with by-move & by-ref bindings Jan 18, 2020
@Centril Centril self-assigned this Jan 18, 2020
@Centril Centril added A-NLL Area: Non-lexical lifetimes (NLL) NLL-complete Working towards the "valid code works" goal labels Jan 19, 2020
@CryZe

This comment has been minimized.

@Centril

This comment has been minimized.

@CryZe

This comment has been minimized.

bors added a commit that referenced this issue Feb 9, 2020
Initial implementation of `#![feature(move_ref_pattern)]`

Following up on #45600, under the gate `#![feature(move_ref_pattern)]`, `(ref x, mut y)` is allowed subject to restrictions necessary for soundness. The match checking implementation and tests for `#![feature(bindings_after_at)]` is also adjusted as necessary.

Closes #45600.
Tracking issue: #68354.

r? @matthewjasper
@Centril Centril added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Feb 10, 2020
@Centril
Copy link
Contributor Author

Centril commented Feb 10, 2020

As of now, we don't believe more tests would needed before we stabilize, but if you can think of some, please do add or propose them. What we're waiting now is probably for more time to pass.

@jjpe
Copy link

jjpe commented Mar 18, 2020

When could this feature potentially be stabilized, assuming no (major) issues are found?
I ask because I would like to optimize a derive-style proc-macro crate I'm writing. Currently the generated code contains a match on 2 by-ref bindings, but performing the optimization would result in one by-ref and one by-move binding in a single match.

Of course the response might be "just use another control flow structure, e.g. if-let", and in principle I could do that. In practice though, because it is code generation code, debugging is harder, and I'd prefer to keep the current code intact as much as possible. So much so that I'd rather wait for this feature to become stable if the amount of time it will take to stabilize is not too great.

@Centril
Copy link
Contributor Author

Centril commented Mar 18, 2020

So this feature hasn't seen any dogfooding inside the compiler itself, and I don't know what the testing situation in the ecosystem is like. However, the src/test/ui testsuite is extensive, so maybe that is enough. If @matthewjasper thinks this is good enough we could probably stabilize in 1.44.

@Centril Centril added the A-patterns Relating to patterns and pattern matching label Apr 10, 2020
@jjpe
Copy link

jjpe commented Jun 29, 2020

Meanwhile, we're a number of months further, the world is different, and I just ran into this very same issue again.
Do you think this feature has been tested enough for inclusion into stable yet? I'd really like to see this stabilized ASAP.

@jjpe
Copy link

jjpe commented Jul 3, 2020

Especially since the test suite is extensive, shouldn't this be stabilizable in time for the next stable release?

@matthewjasper
Copy link
Contributor

Nominating for T-lang to discuss stabilization.

@nikomatsakis
Copy link
Contributor

Discussing in the @rust-lang/lang meeting today:

We would love to see this stabilized. @matthewjasper if you or someone else wants to prepare a PR and stabilization report (showing interesting test cases and with a short description of the behavior to be stabilized), that would be great!

One thing that would be nice is seeing links to this feature being used "in the wild" (i.e., in the ecosystem), but maybe we don't know of any projects. (One could search for the feature gate...)

@joshtriplett
Copy link
Member

It looks like there are no uses of the feature gate outside of rustc and announcements/metadata. That shouldn't necessarily stop us going forward, but it seems worth noting.

This is the kind of feature, though, that people won't necessarily use nightly just to get.

@jjpe
Copy link

jjpe commented Jul 6, 2020

One thing that would be nice is seeing links to this feature being used "in the wild" (i.e., in the ecosystem), but maybe we don't know of any projects. (One could search for the feature gate...)

I've consistently been asking for this feature precisely because I want to use it in the wild :)
Specifically, I've been developing the deltoid and deltoid-derive crates which, once I open source them, will make it a lot easier to do delta calculations on deeply nested data trees (a notable exclusion being types that use borrows, because you can't shove a newly created value into a borrow).
The derive macro will use this feature once the feature is stabilized, but until then it necessarily operates with reduced performance. This is because in order to avoid this pattern (in my case, a tuple of an owned type and a borrow) I now need to borrow the owned type in the match expression, and clone from the destructured borrows that the match construct provides to me.
This feature being in stable will allow me to remove the artificial borrow-and-clone in the generated code.

As for how this pattern appears in my code in the first place, when possible I often match on a tuple of stuff rather than do nested matches, because rightward drift can be a real PITA, both in the subjective more-nesting-is-more-context-to-remember sense as well as just having less space to write nice one-liners, which then become multiliners.

This is the kind of feature, though, that people won't necessarily use nightly just to get.

Quite true. The deltoid and deltoid-derive crates are already used at the company where I work, and suddenly requiring a nightly rustc is a hard pass over there. That's why I have not mentioned that at all, instead compromising in a different way i.e. a performance penalty for the time being.

@tesuji
Copy link
Contributor

tesuji commented Jul 7, 2020

I should say that https://grep.app only scans about one million repositories on GitHub.
The more exact search should use GitHub search feature: https://github.com/search?l=&p=1&q=move_ref_pattern+language%3ARust&ref=advsearch&type=Code
Through some most of them just vendor Rust or Rust testsuite.

@Heliozoa
Copy link
Contributor

Heliozoa commented Jul 24, 2020

FWIW I ran into this issue today when fetching the status of some process running on a server in a loop and trying to match a previous_status variable and the new status to figure out if there's been any change. The function that caused the error looked something like this (playground)

#[derive(PartialEq, Eq)]
struct Finished {}

#[derive(PartialEq, Eq)]
struct Processing {
    status: ProcessStatus,
}

#[derive(PartialEq, Eq)]
enum ProcessStatus {
    One,
    Two,
    Three,
}

#[derive(PartialEq, Eq)]
enum Status {
    Finished(Finished),
    Processing(Processing),
}

fn check_result(_url: &str) -> Status {
    // fetch status from some server
    Status::Processing(Processing {
        status: ProcessStatus::One,
    })
}

fn wait_for_result(url: &str) -> Finished {
    let mut previous_status = None;
    loop {
        match check_result(url) {
            Status::Finished(f) => return f,
            Status::Processing(p) => {
                match (&mut previous_status, p.status) {
                    (None, status) => previous_status = Some(status), // first status
                    (Some(previous), status) if *previous == status => {} // no change, ignore
                    (Some(previous), status) => { // error!
                        // new status
                        *previous = status;
                    }
                }
            }
        }
    }
}

In my case the fix was easy enough, I realized I want to do the same processing that would go in the new status case with the first status as well, so I could just do

                    (Some(previous), status) if *previous == status => {} // no change, ignore
                    (_, status) => {
                        // new status
                        previous_status = Some(status);
                    }

Though in my case a workaround was trivial, this is what a use in the wild might've looked like if this was in stable today so I thought I'd post it. It felt like something that should work, so I'm glad to see that work is underway to stabilize it even if I'm not necessarily in a hurry to use it myself. Thanks!

@nikomatsakis nikomatsakis added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Jul 27, 2020
@nikomatsakis
Copy link
Contributor

Tagging with E-mentor -- this needs a stabilization PR, as described here:

https://rustc-dev-guide.rust-lang.org/stabilization_guide.html

The PR should include a brief report, but it doesn't have to go into details. The short version is:

  • The older borrow checker prevents a single pattern from including both "by move" and "by ref" patterns (e.g., Foo(x, ref y)). This newer borrow checker can handle those soundly, but we still kept them feature gated.
  • Rendering this feature gate unnecessary will highlight whatever tests we have, we should check to be sure that they seem representative when we see the PR.
  • The lang-team decided here that we'd like to see this stabilized already, so we can presumably jump straight to FCP.

@jjpe
Copy link

jjpe commented Jul 31, 2020

@niko I've taken a quick look at what's involved in writing such a stabilization PR.
I'm willing to do it, but I don't have the time at this precise moment for all of it.

Is there a possibility to perhaps have 2 or more people share the honor and responsibility?
Or would the communication overhead act as noise and drown out the signal in such a case?

@nikomatsakis
Copy link
Contributor

@jjpe seems like that would be fine to me, if somebody else wants to help out!

@Amjad50
Copy link
Contributor

Amjad50 commented Aug 27, 2020

Hi @nikomatsakis @jjpe, I'm interested in helping with stabilizing this feature.

@jjpe
Copy link

jjpe commented Aug 28, 2020

While I love that there are now people willing to take this up, in the past month life has gotten much, much busier for me.
So even though I'd really like to do this, I cannot at this time.

@Amjad50
Copy link
Contributor

Amjad50 commented Aug 28, 2020

@rustbot claim

@rustbot rustbot assigned Amjad50 and unassigned Centril Aug 28, 2020
@Amjad50
Copy link
Contributor

Amjad50 commented Aug 28, 2020

I'll start working on stabilizing this feature

@Amjad50
Copy link
Contributor

Amjad50 commented Aug 29, 2020

Process for stabilization:

@Amjad50
Copy link
Contributor

Amjad50 commented Aug 30, 2020

Does this feature removes Error E0007?

Example:

#![feature(bindings_after_at)]

fn main() {
    let x = Some("s".to_string());
    match x {
        op_string @ Some(s) => {},
        //~^ ERROR E0007
        //~| ERROR E0382
        None => {},
    }
}

Error:

error[E0007]: cannot bind by-move with sub-bindings
  --> $DIR/E0007.rs:6:9
   |
LL |         op_string @ Some(s) => {},
   |         ^^^^^^^^^^^^^^^^^^^ binds an already bound by-move value by moving it

JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 15, 2020
…n, r=nikomatsakis

Stabilize move_ref_pattern

# Implementation
- Initially the rule was added in the run-up to 1.0. The AST-based borrow checker was having difficulty correctly enforcing match expressions that combined ref and move bindings, and so it was decided to simplify forbid the combination out right.
- The move to MIR-based borrow checking made it possible to enforce the rules in a finer-grained level, but we kept the rule in place in an effort to be conservative in our changes.
- In rust-lang#68376, @Centril lifted the restriction but required a feature-gate.
- This PR removes the feature-gate.

Tracking issue: rust-lang#68354.

# Description
This PR is to stabilize the feature `move_ref_pattern`, which allows patterns
containing both `by-ref` and `by-move` bindings at the same time.

For example: `Foo(ref x, y)`, where `x` is `by-ref`,
and `y` is `by-move`.

The rules of moving a variable also apply here when moving *part* of a variable,
such as it can't be referenced or moved before.

If this pattern is used, it would result in *partial move*, which means that
part of the variable is moved. The variable that was partially moved from
cannot be used as a whole in this case, only the parts that are still
not moved can be used.

## Documentation
- The reference (rust-lang/reference#881)
- Rust by example (rust-lang/rust-by-example#1377)

## Tests
There are many tests, but I think one of the comperhensive ones:
- [borrowck-move-ref-pattern-pass.rs](https://github.com/Centril/rust/blob/85fbf49ce0e2274d0acf798f6e703747674feec3/src/test/ui/pattern/move-ref-patterns/borrowck-move-ref-pattern-pass.rs)
- [borrowck-move-ref-pattern.rs](https://github.com/Centril/rust/blob/85fbf49ce0e2274d0acf798f6e703747674feec3/src/test/ui/pattern/move-ref-patterns/borrowck-move-ref-pattern.rs)

# Examples

```rust
#[derive(PartialEq, Eq)]
struct Finished {}

#[derive(PartialEq, Eq)]
struct Processing {
    status: ProcessStatus,
}

#[derive(PartialEq, Eq)]
enum ProcessStatus {
    One,
    Two,
    Three,
}

#[derive(PartialEq, Eq)]
enum Status {
    Finished(Finished),
    Processing(Processing),
}

fn check_result(_url: &str) -> Status {
    // fetch status from some server
    Status::Processing(Processing {
        status: ProcessStatus::One,
    })
}

fn wait_for_result(url: &str) -> Finished {
    let mut previous_status = None;
    loop {
        match check_result(url) {
            Status::Finished(f) => return f,
            Status::Processing(p) => {
                match (&mut previous_status, p.status) {
                    (None, status) => previous_status = Some(status), // first status
                    (Some(previous), status) if *previous == status => {} // no change, ignore
                    (Some(previous), status) => { // Now it can be used
                        // new status
                        *previous = status;
                    }
                }
            }
        }
    }
}
```

Before, we would have used:
```rust
                match (&previous_status, p.status) {
                    (Some(previous), status) if *previous == status => {} // no change, ignore
                    (_, status) => {
                        // new status
                        previous_status = Some(status);
                    }
                }
```

Demonstrating *partial move*
```rust
fn main() {
    #[derive(Debug)]
    struct Person {
        name: String,
        age: u8,
    }

    let person = Person {
        name: String::from("Alice"),
        age: 20,
    };

    // `name` is moved out of person, but `age` is referenced
    let Person { name, ref age } = person;

    println!("The person's age is {}", age);

    println!("The person's name is {}", name);

    // Error! borrow of partially moved value: `person` partial move occurs
    //println!("The person struct is {:?}", person);

    // `person` cannot be used but `person.age` can be used as it is not moved
    println!("The person's age from person struct is {}", person.age);
}
```
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Oct 15, 2020
…n, r=nikomatsakis

Stabilize move_ref_pattern

# Implementation
- Initially the rule was added in the run-up to 1.0. The AST-based borrow checker was having difficulty correctly enforcing match expressions that combined ref and move bindings, and so it was decided to simplify forbid the combination out right.
- The move to MIR-based borrow checking made it possible to enforce the rules in a finer-grained level, but we kept the rule in place in an effort to be conservative in our changes.
- In rust-lang#68376, @Centril lifted the restriction but required a feature-gate.
- This PR removes the feature-gate.

Tracking issue: rust-lang#68354.

# Description
This PR is to stabilize the feature `move_ref_pattern`, which allows patterns
containing both `by-ref` and `by-move` bindings at the same time.

For example: `Foo(ref x, y)`, where `x` is `by-ref`,
and `y` is `by-move`.

The rules of moving a variable also apply here when moving *part* of a variable,
such as it can't be referenced or moved before.

If this pattern is used, it would result in *partial move*, which means that
part of the variable is moved. The variable that was partially moved from
cannot be used as a whole in this case, only the parts that are still
not moved can be used.

## Documentation
- The reference (rust-lang/reference#881)
- Rust by example (rust-lang/rust-by-example#1377)

## Tests
There are many tests, but I think one of the comperhensive ones:
- [borrowck-move-ref-pattern-pass.rs](https://github.com/Centril/rust/blob/85fbf49ce0e2274d0acf798f6e703747674feec3/src/test/ui/pattern/move-ref-patterns/borrowck-move-ref-pattern-pass.rs)
- [borrowck-move-ref-pattern.rs](https://github.com/Centril/rust/blob/85fbf49ce0e2274d0acf798f6e703747674feec3/src/test/ui/pattern/move-ref-patterns/borrowck-move-ref-pattern.rs)

# Examples

```rust
#[derive(PartialEq, Eq)]
struct Finished {}

#[derive(PartialEq, Eq)]
struct Processing {
    status: ProcessStatus,
}

#[derive(PartialEq, Eq)]
enum ProcessStatus {
    One,
    Two,
    Three,
}

#[derive(PartialEq, Eq)]
enum Status {
    Finished(Finished),
    Processing(Processing),
}

fn check_result(_url: &str) -> Status {
    // fetch status from some server
    Status::Processing(Processing {
        status: ProcessStatus::One,
    })
}

fn wait_for_result(url: &str) -> Finished {
    let mut previous_status = None;
    loop {
        match check_result(url) {
            Status::Finished(f) => return f,
            Status::Processing(p) => {
                match (&mut previous_status, p.status) {
                    (None, status) => previous_status = Some(status), // first status
                    (Some(previous), status) if *previous == status => {} // no change, ignore
                    (Some(previous), status) => { // Now it can be used
                        // new status
                        *previous = status;
                    }
                }
            }
        }
    }
}
```

Before, we would have used:
```rust
                match (&previous_status, p.status) {
                    (Some(previous), status) if *previous == status => {} // no change, ignore
                    (_, status) => {
                        // new status
                        previous_status = Some(status);
                    }
                }
```

Demonstrating *partial move*
```rust
fn main() {
    #[derive(Debug)]
    struct Person {
        name: String,
        age: u8,
    }

    let person = Person {
        name: String::from("Alice"),
        age: 20,
    };

    // `name` is moved out of person, but `age` is referenced
    let Person { name, ref age } = person;

    println!("The person's age is {}", age);

    println!("The person's name is {}", name);

    // Error! borrow of partially moved value: `person` partial move occurs
    //println!("The person struct is {:?}", person);

    // `person` cannot be used but `person.age` can be used as it is not moved
    println!("The person's age from person struct is {}", person.age);
}
```
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Oct 16, 2020
…n, r=nikomatsakis

Stabilize move_ref_pattern

# Implementation
- Initially the rule was added in the run-up to 1.0. The AST-based borrow checker was having difficulty correctly enforcing match expressions that combined ref and move bindings, and so it was decided to simplify forbid the combination out right.
- The move to MIR-based borrow checking made it possible to enforce the rules in a finer-grained level, but we kept the rule in place in an effort to be conservative in our changes.
- In rust-lang#68376, @Centril lifted the restriction but required a feature-gate.
- This PR removes the feature-gate.

Tracking issue: rust-lang#68354.

# Description
This PR is to stabilize the feature `move_ref_pattern`, which allows patterns
containing both `by-ref` and `by-move` bindings at the same time.

For example: `Foo(ref x, y)`, where `x` is `by-ref`,
and `y` is `by-move`.

The rules of moving a variable also apply here when moving *part* of a variable,
such as it can't be referenced or moved before.

If this pattern is used, it would result in *partial move*, which means that
part of the variable is moved. The variable that was partially moved from
cannot be used as a whole in this case, only the parts that are still
not moved can be used.

## Documentation
- The reference (rust-lang/reference#881)
- Rust by example (rust-lang/rust-by-example#1377)

## Tests
There are many tests, but I think one of the comperhensive ones:
- [borrowck-move-ref-pattern-pass.rs](https://github.com/Centril/rust/blob/85fbf49ce0e2274d0acf798f6e703747674feec3/src/test/ui/pattern/move-ref-patterns/borrowck-move-ref-pattern-pass.rs)
- [borrowck-move-ref-pattern.rs](https://github.com/Centril/rust/blob/85fbf49ce0e2274d0acf798f6e703747674feec3/src/test/ui/pattern/move-ref-patterns/borrowck-move-ref-pattern.rs)

# Examples

```rust
#[derive(PartialEq, Eq)]
struct Finished {}

#[derive(PartialEq, Eq)]
struct Processing {
    status: ProcessStatus,
}

#[derive(PartialEq, Eq)]
enum ProcessStatus {
    One,
    Two,
    Three,
}

#[derive(PartialEq, Eq)]
enum Status {
    Finished(Finished),
    Processing(Processing),
}

fn check_result(_url: &str) -> Status {
    // fetch status from some server
    Status::Processing(Processing {
        status: ProcessStatus::One,
    })
}

fn wait_for_result(url: &str) -> Finished {
    let mut previous_status = None;
    loop {
        match check_result(url) {
            Status::Finished(f) => return f,
            Status::Processing(p) => {
                match (&mut previous_status, p.status) {
                    (None, status) => previous_status = Some(status), // first status
                    (Some(previous), status) if *previous == status => {} // no change, ignore
                    (Some(previous), status) => { // Now it can be used
                        // new status
                        *previous = status;
                    }
                }
            }
        }
    }
}
```

Before, we would have used:
```rust
                match (&previous_status, p.status) {
                    (Some(previous), status) if *previous == status => {} // no change, ignore
                    (_, status) => {
                        // new status
                        previous_status = Some(status);
                    }
                }
```

Demonstrating *partial move*
```rust
fn main() {
    #[derive(Debug)]
    struct Person {
        name: String,
        age: u8,
    }

    let person = Person {
        name: String::from("Alice"),
        age: 20,
    };

    // `name` is moved out of person, but `age` is referenced
    let Person { name, ref age } = person;

    println!("The person's age is {}", age);

    println!("The person's name is {}", name);

    // Error! borrow of partially moved value: `person` partial move occurs
    //println!("The person struct is {:?}", person);

    // `person` cannot be used but `person.age` can be used as it is not moved
    println!("The person's age from person struct is {}", person.age);
}
```
@pacman82
Copy link
Contributor

pacman82 commented Feb 4, 2021

One thing that would be nice is seeing links to this feature being used "in the wild" (i.e., in the ecosystem), but maybe we don't know of any projects. (One could search for the feature gate...)

I have been using this feature in odbc2parquet without realizing it. Since the crate parquet is stable now, I'll probably try not to use it anymore until it is in the stable version of the compiler.

@memoryruins
Copy link
Contributor

@pacman82

I'll probably try not to use it anymore until it is in the stable version of the compiler.

No worries, it is stable now!

The feature was stabilized in #76119 and was released last month in stable rust 1.49 https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1490-2020-12-31 . It should be safe to close this tracking issue now ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) A-patterns Relating to patterns and pattern matching B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. F-move_ref_pattern `#![feature(move_ref_pattern)]` NLL-complete Working towards the "valid code works" goal T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests