Skip to content

Commit

Permalink
refactor(rust): remove impl Default for ModuleId (#271)
Browse files Browse the repository at this point in the history
<!-- Thank you for contributing! -->

### Description

I have seen some out of index panics for accessing `modules`. This PR should be able to avoid them or raise errors as early as possible.

<!-- 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 15, 2023
1 parent fdbf290 commit b9e4158
Show file tree
Hide file tree
Showing 10 changed files with 61 additions and 50 deletions.
40 changes: 23 additions & 17 deletions crates/rolldown/src/bundler/module_loader/module_loader.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::sync::Arc;

use index_vec::IndexVec;
use rolldown_common::{ImportKind, ModuleId, RawPath, ResourceId};
use rolldown_common::{ImportKind, ImportRecordId, ModuleId, RawPath, ResourceId};
use rolldown_fs::FileSystem;
use rustc_hash::{FxHashMap, FxHashSet};

Expand Down Expand Up @@ -135,22 +135,28 @@ impl<T: FileSystem + 'static + Default> ModuleLoader<T> {
};
match msg {
Msg::NormalModuleDone(task_result) => {
let NormalModuleTaskResult { module_id, ast_symbol, resolved_deps, mut builder, .. } =
task_result;

let import_records = builder.import_records.as_mut().unwrap();

resolved_deps.into_iter_enumerated().for_each(|(import_record_idx, info)| {
let id = self.try_spawn_new_task(&info, false, &mut symbols);
let import_record = &mut import_records[import_record_idx];
import_record.resolved_module = id;

// dynamic import as extra entries if enable code splitting
if import_record.kind == ImportKind::DynamicImport {
dynamic_entries.insert((Some(info.path.unique(&self.input_options.cwd)), id));
}
});

let NormalModuleTaskResult {
module_id,
ast_symbol,
resolved_deps,
mut builder,
raw_import_records,
..
} = task_result;

let import_records = raw_import_records
.into_iter()
.zip(resolved_deps.into_iter())
.map(|(raw_rec, info)| {
let id = self.try_spawn_new_task(&info, false, &mut symbols);
// dynamic import as extra entries if enable code splitting
if raw_rec.kind == ImportKind::DynamicImport {
dynamic_entries.insert((Some(info.path.unique(&self.input_options.cwd)), id));
}
raw_rec.into_import_record(id)
})
.collect::<IndexVec<ImportRecordId, _>>();
builder.import_records = Some(import_records);
self.ctx.intermediate_modules[module_id] = Some(Module::Normal(builder.build()));

symbols.add_ast_symbol(module_id, ast_symbol);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ use std::{path::Path, sync::Arc};
use futures::future::join_all;
use index_vec::IndexVec;
use oxc::{ast::Visit, span::SourceType};
use rolldown_common::{ImportRecord, ImportRecordId, ModuleId, ModuleType, ResourceId, SymbolRef};
use rolldown_common::{
ImportRecordId, ModuleId, ModuleType, RawImportRecord, ResourceId, SymbolRef,
};
use rolldown_error::BuildError;
use rolldown_fs::FileSystem;
use rolldown_oxc::{OxcCompiler, OxcProgram};
Expand Down Expand Up @@ -102,7 +104,6 @@ impl<'task, T: FileSystem + Default + 'static> NormalModuleTask<'task, T> {
builder.named_imports = Some(named_imports);
builder.named_exports = Some(named_exports);
builder.stmt_infos = Some(stmt_infos);
builder.import_records = Some(import_records);
builder.imports = Some(imports);
builder.star_exports = Some(star_exports);
builder.default_export_symbol = export_default_symbol_id;
Expand All @@ -121,6 +122,7 @@ impl<'task, T: FileSystem + Default + 'static> NormalModuleTask<'task, T> {
warnings: self.warnings,
ast_symbol,
builder,
raw_import_records: import_records,
}))
.unwrap();
Ok(())
Expand Down Expand Up @@ -223,7 +225,7 @@ impl<'task, T: FileSystem + Default + 'static> NormalModuleTask<'task, T> {

async fn resolve_dependencies(
&mut self,
dependencies: &IndexVec<ImportRecordId, ImportRecord>,
dependencies: &IndexVec<ImportRecordId, RawImportRecord>,
) -> BatchedResult<IndexVec<ImportRecordId, ResolvedRequestInfo>> {
let jobs = dependencies.iter_enumerated().map(|(idx, item)| {
let specifier = item.module_request.clone();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ impl<'task, T: FileSystem + Default + 'static> RuntimeNormalModuleTask<'task, T>
named_imports,
named_exports,
stmt_infos,
import_records,
import_records: _,
star_exports,
export_default_symbol_id,
imports,
Expand All @@ -59,10 +59,10 @@ impl<'task, T: FileSystem + Default + 'static> RuntimeNormalModuleTask<'task, T>
builder.named_imports = Some(named_imports);
builder.named_exports = Some(named_exports);
builder.stmt_infos = Some(stmt_infos);
builder.import_records = Some(import_records);
builder.imports = Some(imports);
builder.star_exports = Some(star_exports);
builder.default_export_symbol = export_default_symbol_id;
builder.import_records = Some(IndexVec::default());
builder.scope = Some(scope);
builder.exports_kind = exports_kind;
builder.namespace_symbol = Some(namespace_symbol);
Expand All @@ -77,6 +77,7 @@ impl<'task, T: FileSystem + Default + 'static> RuntimeNormalModuleTask<'task, T>
warnings: self.warnings,
ast_symbol: symbol,
builder,
raw_import_records: IndexVec::default(),
}))
.unwrap();
}
Expand Down
3 changes: 2 additions & 1 deletion crates/rolldown/src/bundler/module_loader/task_result.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use index_vec::IndexVec;
use rolldown_common::{ImportRecordId, ModuleId};
use rolldown_common::{ImportRecordId, ModuleId, RawImportRecord};
use rolldown_error::BuildError;

