Better cycle error

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<O: 'static, S: Synchronicity> GraphBuilder<O, S> {
         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<O: 'static, S: Synchronicity> GraphBuilder<O, S> {
     }
 }
 
+fn find_cycle<G: IntoEdgesDirected>(
+    graph: G,
+    current_node: G::NodeId,
+    target_node: G::NodeId,
+) -> Option<Vec<G::NodeId>> {
+    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<O: 'static> GraphBuilder<O, Asynchronous> {
     /// 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<NodeId>),
 }
+
+#[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::<i32, crate::synchronicity::Synchronous>::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"),
+        }
+    }
+}

crates/compute_graph/src/lib.rs

@@ -386,7 +386,12 @@ mod tests {
         assert!(graph.is_output_valid());
     }
 
-    struct Double(Input<i32>);
+    pub(crate) struct Double(Input<i32>);
+    impl Double {
+        pub(crate) fn new(input: Input<i32>) -> 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::<i32, Synchronous>::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<RefCell<Option<Input<i32>>>>);
+    impl DeferredInput {
+        pub(crate) fn new(value: Rc<RefCell<Option<Input<i32>>>>) -> Self {
+            Self(value)
         }
     }
-
-    struct DeferredInput(Rc<RefCell<Option<Input<i32>>>>);
     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();