Liveness, borrowck and last use interact badly

[catamorphism]

This arose from #2584. This code snippet from trans::base::trans_assign_op illustrates the problem:

        ret move_val(bcx, DROP_EXISTING, lhs_res.val,
                     // FIXME: should kind be owned?
                     {bcx: bcx, val: target, kind: owned},
                     dty);

bcx has type block, which is now a newtype-like enum whose LHS is an @ of a class. The bug is exposed by the fact that block doesn't get treated as immediate even though its representation is a box, but that's not the main bug. The main problem is that there's an implicit move because of the second use of bcx as a field in the record literal: it's a last use, implying that bcx gets zeroed at that point. Normally, borrowck would flag a move of something that has a reference to it (the reference gets brought about by the first instance of bcx, as the first argument to move_val). But since the implicit moves due to last-use information aren't visible to borrowck, the code happily compiles, runs, and then segfaults when move_val dereferences its first argument.

@nikomatsakis and I discussed this a bit on IRC and there's not a clear solution (though there is an obvious workaround for #2584). Perhaps the answer is to just remove last-use analysis.

[nikomatsakis]

Here is a targeted test case:

fn a_val(&&x: ~int, +y: ~int) -> int {
    *x + *y
}

fn main() {
    let z = ~22;
    a_val(z, z);
}

[nikomatsakis]

I see two possible solutions. One is to have borrowck remove the entries from last_use. This sounds both complex and a bit fragile. The other is to remove last_use and ask users to manually annotate moves.

It is a bit unclear to me what will be the impact of removing last_use on usability. On the one hand, it's a nice way to avoid excess use of the unary move operator. On the other hand, it's a common source of confusion ("why am I getting a copy warning here but not there? oh, I see, that's a last-use"). Also, when you add borrowck into the equation, it's one more thing that users have to think about ("it's not a last use because there is an existing alias in scope"). On the other hand, people will probably end up understanding the idea of "alias in scope" anyway.

[catamorphism]

A standalone test case for this is under run-pass/issue-2633.rs (currently xfailed).

[nikomatsakis]

Another example making use of by-value:

fn a_val(++x: @int, +y: @int) -> int {
    free(y);
    #debug["deref"];
    *x
}

fn free(-x: @int) {}

fn main() {
    let z = @22;
    assert 22 == a_val(z, z);
}

[pcwalton]

I'm in favor of getting rid of the last use analysis. Programmers should not have to run a liveness pass in their heads to figure out where the moves are.

A possibility here is to re-purpose the last use analysis into a fixit for the no-implicit-copies warning. That is, you might get warnings like so if liveness detects that the variable in question is dead:

foo.rs:20: warning: copying a value of type '~str' requires memory allocation
foo.rs:20: note: suggest use of 'move' to avoid the copy

[catamorphism]

I like the idea of using last-use as a source of data for improving error messages, but not for guiding code generation at all.

[nikomatsakis]

I think I like @pcwalton's idea as well. However, that seems like a decision that should be brought up at the Tuesday meeting. For the very short term, it's probably easy enough to have borrowck remove unsafe entries from the last_use table. I'm not thrilled about the idea because we tend to make tables that are built-up by a pass and then read-only thereafter, which seems like reasonable practice.

[catamorphism]

At the meeting today, we agreed to adopt @pcwalton 's idea to make last-use solely a hint source for error messages.

[graydon]

I'm not sure if a fixit justifies a full pass; could just as easily amend the no-implicit-copies message to say "(try a move or copy?)"

In any case, yeah, I would like last-use to not have semantic weight. It was suspiciously non-obvious when we added it and I think it's proven no more obvious after regular use. I like explicit signals from the user in these contexts.

[catamorphism]

I'm going to start making moves explicit in preparation for making last-use semantically non-meaningful.

[catamorphism]

I'm going to take this over, but it's blocked on #3413

[graydon]

Punting to 0.5; last-use should go but it seems like it'll take a little while to stabilize when it happens.

[nikomatsakis]

Deferring to 0.5

[catamorphism]

I have a patch for this (at least, removing any code that looks at the results of last-use, and making all moves explicit), but sadly it's a huge performance hit. Apparently there is a difference between how explicit moves are compiled, and how implicit (last-use) moves are compiled: even if I change the code so that trans still pays attention to the results of last-use, but the kind checker requires explicit moves on last uses, performance still gets much worse (for example, the typechecking pass takes 18s in stage 1, as opposed to 5s in stage 0).

So I'm not checking anything in yet :-(

[graydon]

Can you push to a private branch? I'm happy to perf diff it to find the culprit.

[catamorphism]

@graydon Oops, sorry, missed your comment. I will push to my fork once I get a chance to rebase it.

[catamorphism]

c6780fb closes this. I opened #3755 to cover the remaining part (about hints in error messages).