diff --git a/changelog.d/10178-cjs-cycle-exports.md b/changelog.d/10178-cjs-cycle-exports.md new file mode 100644 index 0000000000..64c722cc17 --- /dev/null +++ b/changelog.d/10178-cjs-cycle-exports.md @@ -0,0 +1,12 @@ +Fix CommonJS cycle re-entry after `module.exports` is replaced, including +esbuild's `__export` / `__toCommonJS` getter exports and semver's exported +Comparator class. Partial publication now retains the module record, so the +existing require adapter reads its current exports instead of the initial +empty object. This adds no getter enumeration, copies, allocations, or extra +publication calls to ordinary CommonJS initialization. + +Generated circular-dependency warnings check whether the property actually +exists before warning, without invoking accessors. Regression coverage imports +two esbuild-style CommonJS modules from ESM, checks the module views during +cycle re-entry, calls through the cycle, and verifies live bindings and empty +stderr. Related: #10178, #10107. diff --git a/crates/perry-codegen/src/lower_call/native/native_runtime_branch.rs b/crates/perry-codegen/src/lower_call/native/native_runtime_branch.rs index 653483e59c..e4df635e5b 100644 --- a/crates/perry-codegen/src/lower_call/native/native_runtime_branch.rs +++ b/crates/perry-codegen/src/lower_call/native/native_runtime_branch.rs @@ -109,8 +109,8 @@ &[(DOUBLE, &from), (DOUBLE, &specifier)], )); } - // Next.js wall 54: publish a CJS module's partial exports before - // its body so same-thread recursive requires can observe them. + // Publish the CJS record before its body so same-thread recursive + // requires see its current exports, including replacements. "registerPathModulePartial" => { let path = args.first().map_or_else( || Ok(double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED))), diff --git a/crates/perry-runtime/src/module_require.rs b/crates/perry-runtime/src/module_require.rs index 68c646e5e9..f0be892bae 100644 --- a/crates/perry-runtime/src/module_require.rs +++ b/crates/perry-runtime/src/module_require.rs @@ -440,10 +440,9 @@ fn registered_path_module_value(path: &str) -> Option { .map(f64::from_bits) } -/// The registry holds whatever the CommonJS wrapper published: the module -/// RECORD once the wrapper reaches its tail, or bare partial exports while a -/// cycle is still initializing. Generated `require` sites want the exports in -/// both cases. +/// Generated wrappers publish the module RECORD at both the partial and final +/// boundaries. Read its current exports so replacements made before a cycle +/// re-entry are visible. Bare values remain supported for other publishers. fn path_module_exports(bits: u64) -> f64 { let value = f64::from_bits(bits); cjs_record_exports(value).unwrap_or(value) @@ -863,7 +862,7 @@ pub unsafe extern "C" fn js_register_path_init(path_ptr: *const u8, path_len: i6 } } -/// Codegen FFI: publish a CommonJS module's initial `exports` object before +/// Codegen FFI: publish a CommonJS module's record before /// executing its body. This is visible only to recursive loads by the owning /// thread; concurrent callers wait for [`js_register_path_module`] and the /// generated initializer to complete. diff --git a/crates/perry-runtime/src/module_require/path_registry.rs b/crates/perry-runtime/src/module_require/path_registry.rs index 490298bfd1..7e7fffcc84 100644 --- a/crates/perry-runtime/src/module_require/path_registry.rs +++ b/crates/perry-runtime/src/module_require/path_registry.rs @@ -183,7 +183,8 @@ impl PathModuleRegistry { true } - /// Publish the initial CommonJS `exports` object before the wrapper body. + /// Publish the CommonJS module record before the wrapper body. The exports + /// adapter unwraps its current `.exports` on each read, including cycles. /// Only same-thread recursive loads may observe it; unrelated waiters stay /// parked while the status is `Initializing`. pub(super) fn register_partial_exports(&self, key: String, exports: u64) -> bool { diff --git a/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs b/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs index 88f08698cb..cbacddaefa 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs @@ -204,6 +204,13 @@ fn path_module_wrap_publishes_partial_then_final_exports_and_tracks_undefined() .rfind("__perry_register_path_module(") .expect("CJS wrapper must publish its final module.exports value"); assert!(partial < body && body < final_publish, "{wrapped}"); + assert!( + wrapped.contains(&format!( + "__perry_register_path_module_partial({:?}, __cjs_module);", + path.to_string_lossy() + )), + "cycle readers must follow module.exports replacements\n{wrapped}" + ); // #8040: both the value lookup and the presence probe must consult the // SAME resolved specifier. A computed relative request is joined against // the module's directory before either call (`__perry_path_spec`), so a diff --git a/crates/perry/src/commands/compile/cjs_wrap/wrap.rs b/crates/perry/src/commands/compile/cjs_wrap/wrap.rs index 5ce09a30d5..40f74665a2 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/wrap.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/wrap.rs @@ -467,8 +467,13 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( cyclic_missing_property_names(source, source_path, spec, target) .into_iter() .map(|property| { + // #10178: this scan only nominates possible + // misses. __export helpers and class statics + // can already be present on a replacement + // module.exports. Check the returned value + // without invoking the exported getter. format!( - "if (childBefore && childBefore.loaded === false) globalThis.process?.emitWarning?.(\"Accessing non-existent property '{property}' of module exports inside circular dependency\"); " + "if (childBefore && childBefore.loaded === false && required != null && (typeof required === 'object' || typeof required === 'function') && !('{property}' in required)) globalThis.process?.emitWarning?.(\"Accessing non-existent property '{property}' of module exports inside circular dependency\"); " ) }) .collect::() @@ -476,7 +481,7 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( String::new() }; format!( - "const childBefore = require.cache[{path:?}]; {warnings}globalThis.__perry_cjs_pending_parent = module; let required; try {{ required = __perry_require_path_module({path:?}); }} finally {{ globalThis.__perry_cjs_pending_parent = undefined; }} {link_child}return required;", + "const childBefore = require.cache[{path:?}]; globalThis.__perry_cjs_pending_parent = module; let required; try {{ required = __perry_require_path_module({path:?}); }} finally {{ globalThis.__perry_cjs_pending_parent = undefined; }} {warnings}{link_child}return required;", path = target.to_string_lossy(), ) }) @@ -979,10 +984,12 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( // Node populates `module.parent` before the body evaluates, so link it // here rather than at the tail's registry publication. __perry_link_path_module_parent(__cjs_module); - // Publish the initial exports before user code. The runtime exposes them - // only to same-thread recursive loads; concurrent first callers wait for - // the final record registration at the bottom of this wrapper. - __perry_register_path_module_partial({module_path_literal}, __cjs_module.exports); + // Publish the record before user code, just as at final publication. + // #10178: esbuild replaces module.exports before requiring its peer. + // Holding the initial empty object loses that replacement at re-entry; + // the runtime's existing record unwrap reads the current exports instead. + // This needs no getter enumeration, copying, or extra publication calls. + __perry_register_path_module_partial({module_path_literal}, __cjs_module); var module = __cjs_module; var exports = __cjs_module.exports; const __perry_cjs_base_require = __perry_cjs_create_require({module_filename_literal}); diff --git a/crates/perry/tests/source_graph_export_regressions.rs b/crates/perry/tests/source_graph_export_regressions.rs index 4797a6c6a9..58df217fed 100644 --- a/crates/perry/tests/source_graph_export_regressions.rs +++ b/crates/perry/tests/source_graph_export_regressions.rs @@ -9,6 +9,9 @@ use std::sync::Once; #[path = "source_graph_export_regressions/issue_10153.rs"] mod issue_10153; +#[path = "source_graph_export_regressions/issue_10178.rs"] +mod issue_10178; + const GC_ENV_OVERRIDES: &[&str] = &[ "PERRY_GEN_GC", "PERRY_GC_SCAVENGE", @@ -90,6 +93,10 @@ fn write(dir: &Path, name: &str, source: &str) { } fn compile_and_run(dir: &Path, entry: &str) -> String { + String::from_utf8_lossy(&compile_and_run_output(dir, entry).stdout).into_owned() +} + +fn compile_and_run_output(dir: &Path, entry: &str) -> std::process::Output { let output = dir.join("main_bin"); let compile = Command::new(perry_bin()) .current_dir(dir) @@ -119,7 +126,7 @@ fn compile_and_run(dir: &Path, entry: &str) -> String { String::from_utf8_lossy(&run.stdout), String::from_utf8_lossy(&run.stderr) ); - String::from_utf8_lossy(&run.stdout).into_owned() + run } fn compile_and_run_with_llvm_trace(dir: &Path, entry: &str) -> (String, String) { diff --git a/crates/perry/tests/source_graph_export_regressions/issue_10178.rs b/crates/perry/tests/source_graph_export_regressions/issue_10178.rs new file mode 100644 index 0000000000..d7e28ab528 --- /dev/null +++ b/crates/perry/tests/source_graph_export_regressions/issue_10178.rs @@ -0,0 +1,97 @@ +//! A CJS cycle must observe module.exports replacements before init finishes. + +use super::{compile_and_run_output, write}; + +#[test] +fn esbuild_getters_are_callable_and_live_through_a_require_cycle() { + let dir = tempfile::tempdir().unwrap(); + write( + dir.path(), + "token.cjs", + include_str!("../../../../test-files/cjs_esbuild_cycle/token.cjs"), + ); + write( + dir.path(), + "consumer.cjs", + include_str!("../../../../test-files/cjs_esbuild_cycle/consumer.cjs"), + ); + write( + dir.path(), + "main.mjs", + &include_str!("../../../../test-files/test_cjs_esbuild_cycle.ts") + .replace("./cjs_esbuild_cycle/token.cjs", "./token.cjs"), + ); + let run = compile_and_run_output(dir.path(), "main.mjs"); + assert_eq!( + String::from_utf8_lossy(&run.stdout), + "function:function:true\n0\n42\n52\n2\n" + ); + assert!( + run.stderr.is_empty(), + "cycle emitted a warning: {}", + String::from_utf8_lossy(&run.stderr) + ); +} + +#[test] +fn class_static_getter_is_visible_in_a_comparator_first_cycle() { + let dir = tempfile::tempdir().unwrap(); + write( + dir.path(), + "comparator.cjs", + "const ANY = Symbol('ANY');\n\ + class Comparator { static get ANY() { return ANY; } }\n\ + module.exports = Comparator;\n\ + const Range = require('./range.cjs');\n\ + Comparator.read = function () { return Range.read(); };\n", + ); + write( + dir.path(), + "range.cjs", + "class Range { static read() { return Comparator.ANY; } }\n\ + module.exports = Range;\n\ + const Comparator = require('./comparator.cjs');\n", + ); + write( + dir.path(), + "main.mjs", + "import Comparator from './comparator.cjs';\n\ + console.log(typeof Comparator.read());\n\ + console.log(Comparator.read() === Comparator.ANY);\n", + ); + let run = compile_and_run_output(dir.path(), "main.mjs"); + assert_eq!(String::from_utf8_lossy(&run.stdout), "symbol\ntrue\n"); + assert!( + run.stderr.is_empty(), + "{}", + String::from_utf8_lossy(&run.stderr) + ); +} + +#[test] +fn missing_property_in_a_cycle_still_warns() { + let dir = tempfile::tempdir().unwrap(); + write( + dir.path(), + "a.cjs", + "exports.before = true;\n\ + const b = require('./b.cjs');\n\ + exports.seen = b.seen;\n\ + exports.after = true;\n", + ); + write( + dir.path(), + "b.cjs", + "const a = require('./a.cjs');\nexports.seen = a.after;\n", + ); + write( + dir.path(), + "main.mjs", + "import a from './a.cjs';\nconsole.log(a.seen);\n", + ); + let run = compile_and_run_output(dir.path(), "main.mjs"); + assert_eq!(String::from_utf8_lossy(&run.stdout), "undefined\n"); + assert!(String::from_utf8_lossy(&run.stderr).contains( + "Accessing non-existent property 'after' of module exports inside circular dependency" + )); +} diff --git a/test-files/cjs_esbuild_cycle/consumer.cjs b/test-files/cjs_esbuild_cycle/consumer.cjs new file mode 100644 index 0000000000..14e3042435 --- /dev/null +++ b/test-files/cjs_esbuild_cycle/consumer.cjs @@ -0,0 +1,24 @@ +var __defProp = Object.defineProperty; +var __getOwnPropNames = Object.getOwnPropertyNames; +var __export = (target, all) => { + for (var name in all) + __defProp(target, name, { get: all[name], enumerable: true }); +}; +var __copyProps = (to, from) => { + for (let key of __getOwnPropNames(from)) + __defProp(to, key, { get: () => from[key], enumerable: true }); + return to; +}; +var __toCommonJS = (mod) => __copyProps(__defProp({}, "__esModule", { value: true }), mod); +var consumer_exports = {}; +__export(consumer_exports, { read: () => read, cycleView: () => cycleView }); +module.exports = __toCommonJS(consumer_exports); +var token = require("./token.cjs"); +// Inspect both views while token's wrapper is still running. Neither +// descriptor inspection nor retaining the namespace should invoke a getter. +var record = globalThis.__esbuildCycleRecord; +var cachedGetter = record ? typeof Object.getOwnPropertyDescriptor(record.exports, "getValue")?.get : "missing-record"; +var requiredGetter = token ? typeof Object.getOwnPropertyDescriptor(token, "getValue")?.get : "missing-exports"; +var sameExports = record ? token === record.exports : false; +function read() { return (0, token.getValue)(); } +function cycleView() { return cachedGetter + ":" + requiredGetter + ":" + sameExports; } diff --git a/test-files/cjs_esbuild_cycle/token.cjs b/test-files/cjs_esbuild_cycle/token.cjs new file mode 100644 index 0000000000..7db8786ca8 --- /dev/null +++ b/test-files/cjs_esbuild_cycle/token.cjs @@ -0,0 +1,30 @@ +var __defProp = Object.defineProperty; +var __getOwnPropNames = Object.getOwnPropertyNames; +var __export = (target, all) => { + for (var name in all) + __defProp(target, name, { get: all[name], enumerable: true }); +}; +var __copyProps = (to, from) => { + for (let key of __getOwnPropNames(from)) + __defProp(to, key, { get: () => from[key], enumerable: true }); + return to; +}; +var __toCommonJS = (mod) => __copyProps(__defProp({}, "__esModule", { value: true }), mod); +var token_exports = {}; +var getterReads = 0; +__export(token_exports, { + getValue: () => (getterReads++, getValue), + callThroughCycle: () => callThroughCycle, + update: () => update, + cycleView: () => cycleView, + reads: () => reads +}); +module.exports = __toCommonJS(token_exports); +globalThis.__esbuildCycleRecord = module; +var consumer = require("./consumer.cjs"); +var offset = 40; +function getValue() { return offset + 2; } +function callThroughCycle() { return (0, consumer.read)(); } +function update() { getValue = function () { return 52; }; } +function cycleView() { return consumer.cycleView(); } +function reads() { return getterReads; } diff --git a/test-files/test_cjs_esbuild_cycle.ts b/test-files/test_cjs_esbuild_cycle.ts new file mode 100644 index 0000000000..3373f7faa4 --- /dev/null +++ b/test-files/test_cjs_esbuild_cycle.ts @@ -0,0 +1,12 @@ +// #10178: an ESM import enters an esbuild CommonJS require cycle. +import process from "node:process"; +import token from "./cjs_esbuild_cycle/token.cjs"; + +if (typeof process.emitWarning !== "function") throw new Error("missing warning support"); +console.log(token.cycleView()); +console.log(token.reads()); +delete globalThis.__esbuildCycleRecord; +console.log(token.callThroughCycle()); +token.update(); +console.log(token.callThroughCycle()); +console.log(token.reads());