Bug 174212 - [JSC] Add support for class fields
Summary: [JSC] Add support for class fields
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caitlin Potter (:caitp)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-06 11:19 PDT by Xan Lopez
Modified: 2018-12-07 01:22 PST (History)
17 users (show)

See Also:


Attachments
WIP data field parsing (5.66 KB, patch)
2017-07-06 11:19 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Data fields WIP (23.40 KB, patch)
2017-11-20 05:55 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Patch (35.87 KB, patch)
2018-03-16 12:25 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Instance class fields (150.26 KB, patch)
2018-07-03 02:50 PDT, Xan Lopez
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (12.85 MB, application/zip)
2018-07-03 04:54 PDT, Build Bot
no flags Details
instance class fields (09/05/18) (171.95 KB, patch)
2018-09-05 03:05 PDT, Xan Lopez
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (12.82 MB, application/zip)
2018-09-05 05:01 PDT, Build Bot
no flags Details
Class fields WIP (176.05 KB, patch)
2018-09-26 13:42 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Class fields WIP (176.03 KB, patch)
2018-09-26 13:50 PDT, Xan Lopez
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.35 MB, application/zip)
2018-09-26 15:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.73 MB, application/zip)
2018-09-26 16:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.39 MB, application/zip)
2018-09-26 16:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews202 for win-future (12.82 MB, application/zip)
2018-09-26 16:50 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.49 MB, application/zip)
2018-09-26 18:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.30 MB, application/zip)
2018-09-27 08:32 PDT, Build Bot
no flags Details
Class fields (10/01) (181.54 KB, patch)
2018-10-01 10:32 PDT, Xan Lopez
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.15 MB, application/zip)
2018-10-01 12:37 PDT, Build Bot
no flags Details
Class fields WIP (10/02) (183.06 KB, patch)
2018-10-02 01:22 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Class fields, parser + bytecode + test changes (184.39 KB, patch)
2018-11-02 07:16 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Class fields, parser + bytecode + test changes (184.40 KB, patch)
2018-11-02 07:26 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Class fields, parser + bytecode + test changes (184.41 KB, patch)
2018-11-02 07:30 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Class fields, parser + bytecode + test changes (184.41 KB, patch)
2018-11-05 03:50 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Class fields, parser + bytecode + test changes (184.40 KB, patch)
2018-11-05 03:57 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Class fields, parser + bytecode + test changes (184.44 KB, patch)
2018-11-05 06:18 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Class fields, parser + bytecode + test changes (184.39 KB, patch)
2018-11-06 02:06 PST, Xan Lopez
xan.lopez: review?
Details | Formatted Diff | Diff
Placeholder baseline JIT for private field opcodes (4.32 KB, patch)
2018-12-05 02:55 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Placeholder baseline JIT for private field opcodes (4.32 KB, patch)
2018-12-05 03:37 PST, Xan Lopez
xan.lopez: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xan Lopez 2017-07-06 11:19:19 PDT
Created attachment 314736 [details]
WIP data field parsing

Ok, a really simple/rough WIP, but I think enough to ask a couple questions. I had a larger patch with a new parseDataField() method (lots of copy/paste) that I'd run in a loop at the begining of parseClass() until it stopped parsing fields, but then realized data fields can be mixed with methods in any order, so had to scrap that. Now I just branch after parsing an identifier to check if it's a method or a field. Issues:

1) ASI is not done. Actually not quite sure how to do that, probably I'm missing something trivial.
2) I'm just using as-is the code to parse assignments. Probably needs tweaking.
3) I'm not generating any code.

My idea for the first patch would be to figure out 1) and 2) and to pass the stuff in https://github.com/tc39/test262/pull/1064
I tried a bunch of examples and they all work, including new ones like class { field; method; field; etc }
Comment 1 Caitlin Potter (:caitp) 2017-07-06 14:14:28 PDT
Comment on attachment 314736 [details]
WIP data field parsing

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

