Bug 200199 – [ESNext] Implement optional chaining

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Ross Kirsling]

[ESNext] Implement optional chaining

[Ross Kirsling]

Created attachment 375035 [details]
Naive approach (double-evaluates LHS)

[Ross Kirsling]

Created attachment 375274 [details]
Patch

[Ross Kirsling]

Created attachment 375356 [details]
Patch

[Darin Adler]

Comment on attachment 375356 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375356&action=review

I’m not 100% qualified to review this, but it looks quite good to me. In particular, I think the test covers enough to give me confidence. The main part I don’t feel qualified to review is the byte code generator part; I am not super solid on understanding register allocation and the proper use of reference counting in the byte code generation.

> Source/JavaScriptCore/parser/Nodes.h:1334
> +        RegisterID* emitBytecode(BytecodeGenerator&, RegisterID* = 0) override;
> +
> +        bool isOptionalChain() const override { return true; }

In new code I think we’d want nullptr and final here rather than 0 and override.

> Source/JavaScriptCore/parser/Parser.cpp:4819
> +            semanticFailIfTrue(newCount, "Cannot call constructor in an optional chain");
> +            semanticFailIfTrue(baseIsSuper, "super is not valid in this context");

Surprised to see inconsistent capitalization here. I guess because "super" is a keyword we can’t capitalize it.

> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:181
> +    // Don't display the ?. of an optional call.
> +    if (idx > 2 && sourceText[idx] == '.' && sourceText[idx - 1] == '?')
> +        idx -= 2;

Which part of the test below covers this?

> JSTests/stress/optional-chaining.js:72
> +shouldBe(({})?.a?.['b'], undefined);

Do we need some test cases with sequences with more than two "?." to make sure we don’t have a mistake in the loops in the code? Or are two guaranteed to be enough?

[Ross Kirsling]

