Bug 200072 - [ESNext] Implement nullish coalescing
Summary: [ESNext] Implement nullish coalescing
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-23 22:10 PDT by Ross Kirsling
Modified: 2019-08-18 20:58 PDT (History)
14 users (show)

See Also:


Attachments
Patch (16.76 KB, patch)
2019-07-23 22:29 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (16.30 KB, patch)
2019-07-23 23:25 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (18.86 KB, patch)
2019-07-24 09:47 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (18.86 KB, patch)
2019-07-24 12:50 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (19.02 KB, patch)
2019-07-24 15:08 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (16.32 KB, patch)
2019-07-24 20:52 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (16.33 KB, patch)
2019-07-24 21:10 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (19.11 KB, patch)
2019-07-24 21:11 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-23 22:10:46 PDT
[ESNext] Implement nullish coalescing
Comment 1 Ross Kirsling 2019-07-23 22:29:02 PDT
Created attachment 374762 [details]
Patch
Comment 2 Ross Kirsling 2019-07-23 23:25:39 PDT
Created attachment 374765 [details]
Patch
Comment 3 Maciej Stachowiak 2019-07-24 00:14:18 PDT
Comment on attachment 374765 [details]
Patch

Feature flag, please! (Looks like at least a compile-time flag should be practical, and maybe even runtime if it wasn't too much of a perf hit).
Comment 4 Ross Kirsling 2019-07-24 00:17:50 PDT
(In reply to Maciej Stachowiak from comment #3)
> Comment on attachment 374765 [details]
> Patch
> 
> Feature flag, please! (Looks like at least a compile-time flag should be
> practical, and maybe even runtime if it wasn't too much of a perf hit).

Will do!
Comment 5 Ross Kirsling 2019-07-24 09:47:01 PDT
Created attachment 374782 [details]
Patch
Comment 6 Ross Kirsling 2019-07-24 09:48:40 PDT
Comment on attachment 374782 [details]
Patch

I believe it should suffice to simply refuse to lex the token -- let me know if we'd like to do this differently.

(I'll probably also want to update this flag to be useNullishAwareOperators in the ?. patch but there's no need to jump the gun here.)
Comment 7 Ross Kirsling 2019-07-24 12:50:47 PDT
Created attachment 374795 [details]
Patch
Comment 8 Caio Lima 2019-07-24 13:48:16 PDT
Comment on attachment 374795 [details]
Patch

LGTM overall. I think it would be good have tests covering short circuiting.
Comment 9 Ross Kirsling 2019-07-24 15:08:57 PDT
Created attachment 374817 [details]
Patch
Comment 10 Ross Kirsling 2019-07-24 15:09:57 PDT
(In reply to Caio Lima from comment #8)
> Comment on attachment 374795 [details]
> Patch
> 
> LGTM overall. I think it would be good have tests covering short circuiting.

Good call! Added these as well as a makeMasquerader case to cover the document.all behavior.
Comment 11 Darin Adler 2019-07-24 18:46:17 PDT
Comment on attachment 374817 [details]
Patch

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

> Source/JavaScriptCore/parser/ASTBuilder.h:1397
> +        return new (m_parserArena) CoalesceNode(location, lhs.first, rhs.first);

What’s the pro and con for extending LogicalOpNode instead adding a new class?

> Source/JavaScriptCore/parser/ResultType.h:184
> +            if (op1.definitelyIsBoolean() || op1.definitelyIsNumber() || op1.definitelyIsString() || op1.definitelyIsBigInt())

Could this be "definitelyIsNotNull" instead? I think we could possibly correctly cover more cases and have more maintainable code that way.
Comment 12 Ross Kirsling 2019-07-24 19:19:35 PDT
Thanks for the review!

(In reply to Darin Adler from comment #11)
> > Source/JavaScriptCore/parser/ASTBuilder.h:1397
> > +        return new (m_parserArena) CoalesceNode(location, lhs.first, rhs.first);
> 
> What’s the pro and con for extending LogicalOpNode instead adding a new
> class?

Hmm, we could do that given how similar the emitBytecode implementation is, but the emitBytecodeInConditionContext would not apply (I guess we'd just early out to emitBytecode) and we'd still call a different ResultType function.

I personally thought the separation might be clearer, but do you have a preference? 

> > Source/JavaScriptCore/parser/ResultType.h:184
> > +            if (op1.definitelyIsBoolean() || op1.definitelyIsNumber() || op1.definitelyIsString() || op1.definitelyIsBigInt())
> 
> Could this be "definitelyIsNotNull" instead? I think we could possibly
> correctly cover more cases and have more maintainable code that way.

We could do definitelyIsNotNullOrUndefined by checking against TypeMaybeNull and TypeMaybeOther, sure.
Comment 13 Ross Kirsling 2019-07-24 20:52:18 PDT
Created attachment 374860 [details]
Patch for landing
Comment 14 Ross Kirsling 2019-07-24 21:10:28 PDT
Created attachment 374863 [details]
Patch for landing
Comment 15 Ross Kirsling 2019-07-24 21:11:09 PDT
Created attachment 374864 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2019-07-25 00:50:52 PDT
Comment on attachment 374864 [details]
Patch for landing

Clearing flags on attachment: 374864

Committed r247819: <https://trac.webkit.org/changeset/247819>
Comment 17 WebKit Commit Bot 2019-07-25 00:50:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2019-07-25 00:51:18 PDT
<rdar://problem/53531846>