diff --git a/changelog.d/8380-cjs-wrap-builtin-coverage.md b/changelog.d/8380-cjs-wrap-builtin-coverage.md new file mode 100644 index 0000000000..f48b4489c8 --- /dev/null +++ b/changelog.d/8380-cjs-wrap-builtin-coverage.md @@ -0,0 +1 @@ +Generate the CJS-wrap `__perry_cjs_require_is_builtin` predicate from the shared `NODE_BUILTIN_MODULES` table so computed `require(specifier)` calls for previously-omitted built-ins (`tls`, `dgram`, `domain`, `fs/promises`, `inspector`, `repl`, `stream/web`, `v8`, `vm`, `wasi`, …) route through `createRequire` instead of raising `MODULE_NOT_FOUND`. Also back built-in named re-exports with `_cjs.` instead of the dropped `import _req_N` binding, and match the full normalized specifier when classifying built-ins. diff --git a/crates/perry-hir/src/ir/mod.rs b/crates/perry-hir/src/ir/mod.rs index 46e46c530b..2963dc48d9 100644 --- a/crates/perry-hir/src/ir/mod.rs +++ b/crates/perry-hir/src/ir/mod.rs @@ -24,7 +24,7 @@ pub use constants::{ current_module_has_allow_dynamic_at, current_module_line_at, current_module_source_mentions_global_this, current_module_source_slice, determine_module_kind, dynamic_stdlib_allowed_for_package, env_define_lookup, is_compile_package_override, - is_native_module, is_native_module_with_externals, is_node_builtin_module, + is_native_module, is_native_module_with_externals, is_node_builtin_module, NODE_BUILTIN_MODULES, package_name_for_source_path, precompile_capture_enabled, precompile_result_at, refuse_dynamic_stdlib_dispatch_enabled, requires_stdlib, set_allow_dynamic_stdlib_packages, set_compile_packages_override, set_current_module_source, set_env_defines, diff --git a/crates/perry/src/commands/compile/cjs_wrap/wrap.rs b/crates/perry/src/commands/compile/cjs_wrap/wrap.rs index c0dbcd8841..f7dcd234a3 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/wrap.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/wrap.rs @@ -160,9 +160,14 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( let builtin_requires: Vec = require_specs .iter() .filter(|spec| { + // Match the complete normalized specifier (`fs/promises`, + // `path/win32`, …) against the shared built-in table instead of + // the truncated base name, so unsupported subpaths such as + // `fs/unknown` fall through to compiled-module resolution rather + // than being routed to `createRequire`. Every valid built-in + // subpath is already an entry in `NODE_BUILTIN_MODULES`. let normalized = spec.strip_prefix("node:").unwrap_or(spec); - let base = normalized.split('/').next().unwrap_or(normalized); - perry_hir::is_node_builtin_module(base) + perry_hir::is_node_builtin_module(normalized) }) .cloned() .collect(); @@ -304,8 +309,7 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( // in the IIFE body and `require("process")` goes through the // synthetic require, which resolves builtins via createRequire. let normalized = spec.strip_prefix("node:").unwrap_or(spec); - let base = normalized.split('/').next().unwrap_or(normalized); - if perry_hir::is_node_builtin_module(base) { + if perry_hir::is_node_builtin_module(normalized) { continue; } if import_local_names.iter().any(|n| n == alias) { @@ -663,10 +667,21 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( named_reexport_requires .iter() .filter_map(|(name, spec)| { - require_specs - .iter() - .position(|s| s == spec) - .map(|n| format!("export {{ {} as {} }};", import_local_names[n], name)) + let n = require_specs.iter().position(|s| s == spec)?; + if builtin_requires.contains(spec) { + // #8343 followup: built-in specs no longer hoist a static + // `import _req_N` (the codegen doesn't initialize + // native-module import bindings in CJS-wrapped modules), + // so `export { _req_N as name }` would reference an + // undeclared ESM binding. The IIFE body's + // `exports.name = require("")` resolves through + // the synthetic require's `createRequire` arm and populates + // `_cjs.name`, so back the re-export with that — the same + // surface `named_export_decls` uses below. + Some(format!("export const {name} = _cjs.{name};")) + } else { + Some(format!("export {{ {} as {} }};", import_local_names[n], name)) + } }) .collect::>() .join("\n") @@ -780,8 +795,7 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( // the synthetic require (which uses createRequire for builtins). .filter(|(_, spec, _)| { let normalized = spec.strip_prefix("node:").unwrap_or(spec); - let base = normalized.split('/').next().unwrap_or(normalized); - !perry_hir::is_node_builtin_module(base) + !perry_hir::is_node_builtin_module(normalized) }) .map(|(_, _, range)| range) .collect::>(); @@ -870,6 +884,20 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( } else { "__perry_cjs_factory" }; + // Generate the `__perry_cjs_require_is_builtin` switch cases from the + // shared `NODE_BUILTIN_MODULES` table so the dynamic/computed `require` + // arm stays in sync with `perry_hir::is_node_builtin_module`. The + // hardcoded list previously omitted 16 entries (`tls`, `dgram`, + // `diagnostics_channel`, `fs/promises`, `inspector`, `repl`, + // `stream/web`, `v8`, `vm`, `wasi`, …), so a computed + // `require(specifier)` for one of those fell through to compiled-module + // resolution and raised `MODULE_NOT_FOUND` instead of routing through + // `createRequire`. Each entry emits both the bare and `node:` spelling. + let builtin_predicate_cases = perry_hir::NODE_BUILTIN_MODULES + .iter() + .map(|name| format!("case '{name}': case 'node:{name}':")) + .collect::>() + .join("\n "); let cjs_preamble = format!( r#" // #3527: `module`/`exports` are reassignable `var`s (mirroring Node, where // they are wrapper-function parameters), so CJS bodies that do @@ -912,48 +940,7 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( }} function __perry_cjs_require_is_builtin(specifier) {{ switch (specifier) {{ - case 'assert': case 'node:assert': - case 'assert/strict': case 'node:assert/strict': - case 'async_hooks': case 'node:async_hooks': - case 'buffer': case 'node:buffer': - case 'child_process': case 'node:child_process': - case 'cluster': case 'node:cluster': - case 'console': case 'node:console': - case 'constants': case 'node:constants': - case 'crypto': case 'node:crypto': - case 'dns': case 'node:dns': - case 'dns/promises': case 'node:dns/promises': - case 'events': case 'node:events': - case 'fs': case 'node:fs': - case 'http': case 'node:http': - case 'http2': case 'node:http2': - case 'https': case 'node:https': - case 'module': case 'node:module': - case 'net': case 'node:net': - case 'os': case 'node:os': - case 'path': case 'node:path': - case 'path/posix': case 'node:path/posix': - case 'path/win32': case 'node:path/win32': - case 'perf_hooks': case 'node:perf_hooks': - case 'process': case 'node:process': - case 'punycode': case 'node:punycode': - case 'querystring': case 'node:querystring': - case 'readline': case 'node:readline': - case 'readline/promises': case 'node:readline/promises': - case 'stream': case 'node:stream': - case 'stream/promises': case 'node:stream/promises': - case 'string_decoder': case 'node:string_decoder': - case 'sys': case 'node:sys': - case 'test': case 'node:test': - case 'test/reporters': case 'node:test/reporters': - case 'timers': case 'node:timers': - case 'timers/promises': case 'node:timers/promises': - case 'tty': case 'node:tty': - case 'url': case 'node:url': - case 'util': case 'node:util': - case 'util/types': case 'node:util/types': - case 'worker_threads': case 'node:worker_threads': - case 'zlib': case 'node:zlib': + {builtin_predicate_cases} return true; default: return false; diff --git a/crates/perry/src/commands/compile/link/optional_framework_dir_tests.rs b/crates/perry/src/commands/compile/link/optional_framework_dir_tests.rs index ec4bfcdec9..67874618b5 100644 --- a/crates/perry/src/commands/compile/link/optional_framework_dir_tests.rs +++ b/crates/perry/src/commands/compile/link/optional_framework_dir_tests.rs @@ -1,4 +1,5 @@ use super::*; +use crate::test_env_lock::env_lock; /// Lay out a temp project: `/perry.toml` + `/src/main.ts`, /// with the perry.toml `[google_auth]` table set to `toml_body`. @@ -41,6 +42,11 @@ fn returns_none_when_no_framework_dir_key() { #[test] fn env_var_takes_precedence_over_perry_toml() { + // Serialize process-env mutation with the other env-touching tests in + // this binary (e.g. optimized_libs/tests). Edition 2021 does not make + // concurrent set_var/remove_var safe; the unique var name avoids value + // races but not the Unix environ-table race. + let _guard = env_lock(); let (_dir, entry) = scaffold("[google_auth]\nframework_dir = \"vendor/from-toml\"\n"); // Unique name so we don't race other tests sharing process env. let env_name = "PERRY_TEST_GA_FRAMEWORK_DIR_SET_C"; diff --git a/crates/perry/tests/cjs_wrap_builtin_require.rs b/crates/perry/tests/cjs_wrap_builtin_require.rs index 4b4342c541..3818e41cea 100644 --- a/crates/perry/tests/cjs_wrap_builtin_require.rs +++ b/crates/perry/tests/cjs_wrap_builtin_require.rs @@ -89,7 +89,10 @@ console.log("join:", typeof path.join, path.join("a", "b")); }; assert_eq!( stdout, - format!("platform: {expected_platform}\ncpus: function\njoin: function a/b\n") + format!( + "platform: {expected_platform}\ncpus: function\njoin: function a{sep}b\n", + sep = std::path::MAIN_SEPARATOR + ) ); } @@ -159,7 +162,10 @@ console.log("join:", join("a", "b")); }; assert_eq!( stdout, - format!("platform: {expected_platform}\narch: string\njoin: a/b\n") + format!( + "platform: {expected_platform}\narch: string\njoin: a{sep}b\n", + sep = std::path::MAIN_SEPARATOR + ) ); } @@ -230,6 +236,12 @@ var __toESM = (mod, isNodeMode, target) => (target = mod != null ? __create(__ge const ns = require("process"); const obj = Object.create(ns); const ctor = obj.constructor; +// Route __toESM through the synthetic class ref (ctor) so +// Object.getPrototypeOf(ctor) takes the class-id-tagged branch in +// js_object_get_prototype_of — the path the sentinel-suppression fix +// changed. Without this the heap-pointer path (for node_os below) hides a +// regression. +__toESM(ctor, 1); // Now run __toESM on another built-in — its Object.create(getPrototypeOf(mod)) // must not trip on the registered sentinel. let node_os = require("os"); @@ -239,3 +251,26 @@ console.log("os.cpus:", typeof node_os.cpus); ); assert_eq!(stdout, "os.cpus: function\n"); } + +/// Regression for the computed/dynamic `require` arm: a specifier held in a +/// variable (not a string literal) must route through the synthetic require's +/// `createRequire` arm for Node.js built-ins. Pre-fix the +/// `__perry_cjs_require_is_builtin` predicate was a hardcoded switch that +/// omitted 16 entries from the shared `NODE_BUILTIN_MODULES` table (`tls`, +/// `dgram`, `diagnostics_channel`, `domain`, `fs/promises`, `inspector`, +/// `repl`, `stream/web`, `v8`, `vm`, `wasi`, …), so `require(spec)` for one of +/// those fell through to compiled-module resolution and raised +/// `MODULE_NOT_FOUND`. `domain` is a stable, simple built-in that was missing. +#[test] +fn cjs_wrap_computed_builtin_require_resolves() { + let dir = tempfile::tempdir().expect("tempdir"); + let stdout = compile_and_run_cjs( + dir.path(), + r#" +const spec = "domain"; +const mod = require(spec); +console.log("typeof:", typeof mod); +"#, + ); + assert_eq!(stdout, "typeof: object\n"); +}