just some ideas (also, it would be good to test your implementation's interaction with the contextual keywords, the generator `*` thing, etc.

> Source/JavaScriptCore/parser/Parser.cpp:2725
>              next();

What about something like:

```
next();
if ({classFieldsEnabled} && !isGenerator && !isAsyncMethod) {
  TreeExpression initializer = 0;
  if (consume(EQUAL))
      initializer = parseAssignmentExpression(context);
  consumeOrFail(SEMICOLON);  // I think class fields require the `;` at the end, per https://tc39.github.io/proposal-class-fields/#sec-updated-syntax
  // TODO: add to the AST node, keeping track of whether the field is static or not, and its initializer.

  continue; // parse the next class element
}
```

Then we could avoid having that stuff in parseMethod where it's accessible through more paths.
Comment 2 Caitlin Potter (:caitp) 2017-07-06 14:18:00 PDT
Comment on attachment 314736 [details]
WIP data field parsing

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

>> Source/JavaScriptCore/parser/Parser.cpp:2725
>>              next();
> 
> What about something like:
> 
> ```
> next();
> if ({classFieldsEnabled} && !isGenerator && !isAsyncMethod) {
>   TreeExpression initializer = 0;
>   if (consume(EQUAL))
>       initializer = parseAssignmentExpression(context);
>   consumeOrFail(SEMICOLON);  // I think class fields require the `;` at the end, per https://tc39.github.io/proposal-class-fields/#sec-updated-syntax
>   // TODO: add to the AST node, keeping track of whether the field is static or not, and its initializer.
> 
>   continue; // parse the next class element
> }
> ```
> 
> Then we could avoid having that stuff in parseMethod where it's accessible through more paths.

well, this isn't quite right I guess, you still need the `(match(EQUAL) || match(SEMICOLON))` thing in the if statement. Should work with that though
Comment 3 Saam Barati 2017-07-06 14:18:13 PDT
(In reply to Xan Lopez from comment #0)
> Created attachment 314736 [details]
> WIP data field parsing
> 
> Ok, a really simple/rough WIP, but I think enough to ask a couple questions.
> I had a larger patch with a new parseDataField() method (lots of copy/paste)
> that I'd run in a loop at the begining of parseClass() until it stopped
> parsing fields, but then realized data fields can be mixed with methods in
> any order, so had to scrap that. Now I just branch after parsing an
> identifier to check if it's a method or a field. Issues:
> 
> 1) ASI is not done. Actually not quite sure how to do that, probably I'm
> missing something trivial.
What's ASI? Do you mean AST? If so, you'll need to hook into SyntaxChecker and ASTBuilder. The SyntaxChecker is used when JSC does a quick pass over the source code to ensure no syntax errors. ASTBuilder is used to actually build and AST. We templatize the parser over the type of TreeBuilder. This means that SyntaxChecker and ASTBuilder need to provide the same API. This often means just filling in empty stubs inside SyntaxChecker. To add AST nodes, you'll need to modify parser/Nodes.h. You'll need to add a constructor, methods that you need, etc. Then, you can hook up the ASTBuilder to call into the API you define as needed. You should be able to find other examples of this by reading code inside ASTBuilder and Parser.cpp. You'll probably need to modify class declaration/expr nodes to know about their properties. Then, when generating code for these, you'll need to hook into the byte code generator, inside bytecompiler/BytecodeGenerator.* and bytecompiler/NodesCodeGen.cpp

> 2) I'm just using as-is the code to parse assignments. Probably needs
> tweaking.
It depends what the grammar is. What does the grammar say here?

> 3) I'm not generating any code.
See above.

> 
> My idea for the first patch would be to figure out 1) and 2) and to pass the
> stuff in https://github.com/tc39/test262/pull/1064
> I tried a bunch of examples and they all work, including new ones like class
> { field; method; field; etc }
We'll need 1, 2, and 3 to land this. We need to actually generate and execute code correctly.
Comment 4 Yusuke Suzuki 2017-07-06 22:02:38 PDT
I guess ASI is Automatic Semicolon Insertion.
Comment 5 Xan Lopez 2017-08-02 11:01:30 PDT
OK, some updates.

The issue with caitp's suggestions about the parser is that they would not support STRING idents, so we still need to do something similar to what my patch does (all PropertyNames per the spec must be support, including StringLiteral).

About:

> > 2) I'm just using as-is the code to parse assignments. Probably needs
> tweaking.

> It depends what the grammar is. What does the grammar say here?

The grammar says this is just an AssignmentExpression, so I think we are fine.

About code generation:

After exploring the code I bit it seemed to me I could just add a new PropertyNode to the PropertyNodeList with the result of the assignment parsing as the second parameter. Something like:

context.createProperty(ident, initializer, PropertyNode::Constant, PropertyNode::Unknown, etc);

This seems to actually work. Of course we'd still need to figure out private and static fields, but this seems good enough to get started. Is there any obvious reason to not use PropertyNodes for data fields?

