Account for 'duplicate' closure regions in borrowck diagnostics

Fixes #67765

When we process closures/generators, we create a new NLL inference variable
each time we encounter an early-bound region (e.g. "'a") in the substs
of the closure. These region variables are then treated as
universal regions when the perform region inference for the closure.

However, we may encounter the same region multiple times, such
as when the closure references multiple upvars that are bound
by the same early-bound lifetime. For example:

`fn foo<'a>(x: &'a u8, y: &'a u8) -> u8 { (|| *x + *y)() }`

This results in the creation of multiple 'duplicate' region variables,
which all correspond to the same early-bound region. During
type-checking of the closure, we don't really care - any constraints
involving these regions will get propagated back up to the enclosing
function, which is then responsible for checking said constraints
using the 'real' regions.

Unfortunately, this presents a problem for diagnostic code, which may
run in the context of the closure. In order to display a good error
message, we need to map arbitrary region inference variables (which may
not correspond to anything meaningful to the user) into a 'nicer' region
variable that can be displayed to the user (e.g. a universally bound
region, written by the user). To accomplish this, we repeatedly
compute an 'upper bound' of the region variable, stopping once
we hit a universally bound region, or are unable to make progress.

During the processing of a closure, we may determine that a region
variable needs to outlive mutliple universal regions. In a closure
context, some of these universal regions may actually be 'the same'
region - that is, they correspond to the same early-bound region.
If this is the case, we will end up trying to compute an upper bound
using these regions variables, which will fail (we don't know about
any relationship between them).

However, we don't actually need to find an upper bound involving these
duplicate regions - since they're all actually "the same" region, we can
just pick an arbirary region variable from a given "duplicate set" (all
region variables that correspond to a given early-bound region).

By doing so, we can generate a more precise diagnostic, since we will be
able to print a message involving a particular early-bound region (and
the variables using it), instead of falling back to a more generic error
message.

Author: Aaron1011

src/librustc_mir/borrow_check/constraints/mod.rs

@@ -93,7 +93,11 @@ pub struct OutlivesConstraint {
 
 impl fmt::Debug for OutlivesConstraint {
     fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(formatter, "({:?}: {:?}) due to {:?}", self.sup, self.sub, self.locations)
+        write!(
+            formatter,
+            "({:?}: {:?}) due to {:?} ({:?})",
+            self.sup, self.sub, self.locations, self.category
+        )
     }
 }
 

src/librustc_mir/borrow_check/diagnostics/region_errors.rs

@@ -23,6 +23,7 @@ use crate::borrow_check::{
 };
 
 use super::{OutlivesSuggestionBuilder, RegionErrorNamingCtx, RegionName, RegionNameSource};
+use rustc_data_structures::fx::FxHashSet;
 
 impl ConstraintDescription for ConstraintCategory {
     fn description(&self) -> &'static str {
@@ -146,13 +147,34 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         if self.universal_regions.is_universal_region(r) {
             Some(r)
         } else {
+            debug!("to_error_region_vid(r={:?}={})", r, self.region_value_str(r));
+
+            // A modified version of `universal_upper_bound`, adapted for
+            // diagnostic purposes.
+            let mut lub = self.universal_regions.fr_fn_body;
             let r_scc = self.constraint_sccs.scc(r);
-            let upper_bound = self.universal_upper_bound(r);
-            if self.scc_values.contains(r_scc, upper_bound) {
-                self.to_error_region_vid(upper_bound)
-            } else {
-                None
+
+            // The set of all 'duplicate' regions that we've seen so far.
+            // See the `diagnostic_dup_regions` field docs for more details
+            let mut duplicates: FxHashSet<RegionVid> = Default::default();
+            for ur in self.scc_values.universal_regions_outlived_by(r_scc) {
+                let duplicate_region = duplicates.contains(&ur);
+                debug!("to_error_region_vid: ur={:?}, duplicate_region={}", ur, duplicate_region);
+                if !duplicate_region {
+                    // Since we're computing an upper bound using
+                    // this region, we do *not* want to compute
+                    // upper bounds using any duplicates of it.
+                    // We extend our set of duplicates with all of the duplicates
+                    // correspodnign to this region (if it has any duplicates),
+                    self.universal_regions
+                        .diagnostic_dup_regions
+                        .get(&ur)
+                        .map(|v| duplicates.extend(v));
+                    lub = self.universal_region_relations.postdom_upper_bound(lub, ur);
+                }
             }
+
+            if self.scc_values.contains(r_scc, lub) { self.to_error_region_vid(lub) } else { None }
         }
     }
 

src/librustc_mir/borrow_check/region_infer/mod.rs

@@ -1230,18 +1230,25 @@ impl<'tcx> RegionInferenceContext<'tcx> {
             });
 
         if !universal_outlives {
+            debug!("eval_outlives: universal_outlives=false, returning false");
             return false;
         }
 
         // Now we have to compare all the points in the sub region and make
         // sure they exist in the sup region.
 
         if self.universal_regions.is_universal_region(sup_region) {
+            debug!("eval_outlives: sup_region={:?} is universal, returning true", sup_region);
             // Micro-opt: universal regions contain all points.
             return true;
         }
 
