From b893cff9497a817942da99b902ea267ed1544571 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 17 Sep 2021 17:55:07 -0400 Subject: [PATCH 1/2] make member constraints pick static if no upper bounds The current member constraint algorithm has a failure mode when it encounters a variable `'0` and the only constraint is that `':static: '0`. In that case, there are no upper bounds, or at least no non-trivial upper bounds. As a result, we are not able to rule out any of the choices, so if you have a constraint like `'0 member ['a, 'b, 'static]`, where `'a` and `'b` are unrelated, then the algorithm gets stuck as there is no 'least choice' from that set. The tweak in this commit changes the algorithm so that *if* there are no upper bounds (and hence `'0` can get as large as desired without creating a region check error), it will just pick `'static`. This should only occur in cases where the data is flowing out from a `'static` value. This change is probably *not* right for impl Trait in let bindings, but those are challenging with member constraints anyway, and not currently supported. Furthermore, this change is not needed in a polonius-like formulation, which effectively permits "ad-hoc intersections" of lifetimes as the value for a region, and hence could give a value like `'a ^ 'b` as the resulting lifetime. Therefore I think there isn't forwards compat danger here. (famous last words?) --- .../rustc_borrowck/src/region_infer/mod.rs | 79 ++++++++++--------- .../src/infer/lexical_region_resolve/mod.rs | 21 ++++- .../multiple-lifetimes/two-refs-and-a-dyn.rs | 20 +++++ .../two-refs-and-a-static.rs | 16 ++++ 4 files changed, 99 insertions(+), 37 deletions(-) create mode 100644 src/test/ui/async-await/multiple-lifetimes/two-refs-and-a-dyn.rs create mode 100644 src/test/ui/async-await/multiple-lifetimes/two-refs-and-a-static.rs diff --git a/compiler/rustc_borrowck/src/region_infer/mod.rs b/compiler/rustc_borrowck/src/region_infer/mod.rs index 128faab8d722e..c5ea428d77f11 100644 --- a/compiler/rustc_borrowck/src/region_infer/mod.rs +++ b/compiler/rustc_borrowck/src/region_infer/mod.rs @@ -717,11 +717,14 @@ impl<'tcx> RegionInferenceContext<'tcx> { // free region that must outlive the member region `R0` (`UB: // R0`). Therefore, we need only keep an option `O` if `UB: O` // for all UB. + let fr_static = self.universal_regions.fr_static; let rev_scc_graph = self.reverse_scc_graph(); let universal_region_relations = &self.universal_region_relations; - for ub in rev_scc_graph.upper_bounds(scc) { + let mut any_upper_bounds = false; + for ub in rev_scc_graph.upper_bounds(scc).filter(|ub| *ub != fr_static) { debug!("apply_member_constraint: ub={:?}", ub); choice_regions.retain(|&o_r| universal_region_relations.outlives(ub, o_r)); + any_upper_bounds = true; } debug!("apply_member_constraint: after ub, choice_regions={:?}", choice_regions); @@ -730,46 +733,50 @@ impl<'tcx> RegionInferenceContext<'tcx> { return false; } - // Otherwise, we need to find the minimum remaining choice, if - // any, and take that. - debug!("apply_member_constraint: choice_regions remaining are {:#?}", choice_regions); - let min = |r1: ty::RegionVid, r2: ty::RegionVid| -> Option { - let r1_outlives_r2 = self.universal_region_relations.outlives(r1, r2); - let r2_outlives_r1 = self.universal_region_relations.outlives(r2, r1); - match (r1_outlives_r2, r2_outlives_r1) { - (true, true) => Some(r1.min(r2)), - (true, false) => Some(r2), - (false, true) => Some(r1), - (false, false) => None, - } - }; - let mut min_choice = choice_regions[0]; - for &other_option in &choice_regions[1..] { - debug!( - "apply_member_constraint: min_choice={:?} other_option={:?}", - min_choice, other_option, - ); - match min(min_choice, other_option) { - Some(m) => min_choice = m, - None => { - debug!( - "apply_member_constraint: {:?} and {:?} are incomparable; no min choice", - min_choice, other_option, - ); - return false; + // If there WERE no upper bounds (apart from static), and static is one of the options, + // then we can just pick that (c.f. #63033). + let choice = if !any_upper_bounds && choice_regions.contains(&fr_static) { + fr_static + } else { + // Otherwise, we need to find the minimum remaining choice, if + // any, and take that. + debug!("apply_member_constraint: choice_regions remaining are {:#?}", choice_regions); + let min = |r1: ty::RegionVid, r2: ty::RegionVid| -> Option { + let r1_outlives_r2 = self.universal_region_relations.outlives(r1, r2); + let r2_outlives_r1 = self.universal_region_relations.outlives(r2, r1); + match (r1_outlives_r2, r2_outlives_r1) { + (true, true) => Some(r1.min(r2)), + (true, false) => Some(r2), + (false, true) => Some(r1), + (false, false) => None, + } + }; + let mut min_choice = choice_regions[0]; + for &other_option in &choice_regions[1..] { + debug!( + "apply_member_constraint: min_choice={:?} other_option={:?}", + min_choice, other_option, + ); + match min(min_choice, other_option) { + Some(m) => min_choice = m, + None => { + debug!( + "apply_member_constraint: {:?} and {:?} are incomparable; no min choice", + min_choice, other_option, + ); + return false; + } } } - } + min_choice + }; - let min_choice_scc = self.constraint_sccs.scc(min_choice); - debug!( - "apply_member_constraint: min_choice={:?} best_choice_scc={:?}", - min_choice, min_choice_scc, - ); - if self.scc_values.add_region(scc, min_choice_scc) { + let choice_scc = self.constraint_sccs.scc(choice); + debug!("apply_member_constraint: min_choice={:?} best_choice_scc={:?}", choice, choice_scc,); + if self.scc_values.add_region(scc, choice_scc) { self.member_constraints_applied.push(AppliedMemberConstraint { member_region_scc: scc, - min_choice, + min_choice: choice, member_constraint_index, }); diff --git a/compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs b/compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs index 869fd225d5114..2b67b50b59b48 100644 --- a/compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs +++ b/compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs @@ -266,6 +266,10 @@ impl<'cx, 'tcx> LexicalResolver<'cx, 'tcx> { /// /// From that list, we look for a *minimal* option `'c_min`. If we /// find one, then we can enforce that `'r: 'c_min`. + /// + /// Alternatively, if we find that there are *NO* upper bounds of `'r` + /// apart from `'static`, and `'static` is one of choices, then + /// we set `'r` to `'static` (c.f. #63033). fn enforce_member_constraint( &self, graph: &RegionGraph<'tcx>, @@ -287,16 +291,31 @@ impl<'cx, 'tcx> LexicalResolver<'cx, 'tcx> { VarValue::ErrorValue => return false, VarValue::Value(r) => r, }; + debug!("enforce_member_constraint: lower_bound={:#?}", member_lower_bound); + if member_constraint.choice_regions.contains(&member_lower_bound) { + debug!("enforce_member_constraint: lower bound is already a valid choice"); + return false; + } // Find all the "upper bounds" -- that is, each region `b` such that // `r0 <= b` must hold. let (member_upper_bounds, ..) = self.collect_bounding_regions(graph, member_vid, OUTGOING, None); + debug!("enforce_member_constraint: upper_bounds={:#?}", member_upper_bounds); + + // If there are no upper bounds, and static is a choice (in practice, it always is), + // then we should just pick static. + if member_upper_bounds.is_empty() + && member_constraint.choice_regions.contains(&&ty::RegionKind::ReStatic) + { + debug!("enforce_member_constraint: selecting 'static since there are no upper bounds",); + *var_values.value_mut(member_vid) = VarValue::Value(self.tcx().lifetimes.re_static); + return true; + } // Get an iterator over the *available choice* -- that is, // each choice region `c` where `lb <= c` and `c <= ub` for all the // upper bounds `ub`. - debug!("enforce_member_constraint: upper_bounds={:#?}", member_upper_bounds); let mut options = member_constraint.choice_regions.iter().filter(|option| { self.sub_concrete_regions(member_lower_bound, option) && member_upper_bounds diff --git a/src/test/ui/async-await/multiple-lifetimes/two-refs-and-a-dyn.rs b/src/test/ui/async-await/multiple-lifetimes/two-refs-and-a-dyn.rs new file mode 100644 index 0000000000000..6ba464c104b74 --- /dev/null +++ b/src/test/ui/async-await/multiple-lifetimes/two-refs-and-a-dyn.rs @@ -0,0 +1,20 @@ +// Regression test for #63033. The scenario here is: +// +// - The returned future captures the `Box`, which is shorthand for `Box`. +// - The actual value that gets captured is `Box` where `'static: '?0` +// - We generate a member constraint `'?0 member ['a, 'b, 'static]` +// - None of those regions are a "least choice", so we got stuck +// +// After the fix, we now select `'static` in cases where there are no upper bounds (apart from +// 'static). +// +// edition:2018 +// check-pass + +#![allow(dead_code)] +trait T {} +struct S; +impl S { + async fn f<'a, 'b>(_a: &'a S, _b: &'b S, _c: Box) {} +} +fn main() {} diff --git a/src/test/ui/async-await/multiple-lifetimes/two-refs-and-a-static.rs b/src/test/ui/async-await/multiple-lifetimes/two-refs-and-a-static.rs new file mode 100644 index 0000000000000..9ba3a1d38f306 --- /dev/null +++ b/src/test/ui/async-await/multiple-lifetimes/two-refs-and-a-static.rs @@ -0,0 +1,16 @@ +// Regression test for #63033. The scenario here is: +// +// - The returned future captures the &'static String` +// - The actual value that gets captured is `&'?0 String` where `'static: '?0` +// - We generate a member constraint `'?0 member ['a, 'b, 'static]` +// - None of those regions are a "least choice", so we got stuck +// +// After the fix, we now select `'static` in cases where there are no upper bounds (apart from +// 'static). +// +// edition:2018 +// check-pass + +async fn test<'a, 'b>(test: &'a String, test2: &'b String, test3: &'static String) {} + +fn main() {} From 92b2be268beac7d298db2d4c0b9a980caefda664 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 23 Sep 2021 18:53:06 -0400 Subject: [PATCH 2/2] rework so that we pick static as a last resort only --- .../rustc_borrowck/src/region_infer/mod.rs | 80 +++++++++++-------- .../rustc_borrowck/src/universal_regions.rs | 3 +- .../src/infer/lexical_region_resolve/mod.rs | 42 +++++----- src/test/ui/upper-bound-from-type-param.rs | 19 +++++ 4 files changed, 89 insertions(+), 55 deletions(-) create mode 100644 src/test/ui/upper-bound-from-type-param.rs diff --git a/compiler/rustc_borrowck/src/region_infer/mod.rs b/compiler/rustc_borrowck/src/region_infer/mod.rs index c5ea428d77f11..f678d7bea91de 100644 --- a/compiler/rustc_borrowck/src/region_infer/mod.rs +++ b/compiler/rustc_borrowck/src/region_infer/mod.rs @@ -733,44 +733,58 @@ impl<'tcx> RegionInferenceContext<'tcx> { return false; } - // If there WERE no upper bounds (apart from static), and static is one of the options, - // then we can just pick that (c.f. #63033). - let choice = if !any_upper_bounds && choice_regions.contains(&fr_static) { - fr_static - } else { - // Otherwise, we need to find the minimum remaining choice, if - // any, and take that. - debug!("apply_member_constraint: choice_regions remaining are {:#?}", choice_regions); - let min = |r1: ty::RegionVid, r2: ty::RegionVid| -> Option { - let r1_outlives_r2 = self.universal_region_relations.outlives(r1, r2); - let r2_outlives_r1 = self.universal_region_relations.outlives(r2, r1); - match (r1_outlives_r2, r2_outlives_r1) { - (true, true) => Some(r1.min(r2)), - (true, false) => Some(r2), - (false, true) => Some(r1), - (false, false) => None, - } - }; - let mut min_choice = choice_regions[0]; - for &other_option in &choice_regions[1..] { - debug!( - "apply_member_constraint: min_choice={:?} other_option={:?}", - min_choice, other_option, - ); - match min(min_choice, other_option) { - Some(m) => min_choice = m, - None => { - debug!( - "apply_member_constraint: {:?} and {:?} are incomparable; no min choice", - min_choice, other_option, + // Otherwise, we need to find the minimum remaining choice, if + // any, and take that. + debug!("apply_member_constraint: choice_regions remaining are {:#?}", choice_regions); + let min = |r1: ty::RegionVid, r2: ty::RegionVid| -> Option { + let r1_outlives_r2 = self.universal_region_relations.outlives(r1, r2); + let r2_outlives_r1 = self.universal_region_relations.outlives(r2, r1); + match (r1_outlives_r2, r2_outlives_r1) { + (true, true) => Some(r1.min(r2)), + (true, false) => Some(r2), + (false, true) => Some(r1), + (false, false) => None, + } + }; + let mut min_choice = choice_regions[0]; + for &other_option in &choice_regions[1..] { + debug!( + "apply_member_constraint: min_choice={:?} other_option={:?}", + min_choice, other_option, + ); + match min(min_choice, other_option) { + Some(m) => min_choice = m, + None => { + debug!( + "apply_member_constraint: {:?} and {:?} are incomparable; no min choice", + min_choice, other_option, + ); + + // If there is no minimum choice, then we *may* wish to use `'static` + // instead, but only if there are no upper bounds, and `'static` is a valid + // choice. + if !any_upper_bounds && choice_regions.contains(&fr_static) { + return self.apply_member_constraint_choice( + scc, + member_constraint_index, + fr_static, ); - return false; } + + // Otherwise, we give up. + return false; } } - min_choice - }; + } + self.apply_member_constraint_choice(scc, member_constraint_index, min_choice) + } + fn apply_member_constraint_choice( + &mut self, + scc: ConstraintSccIndex, + member_constraint_index: NllMemberConstraintIndex, + choice: ty::RegionVid, + ) -> bool { let choice_scc = self.constraint_sccs.scc(choice); debug!("apply_member_constraint: min_choice={:?} best_choice_scc={:?}", choice, choice_scc,); if self.scc_values.add_region(scc, choice_scc) { diff --git a/compiler/rustc_borrowck/src/universal_regions.rs b/compiler/rustc_borrowck/src/universal_regions.rs index bebd19370299d..7a0def53c71c7 100644 --- a/compiler/rustc_borrowck/src/universal_regions.rs +++ b/compiler/rustc_borrowck/src/universal_regions.rs @@ -473,8 +473,9 @@ impl<'cx, 'tcx> UniversalRegionsBuilder<'cx, 'tcx> { } let fr_fn_body = self.infcx.next_nll_region_var(FR).to_region_vid(); - let num_universals = self.infcx.num_region_vars(); + debug!("build: fr_fn_Body = {:?}", fr_fn_body); + let num_universals = self.infcx.num_region_vars(); debug!("build: global regions = {}..{}", FIRST_GLOBAL_INDEX, first_extern_index); debug!("build: extern regions = {}..{}", first_extern_index, first_local_index); debug!("build: local regions = {}..{}", first_local_index, num_universals); diff --git a/compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs b/compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs index 2b67b50b59b48..98014809f68d0 100644 --- a/compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs +++ b/compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs @@ -303,16 +303,6 @@ impl<'cx, 'tcx> LexicalResolver<'cx, 'tcx> { self.collect_bounding_regions(graph, member_vid, OUTGOING, None); debug!("enforce_member_constraint: upper_bounds={:#?}", member_upper_bounds); - // If there are no upper bounds, and static is a choice (in practice, it always is), - // then we should just pick static. - if member_upper_bounds.is_empty() - && member_constraint.choice_regions.contains(&&ty::RegionKind::ReStatic) - { - debug!("enforce_member_constraint: selecting 'static since there are no upper bounds",); - *var_values.value_mut(member_vid) = VarValue::Value(self.tcx().lifetimes.re_static); - return true; - } - // Get an iterator over the *available choice* -- that is, // each choice region `c` where `lb <= c` and `c <= ub` for all the // upper bounds `ub`. @@ -326,19 +316,32 @@ impl<'cx, 'tcx> LexicalResolver<'cx, 'tcx> { // If there is more than one option, we only make a choice if // there is a single *least* choice -- i.e., some available // region that is `<=` all the others. - let mut least_choice: ty::Region<'tcx> = match options.next() { + let mut choice: ty::Region<'tcx> = match options.next() { Some(&r) => r, None => return false, }; - debug!("enforce_member_constraint: least_choice={:?}", least_choice); + debug!("enforce_member_constraint: least_choice={:?}", choice); for &option in options { debug!("enforce_member_constraint: option={:?}", option); - if !self.sub_concrete_regions(least_choice, option) { - if self.sub_concrete_regions(option, least_choice) { + if !self.sub_concrete_regions(choice, option) { + if self.sub_concrete_regions(option, choice) { debug!("enforce_member_constraint: new least choice"); - least_choice = option; + choice = option; } else { debug!("enforce_member_constraint: no least choice"); + + // Pick static if: + // * There is no least choice (which we just found to be true) + // * There are no upper bounds (so the region can grow to any size) + // * Static is a choice (in practice, it always is) + if member_upper_bounds.is_empty() + && member_constraint.choice_regions.contains(&&ty::RegionKind::ReStatic) + { + choice = self.tcx().lifetimes.re_static; + break; + } + + // Otherwise give up return false; } } @@ -353,13 +356,10 @@ impl<'cx, 'tcx> LexicalResolver<'cx, 'tcx> { // choice is needed here so that we don't end up in a cycle of // `expansion` changing the region one way and the code here changing // it back. - let lub = self.lub_concrete_regions(least_choice, member_lower_bound); - debug!( - "enforce_member_constraint: final least choice = {:?}\nlub = {:?}", - least_choice, lub - ); + let lub = self.lub_concrete_regions(choice, member_lower_bound); + debug!("enforce_member_constraint: final choice = {:?}\nlub = {:?}", choice, lub); if lub != member_lower_bound { - *var_values.value_mut(member_vid) = VarValue::Value(least_choice); + *var_values.value_mut(member_vid) = VarValue::Value(choice); true } else { false diff --git a/src/test/ui/upper-bound-from-type-param.rs b/src/test/ui/upper-bound-from-type-param.rs new file mode 100644 index 0000000000000..db172bba70d0d --- /dev/null +++ b/src/test/ui/upper-bound-from-type-param.rs @@ -0,0 +1,19 @@ +// Regression test found by crater run. The scenario here is that we have +// a region variable `?0` from the `impl Future` with no upper bounds +// and a member constraint of `?0 in ['a, 'static]`. If we pick `'static`, +// however, we then fail the check that `F: ?0` (since we only know that +// F: ?a). The problem here is that our upper bound detection +// doesn't consider "type tests" like `F: 'x`. This was fixed by +// preferring to pick a least choice and only using static as a last resort. +// +// edition:2018 +// check-pass + +trait Future {} +impl Future for () {} + +fn sink_error1<'a, F: 'a>(f: F) -> impl Future + 'a { + sink_error2(f) // error: `F` may not live long enough +} +fn sink_error2<'a, F: 'a>(_: F) -> impl Future + 'a {} +fn main() {}