From abd81c9ff55aa50ee2a98d1f8a2b470acb81d4e6 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Sat, 4 Aug 2018 00:12:02 +0200 Subject: [PATCH] An attempt to fix NLL migration mode so that reports region errors when 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.) --- src/librustc/infer/error_reporting/mod.rs | 9 ++++++- ...ue-45983.stderr => issue-45983.ast.stderr} | 2 +- .../ui/borrowck/issue-45983.migrate.stderr | 12 +++++++++ src/test/ui/borrowck/issue-45983.nll.stderr | 6 ++--- src/test/ui/borrowck/issue-45983.rs | 25 ++++++++++++++++++- 5 files changed, 48 insertions(+), 6 deletions(-) rename src/test/ui/borrowck/{issue-45983.stderr => issue-45983.ast.stderr} (92%) create mode 100644 src/test/ui/borrowck/issue-45983.migrate.stderr diff --git a/src/librustc/infer/error_reporting/mod.rs b/src/librustc/infer/error_reporting/mod.rs index 90248e89fa73a..212821cac2e4a 100644 --- a/src/librustc/infer/error_reporting/mod.rs +++ b/src/librustc/infer/error_reporting/mod.rs @@ -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. diff --git a/src/test/ui/borrowck/issue-45983.stderr b/src/test/ui/borrowck/issue-45983.ast.stderr similarity index 92% rename from src/test/ui/borrowck/issue-45983.stderr rename to src/test/ui/borrowck/issue-45983.ast.stderr index 7625b9e76186a..db7cedffd8f72 100644 --- a/src/test/ui/borrowck/issue-45983.stderr +++ b/src/test/ui/borrowck/issue-45983.ast.stderr @@ -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... diff --git a/src/test/ui/borrowck/issue-45983.migrate.stderr b/src/test/ui/borrowck/issue-45983.migrate.stderr new file mode 100644 index 0000000000000..db7cedffd8f72 --- /dev/null +++ b/src/test/ui/borrowck/issue-45983.migrate.stderr @@ -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 + diff --git a/src/test/ui/borrowck/issue-45983.nll.stderr b/src/test/ui/borrowck/issue-45983.nll.stderr index 64086cb07917d..9d62c7dba75ff 100644 --- a/src/test/ui/borrowck/issue-45983.nll.stderr +++ b/src/test/ui/borrowck/issue-45983.nll.stderr @@ -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 @@ -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` diff --git a/src/test/ui/borrowck/issue-45983.rs b/src/test/ui/borrowck/issue-45983.rs index a6e5067195f47..bcbe0d1ffc000 100644 --- a/src/test/ui/borrowck/issue-45983.rs +++ b/src/test/ui/borrowck/issue-45983.rs @@ -8,6 +8,25 @@ // 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 FnOnce(&'r ())>(f: F) { f(&()); } @@ -15,5 +34,9 @@ fn give_any FnOnce(&'r ())>(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 }