fix(cjs-wrap): complete builtin require coverage and fix named re-export bindings

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.<name>` instead of the dropped `import _req_N` binding, and match the full normalized specifier when classifying built-ins.

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,

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<String> = 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("<builtin>")` 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::<Vec<_>>()
             .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::<Vec<_>>();
@@ -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::<Vec<_>>()
+        .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;

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: `<root>/perry.toml` + `<root>/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";

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");
+}