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

[bblum]

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]

More minimal:

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

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

[bblum]

related is the bicentordinally-older bug, #3024.

[catamorphism]

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

[catamorphism]

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]

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]

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]

...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]

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]

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]

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]

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]

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

[bblum]

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]

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]

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]

@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]

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]

@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]

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

[nikomatsakis]

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

[nikomatsakis]

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

[nikomatsakis]

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

[graydon]

accepted for backwards-compatible milestone

[graydon]

sub-bug of #3235