perf: optimise noImportCycles (#6884)

Author: arendjr

.changeset/cyclic-checks-charge.md

@@ -0,0 +1,5 @@
+---
+"@biomejs/biome": patch
+---
+
+Improved the performance of `noImportCycles` by ~30%.

Cargo.lock

@@ -1,5 +1,3 @@
-use std::collections::HashSet;
-
 use biome_analyze::{
     Rule, RuleDiagnostic, RuleDomain, RuleSource, context::RuleContext, declare_lint_rule,
 };
@@ -10,6 +8,7 @@ use biome_module_graph::{JsModuleInfo, ResolvedPath};
 use biome_rowan::AstNode;
 use biome_rule_options::no_import_cycles::NoImportCyclesOptions;
 use camino::{Utf8Path, Utf8PathBuf};
+use rustc_hash::FxHashSet;
 
 use crate::services::module_graph::ResolvedImports;
 
@@ -166,36 +165,32 @@ fn find_cycle(
     start_path: &Utf8Path,
     mut module_info: JsModuleInfo,
 ) -> Option<Box<[Box<str>]>> {
-    let mut seen = HashSet::new();
+    let mut seen = FxHashSet::default();
     let mut stack: Vec<(Box<str>, JsModuleInfo)> = Vec::new();
 
     'outer: loop {
         for resolved_path in module_info.all_import_paths() {
-            let Some(resolved_path) = resolved_path.as_path() else {
+            let Some(path) = resolved_path.as_path() else {
                 continue;
             };
 
-            if resolved_path == ctx.file_path() {
+            if !seen.insert(resolved_path.clone()) {
+                continue;
+            }
+
+            if path == ctx.file_path() {
                 // Return all the paths from `start_path` to `resolved_path`:
                 let paths = Some(start_path.as_str())
                     .into_iter()
                     .map(Box::from)
                     .chain(stack.into_iter().map(|(path, _)| path))
-                    .chain(Some(Box::from(resolved_path.as_str())))
+                    .chain(Some(Box::from(path.as_str())))
                     .collect();
                 return Some(paths);
             }
 
-            // FIXME: Use `get_or_insert_with()` once it's stabilized.
-            //        See: https://github.com/rust-lang/rust/issues/60896
-            if seen.contains(resolved_path.as_str()) {
-                continue;
-            }
-
-            seen.insert(resolved_path.to_string());
-
-            if let Some(next_module_info) = ctx.module_info_for_path(resolved_path) {
-                stack.push((resolved_path.as_str().into(), module_info));
+            if let Some(next_module_info) = ctx.module_info_for_path(path) {
+                stack.push((path.as_str().into(), module_info));
                 module_info = next_module_info;
                 continue 'outer;
             }

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

@@ -26,6 +26,7 @@ biome_resolver       = { workspace = true }
 biome_rowan          = { workspace = true }
 camino               = { workspace = true }
 cfg-if               = { workspace = true }
+indexmap             = { workspace = true }
 once_cell            = "1.21.3"             # Use `std::sync::OnceLock::get_or_try_init` when it is stable.
 papaya               = { workspace = true }
 rust-lapper          = { workspace = true }

crates/biome_module_graph/Cargo.toml

@@ -4,13 +4,14 @@ mod module_resolver;
 mod scope;
 mod visitor;
 
-use std::{collections::BTreeMap, ops::Deref, sync::Arc};
+use std::{ops::Deref, sync::Arc};
 
 use biome_js_syntax::AnyJsImportLike;
 use biome_js_type_info::{BindingId, ImportSymbol, ResolvedTypeId, ScopeId, TypeData};
 use biome_jsdoc_comment::JsdocComment;
 use biome_resolver::ResolvedPath;
 use biome_rowan::{Text, TextRange};
+use indexmap::IndexMap;
 use rust_lapper::Lapper;
 use rustc_hash::FxHashMap;
 
@@ -38,10 +39,9 @@ impl JsModuleInfo {
     /// Returns an iterator over all the static and dynamic imports in this
     /// module.
     pub fn all_import_paths(&self) -> impl Iterator<Item = ResolvedPath> + use<> {
-        let module_info = self.0.as_ref();
         ImportPathIterator {
-            static_import_paths: module_info.static_import_paths.clone(),
-            dynamic_import_paths: module_info.dynamic_import_paths.clone(),
+            module_info: self.clone(),
+            index: 0,
         }
     }
 
@@ -105,7 +105,7 @@ pub struct JsModuleInfoInner {
     /// absolute path it resolves to. The resolved path may be looked up as key
     /// in the [ModuleGraph::data] map, although it is not required to exist
     /// (for instance, if the path is outside the project's scope).
-    pub static_import_paths: BTreeMap<Text, ResolvedPath>,
+    pub static_import_paths: IndexMap<Text, ResolvedPath>,
 
     /// Map of all dynamic import paths found in the module for which the import
     /// specifier could be statically determined.
@@ -121,7 +121,7 @@ pub struct JsModuleInfoInner {
     ///
     /// Paths found in `require()` expressions in CommonJS sources are also
     /// included with the dynamic import paths.
-    pub dynamic_import_paths: BTreeMap<Text, ResolvedPath>,
+    pub dynamic_import_paths: IndexMap<Text, ResolvedPath>,
 
     /// Map of exports from the module.
     ///
@@ -156,21 +156,22 @@ pub struct JsModuleInfoInner {
 }
 
 #[derive(Debug, Default)]
-pub struct Exports(pub(crate) BTreeMap<Text, JsExport>);
+pub struct Exports(pub(crate) IndexMap<Text, JsExport>);
 
 impl Deref for Exports {
-    type Target = BTreeMap<Text, JsExport>;
+    type Target = IndexMap<Text, JsExport>;
 
     fn deref(&self) -> &Self::Target {
         &self.0
     }
 }
 
 #[derive(Debug, Default)]
-pub struct Imports(pub(crate) BTreeMap<Text, JsImport>);
+pub struct Imports(pub(crate) IndexMap<Text, JsImport>);
 
 impl Deref for Imports {
-    type Target = BTreeMap<Text, JsImport>;
+    type Target = IndexMap<Text, JsImport>;
+
     fn deref(&self) -> &Self::Target {
         &self.0
     }
@@ -307,23 +308,29 @@ pub struct JsReexport {
 }
 
 struct ImportPathIterator {
-    static_import_paths: BTreeMap<Text, ResolvedPath>,
-    dynamic_import_paths: BTreeMap<Text, ResolvedPath>,
+    module_info: JsModuleInfo,
+    index: usize,
 }
 
 impl Iterator for ImportPathIterator {
     type Item = ResolvedPath;
 
     fn next(&mut self) -> Option<Self::Item> {
-        if self.static_import_paths.is_empty() {
-            self.dynamic_import_paths
-                .pop_first()
-                .map(|(_source, path)| path)
+        let num_static_imports = self.module_info.static_import_paths.len();
+        let resolved_path = if self.index < num_static_imports {
+            let resolved_path = &self.module_info.static_import_paths[self.index];
+            self.index += 1;
+            resolved_path
+        } else if self.index < self.module_info.dynamic_import_paths.len() + num_static_imports {
+            let resolved_path =
+                &self.module_info.dynamic_import_paths[self.index - num_static_imports];
+            self.index += 1;
+            resolved_path
         } else {
-            self.static_import_paths
-                .pop_first()
-                .map(|(_identifier, path)| path)
-        }
+            return None;
+        };
+
+        Some(resolved_path.clone())
     }
 }
 

crates/biome_module_graph/src/js_module_info.rs

@@ -1,8 +1,4 @@
-use std::{
-    borrow::Cow,
-    collections::{BTreeMap, BTreeSet},
-    sync::Arc,
-};
+use std::{borrow::Cow, collections::BTreeSet, sync::Arc};
 
 use biome_js_semantic::{SemanticEvent, SemanticEventExtractor};
 use biome_js_syntax::{
@@ -19,6 +15,7 @@ use biome_js_type_info::{
 };
 use biome_jsdoc_comment::JsdocComment;
 use biome_rowan::{AstNode, Text, TextRange, TextSize, TokenText};
+use indexmap::IndexMap;
 use rust_lapper::{Interval, Lapper};
 use rustc_hash::FxHashMap;
 
@@ -77,11 +74,11 @@ pub(super) struct JsModuleInfoCollector {
 
     /// Map with all static import paths, from the source specifier to the
     /// resolved path.
-    static_import_paths: BTreeMap<Text, ResolvedPath>,
+    static_import_paths: IndexMap<Text, ResolvedPath>,
 
     /// Map with all dynamic import paths, from the import source to the
     /// resolved path.
-    dynamic_import_paths: BTreeMap<Text, ResolvedPath>,
+    dynamic_import_paths: IndexMap<Text, ResolvedPath>,
 
     /// All collected exports.
     ///
@@ -96,7 +93,7 @@ pub(super) struct JsModuleInfoCollector {
     types: TypeStore,
 
     /// Static imports mapped from the local name of the binding being imported.
-    static_imports: BTreeMap<Text, JsImport>,
+    static_imports: IndexMap<Text, JsImport>,
 }
 
 /// Intermediary representation for an exported symbol.
@@ -519,7 +516,7 @@ impl JsModuleInfoCollector {
         }
     }
 
-    fn finalise(&mut self) -> (BTreeMap<Text, JsExport>, Lapper<u32, ScopeId>) {
+    fn finalise(&mut self) -> (IndexMap<Text, JsExport>, Lapper<u32, ScopeId>) {
         let scope_by_range = Lapper::new(
             self.scope_range_by_start
                 .iter()
@@ -767,8 +764,8 @@ impl JsModuleInfoCollector {
         }
     }
 
-    fn collect_exports(&mut self) -> BTreeMap<Text, JsExport> {
-        let mut finalised_exports = BTreeMap::new();
+    fn collect_exports(&mut self) -> IndexMap<Text, JsExport> {
+        let mut finalised_exports = IndexMap::new();
 
         let exports = std::mem::take(&mut self.exports);
         for export in exports {

crates/biome_module_graph/src/js_module_info/collector.rs

@@ -22,12 +22,12 @@ export = shared;
 
 ```
 Exports {
-  "default" => {
-    ExportOwnExport => JsOwnExport::Type(Module(0) TypeId(3))
-  }
   "foo" => {
     ExportOwnExport => JsOwnExport::Type(Module(0) TypeId(1))
   }
+  "default" => {
+    ExportOwnExport => JsOwnExport::Type(Module(0) TypeId(3))
+  }
 }
 Imports {
   No imports

crates/biome_module_graph/tests/snapshots/test_export_const_type_declaration_with_namespace.snap

@@ -42,12 +42,12 @@ export const superComputer = new DeepThought();
 
 ```
 Exports {
-  "superComputer" => {
-    ExportOwnExport => JsOwnExport::Binding(3)
-  }
   "theAnswer" => {
     ExportOwnExport => JsOwnExport::Binding(0)
   }
+  "superComputer" => {
+    ExportOwnExport => JsOwnExport::Binding(3)
+  }
 }
 Imports {
   No imports

crates/biome_module_graph/tests/snapshots/test_resolve_export_types.snap

@@ -60,29 +60,35 @@ export * as renamed2 from "./renamed-reexports";
 
 ```
 Exports {
-  "a" => {
-    ExportOwnExport => JsOwnExport::Binding(5)
+  "foo" => {
+    ExportOwnExport => JsOwnExport::Binding(0)
   }
-  "b" => {
-    ExportOwnExport => JsOwnExport::Binding(6)
+  "qux" => {
+    ExportOwnExport => JsOwnExport::Binding(4)
   }
   "bar" => {
     ExportOwnExport => JsOwnExport::Binding(1)
   }
+  "quz" => {
+    ExportOwnExport => JsOwnExport::Binding(2)
+  }
   "baz" => {
     ExportOwnExport => JsOwnExport::Binding(3)
   }
+  "a" => {
+    ExportOwnExport => JsOwnExport::Binding(5)
+  }
+  "b" => {
+    ExportOwnExport => JsOwnExport::Binding(6)
+  }
   "d" => {
     ExportOwnExport => JsOwnExport::Binding(7)
   }
-  "default" => {
-    ExportOwnExport => JsOwnExport::Binding(11)
-  }
   "e" => {
     ExportOwnExport => JsOwnExport::Binding(8)
   }
-  "foo" => {
-    ExportOwnExport => JsOwnExport::Binding(0)
+  "default" => {
+    ExportOwnExport => JsOwnExport::Binding(11)
   }
   "oh\nno" => {
     ExportReexport => Reexport(
@@ -91,12 +97,6 @@ Exports {
       Import Symbol: ohNo
     )
   }
-  "qux" => {
-    ExportOwnExport => JsOwnExport::Binding(4)
-  }
-  "quz" => {
-    ExportOwnExport => JsOwnExport::Binding(2)
-  }
   "renamed2" => {
     ExportReexport => Reexport(
       Specifier: "./renamed-reexports"

crates/biome_module_graph/tests/snapshots/test_resolve_exports.snap

@@ -25,18 +25,18 @@ export { A, B };
 
 ```
 Exports {
-  "A" => {
-    ExportOwnExport => JsOwnExport::Type(Module(0) TypeId(2))
-  }
-  "B" => {
-    ExportOwnExport => JsOwnExport::Type(Module(0) TypeId(10))
-  }
   "Union" => {
     ExportOwnExport => JsOwnExport::Binding(3)
   }
   "Union2" => {
     ExportOwnExport => JsOwnExport::Binding(7)
   }
+  "A" => {
+    ExportOwnExport => JsOwnExport::Type(Module(0) TypeId(2))
+  }
+  "B" => {
+    ExportOwnExport => JsOwnExport::Type(Module(0) TypeId(10))
+  }
 }
 Imports {
   No imports