fix(cjs): evaluate a conditional require whatever its target exports - #10756
Closed
proggeramlug wants to merge 2 commits into
Closed
proggeramlug wants to merge 2 commits into
proggeramlug wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (26)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe CommonJS wrapper now detects conditional ChangesConditional require evaluation
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant CJSWrapper
participant DeferredInit
participant PathRegistry
participant RequiredModule
CJSWrapper->>DeferredInit: evaluate conditional require
DeferredInit->>PathRegistry: resolve deferred specifier
PathRegistry->>RequiredModule: initialize target
RequiredModule-->>PathRegistry: return exports
PathRegistry-->>CJSWrapper: return cached exports
Merge Risk: ⚪ Minimal · up to No concrete merge-blocking risk remains in the conditional require changes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 47.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 25 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
proggeramlug
pushed a commit
that referenced
this pull request
Sep 19, 2026
added 2 commits
September 19, 2026 22:55
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 @ 91a566c 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 `<S>__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
proggeramlug
force-pushed
the
fix/10754-conditional-require-shapes
branch
from
September 19, 2026 20:55
57d49eb to
8226c74
Compare
proggeramlug
marked this pull request as ready for review
September 19, 2026 22:25
proggeramlug
pushed a commit
that referenced
this pull request
Sep 19, 2026
Contributor
Author
|
Landed in merge train 228 (#10764), released as v0.5.1607 — main is now Closing rather than merging is how trains work here: both PRs were cherry-picked onto one tree, validated together, and landed under the train's own commit, so GitHub cannot mark this one merged even though your change is on main. Counts re-derived on the assembled tree, not carried: workspace 76/27/44, ledger 376/326, unrooted 578. Worth recording that Validation: all nine cheap gates, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A top-level CommonJS
require()whose target has no CommonJS export markerwas silently never evaluated when the call site was conditional. 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.
Closes #10754The measurement that matters
The issue reports the failure as syntactic:
if (cond) require(...)works,&&/?:/try/switch/ loop-body do not. That is a confound in thereproducer — its
ifcase required a module withmodule.exports = 1, theother five required side-effect-only modules. Crossing the two axes on a
release build of
main@ 91a566c (perry 0.5.1605) shows the realdiscriminator is the target's export shape, not the call-site shape:
module.exportstargetif (go) require(t)go && require(t)go ? require(t) : 0try { if (go) require(t) }switch (go) { case true: require(t) }for (…; go && …;) require(t)(Branch-not-taken is correct in all twelve cells on
main— #10674's deferralholds. Node loads in all twelve taken cells.)
Why
#10674 correctly defers a conditional require: the target stays
ModuleInitKind::Deferredand the CJS shim returns the_lazyreq_Nimportbinding, with codegen firing
<S>__init()at the binding read. That init callis gated on the binding being a known imported function
(
ctx.import_function_prefixes). A target with no default export has no suchentry, so nothing fires and the module body never runs. The
if-shape casesthat "worked" worked because their targets had a default export to read.
The fix
This is PR #10285's implementation, rebased onto current
main, plus oneaddition (below). It:
(
cjs_wrap/deferred_requires.rs) — the text scanner missedif (cond) x = require('S')(still eagerly hoisted onmain, a residualCommonJS
require()outside a function is hoisted and run unconditionally at module init, including insideif (false)and other branches that never run #10437 shape), concise arrows, the ternary alternate arm,do/whilehalves and
&&=/||=/??=;(
__perry_require_path_module) instead of its import binding, soinitialization no longer depends on the target's export shape;
loaded === true, becausere-entering the registry on every call measured 3.4x on a hot require.
Addition on top of #10285. Routing through the registry regressed the
value of a deferred require whose target is not CJS-wrapped at all: a file
with no CommonJS marker (
console.log('x')and nothing else) registers aninitializer but never any exports, so the registry ran its body and handed back
undefinedwhere Node hands back{}. The runtime-record arm now falls back tothe import binding on a genuine registry miss, using
__perry_has_path_moduleas the miss-vs-
undefined-export discriminator — the same guard the genericruntime-
require(path)arm in the same wrapper already uses. Without itconst v = cond ? require('./side-effect-only.cjs') : 0yieldsundefined.Relationship to #10285
This supersedes #10285. Same implementation, rebased (it cherry-picks cleanly),
plus the registry-miss fallback above, plus the gap fixture. #10285 stalled
because it looked superseded by #10674; it is not — #10674 fixed the
classification, #10285 fixes the evaluation site, and only together do all
twelve cells pass.
Both directions, or it isn't a fix
#10437 was the opposite bug (eager hoist: the module loaded when the branch was
not taken). A fix that loads unconditionally is that bug again, so
test-files/test_gap_10754_cjs_conditional_require_shapes.ctsasserts everyshape in both directions and in three cells per shape:
on_sfx_*— taken branch, side-effect-only target → must load;on_exp_*— taken branch, value-returning target → must load and theshim must hand back the target's exports;
off_*— branch not taken → must not load.The existing #10437 fixture covers the not-taken direction with
value-returning targets only; the side-effect-only half is what this adds.
Validation
Two release arms, each built in one invocation
(
cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-static)into its own target dir,
PERRY_RUNTIME_DIRpinned to its own archives, bothstamping
perry 0.5.1605:91a566c8af(unfixedmain);test_gap_10754_cjs_conditional_require_shapes.ctsvs Noderun_parity_tests.shparity_fail(harness exit 1)PASScargo test -p perry --test conditional_require_initNo collateral in the CJS-adjacent gap slices (fast mode,
PERRY_RUN_TIMEOUT=30,prebuilt compiler), 45 tests, all green:
--filter test_gap_ --filter …cjsrequiremoduleimporttest_gap_cjs_conditional_require_deferred(#10437's own fixture, theeager-hoist direction) is in the
cjsslice and passes.Out of scope, noticed while measuring
JSON.stringify(require('./side-effect-only.cjs'))returns""where Nodereturns
{}. Pre-existing on unfixedmainon the eager path as well, so it isnot this change; filed here only so it is on the record.
Summary by CodeRabbit
Bug Fixes
require()calls so side-effect-only modules execute when their branch is taken.tryblocks.Tests