-        self.scc_values.contains_points(sup_region_scc, sub_region_scc)
+        let res = self.scc_values.contains_points(sup_region_scc, sub_region_scc);
+        debug!(
+            "eval_outlives: scc_values.contains_points(sup_region_scc={:?}, sub_region_scc={:?}) = {:?}",
+            sup_region_scc, sub_region_scc, res
+        );
+        res
     }
 
     /// Once regions have been propagated, this method is used to see

src/librustc_mir/borrow_check/type_check/free_region_relations.rs

@@ -96,12 +96,15 @@ impl UniversalRegionRelations<'tcx> {
     /// (See `TransitiveRelation::postdom_upper_bound` for details on
     /// the postdominating upper bound in general.)
     crate fn postdom_upper_bound(&self, fr1: RegionVid, fr2: RegionVid) -> RegionVid {
+        debug!("postdom_upper_bound(fr1={:?}, fr2={:?})", fr1, fr2);
         assert!(self.universal_regions.is_universal_region(fr1));
         assert!(self.universal_regions.is_universal_region(fr2));
-        *self
+        let res = *self
             .inverse_outlives
             .postdom_upper_bound(&fr1, &fr2)
-            .unwrap_or(&self.universal_regions.fr_static)
+            .unwrap_or(&self.universal_regions.fr_static);
+        debug!("postdom_upper_bound(fr1={:?}, fr2={:?}) = {:?}", fr1, fr2, res);
+        res
     }
 
     /// Finds an "upper bound" for `fr` that is not local. In other

src/librustc_mir/borrow_check/universal_regions.rs

@@ -26,6 +26,7 @@ use rustc_index::vec::{Idx, IndexVec};
 use std::iter;
 
 use crate::borrow_check::nll::ToRegionVid;
+use rustc_data_structures::fx::FxHashSet;
 
 #[derive(Debug)]
 pub struct UniversalRegions<'tcx> {
@@ -73,6 +74,37 @@ pub struct UniversalRegions<'tcx> {
     pub unnormalized_input_tys: &'tcx [Ty<'tcx>],
 
     pub yield_ty: Option<Ty<'tcx>>,
+
+    /// Extra information about region relationships, used
+    /// only when printing diagnostics.
+    ///
+    /// When processing closures/generators, we may generate multiple
+    /// region variables that all correspond to the same early-bound region.
+    /// We don't want to record these in `UniversalRegionRelations`,
+    /// as this would interfere with the propagation of closure
+    /// region constraints back to the parent function.
+    ///
+    /// Instead, we record this additional information here.
+    /// We map each region variable to a set of all other
+    /// region variables that correspond to the same early-bound region.
+    ///
+    /// For example, if we generate the following variables:
+    ///
+    /// 'a -> (_#0r, _#1r)
+    /// 'b -> (_#2r, _#3r)
+    ///
+    /// Then the map will look like this:
+    /// _#0r -> _#1r
+    /// _#1r -> _#0r
+    /// _#2r -> _#3r
+    /// _#3r -> _#2r
+    ///
+    /// When we compute upper bounds during diagnostic generation,
+    /// we accumulate a set of 'duplicate' from all non-duplicate
+    /// regions we've seen so far. Before we compute an upper bound,
+    /// we check if the region appears in our duplicates set - if so,
+    /// we skip it.
+    pub diagnostic_dup_regions: FxHashMap<RegionVid, FxHashSet<RegionVid>>,
 }
 
 /// The "defining type" for this MIR. The key feature of the "defining
@@ -234,9 +266,14 @@ impl<'tcx> UniversalRegions<'tcx> {
         assert_eq!(
             region_mapping.len(),
             expected_num_vars,
-            "index vec had unexpected number of variables"
+            "index vec had unexpected number of variables: {:?}",
+            region_mapping
         );
 
+        debug!(
+            "closure_mapping: closure_substs={:?} closure_base_def_id={:?} region_mapping={:?}",
+            closure_substs, closure_base_def_id, region_mapping
+        );
         region_mapping
     }
 
