fix(code-splitting): keep CommonJS modules wrapped under strict execution order#10405
Conversation
✅ Deploy Preview for rolldown-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Merging this PR will not alter performance
Comparing Footnotes
|
|
Really nice fix — the writeup made the root cause easy to follow, and I like that you documented the strict-mode exception in Just two cosmetic things, neither blocking:
|
|
The regression coverage should be two separate Rust integration fixtures, one per finding. 1.
|
|
Verification on the freshly fetched latest $ git rev-parse --short HEAD
cd742e701
$ just t-run crates/rolldown/tests/rolldown/function/experimental/strict_execution_order/standalone_cjs_entry_named_exports/_config.json
# exit 0: wrap-all passed; on-demand passed
$ just t-run crates/rolldown/tests/rolldown/function/experimental/strict_execution_order/standalone_cjs_entry_node_module/_config.json
# exit 0: wrap-all passed; on-demand passedThe same fixtures fail independently on the current PR implementation ( # standalone_cjs_entry_named_exports
- expected: { foo: 1 }
+ actual: { default: { foo: 1 } }
# standalone_cjs_entry_node_module, executed as the CLI
- expected: { filename: "<dist>/main.js", parentIsNull: true, isMain: true }
+ actual: { parentIsNull: false, isMain: false } |
|
Thanks for catching this — both fixtures were red on the previous revision exactly as you showed. The unconditional wrap traded away a standalone CJS entry's Node module contract (named-exports shape, Fixed in aeab531 by gating the link-stage force-wrap on One remaining coarseness, documented in Also took both cosmetic points: |
|
Thanks for the follow-up. The two original I extended both Rust integration fixtures:
with the following variants: "configVariants": [
{
"_configName": "on-demand",
"onDemandWrapping": true
+ },
+ {
+ "_configName": "unmatched-group-wrap-all",
+ "codeSplitting": {
+ "groups": [
+ {
+ "name": "unused",
+ "test": "never-match-entry\\.js$"
+ }
+ ]
+ }
+ },
+ {
+ "_configName": "unmatched-group-on-demand",
+ "codeSplitting": {
+ "groups": [
+ {
+ "name": "unused",
+ "test": "never-match-entry\\.js$"
+ }
+ ]
+ },
+ "onDemandWrapping": true
}
]Finding[P1] The force-wrap condition checks whether any group is configured, rather than whether the CJS entry is actually captured: if self.options.is_strict_execution_order_enabled()
&& self.options.has_manual_code_splitting_groups()For the configuration above, the group matches nothing and the entry remains in its original entry chunk. It is nevertheless forced through # standalone_cjs_entry_named_exports
- expected: { foo: 1 }
+ actual: { default: { foo: 1 } }
# standalone_cjs_entry_node_module
- expected module.filename: "<dist>/main.js"
+ actual module.filename: undefined
- expected CLI: { parentIsNull: true, isMain: true }
+ actual CLI: { parentIsNull: false, isMain: false }VerificationThe exact expanded fixtures pass on latest On the current PR branch ( The wrapping decision therefore needs actual entry-capture information, or an equivalent post-placement downgrade for entries that remain in their own chunks. Group presence alone is not sufficient. |
This is expected behavior. Currently I don't want to introduce a "sound" detection on "if manual group captures modules". It's a bit of complicated and I tend avoid it for now. |
Merge activity
|
…tion order (#10405) ## Bug Two mutually unreachable CommonJS entries, co-located by a `codeSplitting` group, in cjs output with `strictExecutionOrder: true`: ```js // effect.js (entry A) console.log('effect'); module.exports = { name: 'effect' }; // silent.js (entry B, no relation to A) module.exports = { name: 'silent' }; ``` ```js // rolldown.config { input: { effect: './effect.js', silent: './silent.js' }, output: { format: 'cjs', strictExecutionOrder: true, codeSplitting: { groups: [{ name: 'common' }] }, }, } ``` Current main emits both entry bodies raw in the group chunk: ```js // common.js console.log('effect'); // entry A's top level module.exports = { name: 'effect' }; module.exports = { name: 'silent' }; // clobbers the line above // silent.js (entry facade) require("./common.js"); // runs A's side effects; exports lost ``` `require('./silent.js')` executes entry A and returns `{}`. This build is byte-identical to the `strictExecutionOrder: false` output — strict silently stops engaging. v1.2.0 is correct: ```js // common.js (v1.2.0) var require_effect = __commonJSMin((exports, module) => { console.log('effect'); module.exports = { name: 'effect' }; }); var require_silent = __commonJSMin((exports, module) => { module.exports = { name: 'silent' }; }); Object.defineProperty(exports, "require_effect", { get: () => require_effect }); Object.defineProperty(exports, "require_silent", { get: () => require_silent }); // silent.js (v1.2.0 entry facade) module.exports = require("./common.js").require_silent(); ``` Unreleased regression, bisected with the execution-order fuzzer's directed control (`cross-entry-cjs-output-strict`): | build | result | | --- | --- | | v1.2.0 (npm) | pass | | `ce98ae798` (parent of the #10104 squash) | pass | | `1d86ffe06` (#10104) | events-reordered | | current `main` | events-reordered | ## Cause A CommonJS module has exactly one deferral mechanism: its `__commonJS` wrapper. Two rules decide who gets one, and after #10104 a never-imported CommonJS module falls through both: - **Interop wrapping (link stage)** wraps a CJS module only when something imports or requires it. A CJS *entry* in cjs output has no importer, keeps `WrapKind::None`, and renders raw. - **Order wrapping (generate stage)** is ESM-only: `is_order_wrap_eligible` requires `ExportsKind::Esm | None`, and the lowering synthesizes only `init_*`/`__esm` wrappers. Pre-#10104, `wrap_modules` closed this gap — under `strictExecutionOrder` every CommonJS module was forced to `WrapKind::Cjs`. #10104 deleted that override together with the ESM wrap-all logic it replaced, although its own stated non-goal was "Changing CJS or require-of-ESM interop output when no order wrapper is selected". This restores the CJS half only; the ESM half is genuinely owned by order analysis now. ## Why this must live at the link stage, not in order lowering The natural question is why order lowering doesn't just handle the case itself — it runs post-chunking and can see the hazardous placement. Detection is not the problem; repair is: - **For CommonJS, representation *is* deferral.** An ESM order wrapper is a side-effect trigger (`init_x()`); a CJS wrapper must materialize and return `module.exports`, with `module`/`exports` rebound to closure parameters, a `module.exports = require_x()` entry facade, and live getter exports. That is the interop `WrapKind::Cjs` pathway — order lowering has no CJS-shaped tool. - **`wrap_kind()` is immutable after linking, by #10104's own design.** `OrderWrapState` holds all late state explicitly *without changing link-stage `WrapKind`*, and the module finalizer, `render_chunk_exports`, and `compute_cross_chunk_links` all dispatch on `wrap_kind()`. Wrapping CJS late would either mutate that sealed fact or thread a second wrap-kind channel through every consumer — duplicating existing interop machinery. - **The gate — strict plus configured `codeSplitting` groups — is the semantically correct condition.** Without strict, a lone CJS entry rendering raw is the intended minimal output. Under strict, the hazard still needs adverse placement: the only mechanism that can move a never-imported CommonJS module out of its own entry chunk is a group capture (standard chunk assignment keeps an entry with its exclusive dependencies, and every chunk-optimizer merge path either requires the entry chunk to have been emptied by a capture first or degenerates to "loading the chunk *is* executing that entry"). Placement itself is invisible to the link stage, but the groups config is not. With groups configured, `strictExecutionOrder` is exactly the user's opt-in to "stay correct under any placement, pay the wrapper bytes" — the same reasoning as wrap-all deferring hazard-free ESM modules. Without groups the raw body is load-bearing, not just byte-smaller: review surfaced that the wrapper's synthetic `module`/`exports` parameters break a standalone CJS entry's Node module contract — `exports: 'named'` output collapses to `{ default: getter }`, `module.filename` reads undefined, `module.parent` loses its value, and `require.main === module` turns false — all of which the raw form preserves. The first revision of this PR wrapped unconditionally under strict and regressed exactly that; the gate restores main's behavior for every no-groups config. Cost: under strict with groups configured, a never-imported CJS entry is wrapped even when no group actually captures it — identical to every released version's output under groups. If that ever matters, the refinement is a generate-stage *render* downgrade backed by a placement proof (the same conservative-first pattern as the planned `init_*` hoisted-form narrowing), not a late wrap channel. ## Tests `function/experimental/strict_execution_order/cjs_entry_group_isolation` — the shape above, wrap-all and on-demand cells. Snapshot pins the wrapped shape; `_test.mjs` executes the output: requiring the silent entry must run no foreign top level and both entries must keep their own `module.exports`. Red on main without the fix. No existing fixture combined CommonJS-source entries with cjs-format strict output, which is why #10104's snapshot audit could not see this. `standalone_cjs_entry_named_exports` and `standalone_cjs_entry_node_module` (from review, thanks @IWANABETHATGUY) pin the no-groups arm in the same wrap-all and on-demand cells: a standalone CJS entry under strict must keep rendering raw, preserving its named-exports shape and Node module identity (`module.filename`, `module.parent`, `require.main === module`, both required and run as the CLI). Red on this PR's first revision, green on main and on the gated fix. ## Validation - Full integration fixture suite and workspace unit tests pass with no snapshot changes outside the new fixture; clippy `-D warnings` and `cargo fmt --check` clean. - Execution-order fuzzer current-target audit against this branch: `cross-entry-cjs-output-strict` flips to pass. The two `cjs-output-facade-cycle` strict controls (uninitialized-wrapper-export finding from the same fuzzer list) also flip to pass — a wrapped entry is reached through a live getter instead of a not-yet-installed export. That finding's `strictExecutionOrder: false` arm is out of scope here. `family-a-existing-seed` stays known-red (separate bug). - A four-cell matrix ({cjs,esm} × {seo:true,false}) reproduces v1.2.0 behavior exactly: cjs+strict isolated with correct exports, esm cells clean, cjs+seo:false unchanged (#9998). - After gating on groups: full suite green again with the `cjs_entry_group_isolation` snapshot byte-identical, so every groups-present config keeps the first revision's output; no-groups configs are byte-identical to main by construction (the force-wrap block is skipped entirely). The fuzzer results above all exercise group configs and are unaffected. Tracked in #10294 (execution-order fuzzer findings, CJS-output strict cross-entry leak). Related: #10104 (introducing change), #9998 (the pre-existing seo:false arm of the same co-location root, unchanged here), #9997 (the original strict isolation guarantee), #10401 (closed chunking-side pin for the facade-cycle shape; this change is the "wrap captured entries" direction recorded there).
d513322 to
959002b
Compare
Bug
Two mutually unreachable CommonJS entries, co-located by a
codeSplittinggroup, in cjs output withstrictExecutionOrder: true:Current main emits both entry bodies raw in the group chunk:
require('./silent.js')executes entry A and returns{}. This build is byte-identical to thestrictExecutionOrder: falseoutput — strict silently stops engaging. v1.2.0 is correct:Unreleased regression, bisected with the execution-order fuzzer's directed control (
cross-entry-cjs-output-strict):ce98ae798(parent of the #10104 squash)1d86ffe06(#10104)mainCause
A CommonJS module has exactly one deferral mechanism: its
__commonJSwrapper. Two rules decide who gets one, and after #10104 a never-imported CommonJS module falls through both:WrapKind::None, and renders raw.is_order_wrap_eligiblerequiresExportsKind::Esm | None, and the lowering synthesizes onlyinit_*/__esmwrappers.Pre-#10104,
wrap_modulesclosed this gap — understrictExecutionOrderevery CommonJS module was forced toWrapKind::Cjs. #10104 deleted that override together with the ESM wrap-all logic it replaced, although its own stated non-goal was "Changing CJS or require-of-ESM interop output when no order wrapper is selected". This restores the CJS half only; the ESM half is genuinely owned by order analysis now.Why this must live at the link stage, not in order lowering
The natural question is why order lowering doesn't just handle the case itself — it runs post-chunking and can see the hazardous placement. Detection is not the problem; repair is:
init_x()); a CJS wrapper must materialize and returnmodule.exports, withmodule/exportsrebound to closure parameters, amodule.exports = require_x()entry facade, and live getter exports. That is the interopWrapKind::Cjspathway — order lowering has no CJS-shaped tool.wrap_kind()is immutable after linking, by feat(code-splitting): wrap strict execution order modules on demand #10104's own design.OrderWrapStateholds all late state explicitly without changing link-stageWrapKind, and the module finalizer,render_chunk_exports, andcompute_cross_chunk_linksall dispatch onwrap_kind(). Wrapping CJS late would either mutate that sealed fact or thread a second wrap-kind channel through every consumer — duplicating existing interop machinery.codeSplittinggroups — is the semantically correct condition. Without strict, a lone CJS entry rendering raw is the intended minimal output. Under strict, the hazard still needs adverse placement: the only mechanism that can move a never-imported CommonJS module out of its own entry chunk is a group capture (standard chunk assignment keeps an entry with its exclusive dependencies, and every chunk-optimizer merge path either requires the entry chunk to have been emptied by a capture first or degenerates to "loading the chunk is executing that entry"). Placement itself is invisible to the link stage, but the groups config is not. With groups configured,strictExecutionOrderis exactly the user's opt-in to "stay correct under any placement, pay the wrapper bytes" — the same reasoning as wrap-all deferring hazard-free ESM modules.Without groups the raw body is load-bearing, not just byte-smaller: review surfaced that the wrapper's synthetic
module/exportsparameters break a standalone CJS entry's Node module contract —exports: 'named'output collapses to{ default: getter },module.filenamereads undefined,module.parentloses its value, andrequire.main === moduleturns false — all of which the raw form preserves. The first revision of this PR wrapped unconditionally under strict and regressed exactly that; the gate restores main's behavior for every no-groups config.Cost: under strict with groups configured, a never-imported CJS entry is wrapped even when no group actually captures it — identical to every released version's output under groups. If that ever matters, the refinement is a generate-stage render downgrade backed by a placement proof (the same conservative-first pattern as the planned
init_*hoisted-form narrowing), not a late wrap channel.Tests
function/experimental/strict_execution_order/cjs_entry_group_isolation— the shape above, wrap-all and on-demand cells. Snapshot pins the wrapped shape;_test.mjsexecutes the output: requiring the silent entry must run no foreign top level and both entries must keep their ownmodule.exports. Red on main without the fix. No existing fixture combined CommonJS-source entries with cjs-format strict output, which is why #10104's snapshot audit could not see this.standalone_cjs_entry_named_exportsandstandalone_cjs_entry_node_module(from review, thanks @IWANABETHATGUY) pin the no-groups arm in the same wrap-all and on-demand cells: a standalone CJS entry under strict must keep rendering raw, preserving its named-exports shape and Node module identity (module.filename,module.parent,require.main === module, both required and run as the CLI). Red on this PR's first revision, green on main and on the gated fix.Validation
-D warningsandcargo fmt --checkclean.cross-entry-cjs-output-strictflips to pass. The twocjs-output-facade-cyclestrict controls (uninitialized-wrapper-export finding from the same fuzzer list) also flip to pass — a wrapped entry is reached through a live getter instead of a not-yet-installed export. That finding'sstrictExecutionOrder: falsearm is out of scope here.family-a-existing-seedstays known-red (separate bug).cjs_entry_group_isolationsnapshot byte-identical, so every groups-present config keeps the first revision's output; no-groups configs are byte-identical to main by construction (the force-wrap block is skipped entirely). The fuzzer results above all exercise group configs and are unaffected.Tracked in #10294 (execution-order fuzzer findings, CJS-output strict cross-entry leak). Related: #10104 (introducing change), #9998 (the pre-existing seo:false arm of the same co-location root, unchanged here), #9997 (the original strict isolation guarantee), #10401 (closed chunking-side pin for the facade-cycle shape; this change is the "wrap captured entries" direction recorded there).