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

Replace the global fulfillment cache with the evaluation cache #42840

Merged
merged 4 commits into from
Jul 7, 2017
Merged
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
36 changes: 1 addition & 35 deletions src/librustc/traits/fulfill.rs
Original file line number Diff line number Diff line change
@@ -318,7 +318,7 @@ impl<'a, 'b, 'gcx, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'b, 'gcx,
_marker: PhantomData<&'c PendingPredicateObligation<'tcx>>)
where I: Clone + Iterator<Item=&'c PendingPredicateObligation<'tcx>>,
{
if coinductive_match(self.selcx, cycle.clone()) {
if self.selcx.coinductive_match(cycle.clone().map(|s| s.obligation.predicate)) {
debug!("process_child_obligations: coinductive match");
} else {
let cycle : Vec<_> = cycle.map(|c| c.obligation.clone()).collect();
@@ -549,40 +549,6 @@ fn process_predicate<'a, 'gcx, 'tcx>(
}
}

/// For defaulted traits, we use a co-inductive strategy to solve, so
/// that recursion is ok. This routine returns true if the top of the
/// stack (`cycle[0]`):
/// - is a defaulted trait, and
/// - it also appears in the backtrace at some position `X`; and,
/// - all the predicates at positions `X..` between `X` an the top are
/// also defaulted traits.
fn coinductive_match<'a,'c,'gcx,'tcx,I>(selcx: &mut SelectionContext<'a,'gcx,'tcx>,
cycle: I) -> bool
where I: Iterator<Item=&'c PendingPredicateObligation<'tcx>>,
'tcx: 'c
{
let mut cycle = cycle;
cycle
.all(|bt_obligation| {
let result = coinductive_obligation(selcx, &bt_obligation.obligation);
debug!("coinductive_match: bt_obligation={:?} coinductive={}",
bt_obligation, result);
result
})
}

fn coinductive_obligation<'a,'gcx,'tcx>(selcx: &SelectionContext<'a,'gcx,'tcx>,
obligation: &PredicateObligation<'tcx>)
-> bool {
match obligation.predicate {
ty::Predicate::Trait(ref data) => {
selcx.tcx().trait_has_default_impl(data.def_id())
}
_ => {
false
}
}
}

fn register_region_obligation<'tcx>(t_a: Ty<'tcx>,
r_b: ty::Region<'tcx>,
43 changes: 40 additions & 3 deletions src/librustc/traits/select.rs
Original file line number Diff line number Diff line change
@@ -703,14 +703,24 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
// affect the inferencer state and (b) that if we see two
// skolemized types with the same index, they refer to the
// same unbound type variable.
if
if let Some(rec_index) =
stack.iter()
.skip(1) // skip top-most frame
.any(|prev| stack.fresh_trait_ref == prev.fresh_trait_ref)
.position(|prev| stack.fresh_trait_ref == prev.fresh_trait_ref)
{
debug!("evaluate_stack({:?}) --> recursive",
stack.fresh_trait_ref);
return EvaluatedToOk;
let cycle = stack.iter().skip(1).take(rec_index+1);
let cycle = cycle.map(|stack| ty::Predicate::Trait(stack.obligation.predicate));
if self.coinductive_match(cycle) {
debug!("evaluate_stack({:?}) --> recursive, coinductive",
stack.fresh_trait_ref);
return EvaluatedToOk;
} else {
debug!("evaluate_stack({:?}) --> recursive, inductive",
stack.fresh_trait_ref);
return EvaluatedToErr;
Copy link
Contributor

Choose a reason for hiding this comment

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

So we've been working through this logic in the context of chalk -- it turns out that error isn't really the right answer here. In particular, it can lead you to "overconfidence". The correct answer probably requires iteration (at least, that's what we're doing in chalk). Maybe might be tolerable for now, but may break stuff, I'm not sure.

PR rust-lang/chalk#47 by @scalexm outlines the chalk strategy, which is based on a technique called tabling. In short, you start out by saying error, but then -- if you find solutions -- you iterate again, and this time, on the cycle, you return the solution that you found. This may allow you to find a second solution, in which case you can report ambiguity (no unique solution).

This example test is relevant -- on the first iteration, we encounter a cycle testing whether exists<T> { T: Foo } (that is, S<T>: Foo inquires whether S<T>: Foo), but we also encounter a solution -- T = i32. This then enables (on the second round) us to uncover that S<i32>: Foo is another solution. (Indeed, there are infinitely many, iirc.)

Copy link
Contributor Author

@arielb1 arielb1 Jun 29, 2017

Choose a reason for hiding this comment

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

The current logic actually reports an overflow error in this case, which aborts compilation. Maybe it's better to do that here too?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we return "OK" for the "evaluation" result, but abort in the fulfillment cx..? If we aborted in both, it might cause trouble? Maybe worth a try though.

Copy link
Contributor

Choose a reason for hiding this comment

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

The other option is EvaluatedToAmbiguity.

}
}

match self.candidate_from_obligation(stack) {
@@ -720,6 +730,33 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
}
}

/// For defaulted traits, we use a co-inductive strategy to solve, so
/// that recursion is ok. This routine returns true if the top of the
/// stack (`cycle[0]`):
/// - is a defaulted trait, and
/// - it also appears in the backtrace at some position `X`; and,
/// - all the predicates at positions `X..` between `X` an the top are
/// also defaulted traits.
pub fn coinductive_match<I>(&mut self, cycle: I) -> bool
where I: Iterator<Item=ty::Predicate<'tcx>>
{
let mut cycle = cycle;
cycle.all(|predicate| self.coinductive_predicate(predicate))
}

fn coinductive_predicate(&self, predicate: ty::Predicate<'tcx>) -> bool {
let result = match predicate {
ty::Predicate::Trait(ref data) => {
self.tcx().trait_has_default_impl(data.def_id())
}
_ => {
false
}
};
debug!("coinductive_predicate({:?}) = {:?}", predicate, result);
result
}

/// Further evaluate `candidate` to decide whether all type parameters match and whether nested
/// obligations are met. Returns true if `candidate` remains viable after this further
/// scrutiny.