use crate::bundler::module::normal_module_builder::NormalModuleBuilder;
Expand All @@ -10,6 +10,7 @@ pub struct NormalModuleTaskResult {
pub module_id: ModuleId,
pub ast_symbol: AstSymbol,
pub resolved_deps: IndexVec<ImportRecordId, ResolvedRequestInfo>,
pub raw_import_records: IndexVec<ImportRecordId, RawImportRecord>,
pub errors: Vec<BuildError>,
pub warnings: Vec<BuildError>,
pub builder: NormalModuleBuilder,
Expand Down
2 changes: 1 addition & 1 deletion crates/rolldown/src/bundler/stages/link_stage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ impl LinkStage {
.import_records()
.iter()
.filter(|rec| rec.kind.is_static())
.filter_map(|rec| rec.resolved_module.is_valid().then_some(rec.resolved_module))
.map(|rec| rec.resolved_module)
.rev()
.map(Action::Enter),
);
Expand Down
1 change: 0 additions & 1 deletion crates/rolldown/src/bundler/utils/symbols.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ pub struct Symbols {

impl Symbols {
pub fn add_ast_symbol(&mut self, module_id: ModuleId, ast_symbol: AstSymbol) {
debug_assert!(module_id.is_valid());
let module_count = module_id.raw() as usize;
if self.inner.len() <= module_count {
self.inner.resize_with(module_count + 1, IndexVec::default);
Expand Down
8 changes: 4 additions & 4 deletions crates/rolldown/src/bundler/visitors/scanner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ use oxc::{
span::{Atom, Span},
};
use rolldown_common::{
ExportsKind, ImportKind, ImportRecord, ImportRecordId, LocalExport, LocalOrReExport, ModuleId,
ModuleType, NamedImport, ReExport, Specifier, StmtInfo, StmtInfos, SymbolRef,
ExportsKind, ImportKind, ImportRecordId, LocalExport, LocalOrReExport, ModuleId, ModuleType,
NamedImport, RawImportRecord, ReExport, Specifier, StmtInfo, StmtInfos, SymbolRef,
};
use rolldown_oxc::{BindingIdentifierExt, BindingPatternExt};
use rustc_hash::FxHashMap;
Expand All @@ -25,7 +25,7 @@ pub struct ScanResult {
pub named_imports: FxHashMap<SymbolId, NamedImport>,
pub named_exports: FxHashMap<Atom, LocalOrReExport>,
pub stmt_infos: StmtInfos,
pub import_records: IndexVec<ImportRecordId, ImportRecord>,
pub import_records: IndexVec<ImportRecordId, RawImportRecord>,
pub star_exports: Vec<ImportRecordId>,
pub export_default_symbol_id: Option<SymbolId>,
pub imports: FxHashMap<Span, ImportRecordId>,
Expand Down Expand Up @@ -113,7 +113,7 @@ impl<'a> Scanner<'a> {
}

fn add_import_record(&mut self, module_request: &Atom, kind: ImportKind) -> ImportRecordId {
let rec = ImportRecord::new(module_request.clone(), kind);
let rec = RawImportRecord::new(module_request.clone(), kind);
self.result.import_records.push(rec)
}

Expand Down
30 changes: 22 additions & 8 deletions crates/rolldown_common/src/import_record.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,23 +32,37 @@ impl Display for ImportKind {
}

#[derive(Debug)]
pub struct ImportRecord {
pub struct RawImportRecord {
// Module Request
pub module_request: Atom,
pub resolved_module: ModuleId,
// export * as ns from '...'
// import * as ns from '...'
pub is_import_namespace: bool,
pub kind: ImportKind,
}

impl ImportRecord {
impl RawImportRecord {
pub fn new(specifier: Atom, kind: ImportKind) -> Self {
Self {
module_request: specifier,
resolved_module: ModuleId::default(),
is_import_namespace: false,
kind,
Self { module_request: specifier, is_import_namespace: false, kind }
}

pub fn into_import_record(self, resolved_module: ModuleId) -> ImportRecord {
ImportRecord {
module_request: self.module_request,
resolved_module,
is_import_namespace: self.is_import_namespace,
kind: self.kind,
}
}
}

#[derive(Debug)]
pub struct ImportRecord {
// Module Request
pub module_request: Atom,
pub resolved_module: ModuleId,
// export * as ns from '...'
// import * as ns from '...'
pub is_import_namespace: bool,
pub kind: ImportKind,
}
2 changes: 1 addition & 1 deletion crates/rolldown_common/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ mod symbol_ref;
mod wrap_kind;
pub use crate::{
exports_kind::ExportsKind,
import_record::{ImportKind, ImportRecord, ImportRecordId},
import_record::{ImportKind, ImportRecord, ImportRecordId, RawImportRecord},
module_id::ModuleId,
module_path::ResourceId,
module_type::ModuleType,
Expand Down
12 changes: 0 additions & 12 deletions crates/rolldown_common/src/module_id.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,3 @@
index_vec::define_index_type! {
pub struct ModuleId = u32;
}

impl Default for ModuleId {
fn default() -> Self {
Self::from_raw(u32::MAX)
}
}

impl ModuleId {
pub fn is_valid(&self) -> bool {
self.raw() < u32::MAX
}
}

0 comments on commit b9e4158

Please sign in to comment.