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

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

Merged
merged 1 commit into from Aug 1, 2014

Conversation

Projects
None yet
6 participants
@pcwalton
Copy link
Contributor

pcwalton commented Jul 28, 2014

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

@pcwalton

This comment has been minimized.

Copy link
Contributor

pcwalton commented Jul 29, 2014

Rebased.

@pcwalton

This comment has been minimized.

Copy link
Contributor

pcwalton commented Jul 30, 2014

@nikomatsakis I'm happy to punt to @pnkfelix if you'd like.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Aug 1, 2014

(reviewing now)

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Aug 1, 2014

This seems like a pretty severe weakening of @-patterns; so I just want to point out to on-lookers that the decision at the weekly meeting was to put this change in for now, with the expectation/hope that we'll fix things and re-enable bindings in the rhs of @ at some later point.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Aug 1, 2014

@nikomatsakis do you think we should be deleting the tests as the current PR does, or mark them as ignored? (I'm assuming that the ignore flag should suffice, its not like those tests will stop parsing due to this change...)

I guess as long as the hypothetical re-implementor of @-pattern with rhs-binding support does their due-diligence, they should know to re-enable the tests.

bors added a commit that referenced this pull request Aug 1, 2014

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
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 #14587.

[breaking-change]
@pcwalton

This comment has been minimized.

Copy link
Owner

pcwalton commented on 5b85c8c Aug 1, 2014

r=pnkfelix

@bors

This comment has been minimized.

Copy link
Contributor

bors commented on 5b85c8c Aug 1, 2014

saw approval from pnkfelix
at pcwalton@5b85c8c

This comment has been minimized.

Copy link
Contributor

bors replied Aug 1, 2014

merging pcwalton/rust/at-pattern-bindings = 5b85c8c into auto

This comment has been minimized.

Copy link
Contributor

bors replied Aug 1, 2014

pcwalton/rust/at-pattern-bindings = 5b85c8c merged ok, testing candidate = f2fa559

This comment has been minimized.

Copy link
Contributor

bors replied Aug 1, 2014

fast-forwarding master to auto = f2fa559

bors added a commit that referenced this pull request Aug 1, 2014

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

@bors bors closed this Aug 1, 2014

@bors bors merged commit 5b85c8c into rust-lang:master Aug 1, 2014

1 of 2 checks passed

continuous-integration/travis-ci The Travis CI build failed
Details
default all tests passed

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Sep 18, 2014

Distinguish move semantics for `a @ Var(_)` from that of `Var(_)`.
Note that due to PR rust-lang#16053, we only actually deal with `a @ Var(_)`
here, (as opposed to a hypothetical `a @ Var(ref b)`, which cannot
currenty arise, and maybe will never be supported for non-copy data).

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Sep 18, 2014

Distinguish move semantics for `a @ Var(_)` from that of `Var(_)`.
Note that due to PR rust-lang#16053, we only actually deal with `a @ Var(_)`
here, (as opposed to a hypothetical `a @ Var(ref b)`, which cannot
currenty arise, and maybe will never be supported for non-copy data).

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Sep 22, 2014

Distinguish move semantics for `a @ Var(_)` from that of `Var(_)`.
Note that due to PR rust-lang#16053, we only actually deal with `a @ Var(_)`
here, (as opposed to a hypothetical `a @ Var(ref b)`, which cannot
currenty arise, and maybe will never be supported for non-copy data).

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Oct 8, 2014

Distinguish move semantics for `a @ Var(_)` from that of `Var(_)`.
Note that due to PR rust-lang#16053, we only actually deal with `a @ Var(_)`
here, (as opposed to a hypothetical `a @ Var(ref b)`, which cannot
currenty arise, and maybe will never be supported for non-copy data).

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Oct 14, 2014

Distinguish move semantics for `a @ Var(_)` from that of `Var(_)`.
Note that due to PR rust-lang#16053, we only actually deal with `a @ Var(_)`
here, (as opposed to a hypothetical `a @ Var(ref b)`, which cannot
currenty arise, and maybe will never be supported for non-copy data).

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Oct 21, 2014

Distinguish move semantics for `a @ Var(_)` from that of `Var(_)`.
Note that due to PR rust-lang#16053, we only actually deal with `a @ Var(_)`
here, (as opposed to a hypothetical `a @ Var(ref b)`, which cannot
currenty arise, and maybe will never be supported for non-copy data).

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Oct 21, 2014

Distinguish move semantics for `a @ Var(_)` from that of `Var(_)`.
Note that due to PR rust-lang#16053, we only actually deal with `a @ Var(_)`
here, (as opposed to a hypothetical `a @ Var(ref b)`, which cannot
currenty arise, and maybe will never be supported for non-copy data).

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Oct 22, 2014

Distinguish move semantics for `a @ Var(_)` from that of `Var(_)`.
Note that due to PR rust-lang#16053, we only actually deal with `a @ Var(_)`
here, (as opposed to a hypothetical `a @ Var(ref b)`, which cannot
currenty arise, and maybe will never be supported for non-copy data).

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Oct 22, 2014

Distinguish move semantics for `a @ Var(_)` from that of `Var(_)`.
Note that due to PR rust-lang#16053, we only actually deal with `a @ Var(_)`
here, (as opposed to a hypothetical `a @ Var(ref b)`, which cannot
currenty arise, and maybe will never be supported for non-copy data).

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Nov 20, 2014

Distinguish move semantics for `a @ Var(_)` from that of `Var(_)`.
Note that due to PR rust-lang#16053, we only actually deal with `a @ Var(_)`
here, (as opposed to a hypothetical `a @ Var(ref b)`, which cannot
currenty arise, and maybe will never be supported for non-copy data).

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Nov 21, 2014

Distinguish move semantics for `a @ Var(_)` from that of `Var(_)`.
Note that due to PR rust-lang#16053, we only actually deal with `a @ Var(_)`
here, (as opposed to a hypothetical `a @ Var(ref b)`, which cannot
currenty arise, and maybe will never be supported for non-copy data).
@chpio

This comment has been minimized.

Copy link
Contributor

chpio commented Jan 2, 2017

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);
       ...
   }
}

ok, but how to fix this:

match a {
  Some(b @ &MyEnum::Foo(ref c)) => {
  }
}

Foo can only take values, no refs. So this wouldn't work:

match a {
  Some(&MyEnum::Foo(ref c)) => {
    let b = MyEnum::Foo(c);
    // mismatched types
    // expected type `Bar`
    //    found type `&Bar`
  }
}

i can think only of this workaround:

match a {
  Some(b @ MyEnum::Foo(..)) => {
    let c = match b {
      &MyEnum::Foo(ref c) => c,
      _ => unreachable!(),
    };
  }
}
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 3, 2017

i can think only of this workaround:

Yeah, that seems about right. It'd be nice to fix this in a better way (internal refactorings to use MIR ought to help here).

@ozkriff

This comment has been minimized.

Copy link

ozkriff commented Dec 20, 2018

Are there any plans to implement a correct handling of these bindings for Copy types?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment