Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make member constraints pick static if no upper bounds #89056

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 43 additions & 36 deletions compiler/rustc_borrowck/src/region_infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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<ty::RegionVid> {
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<ty::RegionVid> {
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),
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, unrelated to this PR, just out of curiosity / to make sure I understand this part correctly: would this second branch already cover the first? (at the cost of breaking the "symmetry" in the code)

(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,
});

Expand Down
21 changes: 20 additions & 1 deletion compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand All @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice the check here is simpler than the one with any_upper_bounds done above (e.g., non-empty case with only 'statics inside): does collect_bounding_regions never yield 'statics?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conjecture on my part: I wouldn't expect it to ? It wouldn't constrain the variable to add it as an upper bound, it's already the element.

We'll see when we do the Zoom session ^^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the zoom session recorded somewhere btw?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& 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
Expand Down
20 changes: 20 additions & 0 deletions src/test/ui/async-await/multiple-lifetimes/two-refs-and-a-dyn.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Regression test for #63033. The scenario here is:
//
// - The returned future captures the `Box<dyn T>`, which is shorthand for `Box<dyn T + 'static>`.
// - The actual value that gets captured is `Box<dyn T + '?0>` 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<dyn T>) {}
}
fn main() {}
Original file line number Diff line number Diff line change
@@ -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() {}