From 419d3ae02849d47899f8fc221d37b05ac24f4efd Mon Sep 17 00:00:00 2001
From: Aaron Hill <aa1ronham@gmail.com>
Date: Tue, 20 Oct 2020 16:31:54 -0400
Subject: [PATCH] Prefer regions with an `external_name` in
 `approx_universal_upper_bound`

Fixes #75785

When displaying a MIR borrowcheck error, we may need to find an upper
bound for a region, which gives us a region to point to in the error
message. However, a region might outlive multiple distinct universal
regions, in which case the only upper bound is 'static

To try to display a meaningful error message, we compute an
'approximate' upper bound by picking one of the universal regions.
Currently, we pick the region with the lowest index - however, this
caused us to produce a suboptimal error message in issue #75785

This PR `approx_universal_upper_bound` to prefer regions with an
`external_name`. This causes us to prefer regions from function
arguments/upvars, which seems to lead to a nicer error message in some
cases.
---
 .../src/borrow_check/region_infer/mod.rs       | 18 +++++++++++++++++-
 ...ssue-74072-lifetime-name-annotations.stderr |  2 +-
 .../issue-75785-confusing-named-region.rs      | 13 +++++++++++++
 .../issue-75785-confusing-named-region.stderr  | 15 +++++++++++++++
 4 files changed, 46 insertions(+), 2 deletions(-)
 create mode 100644 src/test/ui/async-await/issue-75785-confusing-named-region.rs
 create mode 100644 src/test/ui/async-await/issue-75785-confusing-named-region.stderr

diff --git a/compiler/rustc_mir/src/borrow_check/region_infer/mod.rs b/compiler/rustc_mir/src/borrow_check/region_infer/mod.rs
index 3586be2804d6a..9d45f6fd0d348 100644
--- a/compiler/rustc_mir/src/borrow_check/region_infer/mod.rs
+++ b/compiler/rustc_mir/src/borrow_check/region_infer/mod.rs
@@ -1145,8 +1145,24 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         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);
+            // The upper bound of two non-static regions is static: this
+            // means we know nothing about the relationship between these
+            // two regions. Pick a 'better' one to use when constructing
+            // a diagnostic
             if ur != static_r && lub != static_r && new_lub == static_r {
-                lub = std::cmp::min(ur, lub);
+                // Prefer the region with an `external_name` - this
+                // indicates that the region is early-bound, so working with
+                // it can produce a nicer error.
+                if self.region_definition(ur).external_name.is_some() {
+                    lub = ur;
+                } else if self.region_definition(lub).external_name.is_some() {
+                    // Leave lub unchanged
+                } else {
+                    // If we get here, we don't have any reason to prefer
+                    // one region over the other. Just pick the
+                    // one with the lower index for now.
+                    lub = std::cmp::min(ur, lub);
+                }
             } else {
                 lub = new_lub;
             }
diff --git a/src/test/ui/async-await/issue-74072-lifetime-name-annotations.stderr b/src/test/ui/async-await/issue-74072-lifetime-name-annotations.stderr
index 123c3192cffba..b96cab9f0f51a 100644
--- a/src/test/ui/async-await/issue-74072-lifetime-name-annotations.stderr
+++ b/src/test/ui/async-await/issue-74072-lifetime-name-annotations.stderr
@@ -2,7 +2,7 @@ error[E0506]: cannot assign to `*x` because it is borrowed
   --> $DIR/issue-74072-lifetime-name-annotations.rs:9:5
    |
 LL | pub async fn async_fn(x: &mut i32) -> &i32 {
-   |                                       - let's call the lifetime of this reference `'1`
+   |                          - let's call the lifetime of this reference `'1`
 LL |     let y = &*x;
    |             --- borrow of `*x` occurs here
 LL |     *x += 1;
diff --git a/src/test/ui/async-await/issue-75785-confusing-named-region.rs b/src/test/ui/async-await/issue-75785-confusing-named-region.rs
new file mode 100644
index 0000000000000..452614087be94
--- /dev/null
+++ b/src/test/ui/async-await/issue-75785-confusing-named-region.rs
@@ -0,0 +1,13 @@
+// edition:2018
+//
+// Regression test for issue #75785
+// Tests that we don't point to a confusing named
+// region when emitting a diagnostic
+
+pub async fn async_fn(x: &mut i32) -> (&i32, &i32) {
+    let y = &*x;
+    *x += 1; //~ ERROR cannot assign to
+    (&32, y)
+}
+
+fn main() {}
diff --git a/src/test/ui/async-await/issue-75785-confusing-named-region.stderr b/src/test/ui/async-await/issue-75785-confusing-named-region.stderr
new file mode 100644
index 0000000000000..3b731d9c60a6a
--- /dev/null
+++ b/src/test/ui/async-await/issue-75785-confusing-named-region.stderr
@@ -0,0 +1,15 @@
+error[E0506]: cannot assign to `*x` because it is borrowed
+  --> $DIR/issue-75785-confusing-named-region.rs:9:5
+   |
+LL | pub async fn async_fn(x: &mut i32) -> (&i32, &i32) {
+   |                          - let's call the lifetime of this reference `'1`
+LL |     let y = &*x;
+   |             --- borrow of `*x` occurs here
+LL |     *x += 1;
+   |     ^^^^^^^ assignment to borrowed `*x` occurs here
+LL |     (&32, y)
+   |     -------- returning this value requires that `*x` is borrowed for `'1`
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0506`.