Hoist the lowering of import ... from "bun" above the module body at runtime - #39206
Hoist the lowering of import ... from "bun" above the module body at runtime#39206robobun wants to merge 1 commit into
import ... from "bun" above the module body at runtime#39206Conversation
…body At runtime, `import ... from "bun"` is printed as `var` declarations reading globalThis.Bun, at the position of the import statement. Unlike the import statement it replaces, those declarations are not hoisted, so a binding used above the import was undefined when the module ran. print_ast now prints these statements before the rest of the module. The runtime transpiler cache version is bumped so cached output printed with the old layout is not reused.
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 3:05 PM PT - Aug 15th, 2026
✅ @robobun, your commit 5ee183a7c29faf1246ba71a78b031283ae800ac8 passed in 🧪 To try this PR locally: bunx bun-pr 39206That installs a local version of the PR into your bun-39206 --bun |
|
Status: fix pushed, self-review done, waiting on CI and a maintainer. Reproduced with bun 1.4.0 and a debug build of main: a file that uses a binding above its Self-review outcome: the fix stands as the change that does not alter what the import evaluates to. The larger option (stop lowering the statement at runtime and let the loader link |
There was a problem hiding this comment.
LGTM — small, well-guarded printer fix with matching cache-version bump and thorough test coverage.
What was reviewed:
print_asthoist loop and skip in the main loop use the sameis_global_bun_import_statementpredicate, so no statement is duplicated or dropped; the predicate matches exactly what the existingSImportarm routes toprint_global_bun_import_statement(both checkrecord.tag == ImportRecordTag::BununderIS_BUN_PLATFORM).has_global_bun_import_statementgates the extra pass onImportKind::Stmt, so files without a"bun"import statement take the same single loop as before;require("bun")/import("bun")records don't trigger it.- Hoisted statements go through
print_stmt, soadd_source_mapping(stmt.loc)and module-info recording still happen — only output position moves; the new test asserts a stack-trace line still maps back to source line 2. EXPECTED_VERSIONbumped 25→26 so cached output with the old layout is invalidated.
Extended reasoning...
Overview
This PR fixes import { x } from "bun" bindings being undefined when read by code written above the import statement. The runtime transpiler lowers such imports to var {...} = globalThis.Bun declarations printed at the statement's source position, which — unlike the import they replace — are not hoisted by the engine. The fix makes print_ast (the runtime transpiler's printing entry point) emit these declarations ahead of the module body, then skip them in the main statement loop. Three files touched: ~40 lines in src/js_printer/lib.rs (two new predicates + a guarded pre-pass in print_ast), a one-line cache-version bump in src/jsc/RuntimeTranspilerCache.rs, and a new test block in test/bundler/transpiler/runtime-transpiler.test.ts.
Security risks
None. The change reorders output of statements the printer already emits verbatim; no new parsing, no untrusted input handling, no allocation or FFI changes. The hoisted declarations read globalThis.Bun, which is available before any user module runs.
Level of scrutiny
Medium. print_ast runs for every module the runtime loads, so a mistake here would be broadly visible — but the change is guarded: has_global_bun_import_statement() scans the (short) import-records list and returns false for any file without an ImportRecordTag::Bun + ImportKind::Stmt record, in which case the main loop is byte-identical to before (the hoist_bun_imports && ... short-circuits). For files that do have such an import, the hoist pass and the skip in the main loop use the identical is_global_bun_import_statement predicate, which matches exactly what the existing StmtData::SImport arm (lib.rs:5731-5736) already dispatches to print_global_bun_import_statement. There is no way for a statement to be printed twice or dropped.
Other factors
- Placement is right: the hoist runs after the existing
var {require}=import.meta;prelude and before the parts loop, matching where a real import would be hoisted. Sinceprint_stmtis used for the hoisted call,print_indent/add_source_mapping/prev_stmt_tagbookkeeping is preserved — the test verifies source-map line numbers still resolve correctly after reordering. - Other
print_astcallers (Bun.Transpilerinsrc/bundler/transpiler.rs, the REPL) either don't tag records withImportRecordTag::Bun(no linking) or want the same hoisting semantics; theIS_BUN_PLATFORMconst-generic (bound toASCII_ONLYforprint_ast, lib.rs:7498) gates the whole thing off for non-bun-target printing. - Cache version correctly bumped with a comment following the file's existing convention.
- Tests cover named/default/namespace imports, TS
import typeand inlinetypespecifiers, interleaving with a realnode:pathimport andrequire(), both-eand file entry (.mjs/.ts), and assert source-mapped stack-trace line numbers. They usebunRun/tempDirfrom harness,test.concurrent, and check stderr/stdout before exit code per repo conventions. - The PR description is unusually thorough about mechanism, alternatives, and what was manually verified; the bug-hunting system found nothing.
Problem
import { x } from "bun"isundefinedin any code written above the import statement.import { readFileSync } from "fs"in the same position works, so only the"bun"specifier is affected.typeof:TypeError: escapeHTML is not a function. (In 'escapeHTML("<a>")', 'escapeHTML' is undefined).bun file.mjs,.ts,.cjs,bun -eandbun testfiles alike.bun build --target=bunis not affected."bun"as a module. The printer replaces the statement withvardeclarations (src/js_printer/lib.rs,SImportarm ofprint_stmtcallingprint_global_bun_import_statement), and it prints them where the statement was written. The parser only moves import statements to the front of the file when bundling (src/js_parser/parse/parse_entry.rs,SImportcase in_parse); at runtime it keeps them in place because real import statements are hoisted by the engine anyway. Avarinitialized at the statement's position is not, so the binding exists but is stillundefinedwhen earlier statements run. Transpiled output before this change:Fix
print_ast(the runtime transpiler's printing entry point) now prints the"bun"import statements of a file before the rest of the module, then skips them while printing the parts in order. The output for the example above starts withvar {color } = globalThis.Bun;. Files without such an import are detected from the import records and take the single loop they took before.print_astis only used for the non-bundled path, which is the only path that leaves imports in place. The bundler (bun build --target=bun) already emits these declarations at the top of the module, and the Bake dev server likewise binds builtin imports at the top of its module wrapper, so this makes the runtime agree with them. The import records are tagged by two different runtime paths (Linker::linkandRuntimeTranspilerStore), and both print throughprint_ast, so both are covered by one change. Printing through the existingSImportarm keeps the output text, source mappings and module record data identical to before; only the position moves.globalThis.Bun, which exists before any user module runs, and an ES module's imported modules are all evaluated before its own body starts, so the front of the body is the earliest point in the module and still after every dependency. This is also the position the statement would have been hoisted to had it stayed an import. The one observable difference besides the fix is a module that assigns toBun.*and only then, further down, imports that name from"bun": it now gets the value from before the assignment, which is whatbun buildand a real import give it too.src/jsc/RuntimeTranspilerCache.rs, 25 to 26) because cached output for files with a late"bun"import would otherwise keep the old layout after upgrading. js_printer: fix require("bun") and other printer literals being captured by same-named locals #35739, Stop rewriting a literal import("bun") to Promise.resolve(globalThis.Bun) #37730 and bun:test: hoist jest.mock/vi.mock/mock.module above imports #36297 also bump this constant, so whichever of these lands later needs a one-line rebase there."bun"module), and the same lowering also still differs from a real import in an import cycle (an exported function called before this module's body has run sees the binding asundefined;bun buildshares that limitation), formock.module("bun", ...), and forimport { default as B } from "bun"(bindsundefined, runtime and bundler alike; reported separately). The alternative that fixes all of those at once is to stop lowering the statement at runtime (keep theSImportrewrite only whenoptions.bundling) and let the loader link the existing native module; it is about as small as this change and would delete it, and the tests here are written against the imported values rather than the lowering, so they carry over unchanged. It is not done here because it is user visible in two ways this change is not:import * as ns from "bun"becomes a module namespace (ns !== Bun) and importing a name theBunobject does not have becomes a link-timeSyntaxErrorinstead ofundefined. That is the same trade Stop rewriting a literal import("bun") to Promise.resolve(globalThis.Bun) #37730 proposes forimport("bun"); if that direction is taken for static imports too, this PR becomes unnecessary. The bug itself was found while working on something else, not from a user report; none of the roughly 500 files in this repository that import from"bun"uses a binding above the statement, so this is a correctness fix rather than a reported breakage.test/bundler/transpiler/runtime-transpiler.test.ts, newimport ... from "bun"block. Three cases (bun -e,entry.mjs,entry.ts) use named, default and namespace imports above their import statements, interleaved with anode:pathimport, arequire()call (which shares the hoisting spot with thevar {require}=import.meta;declaration) and, for TypeScript,import typeand inlinetypespecifiers. The file cases also check that a stack trace line number still maps to the source once the output no longer lines up with it. All three fail on the released bun with theTypeErrorabove and pass with this change.runtime-transpiler.test.ts,test/js/bun/resolve/import-meta.test.js,test/js/bun/resolve/builtin-esm-lazy-exports.test.ts,test/cli/run/transpiler-cache.test.ts,test/cli/run/run-eval.test.ts,test/bundler/bundler_bun.test.ts,test/cli/inspect/debugger-buntranspiledmodule.test.ts(bun test --isolate, which is the path that records module info while printing),test/cli/inspect/BunFrontendDevServer.test.ts. Manually checked the printed output for a multi-line import,import "bun",import B, { x } from "bun",import * as ns from "bun", a file that also usesrequire, a.cjsfile, a file with a top-levelusing, andexport { x } from "bun"(a real re-export, untouched).Background
"bun"imports at runtime: when the runtime transpiler resolves a file's imports, the specifier"bun"is matched against the builtin alias table and its import record is taggedImportRecordTag::Bun. The printer turns such a statement intovardeclarations readingglobalThis.Bun(var {x} = globalThis.Bun,var B = globalThis.Bun) instead of printing an import, so the module loader never sees the specifier.require("bun")andimport("bun")are inlined the same way as expressions and are not affected by this PR.varis different: its name exists from the start but it only gets its value when execution reaches the declaration.print_astprints them in that order. When bundling, the parser moves import parts to the front, which is why the bundler never had this problem.Probes
Transpiled output on this branch for a file combining the shapes (debug build source dump):
(
export class Foo {}is moved up by the parser independently of this change; the"use strict"directive the file started with is dropped by the parser for ES modules, also independently of this change.)