From 68860cd6ff937b64ec19069db224c5d17aaba05d Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sat, 22 Feb 2025 07:42:51 +0100 Subject: [PATCH 01/10] 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 02/10] 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 03/10] 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 04/10] 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 b1804b034c32ce1180eac3ea67341b8ea3a4cc17 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sun, 2 Mar 2025 06:56:56 +0100 Subject: [PATCH 05/10] 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 | 48 ++++++++++++++++++++++++ 3 files changed, 73 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..eaff1b16 --- /dev/null +++ b/tests/unit/adaptors/graph/test_cycles.py @@ -0,0 +1,48 @@ +from grimp.adaptors.graph import ImportGraph + + +class TestFindShortestCycle: + def test_finds_shortest_cycle_when_exists(self): + graph = ImportGraph() + # Shortest cycle + graph.add_import(importer="foo", imported="bar") + graph.add_import(importer="bar", imported="baz") + graph.add_import(importer="baz", imported="foo") + # 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") == ("foo", "bar", "baz", "foo") + + graph.remove_import(importer="baz", imported="foo") + + assert graph.find_shortest_cycle("foo") == ("foo", "x", "y", "z", "foo") + + def test_returns_none_if_no_cycle_exists(self): + graph = ImportGraph() + # Shortest cycle + 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", + ) From 0e355aa08e745435257926555574a2a081f9a87c Mon Sep 17 00:00:00 2001 From: K4liber Date: Sun, 27 Jul 2025 11:45:41 +0200 Subject: [PATCH 06/10] use Vec in find_shortest_cycle to get deterministic outputs --- rust/src/graph/cycles.rs | 27 +++++++------ rust/src/graph/pathfinding.rs | 71 +++++++++++++++++++++-------------- 2 files changed, 55 insertions(+), 43 deletions(-) diff --git a/rust/src/graph/cycles.rs b/rust/src/graph/cycles.rs index c7d3f462..e66fea58 100644 --- a/rust/src/graph/cycles.rs +++ b/rust/src/graph/cycles.rs @@ -10,20 +10,19 @@ impl Graph { module: ModuleToken, as_package: bool, ) -> GrimpResult>> { - if as_package { - pathfinding::find_shortest_cycle( - self, - &module.conv::>().with_descendants(self), - &FxHashSet::default(), - &FxHashMap::default(), - ) + let modules: Vec = if as_package { + let mut vec = module.conv::>().with_descendants(self); + vec.sort_by_key(|token| self.get_module(*token).unwrap().name()); + vec } else { - pathfinding::find_shortest_cycle( - self, - &module.conv::>(), - &FxHashSet::default(), - &FxHashMap::default(), - ) - } + let vec: Vec<_> = module.conv::>(); + vec + }; + pathfinding::find_shortest_cycle( + self, + &modules, + &FxHashSet::default(), + &FxHashMap::default(), + ) } } diff --git a/rust/src/graph/pathfinding.rs b/rust/src/graph/pathfinding.rs index 293b41dc..3f71fc01 100644 --- a/rust/src/graph/pathfinding.rs +++ b/rust/src/graph/pathfinding.rs @@ -41,10 +41,18 @@ pub fn find_shortest_path( return Err(GrimpError::SharedDescendants); } + let predecessors: FxIndexMap> = from_modules + .clone() + .into_iter() + .map(|m| (m, None)) + .collect(); + let successors: FxIndexMap> = + to_modules.clone().into_iter().map(|m| (m, None)).collect(); + _find_shortest_path( graph, - from_modules, - to_modules, + predecessors, + successors, excluded_modules, excluded_imports, ) @@ -53,7 +61,7 @@ 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, + modules: &[ModuleToken], excluded_modules: &FxHashSet, excluded_imports: &FxHashMap>, ) -> GrimpResult>> { @@ -64,39 +72,43 @@ pub fn find_shortest_cycle( excluded_imports.entry(*m2).or_default().insert(*m1); } - _find_shortest_path(graph, modules, modules, excluded_modules, &excluded_imports) + let predecessors: FxIndexMap> = modules + .iter() + .cloned() + .map(|m| (m, None)) + .collect(); + + let successors: FxIndexMap> = predecessors + .clone(); + + _find_shortest_path(graph, predecessors, successors, excluded_modules, &excluded_imports) } fn _find_shortest_path( graph: &Graph, - from_modules: &FxHashSet, - to_modules: &FxHashSet, + mut predecessors: FxIndexMap>, + mut successors: FxIndexMap>, excluded_modules: &FxHashSet, excluded_imports: &FxHashMap>, ) -> GrimpResult>> { - let mut predecessors: FxIndexMap> = from_modules - .clone() - .into_iter() - .map(|m| (m, None)) - .collect(); - let mut successors: FxIndexMap> = - to_modules.clone().into_iter().map(|m| (m, None)).collect(); + let mut i_forwards = 0; let mut i_backwards = 0; let middle = 'l: loop { for _ in 0..(predecessors.len() - i_forwards) { let module = *predecessors.get_index(i_forwards).unwrap().0; - let next_modules = graph.imports.get(module).unwrap(); + let mut next_modules: Vec<_> = graph.imports.get(module).unwrap().iter().cloned().collect(); + next_modules.sort_by_key(|next_module| graph.get_module(*next_module).unwrap().name()); for next_module in next_modules { - if import_is_excluded(&module, next_module, excluded_modules, excluded_imports) { + if import_is_excluded(&module, &next_module, excluded_modules, excluded_imports) { continue; } - if !predecessors.contains_key(next_module) { - predecessors.insert(*next_module, Some(module)); + if !predecessors.contains_key(&next_module) { + predecessors.insert(next_module, Some(module)); } - if successors.contains_key(next_module) { - break 'l Some(*next_module); + if successors.contains_key(&next_module) { + break 'l Some(next_module); } } i_forwards += 1; @@ -104,16 +116,17 @@ fn _find_shortest_path( for _ in 0..(successors.len() - i_backwards) { let module = *successors.get_index(i_backwards).unwrap().0; - let next_modules = graph.reverse_imports.get(module).unwrap(); + let mut next_modules: Vec<_> = graph.reverse_imports.get(module).unwrap().iter().cloned().collect(); + next_modules.sort_by_key(|next_module| graph.get_module(*next_module).unwrap().name()); for next_module in next_modules { - if import_is_excluded(next_module, &module, excluded_modules, excluded_imports) { + if import_is_excluded(&next_module, &module, excluded_modules, excluded_imports) { continue; } - if !successors.contains_key(next_module) { - successors.insert(*next_module, Some(module)); + if !successors.contains_key(&next_module) { + successors.insert(next_module, Some(module)); } - if predecessors.contains_key(next_module) { - break 'l Some(*next_module); + if predecessors.contains_key(&next_module) { + break 'l Some(next_module); } } i_backwards += 1; @@ -184,7 +197,7 @@ mod test_find_shortest_cycle { let path = find_shortest_cycle( &graph, - &foo.into(), + &[foo], &FxHashSet::default(), &FxHashMap::default(), )?; @@ -194,7 +207,7 @@ mod test_find_shortest_cycle { let path = find_shortest_cycle( &graph, - &foo.into(), + &[foo], &FxHashSet::default(), &FxHashMap::default(), )?; @@ -214,7 +227,7 @@ mod test_find_shortest_cycle { let path = find_shortest_cycle( &graph, - &foo.into(), + &[foo], &FxHashSet::default(), &FxHashMap::default(), )?; @@ -250,7 +263,7 @@ mod test_find_shortest_cycle { let path = find_shortest_cycle( &graph, - &FxHashSet::from_iter([red, blue]), + &Vec::from_iter([red, blue]), &FxHashSet::default(), &FxHashMap::default(), )?; From 9f2b7de7c75783ebc8c1f6784d1157a180526774 Mon Sep 17 00:00:00 2001 From: K4liber Date: Sat, 16 Aug 2025 13:16:22 +0200 Subject: [PATCH 07/10] add snapshot typing --- src/grimp/adaptors/graph.py | 2 +- tests/functional/test_build_graph_on_real_packages.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/grimp/adaptors/graph.py b/src/grimp/adaptors/graph.py index 36386ad8..23ed3bd0 100644 --- a/src/grimp/adaptors/graph.py +++ b/src/grimp/adaptors/graph.py @@ -221,7 +221,7 @@ def _parse_layers(layers: Sequence[Layer | str | set[str]]) -> tuple[Layer, ...] def _dependencies_from_tuple( - rust_package_dependency_tuple: tuple[_RustPackageDependency, ...] + rust_package_dependency_tuple: tuple[_RustPackageDependency, ...], ) -> set[PackageDependency]: return { PackageDependency( diff --git a/tests/functional/test_build_graph_on_real_packages.py b/tests/functional/test_build_graph_on_real_packages.py index 3746d005..52728960 100644 --- a/tests/functional/test_build_graph_on_real_packages.py +++ b/tests/functional/test_build_graph_on_real_packages.py @@ -2,12 +2,14 @@ import grimp +from syrupy.assertion import SnapshotAssertion + @pytest.mark.parametrize( "package_name", ("django", "flask", "requests", "sqlalchemy", "google.cloud.audit"), ) -def test_build_graph_on_real_package(package_name, snapshot): +def test_build_graph_on_real_package(package_name: str, snapshot: SnapshotAssertion) -> None: graph = grimp.build_graph(package_name, cache_dir=None) imports = graph.find_matching_direct_imports(import_expression="** -> **") assert imports == snapshot From fd5df5d5a7fada3775a2b31046ff8ab8dc039f04 Mon Sep 17 00:00:00 2001 From: K4liber Date: Fri, 5 Sep 2025 20:43:15 +0200 Subject: [PATCH 08/10] optimized performance of _find_shortest_path --- rust/src/graph/pathfinding.rs | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/rust/src/graph/pathfinding.rs b/rust/src/graph/pathfinding.rs index 5e782817..17723b4a 100644 --- a/rust/src/graph/pathfinding.rs +++ b/rust/src/graph/pathfinding.rs @@ -55,6 +55,7 @@ pub fn find_shortest_path( successors, excluded_modules, excluded_imports, + false ) } @@ -81,7 +82,7 @@ pub fn find_shortest_cycle( let successors: FxIndexMap> = predecessors .clone(); - _find_shortest_path(graph, predecessors, successors, excluded_modules, &excluded_imports) + _find_shortest_path(graph, predecessors, successors, excluded_modules, &excluded_imports, true) } fn _find_shortest_path( @@ -90,6 +91,7 @@ fn _find_shortest_path( mut successors: FxIndexMap>, excluded_modules: &FxHashSet, excluded_imports: &FxHashMap>, + sorted: bool, ) -> GrimpResult>> { @@ -98,8 +100,14 @@ fn _find_shortest_path( let middle = 'l: loop { for _ in 0..(predecessors.len() - i_forwards) { let module = *predecessors.get_index(i_forwards).unwrap().0; - let mut next_modules: Vec<_> = graph.imports.get(module).unwrap().iter().cloned().collect(); - next_modules.sort_by_key(|next_module| graph.get_module(*next_module).unwrap().name()); + let mut next_modules: Vec = graph.imports.get(module).unwrap().iter().cloned().collect(); + + if sorted { + next_modules.sort_by_key(|next_module| { + graph.get_module(*next_module).unwrap().name() + }); + } + for next_module in next_modules { if import_is_excluded(&module, &next_module, excluded_modules, excluded_imports) { continue; @@ -116,8 +124,14 @@ fn _find_shortest_path( for _ in 0..(successors.len() - i_backwards) { let module = *successors.get_index(i_backwards).unwrap().0; - let mut next_modules: Vec<_> = graph.reverse_imports.get(module).unwrap().iter().cloned().collect(); - next_modules.sort_by_key(|next_module| graph.get_module(*next_module).unwrap().name()); + let mut next_modules: Vec = graph.reverse_imports.get(module).unwrap().iter().cloned().collect(); + + if sorted { + next_modules.sort_by_key(|next_module| { + graph.get_module(*next_module).unwrap().name() + }); + } + for next_module in next_modules { if import_is_excluded(&next_module, &module, excluded_modules, excluded_imports) { continue; From 1fc43bc31f8a6f7e1c7fea1d54d0927a00a1a1f5 Mon Sep 17 00:00:00 2001 From: K4liber Date: Fri, 5 Sep 2025 20:45:50 +0200 Subject: [PATCH 09/10] ignore missing library stub for SnapshotAssertion --- tests/functional/test_build_graph_on_real_packages.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/functional/test_build_graph_on_real_packages.py b/tests/functional/test_build_graph_on_real_packages.py index 52728960..6be78748 100644 --- a/tests/functional/test_build_graph_on_real_packages.py +++ b/tests/functional/test_build_graph_on_real_packages.py @@ -2,7 +2,7 @@ import grimp -from syrupy.assertion import SnapshotAssertion +from syrupy.assertion import SnapshotAssertion # type: ignore @pytest.mark.parametrize( From 1acbc02e5492404db1b6555ca355cac1a25300e6 Mon Sep 17 00:00:00 2001 From: K4liber Date: Fri, 5 Sep 2025 20:56:22 +0200 Subject: [PATCH 10/10] cargo fmt --- rust/src/filesystem.rs | 13 +++---- rust/src/graph/pathfinding.rs | 68 ++++++++++++++++------------------- rust/src/import_scanning.rs | 16 ++++----- rust/src/lib.rs | 15 ++++---- 4 files changed, 55 insertions(+), 57 deletions(-) diff --git a/rust/src/filesystem.rs b/rust/src/filesystem.rs index f145734b..90985ac4 100644 --- a/rust/src/filesystem.rs +++ b/rust/src/filesystem.rs @@ -88,10 +88,11 @@ impl FileSystem for RealBasicFileSystem { // Coding specification needs to be in the first two lines, or it's ignored. for line in s.lines().take(2) { if let Some(captures) = encoding_re.captures(line) - && let Some(encoding_name) = captures.get(1) { - detected_encoding = Some(encoding_name.as_str().to_string()); - break; - } + && let Some(encoding_name) = captures.get(1) + { + detected_encoding = Some(encoding_name.as_str().to_string()); + break; + } } if let Some(enc_name) = detected_encoding { @@ -261,11 +262,11 @@ impl PyFakeBasicFileSystem { fn read(&self, file_name: &str) -> PyResult { self.inner.read(file_name) } - + // Temporary workaround method for Python tests. fn convert_to_basic(&self) -> PyResult { Ok(PyFakeBasicFileSystem { - inner: self.inner.clone() + inner: self.inner.clone(), }) } } diff --git a/rust/src/graph/pathfinding.rs b/rust/src/graph/pathfinding.rs index 17723b4a..d80fe5ae 100644 --- a/rust/src/graph/pathfinding.rs +++ b/rust/src/graph/pathfinding.rs @@ -55,7 +55,7 @@ pub fn find_shortest_path( successors, excluded_modules, excluded_imports, - false + false, ) } @@ -73,16 +73,19 @@ pub fn find_shortest_cycle( excluded_imports.entry(*m2).or_default().insert(*m1); } - let predecessors: FxIndexMap> = modules - .iter() - .cloned() - .map(|m| (m, None)) - .collect(); + let predecessors: FxIndexMap> = + modules.iter().cloned().map(|m| (m, None)).collect(); - let successors: FxIndexMap> = predecessors - .clone(); + let successors: FxIndexMap> = predecessors.clone(); - _find_shortest_path(graph, predecessors, successors, excluded_modules, &excluded_imports, true) + _find_shortest_path( + graph, + predecessors, + successors, + excluded_modules, + &excluded_imports, + true, + ) } fn _find_shortest_path( @@ -93,19 +96,17 @@ fn _find_shortest_path( excluded_imports: &FxHashMap>, sorted: bool, ) -> GrimpResult>> { - - let mut i_forwards = 0; let mut i_backwards = 0; let middle = 'l: loop { for _ in 0..(predecessors.len() - i_forwards) { let module = *predecessors.get_index(i_forwards).unwrap().0; - let mut next_modules: Vec = graph.imports.get(module).unwrap().iter().cloned().collect(); + let mut next_modules: Vec = + graph.imports.get(module).unwrap().iter().cloned().collect(); if sorted { - next_modules.sort_by_key(|next_module| { - graph.get_module(*next_module).unwrap().name() - }); + next_modules + .sort_by_key(|next_module| graph.get_module(*next_module).unwrap().name()); } for next_module in next_modules { @@ -124,12 +125,17 @@ fn _find_shortest_path( for _ in 0..(successors.len() - i_backwards) { let module = *successors.get_index(i_backwards).unwrap().0; - let mut next_modules: Vec = graph.reverse_imports.get(module).unwrap().iter().cloned().collect(); + let mut next_modules: Vec = graph + .reverse_imports + .get(module) + .unwrap() + .iter() + .cloned() + .collect(); if sorted { - next_modules.sort_by_key(|next_module| { - graph.get_module(*next_module).unwrap().name() - }); + next_modules + .sort_by_key(|next_module| graph.get_module(*next_module).unwrap().name()); } for next_module in next_modules { @@ -209,22 +215,14 @@ mod test_find_shortest_cycle { graph.add_import(y, z); graph.add_import(z, foo); - let path = find_shortest_cycle( - &graph, - &[foo], - &FxHashSet::default(), - &FxHashMap::default(), - )?; + let path = + find_shortest_cycle(&graph, &[foo], &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], - &FxHashSet::default(), - &FxHashMap::default(), - )?; + let path = + find_shortest_cycle(&graph, &[foo], &FxHashSet::default(), &FxHashMap::default())?; assert_eq!(path, Some(vec![foo, x, y, z, foo])); Ok(()) @@ -239,12 +237,8 @@ mod test_find_shortest_cycle { graph.add_import(foo, bar); graph.add_import(bar, baz); - let path = find_shortest_cycle( - &graph, - &[foo], - &FxHashSet::default(), - &FxHashMap::default(), - )?; + let path = + find_shortest_cycle(&graph, &[foo], &FxHashSet::default(), &FxHashMap::default())?; assert_eq!(path, None); diff --git a/rust/src/import_scanning.rs b/rust/src/import_scanning.rs index ddfd683c..cd9435b3 100644 --- a/rust/src/import_scanning.rs +++ b/rust/src/import_scanning.rs @@ -143,14 +143,14 @@ fn scan_for_imports_no_py_single_module( if include_external_packages && let Some(imported_module) = _distill_external_module(&imported_object_name, found_packages) - { - imports.insert(DirectImport { - importer: module.name.to_string(), - imported: imported_module, - line_number: imported_object.line_number, - line_contents: imported_object.line_contents, - }); - } + { + imports.insert(DirectImport { + importer: module.name.to_string(), + imported: imported_module, + line_number: imported_object.line_number, + line_contents: imported_object.line_contents, + }); + } } } } diff --git a/rust/src/lib.rs b/rust/src/lib.rs index b8f23cf9..1431f330 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -13,8 +13,7 @@ use crate::filesystem::{PyFakeBasicFileSystem, PyRealBasicFileSystem}; use crate::graph::higher_order_queries::Level; use crate::graph::{Graph, Module, ModuleIterator, ModuleTokenIterator}; use crate::import_scanning::{ - get_file_system_boxed, py_found_packages_to_rust, scan_for_imports_no_py, - to_py_direct_imports, + get_file_system_boxed, py_found_packages_to_rust, scan_for_imports_no_py, to_py_direct_imports, }; use crate::module_expressions::ModuleExpression; use derive_new::new; @@ -91,8 +90,11 @@ fn scan_for_imports<'py>( match imports_by_module_result { Err(GrimpError::ParseError { - module_filename, line_number, text, .. - }) => { + module_filename, + line_number, + text, + .. + }) => { // TODO: define SourceSyntaxError using pyo3. let exceptions_pymodule = PyModule::import(py, "grimp.exceptions").unwrap(); let py_exception_class = exceptions_pymodule.getattr("SourceSyntaxError").unwrap(); @@ -112,13 +114,14 @@ fn scan_for_imports<'py>( for (module, imports) in imports_by_module.iter() { let py_module_instance = py_module_class.call1((module.name.clone(),)).unwrap(); let py_imports = to_py_direct_imports(py, imports); - imports_by_module_py.set_item(py_module_instance, py_imports).unwrap(); + imports_by_module_py + .set_item(py_module_instance, py_imports) + .unwrap(); } Ok(imports_by_module_py) } - #[pyclass(name = "Graph")] struct GraphWrapper { _graph: Graph,