From ced109f37924742a5a791bc53104fd6223757791 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 3 Jan 2020 18:35:10 -0500 Subject: [PATCH] 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. --- .../borrow_check/constraints/mod.rs | 6 +- .../borrow_check/diagnostics/region_errors.rs | 32 ++++++- .../borrow_check/region_infer/mod.rs | 9 +- .../type_check/free_region_relations.rs | 7 +- .../borrow_check/universal_regions.rs | 93 ++++++++++++++++--- .../issue-67765-async-diagnostic.rs | 13 +++ .../issue-67765-async-diagnostic.stderr | 12 +++ 7 files changed, 150 insertions(+), 22 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 diff --git a/src/librustc_mir/borrow_check/constraints/mod.rs b/src/librustc_mir/borrow_check/constraints/mod.rs index 48defecd28cb0..a8f94508add71 100644 --- a/src/librustc_mir/borrow_check/constraints/mod.rs +++ b/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 + ) } } diff --git a/src/librustc_mir/borrow_check/diagnostics/region_errors.rs b/src/librustc_mir/borrow_check/diagnostics/region_errors.rs index 8d534b6ec8e8e..a78fa363bc107 100644 --- a/src/librustc_mir/borrow_check/diagnostics/region_errors.rs +++ b/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 = 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 } } } diff --git a/src/librustc_mir/borrow_check/region_infer/mod.rs b/src/librustc_mir/borrow_check/region_infer/mod.rs index 73267b0f39973..81cfc0c315c26 100644 --- a/src/librustc_mir/borrow_check/region_infer/mod.rs +++ b/src/librustc_mir/borrow_check/region_infer/mod.rs @@ -1230,6 +1230,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { }); if !universal_outlives { + debug!("eval_outlives: universal_outlives=false, returning false"); return false; } @@ -1237,11 +1238,17 @@ impl<'tcx> RegionInferenceContext<'tcx> { // 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 diff --git a/src/librustc_mir/borrow_check/type_check/free_region_relations.rs b/src/librustc_mir/borrow_check/type_check/free_region_relations.rs index 0e4801b88d87e..84964557b4409 100644 --- a/src/librustc_mir/borrow_check/type_check/free_region_relations.rs +++ b/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 diff --git a/src/librustc_mir/borrow_check/universal_regions.rs b/src/librustc_mir/borrow_check/universal_regions.rs index c7ef017215e0c..6258b0eff5080 100644 --- a/src/librustc_mir/borrow_check/universal_regions.rs +++ b/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>, + + /// 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>, } /// 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>) { 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>) 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>) 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, Vec> = 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> = Default::default(); + for region_set in dup_regions_map.into_iter().map(|(_, v)| v) { + for first in ®ion_set { + for second in ®ion_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( 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 0000000000000..3885de564d3fb --- /dev/null +++ b/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(()) +} 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 0000000000000..0cea366952c1f --- /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: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`.