Aliasing rules are broken for closures

[kvark]

This compiles and runs:

let mut v = vec![0i];
let f = |i: uint| v.get_mut(i);
let x = f(0);
let y = f(0);
(*x,*y)

x and y are not supposed to live at the same time. Results in an UB (in a more complex case)
Rust:

rustc 0.12.0-nightly (af3889f 2014-09-18 21:20:38 +0000)

[kvark]

Initially discovered in rust-voxel. CC @aepsil0n

[alexcrichton]

Nominating.

[thestinger]

Aren't the old closures going to be removed as part of implementing unboxed closures? Unless the problem exists for regular types / trait objects, I think it will solve itself.

[nikomatsakis]

cc me

[nikomatsakis]

I suspect this is related to how we handle lifetimes in closures, which is one of the areas where the code doesn't properly treat closures in the desugared way we'd like to. This is good because I've been looking for breakage that results from this, so I'm glad to have some concrete examples.

In particular I suspect the problem is that we currently consider the closure region bound to be its enclosing expression, which is patently wrong (#3696). There is a better algorithm for handling this described in a comment but I haven't gotten around to implementing it. I was waiting to look into this until the dust form @pcwalton's SEME region work settled, which seems to have happened. So now is the time, I guess. :)

[pnkfelix]

Assigning P-backcompat-lang, 1.0.

[zwarich]

Here is an example of the same issue happening with unboxed closures:

#![feature(unboxed_closures, overloaded_calls)]

fn main() {
    let mut x = 0u;
    let mut f = |&mut:| &mut x;
    let _y = f();
    let _z = f();
}

[zwarich]

The equivalent desugared program

#![feature(overloaded_calls)]

struct F<'a> {
    x: &'a mut uint
}

impl<'a> Fn<(), &'a mut uint> for F<'a> {
    #[rust_call_abi_hack]
    fn call(&self, _: ()) -> &'a mut uint {
        self.x
    }
}

fn main() {
    let mut x = 0u;
    let f = F { x: &mut x };
    let _y = f();
    let _z = f();
}

gives an error

test.rs:10:9: 10:15 error: cannot infer an appropriate lifetime for automatic coercion due to conflicting requirements
test.rs:10         self.x
                   ^~~~~~
test.rs:9:5: 11:6 note: consider using an explicit lifetime parameter as shown: fn call(&'a self, _: ()) -> &'a mut uint
test.rs:9     fn call(&self, _: ()) -> &'a mut uint {
test.rs:10         self.x
test.rs:11     }

This suggests a fix for the issue by appropriately constraining the lifetime parameters of borrows in unboxed closures so that the constraints have no solution in this case.

[bkoropoff]

My grasp of the region code is tenuous, but I think the interesting bit that causes the desugared version to be rejected is in link_reborrowed_region, where the mutability of F::x causes it to recurse and link regions higher up in the deref path to the region of the reborrow. In particular, this causes the region of self.x to become bounded by that of self, which creates a set of constraints with no solution. If F::x is changed to an immutable ref and the rest of the code updated accordingly, the extra link is not created and the code compiles, but of course this is perfectly sound.

It should be possible to make it handle the upvar in a similar way. I've been experimenting with a quick hack, and it seems to fix the simple unsound case above and pass unit tests, but it doesn't deal with the fact that the mutability of an upvar can be changed by later borrows or nested closures that have yet to be processed. It also produces really poor diagnostics.

Can anyone with deeper knowledge of regionck confirm if I'm on the right track here?

[nikomatsakis]

@bkoropoff I will try to take a deeper look tomorrow (I'm traveling and haven't had a chance to dig into this). However, your analysis seems possibly reasonable.

[bkoropoff]

This bug can now be closed