From 62ba76d454947dceb21efb4bdaa6d707616af70e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 19 Sep 2026 22:55:15 +0200 Subject: [PATCH 1/2] fix(cjs): evaluate a conditional require whatever its target exports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A top-level CommonJS `require()` in a conditional position never ran its target when that target carried no CommonJS export marker. The branch was taken, the shim was reached, and the module body never executed — silently, with no crash and no diagnostic. #10754 reports this as a call-site-shape problem (`if` works, `&&` / `?:` / `try` / `switch` / loop-body do not). That is a confound in the reproducer: its `if` case required a module with `module.exports = 1` while the other five required side-effect-only modules. Crossing the two axes against Node 26.5.1 on a release build of main @ 91a566c8af shows the discriminator is the TARGET's export shape — all six shapes fail with a side-effect-only target, all six pass with a value-returning one. #10674 defers a conditional require correctly: the target stays `ModuleInitKind::Deferred` and the shim returns the `_lazyreq_N` import binding, with codegen firing `__init()` at the binding read. That init call is gated on the binding being a known imported FUNCTION (`ctx.import_function_prefixes`), which a target with no default export never is — so nothing fires. - `cjs_wrap/deferred_requires.rs`: an AST visitor replaces the text-scanning deferral classifier, which missed `if (cond) x = require('S')` (still eagerly hoisted on main, a residual #10437 shape), concise arrows, the ternary ALTERNATE arm, both halves of a `do`/`while`, and `&&=`/`||=`/`??=`. `extract_requires::function_local_specs` stays as the parse-failure fallback. - `cjs_wrap/wrap.rs`: a deferred specifier resolves through the path registry rather than its import binding, so initialization no longer depends on the target's export shape; the registry record is memoized per call site once `loaded === true`, because re-entering the registry on every call measured 3.4x on a hot require. - `cjs_wrap/wrap.rs`: the registry only holds EXPORTS for a target that publishes them, which is every CJS-wrapped module and no other. A file with no CommonJS marker is not CJS-wrapped, so it registers an initializer and never any exports, and the registry returned `undefined` where Node returns `{}`. The arm falls back to the import binding on a genuine registry miss, discriminated by `__perry_has_path_module` — the same guard the generic runtime-`require(path)` arm in the same wrapper already uses. - `perry-codegen/src/expr/dyn_extern_i18n.rs`: fire the deferred `__init()` before the imported-class and namespace fast paths, which can themselves depend on module initialization. The implementation is PR #10285's, rebased onto current main; the registry-miss fallback and the gap fixture are new here. `test_gap_10754_cjs_conditional_require_shapes.cts` crosses all six shapes with three cells each — taken/side-effect-only, taken/value-returning and not-taken — because a fix that loads the module unconditionally is #10437 again, not a fix. On unfixed main it differs from the Node oracle on 12 lines (six side-effect-only targets never load; three value-returning targets load before the program's first statement instead of at their call site) and the harness reports parity_fail; with this change it matches Node byte-for-byte and the harness reports PASS. Closes #10754 --- .../perry-codegen/src/expr/dyn_extern_i18n.rs | 23 +- .../compile/cjs_wrap/deferred_requires.rs | 312 ++++++++++++++ .../src/commands/compile/cjs_wrap/mod.rs | 6 +- .../src/commands/compile/cjs_wrap/tests.rs | 5 +- .../src/commands/compile/cjs_wrap/wrap.rs | 95 ++++- .../perry/tests/conditional_require_init.rs | 381 ++++++++++++++++++ test-files/_helpers/gap10754_off_and.cjs | 1 + test-files/_helpers/gap10754_off_for.cjs | 1 + test-files/_helpers/gap10754_off_if.cjs | 1 + test-files/_helpers/gap10754_off_switch.cjs | 1 + test-files/_helpers/gap10754_off_tern.cjs | 1 + test-files/_helpers/gap10754_off_try.cjs | 1 + test-files/_helpers/gap10754_on_exp_and.cjs | 2 + test-files/_helpers/gap10754_on_exp_for.cjs | 2 + test-files/_helpers/gap10754_on_exp_if.cjs | 2 + .../_helpers/gap10754_on_exp_switch.cjs | 2 + test-files/_helpers/gap10754_on_exp_tern.cjs | 2 + test-files/_helpers/gap10754_on_exp_try.cjs | 2 + test-files/_helpers/gap10754_on_sfx_and.cjs | 1 + test-files/_helpers/gap10754_on_sfx_for.cjs | 1 + test-files/_helpers/gap10754_on_sfx_if.cjs | 1 + .../_helpers/gap10754_on_sfx_switch.cjs | 1 + test-files/_helpers/gap10754_on_sfx_tern.cjs | 1 + test-files/_helpers/gap10754_on_sfx_try.cjs | 1 + ...p_10754_cjs_conditional_require_shapes.cts | 135 +++++++ 25 files changed, 947 insertions(+), 34 deletions(-) create mode 100644 crates/perry/src/commands/compile/cjs_wrap/deferred_requires.rs create mode 100644 crates/perry/tests/conditional_require_init.rs create mode 100644 test-files/_helpers/gap10754_off_and.cjs create mode 100644 test-files/_helpers/gap10754_off_for.cjs create mode 100644 test-files/_helpers/gap10754_off_if.cjs create mode 100644 test-files/_helpers/gap10754_off_switch.cjs create mode 100644 test-files/_helpers/gap10754_off_tern.cjs create mode 100644 test-files/_helpers/gap10754_off_try.cjs create mode 100644 test-files/_helpers/gap10754_on_exp_and.cjs create mode 100644 test-files/_helpers/gap10754_on_exp_for.cjs create mode 100644 test-files/_helpers/gap10754_on_exp_if.cjs create mode 100644 test-files/_helpers/gap10754_on_exp_switch.cjs create mode 100644 test-files/_helpers/gap10754_on_exp_tern.cjs create mode 100644 test-files/_helpers/gap10754_on_exp_try.cjs create mode 100644 test-files/_helpers/gap10754_on_sfx_and.cjs create mode 100644 test-files/_helpers/gap10754_on_sfx_for.cjs create mode 100644 test-files/_helpers/gap10754_on_sfx_if.cjs create mode 100644 test-files/_helpers/gap10754_on_sfx_switch.cjs create mode 100644 test-files/_helpers/gap10754_on_sfx_tern.cjs create mode 100644 test-files/_helpers/gap10754_on_sfx_try.cjs create mode 100644 test-files/test_gap_10754_cjs_conditional_require_shapes.cts diff --git a/crates/perry-codegen/src/expr/dyn_extern_i18n.rs b/crates/perry-codegen/src/expr/dyn_extern_i18n.rs index fbdef0d15c..64a278cc65 100644 --- a/crates/perry-codegen/src/expr/dyn_extern_i18n.rs +++ b/crates/perry-codegen/src/expr/dyn_extern_i18n.rs @@ -832,6 +832,17 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // checks work; calling those values via stored references would // need a separate runtime path that this commit doesn't add. Expr::ExternFuncRef { name, .. } => { + // A synthetic deferred require evaluates its dependency at the + // original call site. Do this before the class/namespace fast + // paths too: those values can depend on module initialization. + if name.starts_with("_lazyreq_") { + if let Some(source_prefix) = ctx.import_function_prefixes.get(name) { + let init_fn = format!("{}__init", source_prefix); + ctx.pending_declares + .push((init_fn.clone(), crate::types::VOID, vec![])); + ctx.block().call_void(&init_fn, &[]); + } + } // Imported class references (refs #420 / drizzle): when `name` // resolves to a class registered in `ctx.class_ids` (populated // from `opts.imported_classes` for imported classes too), emit @@ -884,18 +895,6 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } } if let Some(source_prefix) = ctx.import_function_prefixes.get(name).cloned() { - // Next.js lazy-require: a `_lazyreq_N` binding is the CJS require - // shim's handle to a FUNCTION-LOCAL `require('S')`. S is - // `Deferred` (never eager-initialized), so before reading its - // default-export getter, fire `__init()` — idempotent, so - // re-reads cost a guard check. This is the moment Node would run - // S's module body: when `require('S')` is actually called. - if name.starts_with("_lazyreq_") { - let init_fn = format!("{}__init", source_prefix); - ctx.pending_declares - .push((init_fn.clone(), crate::types::VOID, vec![])); - ctx.block().call_void(&init_fn, &[]); - } // Issue #678 followup: a V8-fallback import used as a value // (rather than called directly) has no native singleton // wrapper to point at — the `__perry_wrap_extern_*` for V8 diff --git a/crates/perry/src/commands/compile/cjs_wrap/deferred_requires.rs b/crates/perry/src/commands/compile/cjs_wrap/deferred_requires.rs new file mode 100644 index 0000000000..b349528ba0 --- /dev/null +++ b/crates/perry/src/commands/compile/cjs_wrap/deferred_requires.rs @@ -0,0 +1,312 @@ +//! Preserve the evaluation boundary of conditional and function-local requires. + +use std::collections::{HashMap, HashSet}; + +use swc_ecma_ast as ast; +use swc_ecma_visit::{Visit, VisitWith}; + +/// Synthetic imports collect the target, but must not evaluate it before a +/// conditional branch or a function actually calls `require`. A specifier with +/// any unconditional occurrence keeps the existing eager/alias-adoption path. +/// Use the AST: brace scanning misses concise arrows, unbraced branches, and +/// short-circuit expressions. On a parse failure retain the existing scanner's +/// function-local classification (some CJS sources need wrapping to parse). +pub(super) fn deferred_require_specs(source: &str) -> HashSet { + let Ok(module) = perry_parser::parse_typescript(source, "requires.cjs") else { + return super::extract_requires::function_local_specs(source); + }; + let mut visitor = Requires::default(); + module.visit_with(&mut visitor); + visitor + .sites + .into_iter() + .filter_map(|(specifier, deferred)| deferred.then_some(specifier)) + .collect() +} + +#[derive(Default)] +struct Requires { + deferred: bool, + sites: HashMap, +} + +impl Requires { + fn defer(&mut self, visit: impl FnOnce(&mut Self)) { + let previous = self.deferred; + self.deferred = true; + visit(self); + self.deferred = previous; + } +} + +impl Visit for Requires { + fn visit_call_expr(&mut self, call: &ast::CallExpr) { + if let ast::Callee::Expr(callee) = &call.callee { + if matches!(callee.as_ref(), ast::Expr::Ident(name) if name.sym == *"require") { + if let [arg] = call.args.as_slice() { + if arg.spread.is_none() { + if let ast::Expr::Lit(ast::Lit::Str(specifier)) = arg.expr.as_ref() { + self.sites + .entry(specifier.value.to_string_lossy().into_owned()) + .and_modify(|deferred| *deferred &= self.deferred) + .or_insert(self.deferred); + } + } + } + } + } + call.visit_children_with(self); + } + + fn visit_function(&mut self, function: &ast::Function) { + function.decorators.visit_with(self); + self.defer(|visitor| { + function.params.visit_with(visitor); + function.body.visit_with(visitor); + }); + } + + fn visit_arrow_expr(&mut self, arrow: &ast::ArrowExpr) { + self.defer(|visitor| arrow.visit_children_with(visitor)); + } + + fn visit_constructor(&mut self, constructor: &ast::Constructor) { + self.defer(|visitor| constructor.visit_children_with(visitor)); + } + + fn visit_getter_prop(&mut self, getter: &ast::GetterProp) { + getter.key.visit_with(self); + self.defer(|visitor| getter.body.visit_with(visitor)); + } + + fn visit_setter_prop(&mut self, setter: &ast::SetterProp) { + setter.key.visit_with(self); + self.defer(|visitor| setter.body.visit_with(visitor)); + } + + fn visit_if_stmt(&mut self, stmt: &ast::IfStmt) { + stmt.test.visit_with(self); + self.defer(|visitor| { + stmt.cons.visit_with(visitor); + stmt.alt.visit_with(visitor); + }); + } + + fn visit_cond_expr(&mut self, expr: &ast::CondExpr) { + expr.test.visit_with(self); + self.defer(|visitor| { + expr.cons.visit_with(visitor); + expr.alt.visit_with(visitor); + }); + } + + fn visit_bin_expr(&mut self, expr: &ast::BinExpr) { + expr.left.visit_with(self); + if matches!( + expr.op, + ast::BinaryOp::LogicalAnd | ast::BinaryOp::LogicalOr | ast::BinaryOp::NullishCoalescing + ) { + self.defer(|visitor| expr.right.visit_with(visitor)); + } else { + expr.right.visit_with(self); + } + } + + fn visit_try_stmt(&mut self, stmt: &ast::TryStmt) { + // In particular, a throwing require must stay inside its try/catch. + self.defer(|visitor| stmt.visit_children_with(visitor)); + } + + fn visit_assign_expr(&mut self, expr: &ast::AssignExpr) { + expr.left.visit_with(self); + if matches!( + expr.op, + ast::AssignOp::AndAssign | ast::AssignOp::OrAssign | ast::AssignOp::NullishAssign + ) { + self.defer(|visitor| expr.right.visit_with(visitor)); + } else { + expr.right.visit_with(self); + } + } + + fn visit_switch_stmt(&mut self, stmt: &ast::SwitchStmt) { + stmt.discriminant.visit_with(self); + self.defer(|visitor| stmt.cases.visit_with(visitor)); + } + + fn visit_while_stmt(&mut self, stmt: &ast::WhileStmt) { + stmt.test.visit_with(self); + self.defer(|visitor| stmt.body.visit_with(visitor)); + } + + fn visit_do_while_stmt(&mut self, stmt: &ast::DoWhileStmt) { + // Both halves are conditional: the body can `break` or `return` before + // the test runs, so `do { break } while (require("dep"))` never + // evaluates the require in Node. + self.defer(|visitor| { + stmt.body.visit_with(visitor); + stmt.test.visit_with(visitor); + }); + } + + fn visit_for_stmt(&mut self, stmt: &ast::ForStmt) { + stmt.init.visit_with(self); + stmt.test.visit_with(self); + self.defer(|visitor| { + stmt.update.visit_with(visitor); + stmt.body.visit_with(visitor); + }); + } + + fn visit_for_in_stmt(&mut self, stmt: &ast::ForInStmt) { + stmt.right.visit_with(self); + self.defer(|visitor| { + stmt.left.visit_with(visitor); + stmt.body.visit_with(visitor); + }); + } + + fn visit_for_of_stmt(&mut self, stmt: &ast::ForOfStmt) { + stmt.right.visit_with(self); + self.defer(|visitor| { + stmt.left.visit_with(visitor); + stmt.body.visit_with(visitor); + }); + } +} + +#[cfg(test)] +mod tests { + use super::deferred_require_specs; + + #[test] + fn preserves_conditional_and_function_evaluation_boundaries() { + for source in [ + "if (enabled) require('dep');", + "if (enabled) { const dep = require('dep'); }", + "enabled ? require('dep') : 0;", + "enabled && require('dep');", + "enabled || require('dep');", + "enabled ?? require('dep');", + "value &&= require('dep');", + "value ||= require('dep');", + "value ??= require('dep');", + "try { require('dep'); } catch (e) {}", + "switch (value) { case 1: require('dep'); }", + "while (enabled) require('dep');", + "do { break; } while (require('dep'));", + "do { require('dep'); } while (enabled);", + "for (; enabled;) require('dep');", + "for (const item of items) require('dep');", + "for (const key in object) require('dep');", + "module.exports = () => require('dep');", + "function load(dep = require('dep',)) { return dep; }", + "module.exports = {get value() { return require('dep'); }};", + ] { + assert!(deferred_require_specs(source).contains("dep"), "{source}"); + } + } + + #[test] + fn unconditional_occurrences_keep_existing_eager_classification() { + for source in [ + "const dep = require('dep');", + "if (require('dep')) {}", + "require('dep') && enabled;", + "const x = require('dep') + 1;", + "value = require('dep');", + "{ require('dep'); }", + "if (enabled) require('dep'); require('dep');", + "require('dep'); module.exports = () => require('dep');", + ] { + assert!(!deferred_require_specs(source).contains("dep"), "{source}"); + } + } + + #[test] + fn ignores_comments_strings_and_member_calls() { + assert!(deferred_require_specs( + "// require('dep')\nconst text = \"require('dep')\";\nif (enabled) other.require('dep');" + ).is_empty()); + } + + #[test] + fn wrapping_keeps_conditional_aliases_and_exports_inside_the_body() { + let source = "class Unrelated {}\nif (enabled) {\nconst dep = require('dep');\nexports.value = require('dep');\nconsole.log(dep);\n}\n"; + let wrapped = + super::super::wrap::wrap_commonjs(source, std::path::Path::new("/fixture/index.cjs")); + assert!( + wrapped.contains("import _lazyreq_0 from 'dep';"), + "{wrapped}" + ); + assert!(wrapped.contains("const dep = require('dep');"), "{wrapped}"); + assert!(!wrapped.contains("const dep = _lazyreq_0;"), "{wrapped}"); + assert!( + wrapped.contains("export const value = _cjs.value;"), + "{wrapped}" + ); + assert!( + !wrapped.contains("export { _lazyreq_0 as value };"), + "{wrapped}" + ); + perry_parser::parse_typescript(&wrapped, "wrapped.cjs").unwrap(); + } + + /// A deferred require must not re-enter the path registry on every call. + /// + /// Deferring a conditional require routes it through + /// `__perry_require_path_module`, which is a registry lookup plus a + /// `globalThis` write pair inside a `try`/`finally`. That is needed only until + /// the target is loaded; without the memo it ran on EVERY call and cost 3.4x on + /// a hot require (1.43 B -> 4.86 B instructions over 300k calls). + /// + /// Nothing else pins the memo — delete it and every other test still passes, + /// the only symptom being that hot requires get slow again. So assert the + /// emitted shape: the cache slot is declared, the case is fronted by the + /// short-circuit, and the record is only cached once `loaded === true`. + #[test] + fn a_deferred_require_case_is_fronted_by_its_memo() { + let src = r#" +function get(flag) { + if (flag) { return require("./dep.js").v; } + return 0; +} +module.exports = { get }; +"#; + // The target must RESOLVE: the memo lives in the runtime-record arm, which + // is only emitted for a specifier that resolves to a real file. + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("dep.js"), "module.exports = { v: 1 };\n").unwrap(); + let wrapped = super::super::wrap::wrap_commonjs(src, &dir.path().join("entry.js")); + + for (needle, why) in [ + ("__rec;", "the per-site memo slot declaration"), + ( + "__rec !== undefined) return ", + "the short-circuit that skips the registry", + ), + ( + ".loaded === true", + "the guard that refuses to cache a half-loaded target", + ), + ] { + assert!( + wrapped.contains(needle), + "the deferred-require memo no longer emits `{needle}` ({why}).\n\ + Without it every call to a deferred require re-enters \ + `__perry_require_path_module`, which measured 3.4x slower on a hot \ + require. If the memo moved, update this test; if it was removed on \ + purpose, delete this test and say why in the PR.\n{wrapped}" + ); + } + + // The record, not the exports: a module that replaces `module.exports` + // after evaluation must still read through, as it does in Node. + assert!( + wrapped.contains("__rec.exports"), + "the memo must return `record.exports`, not a cached exports value, or a \ + post-evaluation `module.exports = X` would be invisible to later \ + requires.\n{wrapped}" + ); + } +} diff --git a/crates/perry/src/commands/compile/cjs_wrap/mod.rs b/crates/perry/src/commands/compile/cjs_wrap/mod.rs index 83079a1345..115b8b5898 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/mod.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/mod.rs @@ -37,6 +37,7 @@ //! follows up to a small depth (2 levels) to handle one level of env //! switching; deeper indirection is rare and gets the no-op fallback. +mod deferred_requires; pub(crate) mod detect; mod extract_exports; mod extract_requires; @@ -53,6 +54,7 @@ mod parcel_watcher_tests; mod preamble_canary_tests; // Cross-sibling helpers — siblings reach for these via `use super::*;`. +use deferred_requires::deferred_require_specs; use detect::is_js_reserved_word; use extract_exports::{ extract_exports_from_source, extract_named_exports_from_require, @@ -62,8 +64,8 @@ use extract_exports::{ // #8547: the stdlib-link decision needs the literal `require()` specifiers. pub(crate) use extract_requires::extract_require_specifiers; use extract_requires::{ - extract_export_star_specs, extract_require_aliases_with_ranges, function_local_specs, - identifier_is_declared_binding, identifier_is_reassigned, + extract_export_star_specs, extract_require_aliases_with_ranges, identifier_is_declared_binding, + identifier_is_reassigned, }; use hoist_classes::{ extract_top_level_class_decls, rewrite_module_exports_class_expression, diff --git a/crates/perry/src/commands/compile/cjs_wrap/tests.rs b/crates/perry/src/commands/compile/cjs_wrap/tests.rs index 22ab9db45a..8cf8a8f98f 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/tests.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/tests.rs @@ -751,9 +751,8 @@ exports.spawn = function spawn() { return terminalCtor; }; false, ); assert!( - wrapped.contains("import _req_0 from './windowsTerminal';") - || wrapped.contains("import terminalCtor from './windowsTerminal';"), - "expected live Windows require to stay hoisted, got:\n{}", + wrapped.contains("import _lazyreq_0 from './windowsTerminal';"), + "expected live Windows require to remain collected and initialize in its branch, got:\n{}", wrapped ); assert!( diff --git a/crates/perry/src/commands/compile/cjs_wrap/wrap.rs b/crates/perry/src/commands/compile/cjs_wrap/wrap.rs index da26e86a93..6b8060127a 100644 --- a/crates/perry/src/commands/compile/cjs_wrap/wrap.rs +++ b/crates/perry/src/commands/compile/cjs_wrap/wrap.rs @@ -309,15 +309,21 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( } true }; - // Next.js lazy-require: specifiers whose every `require('S')` call site is - // inside a function body (lazy in Node). Computed up front because it also - // suppresses alias ADOPTION below — a function-local `const dep = - // require('S')` is a function-scoped const, not a module binding, and - // adopting it would hoist `import dep from 'S'` to module scope (eager). We - // instead keep the synthetic binding and rename it `_lazyreq_N` so the - // target stays `Deferred` and inits only when the shim's - // `return _lazyreq_N` runs (i.e. when the function actually calls require). - let mut lazy_specs = function_local_specs(source); + // Specifiers whose every `require('S')` call site is conditional or inside + // a function body. Computed up front because it also suppresses alias + // ADOPTION below — a function-local `const dep = require('S')` is a + // function-scoped const, not a module binding, and adopting it would hoist + // `import dep from 'S'` to module scope (eager). We instead keep the + // synthetic binding and rename it `_lazyreq_N` so the target stays + // `Deferred`, and the shim initializes it through the path registry at the + // moment control flow reaches the call. + // + // #10754: the registry, not the binding, is what makes this work for EVERY + // target. Reading `_lazyreq_N` fires `__init()` only when codegen knows + // the binding as an imported function (`import_function_prefixes`), which a + // target with no default export never is — so a side-effect-only dependency + // was deferred and then never evaluated at all. + let mut lazy_specs = deferred_require_specs(source); let cyclic_specs = cyclic_require_specs(source, source_path); let parent_sensitive_specs = parent_sensitive_require_specs(source, source_path); lazy_specs.extend(cyclic_specs.iter().cloned()); @@ -464,8 +470,11 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( ) }) .unwrap_or_default(); - let needs_runtime_record = - cyclic_specs.contains(spec) || parent_sensitive_specs.contains(spec); + // Deferred targets must initialize even when they have no default + // export getter (for example, a side-effect-only module). The path + // registry owns initialization and cached exports independently of + // the target's export shape, and preserves thrown exceptions here. + let needs_runtime_record = lazy_specs.contains(spec); let runtime_require = if needs_runtime_record { resolved_target .as_ref() @@ -487,9 +496,24 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( } else { String::new() }; + // #10754: the registry initializes the target whatever + // its export shape, but it only holds EXPORTS for a + // target that publishes them — which is every + // CJS-wrapped module and no other. A target with no + // CommonJS marker at all (`console.log('x')` and + // nothing else, the commonest polyfill/registration + // shape) is not CJS-wrapped, so it registers an + // initializer and never any exports: the registry runs + // its body and hands back `undefined` where Node hands + // back `{}`. `__perry_has_path_module` is the + // miss-vs-`undefined`-export discriminator (a real + // module may export `undefined`), and on a genuine miss + // the import binding is the value this arm returned + // before the target was deferred at all. format!( - "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;", + "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; }} if (required === undefined && !__perry_has_path_module({path:?})) required = {local}; {warnings}{link_child}const __perry_rec = require.cache[{path:?}]; if (__perry_rec !== undefined && __perry_rec.loaded === true) {local}__rec = __perry_rec; return required;", path = target.to_string_lossy(), + local = local, ) }) } else { @@ -517,15 +541,29 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( // `typeof {local} === 'boolean'` sentinel guard does not apply // (builtins are never the pruned-build TRUE sentinel). format!(" if (specifier === '{spec}') {{ {required_value} }}") - } else if require_site_in_try(source, spec) { + } else if require_site_in_try(source, spec) && runtime_require.is_none() { format!( " if (specifier === '{spec}') {{ if (typeof {local} === 'boolean') \ throw __perry_cjs_require_error('error', 'MODULE_NOT_FOUND', \ \"Cannot find module '{spec}'\"); {required_value} }}" ) } else { - if needs_runtime_record { - format!(" if (specifier === '{spec}') {{ {required_value} }}") + if needs_runtime_record && runtime_require.is_some() { + // A repeat require must not re-enter the path registry. + // The registry call exists so a DEFERRED target initializes + // even with no default-export getter, but it is only needed + // until the target is loaded; after that it was costing a + // registry lookup, a `globalThis` write pair and a + // try/finally on EVERY call — 3.4x on a hot require. + // + // The RECORD is cached rather than the exports, and only + // once `loaded === true`, so a module that replaces + // `module.exports` after evaluation still reads through + // (matching Node), and a cyclic target mid-initialisation + // keeps going through the registry until it completes. + format!( + " if (specifier === '{spec}') {{ if ({local}__rec !== undefined) return {local}__rec.exports; {required_value} }}" + ) } else if link_child.is_empty() { format!(" if (specifier === '{spec}') return {local};") } else { @@ -537,6 +575,23 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( }) .collect::>() .join("\n"); + // One memo slot per deferred specifier, declared in the factory so each + // module INSTANCE gets its own (they are per-module state, not global). + // A plain local is deliberate: an object keyed by specifier would put a + // property read on the hot require path, which is what this is removing. + let lazy_cache_decls = require_specs + .iter() + .zip(import_local_names.iter()) + .filter(|(spec, _)| { + // Only the specs that get the runtime-record arm ever assign a + // slot; an unresolvable target keeps the plain binding return and + // would otherwise carry a check nothing can ever satisfy. + lazy_specs.contains(*spec) + && super::super::resolve::resolve_relative_import_path(spec, source_path).is_some() + }) + .map(|(_, local)| format!(" let {local}__rec;")) + .collect::>() + .join("\n"); // Heuristic: is any `require('')` call site lexically inside a // `try { … }` block? Reverse brace-depth scan from the call offset to // the nearest unmatched `{`, checking whether `try` precedes it. @@ -734,7 +789,7 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( .iter() .filter_map(|(name, spec)| { let n = require_specs.iter().position(|s| s == spec)?; - if builtin_requires.contains(spec) { + if builtin_requires.contains(spec) || lazy_specs.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), @@ -743,7 +798,10 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( // `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. + // surface `named_export_decls` uses below. Conditional + // requires also need the actual CJS property: forwarding + // their import binding would bypass the branch and expose + // a dependency that the module never required. Some(format!("export const {name} = _cjs.{name};")) } else { Some(format!( @@ -840,6 +898,7 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( // from the blanking filter below) and resolves through the synthetic // require's `createRequire` arm at runtime. .filter(|(_, spec, _)| !builtin_requires.contains(spec)) + .filter(|(_, spec, _)| !lazy_specs.contains(spec)) .filter_map(|(alias, spec, _range)| { let idx = require_specs.iter().position(|s| s == spec)?; // When the alias is already the spec's import local name @@ -858,6 +917,7 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( let ranges = aliases .into_iter() .filter(|(_, spec, _)| require_specs.iter().any(|s| s == spec)) + .filter(|(_, spec, _)| !lazy_specs.contains(spec)) .filter(|(alias, _, _)| !identifier_is_reassigned(source, alias)) // #sdxgen: Don't blank alias declarations for Node.js built-in // modules — let them stay in the IIFE body and resolve through @@ -1100,6 +1160,7 @@ pub(in crate::commands::compile) fn wrap_commonjs_with_body_offset( // `test/reporters` are builtins only in their `node:` form, and the switch // accepted the bare spelling too. The runtime predicate agrees with Node // 26 on all 58 names in both spellings. +{lazy_cache_decls} function require(specifier) {{ if (typeof specifier !== 'string') throw __perry_cjs_require_error('type', 'ERR_INVALID_ARG_TYPE', 'The "id" argument must be of type string.'); if (specifier === '') throw __perry_cjs_require_error('type', 'ERR_INVALID_ARG_VALUE', 'The argument "id" must be a non-empty string.'); diff --git a/crates/perry/tests/conditional_require_init.rs b/crates/perry/tests/conditional_require_init.rs new file mode 100644 index 0000000000..0c3ba8a576 --- /dev/null +++ b/crates/perry/tests/conditional_require_init.rs @@ -0,0 +1,381 @@ +//! Conditional CommonJS dependencies must run at the require call site. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +fn compile(root: &Path, entry: &str) -> PathBuf { + let compiler = PathBuf::from(env!("CARGO_BIN_EXE_perry")); + let runtime = std::env::var_os("PERRY_RUNTIME_DIR") + .map(PathBuf::from) + .unwrap_or_else(|| compiler.parent().unwrap().to_owned()); + let output = root.join("native"); + let result = Command::new(compiler) + .current_dir(root) + .args(["compile", entry, "--no-cache", "--no-auto-optimize"]) + .arg("-o") + .arg(&output) + .env("PERRY_RUNTIME_DIR", runtime) + .output() + .expect("compile fixture"); + assert!( + result.status.success(), + "{}\n{}", + String::from_utf8_lossy(&result.stdout), + String::from_utf8_lossy(&result.stderr) + ); + output +} + +fn run(binary: &Path, args: &[&str]) -> String { + let result = Command::new(binary) + .args(args) + .output() + .expect("run fixture"); + assert!( + result.status.success(), + "{}\n{}", + String::from_utf8_lossy(&result.stdout), + String::from_utf8_lossy(&result.stderr) + ); + String::from_utf8(result.stdout).unwrap() +} + +#[test] +fn conditional_require_defers_the_transitive_graph_and_initializes_once() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write( + root.join("leaf.cjs"), + "console.log('leaf'); module.exports = 42;", + ) + .unwrap(); + std::fs::write(root.join("dep.cjs"), + "const leaf = require('./leaf.cjs');\nconsole.log('dependency');\nmodule.exports = {value: leaf};").unwrap(); + std::fs::write( + root.join("entry.cjs"), + r#" +console.log('entry'); +if (process.argv.includes('--load')) { + const first = require('./dep.cjs'); + const second = require('./dep.cjs'); + console.log(first.value, first === second); +} +console.log('done'); +"#, + ) + .unwrap(); + let binary = compile(root, "entry.cjs"); + assert_eq!(run(&binary, &[]), "entry\ndone\n"); + assert_eq!( + run(&binary, &["--load"]), + "entry\nleaf\ndependency\n42 true\ndone\n" + ); +} + +#[test] +fn concise_arrow_and_short_circuit_require_stay_lazy() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write( + root.join("dep.cjs"), + "console.log('dependency'); module.exports = {value: 42};", + ) + .unwrap(); + std::fs::write( + root.join("entry.cjs"), + r#" +const load = () => require('./dep.cjs'); +console.log('entry'); +process.argv.includes('--load') && console.log(load().value); +console.log('done'); +"#, + ) + .unwrap(); + let binary = compile(root, "entry.cjs"); + assert_eq!(run(&binary, &[]), "entry\ndone\n"); + assert_eq!(run(&binary, &["--load"]), "entry\ndependency\n42\ndone\n"); +} + +#[test] +fn require_exception_is_caught_at_its_original_try_boundary() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write( + root.join("dep.cjs"), + "console.log('dependency'); throw new Error('fixture');", + ) + .unwrap(); + std::fs::write( + root.join("entry.cjs"), + r#" +console.log('entry'); +try { require('./dep.cjs'); } +catch (error) { console.log('caught', error.message); } +console.log('done'); +"#, + ) + .unwrap(); + let binary = compile(root, "entry.cjs"); + assert_eq!( + run(&binary, &[]), + "entry\ndependency\ncaught fixture\ndone\n" + ); +} + +#[test] +fn static_import_still_evaluates_before_the_entry_body() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write( + root.join("dep.mjs"), + "console.log('static'); export const value = 42;", + ) + .unwrap(); + std::fs::write( + root.join("lazy.mjs"), + "console.log('unexpected dynamic init'); export const value = 0;", + ) + .unwrap(); + std::fs::write( + root.join("entry.mjs"), + r#" +import { value } from './dep.mjs'; +export function unused() { return import('./lazy.mjs'); } +console.log('entry', value); +"#, + ) + .unwrap(); + let binary = compile(root, "entry.mjs"); + assert_eq!(run(&binary, &[]), "static\nentry 42\n"); +} + +#[test] +fn conditional_class_require_initializes_static_state() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write( + root.join("dep.cjs"), + "console.log('dependency');\nclass Value { static answer = 42; }\nmodule.exports = Value;", + ) + .unwrap(); + std::fs::write( + root.join("entry.cjs"), + r#" +class Unrelated {} +console.log('entry'); +if (process.argv.includes('--load')) { + const Value = require('./dep.cjs'); + console.log(Value.answer); +} +console.log('done'); +"#, + ) + .unwrap(); + let binary = compile(root, "entry.cjs"); + assert_eq!(run(&binary, &[]), "entry\ndone\n"); + assert_eq!(run(&binary, &["--load"]), "entry\ndependency\n42\ndone\n"); +} + +#[test] +fn conditional_named_export_does_not_forward_an_unloaded_dependency() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write( + root.join("dep.cjs"), + "console.log('dependency'); module.exports = {value: 42};", + ) + .unwrap(); + std::fs::write( + root.join("wrapper.cjs"), + r#" +console.log('wrapper'); +if (process.argv.includes('--load')) exports.optional = require('./dep.cjs'); +"#, + ) + .unwrap(); + std::fs::write( + root.join("entry.mjs"), + r#" +import { optional } from './wrapper.cjs'; +console.log('entry', optional === undefined ? 'absent' : optional.value); +"#, + ) + .unwrap(); + let binary = compile(root, "entry.mjs"); + assert_eq!(run(&binary, &[]), "wrapper\nentry absent\n"); + assert_eq!(run(&binary, &["--load"]), "wrapper\ndependency\nentry 42\n"); +} + +#[test] +fn conditional_side_effect_only_require_runs_once_when_reached() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write(root.join("dep.cjs"), "console.log('dependency');").unwrap(); + std::fs::write( + root.join("entry.cjs"), + r#" +console.log('entry'); +if (process.argv.includes('--load')) { + require('./dep.cjs'); + require('./dep.cjs'); +} +console.log('done'); +"#, + ) + .unwrap(); + let binary = compile(root, "entry.cjs"); + assert_eq!(run(&binary, &[]), "entry\ndone\n"); + assert_eq!(run(&binary, &["--load"]), "entry\ndependency\ndone\n"); +} + +#[test] +fn esm_function_local_class_require_initializes_static_state() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write( + root.join("dep.cjs"), + "console.log('dependency');\nclass Value { static answer = 42; }\nmodule.exports = Value;", + ) + .unwrap(); + std::fs::write( + root.join("entry.mjs"), + r#" +import { createRequire } from 'node:module'; +const require = createRequire(import.meta.url); +const load = () => require('./dep.cjs'); +console.log('entry'); +if (process.argv.includes('--load')) console.log(load().answer); +console.log('done'); +"#, + ) + .unwrap(); + let binary = compile(root, "entry.mjs"); + assert_eq!(run(&binary, &[]), "entry\ndone\n"); + assert_eq!(run(&binary, &["--load"]), "entry\ndependency\n42\ndone\n"); +} + +/// A deferred target in a require cycle must still see its partner's exports +/// assigned at run time: the partner reads the target's partial exports object +/// during the cycle and the value only exists after the target's body ends. +#[test] +fn conditional_require_cycle_partner_sees_runtime_assigned_exports() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write( + root.join("a.cjs"), + r#" +console.log('a start'); +exports.value = undefined; +const b = require('./b.cjs'); +exports.value = () => 'a-runtime'; +exports.read = () => b.callA(); +console.log('a end'); +"#, + ) + .unwrap(); + std::fs::write( + root.join("b.cjs"), + r#" +console.log('b start', typeof require('./a.cjs').value); +const a = require('./a.cjs'); +exports.callA = () => a.value(); +"#, + ) + .unwrap(); + std::fs::write( + root.join("entry.cjs"), + r#" +console.log('entry'); +if (process.argv.includes('--load')) { + const a = require('./a.cjs'); + console.log(a.read(), typeof a.value); +} +console.log('done'); +"#, + ) + .unwrap(); + let binary = compile(root, "entry.cjs"); + assert_eq!(run(&binary, &[]), "entry\ndone\n"); + assert_eq!( + run(&binary, &["--load"]), + "entry\na start\nb start undefined\na end\na-runtime function\ndone\n" + ); +} + +/// The same cycle shape reached from ESM through `createRequire`, next to an +/// ESM partner whose export is assigned (not declared) at module run time. +#[test] +fn esm_conditional_require_cycle_keeps_runtime_assigned_bindings() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write( + root.join("partner.mjs"), + "export let handler;\nhandler = () => 'partner-runtime';\nexport function readLater() { return typeof handler; }\n", + ) + .unwrap(); + std::fs::write( + root.join("dep.cjs"), + "console.log('dep');\nconst cyc = require('./cycle.cjs');\nmodule.exports = { run: () => cyc.call() };\n", + ) + .unwrap(); + std::fs::write( + root.join("cycle.cjs"), + "const dep = require('./dep.cjs');\nlet fn;\nfn = () => 'cycle-runtime:' + typeof dep;\nexports.call = () => fn();\n", + ) + .unwrap(); + std::fs::write( + root.join("entry.mjs"), + r#" +import { createRequire } from 'node:module'; +import { readLater } from './partner.mjs'; +const require = createRequire(import.meta.url); +console.log('entry'); +if (process.argv.includes('--load')) { + const dep = require('./dep.cjs'); + console.log(dep.run(), readLater()); +} +console.log('done'); +"#, + ) + .unwrap(); + let binary = compile(root, "entry.mjs"); + assert_eq!(run(&binary, &[]), "entry\ndone\n"); + assert_eq!( + run(&binary, &["--load"]), + "entry\ndep\ncycle-runtime:object function\ndone\n" + ); +} + +/// A `do…while` test runs only if the body falls through, so a `require` in +/// either half is conditional. Node never evaluates the dependency below. +#[test] +fn do_while_require_stays_at_its_call_site() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write( + root.join("dep.cjs"), + "console.log('dependency');\nmodule.exports = 7;", + ) + .unwrap(); + std::fs::write( + root.join("entry.cjs"), + r#" +console.log('entry'); +if (!process.argv.includes('--load')) { + do { break; } while (require('./dep.cjs')); +} else { + let seen = 0; + do { seen += require('./dep.cjs'); } while (false); + console.log('sum', seen); +} +console.log('done'); +"#, + ) + .unwrap(); + let binary = compile(root, "entry.cjs"); + assert_eq!(run(&binary, &[]), "entry\ndone\n"); + assert_eq!( + run(&binary, &["--load"]), + "entry\ndependency\nsum 7\ndone\n" + ); +} diff --git a/test-files/_helpers/gap10754_off_and.cjs b/test-files/_helpers/gap10754_off_and.cjs new file mode 100644 index 0000000000..e8b45c6255 --- /dev/null +++ b/test-files/_helpers/gap10754_off_and.cjs @@ -0,0 +1 @@ +console.log('load off_and'); diff --git a/test-files/_helpers/gap10754_off_for.cjs b/test-files/_helpers/gap10754_off_for.cjs new file mode 100644 index 0000000000..4100799c60 --- /dev/null +++ b/test-files/_helpers/gap10754_off_for.cjs @@ -0,0 +1 @@ +console.log('load off_for'); diff --git a/test-files/_helpers/gap10754_off_if.cjs b/test-files/_helpers/gap10754_off_if.cjs new file mode 100644 index 0000000000..f89b2896b6 --- /dev/null +++ b/test-files/_helpers/gap10754_off_if.cjs @@ -0,0 +1 @@ +console.log('load off_if'); diff --git a/test-files/_helpers/gap10754_off_switch.cjs b/test-files/_helpers/gap10754_off_switch.cjs new file mode 100644 index 0000000000..39dc47e848 --- /dev/null +++ b/test-files/_helpers/gap10754_off_switch.cjs @@ -0,0 +1 @@ +console.log('load off_switch'); diff --git a/test-files/_helpers/gap10754_off_tern.cjs b/test-files/_helpers/gap10754_off_tern.cjs new file mode 100644 index 0000000000..92f91ea7ca --- /dev/null +++ b/test-files/_helpers/gap10754_off_tern.cjs @@ -0,0 +1 @@ +console.log('load off_tern'); diff --git a/test-files/_helpers/gap10754_off_try.cjs b/test-files/_helpers/gap10754_off_try.cjs new file mode 100644 index 0000000000..15ba939868 --- /dev/null +++ b/test-files/_helpers/gap10754_off_try.cjs @@ -0,0 +1 @@ +console.log('load off_try'); diff --git a/test-files/_helpers/gap10754_on_exp_and.cjs b/test-files/_helpers/gap10754_on_exp_and.cjs new file mode 100644 index 0000000000..c5a8dd2f72 --- /dev/null +++ b/test-files/_helpers/gap10754_on_exp_and.cjs @@ -0,0 +1,2 @@ +console.log('load on_exp_and'); +module.exports = 'v_and'; diff --git a/test-files/_helpers/gap10754_on_exp_for.cjs b/test-files/_helpers/gap10754_on_exp_for.cjs new file mode 100644 index 0000000000..616590103a --- /dev/null +++ b/test-files/_helpers/gap10754_on_exp_for.cjs @@ -0,0 +1,2 @@ +console.log('load on_exp_for'); +module.exports = 'v_for'; diff --git a/test-files/_helpers/gap10754_on_exp_if.cjs b/test-files/_helpers/gap10754_on_exp_if.cjs new file mode 100644 index 0000000000..dcec378d29 --- /dev/null +++ b/test-files/_helpers/gap10754_on_exp_if.cjs @@ -0,0 +1,2 @@ +console.log('load on_exp_if'); +module.exports = 'v_if'; diff --git a/test-files/_helpers/gap10754_on_exp_switch.cjs b/test-files/_helpers/gap10754_on_exp_switch.cjs new file mode 100644 index 0000000000..ed82792aaa --- /dev/null +++ b/test-files/_helpers/gap10754_on_exp_switch.cjs @@ -0,0 +1,2 @@ +console.log('load on_exp_switch'); +module.exports = 'v_switch'; diff --git a/test-files/_helpers/gap10754_on_exp_tern.cjs b/test-files/_helpers/gap10754_on_exp_tern.cjs new file mode 100644 index 0000000000..730cbf2cbb --- /dev/null +++ b/test-files/_helpers/gap10754_on_exp_tern.cjs @@ -0,0 +1,2 @@ +console.log('load on_exp_tern'); +module.exports = 'v_tern'; diff --git a/test-files/_helpers/gap10754_on_exp_try.cjs b/test-files/_helpers/gap10754_on_exp_try.cjs new file mode 100644 index 0000000000..7d474d22ef --- /dev/null +++ b/test-files/_helpers/gap10754_on_exp_try.cjs @@ -0,0 +1,2 @@ +console.log('load on_exp_try'); +module.exports = 'v_try'; diff --git a/test-files/_helpers/gap10754_on_sfx_and.cjs b/test-files/_helpers/gap10754_on_sfx_and.cjs new file mode 100644 index 0000000000..3620d3acb1 --- /dev/null +++ b/test-files/_helpers/gap10754_on_sfx_and.cjs @@ -0,0 +1 @@ +console.log('load on_sfx_and'); diff --git a/test-files/_helpers/gap10754_on_sfx_for.cjs b/test-files/_helpers/gap10754_on_sfx_for.cjs new file mode 100644 index 0000000000..5c99758712 --- /dev/null +++ b/test-files/_helpers/gap10754_on_sfx_for.cjs @@ -0,0 +1 @@ +console.log('load on_sfx_for'); diff --git a/test-files/_helpers/gap10754_on_sfx_if.cjs b/test-files/_helpers/gap10754_on_sfx_if.cjs new file mode 100644 index 0000000000..94566d43d8 --- /dev/null +++ b/test-files/_helpers/gap10754_on_sfx_if.cjs @@ -0,0 +1 @@ +console.log('load on_sfx_if'); diff --git a/test-files/_helpers/gap10754_on_sfx_switch.cjs b/test-files/_helpers/gap10754_on_sfx_switch.cjs new file mode 100644 index 0000000000..1bc263969a --- /dev/null +++ b/test-files/_helpers/gap10754_on_sfx_switch.cjs @@ -0,0 +1 @@ +console.log('load on_sfx_switch'); diff --git a/test-files/_helpers/gap10754_on_sfx_tern.cjs b/test-files/_helpers/gap10754_on_sfx_tern.cjs new file mode 100644 index 0000000000..99700bb86f --- /dev/null +++ b/test-files/_helpers/gap10754_on_sfx_tern.cjs @@ -0,0 +1 @@ +console.log('load on_sfx_tern'); diff --git a/test-files/_helpers/gap10754_on_sfx_try.cjs b/test-files/_helpers/gap10754_on_sfx_try.cjs new file mode 100644 index 0000000000..205828bc7d --- /dev/null +++ b/test-files/_helpers/gap10754_on_sfx_try.cjs @@ -0,0 +1 @@ +console.log('load on_sfx_try'); diff --git a/test-files/test_gap_10754_cjs_conditional_require_shapes.cts b/test-files/test_gap_10754_cjs_conditional_require_shapes.cts new file mode 100644 index 0000000000..7aa6bc5802 --- /dev/null +++ b/test-files/test_gap_10754_cjs_conditional_require_shapes.cts @@ -0,0 +1,135 @@ +// #10754: a top-level CommonJS `require()` whose call site sits in a +// short-circuit (`cond && require(...)`), a ternary consequent +// (`cond ? require(...) : x`), a `try` block, a `switch` case or a loop body +// was SILENTLY NEVER EVALUATED — the module did not load even when the +// branch was taken. Node loads it. No crash, no diagnostic: the dependency's +// side effects simply never happened. +// +// History. #10437 was the opposite bug: every such `require()` was hoisted +// into an eager synthetic import and ran even when its branch did not. +// #10674 fixed that by DEFERRING the target instead of hoisting it, and got +// the `if (cond) require(...)` shape right. In the five shapes above the +// deferral had no evaluation site left: the require shim returned the +// deferred import binding, and firing the target's `__init()` was gated on +// that binding being a known imported FUNCTION. A dependency with no +// default export — a side-effect-only module, the commonest shape for a +// polyfill or a registration hook — has no such binding to read, so nothing +// ever ran it. +// +// The fixture therefore asserts BOTH directions for every shape, because a +// fix that makes the module load unconditionally is #10437 again, not a fix: +// +// on_sfx_* taken branch, SIDE-EFFECT-ONLY target -> must load (the bug) +// on_exp_* taken branch, value-returning target -> must load AND the +// value must be the +// target's exports +// off_* branch not taken -> must NOT load +// +// The not-taken direction with VALUE-RETURNING targets is #10437's own +// fixture (test_gap_cjs_conditional_require_deferred.ts); the side-effect-only +// targets here are the half it does not reach. +// +// `.cts`, deliberately: this repo's package is `"type": "module"`, so a plain +// `.ts` runs as an ES module under Node and a bare `require` dies with +// `require is not defined`. The extension makes both engines agree on the +// CommonJS goal — the goal the issue is about, since the reproducer is a +// `.cjs` program entry. See test_gap_9412's header for the full rationale. +// Keep this file free of top-level `import`/`export`. + +// Runtime-unknown guards: neither engine can fold these at compile time, so +// the compiler cannot decide the branch statically and must emit whatever it +// would emit for a genuine runtime condition (`pg`'s `if (forceNative)`). +const off = Boolean(process.env.PERRY_GAP10754_UNSET); +const on = !off; + +console.log('start'); + +// ---------------------------------------------------------------- not taken +// Each of these must print NOTHING. A load line here is #10437 again. +if (off) require('./_helpers/gap10754_off_if.cjs'); +off && require('./_helpers/gap10754_off_and.cjs'); +const offTern = off ? require('./_helpers/gap10754_off_tern.cjs') : 'skipped'; +try { + if (off) require('./_helpers/gap10754_off_try.cjs'); +} catch (e) { + console.log('unexpected off_try throw'); +} +switch (off) { + case true: + require('./_helpers/gap10754_off_switch.cjs'); + break; + default: +} +for (let i = 0; off && i < 1; i++) require('./_helpers/gap10754_off_for.cjs'); +console.log('not-taken done, offTern=' + offTern); + +// ------------------------------------------- taken, side-effect-only target +// Each load line must appear between its own two markers: the module runs at +// the moment control flow reaches the call, not before the file's first +// statement and not after the branch. +console.log('sfx if:'); +if (on) require('./_helpers/gap10754_on_sfx_if.cjs'); +console.log('sfx and:'); +on && require('./_helpers/gap10754_on_sfx_and.cjs'); +console.log('sfx tern:'); +const sfxTern = on ? require('./_helpers/gap10754_on_sfx_tern.cjs') : 'skipped'; +console.log('sfx try:'); +try { + if (on) require('./_helpers/gap10754_on_sfx_try.cjs'); +} catch (e) { + console.log('unexpected on_sfx_try throw'); +} +console.log('sfx switch:'); +switch (on) { + case true: + require('./_helpers/gap10754_on_sfx_switch.cjs'); + break; + default: +} +console.log('sfx for:'); +for (let i = 0; on && i < 1; i++) require('./_helpers/gap10754_on_sfx_for.cjs'); +console.log('sfx done, typeof sfxTern=' + typeof sfxTern); + +// ---------------------------------------------- taken, value-returning target +// Same six shapes, but the target replaces `module.exports`. Pins that the +// shim hands back the TARGET's exports, not the synthetic import binding or +// an empty object. +console.log('exp if:'); +let expIf: unknown = 'unset'; +if (on) expIf = require('./_helpers/gap10754_on_exp_if.cjs'); +console.log('exp and:'); +let expAnd: unknown = 'unset'; +on && (expAnd = require('./_helpers/gap10754_on_exp_and.cjs')); +console.log('exp tern:'); +const expTern = on ? require('./_helpers/gap10754_on_exp_tern.cjs') : 'unset'; +console.log('exp try:'); +let expTry: unknown = 'unset'; +try { + if (on) expTry = require('./_helpers/gap10754_on_exp_try.cjs'); +} catch (e) { + console.log('unexpected on_exp_try throw'); +} +console.log('exp switch:'); +let expSwitch: unknown = 'unset'; +switch (on) { + case true: + expSwitch = require('./_helpers/gap10754_on_exp_switch.cjs'); + break; + default: +} +console.log('exp for:'); +let expFor: unknown = 'unset'; +for (let i = 0; on && i < 1; i++) expFor = require('./_helpers/gap10754_on_exp_for.cjs'); +console.log( + 'exp values: ' + + [expIf, expAnd, expTern, expTry, expSwitch, expFor].join(','), +); + +// Require caching: a second conditional require of an ALREADY-loaded target +// must not re-run its body and must hand back the same exports. +console.log('cache:'); +let again: unknown = 'unset'; +on && (again = require('./_helpers/gap10754_on_exp_and.cjs')); +console.log('cache same=' + (again === expAnd)); + +console.log('end'); From 8226c7488b430b44a9112648332663d9e569d6c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 19 Sep 2026 22:55:15 +0200 Subject: [PATCH 2/2] changelog: #10756 --- ...conditional-require-target-export-shape.md | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 changelog.d/10756-conditional-require-target-export-shape.md diff --git a/changelog.d/10756-conditional-require-target-export-shape.md b/changelog.d/10756-conditional-require-target-export-shape.md new file mode 100644 index 0000000000..cc9111f9b8 --- /dev/null +++ b/changelog.d/10756-conditional-require-target-export-shape.md @@ -0,0 +1,62 @@ +### Fixed + +**A conditional CommonJS `require()` of a side-effect-only module never ran it (#10754).** + +A top-level `require()` in a conditional position loaded nothing when its target +carried no CommonJS export marker — the branch was taken, the shim was reached, +and the module body never ran. Silent: no crash, no diagnostic, just a +dependency whose side effects never happened. + +The issue reports the failure as syntactic (`if` works, `&&` / `?:` / `try` / +`switch` / loop-body do not). That is a confound in the reproducer: its `if` +case required a module with `module.exports = 1` while the other five required +side-effect-only modules. Crossing the two axes against Node 26.5.1 on a +release build of `main` @ 91a566c8af shows the discriminator is the TARGET's +export shape, not the call-site shape — all six shapes fail with a +side-effect-only target and all six pass with a value-returning one. + +Root cause. #10674 defers a conditional require correctly: the target stays +`ModuleInitKind::Deferred` and the CJS shim returns the `_lazyreq_N` import +binding, with codegen firing `__init()` at the binding read +(`perry-codegen/src/expr/dyn_extern_i18n.rs`). That init call is gated on the +binding being a known imported FUNCTION (`ctx.import_function_prefixes`). A +target with no default export has no such entry, so nothing fires. The `if` +cases that "worked" worked because their targets had a default export to read. + +The fix (PR #10285's implementation, rebased, plus a registry-miss fallback): + +- `cjs_wrap/deferred_requires.rs` — an AST visitor replaces the text-scanning + deferral classifier. The scanner missed `if (cond) x = require('S')` (still + eagerly hoisted on `main`, a residual #10437 shape), concise arrows, the + ternary ALTERNATE arm, both halves of a `do`/`while`, and `&&=`/`||=`/`??=`. + `extract_requires::function_local_specs` remains the parse-failure fallback. +- `cjs_wrap/wrap.rs` — a deferred specifier resolves through the path registry + (`__perry_require_path_module`) rather than its import binding, so + initialization no longer depends on the target's export shape. The registry + record is memoized per call site once `loaded === true`: re-entering the + registry on every call measured 3.4x on a hot require. +- `cjs_wrap/wrap.rs`, new on top of #10285 — the registry only holds EXPORTS for + a target that publishes them, which is every CJS-wrapped module and no other. + A file with no CommonJS marker at all is not CJS-wrapped, so it registers an + initializer and never any exports: the registry ran its body and returned + `undefined` where Node returns `{}`, so + `const v = cond ? require('./side-effect-only.cjs') : 0` came back + `undefined`. The arm now falls back to the import binding on a genuine + registry miss, discriminated by `__perry_has_path_module` (a real module may + export `undefined`) — the same guard the generic runtime-`require(path)` arm + in the same wrapper already uses. +- `perry-codegen/src/expr/dyn_extern_i18n.rs` — fire the deferred `__init()` + before the imported-class and namespace fast paths, which can themselves + depend on module initialization. + +Validation. `test-files/test_gap_10754_cjs_conditional_require_shapes.cts` +crosses all six shapes with three cells each — taken/side-effect-only, +taken/value-returning, and not-taken — because a fix that loads the module +unconditionally is #10437 again, not a fix. It fails on unfixed `main` — +12 differing lines against the Node oracle: six side-effect-only targets never +load at all, and three value-returning targets load before the program's first +statement instead of at their call site (`if (cond) x = require('S')`, +`cond && (x = require('S'))` and the same in a `for` body, the shapes the text +scanner could not see) — and matches Node byte-for-byte with the fix. The +harness agrees: `parity_fail` on unfixed `main`, `PASS` with the change. The existing #10437 +fixture covers the not-taken direction with value-returning targets only.