(In reply to Darin Adler from comment #4)
> Comment on attachment 375356 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375356&action=review
> 
> I’m not 100% qualified to review this, but it looks quite good to me. In
> particular, I think the test covers enough to give me confidence. The main
> part I don’t feel qualified to review is the byte code generator part; I am
> not super solid on understanding register allocation and the proper use of
> reference counting in the byte code generation.

Thanks!

> > Source/JavaScriptCore/parser/Nodes.h:1334
> > +        RegisterID* emitBytecode(BytecodeGenerator&, RegisterID* = 0) override;
> > +
> > +        bool isOptionalChain() const override { return true; }
> 
> In new code I think we’d want nullptr and final here rather than 0 and
> override.

Sounds good.

> > Source/JavaScriptCore/parser/Parser.cpp:4819
> > +            semanticFailIfTrue(newCount, "Cannot call constructor in an optional chain");
> > +            semanticFailIfTrue(baseIsSuper, "super is not valid in this context");
> 
> Surprised to see inconsistent capitalization here. I guess because "super"
> is a keyword we can’t capitalize it.

Yeah, I reused an existing message for the baseIsSuper case here, but I wouldn't need to -- "Cannot use super as the base of an optional chain" would work.

> > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:181
> > +    // Don't display the ?. of an optional call.
> > +    if (idx > 2 && sourceText[idx] == '.' && sourceText[idx - 1] == '?')
> > +        idx -= 2;
> 
> Which part of the test below covers this?

Good point, I should check the error messages and not just their types.

> > JSTests/stress/optional-chaining.js:72
> > +shouldBe(({})?.a?.['b'], undefined);
> 
> Do we need some test cases with sequences with more than two "?." to make
> sure we don’t have a mistake in the loops in the code? Or are two guaranteed
> to be enough?

I believe two *suffices*, but I'm not opposed to adding some more thorough cases.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #5)
> I believe two *suffices*, but I'm not opposed to adding some more thorough
> cases.

Ha, turns out two wasn't sufficient at all. This suggestion helped me identify a couple of remaining issues in the logic. :D

[Ross Kirsling]

Created attachment 375536 [details]
Patch

[Ross Kirsling]

Comment on attachment 375536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375536&action=review

Note to self: I suppose a good follow-up task here would be to add "jump if (not) nullish" ops.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2428
> +    generator.emitNode(finalDest.get(), m_expr);

Whoops, this ought to be emitNodeInTailPosition.

[Darin Adler]

Comment on attachment 375536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375536&action=review

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2428
>> +    generator.emitNode(finalDest.get(), m_expr);
> 
> Whoops, this ought to be emitNodeInTailPosition.

At times like this I always think: "What kind of test could I add that would have failed?"

[Ross Kirsling]

(In reply to Darin Adler from comment #9)
> > Whoops, this ought to be emitNodeInTailPosition.
> 
> At times like this I always think: "What kind of test could I add that would
> have failed?"

Indeed. I'll add a couple of cases to tail-call-recognize.js.

[Ross Kirsling]

Created attachment 375646 [details]
Patch

[Ross Kirsling]

Created attachment 375841 [details]
Patch

[Ross Kirsling]

Comment on attachment 375841 [details]
Patch

JITify basic tests and re-run EWS following r248426.

[Yusuke Suzuki]

Comment on attachment 375841 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375841&action=review

Nice! I put quick comment.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4969
> +    m_optionalChainTargetLabelStack.append(newLabel());

Why is stack required? All the labels in m_optionalChainTargetLabelStack is flushed at the same point. So, only holding one label while m_optionalChainDepth > 0 is enough.

[Ross Kirsling]

(In reply to Yusuke Suzuki from comment #14)
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4969
> > +    m_optionalChainTargetLabelStack.append(newLabel());
> 
> Why is stack required? All the labels in m_optionalChainTargetLabelStack is
> flushed at the same point. So, only holding one label while
> m_optionalChainDepth > 0 is enough.

I had convinced myself that labels were single-use because I tried reusing them earlier and the jumps seemed to get all mixed up. Apparently I'm wrong though, because your suggestion appears to work -- thank you!

[Ross Kirsling]

Created attachment 376179 [details]
Patch

[Yusuke Suzuki]

Can we nest the optional chaining? Like,

xxx.?[yyy.?zzz.?aaa].?bbb

Then, what happens when we fail inside []?

[Ross Kirsling]

(In reply to Yusuke Suzuki from comment #17)
> Can we nest the optional chaining? Like,
> 
> xxx.?[yyy.?zzz.?aaa].?bbb
> 
> Then, what happens when we fail inside []?

Hmm, good point.

If we have:
  let x = { undefined: 3 };
  let y = null;

Then:
  x[y?.z]
  > 3

But:
  x?.[y?.z]
  > undefined

Will add tests and fix this afternoon, thanks.

[Yusuke Suzuki]

(In reply to Ross Kirsling from comment #18)
> (In reply to Yusuke Suzuki from comment #17)
> > Can we nest the optional chaining? Like,
> > 
> > xxx.?[yyy.?zzz.?aaa].?bbb
> > 
> > Then, what happens when we fail inside []?
> 
> Hmm, good point.
> 
> If we have:
>   let x = { undefined: 3 };
>   let y = null;
> 
> Then:
>   x[y?.z]
>   > 3
> 
> But:
>   x?.[y?.z]
>   > undefined
> 
> Will add tests and fix this afternoon, thanks.

I think, if the nest happens, we need some sort of stack with scope, and depth needs to be managed by the scope instead of BytecodeGenerator. But if the nest does not happen, we can just put one label and depth in BytecodeGenerator.
And label should be one-per-chain. So, maybe, the scope should have that, and scope should have the depth too.

[Ross Kirsling]

Created attachment 376217 [details]
Patch

[Ross Kirsling]

(In reply to Yusuke Suzuki from comment #19)
> I think, if the nest happens, we need some sort of stack with scope, and
> depth needs to be managed by the scope instead of BytecodeGenerator. But if
> the nest does not happen, we can just put one label and depth in
> BytecodeGenerator.
> And label should be one-per-chain. So, maybe, the scope should have that,
> and scope should have the depth too.

Yep, I reintroduced a stack for this purpose and added an m_isOutermost to OptionalChainNode to mark the nodes that need to interact with that stack.

[Ross Kirsling]

Comment on attachment 376217 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376217&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:963
> +        void beginOptionalChain();
> +        void endOptionalChain(RegisterID* dst, bool isDelete = false);

Self-nit: It'd be clearer to call these push / pop now, as they're now guarded at the callsite.

[Ross Kirsling]

Created attachment 376220 [details]
Patch

[EWS Watchlist]

Comment on attachment 376220 [details]
Patch

Attachment 376220 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12907455

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases

[Ross Kirsling]

(Grr, we really need to deal with that flaky test.)

[Ross Kirsling]

Created attachment 376231 [details]
Patch

[Ross Kirsling]

Comment on attachment 376231 [details]
Patch

(Reuploaded the same patch to make EWS appear green this time...)

[Ross Kirsling]

Created attachment 376265 [details]
Patch

[Ross Kirsling]

Created attachment 376278 [details]
Patch

[Ross Kirsling]

Added tests for (a?.b)?.() and (a?.b)() now too.

Any other concerns, Yusuke?

[Ross Kirsling]

Ping?

[Darin Adler]

I would love to review+ but I think Yusuke should.

[Yusuke Suzuki]

Comment on attachment 376278 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 376278 [details]
Patch

Clearing flags on attachment: 376278

Committed r248829: <https://trac.webkit.org/changeset/248829>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54437845>