MIR-borrowck: error diagnostics need notes

[pnkfelix]

The prototype MIR-borrowck from PR #43108 only emits errors, never any notes.

To achieve parity of error message quality with AST-borrowck, we need to add notes.

(In most cases, one can probably cross-reference the emitted error with the point where that error is emitted in the AST-borrowck, and then figure out how to construct the same note.)

Mentoring instructions here.

[pnkfelix]

Incidentally, someone taking this on may also want to consider incorporating some sort of fix here: nikomatsakis/nll-rfc#9

[zilbuz]

I might be interested in doing this. Since I'm new to rust, any pointer to where I should start ? In particular, in which files am I expected to make changes ? Thanks :)

[pnkfelix]

@zilbuz I would recommend you join us in https://gitter.im/rust-impl-period/WG-compiler-nll (assuming you have not already). We can give you real time guidance there.

But the quick answer to your immediate questions are:

1. I would expect to see changes, at least, to the code in https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check.rs
2. But the changes would be meant to correspond to notes emitted from https://github.com/rust-lang/rust/blob/master/src/librustc_borrowck/borrowck/check_loans.rs (search for calls to err.span_label, for example).
3. You can find test cases that illustrate the notes being emitted by looking at the unit tests in https://github.com/rust-lang/rust/tree/master/src/test/compile-fail/borrowck

[KiChjang]

I would also be looking into this issue.

[nikomatsakis]

I think there is room for multiple people to hack on this, but it'd be good to coordinate. Ideally @pnkfelix (or maybe I...) would go through the tests and try to make up a bullet list. I don't have time to do this just now, but perhaps if you are actively hacking on this, you could leave a note with which notes you are poking at?

Also, I'm not sure quite what @pnkfelix had in mind in terms of writing tests, but I think that for now, anyway, the best strategy is probably to clone the test from src/test/compile-fail/borrowck into src/test/compile-fail/mir-dataflow and add #[rustc_mir_borrowck] annotations. Or, actually, even better might be to create src/test/ui/mir-borrowck and put the test in there. We can then have two copies of each function, one with #[rustc_mir_borrowck] and one without. Due to the nature of UI tests, this will mean that the "expected output" shows us both the original and new output, making it easy to compare them. (Eventually we can remove the two variants, of course.)

[KiChjang]

So for this simple erroneous program here:

fn main() {
    let i: isize;

    println!("{}", false && { i = 5; true });
    println!("{}", i); //~ ERROR use of possibly uninitialized variable: `i`
}

rustc spits out a lot of junk errors:

error[E0381]: use of possibly uninitialized variable: `i`
  --> src/test/compile-fail/borrowck/borrowck-and-init.rs:17:20
   |
17 |     println!("{}", i); //~ ERROR use of possibly uninitialized variable: `i`
   |                    ^ use of possibly uninitialized `i`

error[E0381]: use of possibly uninitialized variable: `_` (Mir)
  --> src/test/compile-fail/borrowck/borrowck-and-init.rs:16:44
   |
16 |     println!("{}", false && { i = 5; true });
   |                                            ^

error[E0381]: use of possibly uninitialized variable: `i` (Mir)
  --> src/test/compile-fail/borrowck/borrowck-and-init.rs:17:20
   |
17 |     println!("{}", i); //~ ERROR use of possibly uninitialized variable: `i`
   |                    ^

error[E0381]: use of possibly uninitialized variable: `i` (Mir)
  --> src/test/compile-fail/borrowck/borrowck-and-init.rs:18:2
   |
18 | }
   |  ^

error: aborting due to 4 previous errors

I'm going to first try and fix it up so that it only points correctly to the uninitialized i variable.

[nikomatsakis]

@KiChjang just to repeat what I said in gitter for the record, it seems that the problem is that we are considering a StorageDead(X) to be a "use" of X

[pnkfelix]

Yeah that StorageDead issue should be resolved by #44736

[pnkfelix]

@KiChjang regarding the spurious errors: yes, those do need fixing too, though I regard that as an orthogonal task to the task described in this issue (which is solely about adding notes to the existing error message diagnostics).

I am planning to try to keep track of the work here in some manner (either github issues or a checklist on the Paper document associated with NLL). For now I have just put notes and links onto the Paper document itself.

[nikomatsakis]

I think this is no longer true... right? I feel like we should move further work on this error into actionable sub-issues. So just going to close.