Skip to content

Commit

Permalink
address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@arielb1
arielb1 committed May 14, 2016
1 parent 5c39a2a commit f0f5ef5
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 62 deletions.
15 changes: 9 additions & 6 deletions src/librustc/traits/fulfill.rs
View file
Expand Up @@ -13,6 +13,7 @@ use infer::{InferCtxt, InferOk};
use ty::{self, Ty, TypeFoldable, ToPolyTraitRef, TyCtxt};
use rustc_data_structures::obligation_forest::{ObligationForest, Error};
use rustc_data_structures::obligation_forest::{ForestObligation, ObligationProcessor};
use std::marker::PhantomData;
use std::mem;
use syntax::ast;
use util::common::ErrorReported;
Expand Down Expand Up @@ -314,13 +315,14 @@ impl<'a, 'b, 'gcx, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'b, 'gcx,
}).collect()))
}

fn process_backedge<'c, I>(&mut self, cycle: I)
where I: Clone + Iterator<Item=*const Self::Obligation>,
fn process_backedge<'c, I>(&mut self, cycle: I,
_marker: PhantomData<&'c PendingPredicateObligation<'tcx>>)
where I: Clone + Iterator<Item=&'c PendingPredicateObligation<'tcx>>,
{
if coinductive_match(self.selcx, cycle.clone()) {
debug!("process_child_obligations: coinductive match");
} else {
let cycle : Vec<_> = cycle.map(|c| unsafe { &*c }.obligation.clone()).collect();
let cycle : Vec<_> = cycle.map(|c| c.obligation.clone()).collect();
self.selcx.infcx().report_overflow_error_cycle(&cycle);
}
}
Expand Down Expand Up @@ -536,13 +538,14 @@ fn process_predicate<'a, 'gcx, 'tcx>(
/// - 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,'gcx,'tcx,I>(selcx: &mut SelectionContext<'a,'gcx,'tcx>, cycle: I) -> bool
where I: Iterator<Item=*const PendingPredicateObligation<'tcx>>
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 bt_obligation = unsafe { &*bt_obligation };
let result = coinductive_obligation(selcx, &bt_obligation.obligation);
debug!("coinductive_match: bt_obligation={:?} coinductive={}",
bt_obligation, result);
Expand Down
126 changes: 75 additions & 51 deletions src/librustc_data_structures/obligation_forest/mod.rs
View file
Expand Up @@ -21,15 +21,16 @@ use std::cell::Cell;
use std::collections::hash_map::Entry;
use std::fmt::Debug;
use std::hash;
use std::marker::PhantomData;

mod node_index;
use self::node_index::NodeIndex;

#[cfg(test)]
mod test;

pub trait ForestObligation : Clone {
type Predicate : Clone + hash::Hash + Eq + ::std::fmt::Debug;
pub trait ForestObligation : Clone + Debug {
type Predicate : Clone + hash::Hash + Eq + Debug;

fn as_predicate(&self) -> &Self::Predicate;
}
Expand All @@ -42,9 +43,9 @@ pub trait ObligationProcessor {
obligation: &mut Self::Obligation)
-> Result<Option<Vec<Self::Obligation>>, Self::Error>;

// FIXME: crazy lifetime troubles
fn process_backedge<I>(&mut self, cycle: I)
where I: Clone + Iterator<Item=*const Self::Obligation>;
fn process_backedge<'c, I>(&mut self, cycle: I,
_marker: PhantomData<&'c Self::Obligation>)
where I: Clone + Iterator<Item=&'c Self::Obligation>;
}

