refactor(hydroflow_lang): update topo-sort to detect cycles (#1512)

hydroflow_lang/src/graph/flat_to_partitioned.rs

@@ -165,7 +165,8 @@ fn make_subgraph_collect(
                     !matches!(pred, GraphNode::Handoff { .. })
                 })
         },
-    );
+    )
+    .expect("Subgraphs are in-out trees.");
 
     let mut grouped_nodes: SecondaryMap<GraphNodeId, Vec<GraphNodeId>> = Default::default();
     for node_id in topo_sort {

hydroflow_lang/src/graph/graph_algorithms.rs

@@ -23,30 +23,38 @@ where
     let topo_sort_order = {
         // Condensed each SCC into a single node for toposort.
         let mut condensed_preds: BTreeMap<Id, Vec<Id>> = Default::default();
-        for u in (nodes_fn)() {
-            condensed_preds
-                .entry(scc[&u])
-                .or_default()
-                .extend((preds_fn)(u).into_iter().map(|v| scc[&v]));
+        for v in (nodes_fn)() {
+            let v = scc[&v];
+            condensed_preds.entry(v).or_default().extend(
+                (preds_fn)(v)
+                    .into_iter()
+                    .map(|u| scc[&u])
+                    .filter(|&u| v != u),
+            );
         }
 
         topo_sort((nodes_fn)(), |v| {
             condensed_preds.get(&v).into_iter().flatten().cloned()
         })
+        .map_err(drop)
+        .expect("No cycles after SCC condensing.")
     };
     topo_sort_order
 }
 
 /// Topologically sorts a set of nodes. Returns a list where the order of `Id`s will agree with
 /// the order of any path through the graph.
 ///
-/// This naturally requires a directed acyclic graph (DAG).
+/// This succeeds if the input is a directed acyclic graph (DAG).
+///
+/// If the input has a cycle, an `Err` will be returned containing the cycle. Each node in the
+/// cycle will be listed exactly once.
 ///
 /// <https://en.wikipedia.org/wiki/Topological_sorting>
 pub fn topo_sort<Id, NodeIds, PredsFn, PredsIter>(
     node_ids: NodeIds,
     mut preds_fn: PredsFn,
-) -> Vec<Id>
+) -> Result<Vec<Id>, Vec<Id>>
 where
     Id: Copy + Eq + Ord,
     NodeIds: IntoIterator<Item = Id>,
@@ -58,28 +66,49 @@ where
     fn pred_dfs_postorder<Id, PredsFn, PredsIter>(
         node_id: Id,
         preds_fn: &mut PredsFn,
-        marked: &mut BTreeSet<Id>,
+        marked: &mut BTreeMap<Id, bool>, // `false` => temporary, `true` => permanent.
         order: &mut Vec<Id>,
-    ) where
+    ) -> Result<(), ()>
+    where
         Id: Copy + Eq + Ord,
         PredsFn: FnMut(Id) -> PredsIter,
         PredsIter: IntoIterator<Item = Id>,
     {
-        if marked.insert(node_id) {
-            for next_pred in (preds_fn)(node_id) {
-                pred_dfs_postorder(next_pred, preds_fn, marked, order);
+        match marked.get(&node_id) {
+            Some(_permanent @ true) => Ok(()),
+            Some(_temporary @ false) => {
+                // Cycle found!
+                order.clear();
+                order.push(node_id);
+                Err(())
+            }
+            None => {
+                marked.insert(node_id, false);
+                for next_pred in (preds_fn)(node_id) {
+                    pred_dfs_postorder(next_pred, preds_fn, marked, order).map_err(|()| {
+                        if order.len() == 1 || order.first().unwrap() != order.last().unwrap() {
+                            order.push(node_id);
+                        }
+                    })?;
+                }
+                order.push(node_id);
+                marked.insert(node_id, true);
+                Ok(())
             }
-            order.push(node_id);
-        } else {
-            // TODO(mingwei): cycle found!
         }
     }
 
     for node_id in node_ids {
-        pred_dfs_postorder(node_id, &mut preds_fn, &mut marked, &mut order);
+        if pred_dfs_postorder(node_id, &mut preds_fn, &mut marked, &mut order).is_err() {
+            // Cycle found.
+            let end = order.last().unwrap();
+            let beg = order.iter().position(|n| n == end).unwrap();
+            order.drain(0..=beg);
+            return Err(order);
+        }
     }
 
-    order
+    Ok(order)
 }
 
 /// Finds the strongly connected components in the graph. A strongly connected component is a
@@ -156,6 +185,8 @@ where
 
 #[cfg(test)]
 mod test {
+    use itertools::Itertools;
+
     use super::*;
 
     #[test]
@@ -179,6 +210,13 @@ mod test {
                 .filter(move |&&(_, dst)| v == dst)
                 .map(|&(src, _)| src)
         });
+        assert!(
+            sort.is_ok(),
+            "Did not expect cycle: {:?}",
+            sort.unwrap_err()
+        );
+
+        let sort = sort.unwrap();
         println!("{:?}", sort);
 
         let position: BTreeMap<_, _> = sort.iter().enumerate().map(|(i, &x)| (x, i)).collect();
@@ -187,6 +225,54 @@ mod test {
         }
     }
 
+    #[test]
+    pub fn test_toposort_cycle() {
+        // https://commons.wikimedia.org/wiki/File:Directed_graph,_cyclic.svg
+        //          ┌────►C──────┐
+        //          │            │
+        //          │            ▼
+        // A───────►B            E ─────►F
+        //          ▲            │
+        //          │            │
+        //          └─────D◄─────┘
+        let edges = [
+            ('A', 'B'),
+            ('B', 'C'),
+            ('C', 'E'),
+            ('D', 'B'),
+            ('E', 'F'),
+            ('E', 'D'),
+        ];
+        let ids = edges
+            .iter()
+            .flat_map(|&(a, b)| [a, b])
+            .collect::<BTreeSet<_>>();
+        let cycle_rotations = BTreeSet::from_iter([
+            ['B', 'C', 'E', 'D'],
+            ['C', 'E', 'D', 'B'],
+            ['E', 'D', 'B', 'C'],
+            ['D', 'B', 'C', 'E'],
+        ]);
+
+        let permutations = ids.iter().copied().permutations(ids.len());
+        for permutation in permutations {
+            let result = topo_sort(permutation.iter().copied(), |v| {
+                edges
+                    .iter()
+                    .filter(move |&&(_, dst)| v == dst)
+                    .map(|&(src, _)| src)
+            });
+            assert!(result.is_err());
+            let cycle = result.unwrap_err();
+            assert!(
+                cycle_rotations.contains(&*cycle),
+                "cycle: {:?}, vertex order: {:?}",
+                cycle,
+                permutation
+            );
+        }
+    }
+
     #[test]
     pub fn test_scc_kosaraju() {
         // https://commons.wikimedia.org/wiki/File:Scc-1.svg