Add find_shortest_cycle

rust/src/graph/cycles.rs

@@ -0,0 +1,29 @@
+use crate::errors::GrimpResult;
+use crate::graph::pathfinding;
+use crate::graph::{ExtendWithDescendants, Graph, ModuleToken};
+use rustc_hash::{FxHashMap, FxHashSet};
+use tap::Conv;
+
+impl Graph {
+    pub fn find_shortest_cycle(
+        &self,
+        module: ModuleToken,
+        as_package: bool,
+    ) -> GrimpResult<Option<Vec<ModuleToken>>> {
+        if as_package {
+            pathfinding::find_shortest_cycle(
+                self,
+                &module.conv::<FxHashSet<_>>().with_descendants(self),
+                &FxHashSet::default(),
+                &FxHashMap::default(),
+            )
+        } else {
+            pathfinding::find_shortest_cycle(
+                self,
+                &module.conv::<FxHashSet<_>>(),
+                &FxHashSet::default(),
+                &FxHashMap::default(),
+            )
+        }
+    }
+}

rust/src/graph/import_chain_queries.rs

@@ -1,5 +1,5 @@
 use crate::errors::GrimpResult;
-use crate::graph::pathfinding::{find_reach, find_shortest_path_bidirectional};
+use crate::graph::pathfinding::{find_reach, find_shortest_path};
 use crate::graph::{ExtendWithDescendants, Graph, ModuleToken};
 use itertools::Itertools;
 use rustc_hash::{FxHashMap, FxHashSet};
