Skip to content

add cycle detection for inferred outlives predicates #103309

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

add cycle detection for inferred outlives predicates #103309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44ce74e
outlives: add cycle detection for inferred outlives predicates
cormacrelf Oct 19, 2022
36d8682
outlives: move tests to rfc-2093 directory
cormacrelf Oct 21, 2022
619f916
outlives: improve docs on inference cycles
cormacrelf Oct 21, 2022
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
5 changes: 5 additions & 0 deletions compiler/rustc_hir_analysis/src/outlives/explicit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ impl<'tcx> ExplicitPredicatesMap<'tcx> {
pub(crate) fn explicit_predicates_of(
&mut self,
tcx: TyCtxt<'tcx>,
self_did: DefId,
def_id: DefId,
) -> &ty::EarlyBinder<RequiredPredicates<'tcx>> {
self.map.entry(def_id).or_insert_with(|| {
Expand All @@ -35,8 +36,10 @@ impl<'tcx> ExplicitPredicatesMap<'tcx> {
tcx,
ty.into(),
reg,
self_did,
span,
&mut required_predicates,
None,
)
}

Expand All @@ -45,8 +48,10 @@ impl<'tcx> ExplicitPredicatesMap<'tcx> {
tcx,
reg1.into(),
reg2,
self_did,
span,
&mut required_predicates,
None,
)
}

Expand Down
169 changes: 140 additions & 29 deletions compiler/rustc_hir_analysis/src/outlives/implicit_infer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,24 @@ use rustc_span::Span;
use super::explicit::ExplicitPredicatesMap;
use super::utils::*;

pub(super) type GlobalInferredOutlives<'tcx> =
FxHashMap<DefId, ty::EarlyBinder<RequiredPredicates<'tcx>>>;

/// Infer predicates for the items in the crate.
///
/// `global_inferred_outlives`: this is initially the empty map that
/// was generated by walking the items in the crate. This will
/// now be filled with inferred predicates.
pub(super) fn infer_predicates<'tcx>(
tcx: TyCtxt<'tcx>,
) -> FxHashMap<DefId, ty::EarlyBinder<RequiredPredicates<'tcx>>> {
debug!("infer_predicates");

pub(super) fn infer_predicates<'tcx>(tcx: TyCtxt<'tcx>) -> GlobalInferredOutlives<'tcx> {
let mut explicit_map = ExplicitPredicatesMap::new();

let mut global_inferred_outlives = FxHashMap::default();

// If new predicates were added then we need to re-calculate
// all crates since there could be new implied predicates.
let mut round = 1;
'outer: loop {
Comment on lines +26 to 27
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mut round = 1;
'outer: loop {
'outer: for round in 1.. {

debug!("infer_predicates: round {round}");
let mut predicates_added = false;

// Visit all the crates and infer predicates
Expand All @@ -50,6 +51,7 @@ pub(super) fn infer_predicates<'tcx>(
let field_span = tcx.def_span(field_def.did);
insert_required_predicates_to_be_wf(
tcx,
adt_def.did(),
field_ty,
field_span,
&global_inferred_outlives,
Expand All @@ -72,6 +74,22 @@ pub(super) fn infer_predicates<'tcx>(
global_inferred_outlives.get(&item_did.to_def_id()).map_or(0, |p| p.0.len());
if item_required_predicates.len() > item_predicates_len {
predicates_added = true;
if tracing::enabled!(tracing::Level::DEBUG) {
let def_id = item_did.to_def_id();
use std::collections::BTreeSet;
let global_preds: BTreeSet<_> =
global_inferred_outlives.get(&def_id).map_or_else(Default::default, |e| {
e.0.iter().map(|(pred, _)| pred).collect()
});
let computed_preds: BTreeSet<_> =
item_required_predicates.iter().map(|(pred, _)| pred).collect();
let added = computed_preds.difference(&global_preds).collect::<BTreeSet<_>>();
debug!("global_inferred_outlives grew for {def_id:?}, added: {added:?}");
let removed = global_preds.difference(&computed_preds).collect::<BTreeSet<_>>();
if !removed.is_empty() {
debug!("global_inferred_outlives lost predicates: {removed:?}")
}
}
global_inferred_outlives
.insert(item_did.to_def_id(), ty::EarlyBinder(item_required_predicates));
}
Expand All @@ -80,16 +98,18 @@ pub(super) fn infer_predicates<'tcx>(
if !predicates_added {
break 'outer;
}
round += 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for sanity, maybe check the recursion limit here against round (I think that could be a valid PR on its own that we could crater and land to ensure the currently indefinite-loop actually finishes

}

global_inferred_outlives
}

