From f832feb3fa6651ec769453bd8f0b154c42388460 Mon Sep 17 00:00:00 2001
From: Mike Gerwitz <mike.gerwitz@ryansg.com>
Date: Wed, 1 Jul 2020 11:02:20 -0400
Subject: [PATCH] [DEV-8000] ir::asg::base::BaseAsg::check_cycles: Extract into
 function

The only reason this function was a method of `BaseAsg` was because of
`self.graph`, which is accessible within the scope of this
module.  `check_cycles` is logically associated with `SortableAsg`, and so
should exist alongside it (though it can't exist as an associated function
of that trait).
---
 tamer/src/ir/asg/base.rs | 106 ++++++++++++++++++++-------------------
 1 file changed, 55 insertions(+), 51 deletions(-)

diff --git a/tamer/src/ir/asg/base.rs b/tamer/src/ir/asg/base.rs
index 3ae8f00f..e10f5f3d 100644
--- a/tamer/src/ir/asg/base.rs
+++ b/tamer/src/ir/asg/base.rs
@@ -197,56 +197,6 @@ where
                 Err(err.into())
             })
     }
-
-    /// Check graph for cycles
-    ///
-    /// We want to catch any cycles before we start using the graph.
-    ///   Unfortunately, we need to allow cycles for our [`IdentKind::Func`]
-    ///   so we cannot use the typical algorithms in a straightforward manner.
-    ///
-    /// We loop through all SCCs and check that they are not all functions. If
-    ///   they are, we ignore the cycle, otherwise we will return an error.
-    fn check_cycles(&self) -> AsgResult<(), Ix> {
-        // While `tarjan_scc` does do a topological sort, it does not suit our
-        // needs because we need to filter out some allowed cycles. It would
-        // still be possible to use this, but we also need to only check nodes
-        // that are attached to our "roots". We are doing our own sort and as of
-        // the initial writing, this does not have a significant performance
-        // impact.
-        let sccs = petgraph::algo::tarjan_scc(&self.graph);
-
-        let cycles: Vec<_> = sccs
-            .into_iter()
-            .filter_map(|scc| {
-                // For single-node SCCs, we just need to make sure they are
-                // not neighbors with themselves.
-                if scc.len() == 1
-                    && !self.graph.neighbors(scc[0]).any(|nx| nx == scc[0])
-                {
-                    return None;
-                }
-
-                let is_all_funcs = scc.iter().all(|nx| {
-                    let ident = self.get(*nx).expect("missing node");
-                    matches!(ident.kind(), Some(IdentKind::Func(_, _)))
-                });
-
-                if is_all_funcs {
-                    None
-                } else {
-                    let cycles =
-                        scc.iter().map(|nx| ObjectRef::from(*nx)).collect();
-                    Some(cycles)
-                }
-            })
-            .collect();
-
-        if cycles.is_empty() {
-            Ok(())
-        } else {
-            Err(AsgError::Cycles(cycles))
-        }
-    }
 }
 
 impl<'i, O, Ix> Asg<'i, O, Ix> for BaseAsg<O, Ix>
@@ -334,7 +284,7 @@ where
     ) -> AsgResult<Sections<'i, O>, Ix> {
         let mut deps = Sections::new();
 
-        self.check_cycles()?;
+        check_cycles(self)?;
 
         // This is technically a topological sort, but functions have cycles.
         let mut dfs = DfsPostOrder::empty(&self.graph);
@@ -374,6 +324,60 @@ where
     }
 }
 
+/// Check graph for cycles
+///
+/// We want to catch any cycles before we start using the graph.
+///   Unfortunately, we need to allow cycles for our [`IdentKind::Func`]
+///   so we cannot use the typical algorithms in a straightforward manner.
+///
+/// We loop through all SCCs and check that they are not all functions. If
+///   they are, we ignore the cycle, otherwise we will return an error.
+fn check_cycles<'i, O, Ix>(asg: &BaseAsg<O, Ix>) -> AsgResult<(), Ix>
+where
+    Ix: IndexType,
+    O: IdentObjectData<'i> + IdentObjectState<'i, O>,
+{
+    // While `tarjan_scc` does do a topological sort, it does not suit our
+    // needs because we need to filter out some allowed cycles. It would
+    // still be possible to use this, but we also need to only check nodes
+    // that are attached to our "roots". We are doing our own sort and as of
+    // the initial writing, this does not have a significant performance
+    // impact.
+    let sccs = petgraph::algo::tarjan_scc(&asg.graph);
+
+    let cycles: Vec<_> = sccs
+        .into_iter()
+        .filter_map(|scc| {
+            // For single-node SCCs, we just need to make sure they are
+            // not neighbors with themselves.
+            if scc.len() == 1
+                && !asg.graph.neighbors(scc[0]).any(|nx| nx == scc[0])
+            {
+                return None;
+            }
+
+            let is_all_funcs = scc.iter().all(|nx| {
+                let ident = Asg::get(asg, *nx).expect("missing node");
+                matches!(ident.kind(), Some(IdentKind::Func(_, _)))
+            });
+
+            if is_all_funcs {
+                None
+            } else {
+                let cycles =
+                    scc.iter().map(|nx| ObjectRef::from(*nx)).collect();
+                Some(cycles)
+            }
+        })
+        .collect();
+
+    if cycles.is_empty() {
+        Ok(())
+    } else {
+        Err(AsgError::Cycles(cycles))
+    }
+}
+
 #[cfg(test)]
 mod test {
     use super::super::graph::AsgError;