@@ -62,7 +62,7 @@ impl Graph {
         excluded_modules: &FxHashSet<ModuleToken>,
         excluded_imports: &FxHashMap<ModuleToken, FxHashSet<ModuleToken>>,
     ) -> GrimpResult<Option<Vec<ModuleToken>>> {
-        find_shortest_path_bidirectional(
+        find_shortest_path(
             self,
             from_modules,
             to_modules,

rust/src/graph/mod.rs

@@ -8,6 +8,7 @@ use std::sync::RwLock;
 use string_interner::backend::StringBackend;
 use string_interner::{DefaultSymbol, StringInterner};
 
+pub mod cycles;
 pub mod direct_import_queries;
 pub mod graph_manipulation;
 pub mod hierarchy_queries;

rust/src/graph/pathfinding.rs

@@ -1,6 +1,7 @@
 use crate::errors::{GrimpError, GrimpResult};
 use crate::graph::{Graph, ModuleToken, EMPTY_MODULE_TOKENS};
 use indexmap::{IndexMap, IndexSet};
+use itertools::Itertools;
 use rustc_hash::{FxHashMap, FxHashSet, FxHasher};
 use slotmap::SecondaryMap;
 use std::hash::BuildHasherDefault;
@@ -28,7 +29,8 @@ pub fn find_reach(
     &seen.into_iter().collect::<FxHashSet<_>>() - from_modules
 }
 
-pub fn find_shortest_path_bidirectional(
+/// Finds the shortest path, via a bidirectional BFS.
+pub fn find_shortest_path(
     graph: &Graph,
     from_modules: &FxHashSet<ModuleToken>,
     to_modules: &FxHashSet<ModuleToken>,
@@ -39,6 +41,39 @@ pub fn find_shortest_path_bidirectional(
         return Err(GrimpError::SharedDescendants);
     }
 
+    _find_shortest_path(
+        graph,
+        from_modules,
+        to_modules,
+        excluded_modules,
+        excluded_imports,
+    )
+}
+
+/// Finds the shortest cycle from `modules` to `modules`, via a bidirectional BFS.
+pub fn find_shortest_cycle(
+    graph: &Graph,
+    modules: &FxHashSet<ModuleToken>,
+    excluded_modules: &FxHashSet<ModuleToken>,
+    excluded_imports: &FxHashMap<ModuleToken, FxHashSet<ModuleToken>>,
+) -> GrimpResult<Option<Vec<ModuleToken>>> {
+    // Exclude imports internal to `modules`
+    let mut excluded_imports = excluded_imports.clone();
+    for (m1, m2) in modules.iter().tuple_combinations() {
+        excluded_imports.entry(*m1).or_default().insert(*m2);
+        excluded_imports.entry(*m2).or_default().insert(*m1);
+    }
+
+    _find_shortest_path(graph, modules, modules, excluded_modules, &excluded_imports)
+}
+
+fn _find_shortest_path(
+    graph: &Graph,
+    from_modules: &FxHashSet<ModuleToken>,
+    to_modules: &FxHashSet<ModuleToken>,
+    excluded_modules: &FxHashSet<ModuleToken>,
+    excluded_imports: &FxHashMap<ModuleToken, FxHashSet<ModuleToken>>,
+) -> GrimpResult<Option<Vec<ModuleToken>>> {
     let mut predecessors: FxIndexMap<ModuleToken, Option<ModuleToken>> = from_modules
         .clone()
         .into_iter()
@@ -123,3 +158,105 @@ fn import_is_excluded(
             .contains(to_module)
     }
 }
+
+#[cfg(test)]
+mod test_find_shortest_cycle {
+    use super::*;
+
+    #[test]
+    fn test_finds_cycle_single_module() -> GrimpResult<()> {
+        let mut graph = Graph::default();
+        let foo = graph.get_or_add_module("foo").token;
+        let bar = graph.get_or_add_module("bar").token;
+        let baz = graph.get_or_add_module("baz").token;
+        let x = graph.get_or_add_module("x").token;
+        let y = graph.get_or_add_module("y").token;
+        let z = graph.get_or_add_module("z").token;
+        // Shortest cycle
+        graph.add_import(foo, bar);
+        graph.add_import(bar, baz);
+        graph.add_import(baz, foo);
+        // Longer cycle
+        graph.add_import(foo, x);
+        graph.add_import(x, y);
+        graph.add_import(y, z);
+        graph.add_import(z, foo);
+
+        let path = find_shortest_cycle(
+            &graph,
+            &foo.into(),
+            &FxHashSet::default(),
+            &FxHashMap::default(),
+        )?;
+        assert_eq!(path, Some(vec![foo, bar, baz, foo]));
+
+        graph.remove_import(baz, foo);
+
+        let path = find_shortest_cycle(
+            &graph,
+            &foo.into(),
+            &FxHashSet::default(),
+            &FxHashMap::default(),
+        )?;
+        assert_eq!(path, Some(vec![foo, x, y, z, foo]));
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_returns_none_if_no_cycle() -> GrimpResult<()> {
+        let mut graph = Graph::default();
+        let foo = graph.get_or_add_module("foo").token;
+        let bar = graph.get_or_add_module("bar").token;
+        let baz = graph.get_or_add_module("baz").token;
+        graph.add_import(foo, bar);
+        graph.add_import(bar, baz);
+
+        let path = find_shortest_cycle(
+            &graph,
+            &foo.into(),
+            &FxHashSet::default(),
+            &FxHashMap::default(),
+        )?;
+
+        assert_eq!(path, None);
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_finds_cycle_multiple_module() -> GrimpResult<()> {
+        let mut graph = Graph::default();
+
+        graph.get_or_add_module("colors");
+        let red = graph.get_or_add_module("colors.red").token;
+        let blue = graph.get_or_add_module("colors.blue").token;
+        let a = graph.get_or_add_module("a").token;
+        let b = graph.get_or_add_module("b").token;
+        let c = graph.get_or_add_module("c").token;
+        let d = graph.get_or_add_module("d").token;
+
+        // The computation should not be confused by these two imports internal to `modules`.
+        graph.add_import(red, blue);
+        graph.add_import(blue, red);
+        // This is the part we expect to find.
+        graph.add_import(red, a);
+        graph.add_import(a, b);
+        graph.add_import(b, blue);
+        // A longer path.
+        graph.add_import(a, c);
+        graph.add_import(c, d);
+        graph.add_import(d, b);
+
+        let path = find_shortest_cycle(
+            &graph,
+            &FxHashSet::from_iter([red, blue]),
+            &FxHashSet::default(),
+            &FxHashMap::default(),
+        )?;
+
+        assert_eq!(path, Some(vec![red, a, b, blue]));
+
+        Ok(())
+    }
+}

rust/src/lib.rs

@@ -353,6 +353,25 @@ impl GraphWrapper {
         PySet::new(py, chains)
     }
 
+    #[pyo3(signature = (module, as_package=false))]
+    pub fn find_shortest_cycle(
+        &self,
+        module: &str,
+        as_package: bool,
+    ) -> PyResult<Option<Vec<String>>> {
+        let module = self.get_visible_module_by_name(module)?.token();
+        Ok(self
+            ._graph
+            .find_shortest_cycle(module, as_package)?
+            .map(|chain| {
+                chain
+                    .iter()
+                    .into_module_iterator(&self._graph)
+                    .names()
+                    .collect()
+            }))
+    }
+
     #[pyo3(signature = (layers, containers))]
     pub fn find_illegal_dependencies_for_layers<'py>(
         &self,

src/grimp/adaptors/graph.py

@@ -126,6 +126,15 @@ def find_shortest_chains(
     def chain_exists(self, importer: str, imported: str, as_packages: bool = False) -> bool:
         return self._rustgraph.chain_exists(importer, imported, as_packages)
 
+    def find_shortest_cycle(
+        self, module: str, as_package: bool = False
+    ) -> Optional[Tuple[str, ...]]:
+        if not self._rustgraph.contains_module(module):
+            raise ValueError(f"Module {module} is not present in the graph.")
+
+        cycle = self._rustgraph.find_shortest_cycle(module, as_package)
+        return tuple(cycle) if cycle else None
+
     def find_illegal_dependencies_for_layers(
         self,
         layers: Sequence[Layer | str | set[str]],

src/grimp/application/ports/graph.py

@@ -282,6 +282,22 @@ def chain_exists(self, importer: str, imported: str, as_packages: bool = False)
         """
         raise NotImplementedError
 
+    # Cycles
+    # ------
+
+    @abc.abstractmethod
+    def find_shortest_cycle(
+        self, module: str, as_package: bool = False
+    ) -> Optional[Tuple[str, ...]]:
+        """
+        Returns the shortest import cycle from `module` to itself, or `None` if no cycle exist.
+
+        Optional args:
+            as_package:    Whether or not to treat the supplied module as an individual module,
+                           or as an entire subpackage (including any descendants).
+        """
+        raise NotImplementedError
+
     # High level analysis
     # -------------------
 

tests/unit/adaptors/graph/test_cycles.py

@@ -0,0 +1,55 @@
+import pytest
+
+from grimp.adaptors.graph import ImportGraph
+
+
+class TestFindShortestCycle:
+    @pytest.mark.parametrize(
+        "expected_cycle",
+        [
+            ("foo", "bar", "foo"),
+            ("foo", "bar", "baz", "foo"),
+        ],
+    )
+    def test_finds_shortest_cycle_when_exists(self, expected_cycle):
+        graph = ImportGraph()
+        # Shortest cycle
+        for importer, imported in zip(expected_cycle[:-1], expected_cycle[1:]):
+            graph.add_import(importer=importer, imported=imported)
+        # Longer cycle
+        graph.add_import(importer="foo", imported="x")
+        graph.add_import(importer="x", imported="y")
+        graph.add_import(importer="y", imported="z")
+        graph.add_import(importer="z", imported="foo")
+
+        assert graph.find_shortest_cycle("foo") == expected_cycle
+
+        graph.remove_import(importer=expected_cycle[-2], imported=expected_cycle[-1])
+
+        assert graph.find_shortest_cycle("foo") == ("foo", "x", "y", "z", "foo")
+
+    def test_returns_none_if_no_cycle_exists(self):
+        graph = ImportGraph()
+        graph.add_import(importer="foo", imported="bar")
+        graph.add_import(importer="bar", imported="baz")
+        # graph.add_import(importer="baz", imported="foo")  # This import is missing -> No cycle.
+
+        assert graph.find_shortest_cycle("foo") is None
+
+    def test_ignores_internal_imports_when_as_package_is_true(self):
+        graph = ImportGraph()
+        graph.add_module("colors")
+        graph.add_import(importer="colors.red", imported="colors.blue")
+        graph.add_import(importer="colors.blue", imported="colors.red")
+        graph.add_import(importer="colors.red", imported="x")
+        graph.add_import(importer="x", imported="y")
+        graph.add_import(importer="y", imported="z")
+        graph.add_import(importer="z", imported="colors.blue")
+
+        assert graph.find_shortest_cycle("colors", as_package=True) == (
+            "colors.red",
+            "x",
+            "y",
+            "z",
+            "colors.blue",
+        )