Skip to content

Commit 63b7d62

Browse files
sosukesuzukisosukesuzuki
committed
[JSC] Keep the call in (a?.b)(x) outside the optional chain
https://bugs.webkit.org/show_bug.cgi?id=325104 Reviewed by Keith Miller. JSC parses both `(a?.b)(x)` and `a?.b(x)` into OptionalChain(Call(a.b, x)) The parentheses end the optional chain, so the call in `(a?.b)(x)` is not part of it [1]. When `a` is null or undefined, `(a?.b)` evaluates to undefined, and the outer call still has to evaluate `x` and throw a TypeError. JSC instead skipped both the argument evaluation and the call, and returned undefined. The call was pulled into the chain to keep `this`: an OptionalChainNode is not a location, so calling it directly would use an undefined `this` even when `a` is not nullish. This patch parses `(a?.b)(x)` into Call(OptionalChain(a.b), x) and adds OptionalChainNode::emitCallee, which loads the callee and `this` inside the chain. `this` is still `a` when the chain does not short-circuit. When it does, the callee is undefined and evaluation continues with the arguments and the call. Two other behavior changes follow from the new shape. `(a?.b)() = 1` in sloppy mode is now a runtime ReferenceError like `(a.b)() = 1`, as specified for function call assignment targets [2], instead of a SyntaxError. And `x = (o?.f)()` in a class field initializer no longer drops the call. [1]: https://tc39.es/ecma262/#sec-optional-chains [2]: https://tc39.es/ecma262/#sec-runtime-errors-for-function-call-assignment-targets Test: JSTests/stress/optional-chaining-parenthesized-call.js * JSTests/stress/optional-chaining-parenthesized-call.js: Added. (shouldBe): (const.arg): (const.key): (shouldThrowTypeError): (shouldThrowReferenceError): (Private.prototype.method): (Private.call): (Private): (f): (const.receiver.b): (const.receiver.c.d): (Field): (testConditionContext): (testNullish): (testNonNullish): * Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp: (JSC::FunctionCallValueNode::emitBytecode): (JSC::OptionalChainNode::emitCallee): * Source/JavaScriptCore/parser/ASTBuilder.h: (JSC::ASTBuilder::makeFunctionCallNode): * Source/JavaScriptCore/parser/Nodes.h: Canonical link: https://commits.webkit.org/321867@main
1 parent 2730e16 commit 63b7d62

4 files changed

Lines changed: 186 additions & 6 deletions

File tree

