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

Pattern-destructing a struct inside a ~-pointer violates linearity #3224

Closed
bblum opened this Issue Aug 20, 2012 · 24 comments

Comments

Projects
None yet
6 participants
@bblum
Contributor

bblum commented Aug 20, 2012

EDIT(bblum): modernized the example

extern mod extra;
use extra::arc;

struct Bar { x: arc::ARC<()> }

impl Drop for Bar {
    fn finalize(&self) {
        error!("%?", self.x.get());
    }   
}

struct Foo { x: Bar }

fn main() {
    let x = ~Foo { x: Bar { x: arc::ARC(()) } };
    let ~Foo { x: _y } = x;
}

This runs the destructor twice and crashes. It does not run the destructor twice if the ~ are removed.

Unlike #3218, this code should compile, because A is the one with the destructor.

@bblum

This comment has been minimized.

Show comment
Hide comment
@bblum

bblum Aug 20, 2012

Contributor

More minimal:

struct A { x: (); drop { error!("A"); } } 

fn main() {
    let bp = ~A { x: () };
    let ~_a <- bp; 
}
Contributor

bblum commented Aug 20, 2012

More minimal:

struct A { x: (); drop { error!("A"); } } 

fn main() {
    let bp = ~A { x: () };
    let ~_a <- bp; 
}
@bblum

This comment has been minimized.

Show comment
Hide comment
@bblum

bblum Aug 21, 2012

Contributor

related is the bicentordinally-older bug, #3024.

Contributor

bblum commented Aug 21, 2012

related is the bicentordinally-older bug, #3024.

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Oct 20, 2012

Contributor

Never mind my earlier comment; it was so wrong that I deleted it.

Contributor

catamorphism commented Oct 20, 2012

Never mind my earlier comment; it was so wrong that I deleted it.

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Oct 20, 2012

Contributor

There are two reasons this is happening:

  1. The parser treats patterns on the LHS of a let differently from match patterns. It considers let-LHS patterns irrefutable, and match patterns to be always refutable. The parser makes all bindings by-value if the pattern is refutable. So what this code is doing is copying the contents of the unique box even though the initialization is by-move.
  2. The kind checker isn't aware of this, and doesn't check that the RHS is copyable when the statement initializer is <-. That's okay when the pattern is just an ident or _, but not okay if the pattern contains any by-copy bindings.

If I recall correctly, we were going to get rid of <-, so that would be one solution. Another is for the kind checker to be more precise. A third solution is for the parser to treat let patterns the same way as match patterns, but I'm not totally sure what effect that would have. Obviously, pending changes to pattern semantics as described in #3271 affect all of this.

Contributor

catamorphism commented Oct 20, 2012

There are two reasons this is happening:

  1. The parser treats patterns on the LHS of a let differently from match patterns. It considers let-LHS patterns irrefutable, and match patterns to be always refutable. The parser makes all bindings by-value if the pattern is refutable. So what this code is doing is copying the contents of the unique box even though the initialization is by-move.
  2. The kind checker isn't aware of this, and doesn't check that the RHS is copyable when the statement initializer is <-. That's okay when the pattern is just an ident or _, but not okay if the pattern contains any by-copy bindings.

If I recall correctly, we were going to get rid of <-, so that would be one solution. Another is for the kind checker to be more precise. A third solution is for the parser to treat let patterns the same way as match patterns, but I'm not totally sure what effect that would have. Obviously, pending changes to pattern semantics as described in #3271 affect all of this.

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Oct 20, 2012

Contributor

Actually, fixing the code to make this work seems like more trouble than it's worth. I'm just going to remove <-, since that was the plan AFAIK.

Contributor

catamorphism commented Oct 20, 2012

Actually, fixing the code to make this work seems like more trouble than it's worth. I'm just going to remove <-, since that was the plan AFAIK.

@bblum

This comment has been minimized.

Show comment
Hide comment
@bblum

bblum Oct 20, 2012

