Skip to content

Commit

Permalink
fix: ensure valid output for import the same cjs twice (#323)
Browse files Browse the repository at this point in the history
<!-- Thank you for contributing! -->

### Description

https://hyrious.me/esbuild-repl/?version=0.18.19&b=e%00entry.js%00import+%7B+a+%7D+from+%27.%2Ffile%27%0Aconsole.log%28a%29%0Aimport+%7B+a+as+a2+%7D+from+%27.%2Ffile%27%0Aconsole.log%28a2%29&b=%00file.js%00module.exports.a+%3D+1&o=--bundle+--outdir%3D%22%2F%22+--format%3Desm+--splitting

---

This PR also remove concept `ReExport`. A `ReExport` is a combination of `LocalExport` and `NamedExport`. This reduces complexity of dealing with `ReExport`.

<!-- 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 authored Nov 19, 2023
1 parent 9c75b3f commit 87b5000
Show file tree
Hide file tree
Showing 25 changed files with 217 additions and 200 deletions.
81 changes: 39 additions & 42 deletions crates/rolldown/src/bundler/linker/linker.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use rolldown_common::{ExportsKind, LocalOrReExport, ModuleId, NamedImport, Specifier, SymbolRef};
use rolldown_common::{ExportsKind, ModuleId, NamedImport, Specifier, SymbolRef};
use rustc_hash::FxHashSet;

use super::linker_info::{LinkingInfo, LinkingInfoVec};
Expand All @@ -21,7 +21,7 @@ impl<'graph> Linker<'graph> {
// Here take the symbols to avoid borrow graph and mut borrow graph at same time
let mut symbols = std::mem::take(&mut self.graph.symbols);

self.mark_module_wrapped(&mut symbols, linking_infos);
self.mark_module_wrapped(&symbols, linking_infos);

// Create symbols for external module
self.mark_extra_symbols(&mut symbols);
Expand All @@ -39,7 +39,7 @@ impl<'graph> Linker<'graph> {
&mut FxHashSet::default(),
);
let importer_linking_info = &mut linking_infos[*id];
importer.create_initial_resolved_exports(importer_linking_info, &mut symbols);
importer.create_initial_resolved_exports(importer_linking_info);
let resolved = importer.resolve_star_exports(&self.graph.modules);
importer_linking_info.resolved_star_exports = resolved;
}
Expand Down Expand Up @@ -90,7 +90,7 @@ impl<'graph> Linker<'graph> {
}

// TODO: should move this to a separate stage
fn mark_module_wrapped(&self, symbols: &mut Symbols, linking_infos: &mut LinkingInfoVec) {
fn mark_module_wrapped(&self, _symbols: &Symbols, linking_infos: &mut LinkingInfoVec) {
// Generate symbol for import warp module
// Case esm import commonjs, eg var commonjs_ns = __toESM(require_a())
// Case commonjs require esm, eg (init_esm(), __toCommonJS(esm_ns))
Expand All @@ -112,11 +112,11 @@ impl<'graph> Linker<'graph> {
importer_linking_info.reference_symbol_in_facade_stmt_infos(importee_warp_symbol);
match (importer.exports_kind, importee.exports_kind) {
(ExportsKind::Esm | ExportsKind::CommonJs, ExportsKind::CommonJs) => {
importer.create_local_symbol_for_import_cjs(
importee,
importer_linking_info,
symbols,
);
// importer.create_local_symbol_for_import_cjs(
// importee,
// importer_linking_info,
// symbols,
// );
importer_linking_info.reference_symbol_in_facade_stmt_infos(
self.graph.runtime.resolve_symbol("__toESM"),
);
Expand Down Expand Up @@ -165,16 +165,16 @@ impl<'graph> Linker<'graph> {
extra_symbols.push((import_record.resolved_module, info.imported.clone()));
}
});
importer.named_exports.iter().for_each(|(_, export)| match &export {
LocalOrReExport::Local(_) => {}
LocalOrReExport::Re(re) => {
let import_record = &importer.import_records[re.record_id];
let importee = &self.graph.modules[import_record.resolved_module];
if let Module::External(_) = importee {
extra_symbols.push((import_record.resolved_module, re.imported.clone()));
}
}
});
// importer.named_exports.iter().for_each(|(_, export)| match &export {
// LocalOrReExport::Local(_) => {}
// LocalOrReExport::Re(re) => {
// let import_record = &importer.import_records[re.record_id];
// let importee = &self.graph.modules[import_record.resolved_module];
// if let Module::External(_) = importee {
// extra_symbols.push((import_record.resolved_module, re.imported.clone()));
// }
// }
// });
}
Module::External(_) => {}
}
Expand Down Expand Up @@ -212,6 +212,7 @@ impl<'graph> Linker<'graph> {
Module::Normal(importee) => {
match Self::match_import_with_export(
modules,
importer,
importee,
&linking_infos[importee.id],
&linking_infos[importer.id],
Expand Down Expand Up @@ -271,14 +272,15 @@ impl<'graph> Linker<'graph> {

for symbol_ref in potentially_ambiguous_symbol_refs {
match &modules[symbol_ref.owner] {
Module::Normal(module) => {
let module_linking_info = &linking_infos[module.id];
if let Some(info) = module.named_imports.get(&symbol_ref.symbol) {
let importee_id = module.import_records[info.record_id].resolved_module;
Module::Normal(importer) => {
let module_linking_info = &linking_infos[importer.id];
if let Some(info) = importer.named_imports.get(&symbol_ref.symbol) {
let importee_id = importer.import_records[info.record_id].resolved_module;
match &modules[importee_id] {
Module::Normal(importee) => {
results.push(Self::match_import_with_export(
modules,
importer,
importee,
&linking_infos[importee_id],
module_linking_info,
Expand All @@ -288,11 +290,12 @@ impl<'graph> Linker<'graph> {
Module::External(_) => {}
}
} else if let Some(info) = module_linking_info.export_from_map.get(&symbol_ref.symbol) {
let importee_id = module.import_records[info.record_id].resolved_module;
let importee_id = importer.import_records[info.record_id].resolved_module;
match &modules[importee_id] {
Module::Normal(importee) => {
results.push(Self::match_import_with_export(
modules,
importer,
importee,
&linking_infos[importee_id],
module_linking_info,
Expand Down Expand Up @@ -323,18 +326,25 @@ impl<'graph> Linker<'graph> {
}

pub fn match_import_with_export(
modules: &ModuleVec,
_modules: &ModuleVec,
importer: &NormalModule,
importee: &NormalModule,
importee_linking_info: &LinkingInfo,
importer_linking_info: &LinkingInfo,
_importer_linking_info: &LinkingInfo,
info: &NamedImport,
) -> MatchImportKind {
// If importee module is commonjs module, it will generate property access to namespace symbol
// The namespace symbols should be importer created local symbol.
if importee.exports_kind == ExportsKind::CommonJs {
return MatchImportKind::Namespace(
importer_linking_info.local_symbol_for_import_cjs[&importee.id],
);
let rec = &importer.import_records[info.record_id];
match info.imported {
Specifier::Star => {
return MatchImportKind::Found(rec.namespace_ref);
}
Specifier::Literal(_) => {
return MatchImportKind::Namespace(rec.namespace_ref);
}
}
}

match &info.imported {
Expand All @@ -351,19 +361,6 @@ impl<'graph> Linker<'graph> {
potentially_ambiguous_export.clone(),
);
}
if let Some(id) = resolved_export.export_from {
let module = &modules[id];
match module {
Module::Normal(module) => {
if module.exports_kind == ExportsKind::CommonJs {
return MatchImportKind::Namespace(
importer_linking_info.local_symbol_for_import_cjs[&module.id],
);
}
}
Module::External(_) => {}
}
}
return MatchImportKind::Found(resolved_export.symbol_ref);
}
}
Expand Down
2 changes: 0 additions & 2 deletions crates/rolldown/src/bundler/linker/linker_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ pub struct LinkingInfo {
// The unknown export name will be resolved at runtime.
// esbuild add it to `ExportKind`, but the linker shouldn't mutate the module.
pub has_dynamic_exports: bool,
// Store the local symbol for esm import cjs. eg. `var import_ns = __toESM(require_cjs())`
pub local_symbol_for_import_cjs: FxHashMap<ModuleId, SymbolRef>,
}

impl LinkingInfo {
Expand Down
52 changes: 6 additions & 46 deletions crates/rolldown/src/bundler/module/normal_module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use oxc::{
span::{Atom, Span},
};
use rolldown_common::{
ExportsKind, ImportKind, ImportRecord, ImportRecordId, LocalOrReExport, ModuleId, ModuleType,
ExportsKind, ImportKind, ImportRecord, ImportRecordId, LocalExport, ModuleId, ModuleType,
NamedImport, ResolvedExport, ResourceId, StmtInfo, StmtInfos, SymbolRef,
};
use rolldown_oxc::OxcProgram;
Expand Down Expand Up @@ -34,7 +34,7 @@ pub struct NormalModule {
pub namespace_symbol: SymbolRef,
pub ast: OxcProgram,
pub named_imports: FxHashMap<SymbolId, NamedImport>,
pub named_exports: FxHashMap<Atom, LocalOrReExport>,
pub named_exports: FxHashMap<Atom, LocalExport>,
/// `stmt_infos[0]` represents the namespace binding statement
pub stmt_infos: StmtInfos,
pub import_records: IndexVec<ImportRecordId, ImportRecord>,
Expand Down Expand Up @@ -89,36 +89,10 @@ impl NormalModule {
}
}

pub fn create_initial_resolved_exports(
&self,
self_linking_info: &mut LinkingInfo,
symbols: &mut Symbols,
) {
self.named_exports.iter().for_each(|(name, local_or_re_export)| {
let resolved_export = match local_or_re_export {
LocalOrReExport::Local(local) => ResolvedExport {
symbol_ref: local.referenced,
export_from: None,
potentially_ambiguous_symbol_refs: None,
},
LocalOrReExport::Re(re) => {
let symbol_ref = self.declare_symbol(name.clone(), self_linking_info, symbols);
self_linking_info.export_from_map.insert(
symbol_ref.symbol,
NamedImport {
imported: re.imported.clone(),
imported_as: symbol_ref,
record_id: re.record_id,
},
);
let rec = &self.import_records[re.record_id];
ResolvedExport {
symbol_ref,
export_from: Some(rec.resolved_module),
potentially_ambiguous_symbol_refs: None,
}
}
};
pub fn create_initial_resolved_exports(&self, self_linking_info: &mut LinkingInfo) {
self.named_exports.iter().for_each(|(name, local)| {
let resolved_export =
ResolvedExport { symbol_ref: local.referenced, potentially_ambiguous_symbol_refs: None };
self_linking_info.resolved_exports.insert(name.clone(), resolved_export);
});
}
Expand Down Expand Up @@ -234,20 +208,6 @@ impl NormalModule {
symbol_ref
}

#[allow(clippy::map_entry, clippy::use_self)]
pub fn create_local_symbol_for_import_cjs(
&self,
importee: &NormalModule,
self_linking_info: &mut LinkingInfo,
symbols: &mut Symbols,
) {
if !self_linking_info.local_symbol_for_import_cjs.contains_key(&importee.id) {
let name = format!("import_{}", importee.repr_name).into();
let symbol_ref = self.declare_symbol(name, self_linking_info, symbols);
self_linking_info.local_symbol_for_import_cjs.insert(importee.id, symbol_ref);
}
}

pub fn star_export_modules(&self) -> impl Iterator<Item = ModuleId> + '_ {
self.star_exports.iter().map(|rec_id| {
let rec = &self.import_records[*rec_id];
Expand Down
4 changes: 2 additions & 2 deletions crates/rolldown/src/bundler/module/normal_module_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use oxc::{
span::{Atom, Span},
};
use rolldown_common::{
ExportsKind, ImportRecord, ImportRecordId, LocalOrReExport, ModuleId, ModuleType, NamedImport,
ExportsKind, ImportRecord, ImportRecordId, LocalExport, ModuleId, ModuleType, NamedImport,
ResourceId, StmtInfos, SymbolRef,
};
use rolldown_oxc::OxcProgram;
Expand All @@ -21,7 +21,7 @@ pub struct NormalModuleBuilder {
pub path: Option<ResourceId>,
pub ast: Option<OxcProgram>,
pub named_imports: Option<FxHashMap<SymbolId, NamedImport>>,
pub named_exports: Option<FxHashMap<Atom, LocalOrReExport>>,
pub named_exports: Option<FxHashMap<Atom, LocalExport>>,
pub stmt_infos: Option<StmtInfos>,
pub import_records: Option<IndexVec<ImportRecordId, ImportRecord>>,
pub imports: Option<FxHashMap<Span, ImportRecordId>>,
Expand Down
12 changes: 4 additions & 8 deletions crates/rolldown/src/bundler/renderer/impl_visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,12 @@ impl<'ast, 'r> Visit<'ast> for AstRenderer<'r> {
let importee = &self.ctx.graph.modules[importee_id];
let importee_linking_info = &self.ctx.graph.linking_infos[importee_id];
let Module::Normal(importee) = importee else { return };

if importee.exports_kind == ExportsKind::CommonJs {
self.hoisted_module_declaration(
decl.span.start,
self.ctx.generate_import_commonjs_module(
importee,
&self.ctx.graph.linking_infos[importee.id],
true,
Some(decl.span),
),
);
} else if let Some(wrap_ref) = importee_linking_info.wrapper_ref {
Expand All @@ -83,11 +81,9 @@ impl<'ast, 'r> Visit<'ast> for AstRenderer<'r> {
decl.span.start,
format!(
"{re_export_runtime_symbol_name}({namespace_name}, {});",
self.ctx.generate_import_commonjs_module(
importee,
&self.ctx.graph.linking_infos[importee.id],
false
)
self
.ctx
.generate_import_commonjs_module(&self.ctx.graph.linking_infos[importee.id], None)
),
);
}
Expand Down
3 changes: 1 addition & 2 deletions crates/rolldown/src/bundler/renderer/render_esm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@ impl<'r> AstRenderer<'r> {
self.ctx.hoisted_module_declaration(
named_decl.span.start,
self.ctx.generate_import_commonjs_module(
importee,
&self.ctx.graph.linking_infos[importee.id],
true,
Some(named_decl.span),
),
);
}
Expand Down
3 changes: 1 addition & 2 deletions crates/rolldown/src/bundler/renderer/render_wrapped_esm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,8 @@ impl<'r> AstRenderer<'r> {
self.ctx.hoisted_module_declaration(
named_decl.span.start,
self.ctx.generate_import_commonjs_module(
importee,
&self.ctx.graph.linking_infos[importee.id],
true,
Some(named_decl.span),
),
);
}
Expand Down
22 changes: 5 additions & 17 deletions crates/rolldown/src/bundler/renderer/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ use oxc::span::{Atom, Span};
use rolldown_common::SymbolRef;
use string_wizard::UpdateOptions;

use crate::bundler::{
linker::linker_info::LinkingInfo,
module::{Module, NormalModule},
};
use crate::bundler::{linker::linker_info::LinkingInfo, module::Module};

use super::{AstRenderContext, AstRenderer};
impl<'r> AstRenderContext<'r> {
Expand Down Expand Up @@ -40,27 +37,18 @@ impl<'r> AstRenderContext<'r> {

pub fn generate_import_commonjs_module(
&self,
importee: &NormalModule,
importee_linking_info: &LinkingInfo,
with_declaration: bool,
with_declaration: Option<Span>,
) -> String {
let wrap_ref_name = self.canonical_name_for(importee_linking_info.wrapper_ref.unwrap());
let to_esm_ref_name = self.canonical_name_for_runtime("__toESM");
let code = format!(
"{to_esm_ref_name}({wrap_ref_name}(){})",
if self.module.module_type.is_esm() { ", 1" } else { "" }
);
if with_declaration {
let symbol_ref =
self.linking_info.local_symbol_for_import_cjs.get(&importee.id).copied().unwrap_or_else(
|| {
panic!(
"Cannot find local symbol for importee: {:?} with importer {:?} {:?}",
importee.resource_id, self.module.resource_id, self.module.exports_kind
)
},
);
let final_name = self.canonical_name_for(symbol_ref);
if let Some(span) = with_declaration {
let rec = &self.module.import_records[self.module.imports[&span]];
let final_name = self.canonical_name_for(rec.namespace_ref);
format!("var {final_name} = {code};\n")
} else {
code
Expand Down
Loading

0 comments on commit 87b5000

Please sign in to comment.