| Summary: | [JSC] Public class field should accept "static" as field name | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Caio Lima <ticaiolima> | ||||||||
| Component: | JavaScriptCore | Assignee: | Caio Lima <ticaiolima> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | caitp, ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer, xan.lopez, ysuzuki | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=209675 | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 394826 [details]
Patch
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. Created attachment 394916 [details]
Patch
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 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. Created attachment 394944 [details]
Patch
Comment on attachment 394944 [details]
Patch
Thank you very much for the review!
Committed r259216: <https://trac.webkit.org/changeset/259216> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394944 [details]. |
The following program should be valid, but throws a SyntaxError: ``` class A { super; static; set; get; test() { return "foo"; } } ```