Last use doesn't consider closure bodies properly

[nikomatsakis]

The following test case will crash (once fixed, something similar should be added to the test suite):

fn invoke_several_times(f: lambda()) {
    f();
    f();
    f();
}

fn bycopy<K>(+k: K) { log(error,k); }
fn byref<K>(&&k: K) { log(error,k); }

fn testfn(+k: ~int) {
    bycopy(k); // <-- gets incorrectly optimized into a move
    invoke_several_times {||
        byref(k);
    }
    // bycopy(k); <-- (this would be a last use)
}

fn main() {
    testfn(~22);
}

The reason is that the bycopy(k) is considered a last use of k as uses via upvar are not considered by the last use analysis. I think we should probably just disable any 'last use' reasoning about variables that escape into a closure of any kind. After all, the closure could be stored and invoked any number of times, so it's hard to reason about what a last use is in that context.

The only place we could try to get smart is around blocks (i.e., closures with proto block), as they cannot escape and so can be tracked more precisely: in the example above, we know that the block passed to invoke_several_times() won't leak, so a final call to bycopy(k) could safely be considered a last use (as indicated in the example).

As an aside, there is some code in lastuse.rs that seems to try to consider blocks and closures carefully, but so I could not quite figure out what constraints it was trying to enforce. It didn't seem right to me, but I might be missing something. The code analyzes blocks twice, presumably to account for the possibility of the block being invoked multiple times: but seeing as upvars are ignored, I don't think this serves its intended purpose. In addition, it may harm the precision of the analysis for non-upvars within the block? There is also some special reasoning that delays the visiting of closure arguments to a function until after the rest of the function has been analyzed, though I'm not quite sure why. Perhaps to simulate the possible execution order? Removing this special case made no difference in any of my tests.

[marijnh]

I think we should probably just disable any 'last use' reasoning about variables that escape into a closure of any kind. After all, the closure could be stored and invoked any number of times, so it's hard to reason about what a last use is in that context.

Unless your changes were more radical than I understood them, only copying closures can be stored anywhere. A 'block' closure an only be invoked (0-N times) by the call that it is passed to, and behaves much like a loop.

The bug you see probably results from the block being classified incorrectly. I'll debug.

[nikomatsakis]

Yes, of course, I forgot that lambdas etc copy the variables at the point of creation, so it's a moot point for those sorts of closures.