‎JSTests/stress/optional-chaining-parenthesized-call.js‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
function shouldBe(actual, expected) {
2+
if (actual !== expected)
3+
throw new Error(`expected ${String(expected)} but got ${String(actual)}`);
4+
}
5+
6+
const log = [];
7+
const arg = () => { log.push('arg'); return 1; };
8+
const key = () => { log.push('key'); return 'b'; };
9+
10+
function shouldThrowTypeError(func, expectedLog) {
11+
log.length = 0;
12+
let error;
13+
try {
14+
func();
15+
} catch (e) {
16+
error = e;
17+
}
18+
19+
if (!(error instanceof TypeError))
20+
throw new Error(`Expected TypeError but got ${error}`);
21+
shouldBe(log.join(), expectedLog);
22+
}
23+
24+
function shouldThrowReferenceError(source) {
25+
let error;
26+
try {
27+
eval(source);
28+
} catch (e) {
29+
error = e;
30+
}
31+
32+
if (!(error instanceof ReferenceError))
33+
throw new Error(`Expected ReferenceError from ${source} but got ${error}`);
34+
}
35+
36+
class Private {
37+
#method() { return this; }
38+
static call(o) { return (o?.#method)(); }
39+
}
40+
41+
function f() { return this; }
42+
43+
const receiver = {
44+
b(x) { return this === receiver ? x : undefined; },
45+
c: { d() { return this; } },
46+
ownProperty: 1,
47+
};
48+
49+
class Field {
50+
x = (receiver?.b)(8);
51+
static y = (receiver?.b)(9);
52+
}
53+
54+
class NullishField {
55+
static base = null;
56+
x = (NullishField.base?.b)(arg());
57+
}
58+
59+
function testTailPosition(base) {
60+
"use strict";
61+
return (base?.b)(1);
62+
}
63+
64+
function testConditionContext(base) {
65+
if ((base?.b)(1))
66+
return true;
67+
return false;
68+
}
69+
70+
function testNullish(base) {
71+
shouldThrowTypeError(() => (base?.b)(arg()), 'arg');
72+
shouldThrowTypeError(() => (base?.b.c.d)(arg()), 'arg');
73+
shouldThrowTypeError(() => (base?.b?.c)(arg()), 'arg');
74+
shouldThrowTypeError(() => (base?.['b'])(arg()), 'arg');
75+
shouldThrowTypeError(() => (base?.[key()])(arg()), 'arg');
76+
shouldThrowTypeError(() => (base?.call)(arg()), 'arg');
77+
shouldThrowTypeError(() => (base?.b)(arg())?.c, 'arg');
78+
shouldThrowTypeError(() => (base?.b)(...[arg()]), 'arg');
79+
NullishField.base = base;
80+
shouldThrowTypeError(() => new NullishField, 'arg');
81+
shouldThrowTypeError(() => Private.call(base), '');
82+
shouldThrowTypeError(() => testTailPosition(base), '');
83+
shouldThrowTypeError(() => testConditionContext(base), '');
84+
85+
log.length = 0;
86+
shouldBe((base?.b)?.(arg()), undefined);
87+
shouldBe(base?.b(arg()), undefined);
88+
shouldBe(base?.b?.(arg()), undefined);
89+
shouldBe(base?.[key()](arg()), undefined);
90+
shouldBe(log.length, 0);
91+
}
92+
93+
function testNonNullish() {
94+
const thisValue = { thisValue: true };
95+
96+
shouldBe((receiver?.b)(2), 2);
97+
shouldBe((receiver?.['b'])(3), 3);
98+
shouldBe((receiver?.b)(...[5]), 5);
99+
shouldBe((receiver?.b)?.(6), 6);
100+
shouldBe((receiver?.['b'])?.(7), 7);
101+
shouldBe(new Field().x, 8);
102+
shouldBe(Field.y, 9);
103+
let clobbered = receiver;
104+
shouldBe((clobbered?.[(clobbered = null, 'b')])(4), 4);
105+
shouldBe((receiver?.c?.d)(), receiver.c);
106+
shouldBe((receiver?.c.d)(), receiver.c);
107+
shouldBe((receiver?.c).d(), receiver.c);
108+
shouldBe((f?.call)(thisValue), thisValue);
109+
shouldBe((receiver?.hasOwnProperty)('ownProperty'), true);
110+
const privateInstance = new Private;
111+
shouldBe(Private.call(privateInstance), privateInstance);
112+
shouldBe(testTailPosition(receiver), 1);
113+
shouldBe(testConditionContext({ b() { return true; } }), true);
114+
shouldBe(testConditionContext({ b() { return false; } }), false);
115+
shouldThrowTypeError(() => (receiver?.missing)(arg()), 'arg');
116+
}
117+
118+
function testAssignmentTarget() {
119+
shouldThrowReferenceError('(receiver?.b)(1) = 1');
120+
shouldThrowReferenceError('(receiver?.b)(1)++');
121+
shouldThrowReferenceError('for ((receiver?.b)(1) in { x: 1 }) ;');
122+
}
123+
124+
testAssignmentTarget();
125+
126+
for (let i = 0; i < testLoopCount; ++i) {
127+
testNullish(i % 2 ? null : undefined);
128+
testNonNullish();
129+
}

‎Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1399,6 +1399,18 @@ RegisterID* FunctionCallValueNode::emitBytecode(BytecodeGenerator& generator, Re
13991399
return ret;
14001400
}
14011401

1402+
OptionalChainNode* optionalChain = m_expr->isOptionalChain() ? static_cast<OptionalChainNode*>(m_expr) : nullptr;
1403+
if (optionalChain && optionalChain->expr()->isLocation()) {
1404+
ASSERT(!isOptionalCall());
1405+
RefPtr<RegisterID> func = generator.tempDestination(dst);
1406+
RefPtr<RegisterID> returnValue = generator.finalDestination(dst, func.get());
1407+
CallArguments callArguments(generator, m_args);
1408+
optionalChain->emitCallee(generator, func.get(), callArguments.thisRegister());
1409+
RegisterID* ret = generator.emitCallInTailPosition(returnValue.get(), func.get(), NoExpectedFunction, callArguments, divot(), divotStart(), divotEnd(), DebuggableCall::Yes);
1410+
generator.emitProfileType(returnValue.get(), divotStart(), divotEnd());
1411+
return ret;
1412+
}
1413+
14021414
RefPtr<RegisterID> func = nullptr;
14031415
if (m_args && m_args->hasAssignments())
14041416
func = generator.newTemporary();
@@ -3599,6 +3611,46 @@ RegisterID* OptionalChainNode::emitBytecode(BytecodeGenerator& generator, Regist
35993611
return finalDest.unsafeGet();
36003612
}
36013613

3614+
void OptionalChainNode::emitCallee(BytecodeGenerator& generator, RegisterID* function, RegisterID* thisRegister)
3615+
{
3616+
ASSERT(m_isOutermost);
3617+
3618+
generator.pushOptionalChainTarget();
3619+
if (m_expr->isBracketAccessorNode()) {
3620+
BracketAccessorNode* bracket = static_cast<BracketAccessorNode*>(m_expr);
3621+
ASSERT(!bracket->base()->isSuperNode());
3622+
generator.emitNode(thisRegister, bracket->base());
3623+
if (bracket->base()->isOptionalChainBase())
3624+
generator.emitOptionalCheck(thisRegister);
3625+
3626+
if (isNonIndexStringElement(*bracket->subscript())) {
3627+
generator.emitExpressionInfo(bracket->divot(), bracket->divotStart(), bracket->divotEnd());
3628+
generator.emitGetById(function, thisRegister, static_cast<StringNode*>(bracket->subscript())->value());
3629+
} else {
3630+
RefPtr<RegisterID> property = generator.emitNodeForProperty(bracket->subscript());
3631+
generator.emitExpressionInfo(bracket->divot(), bracket->divotStart(), bracket->divotEnd());
3632+
generator.emitGetByVal(function, thisRegister, property.get());
3633+
}
3634+
} else {
3635+
ASSERT(m_expr->isDotAccessorNode());
3636+
DotAccessorNode* dot = static_cast<DotAccessorNode*>(m_expr);
3637+
ASSERT(!dot->base()->isSuperNode());
3638+
generator.emitNode(thisRegister, dot->base());
3639+
if (dot->base()->isOptionalChainBase())
3640+
generator.emitOptionalCheck(thisRegister);
3641+
3642+
generator.emitExpressionInfo(dot->divot(), dot->divotStart(), dot->divotEnd());
3643+
dot->emitGetPropertyValue(generator, function, thisRegister);
3644+
}
3645+
3646+
Ref<Label> endLabel = generator.newLabel();
3647+
generator.emitJump(endLabel.get());
3648+
generator.popOptionalChainTarget();
3649+
generator.emitLoad(function, jsUndefined());
3650+
generator.emitLoad(thisRegister, jsUndefined());
3651+
generator.emitLabel(endLabel.get());
3652+
}
3653+
36023654
void OptionalChainNode::emitBytecodeInConditionContext(BytecodeGenerator& generator, Label& trueTarget, Label& falseTarget, FallThroughMode fallThroughMode)
36033655
{
36043656
if (needsDebugHook()) [[unlikely]]

‎Source/JavaScriptCore/parser/ASTBuilder.h‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1473,15 +1473,12 @@ ExpressionNode* ASTBuilder::makeFunctionCallNode(const JSTokenLocation& location
14731473
return new (m_parserArena) BytecodeIntrinsicNode(BytecodeIntrinsicNode::Type::Function, location, intrinsic->entry(), intrinsic->identifier(), args, divot, divotStart, divotEnd);
14741474
}
14751475

1476-
if (func->isOptionalChain()) {
1476+
if (isOptionalCall && func->isOptionalChain()) {
14771477
OptionalChainNode* optionalChain = static_cast<OptionalChainNode*>(func);
14781478
if (optionalChain->expr()->isLocation()) {
14791479
ASSERT(!optionalChain->expr()->isResolveNode());
1480-
// We must take care to preserve our `this` value in cases like `a?.b?.()` and `(a?.b)()`, respectively.
1481-
if (isOptionalCall)
1482-
return makeFunctionCallNode(location, optionalChain->expr(), previousBaseWasSuper, args, divotStart, divot, divotEnd, callOrApplyChildDepth, isOptionalCall);
1483-
optionalChain->setExpr(makeFunctionCallNode(location, optionalChain->expr(), previousBaseWasSuper, args, divotStart, divot, divotEnd, callOrApplyChildDepth, isOptionalCall));
1484-
return optionalChain;
1480+
// We must take care to preserve our `this` value in cases like `a?.b?.()`.
1481+
return makeFunctionCallNode(location, optionalChain->expr(), previousBaseWasSuper, args, divotStart, divot, divotEnd, callOrApplyChildDepth, isOptionalCall);
14851482
}
14861483
}
14871484

‎Source/JavaScriptCore/parser/Nodes.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1450,6 +1450,8 @@ namespace JSC {
14501450
void setExpr(ExpressionNode* expr) { m_expr = expr; }
14511451
ExpressionNode* expr() const { return m_expr; }
14521452

1453+
void emitCallee(BytecodeGenerator&, RegisterID* function, RegisterID* thisRegister);
1454+
14531455
private:
14541456
RegisterID* emitBytecode(BytecodeGenerator&, RegisterID* = nullptr) final;
14551457
void emitBytecodeInConditionContext(BytecodeGenerator&, Label& trueTarget, Label& falseTarget, FallThroughMode) final;

0 commit comments

Comments
 (0)