Bug 214297

Summary: [JSC] Add support for static private class fields
Product: WebKit Reporter: Xan Lopez <xan.lopez>
Component: JavaScriptCoreAssignee: Xan Lopez <xan.lopez>
Status: RESOLVED FIXED    
Severity: Normal CC: chi187, ews-watchlist, keith_miller, mark.lam, msaboff, sbarati, ticaiolima, tzagallo, webkit-bug-importer, xan.lopez, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 194095    
Bug Blocks:    
Attachments:
Description Flags
Add support for static private fields
ysuzuki: review-
Add support for static private fields, v2
none
Add support for static private fields, v3
none
Add support for static private fields, v4
none
Add support for static private fields, v4 none

Description Xan Lopez 2020-07-14 01:46:13 PDT
Once bug #194095 lands this is very easy to support.
Comment 1 Xan Lopez 2020-07-14 01:51:02 PDT
Created attachment 404216 [details]
Add support for static private fields

v1
Comment 2 Radar WebKit Bug Importer 2020-07-21 01:47:15 PDT
<rdar://problem/65872465>
Comment 3 Yusuke Suzuki 2020-09-23 12:05:44 PDT
Comment on attachment 404216 [details]
Add support for static private fields

Can you rebase and fix EWS failures?
Comment 4 Xan Lopez 2020-09-23 23:21:57 PDT
(In reply to Yusuke Suzuki from comment #3)
> Comment on attachment 404216 [details]
> Add support for static private fields
> 
> Can you rebase and fix EWS failures?

Hey! Maybe it was not totally clear from the first comment, but this patch needs the static public fields patch in bug #194095 to apply, so can't really fix EWS until that one lands. Just uploaded it for reference, that's why it was not flagged as r?. Will get on it when possible for sure!
Comment 5 Xan Lopez 2020-11-18 01:47:09 PST
Created attachment 414430 [details]
Add support for static private fields, v2

v2,
rebase, add specific option for static private fields
Comment 6 Caio Lima 2020-11-18 03:03:45 PST
Comment on attachment 414430 [details]
Add support for static private fields, v2

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

> JSTests/stress/class-fields-static-private-harmony.js:344
> +*/

I think it's fine to remove it or maybe include a helper function to check `SymbolIsPrivate`.

> JSTests/stress/class-fields-static-private-harmony.js:390
> +}

Could we include a test that checks if we throw if we try to redefine a private field as static and vice versa?

I also miss a test that verifies if instance of a class can't access static fields. Do you mind adding them if they are missing?
Comment 7 Yusuke Suzuki 2020-11-18 20:58:03 PST
Comment on attachment 414430 [details]
Add support for static private fields, v2

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

>> JSTests/stress/class-fields-static-private-harmony.js:344
>> +*/
> 
> I think it's fine to remove it or maybe include a helper function to check `SymbolIsPrivate`.

Let's add $vm.isPrivateSymbol and use it here.
Comment 8 Xan Lopez 2020-11-19 03:42:20 PST
Created attachment 414562 [details]
Add support for static private fields, v3

v3,
support the vm.isPrivateSymbol tests
add a couple tests suggested by Caio
Comment 9 Caio Lima 2020-11-19 04:47:06 PST
Comment on attachment 414562 [details]
Add support for static private fields, v3

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

> Source/JavaScriptCore/tools/JSDollarVM.cpp:3385
> +    Symbol* symbol = asSymbol(callFrame->argument(0));

"asSymbol" don't work if `value.asCell()->isSymbol()` is false. This is going to break whenever we call `isPrivateSymbol(x)` and x is not a Symbol. We should just call  `asSymbol` when we are sure that JSValue is a Symbol. I think what you meant to use here is:

```
...
if (!callFrame->argument(0).isSymbol())
...
```
Comment 10 Xan Lopez 2020-11-19 05:06:20 PST
Created attachment 414565 [details]
Add support for static private fields, v4

v4,
be more careful with asSymbol() in $vm.isPrivateSymbol()
Comment 11 Xan Lopez 2020-11-19 05:20:56 PST
Created attachment 414568 [details]
Add support for static private fields, v4

v4bis,
upload the right patch
Comment 12 Caio Lima 2020-11-19 05:30:42 PST
Comment on attachment 414568 [details]
Add support for static private fields, v4

LGTM.
Comment 13 Yusuke Suzuki 2020-11-19 12:23:37 PST
Comment on attachment 414568 [details]
Add support for static private fields, v4

r=me
Comment 14 EWS 2020-11-19 12:46:12 PST
Committed r270043: <https://trac.webkit.org/changeset/270043>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414568 [details].