From 517d361a1f78cf13d589d0f6b94f5ca005bef540 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 27 Jun 2020 13:54:56 -0400 Subject: [PATCH] Use an 'approximate' universal upper bound when reporting region errors Fixes #67765 When reporting errors during MIR region inference, we sometimes use `universal_upper_bound` to obtain a named universal region that we can display to the user. However, this is not always possible - in a case like `fn foo<'a, 'b>() { .. }`, the only upper bound for a region containing `'a` and `'b` is `'static`. When displaying diagnostics, it's usually better to display *some* named region (even if there are multiple involved) rather than fall back to a generic error involving `'static`. This commit adds a new `approx_universal_upper_bound` method, which uses the lowest-numbered universal region if the only alternative is to return `'static`. --- .../borrow_check/diagnostics/region_errors.rs | 4 ++- .../borrow_check/region_infer/mod.rs | 34 +++++++++++++++++++ .../borrow_check/region_infer/opaque_types.rs | 3 +- .../issue-67765-async-diagnostic.rs | 16 +++++++++ .../issue-67765-async-diagnostic.stderr | 12 +++++++ ...ure-doesnt-life-long-enough-issue-67634.rs | 2 +- ...doesnt-life-long-enough-issue-67634.stderr | 24 ++++++++----- src/test/ui/return-disjoint-regions.rs | 7 ++++ src/test/ui/return-disjoint-regions.stderr | 11 ++++++ 9 files changed, 101 insertions(+), 12 deletions(-) create mode 100644 src/test/ui/async-await/issue-67765-async-diagnostic.rs create mode 100644 src/test/ui/async-await/issue-67765-async-diagnostic.stderr create mode 100644 src/test/ui/return-disjoint-regions.rs create mode 100644 src/test/ui/return-disjoint-regions.stderr diff --git a/src/librustc_mir/borrow_check/diagnostics/region_errors.rs b/src/librustc_mir/borrow_check/diagnostics/region_errors.rs index f1923b9e81c..fa4846a20cb 100644 --- a/src/librustc_mir/borrow_check/diagnostics/region_errors.rs +++ b/src/librustc_mir/borrow_check/diagnostics/region_errors.rs @@ -122,7 +122,9 @@ pub(super) fn to_error_region_vid(&self, r: RegionVid) -> Option { if self.regioncx.universal_regions().is_universal_region(r) { Some(r) } else { - let upper_bound = self.regioncx.universal_upper_bound(r); + // We just want something nameable, even if it's not + // actually an upper bound. + let upper_bound = self.regioncx.approx_universal_upper_bound(r); if self.regioncx.upper_bound_in_region_scc(r, upper_bound) { self.to_error_region_vid(upper_bound) diff --git a/src/librustc_mir/borrow_check/region_infer/mod.rs b/src/librustc_mir/borrow_check/region_infer/mod.rs index 3e459bd52f7..081125cb625 100644 --- a/src/librustc_mir/borrow_check/region_infer/mod.rs +++ b/src/librustc_mir/borrow_check/region_infer/mod.rs @@ -1114,6 +1114,40 @@ pub(in crate::borrow_check) fn universal_upper_bound(&self, r: RegionVid) -> Reg lub } + /// Like `universal_upper_bound`, but returns an approximation more suitable + /// for diagnostics. If `r` contains multiple disjoint universal regions + /// (e.g. 'a and 'b in `fn foo<'a, 'b> { ... }`, we pick the lower-numbered region. + /// This corresponds to picking named regions over unnamed regions + /// (e.g. picking early-bound regions over a closure late-bound region). + /// + /// This means that the returned value may not be a true upper bound, since + /// only 'static is known to outlive disjoint universal regions. + /// Therefore, this method should only be used in diagnostic code, + /// where displaying *some* named universal region is better than + /// falling back to 'static. + pub(in crate::borrow_check) fn approx_universal_upper_bound(&self, r: RegionVid) -> RegionVid { + debug!("approx_universal_upper_bound(r={:?}={})", r, self.region_value_str(r)); + + // Find the smallest universal region that contains all other + // universal regions within `region`. + let mut lub = self.universal_regions.fr_fn_body; + let r_scc = self.constraint_sccs.scc(r); + let static_r = self.universal_regions.fr_static; + for ur in self.scc_values.universal_regions_outlived_by(r_scc) { + let new_lub = self.universal_region_relations.postdom_upper_bound(lub, ur); + debug!("approx_universal_upper_bound: ur={:?} lub={:?} new_lub={:?}", ur, lub, new_lub); + if ur != static_r && lub != static_r && new_lub == static_r { + lub = std::cmp::min(ur, lub); + } else { + lub = new_lub; + } + } + + debug!("approx_universal_upper_bound: r={:?} lub={:?}", r, lub); + + lub + } + /// Tests if `test` is true when applied to `lower_bound` at /// `point`. fn eval_verify_bound( diff --git a/src/librustc_mir/borrow_check/region_infer/opaque_types.rs b/src/librustc_mir/borrow_check/region_infer/opaque_types.rs index 7e352bfba77..325dca8c8ca 100644 --- a/src/librustc_mir/borrow_check/region_infer/opaque_types.rs +++ b/src/librustc_mir/borrow_check/region_infer/opaque_types.rs @@ -141,7 +141,8 @@ pub(in crate::borrow_check) fn name_regions(&self, tcx: TyCtxt<'tcx>, ty: T) { tcx.fold_regions(&ty, &mut false, |region, _| match *region { ty::ReVar(vid) => { - let upper_bound = self.universal_upper_bound(vid); + // Find something that we can name + let upper_bound = self.approx_universal_upper_bound(vid); self.definitions[upper_bound].external_name.unwrap_or(region) } _ => region, diff --git a/src/test/ui/async-await/issue-67765-async-diagnostic.rs b/src/test/ui/async-await/issue-67765-async-diagnostic.rs new file mode 100644 index 00000000000..5093916e73a --- /dev/null +++ b/src/test/ui/async-await/issue-67765-async-diagnostic.rs @@ -0,0 +1,16 @@ +// edition:2018 +// +// Regression test for issue #67765 +// Tests that we point at the proper location when giving +// a lifetime error. +fn main() {} + +async fn func<'a>() -> Result<(), &'a str> { + let s = String::new(); + + let b = &s[..]; + + Err(b)?; //~ ERROR cannot return value referencing local variable `s` + + Ok(()) +} diff --git a/src/test/ui/async-await/issue-67765-async-diagnostic.stderr b/src/test/ui/async-await/issue-67765-async-diagnostic.stderr new file mode 100644 index 00000000000..78253042bee --- /dev/null +++ b/src/test/ui/async-await/issue-67765-async-diagnostic.stderr @@ -0,0 +1,12 @@ +error[E0515]: cannot return value referencing local variable `s` + --> $DIR/issue-67765-async-diagnostic.rs:13:11 + | +LL | let b = &s[..]; + | - `s` is borrowed here +LL | +LL | Err(b)?; + | ^ returns a value referencing data owned by the current function + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0515`. diff --git a/src/test/ui/lifetimes/unnamed-closure-doesnt-life-long-enough-issue-67634.rs b/src/test/ui/lifetimes/unnamed-closure-doesnt-life-long-enough-issue-67634.rs index 19d7f019047..8deb3655158 100644 --- a/src/test/ui/lifetimes/unnamed-closure-doesnt-life-long-enough-issue-67634.rs +++ b/src/test/ui/lifetimes/unnamed-closure-doesnt-life-long-enough-issue-67634.rs @@ -1,3 +1,3 @@ fn main() { - [0].iter().flat_map(|a| [0].iter().map(|_| &a)); //~ ERROR `a` does not live long enough + [0].iter().flat_map(|a| [0].iter().map(|_| &a)); //~ ERROR closure may outlive } diff --git a/src/test/ui/lifetimes/unnamed-closure-doesnt-life-long-enough-issue-67634.stderr b/src/test/ui/lifetimes/unnamed-closure-doesnt-life-long-enough-issue-67634.stderr index cb0b481e748..34470119112 100644 --- a/src/test/ui/lifetimes/unnamed-closure-doesnt-life-long-enough-issue-67634.stderr +++ b/src/test/ui/lifetimes/unnamed-closure-doesnt-life-long-enough-issue-67634.stderr @@ -1,15 +1,21 @@ -error[E0597]: `a` does not live long enough - --> $DIR/unnamed-closure-doesnt-life-long-enough-issue-67634.rs:2:49 +error[E0373]: closure may outlive the current function, but it borrows `a`, which is owned by the current function + --> $DIR/unnamed-closure-doesnt-life-long-enough-issue-67634.rs:2:44 | LL | [0].iter().flat_map(|a| [0].iter().map(|_| &a)); - | - ^- ...but `a` will be dropped here, when the enclosing closure returns - | | | - | | `a` would have to be valid for `'_`... - | has type `&i32` + | ^^^ - `a` is borrowed here + | | + | may outlive borrowed value `a` | - = note: functions cannot return a borrow to data owned within the function's scope, functions can only return borrows to data passed as arguments - = note: to learn more, visit +note: closure is returned here + --> $DIR/unnamed-closure-doesnt-life-long-enough-issue-67634.rs:2:29 + | +LL | [0].iter().flat_map(|a| [0].iter().map(|_| &a)); + | ^^^^^^^^^^^^^^^^^^^^^^ +help: to force the closure to take ownership of `a` (and any other referenced variables), use the `move` keyword + | +LL | [0].iter().flat_map(|a| [0].iter().map(move |_| &a)); + | ^^^^^^^^ error: aborting due to previous error -For more information about this error, try `rustc --explain E0597`. +For more information about this error, try `rustc --explain E0373`. diff --git a/src/test/ui/return-disjoint-regions.rs b/src/test/ui/return-disjoint-regions.rs new file mode 100644 index 00000000000..d0feb3b65e1 --- /dev/null +++ b/src/test/ui/return-disjoint-regions.rs @@ -0,0 +1,7 @@ +// See https://github.com/rust-lang/rust/pull/67911#issuecomment-576023915 +fn f<'a, 'b>(x: i32) -> (&'a i32, &'b i32) { + let y = &x; + (y, y) //~ ERROR cannot return +} + +fn main() {} diff --git a/src/test/ui/return-disjoint-regions.stderr b/src/test/ui/return-disjoint-regions.stderr new file mode 100644 index 00000000000..ed159298804 --- /dev/null +++ b/src/test/ui/return-disjoint-regions.stderr @@ -0,0 +1,11 @@ +error[E0515]: cannot return value referencing function parameter `x` + --> $DIR/return-disjoint-regions.rs:4:5 + | +LL | let y = &x; + | -- `x` is borrowed here +LL | (y, y) + | ^^^^^^ returns a value referencing data owned by the current function + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0515`.