transpiler: don't inline a single-use binding into a return when that creates a tail call - #37416
transpiler: don't inline a single-use binding into a return when that creates a tail call#37416robobun wants to merge 1 commit into
Conversation
… creates a tail call The runtime transpiler runs with minify_syntax on, so `const r = f(); return r;` was rewritten into `return f();`. In strict mode code (every ES module) JSC turns that call into a proper tail call, and the returning function's frame is missing from every stack trace captured inside `f`. Node keeps the frame, and so does the source as written. Skip the substitution when the initializer ends in a call (directly, or through the tail positions of `,`, `||`, `&&`, `??` and `?:`) and the use is in tail position of the returned expression. Only the runtime transpiler (bundle=false) is affected; `bun build --minify` keeps inlining, like esbuild. Bump the runtime transpiler cache version so cached output is regenerated.
|
Warning Review limit reached
Next review available in: 9 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 (5)
Comment |
|
Updated 9:43 PM PT - Aug 10th, 2026
✅ @robobun, your commit 873a29f5552268ded537ea1655479e87d1094cf5 passed in 🧪 To try this PR locally: bunx bun-pr 37416That installs a local version of the PR into your bun-37416 --bun |
|
Status: reproduced on bun 1.4.0 and a debug build of main with the |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes runtime transpiler output for every bun run/bun test invocation and adds new recursive tail-position analysis to the parser, a human look at the approach (particularly the !bundle gate choice) would still be worthwhile.
What was reviewed:
- Checked the
!self.options.bundlegate matches the existing arrow-collapse pattern at visit_expr.rs:2539 — it does. - Verified
identifier_is_use_ofis a clean extraction of the pre-existing inline check at the old p.rs:2191. - Checked stack-overflow guards on both new recursive helpers follow the
hoist_symbolspattern;return falseon overflow is the conservative direction (skips the optimization gate, falls through to substitution). - Confirmed the tail-position operator set (
,/||/&&/??/?:) and the ECall/tagged-template/require cases are covered by the 38-case test table.
Extended reasoning...
Overview
This PR narrows the single-use-binding inliner in src/js_parser/p.rs so that const r = f(); return r; is no longer rewritten to return f(); when not bundling. The rewrite creates a tail call the user didn't write, and JSC's proper-tail-call implementation then drops the caller's frame from stack traces. The change adds ~80 lines: a guard in substitute_single_use_symbol_in_stmt's SReturn arm, three helpers (has_call_in_tail_position, is_ref_in_tail_position, is_tail_position_operator), and an extracted identifier_is_use_of. RuntimeTranspilerCache::EXPECTED_VERSION is bumped 25→26. Tests span three files with 40+ assertions.
Security risks
None. This is a pure minifier-transform gate; no untrusted input parsing, no allocation, no FFI. The recursive helpers reuse the existing stack_check/report_stack_overflow guard so adversarial deeply-nested expressions cannot overflow the stack.
Level of scrutiny
High. The runtime transpiler runs on every file bun run/bun test loads, so a defect here affects every user. The change is well-scoped and the !options.bundle gate mirrors the existing arrow-body-collapse gate (visit_expr.rs:2539), but the decision to gate on bundle rather than, say, target or strict-mode is a design call the PR description argues for explicitly — a maintainer should confirm they agree.
Other factors
- Test coverage is unusually thorough: a 19-row "keeps binding" table, a 19-row "still inlines" table, an awaited-call case, a spawned
.mjsfixture verifying function/generator/arrow frames survive, an in-process stack test, and twoitBundledcases pinning both bundling and non-bundling behavior. - The PR description explicitly enumerates and rejects alternatives (disabling JSC TCO per #26001, disabling minify_syntax, gating on parser strict-mode) with reasons.
- The cache version bump is correctly included.
- No prior human or bot review comments to address; CI is still building.
Given the scope (core parser hot path, runtime-wide behavior change) this should get a human sign-off even though the implementation looks correct.
|
On the two points from the review above:
|
|
Not a bug. |
|
Understood, the inliner's output is working as intended and the stack trace difference is accepted behavior. Closing out; nothing further planned here. |
What does this PR do?
A function that stores a call's result in a binding and returns it is missing from the stack traces captured inside that call.
b's frame is gone;b2, which differs only by having a statement between the call and thereturn, is fine. The same applies to anything else that looks at its caller's frame, such asError.captureStackTrace(err, fn)based caller detection (depd,bindings, ...) andFunction(...)taking its source origin from the calling frame.Cause
The runtime transpiler (
bun run,bun test,Bun.Transpilerwith a bun target) runs withminify_syntaxon (bundler/options.rsturns it on for bun targets so constants get inlined), and the single-use variable inlining invisit_stmts/P::substitute_single_use_symbol_in_stmtrewritesinto
function b() { return a(); }. The source as written has no call in tail position; the rewritten function does. JSC implements proper tail calls in strict mode code (every ES module, plus"use strict"CommonJS and class bodies), sob's frame is replaced bya's beforearuns, andbis absent from every stack captured insidea:.stack,Error.prepareStackTracecall sites,Error.captureStackTrace. Generator bodies are affected too; async bodies are not.Tail calls the user actually writes (
return a()) are unchanged by this PR; per #26001 they stay enabled. What this PR stops is the transpiler manufacturing one out of code that did not contain it. #37388 fixes the other rewrite with the same effect (return new Error()turned into the callreturn Error()); the two are independent.Fix
In
substitute_single_use_symbol_in_stmt, when the statement is areturn, skip the substitution ifECall, a tagged template,require()/require.resolve()(printed as calls), or one of those reached through the tail positions of,,||,&&,??and either branch of?:(the same set of positions JSC's bytecode generator propagates tail position through), and@__PURE__calls, which the inliner may move into a branch).Returning
falseleaves the declaration in place, exactly as when the substitution fails for any other reason. Every other shape keeps inlining: uses that are not the returned value (return r.x,return g(r),throw r,if (r)), and initializers that do not end in a call (new F(),await f(),f().x,f() + 1,import()), none of which can become a tail call.This only applies when not bundling (
options.bundle == false), the same gate the arrow body collapse and the unused function/class name removal use: when the user asksbun buildfor--minify-syntaxthe rewrite is what they asked for and matches esbuild, and most of that output does not run on JSC anyway. As with the arrow case this also meansbun build --no-bundle --minify-syntaxandBun.Transpilerwithminify.syntaxkeep the binding; the cost is one local variable that the JIT eliminates.Why this is the right fix and not a workaround: the substitution is a minifier transform that is supposed to be unobservable, and on JSC it is observable in exactly this shape, so the transform is what has to be narrowed. The alternatives are worse: disabling JSC tail calls was rejected for performance (#26001), disabling
minify_syntaxat runtime would lose the constant inlining it exists for, and gating on the parser's notion of strict mode would miss the file in the repro (noimport/export, so nothing in the source says it is strict, but a.mjsfile is still a module).RuntimeTranspilerCacheEXPECTED_VERSIONis bumped to 26 so cached output produced by the old inliner is regenerated.How did you verify your code works?
New tests, all of which fail on the unfixed build (
USE_SYSTEM_BUN=1) and pass withbun bd test:test/bundler/transpiler/runtime-transpiler.test.ts: a table of 19 shapes whose binding must stay (plain/method/optional/.call/curried calls, tagged template,require,require.resolve, the?:/||/&&/??/,forms on both sides, the two-binding chain) plus 19 shapes that must still be inlined, run throughBun.Transpilerwith the runtime's settings; and a spawned.mjsfixture checking that a function, a generator and an arrow all stay in the stack captured by their callee (on main the second frame is the module's top level for all three).test/js/bun/test/stack.test.ts: in-process version of the report (plain binding and the@__PURE__conditional shape), since test files go through the same transpiler.test/bundler/bundler_minify.test.ts:minify/KeepSingleUseBindingBeforeReturnWhenNotBundling(fails on main) andminify/InlineSingleUseBindingIntoReturn, which pins that bundling withminifySyntaxstill inlines.Also ran
test/bundler/bundler_minify.test.ts,test/bundler/transpiler/,test/js/bun/transpiler/,test/js/bun/test/,test/js/node/v8/, the stack related files intest/regression/issue/andtest/js/bun/util/, andtest/js/node/util/against the debug build. The only failures are unrelated to this change: debug-build timeouts (jsx-production.test.ts,util-inspect.test.js, which pass with a longer timeout), the pre-existingat require (51:24)debug-only frame ininspect-error.test.js(handled in #37388), files that polluteError.prepareStackTracewhen several are run in one process (they pass individually), and the fuzz suite's redis abort tracked in #37340.