Skip to content

/rust

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

Binding subpattern with @ allows for illegal mutation #14587

Closed
Marwes opened this Issue Jun 1, 2014 · 6 comments

Comments

Assignees

@nikomatsakis nikomatsakis

Labels
None yet
Projects
None yet
Milestone
  1.0
5 participants
@Marwes @alexcrichton @pnkfelix @nikomatsakis @sfackler
@Marwes
Copy link
Contributor

Marwes commented Jun 1, 2014 

fn main() {
    match &mut Some(1) {
        ref mut z @ &Some(ref a) => {
            **z = None;
            println!("{}", *a);
        }
        _ => ()
    }
}

This code compiles even though it should be rejected since writing to z invalidates the a variable.

@sfackler sfackler added the I-wrong label Jun 1, 2014

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Jun 2, 2014

That sounds quite bad, nominating.

@alexcrichton alexcrichton added the I-nominated label Jun 2, 2014

@pnkfelix

This comment has been minimized.

Sign in to view
Copy link
Member

pnkfelix commented Jun 5, 2014

P-backcompat-lang, 1.0.

@pnkfelix pnkfelix added the P-backcompat-lang label Jun 5, 2014

@pnkfelix pnkfelix added this to the 1.0 milestone Jun 5, 2014

@pnkfelix pnkfelix removed the I-nominated label Jun 5, 2014

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jun 5, 2014

cc me
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jun 5, 2014

Ah, I think I know why this happens. Fascinating. It's no doubt because the matched expression is an rvalue. I think we'll have to upgrade how rvalues are handled in borrowck to fix this.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Jun 5, 2014

cc #12534

@pcwalton pcwalton assigned nikomatsakis Jun 26, 2014

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jul 25, 2014

Basically the problem is that we treat rvalues as though they are only ever once -- but with a devious match expression like this one you can arrange to consume them / borrow them multiple times. I think we ought to modify the memcategorization to store the expr-id of the rvalue and not just the temporary scope -- the borrow checker can then treat rvalues just as it treats lvalues. Basically we want to have the borrow checker reason about the temporary that will be created.

@pcwalton pcwalton referenced this issue Jul 26, 2014

Closed

librustc: WIP patch for upgrading the way rvalues are treated. #16009

pcwalton added a commit to pcwalton/rust that referenced this issue Jul 28, 2014

@pcwalton
librustc: Forbid pattern bindings after `@`s, for memory safety. 
This is an alternative to upgrading the way rvalues are handled in the
borrow check. Making rvalues handled more like lvalues in the borrow
check caused numerous problems related to double mutable borrows and
rvalue scopes. Rather than come up with more borrow check rules to try
to solve these problems, I decided to just forbid pattern bindings after
`@`. This affected fewer than 10 lines of code in the compiler and
libraries.

This breaks code like:

    match x {
        y @ z => { ... }
    }

    match a {
        b @ Some(c) => { ... }
    }

Change this code to use nested `match` or `let` expressions. For
example:

    match x {
        y => {
            let z = y;
            ...
        }
    }

    match a {
        Some(c) => {
            let b = Some(c);
            ...
        }
    }

Closes rust-lang#14587.

[breaking-change]
0614e88

@pcwalton pcwalton referenced this issue Jul 28, 2014

Merged

librustc: Forbid pattern bindings after `@`s, for memory safety. #16053

pcwalton added a commit to pcwalton/rust that referenced this issue Jul 29, 2014

@pcwalton
librustc: Forbid pattern bindings after `@`s, for memory safety. 
This is an alternative to upgrading the way rvalues are handled in the
borrow check. Making rvalues handled more like lvalues in the borrow
check caused numerous problems related to double mutable borrows and
rvalue scopes. Rather than come up with more borrow check rules to try
to solve these problems, I decided to just forbid pattern bindings after
`@`. This affected fewer than 10 lines of code in the compiler and
libraries.

This breaks code like:

    match x {
        y @ z => { ... }
    }

    match a {
        b @ Some(c) => { ... }
    }

Change this code to use nested `match` or `let` expressions. For
example:

    match x {
        y => {
            let z = y;
            ...
        }
    }

    match a {
        Some(c) => {
            let b = Some(c);
            ...
        }
    }

Closes rust-lang#14587.

[breaking-change]
c9149df

bors added a commit that referenced this issue Aug 1, 2014

@bors
auto merge of #16053 : pcwalton/rust/at-pattern-bindings, r=pnkfelix 
This is an alternative to upgrading the way rvalues are handled in the
borrow check. Making rvalues handled more like lvalues in the borrow
check caused numerous problems related to double mutable borrows and
rvalue scopes. Rather than come up with more borrow check rules to try
to solve these problems, I decided to just forbid pattern bindings after
`@`. This affected fewer than 10 lines of code in the compiler and
libraries.

This breaks code like:

    match x {
        y @ z => { ... }
    }

    match a {
        b @ Some(c) => { ... }
    }

Change this code to use nested `match` or `let` expressions. For
example:

    match x {
        y => {
            let z = y;
            ...
        }
    }

    match a {
        Some(c) => {
            let b = Some(c);
            ...
        }
    }

Closes #14587.

[breaking-change]

May need discussion at the meeting, but r? @nikomatsakis anyway
875db47

pcwalton added a commit to pcwalton/rust that referenced this issue Aug 1, 2014

@pcwalton
librustc: Forbid pattern bindings after `@`s, for memory safety. 
This is an alternative to upgrading the way rvalues are handled in the
borrow check. Making rvalues handled more like lvalues in the borrow
check caused numerous problems related to double mutable borrows and
rvalue scopes. Rather than come up with more borrow check rules to try
to solve these problems, I decided to just forbid pattern bindings after
`@`. This affected fewer than 10 lines of code in the compiler and
libraries.

This breaks code like:

    match x {
        y @ z => { ... }
    }

    match a {
        b @ Some(c) => { ... }
    }

Change this code to use nested `match` or `let` expressions. For
example:

    match x {
        y => {
            let z = y;
            ...
        }
    }

    match a {
        Some(c) => {
            let b = Some(c);
            ...
        }
    }

Closes rust-lang#14587.

[breaking-change]
5b85c8c

bors added a commit that referenced this issue Aug 1, 2014

@bors
auto merge of #16053 : pcwalton/rust/at-pattern-bindings, r=pnkfelix 
This is an alternative to upgrading the way rvalues are handled in the
borrow check. Making rvalues handled more like lvalues in the borrow
check caused numerous problems related to double mutable borrows and
rvalue scopes. Rather than come up with more borrow check rules to try
to solve these problems, I decided to just forbid pattern bindings after
`@`. This affected fewer than 10 lines of code in the compiler and
libraries.

This breaks code like:

    match x {
        y @ z => { ... }
    }

    match a {
        b @ Some(c) => { ... }
    }

Change this code to use nested `match` or `let` expressions. For
example:

    match x {
        y => {
            let z = y;
            ...
        }
    }

    match a {
        Some(c) => {
            let b = Some(c);
            ...
        }
    }

Closes #14587.

[breaking-change]

May need discussion at the meeting, but r? @nikomatsakis anyway
Loading status checks…
f2fa559

@bors bors closed this in #16053 Aug 1, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.