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

Initial implementation of #![feature(move_ref_pattern)] #68376

Merged
merged 6 commits into from
Feb 9, 2020

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Jan 19, 2020

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 F-bindings_after_at `#![feature(bindings_after_at)]` F-move_ref_pattern `#![feature(move_ref_pattern)]` labels Jan 19, 2020
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 19, 2020
@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@estebank
Copy link
Contributor

CC #30418

@Centril
Copy link
Contributor Author

Centril commented Jan 26, 2020

@Centril Centril 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 Feb 2, 2020
@Centril Centril 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 Feb 2, 2020
@Centril
Copy link
Contributor Author

Centril commented Feb 2, 2020

Addressed the comments. :)

@Centril
Copy link
Contributor Author

Centril commented Feb 2, 2020

Also, do you think the test suite is sufficient for eventual stabilization purposes or do you think more would be necessary?

@matthewjasper
Copy link
Contributor

I can't think of any other tests that I would want.
@bors r+

@bors
Copy link
Contributor

bors commented Feb 8, 2020

📌 Commit d2b88b7 has been approved by matthewjasper

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2020
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 8, 2020
@bors
Copy link
Contributor

bors commented Feb 9, 2020

⌛ Testing commit d2b88b7 with merge f8d830b...

bors added a commit that referenced this pull request 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
@bors
Copy link
Contributor

bors commented Feb 9, 2020

☀️ Test successful - checks-azure
Approved by: matthewjasper
Pushing f8d830b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 9, 2020
@bors bors merged commit d2b88b7 into rust-lang:master Feb 9, 2020
@Centril Centril deleted the move-ref-patterns branch February 9, 2020 07:31
@Centril Centril added A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns A-patterns Relating to patterns and pattern matching labels Apr 10, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request 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 pull request 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 pull request 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);
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns A-patterns Relating to patterns and pattern matching F-bindings_after_at `#![feature(bindings_after_at)]` F-move_ref_pattern `#![feature(move_ref_pattern)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pattern errors are too imprecise and should be removed in favor of MIR borrowck
5 participants