js_printer: fix require("bun") and other printer literals being captured by same-named locals - #35739
js_printer: fix require("bun") and other printer literals being captured by same-named locals#35739robobun wants to merge 10 commits into
Conversation
A local `let globalThis` would shadow the literal `globalThis.Bun` that
the printer emitted for require('bun') / import('bun') / import ... from 'bun'.
Runtime path: the single-file transpiler uses NoOpRenamer, so no renaming
can protect the literal. Gate the inline on `options.bundling` and let the
import fall through to require('bun') / import('bun'), which the module
loader resolves to the Bun object.
Bundler path: reserve `globalThis` in compute_initial_reserved_names so the
NumberRenamer renames a user local away before the literal is emitted.
Fixes #8058
|
Updated 8:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit c96c43c has some failures in 🧪 To try this PR locally: bunx bun-pr 35739That installs a local version of the PR into your bun-35739 --bun |
WalkthroughBun import and require rewrites now apply only during bundling, ChangesBun import resolution
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/regression/issue/08058.test.ts`:
- Around line 2-6: Remove the explanatory header comment at
test/regression/issue/08058.test.ts lines 2-6, retaining only the issue URL on
line 1; also remove the explanatory comment at lines 102-102 so the regression
test contains exactly the issue URL comment.
- Line 10: Move the `itBundled` test cases currently in
`test/regression/issue/08058.test.ts` (lines 85–122) into a test file under
`test/bundler/`, preserving their assertions and required imports so `itBundled`
accepts the callers and runs the bundled tests.
- Around line 85-86: Add bundled execution coverage in the `bundler` tests for
the changed `SImport` path: add `import * as B from "bun"` cases where
`globalThis` is shadowed, and run each case in both ESM and CJS output modes.
Follow the existing `itBundled` patterns in
`test/regression/issue/08058.test.ts` and retain assertions verifying successful
bundled behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 09a8b673-27a5-4225-b57a-ca2037dea9f2
📒 Files selected for processing (5)
src/js_printer/lib.rssrc/js_printer/renamer.rssrc/jsc/RuntimeTranspilerCache.rstest/js/bun/resolve/import-meta.test.jstest/regression/issue/08058.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…at runtime
Emitting a real ESM import for the statement form (a) eagerly reifies every
property on the Bun object via generateNativeModule_BunObject, and (b) fails
link-time validation for names that exist only as types in bun.d.ts (e.g.
ShellError). Limit the runtime change to require('bun') / import('bun').
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js_printer/lib.rs:5772-5779— The PR description lists the runtime static-import TDZ case (import * as B from "bun"; let globalThis = {}) as one of the two #8058 failure modes being fixed, but commit 7ed11a5 deliberately kept this lowering ungated at runtime — so that case still throwsReferenceError: Cannot access 'globalThis' before initialization, and the runtimedescribe()block has no test for it. Since the exclusion is intentional (reification / link-time-validation, per the new comment), please update the description to note it and either dropFixes #8058or add a note on the issue about the remaining limitation.Extended reasoning...
What's mismatched
The PR description's Problem section lists two manifestations of #8058:
{ let globalThis = { Bun: "x" }; console.log(require("bun")) }→ printsximport * as B from "bun"; let globalThis = {};→ReferenceError: Cannot access 'globalThis' before initialization(TDZ)
The Fix section says the runtime path now lets
import ... from "bun"fall through to the normal external path, and the Tests section says all six cases (require / dynamic import / static import × runtime / bundler) fail onmainand pass with this change. But the shipped code fixes case (1) at runtime and case (2) only in the bundler — case (2) at runtime is deliberately left as-is.The code path
print_require_or_import_exprat lib.rs:2435 is now gated onself.options.bundling, so runtimerequire("bun")/import("bun")fall through to the real module loader. ✅- The
SImporthandler at lib.rs:5772 is not gated onbundling— the new comment explicitly says so ("Unlike print_require_or_import_expr this is not gated onbundling"), and commit 7ed11a5 ("keep the var-destructure lowering for staticimport ... from 'bun'at runtime") reverted an earlier attempt to gate it. print_global_bun_import_statement(lib.rs:1718–1722) callsprint_internal_bun_import(import, Some(b"globalThis.Bun")), which for a star import emitsvar B = globalThis.Bun;(lib.rs:1734–1751).- The runtime transpile path uses
NoOpRenamer(renamer.rs — "only constructed byprint_ast/print_common_js"), so no user binding is renamed. The newcompute_initial_reserved_namesentry forglobalThisonly helps the bundler'sNumberRenamer.
The new comment at :5772 justifies keeping the lowering ("eagerly reify every property on the Bun object" / "fail link-time validation for names that exist only as types") and says "When bundling, the renamer has already renamed any user
globalThisaway" — but is silent on what happens when not bundling, which is exactly where the TDZ case lives.Step-by-step proof
Given
entry.ts:import * as B from "bun"; let globalThis = {}; console.log(typeof B.serve);
- Runtime transpiler hits
Stmt::Import→SImportarm at lib.rs:5772.IS_BUN_PLATFORMis true,record.tag == ImportRecordTag::Bunis true, nooptions.bundlinggate →print_global_bun_import_statement(s). print_internal_bun_importsees!import.star_name_loc.is_empty()→ emitsvar B = globalThis.Bun;.let globalThis = {}prints unchanged (NoOpRenamer::name_for_symbolreturnsoriginal_name).- Emitted module:
var B = globalThis.Bun; let globalThis = {}; console.log(typeof B.serve); - At execution, the module-scope
let globalThisis hoisted into TDZ; line 1'sglobalThis.Bunreads it before initialization →ReferenceError: Cannot access 'globalThis' before initialization.
This is byte-identical to the pre-PR behavior for this specific input.
Test coverage gap
test/regression/issue/08058.test.ts'sdescribe("runtime transpiler")block has four tests:require('bun')shadowed, dynamicimport('bun')shadowed,.cjsrequire shadowed, and a reification assertion (which asserts the lowering is kept, not that shadowing works). There is no runtime test forimport * as B from "bun"with a module-levellet globalThis. The bundlerdescribe()covers it via the reserved-name fix, but that path usesNumberRenamer. So the description's "All six cases fail onmainand pass with this change" over-counts — the runtime static-import case is neither fixed nor tested.Why this is a nit
- No regression: the runtime static-import output for this input is unchanged from before the PR; nothing that worked now breaks.
- Intentional and documented in code: commit 7ed11a5 explicitly reverted the gate, and the code comment states the trade-off (eager reification of the whole
Bunobject; link-time validation failure for type-only names inbun.d.ts). Those are real concerns. - Pathological input: a module-top-level
let globalThisco-located withimport ... from "bun"is vanishingly rare.
Per REVIEW.md ("Fix the whole class in the same PR … If a site is intentionally excluded, say so in the PR"), the ask is just to make the exclusion visible: update the PR description so it doesn't claim the runtime static-import TDZ case is fixed, and either drop
Fixes #8058or leave a note on the issue about the remaining limitation. Optionally, add a runtime test that documents the current behavior so the gap is explicit rather than silent.
|
Re the runtime static-import TDZ note: the PR body was updated before that comment landed. The Problem section no longer lists the TDZ case, the Fix section explicitly says the static |
…me; move tests to module files
Runtime static imports now destructure from `import.meta.require("bun")`
instead of the bare `globalThis.Bun` literal, closing the module-scope
`let globalThis` shadow case without going through a real ESM import
(which would eagerly reify every Bun property and reject type-only names).
Tests moved out of test/regression/issue/: this was never a regression
(the rewrite has had this bug since it was introduced). Runtime cases go
in import-meta.test.js next to the existing require('bun') tests; bundler
cases go in bundler_bun.test.ts.
|
CI status: the diff itself is green where it ran (Lint JavaScript and Source lints pass; local |
…uire-bun-shadowed-globalthis Transpiler cache version bumped to 26: main took 24 and 25 in the meantime.
…ls (from #35575) Folds in #35575: those identifiers are printed as raw text too (inlined require errors, number printing, synthesized undefined), so a local with one of these names captured them the same way a local globalThis captured the require("bun") rewrite. Carries over that PR's bundler tests.
…un" import lowering
robobun
changed the title
There was a problem hiding this comment.
I reviewed this PR (including the folded-in #35575 reserved-name additions and the post-merge cache-version bump to 26) and didn't find any bugs. Because it changes runtime transpiler output for require('bun')/import('bun') and shifts the observable shape of await import('bun'), a maintainer sign-off would still be worthwhile.
What was reviewed:
- The
options.bundlinggate inprint_require_or_import_exprand theimport.meta.require("bun")lowering inprint_global_bun_import_statement; thecontains_import_metaflag write matches the existing pattern at the other three sites. - The five new
EXTRASreserved names —ensure_total_capacityalready accounts forEXTRAS.len(), so no capacity under-reservation. - Cache version 26 is unique after the main merge (24/25 were taken).
- The two open comment-cop flags on renamer.rs/RuntimeTranspilerCache.rs are re-fires of content the author already justified on 07-25.
Extended reasoning...
Overview
The PR stops the runtime transpiler from rewriting require('bun') / import('bun') to the raw literal globalThis.Bun (which a user let globalThis could shadow — #8058). The runtime now falls through to the real module loader for the expression forms, and lowers static import ... from 'bun' via import.meta.require('bun'). On the bundler side (which still emits globalThis.Bun), globalThis plus Error/Infinity/NaN/undefined (folded in from #35575) are added to compute_initial_reserved_names so user locals with those names get renamed away. The transpiler cache version is bumped to 26. Tests cover the runtime forms (import-meta.test.js, describe.concurrent) and the bundler forms (bundler_bun.test.ts, both minify variants and each new reserved name).
Security risks
None identified. This is a code-generation correctness fix in the printer/renamer; no auth, crypto, filesystem, or untrusted-input parsing is touched.
Level of scrutiny
High. src/js_printer/lib.rs is on the critical path for every transpiled module, and the change alters user-observable behavior: await import('bun') now returns the module namespace object (ns.default === Bun) rather than Bun itself. The PR body documents this and the resulting runtime↔bundler divergence (bundler output is byte-identical to before), with #37730 tracking the bundler-side alignment. That is a design decision a maintainer should confirm rather than an automated approval.
Other factors
- My two prior rounds of feedback (namespace-shape divergence note;
describe.concurrentfor the subprocess suite) were both addressed. - Since my last look, #35575 was folded in and
mainwas merged; I checked the merged reserved-name list, the correspondingitBundledcases for Error/Infinity/NaN/undefined, and that the capacity reservation incompute_initial_reserved_namesstill usesEXTRAS.len()so it stays correct at 7 entries. - The
contains_import_meta = truewrite in the new runtime branch mirrors the three existing sites in the same file. - The updated
'import("bun") works'test now assertsns.default === Bunand consistency withimport(eval("'bun'")), locking in the new namespace shape. - The two unresolved comment-cop bot comments dated today are re-fires on the same two-line comment / changelog entry the author already justified on 2026-07-25; nothing new to act on there.
server.ts does `import { serve } from "bun"`, which the runtime now lowers
to a require("bun"); that registers "bun" in require.cache the same way a
non-literal require("bun") always has.
There was a problem hiding this comment.
I re-reviewed after the Aug 13 push (folding in #35575's reserved names, the builtin-esm-lazy-exports comment updates, and the BunFrontendDevServer module-graph snapshot) and found no bugs; my earlier notes on the namespace-shape divergence and describe.concurrent are addressed. A human sign-off is still worthwhile for the user-visible runtime change: await import("bun") now yields the namespace object (ns.default === Bun, ns !== Bun) instead of Bun itself, and "bun" now appears in require.cache.
Checked: the contains_import_meta flag write matches the three existing call sites; import.meta.require("bun") is only reached on the ESM SImport path so import.meta is always valid; the cache version bump to 26 is contiguous after the merge; the extended EXTRAS list is covered by the new itBundled cases for Error/Infinity/NaN/undefined.
Extended reasoning...
Overview
The PR fixes #8058 by (a) gating the printer's globalThis.Bun rewrite on options.bundling so the runtime transpiler falls through to real require/import for "bun", with the static import ... from "bun" form now sourcing from import.meta.require("bun"); and (b) seeding the bundler's reserved-name set with globalThis, Error, Infinity, NaN, undefined so user locals with those names get renamed away before the printer emits them as raw text. The transpiler cache version is bumped to 26. Tests cover both runtime and bundler paths across require/dynamic-import/static-import × ESM/CJS × minified/unminified, plus the four folded-in printer literals from #35575. Two existing test files are updated for the observable side effects ("bun" in the inspector module graph's cjs list; the import("bun") shape assertion).
Security risks
None. No untrusted input handling, auth, or network surface changes.
Level of scrutiny
Moderate-to-high. The printer and renamer are critical hot paths, and the runtime-side change alters user-observable semantics: await import("bun") now returns the module namespace object rather than Bun directly, and a literal require("bun") / import ... from "bun" now populates require.cache["bun"]. The PR body documents both, the bundler output is byte-identical to before, and the runtime side is now consistent with the non-literal-specifier path — but code that did (await import("bun")) === Bun or relied on .default being absent will observe the difference. That's a maintainer-level call, not something an automated review should sign off on alone.
Other factors
My two prior inline notes (the runtime↔bundler namespace-shape divergence, and describe.concurrent for the subprocess suite) were both addressed. The comment-cop bot's flags on this push were dismissed with valid rationale (changelog entry format; two-line invariant doc). The contains_import_meta = true write follows the exact pattern of the three existing sites at lib.rs:2990/3020/7516. Test coverage is thorough, including a guard test that import { env } from "bun" still doesn't eagerly reify the whole Bun object. The PR description also flags an expected rebase interaction with #37730 on the same block.
2 participants
Fixes #8058.
What does this PR do?
The printer rewrites
require("bun")/import("bun")/import ... from "bun"to the literal textglobalThis.Bun(andPromise.resolve(globalThis.Bun)for the dynamic case). A user binding namedglobalThisin scope at the call site shadows that literal:print_require_or_import_exprandprint_global_bun_import_statementemitglobalThis.Bunas raw text with noRef, so it never participates in renaming. In the bundler a renamer runs, butglobalThiswas not in its reserved-name seed, so a user'slet globalThiswas left in place. In the runtime transpiler that path usesNoOpRenamer, so reserving the name would not help there.Two changes, one for each path:
globalThis.Bunliteral is no longer emitted.require("bun")andimport("bun")fall through to the normal external paths (the module loader resolves"bun"to the Bun object, sorequire("bun") === Bunstill holds). Staticimport ... from "bun"keeps its var-destructure lowering but now sources fromimport.meta.require("bun"), which is shadow-proof (import.metais syntax) and keeps the lazy property access that the destructure provides; emitting a real ESM import instead would eagerly reify every Bun property and reject type-only names such asShellErrorat link time.globalThistocompute_initial_reserved_namesso theNumberRenamerrenames a user'slet globalThisaway beforeglobalThis.Bunis emitted. bundler: seed reserved names with globalThis/Error/Infinity/NaN so locals can't capture printer literals #35575 made the same change and also reservedError,Infinity,NaNandundefined, which the printer emits as raw text in the same way (inlined require errors, number printing, synthesizedundefined); that PR is now folded in here and closed, so the seed reserves all of them.The transpiler cache version is bumped so cached entries containing the old
globalThis.Buntext are invalidated.await import("bun")now returns the module namespace object (ns.default === Bun, everyBun.*property re-exported) instead ofBunitself. This matches whatawait import(spec)already returned for a non-literalspec.require("bun"),import Bun from "bun", andimport * as B from "bun"still yieldBundirectly as before. The bundler still inlinesglobalThis.Bunfor the dynamic-import case (its output is unchanged); aligning that with the runtime namespace shape is left for a follow-up. (#37730 does that and touches the sameprint_require_or_import_exprblock and cache version; whichever of the two lands second needs a small rebase.)One more observable side effect of the runtime lowering: a literal
require("bun")orimport ... from "bun"now actually evaluates arequire("bun"), so"bun"appears inrequire.cache(previously only a non-literalrequire(spec)withspec === "bun"did that; the printer rewrite never touched the loader).test/cli/inspect/BunFrontendDevServer.test.ts's module-graph snapshot is updated for this, since itsserver.tsfixture importsservefrom"bun".How did you verify your code works?
New tests in
test/js/bun/resolve/import-meta.test.jscoverrequire("bun"), dynamicimport("bun"), staticimport * as B from "bun", and the CommonJS form, each with a shadowinglet globalThis(andlet Promisefor the dynamic case). A guard test asserts thatimport { env } from "bun"still does not eagerly reify the whole Bun object. NewitBundledcases intest/bundler/bundler_bun.test.tscover the ESM and CJS bundler output with shadowedglobalThisfor all three forms. TheitBundledcases from #35575 are carried over as well: shadowedglobalThiswith and without--minify-identifiers, plus shadowedError(inlined require error),Infinity(1e400),NaN(folded0/0) andundefined(synthesizedimport.meta.hot).On current
main, 14 of the tests across the two files fail (the 3Minifiedvariants and the reification guard pass on both sides and only guard the behavior); all 59 pass with this branch. The transpiler cache version is 26 after mergingmain, which took 24 and 25 in the meantime.BunObject.test.ts, thebun:shellsuite, and the existingimport-meta/bundler_bun/bundler_minifytests continue to pass.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file