js_parser: track template_tag so wrapper folds don't rebind this - #36735
js_parser: track template_tag so wrapper folds don't rebind this#36735robobun wants to merge 8 commits into
this#36735Conversation
A tagged template `expr`...`` binds `this` for `expr` the same way a
call `expr()` does. The visit pass already tracks `p.call_target` so that
folds like `(0, obj.m)()` → `obj.m()` are suppressed (they would change the
receiver from undefined to `obj`), but it had no equivalent for tagged
templates: the `.is_template_tag = ...` TODOs were left over from the port.
That meant every fold that checks `is_call_target && has_value_for_this_in_call()`
was wrong in tag position. Under `minify_syntax`:
var obj = { m() { return this === obj } };
(1 ? obj.m : 0)`x` // node: false, bun: true (folded to obj.m`x`)
(0, obj.m)`x` // node: false, bun: true
(null ?? obj.m)`x` // node: false, bun: true
(true && obj.m)`x` // node: false, bun: true
(false || obj.m)`x` // node: false, bun: true
[obj.m][0]`x` // node: false, bun: true
({m: obj.m}).m`x` // node: false, bun: true
Add `p.template_tag` (mirroring `p.call_target`), set it in `e_template`
before visiting the tag, and compute `is_template_tag` alongside
`is_call_target` in `e_index`/`e_dot`/`e_if`/`e_binary`. Every
`(0, x)` emission now fires for `is_call_target || is_template_tag`, and
the `[x][0]` / `{f:x}.f` folds bail out in tag position (matching esbuild).
The `IdentifierOpts` bit is wired through so `maybe_rewrite_property_access`
sees it.
Bump the runtime transpiler cache version.
WalkthroughChangesTagged-template targets are tracked through parser and expression visitation. Folding and simplification preserve receiver semantics for tagged templates and calls. The transpiler cache version and regression tests were updated. Tagged-template receiver preservation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
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}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js_parser/visit/visit_expr.rs:1506-1513— TheCouldHaveSideEffectsbranches at lines 1499–1504 and 1528–1533 (immediately above the newly-guarded checks) still fold to a bare member expression whensimplify_unused_exprreturnsNone: e.g.(typeof x ? obj.m : 0)tag→ `obj.m`tag, rebindingthistoobj. When the simplified test isEMissingand(is_call_target || is_template_tag) && arm.has_value_for_this_in_call(), emit(0, arm)instead — same guard needed in both arms.Extended reasoning...
What the bug is
The PR adds
is_template_tagto thee_ifvisitor and guards theNoSideEffectsfold paths at visit_expr.rs:1510 and :1539 with(is_call_target || is_template_tag) && arm.has_value_for_this_in_call(). But the siblingCouldHaveSideEffectsbranches directly above them — 1499–1504 and 1528–1533 — are left unguarded. Whensimplify_unused_expr(test)returnsNone,EMissing.join_with_comma(arm)returns barearm, so a member-expression arm lands directly in tag/call position andthisrebinds.Step-by-step proof
Input (with
minify_syntaxon): ``(typeof x ? obj.m : 0)`tag```e_templatesetsp.template_tag = <the EIf>, soe_ifcomputesis_template_tag = true.simplify_boolean(scan_side_effects.rs:49–89) only handlesUnNot/BinLogicalAnd/BinLogicalOr;typeof xpasses through unchanged.to_boolean(typeof x)at scan_side_effects.rs:941–947 returns{ok: true, value: true, side_effects: CouldHaveSideEffects}(typeof is always a non-empty string, but the operand is conservatively side-effecting).side_effects.value == true→ enter the true arm at line 1491;side_effects.side_effects == CouldHaveSideEffects→ enter the block at line 1499, which runs before the newly-guarded check at 1510.simplify_unused_expr(typeof x)at scan_side_effects.rs:219–228 returnsNone(WAS_ORIGINALLY_TYPEOF_IDENTIFIERis set, andtypeof identifiernever throws, so it can be dropped entirely).unwrap_or_elseyieldsEMissing;EMissing.join_with_comma(obj.m)at expr.rs:800–802 returns justobj.m(if self.is_missing() { return b }).- Result: the tag folds to bare
obj.m, so the emitted code is ``obj.mtag``` —this` becomes `obj` instead of `undefined`.
The false arm (1528–1533) has the identical shape: ``(void x ? 0 : obj.m)
tag``` —to_boolean(void x)` = `{value: false, CouldHaveSideEffects}` (scan_side_effects.rs:936–940); `simplify_unused_expr(void x)` recurses into `x` and returns `None` for a bound identifier.Why the existing code doesn't prevent it
The new guard at 1510/1539 only fires when
side_effects == NoSideEffects. TheCouldHaveSideEffectsblock returns first (lines 1503/1532), and thejoin_with_commathere can degenerate to the bare arm wheneversimplify_unused_exprproves the test has no actual effects (typeof identifier,void identifier, an||/&&of pure identifiers, etc.). The same gap exists foris_call_target—(typeof x ? obj.m : 0)()folds toobj.m()— which is pre-existing, but the PR now computes both flags right there and the fix is symmetric.Impact
Miscompilation under
minify_syntax(on by default for bun-target files at runtime): the receiver of a tagged template / call changes fromundefinedto the member's base object. This is exactly the bug class the PR is fixing, in a parallel arm of the same handler the PR edits. Per REVIEW.md — "Fix the whole class in the same PR … parallel switch arms" — the sibling paths belong in this change. The PR description also claims the six(0, x)emission sites include "ternary both arms", which this path bypasses.Fix
In each
CouldHaveSideEffectsblock, whensimplify_unused_exprreturnsNone(or the joined result would be the bare arm) and(is_call_target || is_template_tag) && arm.has_value_for_this_in_call(), emit(0, arm)instead. Something like:if side_effects.side_effects == SideEffects::CouldHaveSideEffects { let test = SideEffects::simplify_unused_expr(p, e_.test); if test.is_none() && (is_call_target || is_template_tag) && e_.yes.has_value_for_this_in_call() { *e = p.new_expr(E::Number::new(0.0), e_.test.loc).join_with_comma(e_.yes); return; } *e = test .unwrap_or_else(|| p.new_expr(E::Missing {}, e_.test.loc)) .join_with_comma(e_.yes); return; }
(and the mirror for
e_.no). Worth adding(typeof x ? obj.m : 0)/(void x ? 0 : obj.m)cases to the new "tagged-template tag folds preservethis" test.
…/tag position When the ternary test is statically known but conservatively classed as CouldHaveSideEffects (e.g. `typeof x`), `simplify_unused_expr` can drop it entirely, so `EMissing.join_with_comma(arm)` returned the bare arm and the `(0, arm)` guard below never ran. Unify both paths so the guard applies regardless of the test's side-effect class; when a real side effect survives, the resulting `(side, arm)` comma already strips `this`. Fixes both `(typeof x ? obj.m : 0)`t`` and the pre-existing call-target case `(typeof x ? obj.m : 0)()`.
|
Addressed in 17ef1c4: the |
`visit_right_and_finish` read `p.call_target`/`p.template_tag` after the left operand had already been visited, so any nested call/tagged-template in left overwrote the slot and the outer binary's identity check came up false. Move the capture into `check_and_prepare` (runs before any child visit) and stash it on `BinaryExpressionVisitor`, matching the early-capture shape of `e_index`/`e_dot`/`e_if`. Fixes `((() => f`a`), obj.m)`x`` folding to `obj.m`x``, and the pre-existing call-target twin `((() => f()), obj.m)()` → `obj.m()`. Also adds the falsy-arm `CouldHaveSideEffects` mirror (`typeof x && 0`) to the ternary coverage.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js_parser/visit/visit_expr.rs (1)
1069-1129: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
!is_call_targetto the array/string index constant fold.
can_be_inlined_from_property_access()acceptsEDotandEIndexvalues. Therefore,[obj.m][0]()can fold toobj.m()and change thethisreceiver from the temporary array toobj.🤖 Prompt for 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. In `@src/js_parser/visit/visit_expr.rs` around lines 1069 - 1129, Restrict the array and string index constant-folding block in the expression visitor to cases where !is_call_target, adding this guard alongside the existing minify_syntax and !is_template_tag checks. Preserve the current literal and array folding behavior for non-call targets, while preventing folds such as [obj.m][0]() that would alter the receiver.
🤖 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/transpiler.test.js`:
- Around line 3993-3997: Update the subprocess test around Bun.spawn to stop
asserting that stderr is exactly empty, while still reading stderr in the
existing Promise.all to drain the pipe. Continue validating the test outcome
through the expected stdout and zero exitCode assertions.
---
Outside diff comments:
In `@src/js_parser/visit/visit_expr.rs`:
- Around line 1069-1129: Restrict the array and string index constant-folding
block in the expression visitor to cases where !is_call_target, adding this
guard alongside the existing minify_syntax and !is_template_tag checks. Preserve
the current literal and array folding behavior for non-call targets, while
preventing folds such as [obj.m][0]() that would alter the receiver.
🪄 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: 49d5b7c3-ffc1-41c7-b339-092d1eb84c4d
📒 Files selected for processing (7)
src/js_parser/fold.rssrc/js_parser/p.rssrc/js_parser/parser.rssrc/js_parser/visit/visit_binary.rssrc/js_parser/visit/visit_expr.rssrc/jsc/RuntimeTranspilerCache.rstest/bundler/transpiler/transpiler.test.js
|
On the two review-bot findings:
|
There was a problem hiding this comment.
No issues found on this pass — both earlier findings (the e_if CouldHaveSideEffects hole and the e_binary late capture) are addressed, and the remaining [x][0] call-target gate is deferred to #36730/#36734 as stated. Leaving for a human because #36599 bundles most of the same template_tag plumbing and #36730/#36734 touch the exact e_index gate this PR edits; merge order across those four is a maintainer call.
What was reviewed:
e_ifrefactor: traced that the unifiedleft = simplify-or-Missing → substitute 0 if missing-and-call/tag → join_with_commapath is behavior-preserving for the oldNoSideEffects/CouldHaveSideEffectssplit and closes thetypeof x/[]simplify-to-nothing hole for both call and tag targets.BinaryExpressionVisitor: confirmedis_call_target/is_template_tagare now captured incheck_and_preparebefore any child visit, and theEPrivateIdentifierearly return never reachesvisit_right_and_finishso the unset fields are dead there.p.template_tagslot: same no-restore pointer-identity pattern asp.call_target; stale values can't false-match since arena addresses aren't reused within a parse.
Extended reasoning...
Overview
Adds p.template_tag: ExprData alongside the existing p.call_target so wrapper-fold sites ((0, x), ternary, ??/||/&&, [x][0], {f:x}.f) preserve the (0, obj.m) indirection when the result is a tagged-template tag, matching how they already behave for call targets. Wires is_template_tag into e_index/e_dot/e_if/BinaryExpressionVisitor and through IdentifierOpts into maybe_rewrite_property_access. Along the way it fixes two pre-existing is_call_target holes in the same code (the e_if CouldHaveSideEffects fall-through and the e_binary capture-after-left-visit timing). Bumps the runtime transpiler cache to v25 and adds both transpiler-output and spawned-runtime tests.
Security risks
None. Pure AST-fold correctness in the visit pass; no I/O, auth, or untrusted-data parsing changes. The transpiler-cache bump is a monotone version increment following the file's convention.
Level of scrutiny
High — this is the JS parser visit pass, executed for every file the runtime transpiles with minify_syntax on (bun-target default). Miscompiles here silently change program semantics. That said, the change is narrowly scoped to mirroring an existing, well-understood pattern (call_target) into a sibling context, and the two structural refactors (e_if arm unification, e_binary capture hoist) were each traced against the old control flow and against esbuild's output.
Other factors
The deciding factor for deferral is coordination, not correctness: the duplicate-PR bot flagged #36599 as bundling most of the same template_tag tracking, and #36730/#36734 both add call/delete/assign gates to the exact e_index fold condition this PR touches (which is why the author intentionally left !is_call_target off that line). A maintainer should pick the merge order and reconcile the cache-version bump across whichever set lands. My two earlier inline findings were addressed in 17ef1c4 and 43d530b; the third was declined with a stated reason that satisfies REVIEW.md's "if a site is intentionally excluded, say so". Tests cover both the printed-output shape and the actual runtime this value via a spawned subprocess.
Two more parallel-arm sites in the same bug class:
- `BinNullishCoalescingAssign | BinLogicalOrAssign` in `visit_right_and_finish`
(the `{}.x ??= v` HMR fold) returned `e_.right` bare with no
`has_value_for_this_in_call()` guard. Fires without `minify_syntax`.
`({}.x ??= obj.m)()` / `({}.x ??= obj.m)\`t\`` now emit `(0, obj.m)`.
- `substitute_single_use_symbol_in_expr`'s `ETemplate` arm lacked the
'don't substitute into a target that could change this' check the parallel
`ECall` arm has. `let x = obj.m; x\`t\`` no longer inlines to
`obj.m\`t\``.
Both were pre-existing for `is_call_target` too and neither is covered by
#36730/#36734.
There was a problem hiding this comment.
All four findings from my earlier passes are addressed in 43d530b / 17ef1c4 / 99c6d51, and this run found nothing new. Deferring to a human for the final look given this is a parser-minification correctness change with three overlapping open PRs (#36599 / #36730 / #36734) that touch the same gates.
What was reviewed:
- Traced the
e_ifrefactor against the old control flow — behavior-preserving for all four (side_effects × call/tag) combinations, andEMissing.join_with_comma(arm)still yields barearmwhen no guard is needed. - Verified
check_and_preparecapturesis_call_target/is_template_tagbefore any child visit (theEPrivateIdentifierarm that visitsrightearly-returns, so never reachesvisit_right_and_finish). - Confirmed the new
ETemplateguard insubstitute_single_use_symbol_in_exprmatches theECallarm at p.rs:2520 byte-for-byte, and the??=/||=arm now emits(0, right)with the same predicate as its four siblings. - Runtime test's 18-entry array lines up with the expected
[false×17, true]output; the IIFE case exercises themangle_stmtsfix under the runtime transpiler.
Extended reasoning...
Overview
Adds p.template_tag: ExprData alongside the existing p.call_target so wrapper-fold sites in the visit pass (e_if, e_binary comma/??/||/&&/??=/||=, e_index [x][0], e_dot/maybe_rewrite_property_access {f:x}.f) can detect when they're in tagged-template tag position and avoid folding to a bare member expression, which would rebind this. Also fixes two pre-existing is_call_target holes (the e_if CouldHaveSideEffects early-return and the e_binary late-capture race), and adds the missing ETemplate guard to substitute_single_use_symbol_in_expr. New IdentifierOpts bit, transpiler-cache version bump, ~80 lines of transpiler-output + runtime tests.
Security risks
None. Pure parser/minifier correctness; no untrusted-input parsing, no auth/crypto/permissions.
Level of scrutiny
High — this is the runtime transpiler's constant-folding pass, where a wrong fold silently changes the semantics of user code (this binding) with no error. That's why I ran three review passes. The mitigants: the change is a direct port of esbuild's handling (the reference implementation per REVIEW.md), every fold site is covered by both a transpiler-output assertion and a spawned runtime check, and the cache version is bumped so stale .pile blobs won't mask it.
Other factors
- All four of my prior findings (e_binary capture timing, e_if CouldHaveSideEffects hole, ??=/||= sibling arm, substitute ETemplate arm) were addressed with tests; the one intentionally-deferred item (
!is_call_targeton the[x][0]gate) is documented as owned by #36730/#36734. - The
e_ifarm was refactored (not just guarded) to unify the CouldHaveSideEffects and NoSideEffects paths; I traced all four combinations and it's behavior-preserving, but a human should eyeball it since it's the only structural rewrite in the diff. - Three open PRs touch the same lines/state in different shapes; landing order and rebase strategy is a maintainer call.
- The declined CodeRabbit nit (
expect(stderr).toBe("")) is fine —bunEnvsetsBUN_DEBUG_QUIET_LOGS=1and every neighboring spawn test in this file does the same.
|
CI note: build #87736 (on 5a46e52, main merged in) has 194/197 lanes passed; the remaining failures are all marked flaky (passed when retried alone) plus one broken manual-unblock pipeline step. No The local gate check ( |
|
Updated 4:48 AM PT - Aug 2nd, 2026
✅ @Jarred-Sumner, your commit 5a46e52826c819d6298d365d7b14ce052082e166 passed in 🧪 To try this PR locally: bunx bun-pr 36735That installs a local version of the PR into your bun-36735 --bun |
There was a problem hiding this comment.
All five of my earlier inline findings have been addressed (17ef1c4, 43d530b, 99c6d51), and the current diff looks correct to me — I re-traced the restructured e_if arms and the moved check_and_prepare capture and both are behavior-preserving apart from the intended fix. Deferring rather than approving because this touches parser constant-folding across five files and its scope is coordinated with three overlapping open PRs (#36599, #36730, #36734); a maintainer should sign off on the sequencing and the e_if restructuring.
What was reviewed
e_ifyes/no arms: verified all six paths (NoSideEffects/CouldHaveSideEffects × call/tag/neither) are behavior-preserving except thesimplify_unused_expr → Nonefix.check_and_preparecapture: theEPrivateIdentifierearly-return that precedes it never reachesvisit_right_and_finish, so the uncaptured fields aren't read on that path.substitute_single_use_symbol_in_exprETemplateguard mirrors theECallarm exactly;IdentifierOptsbit 5 wiring is consistent with bits 2–4.
Extended reasoning...
Overview
Adds p.template_tag: ExprData alongside the existing p.call_target so wrapper folds that check is_call_target && has_value_for_this_in_call() also fire when the folded expression is a tagged-template tag. Touches visit_expr.rs (e_template, e_index, e_dot, e_if), visit_binary.rs (comma / ?? / || / && / ??=/||= arms; capture moved from visit_right_and_finish to check_and_prepare and stashed on the visitor), fold.rs ({f:x}.f gate), p.rs (new field + substitute_single_use_symbol_in_expr ETemplate guard), parser.rs (IdentifierOpts bit 5), a cache-version bump, and 98 lines of tests covering both transpiler output and runtime behaviour via subprocess.
Prior review rounds
I left inline findings in three passes (14:28, 15:15, 16:31 UTC on Aug 1). All were addressed:
e_ifCouldHaveSideEffectspath leaking bare member → fixed by unifying both arms to computeleftfirst and substitute0when missing in call/tag position.e_binarylate capture clobbered by nested call/template in left → fixed by capturing incheck_and_prepareand stashing on the visitor struct.??=/||=HMR fold missing the guard → fixed with the same(0, right)shape as the four sibling arms.substitute_single_use_symbol_in_exprETemplatearm missing theECallguard → copied verbatim.!is_call_targeton the[x][0]gate → intentionally deferred to #36730/#36734 with a stated reason (both PRs touch that exact condition; adding it here conflicts either way).
The bug hunting system found nothing new on the current revision.
Security risks
None. Pure AST-transformation correctness in the transpiler; no I/O, auth, or untrusted-input parsing surface changes.
Level of scrutiny
High. The JS parser's constant-folding runs on every source file at runtime (minify_syntax is on for bun-target files) and in Bun.build. A wrong fold silently changes program semantics. The e_if arms were restructured (not just guarded), and the binary-visitor capture timing was moved — both are the kind of refactor REVIEW.md flags as "guilty until proven behavior-preserving". I traced them and they check out, but this is exactly the class of change a maintainer should eyeball.
Other factors
- Cross-PR coordination: #36599 bundles most of the same
template_tagtracking; #36730/#36734 both touch thee_indexgate this PR edits. The author has scoped this PR to minimize conflicts and documented the overlap, but a maintainer should confirm the landing order. - Test coverage: strong — transpiler-output assertions for every fold site plus a runtime subprocess test that verifies actual
thisbinding matches Node. Theexpect(stderr).toBe("")CodeRabbit flagged is consistent with the file's convention (bunEnvsetsBUN_DEBUG_QUIET_LOGS=1) and the author declined with a stated reason. - Cache version bump: correctly incremented with a changelog entry matching the file's convention.
Given the critical-path location and the three-PR coordination, I'm deferring to a human rather than approving.
2 participants
Problem
A tagged template
expr`...`bindsthisforexprthe same way a callexpr()does: ifexpris a property accessa.b, the receiver isa; ifexpris anything that evaluates to a value ((0, a.b),[a.b][0],cond ? a.b : x, ...), the receiver isundefined.The visit pass already tracks
p.call_targetso wrapper folds in call position keep the(0, a.b)indirection instead of collapsing toa.b()and changing the receiver. There was no equivalent for tagged templates:e_templatevisited its tag with a plainvisit_expr()and the.is_template_tagsetters were commented-out port TODOs. So every fold that checksis_call_target && has_value_for_this_in_call()was unguarded in tag position.Fix
Add
p.template_tag: ExprDataalongsidep.call_target, set it ine_templatebefore visiting the tag, and computeis_template_tagnext tois_call_targetine_index/e_dot/e_if/BinaryExpressionVisitor. The(0, x)emission sites (ternary both arms, comma,??,||,&&, the{}.x ??= vHMR fold) now fire onis_call_target || is_template_tag, and the[x][0]/{f:x}.ffolds bail out in tag position. Anis_template_tagbit onIdentifierOptscarries the flag throughmaybe_rewrite_property_accessso thea["b"] → a.brewrite and the EObject fold see it.substitute_single_use_symbol_in_expr'sETemplatearm gets the same "don't substitute a member into an identifier target" guard theECallarm already has.Pre-existing
is_call_targetholes in the same code got fixed along the way:e_ifCouldHaveSideEffectspath: when the test is statically known but classed as side-effecting (typeof x,[]) and simplifies away entirely,EMissing.join_with_comma(arm)returned the bare arm and the guard below never ran. Both arms now computeleftfirst and substitute0when it would be missing in call/tag position. Fixes(typeof x ? obj.m : 0)().e_binarycapture timing:visit_right_and_finishreadp.call_targetafter the left operand had been visited, so a nested call/template inside left clobbered the slot. The capture now happens incheck_and_prepare(before any child visit) and is stashed on the visitor. Fixes((() => f()), obj.m)().{}.x ??= obj.mfold: had nohas_value_for_this_in_call()guard at all. Fixes({}.x ??= obj.m)().This matches esbuild's handling (verified against
esbuild --minify-syntax).Bumps the runtime transpiler cache version since
minify_syntaxis on for bun-target files at runtime.Verification
Also green:
bundler_minify.test.ts,esbuild/default.test.ts,esbuild/dce.test.ts,runtime-transpiler.test.ts.Overlap
#36599 (multi-property object literal folding) bundles most of the same
template_tagtracking as a supporting change, but does not gate the[x][0]fold on it and keeps the latee_binarycapture. This PR is the focused correctness fix; whichever lands first, the other rebases cleanly except for the cache-version bump. #36730 and #36734 touch the samee_indexgate for call/delete/assign targets; this PR adds only the template-tag term there to minimize conflicts.[review] gate passed · iteration 6 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 1 rejected · iteration 6
evidence per changed file