Bug 214297 – [JSC] Add support for static private class fields

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

[Xan Lopez]

Once bug #194095 lands this is very easy to support.

[Xan Lopez]

Created attachment 404216 [details]
Add support for static private fields

v1

[Radar WebKit Bug Importer]

<rdar://problem/65872465>

[Yusuke Suzuki]

Comment on attachment 404216 [details]
Add support for static private fields

Can you rebase and fix EWS failures?

[Xan Lopez]

(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!

[Xan Lopez]

Created attachment 414430 [details]
Add support for static private fields, v2

v2,
rebase, add specific option for static private fields

[Caio Lima]

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?

[Yusuke Suzuki]

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.

[Xan Lopez]

Created attachment 414562 [details]
Add support for static private fields, v3

v3,
support the vm.isPrivateSymbol tests
add a couple tests suggested by Caio

[Caio Lima]

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())
...
```

[Xan Lopez]

Created attachment 414565 [details]
Add support for static private fields, v4

v4,
be more careful with asSymbol() in $vm.isPrivateSymbol()

[Xan Lopez]

Created attachment 414568 [details]
Add support for static private fields, v4

v4bis,
upload the right patch

[Caio Lima]

Comment on attachment 414568 [details]
Add support for static private fields, v4

LGTM.

[Yusuke Suzuki]

Comment on attachment 414568 [details]
Add support for static private fields, v4

r=me

[EWS]

Committed r270043: <https://trac.webkit.org/changeset/270043>

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