Rollup merge of #73806 - Aaron1011:feature/approx-universal-upper, r=estebank

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`.

Author: Manishearth

src/librustc_mir/borrow_check/diagnostics/region_errors.rs

@@ -122,7 +122,9 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
         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)

src/librustc_mir/borrow_check/region_infer/mod.rs

@@ -1114,6 +1114,40 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         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(

src/librustc_mir/borrow_check/region_infer/opaque_types.rs

@@ -141,7 +141,8 @@ impl<'tcx> RegionInferenceContext<'tcx> {
     {
         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,

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(())
+}

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`.

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
 }

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 <https://doc.rust-lang.org/book/ch04-02-references-and-borrowing.html#dangling-references>
+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`.

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() {}

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`.