From 68860cd6ff937b64ec19069db224c5d17aaba05d Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sat, 22 Feb 2025 07:42:51 +0100 Subject: [PATCH 1/5] Rename find_shortest_path Remove the "bidirectional" from the function name, since this is the only path finding function we have. The "bidirectional" part is just an implementation detail. --- rust/src/graph/import_chain_queries.rs | 4 ++-- rust/src/graph/pathfinding.rs | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/rust/src/graph/import_chain_queries.rs b/rust/src/graph/import_chain_queries.rs index 4f21da48..3a753a60 100644 --- a/rust/src/graph/import_chain_queries.rs +++ b/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, excluded_imports: &FxHashMap>, ) -> GrimpResult>> { - find_shortest_path_bidirectional( + find_shortest_path( self, from_modules, to_modules, diff --git a/rust/src/graph/pathfinding.rs b/rust/src/graph/pathfinding.rs index 55e9450d..742eae77 100644 --- a/rust/src/graph/pathfinding.rs +++ b/rust/src/graph/pathfinding.rs @@ -28,7 +28,8 @@ pub fn find_reach( &seen.into_iter().collect::>() - 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, to_modules: &FxHashSet, From f73738dac92e462e0e74e931a02e72f85ac8169f Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sun, 2 Mar 2025 06:25:30 +0100 Subject: [PATCH 2/5] Split shared descendants part from find_shortest_path --- rust/src/graph/pathfinding.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/rust/src/graph/pathfinding.rs b/rust/src/graph/pathfinding.rs index 742eae77..e4ec8bfa 100644 --- a/rust/src/graph/pathfinding.rs +++ b/rust/src/graph/pathfinding.rs @@ -40,6 +40,22 @@ pub fn find_shortest_path( return Err(GrimpError::SharedDescendants); } + _find_shortest_path( + graph, + from_modules, + to_modules, + excluded_modules, + excluded_imports, + ) +} + +fn _find_shortest_path( + graph: &Graph, + from_modules: &FxHashSet, + to_modules: &FxHashSet, + excluded_modules: &FxHashSet, + excluded_imports: &FxHashMap>, +) -> GrimpResult>> { let mut predecessors: FxIndexMap> = from_modules .clone() .into_iter() From f29fc2cf34ecd68b484379b7148e4afc7a5e2d97 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sun, 2 Mar 2025 06:41:00 +0100 Subject: [PATCH 3/5] Add find_shortest_cycle to pathfinding --- rust/src/graph/pathfinding.rs | 120 ++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/rust/src/graph/pathfinding.rs b/rust/src/graph/pathfinding.rs index e4ec8bfa..293b41dc 100644 --- a/rust/src/graph/pathfinding.rs +++ b/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; @@ -49,6 +50,23 @@ pub fn find_shortest_path( ) } +/// Finds the shortest cycle from `modules` to `modules`, via a bidirectional BFS. +pub fn find_shortest_cycle( + graph: &Graph, + modules: &FxHashSet, + excluded_modules: &FxHashSet, + excluded_imports: &FxHashMap>, +) -> GrimpResult>> { + // 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, @@ -140,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(()) + } +} From 1cbefecd87b768065ea1a60dde53a488d320dfd0 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sun, 2 Mar 2025 06:45:52 +0100 Subject: [PATCH 4/5] Add find_shortest_cycle to rust graph --- rust/src/graph/cycles.rs | 29 +++++++++++++++++++++++++++++ rust/src/graph/mod.rs | 1 + rust/src/lib.rs | 19 +++++++++++++++++++ 3 files changed, 49 insertions(+) create mode 100644 rust/src/graph/cycles.rs diff --git a/rust/src/graph/cycles.rs b/rust/src/graph/cycles.rs new file mode 100644 index 00000000..c7d3f462 --- /dev/null +++ b/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>> { + if as_package { + pathfinding::find_shortest_cycle( + self, + &module.conv::>().with_descendants(self), + &FxHashSet::default(), + &FxHashMap::default(), + ) + } else { + pathfinding::find_shortest_cycle( + self, + &module.conv::>(), + &FxHashSet::default(), + &FxHashMap::default(), + ) + } + } +} diff --git a/rust/src/graph/mod.rs b/rust/src/graph/mod.rs index 8a97b2bd..4ccf76be 100644 --- a/rust/src/graph/mod.rs +++ b/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; diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 49a97a5b..139bb2f4 100644 --- a/rust/src/lib.rs +++ b/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>> { + 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, From 43b7c31634ec2724c680726e64b6af6b46e0ec83 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sun, 2 Mar 2025 06:56:56 +0100 Subject: [PATCH 5/5] Add find_shortest_cycle to python graph --- src/grimp/adaptors/graph.py | 9 ++++ src/grimp/application/ports/graph.py | 16 +++++++ tests/unit/adaptors/graph/test_cycles.py | 55 ++++++++++++++++++++++++ 3 files changed, 80 insertions(+) create mode 100644 tests/unit/adaptors/graph/test_cycles.py diff --git a/src/grimp/adaptors/graph.py b/src/grimp/adaptors/graph.py index 4730ef45..f99eee87 100644 --- a/src/grimp/adaptors/graph.py +++ b/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]], diff --git a/src/grimp/application/ports/graph.py b/src/grimp/application/ports/graph.py index acf39a84..10d580c4 100644 --- a/src/grimp/application/ports/graph.py +++ b/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 # ------------------- diff --git a/tests/unit/adaptors/graph/test_cycles.py b/tests/unit/adaptors/graph/test_cycles.py new file mode 100644 index 00000000..87d63fec --- /dev/null +++ b/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", + )