feat: deconflicting for symbols of nested scopes (#188)

crates/rolldown/src/bundler/chunk/de_conflict.rs

@@ -5,7 +5,7 @@ use crate::bundler::{graph::graph::Graph, module::Module, utils::renamer::Rename
 
 impl Chunk {
   pub fn de_conflict(&mut self, graph: &Graph) {
-    let mut renamer = Renamer::new(&graph.symbols);
+    let mut renamer = Renamer::new(&graph.symbols, graph.modules.len());
 
     self
       .modules
@@ -52,6 +52,22 @@ impl Chunk {
       renamer.add_top_level_symbol(item.import_ref);
     });
 
+    // rename non-top-level names
+
+    self.modules.iter().copied().for_each(|module| {
+      let Module::Normal(module) = &graph.modules[module] else { return };
+      module.scope.descendants().for_each(|scope_id| {
+        if scope_id == module.scope.root_scope_id() {
+          // Top level symbol are already processed above
+          return;
+        }
+        let bindings = module.scope.get_bindings(scope_id);
+        bindings.iter().for_each(|(_binding_name, symbol_id)| {
+          renamer.add_non_top_level_symbol(module.id, (module.id, *symbol_id).into());
+        });
+      });
+    });
+
     self.canonical_names = renamer.into_canonical_names();
   }
 }

crates/rolldown/src/bundler/utils/renamer.rs

@@ -1,45 +1,80 @@
 use std::borrow::Cow;
 
+use index_vec::IndexVec;
 use oxc::span::Atom;
-use rolldown_common::SymbolRef;
-use rustc_hash::FxHashMap;
+use rolldown_common::{ModuleId, SymbolRef};
+use rustc_hash::{FxHashMap, FxHashSet};
 
 use crate::bundler::graph::symbols::Symbols;
 
 #[derive(Debug)]
 pub struct Renamer<'name> {
-  name_to_count: FxHashMap<Cow<'name, Atom>, u32>,
+  top_level_name_to_count: FxHashMap<Cow<'name, Atom>, u32>,
+  used_canonical_names: FxHashSet<Cow<'name, Atom>>,
+  module_to_used_canonical_name_count: IndexVec<ModuleId, FxHashMap<Cow<'name, Atom>, u32>>,
   canonical_names: FxHashMap<SymbolRef, Atom>,
   symbols: &'name Symbols,
 }
 
 impl<'name> Renamer<'name> {
-  pub fn new(symbols: &'name Symbols) -> Self {
-    Self { name_to_count: FxHashMap::default(), canonical_names: FxHashMap::default(), symbols }
+  pub fn new(symbols: &'name Symbols, modules_len: usize) -> Self {
+    Self {
+      top_level_name_to_count: FxHashMap::default(),
+      canonical_names: FxHashMap::default(),
+      symbols,
+      used_canonical_names: FxHashSet::default(),
+      module_to_used_canonical_name_count: index_vec::index_vec![FxHashMap::default(); modules_len],
+    }
   }
 
   pub fn inc(&mut self, name: Cow<'name, Atom>) {
-    *self.name_to_count.entry(name).or_default() += 1;
+    *self.top_level_name_to_count.entry(name).or_default() += 1;
   }
 
   pub fn add_top_level_symbol(&mut self, symbol_ref: SymbolRef) {
     let canonical_ref = self.symbols.par_canonical_ref_for(symbol_ref);
-    let original_name = self.symbols.get_original_name(canonical_ref);
+    let original_name = Cow::Borrowed(self.symbols.get_original_name(canonical_ref));
 
     match self.canonical_names.entry(canonical_ref) {
-      std::collections::hash_map::Entry::Occupied(_) => {}
+      std::collections::hash_map::Entry::Occupied(_) => {
+        // The symbol is already renamed
+      }
       std::collections::hash_map::Entry::Vacant(vacant) => {
-        let count = self.name_to_count.entry(Cow::Borrowed(original_name)).or_default();
-        if *count == 0 {
-          vacant.insert(original_name.clone());
+        let count = self.top_level_name_to_count.entry(original_name.clone()).or_default();
+        let canonical_name = if *count == 0 {
+          original_name
         } else {
-          vacant.insert(format!("{}${}", original_name, *count).into());
-        }
+          Cow::Owned(format!("{}${}", original_name, *count).into())
+        };
+        self.used_canonical_names.insert(canonical_name.clone());
+        vacant.insert(canonical_name.into_owned());
         *count += 1;
       }
     }
   }
 
+  // non-top-level symbols won't be linked cross-module. So the canonical `SymbolRef` for them are themselves.
+  pub fn add_non_top_level_symbol(&mut self, module_id: ModuleId, canonical_ref: SymbolRef) {
+    let original_name = Cow::Borrowed(self.symbols.get_original_name(canonical_ref));
+
+    match self.canonical_names.entry(canonical_ref) {
+      std::collections::hash_map::Entry::Occupied(_) => {
+        // The symbol is already renamed
+      }
+      std::collections::hash_map::Entry::Vacant(vacant) => {
+        let might_shadowed = self.used_canonical_names.contains(&original_name);
+        if might_shadowed {
+          let used_canonical_name_count = &mut self.module_to_used_canonical_name_count[module_id];
+          // The name is already used in top level, so the default count is 1
+          let count = used_canonical_name_count.entry(original_name.clone()).or_insert(1);
+          let canonical_name = format!("{}${}", original_name, *count);
+          vacant.insert(canonical_name.into());
+          *count += 1;
+        }
+      }
+    }
+  }
+
   pub fn into_canonical_names(self) -> FxHashMap<SymbolRef, Atom> {
     self.canonical_names
   }

crates/rolldown/tests/esbuild/default/keep_names_class_static_name/artifacts.snap

@@ -16,12 +16,12 @@ class D { static get name() {} }
 class E { static set name(x) {} }
 class F { static ['name'] = 0 }
 
-let a = class a { static foo }
-let b = class b { static name }
-let c = class c { static name() {} }
-let d = class d { static get name() {} }
-let e = class e { static set name(x) {} }
-let f = class f { static ['name'] = 0 }
+let a = class a$1 { static foo }
+let b = class b$1 { static name }
+let c = class c$1 { static name() {} }
+let d = class d$1 { static get name() {} }
+let e = class e$1 { static set name(x) {} }
+let f = class f$1 { static ['name'] = 0 }
 
 let a2 = class { static foo }
 let b2 = class { static name }

crates/rolldown/tests/fixtures/deconflict/basic_scoped/a.js

@@ -0,0 +1,3 @@
+const a = 'a.js'
+
+export { a }

crates/rolldown/tests/fixtures/deconflict/basic_scoped/artifacts.snap

@@ -0,0 +1,29 @@
+---
+source: crates/rolldown/tests/common/case.rs
+expression: content
+input_file: crates/rolldown/tests/fixtures/deconflict/basic_scoped
+---
+# Assets
+
+## main.mjs
+
+```js
+import { default as assert_default } from "assert";
+
+// a.js
+const a = 'a.js'
+
+
+
+// main.js
+
+
+const a$1 = 'main.js'
+
+
+function foo(a$1$1) {
+  return [a$1$1, a$1, a]
+}
+
+assert_default.deepEqual(foo('foo'), ['foo', 'main.js', 'a.js'])
+```

crates/rolldown/tests/fixtures/deconflict/basic_scoped/main.js

@@ -0,0 +1,10 @@
+import assert from 'assert'
+import { a as aJs } from './a'
+const a = 'main.js'
+
+
+function foo(a$1) {
+  return [a$1, a, aJs]
+}
+
+assert.deepEqual(foo('foo'), ['foo', 'main.js', 'a.js'])

crates/rolldown/tests/fixtures/deconflict/basic_scoped/test.config.json

@@ -0,0 +1 @@
+{}

packages/rollup-tests/src/failed-tests.json

@@ -35,6 +35,7 @@
   "rollup@function@argument-deoptimization@global-calls: tracks argument mutations of calls to globals",
   "rollup@function@array-from-side-effect: Observes side-effects in Array.from",
   "rollup@function@assign-namespace-to-var: allows a namespace to be assigned to a variable",
+  "rollup@function@assignment-patterns: allows reassigments to default parameters that shadow imports",
   "rollup@function@assignment-to-exports-b: exports are rewritten inside a variable init",
   "rollup@function@assignment-to-exports: exports are kept up-to-date",
   "rollup@function@assignment-to-re-exports-conflict: re-exports are kept up-to-date",
@@ -72,6 +73,7 @@
   "rollup@function@cannot-call-external-namespace: warns if code calls an external namespace",
   "rollup@function@cannot-call-internal-namespace: warns if code calls an internal namespace",
   "rollup@function@cannot-resolve-sourcemap-warning: handles when a sourcemap cannot be resolved in a warning",
+  "rollup@function@catch-block-scope: uses correct scope in catch blocks",
   "rollup@function@catch-dynamic-import-failure: allows catching failed dynamic imports",
   "rollup@function@chained-mutable-array-methods: recognizes side-effects when applying mutable array methods to chained array methods (#3555)",
   "rollup@function@check-exports-exportedBindings-as-a-supplementary-test: check exports and exportedBindings in moduleParsed as a supplementary test",
@@ -108,6 +110,7 @@
   "rollup@function@consistent-renaming-d: consistent renaming test d",
   "rollup@function@consistent-renaming-e: consistent renaming test e",
   "rollup@function@consistent-renaming-f: consistent renaming test f",
+  "rollup@function@consistently-renames-destructured-parameters: destructured parameters are properly renamed (#2418)",
   "rollup@function@context-resolve-skipself: allows a plugin to skip its own resolveId hook when using this.resolve",
   "rollup@function@context-resolve: returns the correct results for the context resolve helper",
   "rollup@function@create-undefined-export-property-compact: creates an export as an exports property even if is has no initializer",
@@ -632,6 +635,7 @@
   "rollup@function@statement-order: correct statement order is preserved even in weird edge cases",
   "rollup@function@strip-bom: Works correctly with BOM files.",
   "rollup@function@switch-cases-are-deconflicted: deconflicts variables in switch cases (#1970)",
+  "rollup@function@switch-scope: uses correct scopes for switch statements",
   "rollup@function@symlink: follows symlinks",
   "rollup@function@synthetic-named-export-entry: does not expose synthetic named exports on entry points",
   "rollup@function@synthetic-named-exports-fallback-es2015: adds a fallback in case synthetic named exports are falsy",

packages/rollup-tests/src/status.json

@@ -1,8 +1,8 @@
 {
   "total": 901,
   "failed": 0,
-  "skipFailed": 642,
+  "skipFailed": 646,
   "ignored": 7,
   "skipped": 0,
-  "passed": 252
+  "passed": 248
 }

packages/rollup-tests/src/status.md

@@ -2,7 +2,7 @@
 |----| ---- |
 | total | 901|
 | failed | 0|
-| skipFailed | 642|
+| skipFailed | 646|
 | ignored | 7|
 | skipped | 0|
-| passed | 252|
+| passed | 248|