Skip to content

Commit

Permalink
fix: invalid output for conflict global and local name (#359)
Browse files Browse the repository at this point in the history
<!-- Thank you for contributing! -->

### Description

Fixes #357.

<!-- Please insert your description here and provide especially info about the "what" this PR is solving -->

### Test Plan

<!-- e.g. is there anything you'd like reviewers to focus on? -->

---
  • Loading branch information
hyf0 committed Nov 22, 2023
1 parent 44c6331 commit d464eee
Show file tree
Hide file tree
Showing 6 changed files with 58 additions and 19 deletions.
2 changes: 1 addition & 1 deletion crates/rolldown/src/bundler/chunk/de_conflict.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ impl Chunk {
.flatten()
.for_each(|name| {
// global names should be reserved
renamer.inc(name);
renamer.reserve(name);
});

self.imports_from_other_chunks.iter().flat_map(|(_, items)| items.iter()).for_each(|item| {
Expand Down
18 changes: 8 additions & 10 deletions crates/rolldown/src/bundler/chunk/render_chunk_imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ impl Chunk {
let mut import_items = named_imports
.iter()
.filter_map(|item| {
let alias = graph.symbols.get_original_name(item.imported_as);
let canonical_ref = graph.symbols.par_canonical_ref_for(item.imported_as);
let alias = &self.canonical_names[&canonical_ref];
match &item.imported {
Specifier::Star => {
is_importee_imported = true;
Expand Down Expand Up @@ -63,18 +64,15 @@ impl Chunk {
let mut import_items = items
.iter()
.map(|item| {
let imported = importee_chunk
.canonical_names
.get(&graph.symbols.par_canonical_ref_for(item.import_ref))
.cloned()
.unwrap();
let Specifier::Literal(alias) = item.export_alias.as_ref().unwrap() else {
let canonical_ref = graph.symbols.par_canonical_ref_for(item.import_ref);
let local_binding = &self.canonical_names[&canonical_ref];
let Specifier::Literal(export_alias) = item.export_alias.as_ref().unwrap() else {
panic!("should not be star import from other chunks")
};
if imported == alias {
format!("{imported}")
if export_alias == local_binding {
format!("{export_alias}")
} else {
format!("{imported} as {alias}")
format!("{export_alias} as {local_binding}")
}
})
.collect::<Vec<_>>();
Expand Down
19 changes: 11 additions & 8 deletions crates/rolldown/src/bundler/utils/renamer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ use super::symbols::Symbols;

#[derive(Debug)]
pub struct Renamer<'name> {
top_level_name_to_count: FxHashMap<Cow<'name, Atom>, u32>,
/// The usage count of top level names
top_level_name_counts: 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>,
Expand All @@ -19,28 +20,27 @@ pub struct Renamer<'name> {
impl<'name> Renamer<'name> {
pub fn new(symbols: &'name Symbols, modules_len: usize) -> Self {
Self {
top_level_name_to_count: FxHashMap::default(),
top_level_name_counts: 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.top_level_name_to_count.entry(name).or_default() += 1;
pub fn reserve(&mut self, name: Cow<'name, Atom>) {
let count = self.top_level_name_counts.entry(name).or_default();
debug_assert!(*count <= 1, "It's unnecessary to reserve a global name twice");
*count = 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 = 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 count = self.top_level_name_to_count.entry(original_name.clone()).or_default();
let count = self.top_level_name_counts.entry(original_name.clone()).or_default();
let canonical_name = if *count == 0 {
original_name
} else {
Expand All @@ -50,6 +50,9 @@ impl<'name> Renamer<'name> {
vacant.insert(canonical_name.into_owned());
*count += 1;
}
std::collections::hash_map::Entry::Occupied(_) => {
// The symbol is already renamed
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
source: crates/rolldown/tests/common/case.rs
expression: content
input_file: crates/rolldown/tests/fixtures/deconflict/conflict_between_global_and_local_binding
---
# Assets

## main.mjs

```js
import { default as assert } from "node:assert";
import { __commonJS as __commonJS$1 } from "./_rolldown_runtime.mjs";
// main.js
var require_main = __commonJS$1({
'main.js'(exports, module) {
module.exports = 'main';
globalThis.__commonJS = 0
__commonJS = 1
assert.equal(__commonJS, 1)
}
});
export default require_main();
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import assert from 'node:assert'
module.exports = 'main';
globalThis.__commonJS = 0
__commonJS = 1
assert.equal(__commonJS, 1)
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"input": {
"external": [
"node:assert"
]
}
}

0 comments on commit d464eee

Please sign in to comment.