TaggedTemplateNode: call super.tag... and super[key]... with the method's this - #438
TaggedTemplateNode: call super.tag... and super[key]... with the method's this#438robobun wants to merge 1 commit into
... and super[key]... with the method's this#438Conversation
…this TaggedTemplateNode::emitBytecode passed the register holding the tag's base as the call's this value. For a super property reference that base is the home object's prototype, so super.tag`x` invoked the tag with this = Base.prototype instead of the receiver, unlike super.tag() in FunctionCallDotNode / FunctionCallBracketNode (spec: EvaluateCall step 1.a.i, GetThisValue of a Super Reference). Emit the super cases the way the call nodes do: ensureThis() first, then the super base for the lookup, and pass the this register to the call. Doing the this TDZ check first also stops super[key]`...` from evaluating the key before throwing in a derived constructor that has not called super() yet, matching super[key]() and plain super[key]. JSTests/stress/tagged-templates-super-this.js covers both forms in instance, static, object literal, arrow, eval, class field, generator, async and derived constructor contexts, the TDZ ordering, and checks that tags which are not super references are unaffected.
|
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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes bytecode generation in TaggedTemplateNode::emitBytecode — a core engine path whose output feeds all four JIT tiers — and the preview-build test results mentioned in the description are still pending, a human look would still be worthwhile.
What was reviewed:
- The super branches now mirror
FunctionCallDotNode/FunctionCallBracketNode/BracketAccessorNode::emitBytecode—ensureThis()before the subscript,emitSuperBaseForCallee()for lookup, receiver passed as callthis. - Non-super paths are unchanged:
thisValuestays null, and the 4-argemitGetPropertyValuewith a nullthisValueis identical to the 3-arg overload it replaces. - Checked that
ensureThis()→emitSuperBaseForCallee()ordering matches existing call sites and is not observably different from the spec's actualThis → key → superBase order (home objects are ordinary, no proxy trap on the prototype fetch). - The new stress test covers dot/bracket, static/instance, arrows, eval, field initializers, generators/async, TDZ ordering, primitive receivers, and non-super regressions.
Extended reasoning...
Overview
This PR fixes a spec-compliance bug in TaggedTemplateNode::emitBytecode (Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp). Previously, super.tag\...`andsuper[key]`...`used a singlebaseregister for both the property lookup target (the home object's prototype) and the call'sthis, so the tag was invoked with the prototype as thisinstead of the method's receiver. The fix introduces a separatethisValueregister for the two super forms, populated viagenerator.ensureThis()beforeemitSuperBaseForCallee(), and moves thisValueinto the call'sthisregister when set. A 346-line stress test inJSTests/stress/tagged-templates-super-this.js` exercises the fix across instance/static methods, object-literal methods, arrow functions, direct eval, class field initializers, generators/async, derived-constructor TDZ ordering, primitive receivers, getters, replaced prototypes, and JIT tiers, plus regression cases confirming non-super tags are unaffected.
Security risks
None identified. This is a semantic correctness fix to which this value is passed to a tagged-template tag function. There is no new attack surface: the code already computed both the super base and the receiver (ensureThis() was already called inside the old emitGetPropertyValue path); the change only routes the receiver into the call's this register instead of the base. No parsing, allocation, or bounds logic is touched.
Level of scrutiny
High. Although the diff is small (~20 net lines in one function), it lives in the bytecode compiler, which is the single source of truth feeding LLInt, Baseline, DFG, and FTL. A mistake here would affect every JavaScript program run under Bun/JSC. The PR description also notes that preview-build results (running the new test and the existing super-* / tagged-template* stress tests on the built jsc) are still to be posted. Given the criticality of the file and the pending verification step, a human reviewer with JSC bytecode-generation experience should confirm.
Other factors
- The new super handling is not novel: it copies the exact pattern from
BracketAccessorNode::emitBytecode(lines ~1039–1050),FunctionCallBracketNode::emitBytecode(lines ~2196–2233), andFunctionCallDotNode::emitBytecode(lines ~2250–2265) —ensureThis()for the receiver/TDZ check,emitSuperBaseForCallee()for the lookup base, and the receiver as the call'sthis. - The non-super code paths are byte-for-byte equivalent to before:
thisValueremains null, so the resolve/dot/bracket branches emit the same bytecode, and the call falls through to the existingelse if (base)/elsearms. The dot branch's switch from the 3-arg to the 4-argemitGetPropertyValueoverload with a nullthisValueis a no-op — the 3-arg overload is a thin wrapper that does exactly that. - I checked the evaluation-order change in the bracket-super branch:
ensureThis()now runs before bothemitSuperBaseForCallee()and the subscript, whereas the old code emitted the subscript beforeensureThis(). The new order matches the spec (SuperProperty step 2 fetches the this binding before evaluating the Expression) and matchesBracketAccessorNode::emitBytecode. PlacingemitSuperBaseForCallee()before the subscript (rather than after, as the spec's MakeSuperPropertyReference would strictly imply) is not observable because the home object is always an ordinary object, and this ordering already exists inFunctionCallBracketNode. - Test coverage is unusually thorough for a JSC stress test and includes a
testLoopCountloop withnoInlineto push the code through the optimizing tiers. - No prior human or bot review comments to address; CodeRabbit was rate-limited and did not review.
Preview Builds
|
|
Results with the
Upstream |
The tag of a tagged template can be a parenthesized optional chain.
The parentheses end the chain, but the expression they wrap is still
the property reference a?.b, so EvaluateCall (step 1.a.i) calls the tag
with |this| = a, as it does for (a?.b)(). TaggedTemplateNode::emitBytecode
treated the OptionalChainNode as a plain value and called the tag with
|this| = undefined.
Look through the OptionalChainNode when its expression is a property
access, evaluate the base into its own register and use it as |this|.
The chain target is pushed and popped around the lookup of the tag, so
a nullish base still short-circuits the lookup only: the tag and the
base are both undefined then, and the call throws a TypeError as before.
Tags that are not parenthesized optional chains emit the same bytecode
as before.
The bracket and dot branches now also emit the expression info of the
property access before the lookup, as BracketAccessorNode and
DotAccessorNode do, so a failing lookup names the whole access in its
TypeError ('a.b.c' and 'a?.b[k]' instead of the stale 'a.b' or 'k').
This sits on top of the super.tag`...` fix from #438, which
rewrites the same two branches: the receiver of the tag call is now
computed the way FunctionCallDotNode and FunctionCallBracketNode compute
it, for a super base, an optional chain base and a plain base alike.
|
#534 (the |
…fixture oven-sh/WebKit#534 now also carries oven-sh/WebKit#438's commit, which fixes the this of super.tag`...`: both rewrite the same two branches of TaggedTemplateNode::emitBytecode and conflicted. The preview moves to autobuild-preview-pr-534-492e8dd6, and the jsc-stress fixture of #38920 (tagged-templates-super-this.js, its registration and the drainMicrotasks preload global it uses) comes along.
1 participant
Symptom
A tagged template whose tag is a super property is called with the wrong
this.super.tag\x`inside a method invokestagwiththis= the object the property was found on (Base.prototypefor a class, the[[Prototype]]for an object literal method), whilesuper.tag()in the same method invokes it with the receiver. The spec gives both the samethis(EvaluateCall step 1.a.i uses GetThisValue of the Super Reference, which is the method'sthis`), and V8 does that.Same for
super[key]\x`. Found through Bun, whose transpiler leaves tagged templates alone, so any untranspiledsuper.tag`...`` in user code hits this.A second, smaller consequence: in a derived constructor that has not called
super()yet,super[key()]\x`evaluatedkey()before throwing the ReferenceError for the uninitializedthis, whilesuperkey()and a plainsuper[key()]read throw first (the SuperProperty evaluation rule fetches thethisbinding before the key). Upstreammain` has the same code.Cause
TaggedTemplateNode::emitBytecode(bytecompiler/NodesCodegen.cpp) keeps one register,base, for both the object the tag is looked up on and thethisvalue of the call. For the super forms it already looked the property up with the right receiver (emitGetByVal(..., base, thisValue, property)/emitGetPropertyValuewith a super base), but then movedbase, the home object's prototype, into the call'sthisregister.FunctionCallDotNodeandFunctionCallBracketNodekeep the two apart for super.Change
ensureThis()first (which is also thethisTDZ check), thenemitSuperBaseForCallee()for the lookup, and they record thethisregister in a separatethisValue; the call usesthisValuewhen set andbaseotherwise. The dot branch handsthisValueto the existing four argumentemitGetPropertyValueoverload so the lookup and the call share it.thisValuestays null and the call still receivesbase(orundefinedwhen there is no base).Verification
JSTests/stress/tagged-templates-super-this.js:super.tagandsuper[key](variable, string literal, symbol and index keys) in instance methods, through an intermediate class, static methods, object literal methods (also called through an inheriting object), arrow functions, direct eval, instance and static field initializers, generator and async methods, and a derived constructor aftersuper(); a getter on the super base; the template object, raw strings and substitution values still being passed and the template object still being cached per site; substitutions seeing the samethis; primitive receivers passed through unconverted from a strict method and boxed by a sloppy one, both compared againstsuper.tag(); a replaced super base; the TDZ case, asserting that neither the key nor the substitutions run and the tag is never called; and tags that are not super references (o.tag,o["tag"],o[0],base()[key()]with its evaluation order, a plain binding,this.taginside a method) keeping theirthis; plus atestLoopCountloop through the tiers.jscshell of the currentautobuild-f0f60fd2release prebuilt (unfixed): 18 of the 20 groups fail, every super form in every context plus the TDZ ordering (evaluated: key,key); the two non-super groups pass. The same expectations pass on node v26 (V8), apart from the async group, which needs the shell'sdrainMicrotasksand was checked there separately.NodesCodegen.cppcompiles cleanly (-fsyntax-only, clang 21) against the engine headers; results from this PR's preview build (the new test and the existingsuper-*/tagged-template*stress tests on itsjsc) will be added below once it is published, and oven-sh/bun will pin the preview to run the same file underbun.