diff --git a/changelog.d/9944-module-sync-builtin-exports.md b/changelog.d/9944-module-sync-builtin-exports.md new file mode 100644 index 0000000000..8afb9ea05b --- /dev/null +++ b/changelog.d/9944-module-sync-builtin-exports.md @@ -0,0 +1 @@ +Fixed named imports from Node builtins to retain their ESM export value until `syncBuiltinESMExports()` copies CommonJS namespace changes. diff --git a/crates/perry-codegen/src/runtime_decls/objects.rs b/crates/perry-codegen/src/runtime_decls/objects.rs index 1a9b60f941..e1c704af85 100644 --- a/crates/perry-codegen/src/runtime_decls/objects.rs +++ b/crates/perry-codegen/src/runtime_decls/objects.rs @@ -298,6 +298,11 @@ pub fn declare_phase_b_objects(module: &mut LlModule) { DOUBLE, &[DOUBLE, DOUBLE], ); + module.declare_function( + "js_native_module_named_esm_export_value", + DOUBLE, + &[DOUBLE, DOUBLE], + ); // Issue #894: materialize a NATIVE_MODULE_CLASS_ID-tagged namespace // object for `Expr::NativeModuleRef` when it reaches the value-form // fallback path (the require-call-result-then-member-access shape diff --git a/crates/perry-hir/src/lower/expr_call/mod.rs b/crates/perry-hir/src/lower/expr_call/mod.rs index 752b4a4d98..ed277d5c23 100644 --- a/crates/perry-hir/src/lower/expr_call/mod.rs +++ b/crates/perry-hir/src/lower/expr_call/mod.rs @@ -516,6 +516,20 @@ fn lower_call_inner(ctx: &mut LoweringContext, call: &ast::CallExpr) -> Result bool { /// `satisfies`, angle-bracket assertions, parens) off an expression so a /// cast receiver like `(Readable as any).toWeb(...)` still matches the /// bare-identifier module/class shape the dispatch arms below expect. -fn unwrap_ts_wrappers(e: &ast::Expr) -> &ast::Expr { +pub(super) fn unwrap_ts_wrappers(e: &ast::Expr) -> &ast::Expr { let mut cur = e; loop { match cur { @@ -402,6 +402,33 @@ pub(super) fn is_node_builtin_module_call(ctx: &LoweringContext, callee: &ast::E } } +/// A module that can call `syncBuiltinESMExports()` must invoke named Node +/// imports through their ESM export cells. The ordinary native fast path calls +/// the built-in implementation directly and would therefore ignore a CommonJS +/// replacement copied into the cell by the sync operation. +pub(super) fn named_import_call_needs_esm_binding( + ctx: &LoweringContext, + callee: &ast::Expr, +) -> bool { + let ast::Expr::Ident(ident) = unwrap_ts_wrappers(callee) else { + return false; + }; + let Some((module, Some(export))) = ctx.lookup_native_module(ident.sym.as_ref()) else { + return false; + }; + if !is_node_core(module) + || export == "default" + || (module.strip_prefix("node:").unwrap_or(module) == "module" + && export == "syncBuiltinESMExports") + { + return false; + } + ctx.native_modules.iter().any(|(_, module, method)| { + module.strip_prefix("node:").unwrap_or(module) == "module" + && method.as_deref() == Some("syncBuiltinESMExports") + }) +} + /// node-forge sub-namespace flattening. Unlike the single-level `ns.method()` /// shape the other arms match, forge's API is deeply nested: /// `forge.pki.rsa.generateKeyPair(...)`, `forge.pki.createCertificate()`, diff --git a/crates/perry-hir/src/lower/lower_expr/helpers.rs b/crates/perry-hir/src/lower/lower_expr/helpers.rs index 45679dda18..6c4babbd35 100644 --- a/crates/perry-hir/src/lower/lower_expr/helpers.rs +++ b/crates/perry-hir/src/lower/lower_expr/helpers.rs @@ -381,6 +381,13 @@ pub(crate) fn native_module_binding_value(ctx: &LoweringContext, name: &str) -> } } if let Some(method) = method_name { + if method == "default" { + return Expr::PropertyGet { + byte_offset: 0, + object: Box::new(Expr::NativeModuleRef(module_name.to_string())), + property: method.to_string(), + }; + } // #3946: a `node:process` *property* imported by name // (`import { pid, arch } from "node:process"`) must read // the live process value, not a generic native-module @@ -391,10 +398,23 @@ pub(crate) fn native_module_binding_value(ctx: &LoweringContext, name: &str) -> return e; } } - return Expr::PropertyGet { + // A named ESM import is a live binding to Node's builtin ESM export + // cell, whose value changes only when syncBuiltinESMExports() updates + // that cell. Keep this value read distinct from a property read on a + // default/namespace import, which must observe CommonJS monkey patches + // immediately. + return Expr::Call { + callee: Box::new(Expr::ExternFuncRef { + name: "js_native_module_named_esm_export_value".to_string(), + param_types: vec![Type::String, Type::String], + return_type: Type::Any, + }), + args: vec![ + Expr::String(module_name.to_string()), + Expr::String(method.to_string()), + ], + type_args: vec![], byte_offset: 0, - object: Box::new(Expr::NativeModuleRef(module_name.to_string())), - property: method.to_string(), }; } if ctx.lookup_builtin_module_alias(name).is_none() diff --git a/crates/perry-hir/src/lower/module_decl.rs b/crates/perry-hir/src/lower/module_decl.rs index 66a1843fa1..f39c7dbbb5 100644 --- a/crates/perry-hir/src/lower/module_decl.rs +++ b/crates/perry-hir/src/lower/module_decl.rs @@ -24,6 +24,7 @@ use native_default_import::{ node_submodule_default_export_key, }; use object_literal::is_direct_object_literal; +use static_import_bindings::init_named_cell; pub(super) use static_import_bindings::{ import_is_runtime_erased, pre_register_static_import_bindings, }; @@ -247,6 +248,7 @@ pub(crate) fn lower_module_decl( } else { (source.clone(), Some(imported.clone())) }; + init_named_cell(module, &source, &imported, native_method.as_ref()); ctx.register_native_module(local.clone(), native_module, native_method); // #1991: `perry/ui` exposes these as numeric // `const enum`s in `types/perry/ui/index.d.ts`. diff --git a/crates/perry-hir/src/lower/module_decl/static_import_bindings.rs b/crates/perry-hir/src/lower/module_decl/static_import_bindings.rs index a0ab100da3..d354c6846f 100644 --- a/crates/perry-hir/src/lower/module_decl/static_import_bindings.rs +++ b/crates/perry-hir/src/lower/module_decl/static_import_bindings.rs @@ -4,6 +4,36 @@ use super::*; use swc_ecma_ast as ast; +/// Initialize a Node named import's snapshot-backed ESM export cell before +/// top-level user code can mutate the CommonJS namespace. This also preserves +/// the original value when the binding's first read happens after a mutation. +pub(super) fn init_named_cell( + module: &mut Module, + source: &str, + imported: &str, + method: Option<&String>, +) { + if method.is_none() || !perry_api_manifest::is_node_core_module(source) { + return; + } + module.init.insert( + 0, + Stmt::Expr(Expr::Call { + callee: Box::new(Expr::ExternFuncRef { + name: "js_native_module_named_esm_export_value".to_string(), + param_types: vec![Type::String, Type::String], + return_type: Type::Any, + }), + args: vec![ + Expr::String(source.to_string()), + Expr::String(imported.to_string()), + ], + type_args: vec![], + byte_offset: 0, + }), + ); +} + /// Register ordinary source-module import bindings before statement lowering. /// /// ESM imports are module-scoped and hoisted regardless of where their diff --git a/crates/perry-hir/src/lower/tests.rs b/crates/perry-hir/src/lower/tests.rs index 946f1264e9..353f4883df 100644 --- a/crates/perry-hir/src/lower/tests.rs +++ b/crates/perry-hir/src/lower/tests.rs @@ -486,7 +486,8 @@ fn test_lower_native_module_registration() { fn test_native_module_binding_value_named_import() { // #5242: a named builtin import (`import { relative } from 'path'`) used // as a value (e.g. an object-literal shorthand `{ relative }`) must resolve - // to the callable builtin — `path.relative` — not be dropped to undefined. + // to the snapshot-backed builtin export — `path.relative` — not be dropped + // to undefined or conflated with the mutable default namespace property. let mut ctx = make_ctx(); ctx.register_native_module( "relative".to_string(), @@ -494,15 +495,20 @@ fn test_native_module_binding_value_named_import() { Some("relative".to_string()), ); let value = super::lower_expr::native_module_binding_value(&ctx, "relative"); - match value { - crate::ir::Expr::PropertyGet { - object, property, .. - } => { - assert_eq!(property, "relative"); - assert!(matches!(*object, crate::ir::Expr::NativeModuleRef(ref m) if m == "path")); - } - other => panic!("expected PropertyGet(path.relative), got {other:?}"), - } + assert!(matches!( + value, + crate::ir::Expr::Call { callee, args, .. } + if matches!( + callee.as_ref(), + crate::ir::Expr::ExternFuncRef { name, .. } + if name == "js_native_module_named_esm_export_value" + ) + && matches!( + args.as_slice(), + [crate::ir::Expr::String(module), crate::ir::Expr::String(property)] + if module == "path" && property == "relative" + ) + )); } #[test] @@ -1978,6 +1984,7 @@ mod unresolved_new_global; mod capture_stash; mod mixin_parent_chain; +mod native_module_sync; mod nullish_over_optional_chain; mod ui_widget_add_child; diff --git a/crates/perry-hir/src/lower/tests/native_module_sync.rs b/crates/perry-hir/src/lower/tests/native_module_sync.rs new file mode 100644 index 0000000000..ee45c71208 --- /dev/null +++ b/crates/perry-hir/src/lower/tests/native_module_sync.rs @@ -0,0 +1,27 @@ +#[test] +fn direct_named_calls_use_the_synchronized_esm_cell() { + let source = r#" +import { readFile } from "node:fs"; +import { syncBuiltinESMExports } from "node:module"; +syncBuiltinESMExports(); +readFile(); +"#; + let module = perry_parser::parse_typescript(source, "sync-builtins.ts").expect("source parses"); + let hir = super::super::lower_module(&module, "sync-builtins", "sync-builtins.ts") + .expect("source lowers"); + let dump = format!("{:#?}", hir.init); + assert!( + dump.contains("js_native_module_named_esm_export_value"), + "named call must read the synchronized ESM cell: {dump}" + ); + assert!( + dump.matches("js_native_module_named_esm_export_value") + .count() + >= 2, + "named import must initialize its ESM cell before the dynamic call: {dump}" + ); + assert!( + !dump.contains("module: \"fs\",\n class_name: None,\n object: None,\n method: \"readFile\""), + "named call must not bypass the ESM cell through native dispatch: {dump}" + ); +} diff --git a/crates/perry-runtime/src/object/native_module.rs b/crates/perry-runtime/src/object/native_module.rs index d427223761..10c4746ab3 100644 --- a/crates/perry-runtime/src/object/native_module.rs +++ b/crates/perry-runtime/src/object/native_module.rs @@ -1069,10 +1069,7 @@ fn native_module_string_arg(value: f64) -> Option { Some(String::from_utf8_lossy(bytes).into_owned()) } -/// Snapshot-backed value used for named ESM imports from builtins. CommonJS -/// namespace writes stay isolated until `syncBuiltinESMExports()` copies them. -#[no_mangle] -pub extern "C" fn js_native_module_esm_export_value(module: f64, property: f64) -> f64 { +fn native_module_export_value(module: f64, property: f64, observe_namespace_writes: bool) -> f64 { let Some(module) = native_module_string_arg(module) else { return f64::from_bits(crate::value::TAG_UNDEFINED); }; @@ -1080,13 +1077,14 @@ pub extern "C" fn js_native_module_esm_export_value(module: f64, property: f64) return f64::from_bits(crate::value::TAG_UNDEFINED); }; let module = normalize_native_module_alias(&module).to_string(); - // A user write to the member wins over the built-in snapshot below — - // this entry also serves property reads off the DEFAULT export object - // (`import fs from "node:fs"; fs.rename` after graceful-fs patched it), - // which is Node's live mutable CJS exports object. See - // `native_namespace_user_value`. - if let Some(value) = native_namespace_user_value(&module, &property) { - return value; + if observe_namespace_writes { + // A user write to the member wins over the built-in snapshot below. + // Default and namespace imports expose Node's live mutable CommonJS + // exports object. Named imports pass false and retain their ESM cell + // until syncBuiltinESMExports() refreshes the shared cache. + if let Some(value) = native_namespace_user_value(&module, &property) { + return value; + } } let key = format!("{module}\0{property}"); if let Some(bits) = NATIVE_ESM_EXPORT_VALUES.with(|values| values.borrow().get(&key).copied()) { @@ -1111,6 +1109,20 @@ pub extern "C" fn js_native_module_esm_export_value(module: f64, property: f64) value } +/// Mutable property read used by native-module default and namespace objects. +/// User writes to the CommonJS namespace are observable immediately here. +#[no_mangle] +pub extern "C" fn js_native_module_esm_export_value(module: f64, property: f64) -> f64 { + native_module_export_value(module, property, true) +} + +/// Snapshot-backed value used for named ESM imports from builtins. CommonJS +/// namespace writes stay isolated until `syncBuiltinESMExports()` copies them. +#[no_mangle] +pub extern "C" fn js_native_module_named_esm_export_value(module: f64, property: f64) -> f64 { + native_module_export_value(module, property, false) +} + pub(crate) fn module_constructor_identity_value() -> f64 { const KEY: &str = "module\0Module"; if let Some(bits) = NATIVE_ESM_EXPORT_VALUES.with(|values| values.borrow().get(KEY).copied()) { diff --git a/test-parity/node-suite/module/methods/sync-builtin-exports.ts b/test-parity/node-suite/module/methods/sync-builtin-exports.ts index 1740d90b2e..e68d150299 100644 --- a/test-parity/node-suite/module/methods/sync-builtin-exports.ts +++ b/test-parity/node-suite/module/methods/sync-builtin-exports.ts @@ -4,13 +4,14 @@ import { createRequire, syncBuiltinESMExports } from "node:module"; const req = createRequire(import.meta.url); const cjsFs = req("node:fs"); const original = cjsFs.readFile; -const replacement = function parityReadFile() {}; +const replacement = function parityReadFile() { return "replacement result"; }; try { - console.log("initial identity:", fsDefault === cjsFs, readFile === original); + console.log("initial identity:", fsDefault === cjsFs); cjsFs.readFile = replacement; console.log("before sync:", readFile === original, readFile === replacement); console.log("return:", String(syncBuiltinESMExports())); console.log("after sync:", readFile === original, readFile === replacement); + console.log("after sync call:", (readFile as () => string)()); } finally { cjsFs.readFile = original; syncBuiltinESMExports();