Related to that, I saw https://bugs.webkit.org/show_bug.cgi?id=174935, which seems to suggest Yuzuke is also thinking of using properties to do this if I read it right.
Comment 6 Yusuke Suzuki 2017-08-02 19:18:25 PDT
(In reply to Xan Lopez from comment #5)
> OK, some updates.
> 
> The issue with caitp's suggestions about the parser is that they would not
> support STRING idents, so we still need to do something similar to what my
> patch does (all PropertyNames per the spec must be support, including
> StringLiteral).
> 
> About:
> 
> > > 2) I'm just using as-is the code to parse assignments. Probably needs
> > tweaking.
> 
> > It depends what the grammar is. What does the grammar say here?
> 
> The grammar says this is just an AssignmentExpression, so I think we are
> fine.
> 
> About code generation:
> 
> After exploring the code I bit it seemed to me I could just add a new
> PropertyNode to the PropertyNodeList with the result of the assignment
> parsing as the second parameter. Something like:
> 
> context.createProperty(ident, initializer, PropertyNode::Constant,
> PropertyNode::Unknown, etc);
> 
> This seems to actually work. Of course we'd still need to figure out private
> and static fields, but this seems good enough to get started. Is there any
> obvious reason to not use PropertyNodes for data fields?
> 
> Related to that, I saw https://bugs.webkit.org/show_bug.cgi?id=174935, which
> seems to suggest Yuzuke is also thinking of using properties to do this if I
> read it right.

Let's check and follow what the actual spec says. What the attribute of the class field property?
I just introduced private symbols to implement private fields super efficiently.
But I don't investigate the spec super deeply yet :P
Comment 7 Xan Lopez 2017-08-15 06:57:40 PDT
(In reply to Yusuke Suzuki from comment #6)
> 
> Let's check and follow what the actual spec says. What the attribute of the
> class field property?
> I just introduced private symbols to implement private fields super
> efficiently.
> But I don't investigate the spec super deeply yet :P

(Sorry for taking some days to reply, I was on holidays last week)

Using https://tc39.github.io/proposal-class-fields/#prod-FieldDefinition and https://tc39.github.io/ecma262/#sec-method-definitions as references.

For the attributes:

Both methods and fields are Configurable (the latest spec for fields says they are not Configurable, but it's a mistake, see https://github.com/tc39/proposal-class-fields/issues/35) and Writable. Fields Enumerable and Methods may or may not be. So I think we should be OK on that front.

Also, 2.7 in the fields spec says we should use DefinePropertyOrThrow too, so there's at least some hint that using the Property machinery might be doable. One  obvious roadblock is that instance fields have to be initialized in [[Construct]] (for base classes) or in SuperCall for subclasses, so I guess we would need extra code to do that properly for fields.
Comment 8 Xan Lopez 2017-08-15 09:34:55 PDT
(In reply to Xan Lopez from comment #7)
> Also, 2.7 in the fields spec says we should use DefinePropertyOrThrow too,
> so there's at least some hint that using the Property machinery might be
> doable. One  obvious roadblock is that instance fields have to be
> initialized in [[Construct]] (for base classes) or in SuperCall for
> subclasses, so I guess we would need extra code to do that properly for
> fields.

Been reading the spec a bit more and I think this sort of also applies for static fields. They pretty much use the same mechanisms to be defined, only difference being the target object and when you initialize things (late in ClassDefinitionEvaluation for static, [[Constructor]]/SuperCall for instance). So again it seems we could try to change the createProperty stuff enough to accommodate these cases and it would cover both instance and static.
Comment 9 Xan Lopez 2017-11-20 05:55:58 PST
Created attachment 327359 [details]
Data fields WIP

Just for the sake of keeping track, another version of the patch. This one does more stuff, and among other things I think it deals properly with the issue of initializing field properties with the 'undefined' value (unlike the previous patch).

My main doubt now is how to pass the instance field information from the Node's code generation classes to the Bytecode generator (first FIXME in the patch), so that they can be initialized at the right time (see InitializeInstanceFields vs. InitializeStaticFields in the spec). I'll keep working on this, but suggestions are welcome.
Comment 10 Caitlin Potter (:caitp) 2018-03-16 12:25:53 PDT
Created attachment 335960 [details]
Patch
Comment 11 Build Bot 2018-03-16 12:28:27 PDT
Attachment 335960 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:649:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Caitlin Potter (:caitp) 2018-03-16 12:36:20 PDT
I've updated the previous patch over top of the recent fixes from bugs.webkit.org/show_bug.cgi?id=183523, and added handling for ASI in the parser.

I've pinged Yusuke about solving the runtime side of things better, but don't believe I've heard back from him yet.

There are two ideas I have in mind:

1) put instance-field initialization in a synthetic Function node, whose bytecode is generated while the ClassExprNode's AST is still alive (so no re-parsing necessary) --- I believe this requires some hacks to the way UnlinkedFunctionCodeBlock works).

2) add a new ParseMode for re-parsing the ClassExpression, and create an instance-field initializer function by producing a function with that ParseMode, whose source is the entire class literal (this would require a separate version of parseClass to generate the correct AST during reparsing).

both of these approaches have some other problems that need to be addressed. For computed fields and private name fields, this also requires storing the generated private symbols and names computed during ClassExprNode::emitBytecode() somewhere. v8 uses internal/synthetic Variables, but it looks like JSC's scoping system is a bit different, and I'm not sure I can use non-unique names for synthetic variables.
Comment 13 Caitlin Potter (:caitp) 2018-03-16 12:43:36 PDT
(In reply to Caitlin Potter (:caitp) from comment #12)
> I've updated the previous patch over top of the recent fixes from
> bugs.webkit.org/show_bug.cgi?id=183523, and added handling for ASI in the
> parser.
> 
> I've pinged Yusuke about solving the runtime side of things better, but
> don't believe I've heard back from him yet.
> 
> There are two ideas I have in mind:
> 
> 1) put instance-field initialization in a synthetic Function node, whose
> bytecode is generated while the ClassExprNode's AST is still alive (so no
> re-parsing necessary) --- I believe this requires some hacks to the way
> UnlinkedFunctionCodeBlock works).
> 
> 2) add a new ParseMode for re-parsing the ClassExpression, and create an
> instance-field initializer function by producing a function with that
> ParseMode, whose source is the entire class literal (this would require a
> separate version of parseClass to generate the correct AST during reparsing).
> 
> both of these approaches have some other problems that need to be addressed.
> For computed fields and private name fields, this also requires storing the
> generated private symbols and names computed during
> ClassExprNode::emitBytecode() somewhere. v8 uses internal/synthetic
> Variables, but it looks like JSC's scoping system is a bit different, and
> I'm not sure I can use non-unique names for synthetic variables.

