Skip to content

Commit

Permalink
An attempt to fix NLL migration mode so that reports region errors wh…
Browse files Browse the repository at this point in the history
…en necessary.

Namely, the code here was trying to be clever, and say "lets not
report diagnostics when we 'know' NLL will report an error about them
in the future."

The problem is that in migration mode, when no error was reported here,
the NLL error that we "knew" was coming was downgraded to a warning (!).

This fixes that by only doing the "clever" skipping of region error reporting
when we are not in migration mode.

Rather than make a separate test for issue 53026, I just took the test
that uncovered this in a first place, and extended it (via our
revisions system) to explicitly show all three modes in action:
ACT-borrowck, NLL, and NLL migration mode.

(Tto be honest I hope not to have to add such revisions to many tests.
Instead I hope to adopt some sort of new `compare-mode` for either
borrowck=migrate or for the 2018 edition as a whole.)
  • Loading branch information
pnkfelix committed Aug 3, 2018
1 parent 88e0ff1 commit abd81c9
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 6 deletions.
9 changes: 8 additions & 1 deletion src/librustc/infer/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,14 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
) {
debug!("report_region_errors(): {} errors to start", errors.len());

if will_later_be_reported_by_nll && self.tcx.use_mir_borrowck() {
if will_later_be_reported_by_nll &&
// FIXME: `use_mir_borrowck` seems wrong here...
self.tcx.use_mir_borrowck() &&
// ... this is a band-aid; may be better to explicitly
// match on every borrowck_mode variant to guide decision
// here.
!self.tcx.migrate_borrowck() {

// With `#![feature(nll)]`, we want to present a nice user
// experience, so don't even mention the errors from the
// AST checker.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: borrowed data cannot be stored outside of its closure
--> $DIR/issue-45983.rs:17:27
--> $DIR/issue-45983.rs:36:27
|
LL | let x = None;
| - borrowed data cannot be stored into here...
Expand Down
12 changes: 12 additions & 0 deletions src/test/ui/borrowck/issue-45983.migrate.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error: borrowed data cannot be stored outside of its closure
--> $DIR/issue-45983.rs:36:27
|
LL | let x = None;
| - borrowed data cannot be stored into here...
LL | give_any(|y| x = Some(y));
| --- ^ cannot be stored outside of its closure
| |
| ...because it cannot outlive this closure

error: aborting due to previous error

6 changes: 3 additions & 3 deletions src/test/ui/borrowck/issue-45983.nll.stderr
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
warning: not reporting region error due to nll
--> $DIR/issue-45983.rs:17:27
--> $DIR/issue-45983.rs:36:27
|
LL | give_any(|y| x = Some(y));
| ^

error: borrowed data escapes outside of closure
--> $DIR/issue-45983.rs:17:18
--> $DIR/issue-45983.rs:36:18
|
LL | let x = None;
| - `x` is declared here, outside of the closure body
Expand All @@ -15,7 +15,7 @@ LL | give_any(|y| x = Some(y));
| `y` is a reference that is only valid in the closure body

error[E0594]: cannot assign to `x`, as it is not declared as mutable
--> $DIR/issue-45983.rs:17:18
--> $DIR/issue-45983.rs:36:18
|
LL | let x = None;
| - help: consider changing this to be mutable: `mut x`
Expand Down
25 changes: 24 additions & 1 deletion src/test/ui/borrowck/issue-45983.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,35 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// As documented in Issue #45983, this test is evaluating the quality
// of our diagnostics on erroneous code using higher-ranked closures.
//
// However, as documented on Issue #53026, this test also became a
// prime example of our need to test the NLL migration mode
// *separately* from the existing test suites that focus solely on
// AST-borrwock and NLL.

// revisions: ast migrate nll

// Since we are testing nll (and migration) explicitly as a separate
// revisions, dont worry about the --compare-mode=nll on this test.

// ignore-compare-mode-nll

//[ast]compile-flags: -Z borrowck=ast
//[migrate]compile-flags: -Z borrowck=migrate -Z two-phase-borrows
//[nll]compile-flags: -Z borrowck=mir -Z two-phase-borrows

fn give_any<F: for<'r> FnOnce(&'r ())>(f: F) {
f(&());
}

fn main() {
let x = None;
give_any(|y| x = Some(y));
//~^ ERROR borrowed data cannot be stored outside of its closure
//[ast]~^ ERROR borrowed data cannot be stored outside of its closure
//[migrate]~^^ ERROR borrowed data cannot be stored outside of its closure
//[nll]~^^^ WARN not reporting region error due to nll
//[nll]~| ERROR borrowed data escapes outside of closure
//[nll]~| ERROR cannot assign to `x`, as it is not declared as mutable
}

0 comments on commit abd81c9

Please sign in to comment.