struct SnapshotData {
Expand All @@ -66,8 +67,12 @@ pub struct ObligationForest<O: ForestObligation> {
/// at a higher index than its parent. This is needed by the
/// backtrace iterator (which uses `split_at`).
nodes: Vec<Node<O>>,
/// A cache of predicates that have been successfully completed.
done_cache: FnvHashSet<O::Predicate>,
/// An cache of the nodes in `nodes`, indexed by predicate.
waiting_cache: FnvHashMap<O::Predicate, NodeIndex>,
/// A list of the obligations added in snapshots, to allow
/// for their removal.
cache_list: Vec<O::Predicate>,
snapshots: Vec<SnapshotData>,
scratch: Option<Vec<usize>>,
Expand All @@ -82,33 +87,33 @@ struct Node<O> {
obligation: O,
state: Cell<NodeState>,

// these both go *in the same direction*.
/// Obligations that depend on this obligation for their
/// completion. They must all be in a non-pending state.
dependents: Vec<NodeIndex>,
/// The parent of a node - the original obligation of
/// which it is a subobligation. Except for error reporting,
/// this is just another member of `dependents`.
parent: Option<NodeIndex>,
dependants: Vec<NodeIndex>,
}

/// The state of one node in some tree within the forest. This
/// represents the current state of processing for the obligation (of
/// type `O`) associated with this node.
///
/// Outside of ObligationForest methods, nodes should be either Pending
/// or Waiting.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum NodeState {
/// Obligation not yet resolved to success or error.
/// Obligations for which selection had not yet returned a
/// non-ambiguous result.
Pending,

/// Used before garbage collection
/// This obligation was selected successfuly, but may or
/// may not have subobligations.
Success,

/// Used in DFS loops
InLoop,

/// Obligation resolved to success; `num_incomplete_children`
/// indicates the number of children still in an "incomplete"
/// state. Incomplete means that either the child is still
/// pending, or it has children which are incomplete. (Basically,
/// there is pending work somewhere in the subtree of the child.)
///
/// Once all children have completed, success nodes are removed
/// from the vector by the compression step.
/// This obligation was selected sucessfully, but it has
/// a pending subobligation.
Waiting,

/// This obligation, along with its subobligations, are complete,
Expand All @@ -118,6 +123,10 @@ enum NodeState {
/// This obligation was resolved to an error. Error nodes are
/// removed from the vector by the compression step.
Error,

/// This is a temporary state used in DFS loops to detect cycles,
/// it should not exist outside of these DFSes.
OnDfsStack,
}

#[derive(Debug)]
Expand All @@ -144,7 +153,7 @@ pub struct Error<O, E> {
pub backtrace: Vec<O>,
}

impl<O: Debug + ForestObligation> ObligationForest<O> {
impl<O: ForestObligation> ObligationForest<O> {
pub fn new() -> ObligationForest<O> {
ObligationForest {
nodes: vec![],
Expand Down Expand Up @@ -210,7 +219,12 @@ impl<O: Debug + ForestObligation> ObligationForest<O> {
debug!("register_obligation_at({:?}, {:?}) - duplicate of {:?}!",
obligation, parent, o.get());
if let Some(parent) = parent {
self.nodes[o.get().get()].dependants.push(parent);
if self.nodes[o.get().get()].dependents.contains(&parent) {
debug!("register_obligation_at({:?}, {:?}) - duplicate subobligation",
obligation, parent);
} else {
self.nodes[o.get().get()].dependents.push(parent);
}
}
}
Entry::Vacant(v) => {
Expand All @@ -230,7 +244,6 @@ impl<O: Debug + ForestObligation> ObligationForest<O> {
assert!(!self.in_snapshot());
let mut errors = vec![];
for index in 0..self.nodes.len() {
debug_assert!(!self.nodes[index].is_popped());
if let NodeState::Pending = self.nodes[index].state.get() {
let backtrace = self.error_at(index);
errors.push(Error {
Expand Down Expand Up @@ -269,8 +282,6 @@ impl<O: Debug + ForestObligation> ObligationForest<O> {
let mut stalled = true;

for index in 0..self.nodes.len() {
debug_assert!(!self.nodes[index].is_popped());

debug!("process_obligations: node {} == {:?}",
index,
self.nodes[index]);
Expand Down Expand Up @@ -327,57 +338,69 @@ impl<O: Debug + ForestObligation> ObligationForest<O> {
}
}

pub fn process_cycles<P>(&mut self, processor: &mut P)
/// Mark all NodeState::Success nodes as NodeState::Done and
/// report all cycles between them. This should be called
/// after `mark_as_waiting` marks all nodes with pending
/// subobligations as NodeState::Waiting.
fn process_cycles<P>(&mut self, processor: &mut P)
where P: ObligationProcessor<Obligation=O>
{
let mut stack = self.scratch.take().unwrap();

for node in 0..self.nodes.len() {
self.visit_node(&mut stack, processor, node);
self.find_cycles_from_node(&mut stack, processor, node);
}

self.scratch = Some(stack);
}

fn visit_node<P>(&self, stack: &mut Vec<usize>, processor: &mut P, index: usize)
fn find_cycles_from_node<P>(&self, stack: &mut Vec<usize>,
processor: &mut P, index: usize)
where P: ObligationProcessor<Obligation=O>
{
let node = &self.nodes[index];
let state = node.state.get();
match state {
NodeState::InLoop => {
NodeState::OnDfsStack => {
let index =
stack.iter().rposition(|n| *n == index).unwrap();
// I need a Clone closure
#[derive(Clone)]
struct GetObligation<'a, O: 'a>(&'a [Node<O>]);
impl<'a, 'b, O> FnOnce<(&'b usize,)> for GetObligation<'a, O> {
type Output = *const O;
extern "rust-call" fn call_once(self, args: (&'b usize,)) -> *const O {
type Output = &'a O;
extern "rust-call" fn call_once(self, args: (&'b usize,)) -> &'a O {
&self.0[*args.0].obligation
}
}
impl<'a, 'b, O> FnMut<(&'b usize,)> for GetObligation<'a, O> {
extern "rust-call" fn call_mut(&mut self, args: (&'b usize,)) -> *const O {
extern "rust-call" fn call_mut(&mut self, args: (&'b usize,)) -> &'a O {
&self.0[*args.0].obligation
}
}

processor.process_backedge(stack[index..].iter().map(GetObligation(&self.nodes)));
processor.process_backedge(stack[index..].iter().map(GetObligation(&self.nodes)),
PhantomData);
}
NodeState::Success => {
node.state.set(NodeState::InLoop);
node.state.set(NodeState::OnDfsStack);
stack.push(index);
if let Some(parent) = node.parent {
self.visit_node(stack, processor, parent.get());
self.find_cycles_from_node(stack, processor, parent.get());
}
for dependant in &node.dependants {
self.visit_node(stack, processor, dependant.get());
for dependent in &node.dependents {
self.find_cycles_from_node(stack, processor, dependent.get());
}
stack.pop();
node.state.set(NodeState::Done);
},
_ => return
NodeState::Waiting | NodeState::Pending => {
// this node is still reachable from some pending node. We
// will get to it when they are all processed.
}
NodeState::Done | NodeState::Error => {
// already processed that node
}
};
}

Expand All @@ -391,7 +414,7 @@ impl<O: Debug + ForestObligation> ObligationForest<O> {
loop {
self.nodes[n].state.set(NodeState::Error);
trace.push(self.nodes[n].obligation.clone());
error_stack.extend(self.nodes[n].dependants.iter().map(|x| x.get()));
error_stack.extend(self.nodes[n].dependents.iter().map(|x| x.get()));

// loop to the parent
match self.nodes[n].parent {
Expand All @@ -415,15 +438,15 @@ impl<O: Debug + ForestObligation> ObligationForest<O> {
}

error_stack.extend(
node.dependants.iter().cloned().chain(node.parent).map(|x| x.get())
node.dependents.iter().cloned().chain(node.parent).map(|x| x.get())
);
}

self.scratch = Some(error_stack);
trace
}

/// Marks all nodes that depend on a pending node as "waiting".
/// Marks all nodes that depend on a pending node as NodeState;:Waiting.
fn mark_as_waiting(&self) {
for node in &self.nodes {
if node.state.get() == NodeState::Waiting {
Expand All @@ -441,7 +464,7 @@ impl<O: Debug + ForestObligation> ObligationForest<O> {
fn mark_as_waiting_from(&self, node: &Node<O>) {
match node.state.get() {
NodeState::Pending | NodeState::Done => {},
NodeState::Waiting | NodeState::Error | NodeState::InLoop => return,
NodeState::Waiting | NodeState::Error | NodeState::OnDfsStack => return,
NodeState::Success => {
node.state.set(NodeState::Waiting);
}
Expand All @@ -451,8 +474,8 @@ impl<O: Debug + ForestObligation> ObligationForest<O> {
self.mark_as_waiting_from(&self.nodes[parent.get()]);
}

for dependant in &node.dependants {
self.mark_as_waiting_from(&self.nodes[dependant.get()]);
for dependent in &node.dependents {
self.mark_as_waiting_from(&self.nodes[dependent.get()]);
}
}

Expand Down Expand Up @@ -546,12 +569,12 @@ impl<O: Debug + ForestObligation> ObligationForest<O> {
}

let mut i = 0;
while i < node.dependants.len() {
let new_index = node_rewrites[node.dependants[i].get()];
while i < node.dependents.len() {
let new_index = node_rewrites[node.dependents[i].get()];
if new_index >= nodes_len {
node.dependants.swap_remove(i);
node.dependents.swap_remove(i);
} else {
node.dependants[i] = NodeIndex::new(new_index);
node.dependents[i] = NodeIndex::new(new_index);
i += 1;
}
}
Expand All @@ -577,14 +600,15 @@ impl<O> Node<O> {
obligation: obligation,
parent: parent,
state: Cell::new(NodeState::Pending),
dependants: vec![],
dependents: vec![],
}
}

fn is_popped(&self) -> bool {
match self.state.get() {
NodeState::Pending | NodeState::Success | NodeState::Waiting => false,
NodeState::Error | NodeState::Done | NodeState::InLoop => true,
NodeState::Pending | NodeState::Waiting => false,
NodeState::Error | NodeState::Done => true,
NodeState::OnDfsStack | NodeState::Success => unreachable!()
}
}
}
11 changes: 6 additions & 5 deletions src/librustc_data_structures/obligation_forest/test.rs
View file
Expand Up @@ -25,7 +25,7 @@ impl<'a> super::ForestObligation for &'a str {

struct ClosureObligationProcessor<OF, BF, O, E> {
process_obligation: OF,
process_backedge: BF,
_process_backedge: BF,
marker: PhantomData<(O, E)>,
}

Expand All @@ -36,7 +36,7 @@ fn C<OF, BF, O>(of: OF, bf: BF) -> ClosureObligationProcessor<OF, BF, O, &'stati
{
ClosureObligationProcessor {
process_obligation: of,
process_backedge: bf,
_process_backedge: bf,
marker: PhantomData
}
}
Expand All @@ -57,9 +57,10 @@ impl<OF, BF, O, E> ObligationProcessor for ClosureObligationProcessor<OF, BF, O,
(self.process_obligation)(obligation)
}

fn process_backedge(&mut self, cycle: &[Self::Obligation]) {
(self.process_backedge)(cycle);
}
fn process_backedge<'c, I>(&mut self, _cycle: I,
_marker: PhantomData<&'c Self::Obligation>)
where I: Clone + Iterator<Item=&'c Self::Obligation> {
}
}


Expand Down

0 comments on commit f0f5ef5

Please sign in to comment.