Change check_loans to use ExprUseVisitor

[zwarich]

I tried to split up the less mechanical changes into separate commits so they are easier to review. One thing I'm not quite sure of is whether MoveReason should just be replaced with move_data::MoveKind.

[zwarich]

r? @nikomatsakis

[zwarich]

Maybe it would make more sense for the mutate() callback to ignore all mutations with a MutateMode of Init than just ignoring them for function arguments? All tests pass that way and it seems correct.

[zwarich]

Going over the patch again, I realize that the changed results in src/test/compile-fail/borrowck-use-in-index-lvalue.rs indicate a real problem. I have a fix, which I will push after verifying that a full make check still passes.

[zwarich]

I pushed my changes. The only difference is in the mutate implementation.

[huonw]

(Needs a rebase.)

[zwarich]

Rebased to account for the string refactoring.

[nikomatsakis]

@zwarich I added some comments, a few things concern me, can you let me know what you think?

[zwarich]

I replied to your comments, although I can't seem to find them outside of my email inbox since they were on a pre-rebase version of my changes (is this just some GitHub UI issue?). The most concerning issue (the new incorrect test result) was fixed in a newer version of the changes, and I left questions for you about the others. Let me know what you think and I'll update my changes again.

[zwarich]

@nikomatsakis I pushed some updated commits to my branch. I changed some things in response to your previous comments:

• I renamed the variants of MoveReason to DirectRefMove, PatBindingMove, and CaptureMove.
• I changed check_loans to not use MoveReason at all; it is now able to determine whether moves are captures purely from the move data itself. This also simplifies the code.
• In the implementation of mutate I check whether a path contains any derefs to determine whether the JustWrite case is safe without considering moves. Your suggestion to use the superpath didn't work because of the check for interfering paths in FlowedMoveData's each_move_of. As you can see from the new test cases I added, this check combined with switching to ExprUseVisitor enables a lot of field-sensitivity. The only case remaining is realizing that assigning to a field after a move of the aggregate invalidates the move of the field.
• I renamed the equivalent of check_move_out_from_id to check_for_move_of_borrowed_value, because that name makes a lot more sense.
• I added a lot more tests, both failing and passing, including moving all of the now-passing cases from compile-fail to a new test in run-pass.

[zwarich]

I made two small changes to my branch. I noticed that all of the code in consume and consume_pat was basically identical, and moved it into consume_common. I also renamed check_for_move_of_borrowed_value to check_for_move_of_borrowed_path, since that name makes a bit more sense in this context.

[nikomatsakis]

I am surprised that we can drop *y and *z, as that is borrowed content. Is this simply an artifact of us not reporting an error for some other reason?

[zwarich]

It's fine because *y and *z are integers, and they seem to get copied rather than moved. This code compiles fine:

fn foo() {
    let mut x = A { a: 1, b: box 2 };
    let y = &mut x.a;
    drop(*y);
}

but if I use the x.b field, which is a Box<int>, like this:

fn bar() {
    let mut x = A { a: 1, b: box 2 };
    let y = &mut x.b;
    drop(*y);
}

then I get an error:

test.rs:24:10: 24:12 error: cannot move out of dereference of `&mut`-pointer
test.rs:24     drop(*y);
                    ^~

[nikomatsakis]

On Wed, May 28, 2014 at 12:46:04PM -0700, Cameron Zwarich wrote:

It's fine because *y and *z are integers, and they seem to get copied rather than moved.

Indeed. I overlooked that.