Skip to content

[Task]: Avoid unsafe that is potentially wrong #9242

@sapphi-red

Description

@sapphi-red
  • // `import`ing a module with `ExportsKind::None` promotes it to `ExportsKind::Esm`.
    // SAFETY: If `importee` and `importer` are different modules, no aliasing occurs.
    // If they are the same (self-import), writing `exports_kind` is still correct.
    unsafe {
    let importee_mut = addr_of!(*importee).cast_mut();
    (&mut (*importee_mut)).exports_kind = ExportsKind::Esm;
    }
  • let stmt_infos = unsafe { &mut *(addr_of!(importer.stmt_infos).cast_mut()) };
    • by claude:

      Yes, there is technically UB here, but it's a different and lower-risk pattern than the one in determine_module_exports_kind.
      Lines 47 and 49 create &mut references to fields of importer (which is &NormalModule — immutable):

      let stmt_infos = unsafe { &mut *(addr_of!(importer.stmt_infos).cast_mut()) };
      let depended_runtime_helper_map =
        unsafe { &mut *(addr_of!(importer.depended_runtime_helper).cast_mut()) };

      This violates Rust's aliasing rules: &importer (immutable, covers the whole struct) coexists with &mut importer.stmt_infos (mutable).

  • unsafe impl Send for EcmaAst {}
    unsafe impl Sync for EcmaAst {}
    • we should have a comment or fix the UB if exists
    • by claude:

      Send: Safe in practice. Moving the entire EcmaAst (self-cell of allocator + program together) to another thread is fine since no dangling references remain.
      Sync: Technically unsound, but safe given current usage patterns. The concern is:

      1. Program<'cell> contains Cell types (e.g., Cell<Option<ScopeId>> on AST nodes). Cell is !Sync because &Cell<T> allows mutation via .set(). If two threads hold &EcmaAst and both call .set() on the same Cell, that's a data race.
      2. Allocator (wraps bumpalo::Bump) is !Sync because &Bump allows allocation, which mutates internal state without synchronization. EcmaAst::allocator() exposes &Allocator.

      In practice this is fine because:

      • &EcmaAst is only used for reading the AST in parallel (par_iter in scan/link stages)
      • Mutations go through &mut EcmaAst via with_mut(), which requires exclusive access
      • Nobody allocates through &Allocator from shared references

      So it's a "safe by convention" pattern — correct given current usage, but fragile. A future change that mutates Cell fields or allocates through a shared &EcmaAst would silently introduce a data race.

Metadata

Metadata

Type

Priority

None yet

Effort

None yet

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions