Bug 200199 - [ESNext] Implement optional chaining
Summary: [ESNext] Implement optional chaining
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-27 16:29 PDT by Ross Kirsling
Modified: 2019-08-18 20:58 PDT (History)
13 users (show)

See Also:


Attachments
Naive approach (double-evaluates LHS) (31.33 KB, patch)
2019-07-27 16:30 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (60.00 KB, patch)
2019-07-31 17:59 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (60.13 KB, patch)
2019-08-01 15:22 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (62.29 KB, patch)
2019-08-05 11:06 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (63.46 KB, patch)
2019-08-06 13:18 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (63.77 KB, patch)
2019-08-08 14:55 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (63.36 KB, patch)
2019-08-13 10:13 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (63.64 KB, patch)
2019-08-13 14:57 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (63.66 KB, patch)
2019-08-13 15:38 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (63.66 KB, patch)
2019-08-13 20:10 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (63.60 KB, patch)
2019-08-14 08:13 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (63.72 KB, patch)
2019-08-14 10:37 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2019-07-27 16:29:10 PDT
[ESNext] Implement optional chaining
Comment 1 Ross Kirsling 2019-07-27 16:30:11 PDT
Created attachment 375035 [details]
Naive approach (double-evaluates LHS)
Comment 2 Ross Kirsling 2019-07-31 17:59:09 PDT
Created attachment 375274 [details]
Patch
Comment 3 Ross Kirsling 2019-08-01 15:22:40 PDT
Created attachment 375356 [details]
Patch
Comment 4 Darin Adler 2019-08-04 10:58:03 PDT
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?
Comment 5 Ross Kirsling 2019-08-04 17:34:36 PDT
(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.
Comment 6 Ross Kirsling 2019-08-05 11:01:38 PDT
(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
Comment 7 Ross Kirsling 2019-08-05 11:06:05 PDT
Created attachment 375536 [details]
Patch
Comment 8 Ross Kirsling 2019-08-06 10:02:29 PDT
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.
Comment 9 Darin Adler 2019-08-06 11:32:53 PDT
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?"
Comment 10 Ross Kirsling 2019-08-06 12:37:28 PDT
(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.
Comment 11 Ross Kirsling 2019-08-06 13:18:10 PDT
Created attachment 375646 [details]
Patch
Comment 12 Ross Kirsling 2019-08-08 14:55:52 PDT
Created attachment 375841 [details]
Patch
Comment 13 Ross Kirsling 2019-08-08 14:56:57 PDT
Comment on attachment 375841 [details]
Patch

JITify basic tests and re-run EWS following r248426.
Comment 14 Yusuke Suzuki 2019-08-13 04:27:15 PDT
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.
Comment 15 Ross Kirsling 2019-08-13 10:11:17 PDT
(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!
Comment 16 Ross Kirsling 2019-08-13 10:13:51 PDT
Created attachment 376179 [details]
Patch
Comment 17 Yusuke Suzuki 2019-08-13 12:19:32 PDT
Can we nest the optional chaining? Like,

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

Then, what happens when we fail inside []?
Comment 18 Ross Kirsling 2019-08-13 12:50:14 PDT
(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.
Comment 19 Yusuke Suzuki 2019-08-13 13:21:02 PDT
(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.
Comment 20 Ross Kirsling 2019-08-13 14:57:57 PDT
Created attachment 376217 [details]
Patch
Comment 21 Ross Kirsling 2019-08-13 15:01:15 PDT
(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.
Comment 22 Ross Kirsling 2019-08-13 15:12:01 PDT Comment hidden (obsolete)
Comment 23 Ross Kirsling 2019-08-13 15:38:34 PDT Comment hidden (obsolete)
Comment 24 Build Bot 2019-08-13 17:54:36 PDT Comment hidden (obsolete)
Comment 25 Ross Kirsling 2019-08-13 18:14:40 PDT Comment hidden (obsolete)
Comment 26 Ross Kirsling 2019-08-13 20:10:09 PDT Comment hidden (obsolete)
Comment 27 Ross Kirsling 2019-08-13 20:12:03 PDT Comment hidden (obsolete)
Comment 28 Ross Kirsling 2019-08-14 08:13:02 PDT
Created attachment 376265 [details]
Patch
Comment 29 Ross Kirsling 2019-08-14 10:37:25 PDT
Created attachment 376278 [details]
Patch
Comment 30 Ross Kirsling 2019-08-14 10:39:25 PDT
Added tests for (a?.b)?.() and (a?.b)() now too.

Any other concerns, Yusuke?
Comment 31 Ross Kirsling 2019-08-16 11:17:26 PDT
Ping?
Comment 32 Darin Adler 2019-08-17 17:08:17 PDT
I would love to review+ but I think Yusuke should.
Comment 33 Yusuke Suzuki 2019-08-17 22:03:37 PDT
Comment on attachment 376278 [details]
Patch

r=me
Comment 34 WebKit Commit Bot 2019-08-17 22:54:20 PDT
Comment on attachment 376278 [details]
Patch

Clearing flags on attachment: 376278

Committed r248829: <https://trac.webkit.org/changeset/248829>
Comment 35 WebKit Commit Bot 2019-08-17 22:54:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Radar WebKit Bug Importer 2019-08-17 22:55:50 PDT
<rdar://problem/54437845>