Bug 209703 – [JSC] Public class field should accept "static" as field name

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

[Caio Lima]

The following program should be valid, but throws a SyntaxError:

```
class A {
  super;
  static;
  set;
  get;
  test() { return "foo"; }
}
```

[Caio Lima]

Created attachment 394826 [details]
Patch

[Ross Kirsling]

Comment on attachment 394826 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        It allows class fields being created using "static" as identifier.

We should link to the spec here but unfortunately the PR is still unmerged.
(https://github.com/tc39/ecma262/pull/1668)

Since the idea is the same as other identifiers though, I suppose we could at least link to this:
https://tc39.es/ecma262/#prod-IdentifierName

> Source/JavaScriptCore/parser/Parser.cpp:2880
> +                // Reparse "static()" as a method named or "static" as a class field.

grammar nit: remove 'named'

> JSTests/stress/class-fields-harmony.js:909
> +{

I think Cait brought this file over from V8. Are we sure we want to make arbitrary edits here?
(https://github.com/v8/v8/blob/master/test/mjsunit/harmony/public-instance-class-fields.js)

Also, perhaps we should add this to test262? Because SM is currently failing too.

[Caio Lima]

Created attachment 394916 [details]
Patch

[Caio Lima]

Comment on attachment 394826 [details]
Patch

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

Thank you very much for the review!

>> Source/JavaScriptCore/ChangeLog:8
>> +        It allows class fields being created using "static" as identifier.
> 
> We should link to the spec here but unfortunately the PR is still unmerged.
> (https://github.com/tc39/ecma262/pull/1668)
> 
> Since the idea is the same as other identifiers though, I suppose we could at least link to this:
> https://tc39.es/ecma262/#prod-IdentifierName

I agree. Added.

>> JSTests/stress/class-fields-harmony.js:909
>> +{
> 
> I think Cait brought this file over from V8. Are we sure we want to make arbitrary edits here?
> (https://github.com/v8/v8/blob/master/test/mjsunit/harmony/public-instance-class-fields.js)
> 
> Also, perhaps we should add this to test262? Because SM is currently failing too.

I'll place this in a separate file to avoid any issue. 

Test262 were added by https://github.com/tc39/test262/pull/2550

[Ross Kirsling]

Comment on attachment 394916 [details]
Patch

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

> JSTests/ChangeLog:8
> +        * stress/class-fields-harmony.js:

Oops, looks like the ChangeLog needs to be regenerated.

[Caio Lima]

Created attachment 394944 [details]
Patch

[Caio Lima]

Comment on attachment 394944 [details]
Patch

Thank you very much for the review!

[EWS]

Committed r259216: <https://trac.webkit.org/changeset/259216>

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

[Radar WebKit Bug Importer]

<rdar://problem/61067736>