perf(noImportCycles): exclude node_modules, add more tests (#11251)

Co-authored-by: Bez Sahara <239664238+BezSaharaD@users.noreply.github.com>

Author: dyc3

.changeset/bitter-cats-cut.md

@@ -0,0 +1,5 @@
+---
+"@biomejs/biome": patch
+---
+
+Improved performance of [`noImportCycles`](https://biomejs.dev/linter/rules/no-import-cycles/).

crates/biome_fs/src/lib.rs

@@ -11,5 +11,5 @@ pub use fs::{
     TraversalContext, TraversalScope,
 };
 pub use interner::{PathInterner, PathInternerSet};
-pub use path::BiomePath;
+pub use path::{BiomePath, is_node_modules_path};
 pub use utils::*;

crates/biome_fs/src/path.rs

@@ -13,6 +13,13 @@ use std::hash::Hash;
 use std::path::{Path, PathBuf};
 use std::{fs::File, io, io::Write, ops::Deref};
 
+/// Returns whether `path` contains a directory component named `node_modules`.
+#[inline]
+pub fn is_node_modules_path(path: &Utf8Path) -> bool {
+    path.components()
+        .any(|component| component.as_str().as_bytes() == b"node_modules")
+}
+
 /// The priority of the file
 // NOTE: The order of the variants is important, the one on the top has the highest priority
 #[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Ord, PartialOrd, Hash)]
@@ -196,9 +203,7 @@ impl BiomePath {
     /// Returns `true` if the path is inside `node_modules`
     #[inline(always)]
     pub fn is_dependency(&self) -> bool {
-        self.path
-            .components()
-            .any(|component| component.as_str().as_bytes() == b"node_modules")
+        is_node_modules_path(&self.path)
     }
 
     /// Whether this is a file named `package.json`
@@ -327,6 +332,22 @@ impl Ord for BiomePath {
 #[cfg(test)]
 mod test {
     use crate::path::FileKinds;
+    use camino::Utf8Path;
+
+    use super::is_node_modules_path;
+
+    #[test]
+    fn detects_node_modules_paths() {
+        assert!(is_node_modules_path(Utf8Path::new(
+            "/project/node_modules/package/index.js"
+        )));
+        assert!(is_node_modules_path(Utf8Path::new(
+            "project/packages/app/node_modules/package/index.js"
+        )));
+        assert!(!is_node_modules_path(Utf8Path::new(
+            "/project/node_modules_backup/package/index.js"
+        )));
+    }
 
     #[test]
     fn test_biome_paths() {

crates/biome_js_analyze/src/lint/suspicious/no_import_cycles.rs

@@ -4,14 +4,15 @@ use biome_analyze::{
 };
 use biome_console::markup;
 use biome_diagnostics::Severity;
+use biome_fs::is_node_modules_path;
 use biome_js_syntax::AnyJsImportLike;
 use biome_module_graph::{
     JsImportPath, JsImportPhase, JsModuleInfo, ModuleGraphGeneration, js_module_sccs,
 };
 use biome_resolver::ResolvedPath;
 use biome_rowan::AstNode;
 use biome_rule_options::no_import_cycles::NoImportCyclesOptions;
-use camino::{Utf8Path, Utf8PathBuf};
+use camino::Utf8PathBuf;
 use rustc_hash::FxHashSet;
 
 declare_lint_rule! {
@@ -316,8 +317,3 @@ fn find_cycle(
 
     None
 }
-
-/// Returns `true` if the given path is inside a `node_modules` directory.
-fn is_node_modules_path(path: &Utf8Path) -> bool {
-    path.components().any(|c| c.as_str() == "node_modules")
-}

crates/biome_module_graph/src/db/queries/js_scc.rs

@@ -1,13 +1,14 @@
 use crate::{JsImportPath, ModuleDb, ModuleGraphGeneration, ModuleInfoKind};
+use biome_fs::is_node_modules_path;
 use camino::{Utf8Path, Utf8PathBuf};
 use rustc_hash::FxHashMap;
 
 /// Strongly connected components of the JavaScript import graph.
 ///
 /// Two modules belong to the same component when each is reachable from the
-/// other by following imports. All resolved edges between indexed JavaScript
-/// modules are included, so callers may use this as a conservative prefilter
-/// when they traverse a narrower graph.
+/// other by following imports. Resolved edges between indexed JavaScript
+/// modules outside `node_modules` are included, so callers may use this as a
+/// conservative prefilter when they traverse a narrower graph.
 ///
 /// See: <https://en.wikipedia.org/wiki/Kosaraju%27s_algorithm>
 #[derive(Debug, Eq, PartialEq)]
@@ -41,8 +42,9 @@ pub fn js_module_sccs(db: &dyn ModuleDb, generation: ModuleGraphGeneration) -> J
     let mut id_by_path = FxHashMap::default();
 
     db.for_each_module(&mut |module| {
-        if matches!(module.kind(db), ModuleInfoKind::Js(_)) {
-            id_by_path.insert(module.path(db).to_path_buf(), id_by_path.len() as u32);
+        let path = module.path(db);
+        if matches!(module.kind(db), ModuleInfoKind::Js(_)) && !is_node_modules_path(path) {
+            id_by_path.insert(path.to_path_buf(), id_by_path.len() as u32);
         }
     });
 
@@ -84,7 +86,7 @@ pub fn js_module_sccs(db: &dyn ModuleDb, generation: ModuleGraphGeneration) -> J
 pub(super) fn compute_sccs(edges: &[Vec<u32>]) -> (Vec<u32>, Vec<u32>) {
     let node_count = edges.len();
     let mut visited = vec![false; node_count];
-    let mut next_edge = vec![0; node_count];
+    let mut next_edge = vec![0u32; node_count];
     let mut finish_order = Vec::with_capacity(node_count);
     let mut stack = Vec::new();
 
@@ -101,7 +103,7 @@ pub(super) fn compute_sccs(edges: &[Vec<u32>]) -> (Vec<u32>, Vec<u32>) {
 
         while let Some(&node) = stack.last() {
             let node = node as usize;
-            if let Some(&next) = edges[node].get(next_edge[node]) {
+            if let Some(&next) = edges[node].get(next_edge[node] as usize) {
                 next_edge[node] += 1;
                 if !visited[next as usize] {
                     visited[next as usize] = true;
@@ -158,7 +160,23 @@ pub(super) fn compute_sccs(edges: &[Vec<u32>]) -> (Vec<u32>, Vec<u32>) {
 
 #[cfg(test)]
 mod tests {
-    use super::compute_sccs;
+    use super::{JsModuleSccs, compute_sccs};
+    use camino::{Utf8Path, Utf8PathBuf};
+
+    fn module_sccs(paths: &[&str], edges: &[Vec<u32>]) -> JsModuleSccs {
+        assert_eq!(paths.len(), edges.len());
+        let (component_by_id, component_sizes) = compute_sccs(edges);
+        let component_by_path = paths
+            .iter()
+            .enumerate()
+            .map(|(id, path)| (Utf8PathBuf::from(*path), component_by_id[id]))
+            .collect();
+
+        JsModuleSccs {
+            component_by_path,
+            component_sizes: component_sizes.into_boxed_slice(),
+        }
+    }
 
     #[test]
     fn separates_acyclic_nodes() {
@@ -182,4 +200,76 @@ mod tests {
         assert_eq!(components[4], components[5]);
         assert_eq!(sizes[components[4] as usize], 2);
     }
+
+    #[test]
+    fn importing_into_cycle_does_not_join_cycle() {
+        let (components, sizes) = compute_sccs(&[vec![1], vec![0], vec![0]]);
+
+        assert_eq!(components[0], components[1]);
+        assert_eq!(sizes[components[0] as usize], 2);
+        assert_ne!(components[2], components[0]);
+        assert_eq!(sizes[components[2] as usize], 1);
+    }
+
+    #[test]
+    fn convergence_without_cycle_separates_all_nodes() {
+        let (components, sizes) = compute_sccs(&[vec![1, 2], vec![3], vec![3], vec![]]);
+
+        for (node, &component) in components.iter().enumerate() {
+            assert_eq!(sizes[component as usize], 1);
+            assert!(components[..node].iter().all(|&other| other != component));
+        }
+    }
+
+    #[test]
+    fn chord_does_not_split_cycle() {
+        let (components, sizes) = compute_sccs(&[vec![1], vec![2, 3], vec![3], vec![0]]);
+
+        assert!(
+            components
+                .iter()
+                .all(|&component| component == components[0])
+        );
+        assert_eq!(sizes[components[0] as usize], 4);
+    }
+
+    #[test]
+    fn contains_cycle_between_nodes_in_cycle() {
+        let sccs = module_sccs(&["/a.js", "/b.js"], &[vec![1], vec![0]]);
+
+        assert!(sccs.contains_cycle_between(Utf8Path::new("/a.js"), Utf8Path::new("/b.js")));
+        assert!(sccs.contains_cycle_between(Utf8Path::new("/b.js"), Utf8Path::new("/a.js")));
+    }
+
+    #[test]
+    fn does_not_contain_cycle_for_edge_exiting_cycle() {
+        let sccs = module_sccs(&["/a.js", "/b.js", "/c.js"], &[vec![1, 2], vec![0], vec![]]);
+
+        assert!(sccs.contains_cycle_between(Utf8Path::new("/a.js"), Utf8Path::new("/b.js")));
+        assert!(!sccs.contains_cycle_between(Utf8Path::new("/a.js"), Utf8Path::new("/c.js")));
+    }
+
+    #[test]
+    fn does_not_contain_cycle_for_single_self_import() {
+        let sccs = module_sccs(&["/a.js"], &[vec![0]]);
+
+        assert!(!sccs.contains_cycle_between(Utf8Path::new("/a.js"), Utf8Path::new("/a.js")));
+    }
+
+    #[test]
+    fn does_not_contain_cycle_between_unrelated_cycles() {
+        let sccs = module_sccs(
+            &["/a.js", "/b.js", "/c.js", "/d.js"],
+            &[vec![1], vec![0], vec![3], vec![2]],
+        );
+
+        assert!(!sccs.contains_cycle_between(Utf8Path::new("/a.js"), Utf8Path::new("/c.js")));
+    }
+
+    #[test]
+    fn does_not_contain_cycle_for_unknown_path() {
+        let sccs = module_sccs(&["/a.js", "/b.js"], &[vec![1], vec![0]]);
+
+        assert!(!sccs.contains_cycle_between(Utf8Path::new("/a.js"), Utf8Path::new("/unknown.js")));
+    }
 }

crates/biome_module_graph/tests/js_scc.rs

@@ -90,3 +90,54 @@ fn scc_query_recomputes_after_module_graph_changes() {
             .contains_cycle_between(Utf8Path::new("/src/a.js"), Utf8Path::new("/src/b.js"))
     );
 }
+
+#[test]
+fn scc_query_recomputes_after_modules_are_added_and_removed() {
+    let fs = MemoryFileSystem::default();
+    fs.insert("/src/a.js".into(), "import './b.js';");
+    fs.insert("/src/b.js".into(), "import './a.js';");
+
+    let mut db = WorkspaceDb::default();
+    let a_path = Utf8Path::new("/src/a.js");
+    let b_path = Utf8Path::new("/src/b.js");
+    db.update_or_insert_module(a_path.to_path_buf(), resolve_module(&fs, "/src/a.js"));
+
+    assert!(
+        !js_module_sccs(&db, ModuleGraphGeneration::get(&db))
+            .contains_cycle_between(a_path, b_path)
+    );
+
+    let generation = db.module_graph_generation();
+    db.update_or_insert_module(b_path.to_path_buf(), resolve_module(&fs, "/src/b.js"));
+
+    assert_eq!(db.module_graph_generation(), generation.wrapping_add(1));
+    assert!(
+        js_module_sccs(&db, ModuleGraphGeneration::get(&db)).contains_cycle_between(a_path, b_path)
+    );
+
+    let generation = db.module_graph_generation();
+    db.remove_module(b_path);
+
+    assert_eq!(db.module_graph_generation(), generation.wrapping_add(1));
+    assert!(
+        !js_module_sccs(&db, ModuleGraphGeneration::get(&db))
+            .contains_cycle_between(a_path, b_path)
+    );
+}
+
+#[test]
+fn scc_query_ignores_paths_in_node_modules() {
+    let (_, db) = module_db(&[
+        ("/src/a.js", "import '../node_modules/dependency/index.js';"),
+        ("/src/b.js", "import './a.js';"),
+        (
+            "/node_modules/dependency/index.js",
+            "import '../../src/b.js';",
+        ),
+    ]);
+
+    assert!(
+        !js_module_sccs(&db, ModuleGraphGeneration::get(&db))
+            .contains_cycle_between(Utf8Path::new("/src/a.js"), Utf8Path::new("/src/b.js"))
+    );
+}