fn insert_required_predicates_to_be_wf<'tcx>(
tcx: TyCtxt<'tcx>,
self_did: DefId,
field_ty: Ty<'tcx>,
field_span: Span,
global_inferred_outlives: &FxHashMap<DefId, ty::EarlyBinder<RequiredPredicates<'tcx>>>,
global_inferred_outlives: &GlobalInferredOutlives<'tcx>,
required_predicates: &mut RequiredPredicates<'tcx>,
explicit_map: &mut ExplicitPredicatesMap<'tcx>,
) {
Expand All @@ -109,14 +129,22 @@ fn insert_required_predicates_to_be_wf<'tcx>(
// We also want to calculate potential predicates for the T
ty::Ref(region, rty, _) => {
debug!("Ref");
insert_outlives_predicate(tcx, rty.into(), region, field_span, required_predicates);
insert_outlives_predicate(
tcx,
rty.into(),
region,
self_did,
field_span,
required_predicates,
None,
);
}

// For each Adt (struct/enum/union) type `Foo<'a, T>`, we
// can load the current set of inferred and explicit
// predicates from `global_inferred_outlives` and filter the
// ones that are TypeOutlives.
ty::Adt(def, substs) => {
ty::Adt(adt, substs) => {
// First check the inferred predicates
//
// Example 1:
Expand All @@ -136,21 +164,86 @@ fn insert_required_predicates_to_be_wf<'tcx>(
// `['b => 'a, U => T]` and thus get the requirement that `T:
// 'a` holds for `Foo`.
debug!("Adt");
if let Some(unsubstituted_predicates) = global_inferred_outlives.get(&def.did()) {
for (unsubstituted_predicate, &span) in &unsubstituted_predicates.0 {
// `unsubstituted_predicate` is `U: 'b` in the
// example above. So apply the substitution to
// get `T: 'a` (or `predicate`):
let predicate = unsubstituted_predicates
.rebind(*unsubstituted_predicate)
.subst(tcx, substs);
insert_outlives_predicate(
tcx,
predicate.0,
predicate.1,
span,
required_predicates,
);
if let Some(unsubstituted_predicates) = global_inferred_outlives.get(&adt.did()) {
for (unsubstituted_predicate, stack) in &unsubstituted_predicates.0 {
// We must detect cycles in the inference. If we don't, rustc can hang.
// Cycles can be formed by associated types on traits when they are used like so:
//
// ```
// trait Trait<'a> {
// type Assoc: 'a;
// }
// struct Node<'node, T: Trait<'node>> {
// var: Var<'node, T::Assoc>,
// }
// struct Var<'var, R: 'var> {
// node: Box<Node<'var, RGen<R>>>,
// }
// struct RGen<R>(std::marker::PhantomData<R>);
// impl<'a, R: 'a> Trait<'a> for RGen<R> {
// type Assoc = R; // works with () too
// }
// ```
//
// Visiting Node, we walk the fields and find a Var. Var has an explicit
// R : 'var.
// Node finds this in check_explicit_predicates.
// Node substitutes its type parameters for Var through, and gets an inferred
// <T as Trait<'node>>::Assoc: 'node.
// Visiting Var, we walk the fields and find Node, for which there us an
// unsubstituted predicate in the global map. So Var gets
// <RGen<R> as Trait<'var>>::Assoc: 'var
// But Node contains a Var. So Node gets
// <RGen<<T as Trait<'node>>::Assoc> as Trait<'node>>::Assoc 'node
// Var gets
// <RGen<<RGen<R> as Trait<'var>>::Assoc> as Trait<'var>>::Assoc: 'var
//
// Etc. This goes on forever. The fact that RGen<R>::Assoc *is* R is
// irrelevant. It is simply that both types find a way to add a bigger
// predicate each time.
//
// The outlives requirements propagate through the crate's types like a
// graph walk, so to detect this cycle we can just track visited nodes in
// our branch by pushing them on a stack when we move through the graph.
// An edge move is when a type picks up a predicate from one of its fields
// that it hasn't seen before. We store the stacks alongside the predicates,
// so they carry their provenance with them through the current and
// subsequent rounds of crate-wide inference.
//
// Try: RUSTC_LOG=rustc_hir_analysis::outlives=debug \
// rustc +stage1 src/test/ui/rfc-2093-infer-outlives/issue-102966.rs 2>&1 \
// | rg '(grew|cycle)'
//
// We do not treat a type with an explicit bound as the first in the visit
// stack. So in the example, Node appears first in the stack, and each of
// Node and Var will get a new bound constraining an ::Assoc projection
// before the cycle is cut at Node. If Var were already in the visit stack,
// it would not receive the projection bound, which is something it needs.
Comment on lines +217 to +221
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main thing I would like some eyes on. I think it's right for Var to get a substituted-through version of its own explicit bound here, because Var doesn't know what RGen<R>::Assoc is, but whatever it is, Node does need it to be : 'node. It seems the cautious approach to add the bound if we're not going to find out what the projection resolves to.

But I don't know and don't see much guidance in the RFC or its tests, as this situation wasn't really contemplated. Can anyone think of a test that might illuminate the correctness of this?

Tangentially, this PR has more tracking of the source of inferred outlives than before. Are there error messages we can improve with this information? Like the example of a "long range error" message in the original RFC.


if stack.iter().any(|&(did, _span)| did == self_did) {
debug!(
"detected cycle in inferred_outlives_predicates,\
for unsubstituted predicate {unsubstituted_predicate:?}:\
{self_did:?} found in {stack:?}"
);
} else {
// `unsubstituted_predicate` is `U: 'b` in Example 1 above.
// So apply the substitution to get `T: 'a` (or `predicate`):
let predicate = unsubstituted_predicates
.rebind(*unsubstituted_predicate)
.subst(tcx, substs);

insert_outlives_predicate(
tcx,
predicate.0,
predicate.1,
self_did,
field_span,
required_predicates,
// a copy of this is made for the predicate and (self_did, field_span) is pushed.
Some(stack),
);
}
}
}

Expand All @@ -159,7 +252,8 @@ fn insert_required_predicates_to_be_wf<'tcx>(
// let _: () = substs.region_at(0);
check_explicit_predicates(
tcx,
def.did(),
self_did,
adt.did(),
substs,
required_predicates,
explicit_map,
Expand Down Expand Up @@ -187,6 +281,7 @@ fn insert_required_predicates_to_be_wf<'tcx>(
ex_trait_ref.with_self_ty(tcx, tcx.types.usize).skip_binder().substs;
check_explicit_predicates(
tcx,
self_did,
ex_trait_ref.skip_binder().def_id,
substs,
required_predicates,
Expand All @@ -202,6 +297,7 @@ fn insert_required_predicates_to_be_wf<'tcx>(
debug!("Projection");
check_explicit_predicates(
tcx,
self_did,
tcx.parent(obj.item_def_id),
obj.substs,
required_predicates,
Expand Down Expand Up @@ -232,23 +328,28 @@ fn insert_required_predicates_to_be_wf<'tcx>(
/// applying the substitution as above.
fn check_explicit_predicates<'tcx>(
tcx: TyCtxt<'tcx>,
// i.e. Foo
self_did: DefId,
// i.e. Bar
def_id: DefId,
substs: &[GenericArg<'tcx>],
required_predicates: &mut RequiredPredicates<'tcx>,
explicit_map: &mut ExplicitPredicatesMap<'tcx>,
ignored_self_ty: Option<Ty<'tcx>>,
) {
debug!(
"check_explicit_predicates(def_id={:?}, \
"check_explicit_predicates(\
self_did={:?},\
def_id={:?}, \
substs={:?}, \
explicit_map={:?}, \
required_predicates={:?}, \
ignored_self_ty={:?})",
def_id, substs, explicit_map, required_predicates, ignored_self_ty,
self_did, def_id, substs, explicit_map, required_predicates, ignored_self_ty,
);
let explicit_predicates = explicit_map.explicit_predicates_of(tcx, def_id);
let explicit_predicates = explicit_map.explicit_predicates_of(tcx, self_did, def_id);

for (outlives_predicate, &span) in &explicit_predicates.0 {
for (outlives_predicate, stack) in &explicit_predicates.0 {
debug!("outlives_predicate = {:?}", &outlives_predicate);

// Careful: If we are inferring the effects of a `dyn Trait<..>`
Expand Down Expand Up @@ -293,8 +394,18 @@ fn check_explicit_predicates<'tcx>(
continue;
}

let &(_foo_did, span) = stack.last().unwrap();
let predicate = explicit_predicates.rebind(*outlives_predicate).subst(tcx, substs);
debug!("predicate = {:?}", &predicate);
insert_outlives_predicate(tcx, predicate.0, predicate.1, span, required_predicates);
insert_outlives_predicate(
tcx,
predicate.0,
predicate.1,
// i.e. Foo, not the field ADT definition.
self_did,
span,
required_predicates,
None,
);
}
}
7 changes: 4 additions & 3 deletions compiler/rustc_hir_analysis/src/outlives/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,21 +98,22 @@ fn inferred_outlives_crate(tcx: TyCtxt<'_>, (): ()) -> CratePredicatesMap<'_> {
.iter()
.map(|(&def_id, set)| {
let predicates = &*tcx.arena.alloc_from_iter(set.0.iter().filter_map(
|(ty::OutlivesPredicate(kind1, region2), &span)| {
|(ty::OutlivesPredicate(kind1, region2), stack)| {
let &(_, last_span) = stack.last().unwrap();
match kind1.unpack() {
GenericArgKind::Type(ty1) => Some((
ty::Binder::dummy(ty::PredicateKind::TypeOutlives(
ty::OutlivesPredicate(ty1, *region2),
))
.to_predicate(tcx),
span,
last_span,
)),
GenericArgKind::Lifetime(region1) => Some((
ty::Binder::dummy(ty::PredicateKind::RegionOutlives(
ty::OutlivesPredicate(region1, *region2),
))
.to_predicate(tcx),
span,
last_span,
)),
GenericArgKind::Const(_) => {
// Generic consts don't impose any constraints.
Expand Down
Loading