fix(bundler): Use proper algorithm for dependency analysis (#1610)

swc_bundler:
 - Optimize detection of circular imports.

bundler/Cargo.toml

@@ -9,7 +9,7 @@ include = ["Cargo.toml", "build.rs", "src/**/*.rs", "src/**/*.js"]
 license = "Apache-2.0/MIT"
 name = "swc_bundler"
 repository = "https://github.com/swc-project/swc.git"
-version = "0.32.5"
+version = "0.32.7"
 
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 [features]

bundler/src/bundler/chunk/cjs.rs

@@ -63,7 +63,7 @@ where
         module.visit_mut_with(&mut DefaultHandler {
             local_ctxt: info.local_ctxt(),
         });
-        module.sort(info.id, &ctx.graph, &self.cm);
+        module.sort(info.id, &ctx.graph, &ctx.cycles, &self.cm);
 
         let stmt = ModuleItem::Stmt(wrap_module(
             SyntaxContext::empty(),

bundler/src/bundler/chunk/merge.rs

@@ -29,6 +29,7 @@ use EdgeDirection::Outgoing;
 pub(super) struct Ctx {
     /// Full dependency graph.
     pub graph: ModuleGraph,
+    pub cycles: Vec<Vec<ModuleId>>,
     pub merged: CHashSet<ModuleId>,
     pub transitive_remap: CloneMap<SyntaxContext, SyntaxContext>,
     pub export_stars_in_wrapped: Lock<FxHashMap<ModuleId, Vec<SyntaxContext>>>,
@@ -405,7 +406,7 @@ where
 
         inline(self.injected_ctxt, entry);
 
-        entry.sort(id, &ctx.graph, &self.cm);
+        entry.sort(id, &ctx.graph, &ctx.cycles, &self.cm);
 
         // crate::debug::print_hygiene("done", &self.cm, &entry.clone().into());
 

bundler/src/bundler/chunk/mod.rs

@@ -46,7 +46,7 @@ where
         entries: AHashMap<String, TransformedModule>,
     ) -> Result<Vec<Bundle>, Error> {
         let start = Instant::now();
-        let (plan, graph) = self.determine_entries(entries).context("failed to plan")?;
+        let (plan, graph, cycles) = self.determine_entries(entries).context("failed to plan")?;
         let dur = Instant::now() - start;
         log::debug!("Dependency analysis took {:?}", dur);
 
@@ -68,6 +68,7 @@ where
 
         let ctx = Ctx {
             graph,
+            cycles,
             merged: Default::default(),
             transitive_remap: Default::default(),
             export_stars_in_wrapped: Default::default(),

bundler/src/bundler/chunk/plan/mod.rs

@@ -10,6 +10,7 @@ struct PlanBuilder {
     tracked: FxHashSet<(ModuleId, ModuleId)>,
 
     graph: ModuleGraph,
+    cycles: Vec<Vec<ModuleId>>,
     all: Vec<ModuleId>,
 
     kinds: FxHashMap<ModuleId, BundleKind>,
@@ -31,16 +32,7 @@ where
     pub(super) fn determine_entries(
         &self,
         entries: AHashMap<String, TransformedModule>,
-    ) -> Result<(Plan, ModuleGraph), Error> {
-        let plan = self.calculate_plan(entries)?;
-
-        Ok(plan)
-    }
-
-    fn calculate_plan(
-        &self,
-        entries: AHashMap<String, TransformedModule>,
-    ) -> Result<(Plan, ModuleGraph), Error> {
+    ) -> Result<(Plan, ModuleGraph, Vec<Vec<ModuleId>>), Error> {
         let mut builder = PlanBuilder::default();
 
         for (name, module) in entries {
@@ -49,7 +41,7 @@ where
                 None => {}
             }
 
-            self.add_to_graph(&mut builder, module.id);
+            self.add_to_graph(&mut builder, module.id, &mut vec![]);
         }
 
         Ok((
@@ -58,11 +50,31 @@ where
                 all: builder.all,
             },
             builder.graph,
+            builder.cycles,
         ))
     }
 
-    fn add_to_graph(&self, builder: &mut PlanBuilder, module_id: ModuleId) {
-        if !builder.all.contains(&module_id) {
+    fn add_to_graph(
+        &self,
+        builder: &mut PlanBuilder,
+        module_id: ModuleId,
+        path: &mut Vec<ModuleId>,
+    ) {
+        let visited = builder.all.contains(&module_id);
+        let cycle_rpos = if visited {
+            path.iter().rposition(|v| *v == module_id)
+        } else {
+            None
+        };
+
+        if let Some(rpos) = cycle_rpos {
+            let cycle = path[rpos..].to_vec();
+            builder.cycles.push(cycle);
+        }
+
+        path.push(module_id);
+
+        if !visited {
             builder.all.push(module_id);
         }
         builder.graph.add_node(module_id);
@@ -84,8 +96,11 @@ where
 
             // Prevent infinite loops.
             if builder.tracked.insert((module_id, src.module_id)) {
-                self.add_to_graph(builder, src.module_id);
+                self.add_to_graph(builder, src.module_id, path);
             }
         }
+
+        let res = path.pop();
+        debug_assert_eq!(res, Some(module_id));
     }
 }

bundler/src/modules/sort/chunk.rs

@@ -2,10 +2,8 @@ use super::stmt::sort_stmts;
 use crate::dep_graph::ModuleGraph;
 use crate::modules::Modules;
 use crate::ModuleId;
-use fxhash::FxBuildHasher;
 use fxhash::FxHashSet;
 use indexmap::IndexSet;
-use petgraph::algo::all_simple_paths;
 use petgraph::EdgeDirection::Outgoing;
 use std::collections::VecDeque;
 use std::iter::from_fn;
@@ -29,6 +27,7 @@ impl Modules {
         &mut self,
         entry_id: ModuleId,
         graph: &ModuleGraph,
+        cycle: &Vec<Vec<ModuleId>>,
         cm: &Lrc<SourceMap>,
     ) -> Vec<Chunk> {
         let injected_ctxt = self.injected_ctxt;
@@ -51,6 +50,7 @@ impl Modules {
             modules,
             entry_id,
             graph,
+            cycle,
             cm,
         ));
 
@@ -64,6 +64,7 @@ fn toposort_real_modules<'a>(
     mut modules: Vec<(ModuleId, Module)>,
     entry: ModuleId,
     graph: &'a ModuleGraph,
+    cycles: &'a Vec<Vec<ModuleId>>,
     cm: &Lrc<SourceMap>,
 ) -> Vec<Chunk> {
     let mut queue = modules.iter().map(|v| v.0).collect::<VecDeque<_>>();
@@ -72,7 +73,7 @@ fn toposort_real_modules<'a>(
     let mut chunks = vec![];
 
     let start = Instant::now();
-    let sorted_ids = toposort_real_module_ids(queue, graph).collect::<Vec<_>>();
+    let sorted_ids = toposort_real_module_ids(queue, graph, &cycles).collect::<Vec<_>>();
     let end = Instant::now();
     log::debug!("Toposort of module ids took {:?}", end - start);
     for ids in sorted_ids {
@@ -128,50 +129,35 @@ fn toposort_real_modules<'a>(
     chunks
 }
 
-/// Get all modules in a cycle.
-fn all_modules_in_circle(
+fn cycles_for(
+    cycles: &Vec<Vec<ModuleId>>,
     id: ModuleId,
-    done: &FxHashSet<ModuleId>,
-    already_in_index: &mut IndexSet<ModuleId, FxBuildHasher>,
-    graph: &ModuleGraph,
-) -> IndexSet<ModuleId, FxBuildHasher> {
-    let deps = graph
-        .neighbors_directed(id, Outgoing)
-        .filter(|dep| !done.contains(&dep) && !already_in_index.contains(dep))
-        .collect::<Vec<_>>();
-
-    let mut ids = deps
+    checked: &mut Vec<ModuleId>,
+) -> IndexSet<ModuleId> {
+    checked.push(id);
+    let mut v = cycles
         .iter()
-        .copied()
-        .flat_map(|dep| {
-            let mut paths =
-                all_simple_paths::<Vec<_>, _>(&graph, dep, id, 0, None).collect::<Vec<_>>();
-
-            for path in paths.iter_mut() {
-                path.reverse();
-            }
-
-            paths
-        })
+        .filter(|v| v.contains(&id))
         .flatten()
-        .filter(|module_id| !done.contains(&module_id) && !already_in_index.contains(module_id))
-        .collect::<IndexSet<ModuleId, FxBuildHasher>>();
+        .copied()
+        .collect::<IndexSet<_>>();
 
-    already_in_index.extend(ids.iter().copied());
-    let mut new_ids = IndexSet::<_, FxBuildHasher>::default();
+    let ids = v.clone();
 
-    for &dep_id in &ids {
-        let others = all_modules_in_circle(dep_id, done, already_in_index, graph);
-        new_ids.extend(others)
+    for added in ids {
+        if checked.contains(&added) {
+            continue;
+        }
+        v.extend(cycles_for(cycles, added, checked));
     }
-    ids.extend(new_ids);
 
-    ids
+    v
 }
 
 fn toposort_real_module_ids<'a>(
     mut queue: VecDeque<ModuleId>,
     graph: &'a ModuleGraph,
+    cycles: &'a Vec<Vec<ModuleId>>,
 ) -> impl 'a + Iterator<Item = Vec<ModuleId>> {
     let mut done = FxHashSet::<ModuleId>::default();
 
@@ -197,8 +183,7 @@ fn toposort_real_module_ids<'a>(
 
             // dbg!(&deps);
 
-            let mut all_modules_in_circle =
-                all_modules_in_circle(id, &done, &mut Default::default(), graph);
+            let mut all_modules_in_circle = cycles_for(cycles, id, &mut Default::default());
             all_modules_in_circle.reverse();
 
             if all_modules_in_circle.is_empty() {

bundler/src/modules/sort/mod.rs

@@ -19,12 +19,18 @@ impl Modules {
     /// dependency between statements.
     ///
     /// TODO: Change this to return [Module].
-    pub fn sort(&mut self, entry_id: ModuleId, module_graph: &ModuleGraph, cm: &Lrc<SourceMap>) {
+    pub fn sort(
+        &mut self,
+        entry_id: ModuleId,
+        module_graph: &ModuleGraph,
+        cycles: &Vec<Vec<ModuleId>>,
+        cm: &Lrc<SourceMap>,
+    ) {
         log::debug!("Sorting {:?}", entry_id);
 
         let injected_ctxt = self.injected_ctxt;
         let start = Instant::now();
-        let chunks = self.take_chunks(entry_id, module_graph, cm);
+        let chunks = self.take_chunks(entry_id, module_graph, cycles, cm);
         let dur = Instant::now() - start;
         log::debug!("Sorting took {:?}", dur);
 

bundler/tests/fixture/deno-8211-3/output/entry.inlined.ts

@@ -1837,9 +1837,9 @@ function hasInvalidTimeData(obj) {
     } else return false;
 }
 const INVALID = "Invalid DateTime";
+const INVALID1 = "Invalid Duration";
 let intlDTCache = {
 };
-const INVALID1 = "Invalid Duration";
 let now = ()=>Date.now()
 , defaultZone = null, defaultLocale = null, defaultNumberingSystem = null, defaultOutputCalendar = null, throwOnInvalid = false;
 const INVALID2 = "Invalid Interval";
@@ -2575,6 +2575,17 @@ function diffRelative(start, end, opts) {
     }
     return format(0, opts.units[opts.units.length - 1]);
 }
+function friendlyDuration(durationish) {
+    if (isNumber(durationish)) {
+        return Duration.fromMillis(durationish);
+    } else if (Duration.isDuration(durationish)) {
+        return durationish;
+    } else if (typeof durationish === "object") {
+        return Duration.fromObject(durationish);
+    } else {
+        throw new InvalidArgumentError(`Unknown duration argument ${durationish} of type ${typeof durationish}`);
+    }
+}
 class DateTime {
     constructor(config1){
         const zone1 = config1.zone || Settings.defaultZone;
@@ -3709,17 +3720,6 @@ class Locale {
         return this.locale === other.locale && this.numberingSystem === other.numberingSystem && this.outputCalendar === other.outputCalendar;
     }
 }
-function friendlyDuration(durationish) {
-    if (isNumber(durationish)) {
-        return Duration.fromMillis(durationish);
-    } else if (Duration.isDuration(durationish)) {
-        return durationish;
-    } else if (typeof durationish === "object") {
-        return Duration.fromObject(durationish);
-    } else {
-        throw new InvalidArgumentError(`Unknown duration argument ${durationish} of type ${typeof durationish}`);
-    }
-}
 class Settings {
     static get now() {
         return now;

bundler/tests/fixture/deno-8211-3/output/entry.ts

@@ -1837,9 +1837,9 @@ function hasInvalidTimeData(obj) {
     } else return false;
 }
 const INVALID = "Invalid DateTime";
+const INVALID1 = "Invalid Duration";
 let intlDTCache = {
 };
-const INVALID1 = "Invalid Duration";
 let now = ()=>Date.now()
 , defaultZone = null, defaultLocale = null, defaultNumberingSystem = null, defaultOutputCalendar = null, throwOnInvalid = false;
 const INVALID2 = "Invalid Interval";
@@ -2576,6 +2576,17 @@ function diffRelative(start, end, opts) {
     }
     return format(0, opts.units[opts.units.length - 1]);
 }
+function friendlyDuration(durationish) {
+    if (isNumber(durationish)) {
+        return Duration.fromMillis(durationish);
+    } else if (Duration.isDuration(durationish)) {
+        return durationish;
+    } else if (typeof durationish === "object") {
+        return Duration.fromObject(durationish);
+    } else {
+        throw new InvalidArgumentError(`Unknown duration argument ${durationish} of type ${typeof durationish}`);
+    }
+}
 class DateTime {
     constructor(config1){
         const zone1 = config1.zone || Settings.defaultZone;
@@ -3710,17 +3721,6 @@ class Locale {
         return this.locale === other.locale && this.numberingSystem === other.numberingSystem && this.outputCalendar === other.outputCalendar;
     }
 }
-function friendlyDuration(durationish) {
-    if (isNumber(durationish)) {
-        return Duration.fromMillis(durationish);
-    } else if (Duration.isDuration(durationish)) {
-        return durationish;
-    } else if (typeof durationish === "object") {
-        return Duration.fromObject(durationish);
-    } else {
-        throw new InvalidArgumentError(`Unknown duration argument ${durationish} of type ${typeof durationish}`);
-    }
-}
 class Settings {
     static get now() {
         return now;

spack/tests/pass/circular/top-level-idents/output/entry.js

@@ -1,4 +1,4 @@
 console.log('a');
-console.log('c');
 console.log('b');
+console.log('c');
 console.log('entry');