@@ -378,8 +415,8 @@ impl<'cx, 'tcx> UniversalRegionsBuilder<'cx, 'tcx> {
         // add will be external.
         let first_extern_index = self.infcx.num_region_vars();
 
-        let defining_ty = self.defining_ty();
-        debug!("build: defining_ty={:?}", defining_ty);
+        let (defining_ty, dup_regions) = self.defining_ty();
+        debug!("build: defining_ty={:?} dup_regions={:?}", defining_ty, dup_regions);
 
         let mut indices = self.compute_indices(fr_static, defining_ty);
         debug!("build: indices={:?}", indices);
@@ -396,8 +433,12 @@ impl<'cx, 'tcx> UniversalRegionsBuilder<'cx, 'tcx> {
             self.infcx.replace_late_bound_regions_with_nll_infer_vars(self.mir_def_id, &mut indices)
         }
 
+        debug!("build: after closure: indices={:?}", indices);
+
         let bound_inputs_and_output = self.compute_inputs_and_output(&indices, defining_ty);
 
+        debug!("build: compute_inputs_and_output: indices={:?}", indices);
+
         // "Liberate" the late-bound regions. These correspond to
         // "local" free regions.
         let first_local_index = self.infcx.num_region_vars();
@@ -462,13 +503,14 @@ impl<'cx, 'tcx> UniversalRegionsBuilder<'cx, 'tcx> {
             defining_ty,
             unnormalized_output_ty,
             unnormalized_input_tys,
-            yield_ty: yield_ty,
+            yield_ty,
+            diagnostic_dup_regions: dup_regions,
         }
     }
 
     /// Returns the "defining type" of the current MIR;
     /// see `DefiningTy` for details.
-    fn defining_ty(&self) -> DefiningTy<'tcx> {
+    fn defining_ty(&self) -> (DefiningTy<'tcx>, FxHashMap<RegionVid, FxHashSet<RegionVid>>) {
         let tcx = self.infcx.tcx;
         let closure_base_def_id = tcx.closure_base_def_id(self.mir_def_id);
 
@@ -483,10 +525,10 @@ impl<'cx, 'tcx> UniversalRegionsBuilder<'cx, 'tcx> {
 
                 debug!("defining_ty (pre-replacement): {:?}", defining_ty);
 
-                let defining_ty =
+                let (defining_ty, dup_regions) =
                     self.infcx.replace_free_regions_with_nll_infer_vars(FR, &defining_ty);
 
-                match defining_ty.kind {
+                let def_ty = match defining_ty.kind {
                     ty::Closure(def_id, substs) => DefiningTy::Closure(def_id, substs),
                     ty::Generator(def_id, substs, movability) => {
                         DefiningTy::Generator(def_id, substs, movability)
@@ -498,15 +540,16 @@ impl<'cx, 'tcx> UniversalRegionsBuilder<'cx, 'tcx> {
                         self.mir_def_id,
                         defining_ty
                     ),
-                }
+                };
+                (def_ty, dup_regions)
             }
 
             BodyOwnerKind::Const | BodyOwnerKind::Static(..) => {
                 assert_eq!(closure_base_def_id, self.mir_def_id);
                 let identity_substs = InternalSubsts::identity_for_item(tcx, closure_base_def_id);
-                let substs =
+                let (substs, dup_regions) =
                     self.infcx.replace_free_regions_with_nll_infer_vars(FR, &identity_substs);
-                DefiningTy::Const(self.mir_def_id, substs)
+                (DefiningTy::Const(self.mir_def_id, substs), dup_regions)
             }
         }
     }
@@ -609,7 +652,7 @@ trait InferCtxtExt<'tcx> {
         &self,
         origin: NLLRegionVariableOrigin,
         value: &T,
-    ) -> T
+    ) -> (T, FxHashMap<RegionVid, FxHashSet<RegionVid>>)
     where
         T: TypeFoldable<'tcx>;
 
@@ -635,11 +678,35 @@ impl<'cx, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'cx, 'tcx> {
         &self,
         origin: NLLRegionVariableOrigin,
         value: &T,
-    ) -> T
+    ) -> (T, FxHashMap<RegionVid, FxHashSet<RegionVid>>)
     where
         T: TypeFoldable<'tcx>,
     {
-        self.tcx.fold_regions(value, &mut false, |_region, _depth| self.next_nll_region_var(origin))
+        let mut dup_regions_map: FxHashMap<ty::Region<'tcx>, Vec<RegionVid>> = Default::default();
+        let folded = self.tcx.fold_regions(value, &mut false, |region, _depth| {
+            let new_region = self.next_nll_region_var(origin);
+            let new_vid = match new_region {
+                ty::ReVar(vid) => vid,
+                _ => unreachable!(),
+            };
+            dup_regions_map.entry(region).or_insert_with(|| Vec::new()).push(*new_vid);
+            new_region
+        });
+        let mut dup_regions: FxHashMap<RegionVid, FxHashSet<RegionVid>> = Default::default();
+        for region_set in dup_regions_map.into_iter().map(|(_, v)| v) {
+            for first in &region_set {
+                for second in &region_set {
+                    if first != second {
+                        dup_regions.entry(*first).or_default().insert(*second);
+                    }
+                }
+            }
+        }
+        debug!(
+            "replace_free_regions_with_nll_infer_vars({:?}): dup_regions={:?} folded={:?}",
+            value, dup_regions, folded
+        );
+        (folded, dup_regions)
     }
 
     fn replace_bound_regions_with_nll_infer_vars<T>(

src/test/ui/async-await/issue-67765-async-diagnostic.rs

@@ -0,0 +1,13 @@
+// edition:2018
+
+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:10: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`.