Contributor

I kinda feel like it's important to be able to move out of unique boxes when deallocating them. The only alternative is the option dance, which changes your datatypes everywhere else too, uses extra memory, etc. One use case is in arc::unwrap -- internally it uses the option dance at present.

Contributor

bblum commented Oct 20, 2012

I kinda feel like it's important to be able to move out of unique boxes when deallocating them. The only alternative is the option dance, which changes your datatypes everywhere else too, uses extra memory, etc. One use case is in arc::unwrap -- internally it uses the option dance at present.

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Oct 20, 2012

Contributor

...I don't think any of the three possible solutions would prevent moving out of unique boxes when deallocating? Unless I'm missing something. (Getting rid of <-> just means you would write let ~a = move whatever instead of let ~a <- whatever.

Contributor

catamorphism commented Oct 20, 2012

...I don't think any of the three possible solutions would prevent moving out of unique boxes when deallocating? Unless I'm missing something. (Getting rid of <-> just means you would write let ~a = move whatever instead of let ~a <- whatever.

@bblum

This comment has been minimized.

Show comment
Hide comment
@bblum

bblum Oct 20, 2012

Contributor

Oh okay yeah. I've been thinking of <- foo and = move foo as the same thing. I guess they are not in the compiler. Getting rid of <- seems like the right thing to do no matter what.

Contributor

bblum commented Oct 20, 2012

Oh okay yeah. I've been thinking of <- foo and = move foo as the same thing. I guess they are not in the compiler. Getting rid of <- seems like the right thing to do no matter what.

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Oct 23, 2012

Contributor

Heh, re-reading the whole thing, I suspect getting rid of <- doesn't actually solve the problem, and that your original test case will still exhibit the wrong behavior. But I will see.

Contributor

catamorphism commented Oct 23, 2012

Heh, re-reading the whole thing, I suspect getting rid of <- doesn't actually solve the problem, and that your original test case will still exhibit the wrong behavior. But I will see.

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Oct 23, 2012

Contributor

Indeed, the following code still shows the bug:

struct A { x: &mut int, drop { *(self.x) -= 1; error!("A"); } } 
struct B { x: A }

fn main() {
    let mut y = 3;
    let bp = ~B { x: A { x: &mut y } };
    let ~B { x: _a } = move bp;
    assert(y == 2);
}

Fixing this will be part of #3271 -- the problem is right now, the binding of x in the pattern-match is a by-copy binding.

Contributor

catamorphism commented Oct 23, 2012

Indeed, the following code still shows the bug:

struct A { x: &mut int, drop { *(self.x) -= 1; error!("A"); } } 
struct B { x: A }

fn main() {
    let mut y = 3;
    let bp = ~B { x: A { x: &mut y } };
    let ~B { x: _a } = move bp;
    assert(y == 2);
}

Fixing this will be part of #3271 -- the problem is right now, the binding of x in the pattern-match is a by-copy binding.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Oct 25, 2012

Contributor

I thnk this has nothing to do with being a copy binding or not. If a struct has a destructor, you should not be able to move it into a destructuring pattern!

Contributor

nikomatsakis commented Oct 25, 2012

I thnk this has nothing to do with being a copy binding or not. If a struct has a destructor, you should not be able to move it into a destructuring pattern!

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Oct 25, 2012

Contributor

What's wrong with moving a struct with a destructor into a destructuring pattern if the pattern is a by-ref binding?

Contributor

catamorphism commented Oct 25, 2012

What's wrong with moving a struct with a destructor into a destructuring pattern if the pattern is a by-ref binding?

@bblum

This comment has been minimized.

Show comment
Hide comment
@bblum

bblum Oct 25, 2012

Contributor

Even if there's not a by-ref binding, it should still be legal as long as the struct itself isn't being taken apart.

The point is to write let ~t: T = move foo where foo : ~T -- in essence, moving the T from the heap to the stack, even if T is generic and possibly non-copyable.

Contributor

bblum commented Oct 25, 2012

Even if there's not a by-ref binding, it should still be legal as long as the struct itself isn't being taken apart.

The point is to write let ~t: T = move foo where foo : ~T -- in essence, moving the T from the heap to the stack, even if T is generic and possibly non-copyable.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Oct 25, 2012

Contributor

Ah, @bblum is correct, I was not being careful in my reading of the example. You should be able to move out of the ~, but you should not be able to take the struct apart. So many linearity bugs, who can keep them all straight?

I suppose @catamorphism that you are correct, we could allow you to move and then take a ref binding. This is sort of moving the value into a temporary, taking a ref into the temporary, and then letting the temp get cleaned up. Anyway, this gets to the matter of rvalue lifetimes, which I realize I should have also put on the 0.5 priorities. Bah.

Contributor

nikomatsakis commented Oct 25, 2012

Ah, @bblum is correct, I was not being careful in my reading of the example. You should be able to move out of the ~, but you should not be able to take the struct apart. So many linearity bugs, who can keep them all straight?

I suppose @catamorphism that you are correct, we could allow you to move and then take a ref binding. This is sort of moving the value into a temporary, taking a ref into the temporary, and then letting the temp get cleaned up. Anyway, this gets to the matter of rvalue lifetimes, which I realize I should have also put on the 0.5 priorities. Bah.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Mar 22, 2013

Member

I attempted to transcribe the example into modern syntax, but we have gotten rid of move entirely (I believe that was part of the "moves-based-on-type" work), and have also changed some constructs to require explicit lifetime annotations (#4846). When I transcribed catamorphism's last example above, this is what I got:

struct A { x: &'self mut int }
struct B { x: A<'self> }

impl Drop for A<'self> {
    fn finalize(&self) {
        *(self.x) -= 1; error!("A");
    }
}

fn main() {
    let mut y = 3;
    let bp = ~B { x: A { x: &mut y } };
    let ~B { x: _a } = bp;
    fail_unless!(y == 2);
}

Note that I am not certain that this faithfully captures the original intent of the test (it does indeed fail, and then it also calls the finalizer twice).

In any case, I am pretty sure this issue will not be resolved for 0.6 release.

Member

pnkfelix commented Mar 22, 2013

I attempted to transcribe the example into modern syntax, but we have gotten rid of move entirely (I believe that was part of the "moves-based-on-type" work), and have also changed some constructs to require explicit lifetime annotations (#4846). When I transcribed catamorphism's last example above, this is what I got:

struct A { x: &'self mut int }
struct B { x: A<'self> }

impl Drop for A<'self> {
    fn finalize(&self) {
        *(self.x) -= 1; error!("A");
    }
}

fn main() {
    let mut y = 3;
    let bp = ~B { x: A { x: &mut y } };
    let ~B { x: _a } = bp;
    fail_unless!(y == 2);
}

Note that I am not certain that this faithfully captures the original intent of the test (it does indeed fail, and then it also calls the finalizer twice).

In any case, I am pretty sure this issue will not be resolved for 0.6 release.

@ben0x539

This comment has been minimized.

Show comment
Hide comment
@ben0x539

ben0x539 Mar 24, 2013

Contributor

@lifthrasiir posted a snippet to #rust where an irrefutable &-pattern (originally in a closure parameter) caused a non-copyable value to be silently copied (... originally causing an SDL surface to be destroyed early). It reduces to something like

struct X { v: int }

impl Drop for X {
    fn finalize(&self) { io::println(fmt!("%?", ptr::to_uint(self))); }
}

fn main() {
    let x = X { v: 42 };
    let &y = &x;
}

which runs the finalizer twice with different output. Is that the same issue?

Note that with match &x { &y => () }; instead of the second let line, it errors with "by-move pattern bindings may not occur behind @ or & bindings" whereas x isn't moved here at all.

Contributor

ben0x539 commented Mar 24, 2013

@lifthrasiir posted a snippet to #rust where an irrefutable &-pattern (originally in a closure parameter) caused a non-copyable value to be silently copied (... originally causing an SDL surface to be destroyed early). It reduces to something like

struct X { v: int }

impl Drop for X {
    fn finalize(&self) { io::println(fmt!("%?", ptr::to_uint(self))); }
}

fn main() {
    let x = X { v: 42 };
    let &y = &x;
}

which runs the finalizer twice with different output. Is that the same issue?

Note that with match &x { &y => () }; instead of the second let line, it errors with "by-move pattern bindings may not occur behind @ or & bindings" whereas x isn't moved here at all.

@graydon

This comment has been minimized.

Show comment
Hide comment
@graydon

graydon Mar 28, 2013

Contributor

I think this is serious but on the same level as other linearity bugs -- not fixable in the 0.6 timeframe and not so user-facing or semantics-effecting that it requires attention above the others. De-milestoning.

Contributor

graydon commented Mar 28, 2013

I think this is serious but on the same level as other linearity bugs -- not fixable in the 0.6 timeframe and not so user-facing or semantics-effecting that it requires attention above the others. De-milestoning.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Apr 27, 2013

Member

@catamorphism and @nikomatsakis: I'm debating about nominating this for a maturity milestone. In particular, the fact that there has been:

  • discussion about what bug the code is actually exposing, and,
  • discussion about what one should and should not be able to do, and
  • large changes to the language itself that removed the original test case from the language

has made me wonder if this is a candidate for "Maturity Milestone 1: well defined", in the sense that we should specify the interactions of moves/linearity and finalization (that's milestone 1), and write a stable regression test for this example (which might more appropriately belong with milestone 2: backwards compatible).

But I thought I would bring the topic up here first, rather than waiting until the triage meeting and potentially bogging it down for more minutes than this issue warrants.

Member

pnkfelix commented Apr 27, 2013

@catamorphism and @nikomatsakis: I'm debating about nominating this for a maturity milestone. In particular, the fact that there has been:

  • discussion about what bug the code is actually exposing, and,
  • discussion about what one should and should not be able to do, and
  • large changes to the language itself that removed the original test case from the language

has made me wonder if this is a candidate for "Maturity Milestone 1: well defined", in the sense that we should specify the interactions of moves/linearity and finalization (that's milestone 1), and write a stable regression test for this example (which might more appropriately belong with milestone 2: backwards compatible).

But I thought I would bring the topic up here first, rather than waiting until the triage meeting and potentially bogging it down for more minutes than this issue warrants.

@catamorphism

This comment has been minimized.

Show comment
Hide comment
@catamorphism

catamorphism Apr 28, 2013

Contributor

@pnkfelix I think this belongs in milestone 1, too.

Contributor

catamorphism commented Apr 28, 2013

@pnkfelix I think this belongs in milestone 1, too.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 9, 2013

Contributor

I'll write up a post on how I think it ought to work.

Contributor

nikomatsakis commented May 9, 2013

I'll write up a post on how I think it ought to work.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 9, 2013

Contributor

(I don't really feel there is any doubt)

Contributor

nikomatsakis commented May 9, 2013

(I don't really feel there is any doubt)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 9, 2013

Contributor

In particular we already agreed to #4384 which is a generalization of the questions raised here.

Contributor

nikomatsakis commented May 9, 2013

In particular we already agreed to #4384 which is a generalization of the questions raised here.

@graydon

This comment has been minimized.

Show comment
Hide comment
@graydon

graydon Jun 20, 2013

Contributor

accepted for backwards-compatible milestone

Contributor

graydon commented Jun 20, 2013

accepted for backwards-compatible milestone

@graydon

This comment has been minimized.

Show comment
Hide comment
@graydon

graydon Jun 20, 2013

Contributor

sub-bug of #3235

Contributor

graydon commented Jun 20, 2013

sub-bug of #3235

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