Bug 214297 - [JSC] Add support for static private class fields
Summary: [JSC] Add support for static private class fields
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xan Lopez
URL:
Keywords: InRadar
Depends on: 194095
Blocks:
  Show dependency treegraph
 
Reported: 2020-07-14 01:46 PDT by Xan Lopez
Modified: 2020-11-19 12:46 PST (History)
11 users (show)

See Also:


Attachments
Add support for static private fields (11.62 KB, patch)
2020-07-14 01:51 PDT, Xan Lopez
ysuzuki: review-
Details | Formatted Diff | Diff
Add support for static private fields, v2 (13.94 KB, patch)
2020-11-18 01:47 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Add support for static private fields, v3 (18.84 KB, patch)
2020-11-19 03:42 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Add support for static private fields, v4 (18.84 KB, patch)
2020-11-19 05:06 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Add support for static private fields, v4 (18.84 KB, patch)
2020-11-19 05:20 PST, Xan Lopez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].