fix: do not rename nested "exports" bindings that do not conflict#6360
Conversation
PR rollup#5947 added "exports" to the universal RESERVED_NAMES set, which caused every variable or parameter named "exports" to be renamed to "exports$1" in any scope, even when no conflict exists with the output format's runtime "exports" identifier. Because eval strings are not rewritten, this silently broke code like: (unused, exports) => { console.log(exports.bar); eval('exports.bar = 1'); } where the parameter was renamed but the eval argument was not. The same over-aggressive rename also applied to unrelated nested bindings in ES output, which has no runtime "exports" at all. This change removes "exports" from RESERVED_NAMES and restores the system-format-specific check in ChildScope.addUsedOutsideNames so that nested scopes in SystemJS output still treat "exports" as used when they reference an outer exported binding. Top-level protection for CJS/AMD/UMD/IIFE/SYSTEM output is unchanged: RESERVED_USED_NAMES in Chunk.ts still seeds the chunk-level usedNames with "exports", and reassigned exports still carry renderBaseName === "exports" which propagates into nested scopes via getBaseVariableName(). The deconflictTopLevelVariables cache from PR rollup#5947 is untouched, so its performance benefits are preserved; a benchmark that reliably sees the PR's generate-time speedup (~20% vs pre-PR) shows no regression from this change. Fixes rollup#6357 Made-with: Cursor
|
@tariqrafique is attempting to deploy a commit to the rollup-js Team on Vercel. A member of the Team first needs to authorize it. |
|
The reason this was done is because with the new caching logic for variable names implemented, you could otherwise have bugs when generating e.g. SystemJS and ES output together from the same output because in SystemJS, it would need to rename |
|
Thanks for taking the time to review and for the context on the caching design — that helps a lot. I dug deeper into this based on your feedback and wanted to share what I found, in case it changes the picture. The However, for The change here restores the format-conditional logic that was in To verify, I tested generating ES + SystemJS from the same bundle, in both orders, with and without cache reuse — all outputs are correct and deterministic, matching 4.52.5 behavior. Would it be helpful if I added a multi-format generation test to the suite to explicitly cover this invariant going forward? |
Ah, you are right. Nested names are not cached, and top-level exports is protected. Then your change is fine. We might consider adding a multi-format test for this scenario, but it would serve mostly for documentation, I do not think we will add caching for nested variables any time soon. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6360 +/- ##
=======================================
Coverage 98.78% 98.78%
=======================================
Files 274 274
Lines 10791 10793 +2
Branches 2881 2882 +1
=======================================
+ Hits 10660 10662 +2
Misses 89 89
Partials 42 42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@lukastaegert thank you for reviewing this. What would it take to meet the criteria for merging this in? |
|
This looks fine form my side, will merge this now |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
Note: I went through many iterations of fixes to this issue. Used cursor with opus 4.7 max thinking to help design and evaluate fixes.
This fix seemed contained and resonable to me. It goes back to the way things mostly were before #5947 for avoiding un-necessary rewrite of
exportsvariables.Additionaly it doesn't impact performance either
Since #5947 was merged for 4.53.0, Rollup renames every nested
exportsbinding — even ones that cannot conflict with the output format's runtimeexportsidentifier. The rename silently breaks code where anexportsname appears inside a string, for example insideeval, because Rollup doesn't rewrite string contents.Reproducer
4.52.5 (expected):
4.53.0 – 4.60.x (current, broken):
The parameter was renamed to
exports$1, but the string argument toevalwas not, so the two stop referring to the same binding. The reporter ran into this while running webpack-produced development bundles through Rollup, where webpack's runtime usesevalto execute module factories whose parameters are literally namedexports.As the reporter notes in #6357, the rename is conceptually wrong even without the
eval: it's an over-deconfliction of a legal nested binding that never needed to change.Root cause
#5947 did two things:
module,exports,require,__filename,__dirname, interop helpers) to a new chunk-levelRESERVED_USED_NAMESseed inChunk.ts, so the top-level deconflict step sees them as already-used.'exports'to the universalRESERVED_NAMESset insrc/utils/RESERVED_NAMES.ts.The first change is fine — it's how the top-level
const exports = 1case has always been protected in CJS/AMD/UMD/IIFE/SYSTEM. The second change is the regression:RESERVED_NAMESis consulted bygetSafeNamefor every scope.ChildScope.deconflictruns it on each nested scope's variables, so any nested binding or parameter namedexportsis forced intoexports$1, regardless of whether the surrounding format even has a runtimeexports(ES doesn't), or whether the scope references anything that actually needs it.#5947 also dropped a pre-existing conditional in
ChildScope.addUsedOutsideNamesthat added'exports'to a scope's used names only when that scope accessed an outer binding that was being re-assigned throughexports("name", value)in SystemJS output. With'exports'now universally reserved, that conditional was no longer strictly needed — but removing it meant that the "I actually do need to avoidexports" decision stopped being driven by the scope's own behavior and started being driven by a global flag.Fix
'exports'fromsrc/utils/RESERVED_NAMES.ts.exportsisn't a JavaScript reserved word; it's only a runtime identifier in certain output formats.ChildScope.addUsedOutsideNamesso that nested scopes in SystemJS output continue to reserveexportsexactly when they reference an outer binding that will be assigned viaexports("name", value)at render time.formatandexportNamesByVariablethrough the one extra call site indeconflictChunk.For CJS/AMD/UMD/IIFE the existing machinery still does the right thing without any extra work: re-assigned exports already carry
renderBaseName === 'exports', andChildScope.addUsedOutsideNamesalready pulls that name into the scope'susedNamesviagetBaseVariableName(). So a nestedexportsbinding in a scope that references such an outer variable still gets renamed toexports$1— and one that doesn't (the reporter's case) stays asexports.The top-level protection in
Chunk.ts(RESERVED_USED_NAMES) and themodule.info.safeVariableNamescache added by #5947 are both untouched.Tests
test/form/samples/no-rename-unrelated-exports-parameter/covers the reporter's exact reproducer across all six formats (amd,cjs,es,iife,system,umd).deconflict-format-specific-exports— this test is literally titled "only deconflict 'exports' for formats where it is necessary"; the expected outputs had drifted away from that invariant for nested scopes in every format. Restored.deconflict-format-specific-globals— same fix for the nested-scope case.supports-es5-shim,supports-es6-shim— the wrapper function of the bundled shim ((function (module, exports) { … }(…))) no longer has itsexportsparameter spuriously renamed.chunking-form/chunk-assigment-in-dynamic— the IIFE(function (exports) { exports.preFaPrint = … }(c))generated by@rollup/plugin-commonjsfor the wrapped module no longer has itsexportsparameter renamed.Top-level rename behaviour that was intentionally unified by #5947 (e.g.
const exports = 1at module top-level being renamed in ES output for caching consistency) is left unchanged.Performance
Because #5947's explicit goal was performance, I validated that this change doesn't regress it. I built three versions of Rollup and ran 60 iterations (2 runs × 30, alternated to control for drift) of a 500-module synthetic fixture that exercises the deconflict hot path, generating all six output formats per iteration:
The benchmark is demonstrably sensitive enough to detect the original #5947 speedup (|t| ≈ 30–37 on generate-time metrics — well above the 95% confidence threshold of 2). Times in ms, mean ± standard error:
Per-format generate time:
Negative = faster. #5947's speedup (−20% generate, −24% to −27% per non-ES format) is fully preserved. This PR is additionally 3.9% faster on the generate phase (t=6.4), because (a)
RESERVED_NAMES.has()has one fewer element to check on everygetSafeNamecall, and (b) nested bindings that used to be spuriously renamed no longer go throughgetSafeName+ MagicString rewrite.On the ES format, #5947 actually made generate time slightly slower (+4.7%) by forcing ES builds to treat
exports/module/require/__filename/__dirnameas reserved at the top level even though ES has no runtime for them; this PR brings ES back to essentially pre-#5947 levels (40.7ms → 39.0ms) by lifting the unnecessary nested-scope reservation.