Conversation
A let/const binding with an anonymous function, arrow or class expression
as its initializer is a NamedEvaluation context: the value receives the
binding name as its ".name" property. The single-use inlining pass was
substituting such an initializer into its sole use site, which drops that
name and makes it observably empty.
This is always on at runtime (target=bun implies minify_syntax), and the
block-level function-declaration lowering produces exactly this shape, so
"{ function f() {} ... }" was losing f.name whenever f was referenced
exactly once. First observed as performance.timerify() emitting entries
with an empty name for a block-scoped function declaration.
|
Warning Review limit reached
Next review available in: 19 seconds 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 4:35 AM PT - Jul 29th, 2026
❌ @robobun, your commit a331541 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36313That installs a local version of the PR into your bun-36313 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…es gap) --keep-names does not emit __name() for the block-level function declaration lowering, so the bundled output gives f.name === "f2" (node agrees on the bundled output). These two tests only passed before because the runtime transpiler inlined the single-use let back into the var assignment, which re-applied NamedEvaluation with the original name. Same gap FunctionHoistingKeepNames1/2 were already marked todo for.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — my earlier feedback (link a tracking issue for the --keep-names block-level-lowering gap) was addressed: #36320 is now referenced from all four FunctionHoistingKeepNames todos.
What was reviewed:
- The bail-out reuses the existing
is_anonymous_named()helper (arrow / nameless fn / nameless class) and usesbreak, matching every other exit in that loop; refusing to inline is always semantics-preserving, so this can only cost minification, not correctness. - Verified named function expressions and non-function initializers still fall through and inline (covered by
t4/t5). - Re-checked the
FunctionHoistingKeepNames3/4→todotransition: bundler output is unchanged, node already produced "f2" on the same bytes, and the gap is now tracked in #36320. - Confirmed the #36320 links are present in the current PR diff (the finder's "lost during branch rewrite" concern was ruled out).
Extended reasoning...
Overview
Six-line guard in src/js_parser/visit/mod.rs inside the single-use-let inlining loop: if the initializer of the last decl is an anonymous function/arrow/class expression, break out of the inlining loop instead of substituting it into its sole use site. This preserves the .name the value receives via NamedEvaluation at the declaration site. New test file exercises the runtime transpiler (spawned subprocess), the Bun.Transpiler API, and the block-level function-declaration lowering shape from #20398/#22770. Two --keep-names bundler tests that were passing by accident (bun's runtime transpiler was re-inlining the bundled output and masking a pre-existing gap) are marked todo and linked to #36320.
Security risks
None. Pure minifier-optimization bail-out; no untrusted input handling, no memory management, no FFI.
Level of scrutiny
The parser's inlining pass runs on every file bun executes, so it warrants care — but this change is strictly conservative: it only declines an optimization, so the worst case is slightly less compact output, never a new correctness bug. is_anonymous_named() is the same helper already used throughout visit_decls/visit_binding for exactly this NamedEvaluation purpose, and break matches every other bail-out in this loop (the loop only ever looks at the last decl, so an un-inlined last decl blocks earlier ones regardless). I couldn't construct a shape where this over- or under-bails: named function expressions carry an intrinsic name and correctly fall through to the identifier check; conditional/comma-wrapped anonymous functions aren't NamedEvaluation targets and is_anonymous_named() correctly returns false for those.
Other factors
- My previous review's only ask (tracking issue for the
todo'd tests) was addressed; #36320 is linked from all fourFunctionHoistingKeepNamesentries in the current diff. - The PR description's mechgate evidence shows the new tests fail on main (
["","","","named",43,""]) and pass with the fix, in both debug+ASAN and release. - Test hygiene follows CLAUDE.md conventions:
bunExe/bunEnv,using tempDir, concurrent pipe drain, exit code asserted alongside stdout/stderr in one object. - All inline review threads on the PR are resolved; no outstanding human feedback.
There was a problem hiding this comment.
Thanks for filing #36320 and linking it from the four FunctionHoistingKeepNames todos — that addresses my earlier note. The fix itself looks correct and the tests are thorough; deferring to a maintainer only because this sits in the parser's single-use inlining path and trades a very common inlining opportunity (const f = () => {}) for spec-correct .name, which is worth a human ack.
What was reviewed:
is_anonymous_named()covers exactly EArrow / nameless EFunction / nameless EClass;breakmatches the other bail-outs in the same loop.- Traced the KeepNames3/4 green→todo transition — bundler output is unchanged; the runtime transpiler was previously masking the pre-existing #36320 gap.
- New tests spawn a fresh process, drain pipes concurrently, and cover fn/arrow/class/named/non-fn plus the #20398 array-literal substitution shape.
Extended reasoning...
Overview
Six-line addition to the single-use-variable inlining loop in src/js_parser/visit/mod.rs: bail out when the initializer is an anonymous function/arrow/class expression, because the let/const binding is a NamedEvaluation context and inlining would drop the observable .name. Ships a new 3-test file exercising the runtime transpiler and Bun.Transpiler, and marks extra/FunctionHoistingKeepNames3/4 as todo (linked to newly-filed #36320) since they only passed before because the runtime transpiler's inlining was papering over a pre-existing --keep-names gap.
Security risks
None. Pure AST-optimization change; no untrusted input handling, allocation, or FFI touched.
Level of scrutiny
High — src/js_parser/visit/mod.rs runs on every JS/TS file Bun loads, and the inlining pass is active by default (runtime transpiler enables minify_syntax). The change is strictly conservative (only ever skips an optimization, never adds one), so the correctness risk is low, but the surface area is the entire runtime. The design tradeoff — losing inlining for every single-use const f = () => {} in a nested scope in exchange for spec-correct .name — is clearly the right call, but a maintainer should confirm they're happy taking it rather than a narrower fix (e.g. only when keep_names is off, or naming the function expression at the block-level-lowering site instead).
Other factors
- My previous inline nit (link a tracking issue for the two green→todo tests) was addressed in a331541; all four
FunctionHoistingKeepNamestodos now reference #36320. is_anonymous_named()is the same helper already used byvisit_decls/visit_binding/visit_classfor NamedEvaluation handling, so no new predicate.- The
breakplacement is consistent with every other bail-out in the'innerloop; it correctly stops back-to-back inlining once an un-inlinable decl is hit (earlier decls can't be reordered past it anyway). - Tests follow harness conventions (
bunEnv/bunExe/tempDir/await using, concurrent pipe drain, combined-object assertion), and the PR body shows the suite failing on main and passing with the fix under both ASAN-debug and release.
|
Diff is ready. Build #84848: 189/196 jobs passed. Remaining red is unrelated to this change:
The source change is the 6-line |
|
One more user-visible effect of the same inlining, for the record: the substituted function also loses its name in // a.mjs
function outer() { const nested = () => { throw new Error("A"); }; nested(); }
try { outer(); } catch (e) { console.log(e.stack.split("\n")[1].trim()); }The same happens for I applied the |
This branch has not been deployed
2 participants
What does this PR do?
The single-use-variable inlining pass (active whenever
minify_syntaxis on, which is always the case for the runtime transpiler) was substituting anonymous function/arrow/class initializers into their sole use site. The declaration site of alet/constwith an anonymous function initializer is a NamedEvaluation context, so this substitution drops the.namethe value would otherwise receive.The block-level function-declaration lowering produces exactly this shape (
{ function f() {} }becomes{ let f = function() {} }with the name cleared), so a block-scoped function declaration lost its.nameat runtime whenever it was referenced exactly once.Fixes #20398
Fixes #22770
Repro
Originally observed as
performance.timerify()emittingfunctionentries with an emptynamefor a block-scoped function declaration wrapped via an aliasedtimerifyreference.Fix
Skip single-use inlining when the initializer is an anonymous function/arrow/class expression. Named function expressions and all non-function values still inline as before.
How did you verify your code works?
New tests in
test/js/bun/transpiler/transpiler-inline-anonymous-fn-name.test.tsspawn a fresh bun process so the fixture goes through the real runtime transpiler:["","","","named",43,""]/["",""]["fn","arrow","Cls","named",43,"f"]/["slow","work"]Also ran
test/js/node/perf_hooks/perf_hooks.test.ts,test/bundler/bundler_minify.test.ts, andtest/bundler/transpiler/(all pass).[review] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file