Oh, the _other_ other idea was, during constructor code generation, if the class is flagged as having instance fields, re-parse the class again and generate instance field initialization inline in bytecode.
Comment 14 Caitlin Potter (:caitp) 2018-04-11 10:01:03 PDT
Comment on attachment 335960 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.h:1630
> +    bool autoSemiColonAfterClassField()

it turns out, this is wrong, and the proposal doesn't prescribe new ASI requirements, but instead just outlines the consequence of omitting semicolons in certain cases. delete this and allowAutomaticSemicolonAfterClassField(), replace with autoSemiColon()
Comment 15 Caitlin Potter (:caitp) 2018-04-23 10:02:54 PDT
so, my local prototype for this feature is pretty much ready (it should pass most of the test262 coverage, there are a few things that don't work yet, and room for improvement in parts of it).

it's currently split up into 5 commits, the first one based on Xan's original patch.
Comment 16 Xan Lopez 2018-07-03 02:50:53 PDT
Created attachment 344172 [details]
Instance class fields

A new patch with the work done since May. We now pass most of the in-tree class-fields-public tests, and support both public and private instance fields. See the patch message for remaining TODOs and FIXMEs.
Comment 17 Build Bot 2018-07-03 02:54:42 PDT
Attachment 344172 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:1272:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2185:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:587:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 3 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Build Bot 2018-07-03 04:53:51 PDT
Comment on attachment 344172 [details]
Instance class fields

Attachment 344172 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8422699

New failing tests:
js/Object-getOwnPropertyNames.html
js/class-syntax-semicolon.html
Comment 19 Build Bot 2018-07-03 04:54:02 PDT
Created attachment 344176 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 20 Caitlin Potter (:caitp) 2018-07-03 08:39:18 PDT
Comment on attachment 344172 [details]
Instance class fields

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

A couple notes to clean this up a little bit (not including style linter problems)

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3092
> +    RefPtr<RegisterID> initializer = emitDirectGetById(newTemporary(), constructor, propertyNames().instanceFieldInitializerSymbol);

(e.g. here)

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4173
> +            generator.emitDirectPutById(constructor.get(), generator.propertyNames().instanceFieldInitializerSymbol, instanceFieldInitializer.get(), PropertyNode::Unknown);

(and here)

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2169
> +    case ToPropertyKey: {

the initial work I did on this ToPropertyKey thing was split up so that the JIT/DFG/FTL stuff is separated, we might want to keep it to the baseline stuff only for now, unless someone decides having JIT support is worthwhile. I really just wanted to learn more about how the speculative jit and FTL lowering works in JSC.

but to justify the ToPropertyKey thing, we do fail tests without it, since we need to convert computed names to a string before they get defined on the instance (for computed instance fields).

> Source/JavaScriptCore/parser/Parser.cpp:3006
> +            [[fallthrough]];

s/[[fallthrough]]/FALLTHROUGH/ and move it after the label so it's right before the next case label

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:289
> +    macro(instanceFieldInitializer)

we should move this to BuiltinNames.h (references in NodesCodegen.cpp and BytecodeGenerator.cpp need to be updated to the `builtinNames().instanceFieldInitializerPrivateName` version) --- to fix the O.getOwnPropertyNames failure.
Comment 21 Caitlin Potter (:caitp) 2018-07-03 08:47:42 PDT
the remaining failures (LayoutTests/js/class-field-syntax.html) just needs its expected.txt file updated, since the semicolon is valid in those contexts now.
Comment 22 Xan Lopez 2018-07-04 01:51:07 PDT
FWIW, we'll keep working on this, and we plan to tackle the remaining bugs/optimizations. We can wait for a review until then, and of course we can split the patch logically to make that easier. Just uploading the current WIP since it's already pretty advanced, any feedback will be welcome.
Comment 23 Xan Lopez 2018-09-05 03:05:59 PDT
Created attachment 348905 [details]
instance class fields (09/05/18)

Another big patch for comments. Most outstanding issues in the previous patch are fixed now, and we pass all the test262 tests in the tree (both -public and -private).

The commit by commit branch can be reviewed at: https://github.com/igalia/webkit/commits/class-fields
Comment 24 Build Bot 2018-09-05 03:08:43 PDT
Attachment 348905 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:161:  The parameter name "identifier" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:162:  The parameter name "identifier" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:184:  The parameter name "identifier" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:193:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1250:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1313:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1378:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/VariableEnvironment.cpp:126:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/parser/VariableEnvironment.cpp:136:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:479:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:1233:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:1719:  The parameter name "ident" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 12 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Build Bot 2018-09-05 05:01:47 PDT
Comment on attachment 348905 [details]
instance class fields (09/05/18)

Attachment 348905 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9100535

New failing tests:
js/class-syntax-semicolon.html
Comment 26 Build Bot 2018-09-05 05:01:59 PDT
Created attachment 348907 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 27 Xan Lopez 2018-09-26 13:42:43 PDT
Created attachment 350893 [details]
Class fields WIP
Comment 28 Build Bot 2018-09-26 13:46:19 PDT
Attachment 350893 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:213:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 65 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Xan Lopez 2018-09-26 13:50:30 PDT
Created attachment 350896 [details]
Class fields WIP
Comment 30 Xan Lopez 2018-09-26 14:03:33 PDT
(In reply to Xan Lopez from comment #29)
> Created attachment 350896 [details]
> Class fields WIP

This patch passes all the test262 test in the tree, as the commit says, so at this point we'd definitely welcome an initial review to see where we are at and what are the future steps to get this landed.
Comment 31 Build Bot 2018-09-26 15:20:56 PDT
Comment on attachment 350896 [details]
Class fields WIP

Attachment 350896 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/9360194

New failing tests:
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout-no-ftl
stress/generator-syntax.js.no-cjit-collect-continuously
stress/generator-syntax.js.ftl-no-cjit-small-pool
stress/method-name.js.no-cjit-validate-phases
stress/generator-syntax.js.no-llint
stress/method-name.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout-no-cjit
stress/async-await-syntax.js.no-ftl
stress/async-await-syntax.js.ftl-no-cjit-validate-sampling-profiler
stress/method-name.js.ftl-no-cjit-b3o1
stress/generator-syntax.js.ftl-no-cjit-no-put-stack-validate
stress/generator-syntax.js.ftl-no-cjit-b3o1
stress/generator-syntax.js.ftl-no-cjit-no-inline-validate
stress/generator-syntax.js.dfg-maximal-flush-validate-no-cjit
stress/method-name.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout
stress/async-await-syntax.js.ftl-no-cjit-no-inline-validate
stress/async-await-syntax.js.dfg-maximal-flush-validate-no-cjit
stress/async-await-syntax.js.dfg-eager
stress/async-await-syntax.js.ftl-eager
stress/async-await-syntax.js.no-cjit-collect-continuously
stress/async-await-syntax.js.ftl-no-cjit-small-pool
stress/async-await-syntax.js.no-llint
stress/method-name.js.dfg-eager-no-cjit-validate
stress/async-await-syntax.js.ftl-no-cjit-no-put-stack-validate
stress/async-await-syntax.js.ftl-eager-no-cjit-b3o1
stress/method-name.js.ftl-no-cjit-validate-sampling-profiler
stress/generator-syntax.js.no-cjit-validate-phases
stress/generator-syntax.js.ftl-eager
stress/async-await-syntax.js.ftl-eager-no-cjit
stress/method-name.js.no-llint
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout-ftl-eager-no-cjit
stress/async-await-syntax.js.no-cjit-validate-phases
stress/method-name.js.dfg-eager
stress/method-name.js.ftl-eager
stress/method-name.js.no-cjit-collect-continuously
stress/method-name.js.default
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout-ftl-no-cjit
stress/generator-syntax.js.dfg-eager-no-cjit-validate
stress/async-await-syntax.js.ftl-no-cjit-b3o1
stress/generator-syntax.js.dfg-eager
stress/method-name.js.ftl-no-cjit-small-pool
stress/method-name.js.dfg-maximal-flush-validate-no-cjit
stress/method-name.js.no-ftl
stress/generator-syntax.js.no-ftl
stress/generator-syntax.js.ftl-no-cjit-validate-sampling-profiler
stress/method-name.js.ftl-no-cjit-no-inline-validate
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout-dfg-eager-no-cjit
stress/async-await-syntax.js.default
stress/method-name.js.ftl-no-cjit-no-put-stack-validate
stress/async-await-syntax.js.dfg-eager-no-cjit-validate
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout-no-llint
stress/generator-syntax.js.ftl-eager-no-cjit-b3o1
stress/generator-syntax.js.default
stress/generator-syntax.js.ftl-eager-no-cjit
apiTests
Comment 32 Build Bot 2018-09-26 15:28:48 PDT
Comment on attachment 350896 [details]
Class fields WIP

Attachment 350896 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9360465

New failing tests:
js/class-syntax-semicolon.html
Comment 33 Build Bot 2018-09-26 15:28:50 PDT
Created attachment 350917 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 34 Build Bot 2018-09-26 16:38:54 PDT
Comment on attachment 350896 [details]
Class fields WIP

Attachment 350896 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9361424

New failing tests:
js/class-syntax-semicolon.html
Comment 35 Build Bot 2018-09-26 16:38:56 PDT
Created attachment 350923 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 36 Build Bot 2018-09-26 16:45:20 PDT
Comment on attachment 350896 [details]
Class fields WIP

Attachment 350896 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9361215

New failing tests:
js/class-syntax-semicolon.html
Comment 37 Build Bot 2018-09-26 16:45:22 PDT
Created attachment 350924 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 38 Build Bot 2018-09-26 16:50:03 PDT
Comment on attachment 350896 [details]
Class fields WIP

Attachment 350896 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9361429

New failing tests:
js/class-syntax-semicolon.html
Comment 39 Build Bot 2018-09-26 16:50:15 PDT
Created attachment 350925 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 40 Build Bot 2018-09-26 18:46:02 PDT
Comment on attachment 350896 [details]
Class fields WIP

Attachment 350896 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9362553

New failing tests:
js/class-syntax-semicolon.html
Comment 41 Build Bot 2018-09-26 18:46:04 PDT
Created attachment 350935 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 42 Build Bot 2018-09-27 08:32:37 PDT
Comment on attachment 350896 [details]
Class fields WIP

Attachment 350896 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9368001

New failing tests:
js/class-syntax-semicolon.html
Comment 43 Build Bot 2018-09-27 08:32:39 PDT
Created attachment 350962 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 44 Xan Lopez 2018-10-01 10:32:23 PDT
Created attachment 351261 [details]
Class fields (10/01)
Comment 45 Build Bot 2018-10-01 12:07:25 PDT
Comment on attachment 351261 [details]
Class fields (10/01)

Attachment 351261 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/9412760

New failing tests:
stress/async-await-syntax.js.ftl-eager-no-cjit-b3o1
stress/async-await-syntax.js.ftl-eager-no-cjit
stress/async-await-syntax.js.ftl-no-cjit-no-inline-validate
stress/async-await-syntax.js.dfg-maximal-flush-validate-no-cjit
stress/async-await-syntax.js.dfg-eager
stress/async-await-syntax.js.ftl-eager
stress/async-await-syntax.js.no-ftl
stress/async-await-syntax.js.ftl-no-cjit-validate-sampling-profiler
stress/async-await-syntax.js.no-cjit-validate-phases
stress/async-await-syntax.js.ftl-no-cjit-small-pool
stress/async-await-syntax.js.default
stress/async-await-syntax.js.no-llint
stress/async-await-syntax.js.dfg-eager-no-cjit-validate
stress/async-await-syntax.js.no-cjit-collect-continuously
stress/async-await-syntax.js.ftl-no-cjit-no-put-stack-validate
stress/async-await-syntax.js.ftl-no-cjit-b3o1
apiTests
Comment 46 Build Bot 2018-10-01 12:37:29 PDT
Comment on attachment 351261 [details]
Class fields (10/01)

Attachment 351261 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9413513

New failing tests:
media/range-extract-contents-crash.html
Comment 47 Build Bot 2018-10-01 12:37:31 PDT
Created attachment 351286 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 48 Xan Lopez 2018-10-02 01:22:28 PDT
Created attachment 351354 [details]
Class fields WIP (10/02)
Comment 49 Michael Catanzaro 2018-10-05 02:13:38 PDT
My understanding is this patch is ready to be reviewed and committed.

A couple things needed first:

 * Patch requires a ChangeLog entry created with prepare-ChangeLog
 * Attachment should not include "WIP" since then everyone looking at this bug thinks it's not ready for review yet :)
Comment 50 Daniel Ehrenberg 2018-10-06 12:08:35 PDT
A few suggestions for this patch:
- Put this new behavior behind a flag which is initially off (at least a runtime flag, and possibly also a compile-time flag if needed to avoid performance regressions); if possible, ensure that the appropriate tests run both with and without the flag.
- Include a more detailed commit message, summarizing the design of the patch, etc. It looks like the FIXMEs/TODOs are potential future optimizations, not things that would need to be fixed before landing. Is this right? If so, maybe this could be clarified in the commit message.
- Consider separating this large change into multiple small changes, to make review simpler.

We'll review this change further within Igalia before needing any upstream review. The WIP tag indicates that. If someone adventurous wants to dive into the current code, that'd still be appreciated, of course.

Xan, if you have any particular questions for a high-level design review from upstream, it'd be helpful to name those explicitly in comments here or on IRC.
Comment 51 Michael Catanzaro 2018-10-07 12:13:59 PDT
Comment on attachment 351354 [details]
Class fields WIP (10/02)

Removing r? per Dan's comment.
Comment 52 Xan Lopez 2018-10-10 02:52:14 PDT
Now that the tests are all green we are working on splitting parser+bytecode+test fixes in one patch to ease review, leaving the current optimizations aside for a second round. We of course still have the git repository with the whole series, and anyone can have a look at the full patch, but it's good if you wait for the next patch uploaded to bugzilla. We'll also address the suggestions you all made here.
Comment 53 Xan Lopez 2018-11-02 07:16:46 PDT
Created attachment 353696 [details]
Class fields, parser + bytecode + test changes

Here is the patch implementing the parser + bytecode changes for instance public/private fields. Some comments:

- As mentioned in the previous comment, we have left out the JIT/DFG changes for a follow-up patch. This should ease the review process.

- There are no runtime/compile-time flags for the feature. This is mostly to allow the bots to exercise the new code properly, but they can of course be added before landing if they are necessary.

- We have tried to explain the main design decisions and observable changes in the ChangeLog.
Comment 54 Xan Lopez 2018-11-02 07:26:09 PDT
Created attachment 353698 [details]
Class fields, parser + bytecode + test changes
Comment 55 Xan Lopez 2018-11-02 07:30:23 PDT
Created attachment 353699 [details]
Class fields, parser + bytecode + test changes
Comment 56 Xan Lopez 2018-11-05 03:50:37 PST
Created attachment 353848 [details]
Class fields, parser + bytecode + test changes
Comment 57 Xan Lopez 2018-11-05 03:57:27 PST
Created attachment 353849 [details]
Class fields, parser + bytecode + test changes
Comment 58 Dominik Inführ 2018-11-05 05:19:21 PST
Comment on attachment 353849 [details]
Class fields, parser + bytecode + test changes

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

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2635
> +    loadConstantOrVariablePayload(size, t0, CellTag, t3, .opGetPrivateFieldSlow)

This should probably be `.opPutPrivateFieldSlow`.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1366
> +        metadata.oldStructure = metadata.newStructure = metadata.offset = 0;

This also fails to build, GCC doesn't allow assigning 0 to a Structure*.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1430
> +            metadata.structure = metadata.offset = 0;

Ditto

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1492
> +        metadata.structure = metadata.offset = 0;

Ditto.
Comment 59 Xan Lopez 2018-11-05 06:18:46 PST
Created attachment 353857 [details]
Class fields, parser + bytecode + test changes
Comment 60 Xan Lopez 2018-11-06 02:06:36 PST
Created attachment 353954 [details]
Class fields, parser + bytecode + test changes

A couple of bots took so long to process the patch that there were conflicts to apply it. Try again to see what's their status, since all the others are now green.
Comment 61 Xan Lopez 2018-11-07 05:22:24 PST
Comment on attachment 353954 [details]
Class fields, parser + bytecode + test changes

Marking as r?. Please see comment #53 for some additional comments about the patch.
Comment 62 Caitlin Potter (:caitp) 2018-12-04 08:43:23 PST
This is ready for review
Comment 63 Caio Lima 2018-12-04 11:15:03 PST
Comment on attachment 353954 [details]
Class fields, parser + bytecode + test changes

I think we either should add a feature flag or add Baseline support before landing. The problem is current Patch crashes on following program:

```
//@ runBigIntEnabled

class A {
    #a = 3;

    getV() {
        return this.#a;
    }

}

let o = new A();
for (let i = 0; i < 1000; i++) {
    o.getV();
}
```

Here is the StackTrace:

```
SHOULD NEVER BE REACHED
...webkit/Source/JavaScriptCore/jit/JIT.cpp(444) : void JSC::JIT::privateCompileMainPass()
1   0x109de1949 WTFCrash
2   0x1084d1cab WTFCrashWithInfo(int, char const*, char const*, int)
3   0x1095b6029 JSC::JIT::privateCompileMainPass()
4   0x1095b92e0 JSC::JIT::compileWithoutLinking(JSC::JITCompilationEffort)
5   0x109657755 JSC::JITWorklist::Plan::compileInThread()
6   0x109652a6a JSC::JITWorklist::Plan::compileNow(JSC::CodeBlock*, unsigned int)
7   0x109652876 JSC::JITWorklist::compileLater(JSC::CodeBlock*, unsigned int)
8   0x10969c0bf JSC::LLInt::jitCompileAndSetHeuristics(JSC::CodeBlock*, JSC::ExecState*, unsigned int)
9   0x10969b788 JSC::LLInt::entryOSR(JSC::ExecState*, JSC::Instruction const*, JSC::CodeBlock*, char const*, JSC::LLInt::EntryKind)
10  0x10969b94a llint_entry_osr_function_for_call
11  0x109686464 llint_entry
12  0x109697d95 llint_entry
13  0x109684020 vmEntryToJavaScript
14  0x10958430e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
15  0x1095838bf JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
16  0x10989a555 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
17  0x108516bbb runWithOptions(GlobalObject*, CommandLine&, bool&)
18  0x1084eec2c jscmain(int, char**)::$_4::operator()(JSC::VM&, GlobalObject*, bool&) const
19  0x1084d3c54 int runJSC<jscmain(int, char**)::$_4>(CommandLine, bool, jscmain(int, char**)::$_4 const&)
20  0x1084d265f jscmain(int, char**)
21  0x1084d25be main
22  0x7fff6fc77015 start
23  0x4
```
Comment 64 Xan Lopez 2018-12-05 02:55:40 PST
Created attachment 356594 [details]
Placeholder baseline JIT for private field opcodes

Thanks Caio, this should take care of it for an initial patch where we leave off JIT/DFG. I'm uploading it as a separate patch to ease review, but it could be trivially be folded in the previous one.
Comment 65 Build Bot 2018-12-05 02:58:43 PST
Attachment 356594 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITOpcodes.cpp:1556:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITOpcodes.cpp:1556:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITOpcodes.cpp:1560:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 66 Xan Lopez 2018-12-05 03:37:26 PST
Created attachment 356595 [details]
Placeholder baseline JIT for private field opcodes

Fix style.