transpiler: stop minifying new Error() (and other known-global constructs) into calls that JSC tail-calls - #37388
transpiler: stop minifying new Error() (and other known-global constructs) into calls that JSC tail-calls#37388robobun wants to merge 7 commits into
new Error() (and other known-global constructs) into calls that JSC tail-calls#37388Conversation
…ays in the stack With minify_syntax on (always the case for the runtime transpiler, and for bun build --minify) `new Error(...)` and the other seven native error constructors were rewritten to plain calls. Modules are strict mode code and JSC implements proper tail calls there, so `return Error(...)` pops the calling function's frame before the error captures its stack. Any `return new Error(...)` helper, including `const e = new Error(); return e` after the single-use binding is inlined, produced a stack without the function that created the error. Constructs are never tail calls, so keep the `new`. Error positions in code frames and call sites move back from the constructor name to the `new` keyword, which is where V8 reports them; the affected snapshots are updated accordingly.
|
Updated 3:30 AM PT - Aug 11th, 2026
✅ @robobun, your commit 5d87dd89675932e3312e1b67d8161a0332ee36eb passed in 🧪 To try this PR locally: bunx bun-pr 37388That installs a local version of the PR into your bun-37388 --bun |
|
Status: reproduced on bun 1.3.14, 1.4.0 and a debug build of main with the snippet in the description ( |
WalkthroughThe minifier preserves ChangesError stack preservation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
…s from the calling frame `return Function(...)` is the same tail call as `return Error(...)`: the Function constructor then takes the source origin for the new body from the caller's caller, so an import() inside the body resolves relative to the wrong file.
robobun
changed the title
new on Error constructors so the creating function stays in the stack tracenew on Error and Function constructors instead of minifying them into tail calls
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bundler/transpiler/runtime-transpiler.test.ts`:
- Line 276: Resolve main.mjs against the existing dir path before constructing
the Bun.spawn command, and use that absolute path in cmd instead of the relative
"main.mjs" entry. Preserve the current working-directory behavior and the rest
of the spawn configuration.
🪄 Autofix
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: 885a989c-bf00-4f97-a02b-2f0ccfa0aadb
📒 Files selected for processing (3)
src/ast/known_global.rstest/bundler/bundler_minify.test.tstest/bundler/transpiler/runtime-transpiler.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
I was working on the same bug from a separate report and ended up with the same change to The runtime transpiler cache version needs a bump. Small extra data point in case it is useful for the tests: the same fixture run as |
There was a problem hiding this comment.
The serve.test.ts column assertion I flagged earlier is fixed in fdfae0b; this run found no further issues. The source change is small and the reasoning (JSC proper tail calls erasing the creating frame; new Function source origin) checks out, but since it reverses #22493's optimization and shifts reported error columns for every bun run/bun build user, a maintainer sign-off on the tradeoff would be worthwhile.
What was reviewed:
known_global.rs: Error-family +Functionnow returnNone;call_from_newstill live viaObject/Arrayarms.- Column/snapshot updates: spot-checked the −4 delta in inspect-error, hot.test.ts, serve.test.ts, test-test;
node:util.inspectsnapshot correctly keeps col 19 (.stackformatter reports constructor name, notnew). normalizeErrorregex rewrite: verified it matches the newat require (51:24)debug-frame shape without stripping real[dir]/...:N:Nframes.
Extended reasoning...
Overview
One source file changed (src/ast/known_global.rs): the 8 native Error constructors and Function are moved from the "strip new" arm to a None-returning arm, matching the existing RegExp treatment. The other 12 files are test updates: three add new coverage (stack.test.ts, bundler_minify.test.ts, runtime-transpiler.test.ts), the rest are mechanical column/caret snapshot shifts (−4) that follow from new no longer being stripped, plus the serve.test.ts fix from my earlier comment and a rewrite of the debug-frame filter in inspect-error.test.js.
Security risks
None. This is a minifier correctness fix; no untrusted input handling, no auth/crypto/permissions.
Level of scrutiny
Medium-high. The Rust change is trivially safe (returns None = keep the AST node as-is), but it runs on every module the runtime transpiler touches and changes bun build --minify output size. It also reverses an optimization introduced in #22493 — the PR makes a strong case (semantics-preserving is non-negotiable; esbuild agrees; ~360 bytes on a 222KB bundle), but a maintainer should confirm the tradeoff rather than have it auto-approved.
Other factors
- All prior review feedback resolved: comment-cop's long-comment complaint (trimmed in 48434e6), CodeRabbit's path suggestion (declined with justification, withdrawn), and my serve.test.ts CI-failure finding (fixed in fdfae0b).
call_from_newis not dead code — still used by theObjectandArrayarms.- New tests cover all 8 error constructors, the inlined-const shape, arrow bodies, bundler output captures, runtime stack frames, and the
new Functionimport() origin. Coverage looks complete for the bug class. - I did not independently re-derive the
bundler_npm.test.tssourcemap coordinate/filesize updates or theerr-stack-fixture.jsoriginalColumn18→22 shift; those are exact-match assertions CI will validate.
|
Heads up on an interaction with #37396, which makes the |
…the transpiler cache version The remaining call rewrites (`new Object(x)` and `new Array(x)` with a non-literal or length argument) have the same problem as the error constructors: `return Array(...lengths)` is a varargs tail call, so the RangeError thrown for an invalid length has no frame for the function that asked for the array. Only the literal foldings remain, which makes the rule a single sentence. Cached transpiler output written by earlier versions still holds the call form, so the runtime transpiler cache version moves to 26.
robobun
changed the title
new on Error and Function constructors instead of minifying them into tail callsnew Error() (and other known-global constructs) into calls that JSC tail-calls
There was a problem hiding this comment.
I reviewed this PR after the serve.test.ts fix (fdfae0b) and cache-version bump (967a6e8) landed and didn't find further bugs. Because it changes runtime-transpiler output for every module and shifts the reported column of every new Error(...) code frame, a human look would still be worthwhile before merge.
What was reviewed:
minify_global_constructor: every formercall_from_newsite now returnsNone; the literal-folding arms forObject/Arrayare unchanged, and the deleted helper has no remaining callers.- Column/snapshot updates: spot-checked the −4 shift in hot.test.ts, serve.test.ts, test-test.test.ts, test-error-code-done-callback.test.ts and inspect-error.test.js against their fixtures.
- New tests in stack.test.ts / bundler_minify.test.ts / runtime-transpiler.test.ts cover the Error family, the
new Array(n)throw path, andnew Functionsource-origin resolution respectively. - The two open comment-cop flags (known_global.rs:95, RuntimeTranspilerCache.rs:56) look like false positives — both 3-line comments match the established style of their neighbors (the RegExp note and the Version 22–25 log entries).
Extended reasoning...
Overview
The PR stops KnownGlobal::minify_global_constructor from rewriting new X(...) into a plain call X(...) for the eight native error constructors, Function, and the non-foldable Object/Array cases. The call_from_new helper is deleted and every site that used it now returns None, leaving the new expression as written. The literal-folding paths (new Object() → {}, new Array(1,2,3) → [1,2,3], small new Array(n) → [,,,]) are untouched. EXPECTED_VERSION in RuntimeTranspilerCache.rs is bumped 25→26 so cached .pile entries holding the old call form are invalidated. Twelve test files carry the resulting −4 column shift in code-frame snapshots plus new coverage for the tail-call mechanism.
Security risks
None identified. The change removes a byte-saving rewrite; output becomes closer to the input source. No parsing of untrusted data, no new syscalls, no auth/permission surface.
Level of scrutiny
High. minify_global_constructor runs on every new Ident(...) expression in every file the runtime transpiler and bun build --minify process, and the reported error column is user-visible in stack traces, code frames, the dev error page, and sourcemap-consuming tooling. The Rust diff itself is small and strictly de-optimizing (fewer transforms applied), which limits the blast radius, but the breadth of affected snapshot assertions across the suite and the noted interaction with #37396 make this worth a maintainer's eyes.
Other factors
- My earlier inline finding (serve.test.ts:1954 column) was addressed in fdfae0b and the author swept the rest of the suite for similar assertions.
- The farm-branch suggestion to bump the transpiler cache version was applied in 967a6e8; the new Version 26 comment follows the same multi-line format as Versions 22–25, so the reopened comment-cop flag on it appears to be a heuristic false positive. Same for the 3-line rationale in known_global.rs, which mirrors the existing RegExp note. These are still open on the thread, so a human should confirm and resolve.
- robobun flagged a textual conflict and two expectation refreshes needed against #37396 depending on merge order — informational, not a defect in this PR.
- Test coverage is thorough: the new stack.test.ts case exercises all eight error constructors plus the inlined-const and arrow shapes; bundler_minify.test.ts asserts both the printed output and the runtime stack of the minified bundle; runtime-transpiler.test.ts pins the
new Functionsource-origin behavior. Thebundler_npm.test.tsfilesize/mapping updates quantify the byte cost (+388 on a 222 KB React SSR bundle).
|
Thanks, agreed on the interaction with #37396: the two are independent, and whichever lands second refreshes those two |
With a spread in the argument list the runtime argument count is unknown; when the spread is empty, new Array(5, ...rest) is new Array(5), a length, while [5, ...rest] is a one element array.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs — both earlier findings (the serve.test.ts column and the new Array(x, ...rest) fold) are addressed. Because this changes what the runtime transpiler emits for every module (error-frame columns move, minified output grows ~4 bytes per construct, cache version bumps), a maintainer sign-off on the size/behavior trade-off and the coordination with #37396 would be worthwhile.
What was reviewed:
minify_global_constructor: verified every formercall_from_newsite now returnsNoneor a literal; the new spread guard on the Array arm covers both n==1 and n>1.EXPECTED_VERSIONbump follows the pattern for versions 22-25; the changelog line matches the parser-output change.- Column deltas: spot-checked the −4 shifts in inspect-error/hot/serve/test-test against the fixture indent, and the +4
originalColumnin stack.test.ts (transpiled-output position, now 4 bytes longer withnew). normalizeErrorrewrite in inspect-error.test.js: the new regex still matches the old(:1:21)shape as well as the current(51:24)form.
Extended reasoning...
Overview
The PR stops KnownGlobal::minify_global_constructor (src/ast/known_global.rs) from ever rewriting new X(...) into a plain call X(...). The call_from_new helper is deleted; the eight error constructors, Function, RegExp, and the non-literal-foldable Object/Array cases now return None (keep the construct as written). A spread-argument guard is added to the Array arm so new Array(5, ...rest) is never folded into [5, ...rest]. RuntimeTranspilerCache::EXPECTED_VERSION bumps 25→26 so cached call-form output is invalidated. Thirteen test files are updated: three gain new tests (stack.test.ts, bundler_minify.test.ts, runtime-transpiler.test.ts), the rest are mechanical column-snapshot shifts (−4) plus the React SSR bundle size (222000→222388) and sourcemap column offsets in bundler_npm.test.ts.
Security risks
None. The change is purely subtractive on the minifier side (it stops performing a rewrite) and a version-constant bump on the cache side. No new parsing of untrusted input, no new I/O, no allocation-size arithmetic.
Level of scrutiny
High. minify_global_constructor runs during the visit pass of every module the runtime transpiler processes (bun run, bun test) and every --minify build. The change alters user-visible output: error-frame columns move by −4 across the board (Bun.inspect, code frames, dev error page), and minified bundle sizes grow slightly. While the Rust diff itself is small and easy to verify as correct-by-subtraction, the behavioral surface area and the explicit trade-off (4 bytes per construct vs. correct stack frames) is the kind of decision a maintainer should ratify.
Other factors
- Both prior automated findings on this PR were fixed in fdfae0b (serve.test.ts column 19→15) and 5d87dd8 (spread guard before the
match ndispatch). - Test coverage is thorough: every rewritten constructor has a capture assertion pinning the
newform, plus runtime tests that execute the bundled output and check the stack frames. The author verified USE_SYSTEM_BUN=1 fails and the debug build passes across the touched directories. - There is a documented interaction with open PR #37396 (which moves the
.stackstring column tonewas well); whichever lands second refreshes two expectations in stack.test.ts and inspect-error.test.js. This is coordination a human should be aware of, not a defect in this PR. - The
normalizeErrorhelper rewrite in inspect-error.test.js is a functional change (regex instead of substring match) needed for the test to pass under debug builds; I checked the new pattern still matches the old(:1:21)internal-frame format via the:?before\d+.
1 participant
What does this PR do?
An error created with
return new Error(...)has no stack frame for the function that created it. Error factory helpers are common (const e = new Error(msg); return e;has the same problem, since the single-use binding gets inlined into thereturn), so a lot of user stack traces are missing their most useful frame.(The remaining four column difference in the
.stackstring is a separate, pre-existing thing: that formatter reports the constructor name's column for anynew X()frame, user classes included, while Bun's code frames and V8 report thenewkeyword.)Cause
KnownGlobal::minify_global_constructor(src/ast/known_global.rs) rewritesnew Error(...), and the other seven native error constructors, into plain callsError(...)to save four bytes. It runs wheneverminify_syntaxis on, which is always the case for the runtime transpiler (bun run,bun test, ...) and forbun build --minify.Modules are strict mode code, and JSC implements proper tail calls in strict mode.
return Error("a")is a call in tail position, so JSC emitsop_tail_calland popsa()'s frame before the Error constructor runs; the stack captured inErrorInstance::finishCreationnever contains it (BUN_JSC_dumpGeneratedBytecodes=1showstail_callforaversuscallwhen another statement follows). Constructs are never tail calls, which is whythrow new Error(), class constructors, and any function with a statement between thenewand thereturnwere unaffected.Fix
Keep the
newon the error constructors. The rewrite is not semantics preserving on JSC:Error()andnew Error()build the same object, but in tail position they do not produce the same.stack. Tail calls themselves stay enabled (disabling them was rejected in #26001 for performance); this only stops the transpiler from turning constructs the user wrote into tail calls they did not write. esbuild's--minifykeepsnew Error()as well. The cost is 4 bytes per error construction in--minifyoutput (2 for the zero-argument form, which prints asnew Error); the React SSR development bundle inbundler_npm.test.tsgrows by 360 bytes from 222000.The same function stripped
newfromnew Function(...), and that constructor reads the calling frame too: it takes the source origin of the new body from it, which is what a dynamicimport()inside the body resolves against. Withreturn new Function("return import('./x.mjs')")inlib/make.mjs, Bun resolved./x.mjsrelative to the module that calledmake()instead oflib/(node:lib/). Same fix, so it is included here.That left
new Object(x)andnew Array(x)(non-literal argument, or a possible length) as the only constructs still rewritten into calls, andArrayhas the same problem once more:return Array(...lengths)is a varargs tail call, so the RangeError thrown for an invalid length has no frame for the function that asked for the array (a directArray(n)only keeps its frame because JSC's bytecode generator special-cases that callee;Object(x)cannot throw). Rather than document which calls happen to be safe,minify_global_constructornow only ever folds a construct into a literal (new Object()to{},new Array(1, 2)to[1, 2],new Array(3)to[,,,]under whitespace minification, and so on) and never into a call;call_from_newis gone. That costs another 4 bytes pernew Array(n)/new Object(x)site in--minifyoutput, 28 bytes on the React SSR bundle (222388 total). The constructors that are deliberately not folded stay in the table, asRegExpdid, so the rule is recorded next to the code that would do it.While here, the one literal folding that was not equivalent is fixed too:
new Array(5, ...rest)was folded into[5, ...rest], but with an emptyrestthe original isnew Array(5), a length. Any spread argument now leaves the construct alone;GlobalConstructorSemanticsPreservedruns that case.Since the runtime transpiler cache stores transpiled output and is keyed only on cache version, source hash and option flags, entries written by earlier builds would keep serving the call form;
EXPECTED_VERSIONinRuntimeTranspilerCache.rsmoves to 26, as the note at the top ofparser.rsasks for.As a consequence, the position Bun reports for an error created with
new XError(...)in code frames and call sites moves back from the constructor name to thenewkeyword, which is where V8 reports it and where Bun reported it before #22493 introduced the rewrite. The snapshots and column assertions that #22493 updated are updated back (inspect-error,inspect,reportError,console-log,test-test,test-error-code-done-callback,hot,stack,bundler_npm), as is the dev error page frame position inserve.test.ts.inspect-error.test.js's helper that strips debug-build-only internal frames also had to learn the currentat require (51:24)format, otherwise the file cannot pass under a debug build.How did you verify your code works?
New tests, all failing on the unfixed build and passing with the fix:
test/js/bun/test/stack.test.ts: a function returningnew Error()is in the error's stack. Covers all eight constructors, the inlinedconstshape, and an expression-bodied arrow; on the unfixed build all ten factories are missing from their error's stack. A second test checks the RangeError fromreturn new Array(...lengths)still names the function (unfixed build:at Array (unknown)followed directly by the caller).test/bundler/bundler_minify.test.ts:minify/ErrorConstructorKeepsNew(bundler output keepsnewfor every error constructor withminifySyntax; previouslyErrorConstructorOptimizationasserted the opposite),minify/ReturnedConstructorKeepsItsFrame(runs the minified bundle and checks the Error, inlined TypeError and throwing Array frames), and thenew Function(...),new Object(x),new Array(n)/new Array(...xs)captures inminify/AdditionalGlobalConstructorOptimizationandminify/ArrayConstructorWithNumberAndMinifyWhitespace, which previously asserted the call form.test/bundler/transpiler/runtime-transpiler.test.ts: a body built withreturn new Function()resolvesimport()relative to the returning module (printswhich.mjsinstead oflib/which.mjson the unfixed build).Every other test file touched here fails on the unfixed build, because of the column change or because it pinned the call form (
bundler_npm,minify-new-array-with-if), and passes with the fix. I also rantest/js/bun/test,test/js/bun/util,test/js/node/util,test/js/node/v8,test/js/web/console,test/regression/issue,test/bundler/transpiler,test/cli/runandtest/cli/hotagainst the debug build; the remaining failures there are debug-build timeouts, tests needing services or network, or cross-file pollution when many files run in one process, and the stack related ones among them pass when run on their own.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_npm.test.ts test/cli/hot/hot.test.ts test/js/bun/http/serve.test.ts test/js/bun/test/test-test.test.ts test/js/bun/util/inspect.test.js