Repository navigation
Conversation
Preview Builds
|
![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 control flow for optional-chain callees in ASTBuilder/NodesCodegen — a language-semantics change on paths owned by JSC reviewers — a human look is still worthwhile.
What was reviewed:
- Traced the new
pushOptionalChainTarget/emitOptionalChainCalleeEndflow in bothFunctionCallDotNodeandFunctionCallBracketNodefor single-link ((a?.b)()) and multi-link ((a?.b.c)(),(a?.b[k])()) short-circuits — the landing pad writes bothfunctionandthisRegisterbefore the call, so no uninitialized temporaries reachemitCallInTailPosition. - Checked
makeFunctionCallNode: the unwrap only fires whenexpr()->isLocation()and!isOptionalCall, so(a?.b)?.()and(a?.())()keep their existing shapes; the.call/.apply/hasOwnPropertyspecial nodes are correctly bypassed when the flag is set. - Confirmed the change is bytecode-level (tier-agnostic) and that the new stress test throws on failure and uses
testLoopCountper JSTests conventions.
Extended reasoning...
Overview
This PR fixes a spec-compliance bug in JavaScriptCore's handling of (a?.b)() when a is nullish: parentheses terminate the optional chain, so the call must still evaluate its arguments and throw a TypeError rather than short-circuiting to undefined. The fix touches parser/ASTBuilder.h (no longer wraps the call inside the callee's OptionalChainNode; instead unwraps to the inner Dot/Bracket accessor and sets a new m_calleeIsOptionalChain flag), parser/Nodes.h (adds the flag and setter to FunctionCallDotNode and FunctionCallBracketNode), and bytecompiler/NodesCodegen.cpp (both emitBytecode methods push an optional-chain target before evaluating the base and, via a new emitOptionalChainCalleeEnd helper, emit a jump over a landing pad that loads undefined into the function and this registers). A 469-line stress test and three assertions in the existing optional-chaining.js cover dot/bracket/private-name callees, multi-link chains, .call/.apply/.hasOwnProperty, argument evaluation order, tail calls, generators/async, and a testLoopCount tier-up loop.
Security risks
No direct security surface (auth, crypto, permissions) is touched. The relevant risk class for bytecode-compiler changes is uninitialized-register reads or unbalanced control flow. I traced the short-circuit paths: for (a?.b.c)() and (a?.b[k])() the jump lands at popOptionalChainTarget() where both function and callArguments.thisRegister() are explicitly loaded with undefined before the call, so the previously-unwritten this temporary is never read uninitialized. The push/pop of the optional-chain target is balanced within each emitBytecode (the flag is only set when !isOptionalCall, asserted at the pop site). The OptionalChainNode wrapper is discarded rather than reused, so there's no double-emission of its target.
Level of scrutiny
High. This is a semantics change in the JavaScript engine's AST→bytecode pipeline that affects every execution tier and is on a CODEOWNERS-covered path (/Source/JavaScriptCore and /JSTests → @ WebKit/jsc-reviewers). Per the approval guidelines, CODEOWNER-covered paths and language-semantics changes should not be auto-approved. The PR also intentionally disables the specialized CallFunctionCallDotNode/ApplyFunctionCallDotNode/HasOwnPropertyFunctionCallDotNode fast paths for the parenthesized-chain shape — a deliberate perf/correctness trade-off a JSC reviewer should sign off on.
Other factors
The change is well-scoped, thoroughly commented, and mirrors the approach the PR describes for V8 and SpiderMonkey. Test coverage is extensive and follows JSTests conventions (throws on failure, uses testLoopCount, no logging). The bug hunter ran to a dry streak with no findings. I found no correctness issues myself, but the combination of CODEOWNERS coverage and the nature of the change (bytecode control-flow edit affecting all four tiers) means a human JSC reviewer should confirm.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughThe parser now represents optional-chain callees explicitly. Bytecode generation handles short-circuited parenthesized calls. Stress tests cover synchronous, asynchronous, private-name, context-sensitive, and optimized execution paths. ChangesOptional-chain call compilation
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable regression is currently established, so the change is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides a detailed problem statement, technical fix, behavior details, affected scenarios, and test results. However, it omits required template information: a Bugzilla bug title and URL, a “Reviewed by NOBODY (OOPS!).” line, and a clear path/function change list.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
29e9338 to
13bd3c9
Compare
robobun
changed the title
(a?.b)() with a nullish a throws a TypeError instead of skipping the call(a?.b)(): a nullish a throws a TypeError, and a class field initializer makes the call
|
oven-sh/bun#42684 pins the preview The full |
… initializer makes the call The parentheses in `(a?.b)()` end the optional chain, so the call is not part of it. When `a` is nullish, `(a?.b)` is the value undefined and EvaluateCall throws a TypeError after it evaluated the arguments (https://tc39.es/ecma262/#sec-evaluatecall). V8 and SpiderMonkey do that. JSC returned undefined and did not evaluate the arguments: const o = undefined; (o?.a)(); // JSC: undefined. Expected: TypeError (o?.a.b)(); // same (o?.[0])(f()); // same, and f is not called ((o?.a))(); // same The cause is in ASTBuilder::makeFunctionCallNode. `(a?.b)()` has to call `b` with |this| = `a`, so the function looks through the OptionalChainNode of the callee to make a FunctionCallDotNode or FunctionCallBracketNode. It then put that call node inside the OptionalChainNode. That is the tree of `a?.b()`, so the short-circuit jump also skipped the call. The node that came back was also the OptionalChainNode of `a?.b`, which starts at `a` and not at the opening parenthesis. A class field initializer is parsed a second time from the position of its node, and that parse stopped at the closing parenthesis. So `class C { x = (o?.f)(); }` with a present `o` stored `o.f` in `x` and never called it. The call node is no longer put inside the OptionalChainNode. The two generic call nodes get a flag, calleeIsOptionalChain. With the flag, emitBytecode pushes the optional chain target before it evaluates the base and pops it right after the lookup of the callee. At the target the callee and |this| are loaded with undefined. The arguments are evaluated after that point and the call throws, with the message of any other call of undefined: "(o?.a) is not a function. (In '(o?.a)()', '(o?.a)' is undefined)". |this| has to be written at the target because a chain that short-circuits before its last link, as in `(a?.b.c)()`, never wrote that register. The nodes for `.call()`, `.apply()` and `.hasOwnProperty()` do not know the flag, so `(a?.call)()` and the like now get the generic FunctionCallDotNode. A present base emits the same instructions as before plus one jump over the two loads. `a?.b()`, `a?.b?.()` and `(a?.b)?.()` are each one chain and emit the same instructions as before. * JSTests/stress/optional-chaining-parenthesized-call.js: Added. * JSTests/stress/optional-chaining.js: * Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp: (JSC::emitOptionalChainCalleeEnd): (JSC::FunctionCallBracketNode::emitBytecode): (JSC::FunctionCallDotNode::emitBytecode): * Source/JavaScriptCore/parser/ASTBuilder.h: (JSC::ASTBuilder::makeFunctionCallNode): * Source/JavaScriptCore/parser/Nodes.h: (JSC::FunctionCallBracketNode::setCalleeIsOptionalChain): (JSC::FunctionCallDotNode::setCalleeIsOptionalChain):
13bd3c9 to
85a710c
Compare
The WebKit branch now sits on cf1b36ec, the commit this repo pinned before. The preview differs from that pin by the one commit of oven-sh/WebKit#650 and no longer carries oven-sh/WebKit#634 and oven-sh/WebKit#636.
1 participant
Problem
(a?.b)()with a nullishareturnsundefined. The spec, V8 and SpiderMonkey run the arguments, then throw a TypeError.x = (a?.b)()with a presentastoresa.band never calls it. TypeScript(a?.b as F)()and(a?.b)!()print as this shape.ASTBuilder::makeFunctionCallNode(parser/ASTBuilder.h:1481) puts the call node back inside theOptionalChainNodeofa?.b. That is the tree ofa?.b(): the short-circuit jump skips the call. The node also starts ata, and a field initializer is parsed again from its node's start, so that parse stops at the).Fix
OptionalChainNode.FunctionCallDotNodeandFunctionCallBracketNodeget a flag,calleeIsOptionalChain.emitBytecodepushes the chain target before the base and pops it right after the callee lookup. The target loadsundefinedinto the callee andthis. The arguments run, then the call throws(o?.a) is not a function.(a?.call)(),(a?.apply)()and(a?.hasOwnProperty)(k)get the genericFunctionCallDotNode(the special nodes ignore the flag).JSTests/stress/optional-chaining-parenthesized-call.js(new) fails on thejscofautobuild-cf1b36ecand passes here. FullJSTests/stressand a test262 subset: no other change.Background
a?.b.cgivesundefinedand skips the rest whenais nullish. Parentheses end the chain, but(a?.b)is still a property reference: the call getsthis=a.OptionalChainNodepushes a jump target. Each?.base emitsjundefined_or_nullto it. The target loadsundefined.(a?.b.c)()that short-circuits atanever writes thethisregister, so the target writes it too.Notes
(a?.b.c)(),(a?.[k])()and((a?.b))()behave like(a?.b)(). Nobody reported either symptom: both came up in tests of bun's decorator lowering. Upstreammainhas the same code (since the 2019 implementation of optional chaining), so a bugs.webkit.org report and an upstream patch are still to do. The tagged template(a?.b)``,(a?.b).cand `(0, a?.b)()` already throw.Bytecode of
function f(o, x) { return (o?.a)(x); }with this change:jmpover the two loads.a?.b(),a?.b?.()and(a?.b)?.()are each one chain and emit the same instructions as before.BytecodeGenerator::VisitCallforNAMED_OPTIONAL_CHAIN_PROPERTY_CALLandKEYED_OPTIONAL_CHAIN_PROPERTY_CALL:BuildOptionalChainwraps only the receiver and the property load. SpiderMonkey does it inBytecodeEmitter::emitCalleeAndThisForOptionalChain, which sets the callee andthistoundefinedat the jump target.language/expressions/optional-chaining/optional-call-preserves-this.jsfor thethisof(a?.b)(). It has no case for a nullisha.PropertyListNode::emitBytecoderecordsinitializer->position()andparseClassFieldInitializerSourceElementsparses again from there.makeFunctionCallNodereturned theOptionalChainNodethatparseMemberExpressionmade for the inside of the parentheses, which starts ata. The call node it returns now starts at the opening parenthesis.class G { y = delete o?.g }givesythe value ofo?.gand deletes nothing.makeDeleteNodereturns theOptionalChainNodetoo, and that one starts after thedeletekeyword....with |this| = a, and super.tag...with the method's this #534 fixes thethisof the tagged template(a?.b)`...`with the same push, pop and two loads inTaggedTemplateNode::emitBytecode. It names this call bug as out of scope. The two changes touch different functions.recordCallOrApplyDepthinParser.cpponly counts a callee that is aDotAccessorNode, so(a?.call)((b?.call)(...))gotCallFunctionCallDotNodewith depth 0. With the generic node that shape has no exponential code size.undefinedandnullbases, dot, bracket, index, symbol and private name callees (method, field, getter, static method), double parentheses, short-circuits at each link of a longer chain, the argument evaluation order and an argument that throws,call,applyandhasOwnProperty, locals that keep their value after a short-circuit, strict tail calls, arrow, generator (with ayieldin the arguments), async,evalandnew Functioncontexts, condition,??,||,typeof,deleteand template contexts,new (a?.b)(), a super property andnew.targetas the base, instance, static, private and computed class fields, and atestLoopCountloop through the tiers with a present base first and a nullish base after.jsc: the new test and the threeoptional-chaining*tests pass with the default options,--useJIT=false,--useLLInt=false,--useDFGJIT=false,--useFTLJIT=false, eager DFG and FTL thresholds with--useConcurrentJIT=false,--validateBytecode=true --validateGraph=true --validateGraphAtEachPhase=true,--forceDebuggerBytecodeGeneration=true, and--useTypeProfiler=true --useControlFlowProfiler=true.cf1b36ec, the commit oven-sh/bun pins now, so that the preview differs from that pin by this commit only. The first two pushes sat on28f58fb0.JSTests/stress(5833 files, default options plus--useDollarVM=true, 120 s per file, 10 in parallel) on the releasejscofautobuild-cf1b36ecand of the first preview of this PR (29e9338a, on28f58fb0, sameSource/change): the files that differ are the new test,optional-chaining.js, the new test of [JSC] A jettison for a dead weak reference counts toward the reoptimization back-off #634 (not in the old pin), onememoryHog!test that was killed under load and passes alone on both, and one timeout under load.language/expressions/{optional-chaining,call,tagged-template,coalesce,delete,new,super,property-accessors,grouping,typeof}andlanguage/{statements,expressions}/class/elements, sloppy and strict (6733 runs): the same 16 known failures before and after.