diff --git a/crates/compute_graph/src/builder.rs b/crates/compute_graph/src/builder.rs index 92253ec..1ceb63b 100644 --- a/crates/compute_graph/src/builder.rs +++ b/crates/compute_graph/src/builder.rs @@ -1,12 +1,12 @@ use crate::node::{ - AsyncConstNode, AsyncRuleNode, ConstNode, InvalidatableConstNode, Node, NodeValue, RuleNode, + AsyncConstNode, AsyncRuleNode, ConstNode, ErasedNode, InvalidatableConstNode, Node, NodeValue, + RuleNode, }; -use crate::rule::{AsyncRule, Rule}; +use crate::rule::{AsyncRule, Input, Rule}; +use crate::synchronicity::{Asynchronous, Synchronicity, Synchronous}; use crate::util; -use crate::{ - Asynchronous, ErasedNode, Graph, Input, InvalidationSignal, NodeGraph, NodeId, Synchronicity, - Synchronous, ValueInvalidationSignal, -}; +use crate::{Graph, InvalidationSignal, NodeGraph, NodeId, ValueInvalidationSignal}; +use petgraph::visit::{EdgeRef, IntoEdgesDirected}; use std::cell::{Cell, RefCell}; use std::future::Future; use std::rc::Rc; @@ -222,9 +222,13 @@ impl GraphBuilder { drop(graph); let sorted_nodes = - petgraph::algo::toposort(&**self.node_graph.borrow(), None).map_err(|_| { - // TODO: actually build a vec describing the cycle path for debugging - BuildGraphError::Cycle(vec![]) + petgraph::algo::toposort(&**self.node_graph.borrow(), None).map_err(|cycle| { + let start_node = cycle.node_id(); + // Do a depth-first search of the dependencies of start_node until we get back to start_node. + let mut cycle = + find_cycle(&self.node_graph.borrow().0, start_node, start_node).unwrap(); + cycle.push(start_node); + BuildGraphError::Cycle(cycle) })?; let graph = Graph { @@ -239,6 +243,23 @@ impl GraphBuilder { } } +fn find_cycle( + graph: G, + current_node: G::NodeId, + target_node: G::NodeId, +) -> Option> { + for dep in graph.edges_directed(current_node, petgraph::Direction::Incoming) { + let node = dep.source(); + if node == target_node { + return Some(vec![node]); + } else if let Some(mut path) = find_cycle(graph, node, target_node) { + path.push(node); + return Some(path); + } + } + None +} + impl GraphBuilder { /// Sets an asynchronous rule for the output node. /// @@ -311,3 +332,56 @@ pub enum BuildGraphError { /// the path on which the cycle was found. Cycle(Vec), } + +#[cfg(test)] +mod tests { + use crate::tests::{DeferredInput, Double}; + use std::cell::RefCell; + use std::rc::Rc; + + #[test] + fn find_cycle() { + let mut graph = petgraph::graph::DiGraph::new(); + let a = graph.add_node(()); + let b = graph.add_node(()); + let c = graph.add_node(()); + let d = graph.add_node(()); + graph.add_edge(a, b, ()); + graph.add_edge(a, c, ()); + graph.add_edge(b, d, ()); + graph.add_edge(d, a, ()); + assert!(petgraph::algo::is_cyclic_directed(&graph)); + assert_eq!(super::find_cycle(&graph, a, a), Some(vec![a, b, d])); + } + + #[test] + fn cant_build_no_output() { + let builder = super::GraphBuilder::::new(); + match builder.build() { + Err(super::BuildGraphError::NoOutput) => (), + Err(e) => assert!(false, "unexpected error {:?}", e), + Ok(_) => assert!(false, "shouldn't have built graph"), + } + } + + #[test] + fn cant_build_cycle() { + let mut builder = super::GraphBuilder::new(); + let a_input = Rc::new(RefCell::new(None)); + let a = builder.add_rule(DeferredInput::new(Rc::clone(&a_input))); + let b_input = Rc::new(RefCell::new(Some(a.clone()))); + let b = builder.add_rule(DeferredInput::new(b_input)); + *a_input.borrow_mut() = Some(b.clone()); + builder.set_output(Double::new(b.clone())); + match builder.build() { + Err(super::BuildGraphError::Cycle(cycle)) => { + let a_start = cycle == vec![a.node_idx, b.node_idx, a.node_idx]; + let b_start = cycle == vec![b.node_idx, a.node_idx, b.node_idx]; + // either is a permisisble way of describing the cycle + assert!(a_start || b_start); + } + Err(e) => assert!(false, "unexpected error {:?}", e), + Ok(_) => assert!(false, "shouldn't have built graph"), + } + } +} diff --git a/crates/compute_graph/src/lib.rs b/crates/compute_graph/src/lib.rs index 5933583..cc39f12 100644 --- a/crates/compute_graph/src/lib.rs +++ b/crates/compute_graph/src/lib.rs @@ -386,7 +386,12 @@ mod tests { assert!(graph.is_output_valid()); } - struct Double(Input); + pub(crate) struct Double(Input); + impl Double { + pub(crate) fn new(input: Input) -> Self { + Self(input) + } + } impl InputVisitable for Double { fn visit_inputs(&self, visitor: &mut impl InputVisitor) { visitor.visit(&self.0); @@ -478,17 +483,12 @@ mod tests { assert_eq!(*graph.evaluate(), 3); } - #[test] - fn cant_freeze_no_output() { - let builder = GraphBuilder::::new(); - match builder.build() { - Err(BuildGraphError::NoOutput) => (), - Err(e) => assert!(false, "unexpected error {:?}", e), - Ok(_) => assert!(false, "shouldn't have frozen graph"), + pub(crate) struct DeferredInput(Rc>>>); + impl DeferredInput { + pub(crate) fn new(value: Rc>>>) -> Self { + Self(value) } } - - struct DeferredInput(Rc>>>); impl InputVisitable for DeferredInput { fn visit_inputs(&self, visitor: &mut impl InputVisitor) { let borrowed = self.0.borrow(); @@ -503,22 +503,6 @@ mod tests { } } - #[test] - fn cant_freeze_cycle() { - let mut builder = GraphBuilder::new(); - let a_input = Rc::new(RefCell::new(None)); - let a = builder.add_rule(DeferredInput(Rc::clone(&a_input))); - let b_input = Rc::new(RefCell::new(Some(a))); - let b = builder.add_rule(DeferredInput(b_input)); - *a_input.borrow_mut() = Some(b.clone()); - builder.set_output(Double(b)); - match builder.build() { - Err(BuildGraphError::Cycle(_)) => (), - Err(e) => assert!(false, "unexpected error {:?}", e), - Ok(_) => assert!(false, "shouldn't have frozen graph"), - } - } - #[test] fn modify_graph() { let mut builder = GraphBuilder::new();