Merge pull request #239 from Peter554/better-benchmark-for-find-shortest-chains

Add more useful benchmark for find_shortest_chains

Author: seddonym

.github/workflows/main.yml

@@ -46,6 +46,8 @@ jobs:
       - uses: actions-rs/toolchain@v1
         with:
           toolchain: stable
+      - name: Print versions
+        run: cargo version --verbose && cargo clippy --version
       - name: Run tests
         run: cargo test --no-default-features
         working-directory: ./rust
@@ -81,4 +83,4 @@ jobs:
           with:
             token: ${{ secrets.CODSPEED_TOKEN }}
             run: |
-              uv run pytest tests/benchmarking/ --codspeed
+              uv run pytest tests/benchmarking/ --codspeed

rust/src/filesystem.rs

@@ -87,12 +87,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) {
-                if let Some(encoding_name) = captures.get(1) {
+            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;
                 }
-            }
         }
 
         if let Some(enc_name) = detected_encoding {

rust/src/graph/hierarchy_queries.rs

@@ -17,7 +17,7 @@ impl Graph {
     }
 
     // TODO(peter) Guarantee order?
-    pub fn all_modules(&self) -> impl ModuleIterator {
+    pub fn all_modules(&self) -> impl ModuleIterator<'_> {
         self.modules.values()
     }
 
@@ -28,7 +28,7 @@ impl Graph {
         }
     }
 
-    pub fn get_module_children(&self, module: ModuleToken) -> impl ModuleIterator {
+    pub fn get_module_children(&self, module: ModuleToken) -> impl ModuleIterator<'_> {
         let children = match self.module_children.get(module) {
             Some(children) => children
                 .iter()
@@ -42,7 +42,7 @@ impl Graph {
     /// Returns an iterator over the passed modules descendants.
     ///
     /// Parent modules will be yielded before their child modules.
-    pub fn get_module_descendants(&self, module: ModuleToken) -> impl ModuleIterator {
+    pub fn get_module_descendants(&self, module: ModuleToken) -> impl ModuleIterator<'_> {
         let mut descendants = self.get_module_children(module).collect::<Vec<_>>();
         for child in descendants.clone() {
             descendants.extend(self.get_module_descendants(child.token).collect::<Vec<_>>())
@@ -53,7 +53,7 @@ impl Graph {
     pub fn find_matching_modules(
         &self,
         expression: &ModuleExpression,
-    ) -> impl ModuleIterator + use<'_> {
+    ) -> impl ModuleIterator<'_> + use<'_> {
         let interner = MODULE_NAMES.read().unwrap();
         let modules: FxHashSet<_> = self
             .modules

rust/src/import_scanning.rs

@@ -140,8 +140,8 @@ fn scan_for_imports_no_py_single_module(
             }
             None => {
                 // It's an external import.
-                if include_external_packages {
-                    if let Some(imported_module) =
+                if include_external_packages
+                    && let Some(imported_module) =
                         _distill_external_module(&imported_object_name, found_packages)
                     {
                         imports.insert(DirectImport {
@@ -151,7 +151,6 @@ fn scan_for_imports_no_py_single_module(
                             line_contents: imported_object.line_contents,
                         });
                     }
-                }
             }
         }
     }

tests/benchmarking/test_benchmarking.py

@@ -464,6 +464,40 @@ def test_no_chains(self, large_graph, benchmark):
         )
         assert result == set()
 
+    def test_chains_found_sparse_imports(self, benchmark):
+        """
+        A test case where the number of import chains is significantly less
+        than the number of module combinations. In this case package `a` has
+        100 modules and package `c` has 100 modules, however the number of import
+        chains between them is only 10.
+
+        This case of "sparse" imports is typical of realistic scenarios, where there are
+        large packages and only a small handful of contract violations.
+
+        This test case is designed to expose weaknesses of naive O(N^2) algorithms
+        which compute chains by pathfinding between every pair of modules.
+        """
+        graph = ImportGraph()
+        graph.add_module("a")
+        graph.add_module("b")
+        graph.add_module("c")
+        for i in range(100):
+            graph.add_module(f"a.m{i}")
+            graph.add_module(f"b.m{i}")
+            graph.add_module(f"c.m{i}")
+            if i % 10 == 0:
+                graph.add_import(importer=f"a.m{i}", imported=f"b.m{i}")
+                graph.add_import(importer=f"b.m{i}", imported=f"c.m{i}")
+
+        result = _run_benchmark(
+            benchmark,
+            graph.find_shortest_chains,
+            "a",
+            "c",
+            as_packages=True,
+        )
+        assert len(result) == 10
+
 
 def test_copy_graph(large_graph, benchmark):
     _run_benchmark(benchmark, lambda: deepcopy(large_graph))