fix(deconflict): rename CJS-wrapped locals that shadow chunk-root bindings#9921
Merged
Merged
Conversation
✅ Deploy Preview for rolldown-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hello and thank you for tackling this! I'm happy to help test this against the originally reported issue if there's a way to do so. I extracted the reproduction code from a much larger codebase (Reddit's frontend repo). |
@rolldown/browser
@rolldown/debug
rolldown
@rolldown/binding-android-arm64
@rolldown/binding-darwin-arm64
@rolldown/binding-darwin-x64
@rolldown/binding-freebsd-x64
@rolldown/binding-linux-arm-gnueabihf
@rolldown/binding-linux-arm64-gnu
@rolldown/binding-linux-arm64-musl
@rolldown/binding-linux-ppc64-gnu
@rolldown/binding-linux-s390x-gnu
@rolldown/binding-linux-x64-gnu
@rolldown/binding-linux-x64-musl
@rolldown/binding-openharmony-arm64
@rolldown/binding-wasm32-wasi
@rolldown/binding-win32-arm64-msvc
@rolldown/binding-win32-x64-msvc
commit: |
Member
Author
You can try overriding rolldown with pkg.pr version. |
|
This change has fixed the original issue. Thank you! |
0acfd6b to
89d1a08
Compare
89d1a08 to
c3968fa
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
Member
Author
How to use the Graphite Merge QueueAdd the label graphite: merge-when-ready to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a deconfliction bug where CJS-wrapped module locals (var hoisted inside the __commonJS closure) could shadow referenced chunk-root bindings, causing runtime failures like Cannot read properties of undefined.
Changes:
- Add a CJS-specific post-pass (
rename_cjs_locals_shadowing_referenced_chunk_bindings) that detects root-scope locals shadowing referenced chunk-root bindings and renames only the shadowing local. - Add
Renamer::override_root_scope_bindingto safely override already-assigned root-scope names while avoiding reserved names and any existing module binding names (including sibling locals likefoo$1). - Add two integration regression fixtures (issue repro + second-order suffix-skipping variant) that execute the built bundle.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rolldown/src/utils/renamer.rs | Adds the override helper and a CJS-only pass to rename shadowing root-scope locals based on actual referenced chunk-root bindings. |
| crates/rolldown/src/utils/chunk/deconflict_chunk_symbols.rs | Runs the new CJS post-pass as part of nested-scope shadowing renames. |
| crates/rolldown/tests/rolldown/issues/9882/main.js | Source fixture reproducing the original shadowing bug scenario. |
| crates/rolldown/tests/rolldown/issues/9882/dependency.js | Dependency module whose binding is hoisted to chunk root and referenced by the CJS-wrapped entry. |
| crates/rolldown/tests/rolldown/issues/9882/artifacts.snap | Snapshot asserting the generated output no longer shadows the chunk-root binding. |
| crates/rolldown/tests/rolldown/issues/9882/_test.mjs | Executes the built bundle to ensure no runtime throw. |
| crates/rolldown/tests/rolldown/issues/9882/_config.json | Integration test configuration for the #9882 fixture. |
| crates/rolldown/tests/rolldown/topics/deconflict/cjs_shadowing_renamed_chunk_binding/main.js | Second-order fixture ensuring the shadowing-local rename skips existing sibling suffixes. |
| crates/rolldown/tests/rolldown/topics/deconflict/cjs_shadowing_renamed_chunk_binding/dependency.js | Dependency module used by the second-order deconfliction fixture. |
| crates/rolldown/tests/rolldown/topics/deconflict/cjs_shadowing_renamed_chunk_binding/artifacts.snap | Snapshot validating the correct deconfliction result for the second-order case. |
| crates/rolldown/tests/rolldown/topics/deconflict/cjs_shadowing_renamed_chunk_binding/_test.mjs | Executes the built bundle to ensure no runtime throw for the second-order variant. |
| crates/rolldown/tests/rolldown/topics/deconflict/cjs_shadowing_renamed_chunk_binding/_config.json | Integration test configuration for the second-order fixture. |
hyf0
reviewed
Jun 29, 2026
hyf0
approved these changes
Jun 29, 2026
c04f207 to
239c39f
Compare
Member
Author
Merge activity
|
…dings (#9921) ### What this PR solves Fixes #9882. A CJS-wrapped module's author-local `var` declared inside the generated `__commonJS` closure could shadow a same-named **chunk-root binding** imported from a peer ESM module. Because `var` declarations are hoisted to the top of the closure, the local shadowed the captured outer binding for the whole closure body, so references read the (still-`undefined`) local instead of the initialized import — producing a runtime `TypeError` (e.g. `Cannot read properties of undefined (reading 'EventMatch')`). Example shape: an entry is CJS-wrapped (`typeof exports`/`typeof module` probing), declares `var sharedValue = …`, and also imports `{ SharedEnum }` from a peer ESM module whose hoisted `var sharedValue;` is the chunk-root binding. The local shadows the import. ### Fix Adds a post-deconfliction pass `NestedScopeRenamer::rename_cjs_locals_shadowing_referenced_chunk_bindings` (in `renamer.rs`), run from `rename_shadowing_symbols_in_nested_scopes` **after** all final names are assigned. For `WrapKind::Cjs` modules only, for each *referenced* named import / star-import member-expression it takes the import's final canonical name, looks up the module's root scope, and if a *different* module-owned binding shadows that name it renames **only the shadowing local** (never the chunk-root binding) via `Renamer::override_root_scope_binding`. The override picks the next `$N` suffix skipping both resolver-reserved names and every binding in the module, so it cannot collide with a sibling local (covers the second-order case where the entry already has `sharedValue` *and* `sharedValue$1`). It is order-independent, so it also covers the reverse-ordering case (non-entry CJS module). ### Alternatives explored - **Widen `chunk_scope_captured_names`** to include peer ESM/None chunk-root bindings, and/or reserve names on the non-deconflict path in `renamer.rs`. Both worked but over-approximated, producing harmless-but-noisy snapshot churn across unrelated fixtures. The reference-precise post-pass renames only genuine shadows, with **zero** snapshot churn beyond the new fixtures. ### Notes for reviewers - The `binding != reference` guard is load-bearing: without it an `import * as m` whose member expr is `m.default` would rename `m` onto itself (regression risk for #7444). The existing star/named-import passes use the same guard. - Why a separate pass: the shadow is a module-root-scope binding already named by the main loop, and `register_nested_scope_symbols` bails on already-named symbols — so the existing `rename_bindings_shadowing_named_imports` can't reach it. ### Tests Two regression fixtures, both executing the built bundle via `_test.mjs`: - `crates/rolldown/tests/rolldown/issues/9882/` — the original report. - `crates/rolldown/tests/rolldown/topics/deconflict/cjs_shadowing_renamed_chunk_binding/` — second-order variant (entry already has `sharedValue` + `sharedValue$1`). Full `rolldown` integration suite passes; clippy clean.
239c39f to
6345ddf
Compare
graphite-app Bot
pushed a commit
that referenced
this pull request
Jun 29, 2026
) Pure no-op refactor, split out of the #9882 follow-up work — **stacked on #9921**. Extracts the inline "chunk-scope captured names" collection block out of `deconflict_chunk_symbols` into a dedicated `collect_chunk_scope_captured_names` helper. No behavior change. ### Why The follow-up fix #9970 adds wrapped-ESM namespace-object capture lines to this block, which pushes `deconflict_chunk_symbols` past clippy's `too_many_lines` (200) ceiling. Extracting the helper first keeps #9970 focused purely on the actual fix. ### Changes - Move the captured-names collection into `collect_chunk_scope_captured_names(chunk, link_output, format, &renamer)` — verbatim logic, just relocated. ### Tests No new behavior → no new fixtures. Verified as a no-op: all `topics/deconflict` + `issues/9882` snapshot tests pass unchanged.
IWANABETHATGUY
added a commit
that referenced
this pull request
Jun 29, 2026
…jects A CJS-wrapped module that `require()`s a wrapped-ESM module reads the importee's chunk-root namespace object via `(init_x(), __toCommonJS(xxx_exports))`. An author-local sharing that namespace object's final name shadows the read, producing the self-referential `var xxx_exports = (init_x(), __toCommonJS(xxx_exports))` -> `__toCommonJS(undefined)` -> TypeError at module-eval (issue #9882, require()/namespace channel). Handle this in the precise `rename_cjs_locals_shadowing_referenced_chunk_bindings` post-pass (the same mechanism #9921 uses for named-import and star-member shadowing): for each `require()` of a non-CommonJS importee, rename a root-scope local sharing the importee namespace object''s *final* name. The post-pass runs after root-scope names are assigned, so it sees the namespace''s real name and only touches modules that actually reference it -- renaming the shadowing local, never the namespace. Adds regression fixtures: cjs_shadowing_require_namespace_object (the base case) and cjs_unrelated_wrapper_reserves_namespace_suffix (an unrelated CJS local must not push the namespace onto a suffix shadowed by another module).
IWANABETHATGUY
added a commit
that referenced
this pull request
Jun 29, 2026
…jects A CJS-wrapped module that `require()`s a wrapped-ESM module reads the importee's chunk-root namespace object via `(init_x(), __toCommonJS(xxx_exports))`. An author-local sharing that namespace object's final name shadows the read, producing the self-referential `var xxx_exports = (init_x(), __toCommonJS(xxx_exports))` -> `__toCommonJS(undefined)` -> TypeError at module-eval (issue #9882, require()/namespace channel). Handle this in the precise `rename_cjs_locals_shadowing_referenced_chunk_bindings` post-pass (the same mechanism #9921 uses for named-import and star-member shadowing): for each `require()` of a non-CommonJS importee, rename a root-scope local sharing the importee namespace object''s *final* name. The post-pass runs after root-scope names are assigned, so it sees the namespace''s real name and only touches modules that actually reference it -- renaming the shadowing local, never the namespace. Adds the regression fixture cjs_shadowing_require_namespace_object: it throws `__toCommonJS(undefined)` on the base and passes with the fix.
Merged
shulaoda
added a commit
that referenced
this pull request
Jul 1, 2026
## [1.1.4] - 2026-07-01 ### 🚀 Features - disable `experimental.lazyBarrel` by default (#10071) by @shulaoda ### 🐛 Bug Fixes - dev: disable lazy barrel in dev mode (#10060) by @shulaoda - generate: keep full JSON interface under preserveModules namespa… (#10056) by @IWANABETHATGUY - check finalize_other_specifiers in its own Debug attribute (#10032) by @shulaoda - serialize the KeepAssign unused minify option as "keep_assign" (#10031) by @shulaoda - keep fragments after the newline fragment in MagicString::last_line (#10023) by @shulaoda - generate: undeclared JSON named exports under preserveModules (#10020) (#10027) by @IWANABETHATGUY - deconflict: rename CJS-wrapped locals that shadow chunk-root bindings (#9921) by @IWANABETHATGUY - rolldown: keep entry facade when a shared chunk holds another entry's module (#9997) by @hyf0 - treeshake: also bail JSON default split when the object escapes (#9996) by @IWANABETHATGUY - don't classify await in a strict-mode function as top-level await (#9987) by @shulaoda - avoid spurious leading newline in addon hooks (banner/footer/intro/outro) (#9989) by @shulaoda - handle JSON default mutation bailouts (#9972) by @TheAlexLichter - plugin: make lazy hook metadata enumerable (#9991) by @TheAlexLichter - dev: make init errors in lazy-compiled modules catchable (#9981) by @h-a-n-a - treeshake: keep computed-key side effects on namespace member access (#9986) by @shulaoda - binding: validate replace plugin delimiters length instead of panicking (#9984) by @shulaoda - reconstruct nested rest patterns in into_expression (#9980) by @IWANABETHATGUY - reconstruct rest patterns as spread in into_expression (#9976) by @shulaoda - preserve export keyword on multi-declarator exports under keepNames (#9974) by @shulaoda - deterministically keep the shortest name for deduplicated assets (#9948) by @x1024 - treeshake: apply @__NO_SIDE_EFFECTS__ to cross-chunk namespace calls (#9960) by @IWANABETHATGUY ### 🚜 Refactor - drop redundant program scope enter/leave in finalizer (#10049) by @shulaoda - deconflict: extract collect_chunk_scope_captured_names (#10006) by @IWANABETHATGUY - unify pre-scan multi-declarator split into one decision site (#9982) by @IWANABETHATGUY - common: return bool from SymbolRef::is_not_reassigned (#9962) by @IWANABETHATGUY ### 📚 Documentation - rolldown: remove outdated comment for removing parenthesized expression (#10062) by @Dunqing - use GitHub-flavored alert for Etiquette note in contribution guide (#10012) by @IWANABETHATGUY - replace: explain the delimiters left and right boundaries (#9985) by @shulaoda - ast-mutation: remove stale Address Use section after pre-scan refactor (#9983) by @IWANABETHATGUY - remove fathom (#9968) by @mdong1909 - contribution-guide: code-format main branch references (#9966) by @IWANABETHATGUY - contribution-guide: fix stale REPL note and tidy wording (#9957) by @hyf0 - contribution-guide: clarify when to discuss before opening a PR (#9955) by @hyf0 ### ⚡ Performance - disable preserve_parens across all parse paths (#10057) by @Dunqing - common: inline declared_symbols with SmallVec (#9920) by @IWANABETHATGUY - common: pack TaggedSymbolRef into 8 bytes (#9919) by @IWANABETHATGUY - sourcemap: skip newline scan on the no-sourcemap join fast path (#9936) by @Boshen ### 🧪 Testing - dev: error in lazy module should be catchable (#9975) by @sapphi-red - dev: reject unknown lazy compile modules (#9969) by @sapphi-red ### ⚙️ Miscellaneous Tasks - deps: update actions/cache action to v6 (#10001) by @renovate[bot] - trigger vite ecosystem-ci from PR comments (#10058) by @shulaoda - deps: update napi to v3.10.0 (#10063) by @renovate[bot] - remove unused From impl for RolldownLabelSpan (#10055) by @shulaoda - remove dead Diagnostic::with_kind method (#10054) by @shulaoda - remove unused StatementExt methods (#10053) by @shulaoda - remove unused ExpressionExt methods (#10052) by @shulaoda - remove commented-out re_export_all_names field (#10051) by @shulaoda - deps: update pnpm to v11.9.0 (#10047) by @renovate[bot] - remove the unused BindingGenerateHmrPatchReturn napi type (#10034) by @shulaoda - remove the dead inline_entry_chunk_wrapping scaffolding (#10037) by @shulaoda - deps: bump oxc_resolver to 11.22.0 (#10045) by @Boshen - remove never-constructed MatchImportKind::_Ignore variant (#10041) by @shulaoda - remove the unused ScheduledBuild napi struct (#10033) by @shulaoda - remove dead compute_hmr_update_single method (#10040) by @shulaoda - drop the redundant visited.insert in manual code splitting (#10038) by @shulaoda - remove the dead output_assets vector in render_chunk_to_assets (#10036) by @shulaoda - remove the unused From<String>/Display impls for BindingLogLevel (#10035) by @shulaoda - deps: upgrade oxc to 0.138.0 and migrate to per-type AST construction (#10018) by @shulaoda - deps: update rust crates (#9911) by @renovate[bot] - deps: update test262 submodule for tests (#10016) by @rolldown-guard[bot] - deps: update github actions (#9999) by @renovate[bot] - deps: update npm packages (#10000) by @renovate[bot] ###◀️ Revert - "fix(plugin): make lazy hook metadata enumerable (#9991)" (#10005) by @shulaoda ### ❤️ New Contributors * @x1024 made their first contribution in [#9948](#9948) Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>
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
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 this PR solves
Fixes #9882.
A CJS-wrapped module's author-local
vardeclared inside the generated__commonJSclosure could shadow a same-named chunk-root binding imported from a peer ESM module. Becausevardeclarations are hoisted to the top of the closure, the local shadowed the captured outer binding for the whole closure body, so references read the (still-undefined) local instead of the initialized import — producing a runtimeTypeError(e.g.Cannot read properties of undefined (reading 'EventMatch')).Example shape: an entry is CJS-wrapped (
typeof exports/typeof moduleprobing), declaresvar sharedValue = …, and also imports{ SharedEnum }from a peer ESM module whose hoistedvar sharedValue;is the chunk-root binding. The local shadows the import.Fix
Adds a post-deconfliction pass
NestedScopeRenamer::rename_cjs_locals_shadowing_referenced_chunk_bindings(inrenamer.rs), run fromrename_shadowing_symbols_in_nested_scopesafter all final names are assigned. ForWrapKind::Cjsmodules only, for each referenced named import / star-import member-expression it takes the import's final canonical name, looks up the module's root scope, and if a different module-owned binding shadows that name it renames only the shadowing local (never the chunk-root binding) viaRenamer::override_root_scope_binding.The override picks the next
$Nsuffix skipping both resolver-reserved names and every binding in the module, so it cannot collide with a sibling local (covers the second-order case where the entry already hassharedValueandsharedValue$1). It is order-independent, so it also covers the reverse-ordering case (non-entry CJS module).Alternatives explored
chunk_scope_captured_namesto include peer ESM/None chunk-root bindings, and/or reserve names on the non-deconflict path inrenamer.rs. Both worked but over-approximated, producing harmless-but-noisy snapshot churn across unrelated fixtures. The reference-precise post-pass renames only genuine shadows, with zero snapshot churn beyond the new fixtures.Notes for reviewers
binding != referenceguard is load-bearing: without it animport * as mwhose member expr ism.defaultwould renamemonto itself (regression risk for [Bug]: Duplicate import binding names when external imports share the same local identifier #7444). The existing star/named-import passes use the same guard.register_nested_scope_symbolsbails on already-named symbols — so the existingrename_bindings_shadowing_named_importscan't reach it.Tests
Two regression fixtures, both executing the built bundle via
_test.mjs:crates/rolldown/tests/rolldown/issues/9882/— the original report.crates/rolldown/tests/rolldown/topics/deconflict/cjs_shadowing_renamed_chunk_binding/— second-order variant (entry already hassharedValue+sharedValue$1).Full
rolldownintegration suite passes; clippy clean.