MIR-borrowck: Adding notes to E0506 by zilbuz · Pull Request #44811 · rust-lang/rust

zilbuz · 2017-09-24T14:49:57Z

This PR adds notes to the MIR borrowck error E0506.

rust-highfive · 2017-09-24T14:50:03Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

GuillaumeGomez · 2017-09-24T16:12:17Z

Could you please add a test for this change? Take a look at how they're done in src/test/compile-fail.

zilbuz · 2017-09-24T17:59:45Z

There is a test for E0506 (https://github.com/rust-lang/rust/blob/master/src/test/compile-fail/E0506.rs), but it doesn't test this message because the compiler uses the AST borrowck instead of the MIR borrowck. I manually checked that the messages were the same though. I assumed that the tests for MIR borrowck would be added on a later PR (@pnkfelix ?). Or maybe you're asking for a different kind of test ?

KiChjang · 2017-09-24T18:56:28Z

We could actually use a UI test for this, but it's not feasible right now because we can't properly display lvalues in the MIR borrowck.

pnkfelix · 2017-09-25T13:11:03Z

In principle we could add a test, by telling the test infrastructure to pass the necessary -Z flags.

In particular, you can do // compile-flags: -Z emit-end-regions -Z borrowck-mir at the top of the file. (You can find other tests that pass other such flags in src/test/compile-fail/)

(I assume that the UI tests accept the same compile-flags comment.)

Whether its a good idea to put in tests for MIR-borrowck at this stage... it probably is worth it, just to make sure we don't regress. But maybe they should go into their own sub-directory or something, just to keep them isolated from "real tests" of current functionality.

pnkfelix · 2017-09-25T14:03:24Z

(After discussion in rust-impl-period/WG-compiler-nll, we decided that we will try the approach of extending the existing tests under compile-fail/ via the // revisions: ... system so that we can keep a single test file and list different sets of expected error messages based on whether or not we are passing -Z borrowck-mir.)

bors · 2017-09-26T14:29:54Z

☔ The latest upstream changes (presumably #44736) made this pull request unmergeable. Please resolve the merge conflicts.

…f_borrowed()`

…n MIR borrowck

zilbuz · 2017-09-26T20:03:06Z

Alright I edited the tests on compile-fail that raise E0506. I omitted the tests that triggers ICE as other issues are supposed to fix them.

arielb1 · 2017-09-27T11:21:20Z

Nice. I suppose we'll also want to add "borrow ends here" when we start supporting these, which should be soon.

@bors r+

bors · 2017-09-27T11:21:21Z

📌 Commit b683538 has been approved by arielb1

arielb1 · 2017-09-27T11:23:15Z

 }

- fn report_illegal_mutation_of_borrowed(&mut self, _: Context, (lvalue, span): (&Lvalue, Span)) {
+ fn report_illegal_mutation_of_borrowed(&mut self,


I think this should be moved to borrowck_errors actually, but that could be done in another commit.

You mean like @mikhail-m1 has done in #44882 ?

Done here: #44922

pnkfelix · 2017-09-27T13:26:37Z

 borrow2(x, x);
- //~^ ERROR cannot borrow `*x` as immutable because it is also borrowed as mutable
+ //[ast]~^ ERROR cannot borrow `*x` as immutable because it is also borrowed as mutable
+ //[mir]~^^ ERROR cannot borrow `*x` as immutable because it is also borrowed as mutable (Ast)
