[JSC] Add support for instance class fields

[Xan Lopez]

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 }

[Caitlin Potter (:caitp)]

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.

[Caitlin Potter (:caitp)]

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

[Saam Barati]

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

[Yusuke Suzuki]

I guess ASI is Automatic Semicolon Insertion.

[Xan Lopez]

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.

[Yusuke Suzuki]

(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

[Xan Lopez]

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

[Xan Lopez]

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

[Xan Lopez]

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.

[Caitlin Potter (:caitp)]

Created attachment 335960 [details]
Patch

[EWS Watchlist]

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.

[Caitlin Potter (:caitp)]

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.

[Caitlin Potter (:caitp)]

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

[Caitlin Potter (:caitp)]

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

[Caitlin Potter (:caitp)]

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.

[Xan Lopez]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Caitlin Potter (:caitp)]

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.

[Caitlin Potter (:caitp)]

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.

[Xan Lopez]

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.

[Xan Lopez]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Xan Lopez]

Created attachment 350893 [details]
Class fields WIP

[EWS Watchlist]

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.

[Xan Lopez]

Created attachment 350896 [details]
Class fields WIP

[Xan Lopez]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Xan Lopez]

Created attachment 351261 [details]
Class fields (10/01)

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Xan Lopez]

Created attachment 351354 [details]
Class fields WIP (10/02)

[Michael Catanzaro]

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 :)

[Daniel Ehrenberg]

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.

[Michael Catanzaro]

Comment on attachment 351354 [details]
Class fields WIP (10/02)

Removing r? per Dan's comment.

[Xan Lopez]

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.

[Xan Lopez]

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.

[Xan Lopez]

Created attachment 353698 [details]
Class fields, parser + bytecode + test changes

[Xan Lopez]

Created attachment 353699 [details]
Class fields, parser + bytecode + test changes

[Xan Lopez]

Created attachment 353848 [details]
Class fields, parser + bytecode + test changes

[Xan Lopez]

Created attachment 353849 [details]
Class fields, parser + bytecode + test changes

[Dominik Inführ]

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.

[Xan Lopez]

Created attachment 353857 [details]
Class fields, parser + bytecode + test changes

[Xan Lopez]

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.

[Xan Lopez]

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.

[Caitlin Potter (:caitp)]

This is ready for review

[Caio Lima]

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
```

[Xan Lopez]

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.

[EWS Watchlist]

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.

[Xan Lopez]

Created attachment 356595 [details]
Placeholder baseline JIT for private field opcodes

Fix style.

[Saam Barati]

Do you still need a review for this? Why are there two different patches?

[Xan Lopez]

(In reply to Saam Barati from comment #67)
> Do you still need a review for this? Why are there two different patches?

Indeed it does! As I said in comment #64 the second patch is just a trivial fix that can be folded easily in the main patch, but the main focus right now for us would be to get a first review of the "Class fields, parser + bytecode + test changes" patch, which is the first step to get the feature in the tree.

[Xan Lopez]

Created attachment 360454 [details]
Class fields, parser + bytecode + test changes

Patch refresh.

This includes the previous patch rebased against master (01/29/2019), plus the fix for the crash when JIT baseline kicks in, plus a few fixes that were uncovered with the recent test262 refresh.

[Caitlin Potter (:caitp)]

(In reply to Saam Barati from comment #67)
> Do you still need a review for this? Why are there two different patches?

As mentioned offline, we do still need review on this, and are under some pressure to get this upstream. It would be really good if we could get eyes taking a look at this :) The two different patches have been merged into one.

(PING: Yusuke, Keith, msaboff)

[Caio Lima]

Comment on attachment 360454 [details]
Class fields, parser + bytecode + test changes

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

Patch mostly LGTM. I have some minor comments.

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:46
> +static_assert(sizeof(UnlinkedFunctionExecutable) <= 176, "UnlinkedFunctionExecutable should fit in a 160-byte cell. If you increase the size of this class, consider making a size class that perfectly fits it.");

I think we should change the 160 in the message as well.

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:199
> +    Vector<JSTextPosition> m_instanceFieldLocations;

I think this should be a FIXME. Also It is good have a link to the bug.

[Maciej Stachowiak]

Comment on attachment 360454 [details]
Class fields, parser + bytecode + test changes

Boring feedback: pleas put this behind a runtime or compile-time feature flag (per WebKit project policy for new features), so we can decouple decisions about shipping it from decisions about landing it. (I don't see a flag in this large-ish patch but maybe I missed something).

[Saam Barati]

Comment on attachment 360454 [details]
Class fields, parser + bytecode + test changes

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

> JSTests/ChangeLog:21
> +        * stress/async-await-syntax.js:

Some fly by feedback: am I reading this right that there are no new tests besides what’s in test262? If so, this patch should be adding a lot of JSC specific tests. Tests that you think stress interesting implementation details and how your patch interacts with the JIT/GC

[Saam Barati]

Comment on attachment 360454 [details]
Class fields, parser + bytecode + test changes

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

I still have to read some more code, but here is my initial feedback.

> Source/JavaScriptCore/ChangeLog:19
> +        present. The field is always loaded during class construction or
> +        super calls, but is not invoked if the value is undefined. This is
> +        similar to what the v8 engine does.

Why do we always need to load it? Seems like this is bad for perf. Shouldn't we know statically if we have this function?

Also, why are we implementing this as a function call instead of just inlining the instantiation of these fields directly? I vaguely remember a reason for it done this way, but it seems strictly less optimal than just inlining it. It would be great if using class fields isn't slower than just normally adding properties to |this|.

What scope are the fields evaluated in?

> Source/JavaScriptCore/ChangeLog:23
> +        WTF::Vector<JSTextPosition>.  These are used to reparse the class

I think it'd be awesome to find some way to inline field instantiation. It might not be super trivial. Some issues I see:
- Teaching the parser how to swap over and parse something in a different context.
- Teaching the bytecode generator how to do some scope swapping to ensure we're not evaluating code in a scope we shouldn't be.

> Source/JavaScriptCore/ChangeLog:33
> +        (although a new EvalContextType is needed to handle the use of

We don't already know this? What are the early errors? Can you enumerate them?

> Source/JavaScriptCore/ChangeLog:54
> +            For each computed class field name, `[ {expr} ] = {initializer}`:

what happens for duplicate names? We call the set twice?

> Source/JavaScriptCore/ChangeLog:64
> +            3) instance[fieldName] = value

Is this actually a Put. Or is this a defineOwnProperty?

> Source/JavaScriptCore/ChangeLog:68
> +        Although this is somewhat hacky, and should be replaced with
> +        something better, it works and is fairly straight forward to
> +        understand.

This doesn't seem that crazy.

> Source/JavaScriptCore/ChangeLog:70
> +        Finally this introduces a new opcode (op_to_property_key). There

"There" => "Previously, there"

> Source/JavaScriptCore/ChangeLog:86
> +           (Caveat: this requires looking them up in scope each time
> +           they're used --- though DFG can hopefully identify when they're
> +           reused and only emit the load once, or replace it with a constant
> +           in some cases).

The DFG will indeed CSE these. It will also constant fold them if a class is only ever evaluated once (so the scope is only written to once).

> Source/JavaScriptCore/ChangeLog:91
> +        3) avoid adding additional slow cases to get/put operations to the common
> +           cases, avoid adding new GetPutInfo complexity, etc

I guess I'm missing why this is actually needed. What about get|put_by_val doesn't work?

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:198
> +    // TODO: only allocate this if needed.

Do we have a RareData for this class?

>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:199
>> +    Vector<JSTextPosition> m_instanceFieldLocations;
> 
> I think this should be a FIXME. Also It is good have a link to the bug.

+1

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1946
> +                if (privateName.value.isDeclared())

What does it mean for it to not be declared? Is this computed vs not computed?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1950
> +            // FIXME: determine if private names are captured in a smarter way.

is this valid given your comment below?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2825
> +RegisterID* BytecodeGenerator::emitInstanceFieldInitializationIfNeeded(RegisterID* dst, RegisterID* constructor, const JSTextPosition& divot, const JSTextPosition& divotStart, const JSTextPosition& divotEnd)

Seems like we should know this statically. Am I missing something as to why that's not feasible? Instead of this branch being at runtime, it should be at bytecode generation time.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:734
> +    const Identifier& description = *node.name();

I'm super confused. What is this if this is supposed to be a computed field name?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:742
> +    // TODO: add type profiling if we need it?

General comment: TODOs should be FIXMEs with a link. However, I think you're ok on this once. I don't see a reason you need to have the typeProfiler do anything here.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:746
> +    generator.liftTDZCheckIfPossible(var);

This seems wrong, but I'm not totally sure the context of what's going on here. See comment above about Identifier.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4041
> +            generator.emitSetFunctionName(value.get(), *m_ident);

An idea for tests that I suggested earlier: this is exactly the kind of thing that's good to have tests for.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4047
> +        Optional<uint32_t> optionalIndex = parseIndex(*m_ident);
> +        if (!optionalIndex)

Style nit: I like to write this in a different way:

if (Optional<uint32_t> optionalIndex = parseIndex(*m_ident))
   ... use optionalndex
else
   ... don't use optionalIndex

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4055
> +    case DefineFieldNode::PrivateName: {

Are these computed private names?

Anyways, in general, I have a question on computed private names. What are the semantics, of say:
class C {
   #["foo"] = 42
   #["foo"] = 44
}

Do I end up having two properties or one here?

Also, is there syntax to access private names in a computed fashion?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4057
> +        generator.emitPrivateFieldAdd(generator.thisRegister(), *m_ident, value.get());

Why isn't this just put_to_scope?

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:284
> +    case op_add_private_field: // TODO: add JIT support for private field ops

Make this a "FIXME:" w/ bugzilla link

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2596
> +    # (Can we be sure there's room in the existing backing store for non-inline fields?)

What's up with this comment? Did you figure it out?

> Source/JavaScriptCore/runtime/JSScope.cpp:343
> +            auto privateNames = symbolTable->privateNames();

Why isn't this a hashtable?

> Source/JavaScriptCore/runtime/JSSymbolTableObject.h:51
> +    PrivateSymbolImpl* getPrivateSymbol(const Identifier& name)

I think describing the object model you're going for here would help me read through this code more easily. For example, I don't follow the object model for private fields yet

> Source/JavaScriptCore/runtime/JSSymbolTableObject.h:98
> +    std::unique_ptr<JSSymbolTableObjectRareData> m_rareData;

I'm confused why we need this. Why isn't this just part of the SymbolTable itself?

> Source/JavaScriptCore/runtime/SymbolTable.h:738
> +        PrivateNameSet m_privateNames;

Why isn't this just in the normal hash table? I think that'd Just Work.

[Daniel Ehrenberg]

Jumping in to answer some specification questions.

> Is this actually a Put. Or is this a defineOwnProperty?

This should be a defineOwnProperty.

> what happens for duplicate names? We call the set twice?

That's what it should do, spec-wise (whether or not it's computed)

> Are these computed private names?
> 
> Also, is there syntax to access private names in a computed fashion?

Computed private names are not supported.

> Also, why are we implementing this as a function call instead of just inlining the instantiation of these fields directly?

You can probably do that most of the time, but given the flexibility of where super() can be called (e.g., inside an eval), it's probably too hard to do 100% of the time. See https://github.com/tc39/proposal-class-fields#execution-of-initializer-expressions

[Yusuke Suzuki]

Comment on attachment 360454 [details]
Class fields, parser + bytecode + test changes

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

>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:198
>> +    // TODO: only allocate this if needed.
> 
> Do we have a RareData for this class?

Please do this in this patch since sizeof(UnlinkedFunctionExecutable) is significantly allocated cells.

[Caitlin Potter (:caitp)]

Comment on attachment 360454 [details]
Class fields, parser + bytecode + test changes

I don't have answers to all of these questions, particularly WRT the way private names are kept in Scope / SymbolTable. I think there was a reason for everything that was done there, but there's
a good chance it can be simplified (as some of it is very old and leftover from an older iteration, etc).

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

>> Source/JavaScriptCore/ChangeLog:19
>> +        similar to what the v8 engine does.
> 
> Why do we always need to load it? Seems like this is bad for perf. Shouldn't we know statically if we have this function?
> 
> Also, why are we implementing this as a function call instead of just inlining the instantiation of these fields directly? I vaguely remember a reason for it done this way, but it seems strictly less optimal than just inlining it. It would be great if using class fields isn't slower than just normally adding properties to |this|.
> 
> What scope are the fields evaluated in?

> Why do we always need to load it? Seems like this is bad for perf. Shouldn't we know statically if we have this function?

It isn't great for perf, but it probably doesn't hurt that much, it's one extra property load per class constructor/super call.

The main reason why it's done this way currently is, when we're emitting the bytecodes for the constructor, it doesn't include
the AST for the rest of the class fields, and we haven't yet slipped information about class fields into the CodeBlock/Executable
(which currently is awkward to sneak in).

Since the information is not currently available when the constructor is generated, we do this dynamic load to avoid the problem. It
shouldn't be too difficult to make it work, though.

---

> Also, why are we implementing this as a function call instead of just inlining the instantiation of these fields directly? I vaguely remember a reason for it done this way, but it seems strictly less optimal than just inlining it. It would be great if using class fields isn't slower than just normally adding properties to |this|.

Yeah, this is similar to the previous point --- not having information about the class fields during constructor generation. The scope rules also make it a bit awkward (need a new scope whose parent is the class scope, and which sees `this` and `super`, but nothing else). I'm sure this is doable, and we have plans to do this in a followup patch, but it seemed a bit much for the first implementation.

---

> What scope are the fields evaluated in?

Essentially, it's a nested block scope under the class lexical scope, with the following bindings:

- if there is a class heritage, it can access super properties (but, super calls are illegal)
- it can access `this` (the class instance)
- `arguments` is forbidden ("It is a Syntax Error if ContainsArguments of Initializer is true." with a similar rule for arguments-in-eval-in-initializer)

>> Source/JavaScriptCore/ChangeLog:23
>> +        WTF::Vector<JSTextPosition>.  These are used to reparse the class
> 
> I think it'd be awesome to find some way to inline field instantiation. It might not be super trivial. Some issues I see:
> - Teaching the parser how to swap over and parse something in a different context.
> - Teaching the bytecode generator how to do some scope swapping to ensure we're not evaluating code in a scope we shouldn't be.

Agree --- do you see this as a blocker for landing? It seems suitable for a followup optimization. Loading the initializer function is unfortunate and may incur a very minor performance hit for ES6 classes, but I think that hit would be relatively minor, and probably too short lived to make it into a release.

>> Source/JavaScriptCore/ChangeLog:33
>> +        (although a new EvalContextType is needed to handle the use of
> 
> We don't already know this? What are the early errors? Can you enumerate them?

`arguments` is not allowed in a direct eval call within a field initializer.

>> Source/JavaScriptCore/ChangeLog:54
>> +            For each computed class field name, `[ {expr} ] = {initializer}`:
> 
> what happens for duplicate names? We call the set twice?

Yes --- If multiple computed fields become the same name during class field evaluation, then the same instance field would be defined multiple times per instance, per spec.

>> Source/JavaScriptCore/ChangeLog:64
>> +            3) instance[fieldName] = value
> 
> Is this actually a Put. Or is this a defineOwnProperty?

For non-PrivateName fields, it's specced as CreateDataPropertyOrThrow, which is essentially a Put.

>> Source/JavaScriptCore/ChangeLog:86
>> +           in some cases).
> 
> The DFG will indeed CSE these. It will also constant fold them if a class is only ever evaluated once (so the scope is only written to once).

in this patch it won't, we never really got that far with the optimization. But I agree it should be possible to do this better when we tier up.

>> Source/JavaScriptCore/ChangeLog:91
>> +           cases, avoid adding new GetPutInfo complexity, etc
> 
> I guess I'm missing why this is actually needed. What about get|put_by_val doesn't work?

It's doable, however there are a number of reasons why I originally opted to go the other route:

It looks like get/put_by_value are mainly optimized for indexed property access (at least in baseline, not sure how they do after tiering up). They also handle a lot of different cases already,
and private field support would add a number of other requirements, which would likely keep private fields permanently on the slow path, and dispatching to private field cases would have a slight impact
on other code.
Specifically, we'd need to dispatch for: private fields (distinct from existing PrivateIdentifier symbols, slightly different rules), brand checking (private methods/accessors), throwing if the field does not exist (or already exists in the DefineField case).

In addition to that, we'd probably need to wrap the private names in a JSCell, so that they can be fit in a JSValue, to avoid changing too much of the code. If we wound up using a raw UID and dispatching
early based on a flag, then it doesn't really save any code. Allocating JSCells for each of these potentially eats up a lot of heap space if classes are evaluated multiple times (as is done in some of the mixin examples I've seen from JS developers).

We are happy to change this to use the existing opcodes before or after landing, but I think there are good reasons not to.

To summarize:
  - avoid slowing down common operations by adding more dispatching
  - avoid using up extra JS heap space by putting our UIDs in a JSCell wrapper (a new one for each field for each time the class is evaluated) for compat with op_get|put_by_val
  - (very minor point) better developer ergonomics: easier to read "get_private_field #foo" vs the combination of ops that would be needed for the other cases

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4055
>> +    case DefineFieldNode::PrivateName: {
> 
> Are these computed private names?
> 
> Anyways, in general, I have a question on computed private names. What are the semantics, of say:
> class C {
>    #["foo"] = 42
>    #["foo"] = 44
> }
> 
> Do I end up having two properties or one here?
> 
> Also, is there syntax to access private names in a computed fashion?

I believe this was answered by Dan earlier, but private names are never computed.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4057
>> +        generator.emitPrivateFieldAdd(generator.thisRegister(), *m_ident, value.get());
> 
> Why isn't this just put_to_scope?

I'm not sure I understand how this would work with `put_to_scope` --- the method scope wouldn't be very useful, the class scope could be shared by multiple different instances (which might want to store different values in their private field).

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2596
>> +    # (Can we be sure there's room in the existing backing store for non-inline fields?)
> 
> What's up with this comment? Did you figure it out?

IIRC (and it's been a while since I wrote this), `storePropertyAtVariableOffset` takes care of bailing out if the backing store is not big enough, which means we can probably get rid of the comment.

[Xan Lopez]

Just a couple extra comments:

(In reply to Saam Barati from comment #74)

> > Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:198
> > +    // TODO: only allocate this if needed.
> 
> Do we have a RareData for this class?

This was indeed planned, so we'll try to have it in the next patch already.

> 
> What does it mean for it to not be declared? Is this computed vs not
> computed?

This is meant to track whether a private name is being used in the class before it's declared.

[Saam Barati]

(In reply to Caitlin Potter (:caitp) from comment #77)
> Comment on attachment 360454 [details]
> Class fields, parser + bytecode + test changes
> 
> I don't have answers to all of these questions, particularly WRT the way
> private names are kept in Scope / SymbolTable. I think there was a reason
> for everything that was done there, but there's
> a good chance it can be simplified (as some of it is very old and leftover
> from an older iteration, etc).
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360454&action=review
> 
> >> Source/JavaScriptCore/ChangeLog:19
> >> +        similar to what the v8 engine does.
> > 
> > Why do we always need to load it? Seems like this is bad for perf. Shouldn't we know statically if we have this function?
> > 
> > Also, why are we implementing this as a function call instead of just inlining the instantiation of these fields directly? I vaguely remember a reason for it done this way, but it seems strictly less optimal than just inlining it. It would be great if using class fields isn't slower than just normally adding properties to |this|.
> > 
> > What scope are the fields evaluated in?
> 
> > Why do we always need to load it? Seems like this is bad for perf. Shouldn't we know statically if we have this function?
> 
> It isn't great for perf, but it probably doesn't hurt that much, it's one
> extra property load per class constructor/super call.
> 
> The main reason why it's done this way currently is, when we're emitting the
> bytecodes for the constructor, it doesn't include
> the AST for the rest of the class fields, and we haven't yet slipped
> information about class fields into the CodeBlock/Executable
> (which currently is awkward to sneak in).
> 
> Since the information is not currently available when the constructor is
> generated, we do this dynamic load to avoid the problem. It
> shouldn't be too difficult to make it work, though.

I think we should make this work. I'm also not sure exactly how annoying it'll be to thread this through, but it should be a goal to not make existing classes slower to support class fields.

It's also our goal to make using class fields no slower than just doing `this.<field> = <expr>` in the constructor. But I think it's reasonable if we continue to speed this up in follow up patches.

> 
> ---
> 
> > Also, why are we implementing this as a function call instead of just inlining the instantiation of these fields directly? I vaguely remember a reason for it done this way, but it seems strictly less optimal than just inlining it. It would be great if using class fields isn't slower than just normally adding properties to |this|.
> 
> Yeah, this is similar to the previous point --- not having information about
> the class fields during constructor generation. The scope rules also make it
> a bit awkward (need a new scope whose parent is the class scope, and which
> sees `this` and `super`, but nothing else). I'm sure this is doable, and we
> have plans to do this in a followup patch, but it seemed a bit much for the
> first implementation.

This sounds reasonable to do in a followup. Can we open a bug to track doing that?

> 
> ---
> 
> > What scope are the fields evaluated in?
> 
> Essentially, it's a nested block scope under the class lexical scope, with
> the following bindings:
> 
> - if there is a class heritage, it can access super properties (but, super
> calls are illegal)
> - it can access `this` (the class instance)
> - `arguments` is forbidden ("It is a Syntax Error if ContainsArguments of
> Initializer is true." with a similar rule for
> arguments-in-eval-in-initializer)
> 
> >> Source/JavaScriptCore/ChangeLog:23
> >> +        WTF::Vector<JSTextPosition>.  These are used to reparse the class
> > 
> > I think it'd be awesome to find some way to inline field instantiation. It might not be super trivial. Some issues I see:
> > - Teaching the parser how to swap over and parse something in a different context.
> > - Teaching the bytecode generator how to do some scope swapping to ensure we're not evaluating code in a scope we shouldn't be.
> 
> Agree --- do you see this as a blocker for landing? It seems suitable for a
> followup optimization. Loading the initializer function is unfortunate and
> may incur a very minor performance hit for ES6 classes, but I think that hit
> would be relatively minor, and probably too short lived to make it into a
> release.
> 
> >> Source/JavaScriptCore/ChangeLog:33
> >> +        (although a new EvalContextType is needed to handle the use of
> > 
> > We don't already know this? What are the early errors? Can you enumerate them?
> 
> `arguments` is not allowed in a direct eval call within a field initializer.
> 
> >> Source/JavaScriptCore/ChangeLog:54
> >> +            For each computed class field name, `[ {expr} ] = {initializer}`:
> > 
> > what happens for duplicate names? We call the set twice?
> 
> Yes --- If multiple computed fields become the same name during class field
> evaluation, then the same instance field would be defined multiple times per
> instance, per spec.
> 
> >> Source/JavaScriptCore/ChangeLog:64
> >> +            3) instance[fieldName] = value
> > 
> > Is this actually a Put. Or is this a defineOwnProperty?
> 
> For non-PrivateName fields, it's specced as CreateDataPropertyOrThrow, which
> is essentially a Put.
> 
> >> Source/JavaScriptCore/ChangeLog:86
> >> +           in some cases).
> > 
> > The DFG will indeed CSE these. It will also constant fold them if a class is only ever evaluated once (so the scope is only written to once).
> 
> in this patch it won't, we never really got that far with the optimization.
> But I agree it should be possible to do this better when we tier up.
> 
> >> Source/JavaScriptCore/ChangeLog:91
> >> +           cases, avoid adding new GetPutInfo complexity, etc
> > 
> > I guess I'm missing why this is actually needed. What about get|put_by_val doesn't work?
> 
> It's doable, however there are a number of reasons why I originally opted to
> go the other route:
> 
> It looks like get/put_by_value are mainly optimized for indexed property
> access (at least in baseline, not sure how they do after tiering up). They
> also handle a lot of different cases already,
> and private field support would add a number of other requirements, which
> would likely keep private fields permanently on the slow path, and
> dispatching to private field cases would have a slight impact
> on other code.
> Specifically, we'd need to dispatch for: private fields (distinct from
> existing PrivateIdentifier symbols, slightly different rules), brand
> checking (private methods/accessors), throwing if the field does not exist
> (or already exists in the DefineField case).
> 
> In addition to that, we'd probably need to wrap the private names in a
> JSCell, so that they can be fit in a JSValue, to avoid changing too much of
> the code. If we wound up using a raw UID and dispatching
> early based on a flag, then it doesn't really save any code. Allocating
> JSCells for each of these potentially eats up a lot of heap space if classes
> are evaluated multiple times (as is done in some of the mixin examples I've
> seen from JS developers).
> 
> We are happy to change this to use the existing opcodes before or after
> landing, but I think there are good reasons not to.
> 
> To summarize:
>   - avoid slowing down common operations by adding more dispatching
>   - avoid using up extra JS heap space by putting our UIDs in a JSCell
> wrapper (a new one for each field for each time the class is evaluated) for
> compat with op_get|put_by_val
>   - (very minor point) better developer ergonomics: easier to read
> "get_private_field #foo" vs the combination of ops that would be needed for
> the other cases
Ok. Let's keep them separate for now. If we have a good reason to change this in the future we can do that then.

> 
> >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4055
> >> +    case DefineFieldNode::PrivateName: {
> > 
> > Are these computed private names?
> > 
> > Anyways, in general, I have a question on computed private names. What are the semantics, of say:
> > class C {
> >    #["foo"] = 42
> >    #["foo"] = 44
> > }
> > 
> > Do I end up having two properties or one here?
> > 
> > Also, is there syntax to access private names in a computed fashion?
> 
> I believe this was answered by Dan earlier, but private names are never
> computed.
> 
> >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4057
> >> +        generator.emitPrivateFieldAdd(generator.thisRegister(), *m_ident, value.get());
> > 
> > Why isn't this just put_to_scope?
> 
> I'm not sure I understand how this would work with `put_to_scope` --- the
> method scope wouldn't be very useful, the class scope could be shared by
> multiple different instances (which might want to store different values in
> their private field).
> 
> >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2596
> >> +    # (Can we be sure there's room in the existing backing store for non-inline fields?)
> > 
> > What's up with this comment? Did you figure it out?
> 
> IIRC (and it's been a while since I wrote this),
> `storePropertyAtVariableOffset` takes care of bailing out if the backing
> store is not big enough, which means we can probably get rid of the comment.

[Saam Barati]

(In reply to Saam Barati from comment #73)
> Comment on attachment 360454 [details]
> Class fields, parser + bytecode + test changes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360454&action=review
> 
> > JSTests/ChangeLog:21
> > +        * stress/async-await-syntax.js:
> 
> Some fly by feedback: am I reading this right that there are no new tests
> besides what’s in test262? If so, this patch should be adding a lot of JSC
> specific tests. Tests that you think stress interesting implementation
> details and how your patch interacts with the JIT/GC

I don't think this has been addressed yet. We need a lot more testing of this.

[Saam Barati]

Hi Xan, can you comment on why the JSScope change is the way it is? I don't follow if there is a reason it has to be done that way opposed to just being statically part of the scope's inline fields.

[Saam Barati]

(In reply to Saam Barati from comment #80)
> (In reply to Saam Barati from comment #73)
> > Comment on attachment 360454 [details]
> > Class fields, parser + bytecode + test changes
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=360454&action=review
> > 
> > > JSTests/ChangeLog:21
> > > +        * stress/async-await-syntax.js:
> > 
> > Some fly by feedback: am I reading this right that there are no new tests
> > besides what’s in test262? If so, this patch should be adding a lot of JSC
> > specific tests. Tests that you think stress interesting implementation
> > details and how your patch interacts with the JIT/GC
> 
> I don't think this has been addressed yet. We need a lot more testing of
> this.

Ignore me, I just realized this is the original patch that I left comments on. I'm going to set r- for now based on my earlier feedback and Yusuke's request.

[Xan Lopez]

(In reply to Saam Barati from comment #81)
> Hi Xan, can you comment on why the JSScope change is the way it is? I don't
> follow if there is a reason it has to be done that way opposed to just being
> statically part of the scope's inline fields.

I think you are asking about what caitp commented on in comment #77; this either was necessary in an earlier iteration of the patch, and now does not, or we forgot the details. We'll try to simplify this and either include that in the next patch or get back to you with a proper explanation of why it's needed.

[Xan Lopez]

Created attachment 366588 [details]
Class fields, parser + bytecode + test changes

Here's a new version of the patch. The relevant changes are:

- Added a runtime flag to disable the feature.
- Added a bunch of tests, including many from V8 (found some bugs thanks to these).
- Reduced UnlinkedFunctionExecutable size to remain below the desired size limit.
- Fixed a number of bugs.
- Fixed various style issues.
- Explained some additional design decisions in the ChangeLog (especially the rationale for all the private name hashes in SymbolTable/JSSymbolTableObject/VariableEnvironment).

The patch series with all the work can be found at: https://github.com/xanlpz/webkit/tree/class-fields-fixes

[Caio Lima]

Created attachment 367749 [details]
Class fields, parser + byte code + test changes

I'm sending this because Xan is on holidays, but most of the Patch is an effort from Xan and Caitp, with some fixes that I've made.

There is a case where the implementation is not passing the test (see below), but we think it is ready for another batch of review.

```
{
  class C {
    x = Object.freeze(this);
    c = 42;
  }
  assertThrows(() => { new C; }, TypeError);
}
```

[EWS Watchlist]

Attachment 367749 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:204:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 367749 [details]
Class fields, parser + byte code + test changes

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

New failing tests:
stress/class-fields-private-out-of-line.js.ftl-eager-no-cjit

[Xan Lopez]

Created attachment 368117 [details]
Class fields, parser + bytecode + test changes

Patch refresh. Fix a style nit, punt failing new test, rebase to ToT.

[Caitlin Potter (:caitp)]

I've been working on inlinining field initialization (getting rid of the synthetic function), and while it's mostly straight forward to do this, there are some things that are a bit awkward.

In order to perform field initialization during bytecode generation, that AST for each field needs to be present somewhere. This should be sensible enough. The problem: The source code for the instance fields is never present when parsing the constructor, or a nested arrow function or eval() that calls super() (the places where the information is needed).

To work around this, we need to run a separate parse phase with the class source, and add the result to the AST somewhere (I have done this by adding an extra SourceElements field to ScopeNode to hold instance field initialization code).

The main difficulty is having this separate parse phase produce an AST and identifiers that share the same lifetime as the original parse phase, in order to actually use that AST.

Several ways I imagine doing this:

1) do the separate parse phase before creating the final root node (which will require passing additional parameters) --- This might be a pain to extend for other uses later on, and is a bit gross.

2) move core methods of Parser into a base class, and pass a WTF::Function/std::function to parse() to run after parsing, taking the ParserBase interface as a parameter in order to set up different SourceCode / update the lexer, etc, all while still using the same arena.

3) Add a constructor to Parser which reuses the Arena from some existing ParseArenaRoot by swapping with it (it looks like ParserArena is relatively cheap to swap), and explicitly run the extra parse phase where it's needed, and swapping again upon finishing.

So, I was wondering if anyone had any preference for those ideas, or maybe have some alternative vision for doing it, or if we should forget about inlining the field initializer function in baseline anyways, and just let optimizing phases do it.

[Caio Lima]

(In reply to Caitlin Potter (:caitp) from comment #89)
> 1) do the separate parse phase before creating the final root node (which
> will require passing additional parameters) --- This might be a pain to
> extend for other uses later on, and is a bit gross.
> 
> 2) move core methods of Parser into a base class, and pass a
> WTF::Function/std::function to parse() to run after parsing, taking the
> ParserBase interface as a parameter in order to set up different SourceCode
> / update the lexer, etc, all while still using the same arena.
> 
> 3) Add a constructor to Parser which reuses the Arena from some existing
> ParseArenaRoot by swapping with it (it looks like ParserArena is relatively
> cheap to swap), and explicitly run the extra parse phase where it's needed,
> and swapping again upon finishing.
> 
> So, I was wondering if anyone had any preference for those ideas, or maybe
> have some alternative vision for doing it, or if we should forget about
> inlining the field initializer function in baseline anyways, and just let
> optimizing phases do it.

I don't think that we should always fallback to "field initializer function", since it will be way slower than having `consturctor() { <fields_initialization> }`, maily when there is no JIT available.
IMHO, 3) sounds good, mainly if it is cheap to swap ParseArena. 2) sounds ok as well, but it maybe can make the code a little bit more complex than 3).

[Saam Barati]

(In reply to Caitlin Potter (:caitp) from comment #89)
> I've been working on inlinining field initialization (getting rid of the
> synthetic function), and while it's mostly straight forward to do this,
> there are some things that are a bit awkward.
> 
> In order to perform field initialization during bytecode generation, that
> AST for each field needs to be present somewhere. This should be sensible
> enough. The problem: The source code for the instance fields is never
> present when parsing the constructor, or a nested arrow function or eval()
> that calls super() (the places where the information is needed).
> 
> To work around this, we need to run a separate parse phase with the class
> source, and add the result to the AST somewhere (I have done this by adding
> an extra SourceElements field to ScopeNode to hold instance field
> initialization code).
> 
> The main difficulty is having this separate parse phase produce an AST and
> identifiers that share the same lifetime as the original parse phase, in
> order to actually use that AST.
> 
> Several ways I imagine doing this:
> 
> 1) do the separate parse phase before creating the final root node (which
> will require passing additional parameters) --- This might be a pain to
> extend for other uses later on, and is a bit gross.
> 
> 2) move core methods of Parser into a base class, and pass a
> WTF::Function/std::function to parse() to run after parsing, taking the
> ParserBase interface as a parameter in order to set up different SourceCode
> / update the lexer, etc, all while still using the same arena.
> 
> 3) Add a constructor to Parser which reuses the Arena from some existing
> ParseArenaRoot by swapping with it (it looks like ParserArena is relatively
> cheap to swap), and explicitly run the extra parse phase where it's needed,
> and swapping again upon finishing.
> 
> So, I was wondering if anyone had any preference for those ideas, or maybe
> have some alternative vision for doing it, or if we should forget about
> inlining the field initializer function in baseline anyways, and just let
> optimizing phases do it.

I think we should definitely do the inlining. However, depending on what you think, I think it's a reasonable enhancement to do as a followup patch.

[Caitlin Potter (:caitp)]

(In reply to Saam Barati from comment #91)
> I think we should definitely do the inlining. However, depending on what you
> think, I think it's a reasonable enhancement to do as a followup patch.

Do any of those designs seem like the right way to do it, or do you have a different idea?

[Saam Barati]

(In reply to Caitlin Potter (:caitp) from comment #92)
> (In reply to Saam Barati from comment #91)
> > I think we should definitely do the inlining. However, depending on what you
> > think, I think it's a reasonable enhancement to do as a followup patch.
> 
> Do any of those designs seem like the right way to do it, or do you have a
> different idea?

We only need these field initializations when generating byte code for the constructor, right? Why not leave metadata around for the constructor function as to where each field is. When we go to reparse the constructor we can then also parse those field initializers? Or perhaps before parsing the constructor.

Maybe this is bad because it bakes in that we'll parse once with SyntaxChecker before using the ASTBuilder.

Finding a way to keep using the same arena seems like the right approach. I think an entrypoint for this thing doesn't sound crazy to me, but hopefully it wouldn't be that different than the pre-existing entry points. It'd just end up appending all these fields to some list of AST nodes.

I guess I like something like (2), but I'm not sure we really need a different class. I think we can just have an entrypoint into this type of a parse.

[Caitlin Potter (:caitp)]

(In reply to Saam Barati from comment #93)
> (In reply to Caitlin Potter (:caitp) from comment #92)
> > (In reply to Saam Barati from comment #91)
> > > I think we should definitely do the inlining. However, depending on what you
> > > think, I think it's a reasonable enhancement to do as a followup patch.
> > 
> > Do any of those designs seem like the right way to do it, or do you have a
> > different idea?
> 
> We only need these field initializations when generating byte code for the
> constructor, right? Why not leave metadata around for the constructor
> function as to where each field is. When we go to reparse the constructor we
> can then also parse those field initializers? Or perhaps before parsing the
> constructor.

Not necessarily — It could be for an arrow function or direct eval inside the constructor.

> Maybe this is bad because it bakes in that we'll parse once with
> SyntaxChecker before using the ASTBuilder.
> 
> Finding a way to keep using the same arena seems like the right approach. I
> think an entrypoint for this thing doesn't sound crazy to me, but hopefully
> it wouldn't be that different than the pre-existing entry points. It'd just
> end up appending all these fields to some list of AST nodes.
> 
> I guess I like something like (2), but I'm not sure we really need a
> different class. I think we can just have an entrypoint into this type of a
> parse.

The reason you’d use a separate class, it would look something like this:

‘’’
rootNode = parse<NodeType>(vm, source, ..., [&](ParserBase& parser) {
  // After parsing the ParseNode root, before creating it
  initializer = parser.parseClassInstanceFields(classSource, instanceFieldPositions);
});
rootNode->setInstanceFieldInitializer(initializer);
‘’’

Having stuff in ParserBase means the callback doesn’t have to care about the lexer type.

But, it is still awkward. It’s challenging to find the right abstraction.

[Xan Lopez]

Created attachment 369295 [details]
Class fields, parser + bytecode + test changes

New patch.
- Rebased against ToT.
- Some additional bug fixes.

[EWS Watchlist]

Comment on attachment 369295 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
svg/repaint/remove-border-property-on-root.html
legacy-animation-engine/compositing/reflections/load-video-in-reflection.html

[EWS Watchlist]

Created attachment 369383 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Xan Lopez]

Created attachment 371258 [details]
Class fields, parser + bytecode + test changes

New patch. Changes:

- Rebased against ToT.
- Only load and call @instanceFieldInitializer when needed.
- Fix last V8 test.
- Fix new.target and super property restrictions.

[EWS Watchlist]

Comment on attachment 371258 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
js/function-toString-vs-name.html
storage/indexeddb/result-request-cycle.html
inspector/console/command-line-api-getEventListeners.html

[EWS Watchlist]

Created attachment 371265 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 371258 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
inspector/console/command-line-api-getEventListeners.html
http/wpt/service-workers/service-worker-networkprocess-crash.html
storage/indexeddb/result-request-cycle.html
js/function-toString-vs-name.html

[EWS Watchlist]

Created attachment 371266 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 371258 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
es6.yaml/es6/function_name_property_object_methods_function.js.default
stress/class-fields-harmony.js.bytecode-cache
stress/class-fields-private-cached-bytecode.js.bytecode-cache
stress/class-fields-private-use-eval.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-ftl-no-cjit
stress/class-fields-function-name.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-llint
stress/class-fields-bytecode-cache.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-ftl-eager-no-cjit
apiTests

[EWS Watchlist]

Comment on attachment 371258 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
js/function-toString-vs-name.html
storage/indexeddb/result-request-cycle.html
inspector/console/command-line-api-getEventListeners.html

[EWS Watchlist]

Created attachment 371267 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 371258 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
storage/indexeddb/result-request-cycle.html
js/function-toString-vs-name.html

[EWS Watchlist]

Created attachment 371268 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5

[Xan Lopez]

Created attachment 371270 [details]
Class fields, parser + bytecode + test changes

New patch.
- Try to fix fallout after https://bugs.webkit.org/show_bug.cgi?id=197979

[EWS Watchlist]

Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
js/function-toString-vs-name.html
storage/indexeddb/result-request-cycle.html
inspector/console/command-line-api-getEventListeners.html

[EWS Watchlist]

Created attachment 371272 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
inspector/console/command-line-api-getEventListeners.html
http/wpt/service-workers/service-worker-networkprocess-crash.html
storage/indexeddb/result-request-cycle.html
js/function-toString-vs-name.html

[EWS Watchlist]

Created attachment 371273 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
es6.yaml/es6/function_name_property_object_methods_function.js.default
stress/class-fields-harmony.js.bytecode-cache
stress/class-fields-private-cached-bytecode.js.bytecode-cache
stress/class-fields-private-use-eval.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-ftl-no-cjit
stress/class-fields-function-name.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-llint
stress/class-fields-bytecode-cache.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-ftl-eager-no-cjit
apiTests

[EWS Watchlist]

Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
js/function-toString-vs-name.html
storage/indexeddb/result-request-cycle.html
inspector/console/command-line-api-getEventListeners.html

[EWS Watchlist]

Created attachment 371276 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
storage/indexeddb/result-request-cycle.html
js/function-toString-vs-name.html

[EWS Watchlist]

Created attachment 371277 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5

[EWS Watchlist]

Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
editing/style/apply-style-iframe-crash.html
editing/style/iframe-onload-crash-mac.html
js/function-toString-vs-name.html

[EWS Watchlist]

Created attachment 371278 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[jsc-armv7 EWS]

Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

Attachment 371270 [details] did not pass jsc-armv7-ews (jsc-only):
Output: https://webkit-queues.webkit.org/results/12373585

New failing tests:
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-dfg-eager-no-cjit
es6.yaml/es6/function_name_property_object_methods_function.js.default
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-llint
apiTests

[Xan Lopez]

Created attachment 371383 [details]
Class fields, parser + bytecode + test changes

New patch. Try to fix previous failures in bots (code cache and toString bugs).

[Xan Lopez]

Created attachment 371384 [details]
Class fields, parser + bytecode + test changes

Rebased again against ToT.

[Xan Lopez]

Created attachment 371387 [details]
Class fields, parser + bytecode + test changes

New patch.

Bots seem to be green now, but merge one more fix.

[EWS Watchlist]

Comment on attachment 371387 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
editing/style/iframe-onload-crash-mac.html
editing/style/apply-style-iframe-crash.html

[EWS Watchlist]

Created attachment 371392 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Xan Lopez]

Created attachment 373112 [details]
Class fields, parser + bytecode + test changes

New patch:
- Rebased against ToT (28/06/19)

[EWS Watchlist]

Comment on attachment 373112 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
editing/style/iframe-onload-crash-mac.html
editing/style/apply-style-iframe-crash.html

[EWS Watchlist]

Created attachment 373117 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[EWS Watchlist]

Comment on attachment 373112 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
stress/regress-181388.js.bytecode-cache
stress/super-property-access-object-literal-to-this.js.bytecode-cache
stress/modules-syntax-error.js.bytecode-cache
stress/initialize_functions_after_arguments.js.bytecode-cache
stress/regress-185896.js.bytecode-cache
stress/get-by-id-direct.js.bytecode-cache
stress/lexical-let-not-strict-mode.js.bytecode-cache
stress/closure-recursive-tail-call-infinite-loop.js.bytecode-cache
stress/module-namespace-access-poly.js.bytecode-cache
stress/arith-nodes-abstract-interpreter-untypeduse.js.bytecode-cache
microbenchmarks/captured-assignments.js.bytecode-cache
stress/class-syntax-tdz-in-eval.js.bytecode-cache
stress/proxy-get-prototype-of.js.bytecode-cache
stress/proxy-all-the-parameters.js.bytecode-cache
stress/reserved-word-with-escape.js.bytecode-cache
microbenchmarks/raytrace-with-empty-try-catch.js.bytecode-cache
microbenchmarks/eval-cached.js.bytecode-cache
stress/activation-sink-default-value-tdz-error.js.bytecode-cache
stress/sampling-profiler-microtasks.js.bytecode-cache
stress/activation-sink.js.bytecode-cache
stress/arrowfunction-activation-sink-osrexit.js.bytecode-cache
stress/class-fields-private-use-eval.js.bytecode-cache
stress/value-div-ai-rule.js.bytecode-cache
stress/object-number-properties.js.bytecode-cache
stress/rest-parameter-basics.js.bytecode-cache
microbenchmarks/arrowfunciton-reference-arguments.js.bytecode-cache
stress/eval-func-decl-in-frozen-global.js.bytecode-cache
stress/async-iteration-syntax.js.bytecode-cache
microbenchmarks/generate-multiple-llint-entrypoints.js.bytecode-cache
stress/type-check-hoisting-phase-hoist-check-structure-on-tdz-this-value.js.bytecode-cache
stress/proxy-revoke.js.bytecode-cache
stress/symbol-tostringtag.js.bytecode-cache
stress/for-in-invalidation-for-any-write.js.bytecode-cache
stress/eval-in-arrow-function.js.bytecode-cache
stress/dfg-object-proto-accessor.js.bytecode-cache
stress/proxy-basic.js.bytecode-cache
microbenchmarks/getter-no-activation.js.bytecode-cache
stress/sloppy-mode-hoist-arguments-function-non-simple-parameter-list.js.bytecode-cache
stress/raise-error-in-iterator-close.js.bytecode-cache
microbenchmarks/data-view-accesses-2.js.bytecode-cache
stress/constant-closure-var-with-dynamic-invalidation.js.bytecode-cache
stress/function-with-defaults-inlining.js.bytecode-cache
stress/object-is-fold-to-compare-eq-ptr.js.bytecode-cache
stress/async-await-mozilla.js.bytecode-cache
stress/code-cache-incorrect-caching.js.bytecode-cache
stress/istypedarrayview-intrinsic.js.bytecode-cache
sunspider-1.0/3d-raytrace.js.bytecode-cache
microbenchmarks/large-int-captured.js.bytecode-cache
stress/arrowfunction-lexical-bind-supercall-4.js.bytecode-cache
stress/activation-sink-osrexit-default-value-tdz-error.js.bytecode-cache
stress/arrowfunction-lexical-bind-arguments-non-strict-1.js.bytecode-cache
stress/spec-empty-flows-through-cell-checks.js.bytecode-cache
stress/typedarray-sort.js.bytecode-cache
microbenchmarks/singleton-scope.js.bytecode-cache
stress/tricky-infinite-loop-that-uses-captured-variables.js.bytecode-cache
stress/heap-allocator-allocates-incorrect-size-for-activation.js.bytecode-cache
stress/class-fields-harmony.js.bytecode-cache
stress/proxy-is-extensible.js.bytecode-cache
stress/string-raw.js.bytecode-cache
microbenchmarks/is-not-cell-speculation-for-empty-value.js.bytecode-cache
stress/arrowfunction-tdz-4.js.bytecode-cache
stress/generator-name.js.bytecode-cache
stress/domjit-exception-ic.js.bytecode-cache
stress/phantom-new-array-buffer-forward-varargs.js.bytecode-cache
stress/adhoc-setter-frame-should-emit-setlocal-again.js.bytecode-cache
stress/proxy-own-keys.js.bytecode-cache
stress/generator-eval-this.js.bytecode-cache
stress/constant-folding-phase-insert-check-handle-varargs.js.bytecode-cache
stress/class-method-does-not-declare-variable-to-upper-scope.js.bytecode-cache
stress/proxy-with-private-symbols.js.bytecode-cache
stress/to-number-throws-correct-exception.js.bytecode-cache
stress/eval-func-decl-block-scoping-reassign.js.bytecode-cache
stress/value-add-on-double-array-with-holes.js.bytecode-cache
stress/arrowfunction-run-10-2.js.bytecode-cache
stress/arrowfunction-lexical-this-sinking-osrexit.js.bytecode-cache
stress/allocation-sinking-puthint-control-flow.js.bytecode-cache
stress/put-indexed-getter-setter.js.bytecode-cache
stress/dataview-jit-bounds-checks.js.bytecode-cache
stress/object-rest-deconstruct.js.bytecode-cache
stress/arrowfunction-lexical-bind-newtarget.js.bytecode-cache
stress/string-repeat-edge-cases.js.bytecode-cache
stress/typedarray-find.js.bytecode-cache
stress/regress-153486.js.bytecode-cache
microbenchmarks/infer-one-time-closure-ten-vars.js.bytecode-cache
stress/sorting-boolean-result-comparator.js.bytecode-cache
stress/generator-fib-ftl-and-string.js.bytecode-cache
stress/function-configurable-properties.js.bytecode-cache
stress/create-subclass-structure-may-throw-exception-when-getting-prototype.js.bytecode-cache
stress/function-with-defaults-non-inlining.js.bytecode-cache
stress/proxy-get-own-property.js.bytecode-cache
microbenchmarks/sorting-benchmark.js.bytecode-cache
stress/error-messages-for-in-operator-should-not-crash.js.bytecode-cache
stress/template-literal-line-terminators.js.bytecode-cache
stress/create-error-out-of-memory-rope-string.js.bytecode-cache
stress/template-literal.js.bytecode-cache
stress/proxy-instanceof.js.bytecode-cache
stress/for-in-of-const.js.bytecode-cache
stress/activation-sink-osrexit-default-value.js.bytecode-cache
stress/typedarray-configure-index.js.bytecode-cache
stress/rest-parameter-is-destructuring.js.bytecode-cache
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline
stress/yield-label-generator.js.bytecode-cache
stress/compare-number-and-other.js.bytecode-cache
sunspider-1.0/string-tagcloud.js.bytecode-cache
stress/spread-capture-rest.js.bytecode-cache
stress/ai-create-this-to-new-object-fire.js.bytecode-cache
stress/scoped-arguments-out-of-bounds-change-structure.js.bytecode-cache
stress/materialize-activation-referenced-from-phantom-function.js.bytecode-cache
stress/force-exit-then-eval-dfg.js.bytecode-cache
stress/dataview-jit-set.js.bytecode-cache
stress/eval-func-decl-in-eval-within-block-with-let.js.bytecode-cache
microbenchmarks/infer-one-time-closure.js.bytecode-cache
microbenchmarks/infer-closure-const-then-reenter.js.bytecode-cache
stress/infinite-loop-that-uses-captured-variables-but-they-do-not-escape.js.bytecode-cache
stress/array-fill-put-by-val.js.bytecode-cache
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases
stress/regexp-syntax-error-invalid-flags.js.bytecode-cache
stress/rest-elements.js.bytecode-cache
stress/inline-closure-call.js.bytecode-cache
microbenchmarks/string-rope-with-object.js.bytecode-cache
stress/proxy-underscore-proto.js.bytecode-cache
stress/async-arrow-functions-lexical-this-binding.js.bytecode-cache
stress/proxy-delete.js.bytecode-cache
stress/proxy-json.js.bytecode-cache
stress/module-namespace-access-non-constant.js.bytecode-cache
stress/tagged-templates.js.bytecode-cache
stress/cloned-arguments-should-visit-callee-during-gc.js.bytecode-cache
stress/module-namespace-access.js.bytecode-cache
stress/relaxed-line-terminators-in-string.js.bytecode-cache
stress/proxy-returning-incomplete-property-descriptor.js.bytecode-cache
stress/IIFE-function-name-captured.js.bytecode-cache
stress/arity-mismatch-arguments-length.js.bytecode-cache
stress/reflect-set-receiver-proxy-set.js.bytecode-cache
stress/generator-function-declaration-sinking-osrexit.js.bytecode-cache
stress/direct-eval-in-object-literal-methods.js.bytecode-cache
stress/do-eval-virtual-call-correctly.js.bytecode-cache
stress/tdz-this-in-try-catch.js.bytecode-cache
microbenchmarks/emscripten-cube2hash.js.bytecode-cache
stress/async-iteration-yield-star-interface.js.bytecode-cache
stress/tricky-inferred-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js.bytecode-cache
stress/generic-arguments-correct-delete-behavior.js.bytecode-cache
stress/trailing-comma-in-patterns.js.bytecode-cache
stress/lexical-let-and-with-statement.js.bytecode-cache
stress/yield-star-throw-continue.js.bytecode-cache
stress/completion-value.js.bytecode-cache
stress/catch-parameter-syntax.js.bytecode-cache
microbenchmarks/scoped-arguments-possibly-overridden-length.js.bytecode-cache
stress/scoped-then-direct-arguments-get-by-val-in-baseline.js.bytecode-cache
stress/catch-parameter-destructuring.js.bytecode-cache
stress/promise-finally.js.bytecode-cache
stress/class-syntax-no-tdz-in-eval.js.bytecode-cache
microbenchmarks/sink-huge-activation.js.bytecode-cache
microbenchmarks/function-with-eval.js.bytecode-cache
stress/const-not-strict-mode.js.bytecode-cache
stress/domjit-exception.js.bytecode-cache
stress/get-declared-unpassed-argument-in-scoped-arguments.js.bytecode-cache
stress/singleton-scope-then-overwrite.js.bytecode-cache
stress/activation-sink-osrexit.js.bytecode-cache
stress/spread-correct-global-object-on-exception.js.bytecode-cache
stress/string-object-put-by-index.js.bytecode-cache
stress/eval-func-decl-block-with-var-and-remove.js.bytecode-cache
stress/string-compare.js.bytecode-cache
stress/regress-182419.js.bytecode-cache
stress/generator-return-before-first-call.js.bytecode-cache
stress/typed-array-canonical-numeric-index-string.js.bytecode-cache
stress/new-target.js.bytecode-cache
stress/operation-in-throw-error.js.bytecode-cache
stress/create-list-from-array-like-not-object.js.bytecode-cache
microbenchmarks/simple-poly-call-nested.js.bytecode-cache
stress/eval-that-is-not-eval.js.bytecode-cache
stress/var-injection-cache-invalidation.js.bytecode-cache
microbenchmarks/generator-sunspider-access-nsieve.js.bytecode-cache
stress/regress-170661.js.bytecode-cache
stress/phantom-spread-forward-varargs.js.bytecode-cache
stress/dont-crash-when-hoist-check-structure-on-tdz.js.bytecode-cache
microbenchmarks/set-for-each.js.bytecode-cache
stress/for-in-array-mode.js.bytecode-cache
stress/typedarray-set.js.bytecode-cache
stress/string-prototype-scopes.js.bytecode-cache
stress/infer-uninitialized-closure-var.js.bytecode-cache
microbenchmarks/raytrace-with-try-catch.js.bytecode-cache
stress/proxy-of-a-proxy.js.bytecode-cache
stress/native-error-properties.js.bytecode-cache
stress/new-target-syntax-errors.js.bytecode-cache
stress/forward-varargs-double-new-array-buffer.js.bytecode-cache
stress/obj-rest-destructuring-order.js.bytecode-cache
stress/async-arrow-functions-lexical-arguments-binding.js.bytecode-cache
stress/goofy-function-reentry-incorrect-inference.js.bytecode-cache
stress/yield-named-variable-generator.js.bytecode-cache
stress/eval-script-contains-null-character.js.bytecode-cache
stress/jit-gracefully-handle-double-constants-in-math-operators.js.bytecode-cache
stress/async-generator-assertion.js.bytecode-cache
stress/regress-168546.js.bytecode-cache
stress/yield-out-of-generator.js.bytecode-cache
stress/typedarray-of.js.bytecode-cache
stress/regexp-matches-array-bad-time.js.bytecode-cache
microbenchmarks/map-for-each.js.bytecode-cache
stress/generator-with-this-strict.js.bytecode-cache
stress/dataview-jit-unaligned-accesses.js.bytecode-cache
stress/get-prototype-create-this-effectful.js.bytecode-cache
stress/unscopables.js.bytecode-cache
microbenchmarks/eval-code-ftl.js.bytecode-cache
stress/catch-set-argument-speculation-failure.js.bytecode-cache
stress/generator-arguments.js.bytecode-cache
stress/array-message-passing.js.bytecode-cache
stress/function-reentry-infer-on-self.js.bytecode-cache
stress/async-function-declaration-sinking-put.js.bytecode-cache
stress/import-syntax.js.bytecode-cache
stress/class-syntax-super-in-eval.js.bytecode-cache
stress/generator-with-this.js.bytecode-cache
stress/typedarray-view-string-properties-neutered.js.bytecode-cache
stress/eval-func-decl-in-if.js.bytecode-cache
stress/arrow-function-syntax.js.bytecode-cache
stress/arrowfunction-bound.js.bytecode-cache
stress/global-environment-does-not-trap-unscopables.js.bytecode-cache
stress/import-from-eval.js.bytecode-cache
stress/const-loop-semantics.js.bytecode-cache
stress/eval-func-decl-block-with-var-sinthesize.js.bytecode-cache
stress/sink-object-past-invalid-check-sneaky.js.bytecode-cache
stress/disposable-call-site-index-with-call-and-this.js.bytecode-cache
stress/sampling-profiler-bound-function-name.js.bytecode-cache
stress/activation-sink-default-value.js.bytecode-cache
stress/global-lexical-binding-epoch-should-be-correct-one.js.bytecode-cache
stress/put-direct-index-broken-2.js.bytecode-cache
microbenchmarks/call-or-not-call.js.bytecode-cache
stress/sampling-profiler-deep-stack.js.bytecode-cache
stress/check-structure-ir-ensures-empty-does-not-flow-through.js.bytecode-cache
stress/math-pow-stable-results.js.bytecode-cache
stress/clobberize-needs-to-model-spread-effects.js.bytecode-cache
stress/typedarray-indexOf.js.bytecode-cache
stress/generator-arguments-from-function.js.bytecode-cache
stress/iterator-field-order.js.bytecode-cache
stress/typedarray-bad-getter.js.bytecode-cache
stress/block-scope-redeclarations.js.bytecode-cache
microbenchmarks/Float32Array-matrix-mult.js.bytecode-cache
microbenchmarks/arrowfunciton-direct-arguments.js.bytecode-cache
stress/compare-eq-should-use-known-other-use.js.bytecode-cache
stress/async-await-basic.js.bytecode-cache
stress/ai-needs-to-model-spreads-effects.js.bytecode-cache
stress/arith-profile-for-negate-can-see-non-number-due-to-dfg-osr-exit-profiling.js.bytecode-cache
microbenchmarks/data-view-accesses.js.bytecode-cache
stress/arrowfunction-lexical-this-activation-sink.js.bytecode-cache
stress/arrowfunction-call.js.bytecode-cache
stress/typedarray-every.js.bytecode-cache
stress/regress-187074.js.bytecode-cache
stress/to-object-intrinsic-null-or-undefined-edge.js.bytecode-cache
sunspider-1.0/date-format-tofte.js.bytecode-cache
stress/for-in-capture-string-loop-var.js.bytecode-cache
stress/async-arrow-functions-lexical-new.target-binding.js.bytecode-cache
microbenchmarks/infer-closure-const-then-mov.js.bytecode-cache
stress/proxy-set.js.bytecode-cache
stress/super-property-access-exceptions.js.bytecode-cache
stress/async-function-declaration-sinking-osrexit.js.bytecode-cache
stress/tagged-templates-raw-strings.js.bytecode-cache
stress/generator-fib-ftl-and-array.js.bytecode-cache
stress/proxy-prevent-extensions.js.bytecode-cache
stress/generator-return.js.bytecode-cache
stress/sink-object-past-invalid-check.js.bytecode-cache
stress/arrow-functions-as-default-parameter-values.js.bytecode-cache
stress/typedarray-lastIndexOf.js.bytecode-cache
stress/dead-access-to-captured-variable-preceded-by-a-live-store.js.bytecode-cache
stress/default-value-parsing-should-propagate-error.js.bytecode-cache
stress/arrowfunction-lexical-bind-this-8.js.bytecode-cache
stress/tagged-templates-in-function-in-direct-eval.js.bytecode-cache
stress/force-exit-then-eval.js.bytecode-cache
stress/freeze-and-seal-should-prevent-extensions.js.bytecode-cache
microbenchmarks/simple-activation-demo.js.bytecode-cache
stress/arguments-iterator.js.bytecode-cache
stress/iterator-return-beyond-multiple-iteration-scopes.js.bytecode-cache
stress/async-iteration-basic.js.bytecode-cache
stress/arrowfunction-lexical-this-sinking-put.js.bytecode-cache
stress/destructuring-rest-element.js.bytecode-cache
stress/class_elements.js.bytecode-cache
stress/yield-reserved-word.js.bytecode-cache
stress/class-fields-stress-instance.js.bytecode-cache
microbenchmarks/polyvariant-get-by-id-shorter-tower.js.bytecode-cache
stress/captured-arguments-variable.js.bytecode-cache
stress/arrowfunction-tdz-3.js.bytecode-cache
stress/es6-default-parameters.js.bytecode-cache
stress/regexp-replace-proxy.js.bytecode-cache
stress/arrowfunction-lexical-bind-this-2.js.bytecode-cache
stress/eval-func-decl-in-block-scope-and-bind-to-top-eval-scope.js.bytecode-cache
stress/destructuring-assignment-syntax.js.bytecode-cache
stress/stress-cleared-calllinkinfo.js.bytecode-cache
stress/lexical-let-tdz.js.bytecode-cache
stress/proxy-has-property.js.bytecode-cache
stress/arrowfunction-activation-sink.js.bytecode-cache
stress/to-this-before-arrow-function-closes-over-this-that-starts-as-lexical-environment.js.bytecode-cache
stress/typedarray-forEach.js.bytecode-cache
stress/typedarray-reduceRight.js.bytecode-cache
stress/generator-frame-empty.js.bytecode-cache
stress/yield-label.js.bytecode-cache
stress/type-of-functions-and-objects.js.bytecode-cache
sunspider-1.0/date-format-xparb.js.bytecode-cache
stress/module-namespace-access-transitive-exports.js.bytecode-cache
stress/put-local-conservative.js.bytecode-cache
stress/const-exception-handling.js.bytecode-cache
stress/async-iteration-yield-star.js.bytecode-cache
microbenchmarks/eval-code-ftl-reentry.js.bytecode-cache
stress/class-syntax-definition-semantics.js.bytecode-cache
microbenchmarks/implicit-bigswitch-indirect-symbol.js.bytecode-cache
stress/generator-with-new-target.js.bytecode-cache
stress/class-derived-from-null.js.bytecode-cache
stress/arrowfunction-lexical-bind-arguments-non-strict-2.js.bytecode-cache
stress/tricky-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js.bytecode-cache
stress/infinite-loop-that-uses-captured-variables-with-osr-entry.js.bytecode-cache
stress/async-await-module-reserved-word.js.bytecode-cache
stress/generator-reduced-save-point-put-to-scope.js.bytecode-cache
stress/const-semantics.js.bytecode-cache
stress/super-property-access.js.bytecode-cache
stress/object-entries.js.bytecode-cache
stress/ftl-tail-call-throw-exception-from-slow-path-recover-stack-values.js.bytecode-cache
stress/arrowfunction-run-10000-2.js.bytecode-cache
stress/global-import-function-should-return-a-promise-when-clearing-exceptions.js.bytecode-cache
stress/async-iteration-yield-promise.js.bytecode-cache
stress/catch-variables-under-tdz.js.bytecode-cache
stress/generator-transfer-register-beyond-mutiple-yields.js.bytecode-cache
microbenchmarks/large-int.js.bytecode-cache
stress/typedarray-filter.js.bytecode-cache
stress/super-in-lexical-scope.js.bytecode-cache
stress/rest-parameter-various-types.js.bytecode-cache
stress/bug-165091.js.bytecode-cache
stress/ordinary-set-exceptions.js.bytecode-cache
stress/class-fields-private-out-of-line.js.bytecode-cache
stress/generator-function-expression-sinking-osrexit.js.bytecode-cache
microbenchmarks/create-lots-of-functions.js.bytecode-cache
stress/generator-syntax.js.bytecode-cache
stress/exception-dfg-not-strict-equal.js.bytecode-cache
stress/array-species-functions.js.bytecode-cache
stress/to-this-on-constant-lexical-environment.js.bytecode-cache
stress/arrowfunction-lexical-bind-supercall-1.js.bytecode-cache
stress/function-constructor-semantics.js.bytecode-cache
stress/object-assign-correctness.js.bytecode-cache
stress/dfg-reflect-get-prototype-of.js.bytecode-cache
stress/async-function-expression-sinking-put.js.bytecode-cache
stress/inferred-types-regex-matches-array.js.bytecode-cache
stress/generator-type-check.js.bytecode-cache
stress/generator-this.js.bytecode-cache
microbenchmarks/eval-not-eval-compute-args.js.bytecode-cache
stress/regexp-last-index-writable.js.bytecode-cache
stress/destructuring-assignment-require-object-coercible.js.bytecode-cache
stress/spread-calling.js.bytecode-cache
microbenchmarks/fold-double-to-int.js.bytecode-cache
stress/infinite-loop-that-uses-captured-variables-before-throwing.js.bytecode-cache
stress/const-and-with-statement.js.bytecode-cache
stress/async-await-syntax.js.bytecode-cache
stress/tagged-templates-identity.js.bytecode-cache
microbenchmarks/generator-fib.js.bytecode-cache
stress/eval-func-decl-block-with-remove.js.bytecode-cache
stress/typed-array-put-by-val-profiling.js.bytecode-cache
stress/jsc-read.js.bytecode-cache
stress/arrowfunction-lexical-bind-this-7.js.bytecode-cache
stress/arrowfunction-lexical-bind-this-4.js.bytecode-cache
stress/arith-sub-on-double-array-with-holes.js.bytecode-cache
stress/proxy-with-statement.js.bytecode-cache
stress/typedarray-reduce.js.bytecode-cache
stress/get-array-length-concurrently-change-mode.js.bytecode-cache
stress/arith-add-on-double-array-with-holes.js.bytecode-cache
stress/typedarray-copyWithin.js.bytecode-cache
stress/poly-call-exit.js.bytecode-cache
stress/typedarray-slice.js.bytecode-cache
stress/generator-throw-before-first-call.js.bytecode-cache
stress/lexical-let-exception-handling.js.bytecode-cache
stress/proxy-stack-overflow-exceptions.js.bytecode-cache
stress/parse-int-intrinsic.js.bytecode-cache
stress/simple-module.mjs.bytecode-cache
stress/pow-evaluation-order.js.bytecode-cache
stress/create-subclass-structure-might-throw.js.bytecode-cache
sunspider-1.0/string-unpack-code.js.bytecode-cache
stress/generator-fib-ftl-and-object.js.bytecode-cache
stress/marked-argument-buffer.js.bytecode-cache
microbenchmarks/infer-closure-const-then-mov-no-inline.js.bytecode-cache
stress/eval-func-decl-in-eval-within-with-scope.js.bytecode-cache
stress/proxy-construct.js.bytecode-cache
stress/arrowfunction-run-10000-1.js.bytecode-cache
stress/modules-syntax-error-with-names.js.bytecode-cache
stress/arrowfunction-lexical-bind-supercall-2.js.bytecode-cache
stress/global-object-proto-getter.js.bytecode-cache
stress/optional-catch-binding-syntax.js.bytecode-cache
stress/typeof-symbol.js.bytecode-cache
stress/arrowfunction-lexical-this-sinking-no-double-allocate.js.bytecode-cache
stress/block-scoped-function-declarations.js.bytecode-cache
stress/get-by-offset-should-use-correct-child.js.bytecode-cache
stress/v8-regexp-strict.js.bytecode-cache
stress/typedarray-subarray.js.bytecode-cache
microbenchmarks/polyvariant-get-by-id-tower.js.bytecode-cache
stress/generator-function-expression-sinking-put.js.bytecode-cache
stress/typedarray-getter-on-self.js.bytecode-cache
stress/regress-151324.js.bytecode-cache
stress/yield-and-line-terminator.js.bytecode-cache
stress/spread-non-array.js.bytecode-cache
stress/import-basic.js.bytecode-cache
stress/dont-validate-stack-offset-in-b3-because-it-might-be-guarded-by-control-flow.js.bytecode-cache
stress/re-execute-error-module.js.bytecode-cache
stress/async-iteration-async-from-sync.js.bytecode-cache
stress/dataview-typedarray-toindex.js.bytecode-cache
stress/const-tdz.js.bytecode-cache
stress/arrowfunction-lexical-bind-this-1.js.bytecode-cache
stress/v8-splay-strict.js.bytecode-cache
stress/arrowfunction-lexical-this-activation-sink-osrexit.js.bytecode-cache
microbenchmarks/eval-not-eval-compute.js.bytecode-cache
stress/array-copywithin.js.bytecode-cache
stress/generator-function-declaration-sinking-put.js.bytecode-cache
stress/big-wasm-memory-grow.js.bytecode-cache
microbenchmarks/scoped-arguments-length.js.bytecode-cache
stress/inline-call-to-recursive-tail-call.js.bytecode-cache
stress/class-syntax-block-scoping.js.bytecode-cache
stress/regexp-replace-double-watchpoint.js.bytecode-cache
stress/values-unscopables.js.bytecode-cache
stress/call-apply-exponential-bytecode-size.js.bytecode-cache
stress/to-property-key-correctness.js.bytecode-cache
microbenchmarks/large-int-neg.js.bytecode-cache
microbenchmarks/Int16Array-bubble-sort.js.bytecode-cache
stress/source-origin.js.bytecode-cache
stress/arrow-function-token-is-not-keyword.js.bytecode-cache
stress/arrowfunction-lexical-bind-this-6.js.bytecode-cache
stress/eval-cached.js.bytecode-cache
stress/reflect-set-with-global-proxy.js.bytecode-cache
stress/reflect-construct.js.bytecode-cache
stress/lexical-scoping-break-continue.js.bytecode-cache
stress/typedarray-some.js.bytecode-cache
stress/generator-with-super.js.bytecode-cache
stress/tagged-template-object-collect.js.bytecode-cache
stress/tagged-templates-in-direct-eval-should-not-produce-same-site-object.js.bytecode-cache
stress/inferred-infinite-loop-that-uses-captured-variables.js.bytecode-cache
stress/lexical-let-global-not-captured-variables.js.bytecode-cache
stress/trailing-comma-in-function-parameters.js.bytecode-cache
stress/catch-clause-should-be-under-tdz4.js.bytecode-cache
stress/let-lexical-binding-shadowing-global-properties-and-eval-injection.js.bytecode-cache
stress/activation-test-loop.js.bytecode-cache
stress/capture-escape-and-throw.js.bytecode-cache
stress/string-replace-proxy.js.bytecode-cache
stress/cached-prototype-setter.js.bytecode-cache
stress/dead-get-closure-var.js.bytecode-cache
stress/call-apply-builtin-functions-dont-use-iterators.js.bytecode-cache
stress/eval-func-decl-in-global.js.bytecode-cache
stress/value-mod-ai-rule.js.bytecode-cache
stress/regress-169445.js.bytecode-cache
microbenchmarks/infer-one-time-closure-two-vars.js.bytecode-cache
stress/super-property-access-to-this.js.bytecode-cache
stress/proxy-array-prototype-methods.js.bytecode-cache
stress/typedarray-includes.js.bytecode-cache
stress/typedarray-functions-with-neutered.js.bytecode-cache
stress/arrowfunction-tdz-2.js.bytecode-cache
stress/tagged-template-tdz.js.bytecode-cache
stress/module-namespace-access-change.js.bytecode-cache
stress/infinite-loop-that-uses-captured-variables.js.bytecode-cache
stress/parameter-scoping.js.bytecode-cache
stress/tagged-templates-template-object.js.bytecode-cache
stress/string-prototype-scopes-strict.js.bytecode-cache
stress/generator-and-super.js.bytecode-cache
stress/proxy-class.js.bytecode-cache
stress/proxy-call.js.bytecode-cache
stress/licm-should-handle-if-a-hoist-causes-a-provable-osr-exit.js.bytecode-cache
stress/async-iteration-for-await-of-syntax.js.bytecode-cache
stress/for-in-tests.js.bytecode-cache
stress/obj-spread-order.js.bytecode-cache
stress/verify-bytecode-generator-cached-variables-under-tdz.js.bytecode-cache
stress/dfg-object-proto-getter.js.bytecode-cache
stress/singleton-scope-then-realloc-and-overwrite.js.bytecode-cache
stress/typedarray-constructor.js.bytecode-cache
stress/arrowfunction-lexical-bind-superproperty.js.bytecode-cache
microbenchmarks/infer-closure-const-then-put-to-scope-no-inline.js.bytecode-cache
stress/r238510-bad-loop.js.bytecode-cache
stress/class-syntax-tdz.js.bytecode-cache
microbenchmarks/v8-regexp-search.js.bytecode-cache
stress/phantom-new-array-buffer-forward-varargs2.js.bytecode-cache
stress/exit-then-eval.js.bytecode-cache
stress/array-from-with-iterator.js.bytecode-cache
v8-v6/v8-regexp.js.bytecode-cache
stress/regress-173053.js.bytecode-cache
stress/op-negate-inline-cache.js.bytecode-cache
stress/proxy-define-own-property.js.bytecode-cache
stress/dfg-object-prototype-of.js.bytecode-cache
stress/rest-parameter-having-a-bad-time.js.bytecode-cache
stress/scoped-arguments-test.js.bytecode-cache
stress/array-find-does-not-lookup-twice.js.bytecode-cache
stress/generator-class-methods.js.bytecode-cache
stress/regress-192626.js.bytecode-cache
stress/async-iteration-for-await-of.js.bytecode-cache
stress/lexical-let-loop-semantics.js.bytecode-cache
microbenchmarks/infer-closure-const-then-put-to-scope.js.bytecode-cache
microbenchmarks/inline-get-scoped-var.js.bytecode-cache
stress/global-lexical-var-injection.js.bytecode-cache
stress/eval-func-decl-with-let-const-class.js.bytecode-cache
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.bytecode-cache
stress/arrowfunction-activation-sink-osrexit-default-value-tdz-error.js.bytecode-cache
stress/custom-iterators.js.bytecode-cache
stress/arrowfunction-lexical-bind-arguments-strict.js.bytecode-cache
stress/pow-expects-update-expression-on-lhs.js.bytecode-cache
stress/sloppy-mode-function-hoisting.js.bytecode-cache
stress/variable-under-tdz-eval-tricky.js.bytecode-cache
stress/reflect-set-proxy-set.js.bytecode-cache
stress/function-name-scope.js.bytecode-cache
stress/scoped-arguments-array-length.js.bytecode-cache
stress/template-string-tags-eval.js.bytecode-cache
stress/spread-multi-layers.js.bytecode-cache
stress/const-lexical-binding-shadowing-global-properties-and-eval-injection.js.bytecode-cache
stress/v8-parameter-scoping.js.bytecode-cache
stress/proxy-get-set-correct-receiver.js.bytecode-cache
stress/arrowfunction-activation-sink-osrexit-default-value.js.bytecode-cache
stress/object-assign-changing-properties.js.bytecode-cache
stress/lift-template-literal.js.bytecode-cache
stress/closure-recursive-tail-call.js.bytecode-cache
stress/computed-accessor-parsing.js.bytecode-cache
stress/duplicate-computed-accessors.js.bytecode-cache
stress/object-from-entries.js.bytecode-cache
stress/tagged-templates-syntax.js.bytecode-cache
stress/with.js.bytecode-cache
stress/IIFE-es6-default-parameters.js.bytecode-cache
stress/generator-class-methods-syntax.js.bytecode-cache
stress/arrowfunction-lexical-bind-this-3.js.bytecode-cache
stress/poly-call-exit-this.js.bytecode-cache
stress/async-arrow-functions-lexical-binding-in-class.js.bytecode-cache
stress/arrowfunction-tdz-1.js.bytecode-cache
stress/regress-191579.js.bytecode-cache
stress/get-from-scope-dynamic-onto-proxy.js.bytecode-cache
stress/rest-parameter-and-default-arguments.js.bytecode-cache
stress/arrowfunction-run-10-1.js.bytecode-cache
stress/spread-non-varargs.js.bytecode-cache
stress/class-expression-generates-environment.js.bytecode-cache
stress/typedarray-map.js.bytecode-cache
stress/get-by-id-strict-nested-arguments.js.bytecode-cache
stress/template-literal-syntax.js.bytecode-cache
microbenchmarks/slow-ternaries.js.bytecode-cache
stress/constructor-with-return.js.bytecode-cache
stress/arrowfunction-lexical-bind-this-5.js.bytecode-cache
stress/map-cse-correctness.js.bytecode-cache
v8-v6/v8-splay.js.bytecode-cache
stress/tricky-indirectly-inferred-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js.bytecode-cache
microbenchmarks/scoped-arguments-overridden-length.js.bytecode-cache
stress/super-get-by-id.js.bytecode-cache
stress/for-in-invalidate-context-weird-assignments.js.bytecode-cache
microbenchmarks/infer-one-time-deep-closure.js.bytecode-cache
stress/object-assign-fast-path.js.bytecode-cache
stress/regress-187373.js.bytecode-cache
stress/gc-should-reap-dead-watchpoints.js.bytecode-cache
stress/put-dynamic-var-strict-and-sloppy.js.bytecode-cache
stress/arity-mismatch-rest.js.bytecode-cache
v8-v6/v8-earley-boyer.js.bytecode-cache
stress/generator-throw.js.bytecode-cache
stress/exception-in-to-property-key-should-be-handled-early.js.bytecode-cache
stress/pow-stable-results.js.bytecode-cache
stress/object-values.js.bytecode-cache
stress/JSON-parse-should-cache-array-lengths.js.bytecode-cache
microbenchmarks/eval-compute.js.bytecode-cache
stress/scoped-arguments-check-array.js.bytecode-cache
stress/regress-159954.js.bytecode-cache
stress/arith-mul-with-constants.js.bytecode-cache
stress/spread-forward-varargs-rest-parameter-change-iterator-protocol.js.bytecode-cache
stress/string-prototype-methods-endsWith-startsWith-includes-correctness.js.bytecode-cache
stress/dfg-to-string-on-string-object-does-not-gc.js.bytecode-cache
stress/method-name.js.bytecode-cache
stress/typedarray-fill.js.bytecode-cache
stress/eval-and-super.js.bytecode-cache
stress/big-wasm-memory-grow-no-max.js.bytecode-cache
stress/big-wasm-memory.js.bytecode-cache
stress/eval-func-decl-within-eval-without-reassign-to-let.js.bytecode-cache
stress/arrowfunction-lexical-bind-supercall-3.js.bytecode-cache
stress/generator-yield-star.js.bytecode-cache
microbenchmarks/object-keys-map-values.js.bytecode-cache
stress/async-arrow-function-in-class-heritage.js.bytecode-cache
stress/typedarray-from.js.bytecode-cache
stress/proxy-set-prototype-of.js.bytecode-cache
stress/put-by-id-build-list-order-recurse.js.bytecode-cache
stress/lexical-let-semantics.js.bytecode-cache
stress/array-reverse-proxy.js.bytecode-cache
stress/generator-function-name.js.bytecode-cache
stress/object-values-changing-properties.js.bytecode-cache
stress/async-arrow-functions-lexical-super-binding.js.bytecode-cache
microbenchmarks/Int16Array-bubble-sort-with-byteLength.js.bytecode-cache
stress/reflect-set.js.bytecode-cache
stress/regress-163748.js.bytecode-cache
stress/many-nested-functions-parser-stack-overflow.js.bytecode-cache
stress/regress-158437.js.bytecode-cache
stress/singleton-scope-then-realloc.js.bytecode-cache
stress/super-property-access-tdz.js.bytecode-cache
stress/assignment-in-function-call-bracket-node.js.bytecode-cache
microbenchmarks/try-catch-get-by-val-scoped-arguments.js.bytecode-cache
stress/get-by-id-direct-getter.js.bytecode-cache
microbenchmarks/infer-closure-const-then-reenter-no-inline.js.bytecode-cache
stress/dead-access-to-captured-variable-preceded-by-a-live-store-in-function-with-multiple-basic-blocks.js.bytecode-cache
stress/promise-species-functions.js.bytecode-cache
stress/variable-named-eval-under-tdz.js.bytecode-cache
stress/ensure-proper-strict-mode-parsing.js.bytecode-cache
stress/spread-optimized-properly.js.bytecode-cache
stress/typed-array-get-by-val-profiling.js.bytecode-cache
stress/typedarray-findIndex.js.bytecode-cache
stress/regress-190693.js.bytecode-cache
stress/destructuring-assignment-accepts-iterables.js.bytecode-cache
stress/v8-earley-boyer-strict.js.bytecode-cache
stress/class-syntax-double-constructor.js.bytecode-cache
stress/constructing-builtin-functions-with-getter-prototype-should-only-call-getter-once-per-new-2.js.bytecode-cache
stress/dfg-tail-calls.js.bytecode-cache
stress/regexp-match-proxy.js.bytecode-cache
stress/proxy-with-unbalanced-getter-setter.js.bytecode-cache
stress/generator-fib-ftl.js.bytecode-cache
stress/regexp-prototype-tostring.js.bytecode-cache
stress/async-function-expression-sinking-osrexit.js.bytecode-cache
stress/async-await-reserved-word.js.bytecode-cache
stress/sink-object-past-invalid-check-sneakier.js.bytecode-cache
stress/object-spread.js.bytecode-cache
stress/regexp-matches-array-slow-put.js.bytecode-cache
microbenchmarks/apply-not-apply.js.bytecode-cache
stress/eval-func-decl-within-eval-with-reassign-to-var.js.bytecode-cache
stress/super-property-access-object-literal-to-this-2.js.bytecode-cache
stress/eval-func-decl-in-global-of-eval.js.bytecode-cache
stress/yield-named-variable.js.bytecode-cache
apiTests

[Xan Lopez]

Created attachment 373142 [details]
Class fields, parser + bytecode + test changes

New patch. Try to fix bytecode cache crashes.

[EWS Watchlist]

Comment on attachment 373142 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
editing/style/apply-style-iframe-crash.html
editing/style/iframe-onload-crash-mac.html

[EWS Watchlist]

Created attachment 373153 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Saam Barati]

Comment on attachment 373142 [details]
Class fields, parser + bytecode + test changes

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

> JSTests/stress/class-fields-harmony.js:3
> +// Copyright 2017 the V8 project authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.

Please include their license here instead of found in the LICENSE file

> Source/JavaScriptCore/ChangeLog:65
> +        Private fields are stored in a separate hash inside
> +        VariableEnvironment instead of extending the
> +        VariableEnvironmentEntry Traits. This is done for several reasons:
> +
> +        1) Make private names first class in scopes (different from the
> +        string naming mechanics).
> +        2) Easily determine if a scope is a PrivateNameEnvironment by the
> +        presence of the private name's hash.
> +        3) In a future patch for private methods also allow to easily
> +        determine if a private name access is happening from a field or
> +        from an accessor/method.
> +
> +        Slightly related to the previous, there are also private name
> +        hashes in both SymbolTable and JSSymbolTableObject. The main
> +        reason for this is that while SymbolTables are shared between
> +        multiple evaluations of the same scope, JSSymbolTableObjects are
> +        not, since they are newly created during evaluation. We thus store
> +        the string form of the private names in SymbolTable, and the
> +        actual SymbolImpl in each JSSymbolTableObject. This matches the
> +        expected semantics of private names, where two instances of the
> +        same class with a private field should not collide.

This part of this implementation has been nagging at me. I think I've found a nicer way to accomplish the same thing. Let me know if you think it works.

Right now, let's say a class C has N private fields. We generate N new symbols each time we create that class C. This is so if we create two C's from the text of C, they don't have visibility into each other. However, instead of creating this new scope mechanism, instead, I think we can just index into a normal scope object using a private symbol per class C.

So anytime we create a class C, we give it a symbol specific to it. Then, all instances have a field keyed off that symbol, and that property that exists at that symbol key points to a normal scope (and we throw if that access returns undefined since we're now dealing with a bad instance).

So, if we have code like "this.#foo", we will emit something like:
```
let symbol = C.@symbolForPrivateFields;
let scope = this[symbol];
if (!scope) throw new Error("...");
result = get_from_scope(scope, "foo")
```

I think this achieves the same scoping mechanism and the same property that two unique Cs can't see each other's fields. Also, it means we have no new scoping related byte codes.

Does my analysis that sound right to y'all?

Are there any downsides to doing it this way over what's in this patch?

[Caio Lima]

(In reply to Saam Barati from comment #133)
> Comment on attachment 373142 [details]
> Class fields, parser + bytecode + test changes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373142&action=review
> 
> > JSTests/stress/class-fields-harmony.js:3
> > +// Copyright 2017 the V8 project authors. All rights reserved.
> > +// Use of this source code is governed by a BSD-style license that can be
> > +// found in the LICENSE file.
> 
> Please include their license here instead of found in the LICENSE file
> 
> > Source/JavaScriptCore/ChangeLog:65
> > +        Private fields are stored in a separate hash inside
> > +        VariableEnvironment instead of extending the
> > +        VariableEnvironmentEntry Traits. This is done for several reasons:
> > +
> > +        1) Make private names first class in scopes (different from the
> > +        string naming mechanics).
> > +        2) Easily determine if a scope is a PrivateNameEnvironment by the
> > +        presence of the private name's hash.
> > +        3) In a future patch for private methods also allow to easily
> > +        determine if a private name access is happening from a field or
> > +        from an accessor/method.
> > +
> > +        Slightly related to the previous, there are also private name
> > +        hashes in both SymbolTable and JSSymbolTableObject. The main
> > +        reason for this is that while SymbolTables are shared between
> > +        multiple evaluations of the same scope, JSSymbolTableObjects are
> > +        not, since they are newly created during evaluation. We thus store
> > +        the string form of the private names in SymbolTable, and the
> > +        actual SymbolImpl in each JSSymbolTableObject. This matches the
> > +        expected semantics of private names, where two instances of the
> > +        same class with a private field should not collide.
> 
> This part of this implementation has been nagging at me. I think I've found
> a nicer way to accomplish the same thing. Let me know if you think it works.

I'm wondering if what has been nagging at you is the change we are doing into  SymbolTable and JSSymbolTableObject or if it also includes VariableEnvironment changes. I'm asking that because we are using VariableEnvironment changes to proper emit code to access private methods/accessors on other patches. This is required because we need to know at compile time if a private name is referring to a field, a method or an accessor, since the access to each one is different.
 
> Right now, let's say a class C has N private fields. We generate N new
> symbols each time we create that class C. This is so if we create two C's
> from the text of C, they don't have visibility into each other. However,
> instead of creating this new scope mechanism, instead, I think we can just
> index into a normal scope object using a private symbol per class C.
> 
> So anytime we create a class C, we give it a symbol specific to it. Then,
> all instances have a field keyed off that symbol, and that property that
> exists at that symbol key points to a normal scope (and we throw if that
> access returns undefined since we're now dealing with a bad instance).
> 
> So, if we have code like "this.#foo", we will emit something like:
> ```
> let symbol = C.@symbolForPrivateFields;
> let scope = this[symbol];
> if (!scope) throw new Error("...");
> result = get_from_scope(scope, "foo")
> ```
> 
> I think this achieves the same scoping mechanism and the same property that
> two unique Cs can't see each other's fields. Also, it means we have no new
> scoping related byte codes.

IIUC, your idea is to store private fields into a separate scope instead of into Object, right?

> Does my analysis that sound right to y'all?
> 
> Are there any downsides to doing it this way over what's in this patch?

Personally, I like this idea but I need to think a little bit more about this. One comment that I have is the case where `this` can be a Proxy. When such situation happens, we can't use `this[symbol]` because it won't work when we try to get private fields installed in a Proxy (see test https://github.com/tc39/test262/blob/master/test/language/statements/class/elements/privatefield-on-proxy.js). In such case, we will need a kind of `get_by_val_direct` or something like that to properly access the private field. Beside that, I think this is an approach that is worth it trying.

[Saam Barati]

(In reply to Caio Lima from comment #134)
> (In reply to Saam Barati from comment #133)
> > Comment on attachment 373142 [details]
> > Class fields, parser + bytecode + test changes
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=373142&action=review
> > 
> > > JSTests/stress/class-fields-harmony.js:3
> > > +// Copyright 2017 the V8 project authors. All rights reserved.
> > > +// Use of this source code is governed by a BSD-style license that can be
> > > +// found in the LICENSE file.
> > 
> > Please include their license here instead of found in the LICENSE file
> > 
> > > Source/JavaScriptCore/ChangeLog:65
> > > +        Private fields are stored in a separate hash inside
> > > +        VariableEnvironment instead of extending the
> > > +        VariableEnvironmentEntry Traits. This is done for several reasons:
> > > +
> > > +        1) Make private names first class in scopes (different from the
> > > +        string naming mechanics).
> > > +        2) Easily determine if a scope is a PrivateNameEnvironment by the
> > > +        presence of the private name's hash.
> > > +        3) In a future patch for private methods also allow to easily
> > > +        determine if a private name access is happening from a field or
> > > +        from an accessor/method.
> > > +
> > > +        Slightly related to the previous, there are also private name
> > > +        hashes in both SymbolTable and JSSymbolTableObject. The main
> > > +        reason for this is that while SymbolTables are shared between
> > > +        multiple evaluations of the same scope, JSSymbolTableObjects are
> > > +        not, since they are newly created during evaluation. We thus store
> > > +        the string form of the private names in SymbolTable, and the
> > > +        actual SymbolImpl in each JSSymbolTableObject. This matches the
> > > +        expected semantics of private names, where two instances of the
> > > +        same class with a private field should not collide.
> > 
> > This part of this implementation has been nagging at me. I think I've found
> > a nicer way to accomplish the same thing. Let me know if you think it works.
> 
> I'm wondering if what has been nagging at you is the change we are doing
> into  SymbolTable and JSSymbolTableObject or if it also includes
> VariableEnvironment changes. I'm asking that because we are using
> VariableEnvironment changes to proper emit code to access private
> methods/accessors on other patches. This is required because we need to know
> at compile time if a private name is referring to a field, a method or an
> accessor, since the access to each one is different.
it’s just more the the fact that we didn’t map it onto existing primitives. I think this proposal gives us that.

>  
> > Right now, let's say a class C has N private fields. We generate N new
> > symbols each time we create that class C. This is so if we create two C's
> > from the text of C, they don't have visibility into each other. However,
> > instead of creating this new scope mechanism, instead, I think we can just
> > index into a normal scope object using a private symbol per class C.
> > 
> > So anytime we create a class C, we give it a symbol specific to it. Then,
> > all instances have a field keyed off that symbol, and that property that
> > exists at that symbol key points to a normal scope (and we throw if that
> > access returns undefined since we're now dealing with a bad instance).
> > 
> > So, if we have code like "this.#foo", we will emit something like:
> > ```
> > let symbol = C.@symbolForPrivateFields;
> > let scope = this[symbol];
> > if (!scope) throw new Error("...");
> > result = get_from_scope(scope, "foo")
> > ```
> > 
> > I think this achieves the same scoping mechanism and the same property that
> > two unique Cs can't see each other's fields. Also, it means we have no new
> > scoping related byte codes.
> 
> IIUC, your idea is to store private fields into a separate scope instead of
> into Object, right?
> 
> > Does my analysis that sound right to y'all?
> > 
> > Are there any downsides to doing it this way over what's in this patch?
> 
> Personally, I like this idea but I need to think a little bit more about
> this. One comment that I have is the case where `this` can be a Proxy. When
> such situation happens, we can't use `this[symbol]` because it won't work
> when we try to get private fields installed in a Proxy (see test
> https://github.com/tc39/test262/blob/master/test/language/statements/class/
> elements/privatefield-on-proxy.js). In such case, we will need a kind of
> `get_by_val_direct` or something like that to properly access the private
> field. Beside that, I think this is an approach that is worth it trying.

[Caitlin Potter (:caitp)]

(In reply to Saam Barati from comment #133)
> Comment on attachment 373142 [details]
> Class fields, parser + bytecode + test changes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373142&action=review
> 
> > JSTests/stress/class-fields-harmony.js:3
> > +// Copyright 2017 the V8 project authors. All rights reserved.
> > +// Use of this source code is governed by a BSD-style license that can be
> > +// found in the LICENSE file.
> 
> Please include their license here instead of found in the LICENSE file
> 
> > Source/JavaScriptCore/ChangeLog:65
> > +        Private fields are stored in a separate hash inside
> > +        VariableEnvironment instead of extending the
> > +        VariableEnvironmentEntry Traits. This is done for several reasons:
> > +
> > +        1) Make private names first class in scopes (different from the
> > +        string naming mechanics).
> > +        2) Easily determine if a scope is a PrivateNameEnvironment by the
> > +        presence of the private name's hash.
> > +        3) In a future patch for private methods also allow to easily
> > +        determine if a private name access is happening from a field or
> > +        from an accessor/method.
> > +
> > +        Slightly related to the previous, there are also private name
> > +        hashes in both SymbolTable and JSSymbolTableObject. The main
> > +        reason for this is that while SymbolTables are shared between
> > +        multiple evaluations of the same scope, JSSymbolTableObjects are
> > +        not, since they are newly created during evaluation. We thus store
> > +        the string form of the private names in SymbolTable, and the
> > +        actual SymbolImpl in each JSSymbolTableObject. This matches the
> > +        expected semantics of private names, where two instances of the
> > +        same class with a private field should not collide.
> 
> This part of this implementation has been nagging at me. I think I've found
> a nicer way to accomplish the same thing. Let me know if you think it works.
> 
> Right now, let's say a class C has N private fields. We generate N new
> symbols each time we create that class C. This is so if we create two C's
> from the text of C, they don't have visibility into each other. However,
> instead of creating this new scope mechanism, instead, I think we can just
> index into a normal scope object using a private symbol per class C.
> 
> So anytime we create a class C, we give it a symbol specific to it. Then,
> all instances have a field keyed off that symbol, and that property that
> exists at that symbol key points to a normal scope (and we throw if that
> access returns undefined since we're now dealing with a bad instance).
> 
> So, if we have code like "this.#foo", we will emit something like:
> ```
> let symbol = C.@symbolForPrivateFields;
> let scope = this[symbol];
> if (!scope) throw new Error("...");
> result = get_from_scope(scope, "foo")
> ```
> 
> I think this achieves the same scoping mechanism and the same property that
> two unique Cs can't see each other's fields. Also, it means we have no new
> scoping related byte codes.
> 
> Does my analysis that sound right to y'all?
> 
> Are there any downsides to doing it this way over what's in this patch?

This seems very similar to what is already done, except that you're changing where the private name is loaded (outside of the get/put op rather than inside it) --- I'm fine with doing this, no problem there. It also means allocating a special scope for private names, separate from the class lexical scope --- then scope inheritance gets a bit harder to track for nested scopes.

For get/put operations, we need either new opcodes or new get/put flags (or both if we go with the get_by_val_direct/put_by_val_direct idea) to add the extra private field semantics (throwing at runtime if field isn't defined yet, distinction between "put" and "define", etc)

Is it important that we do this before landing? This will take up some time to implement, and we've been trying to get this landed for many months now. From my POV, the changes are fine, but because it will take some time, I'd prefer to add these as a followup, and call it an optimization.

[Caitlin Potter (:caitp)]

Ping --- asking again, can we initially land a version without that change first?

We are working on a number of these features in parallel, and it would be easiest to make this change afterwords, so that there isn't a need to go back and change the parallel features.

[Caitlin Potter (:caitp)]

Comment on attachment 373142 [details]
Class fields, parser + bytecode + test changes

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

>> Source/JavaScriptCore/ChangeLog:65
>> +        same class with a private field should not collide.
> 
> This part of this implementation has been nagging at me. I think I've found a nicer way to accomplish the same thing. Let me know if you think it works.
> 
> Right now, let's say a class C has N private fields. We generate N new symbols each time we create that class C. This is so if we create two C's from the text of C, they don't have visibility into each other. However, instead of creating this new scope mechanism, instead, I think we can just index into a normal scope object using a private symbol per class C.
> 
> So anytime we create a class C, we give it a symbol specific to it. Then, all instances have a field keyed off that symbol, and that property that exists at that symbol key points to a normal scope (and we throw if that access returns undefined since we're now dealing with a bad instance).
> 
> So, if we have code like "this.#foo", we will emit something like:
> ```
> let symbol = C.@symbolForPrivateFields;
> let scope = this[symbol];
> if (!scope) throw new Error("...");
> result = get_from_scope(scope, "foo")
> ```
> 
> I think this achieves the same scoping mechanism and the same property that two unique Cs can't see each other's fields. Also, it means we have no new scoping related byte codes.
> 
> Does my analysis that sound right to y'all?
> 
> Are there any downsides to doing it this way over what's in this patch?

I'm trying to work out  when we allocate the scope.

In a situation like

```
class Base {
  #x = 1;


  C = class Child extends Base {
    #y = 2;

    xy() {
      return this.#x + this.#y;
    }
  }
}

(new Base.C).xy();
```

What is the order of operations here? Is `#x` in `this[Base.@symbolForPrivateFields]` and `#y` in `this[Child.@symbolForPrivateFields]`? Or the same scope? how much metadata do we need to store to know which scope to look in when using nested PrivateNameEnvironments?

I keep running into confusing cases with this model that are hard to explain/work through, and because of that I don't really like the approach, even if it can take advantage of some stuff that already exists.

[Caitlin Potter (:caitp)]

Created attachment 380353 [details]
Class fields, parser + bytecode + JIT + test changes

This version includes our version of the changes requested by Saam --- see changelog for details

[Caio Lima]

Comment on attachment 380353 [details]
Class fields, parser + bytecode + JIT + test changes

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

LGTM overall. I've left some comments into ChangeLog.

> Source/JavaScriptCore/ChangeLog:15
> +        a private name #instanceFieldInitializer is conditionally added to

I think it is better use `@instanceFieldInitializer`.

> Source/JavaScriptCore/ChangeLog:19
> +        similar to what the v8 engine does.

This is not true anymore. We only emit the call to this function if class contains fields.

> Source/JavaScriptCore/ChangeLog:65
> +        same class with a private field should not collide.

From new implementation, I think this is also not true anymore, since we are storing PrivateSymbols into class scope, as explained below.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1999
> +            // TODO: only do this if there is an eval() within a nested scope --- otherwise it isn't needed.

nit: turn this TODO into a FIXME.

> Source/JavaScriptCore/runtime/ModuleProgramExecutable.h:1
> +

nit: remove this.

[Caitlin Potter (:caitp)]

Created attachment 380552 [details]
Class fields, parser + bytecode + JIT + test changes

[Caitlin Potter (:caitp)]

Comment on attachment 380552 [details]
Class fields, parser + bytecode + JIT + test changes

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

just taking some notes for if I upload a new version tomorrow...

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:-2245
> -    bool hasCapturedVariables = false;

renaming `hasCapturedVariables` to `needsToPopScope` is not needed, can be removed from diff

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3243
> +bool BytecodeGenerator::shouldEmitSetFunctionName(ExpressionNode* valueNode)

There are alternative ways to do this, like having a private `emitSetFunctionNameIfNeeded()` template which takes a lambda that returns a RegisterID*, and having public versions for RegisterID* and const Identifier& respectively --- but it doesn't matter much.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4503
> +    return emitLoadDerivedConstructorFromArrowFunctionLexicalEnvironment();

it's not obvious that this works for eval() --- it might be worth renaming the "ArrowFunctionLexicalEnvironment" to something else --- but not here.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:-786
> -    RegisterID* ret;

all the DotAccessor changes make things a lot more confusing to me :( Any thoughts on this approach?

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1544
> +getByValOp(get_by_val_direct, OpGetByValDirect, _slow_path_get_by_val_direct, macro(a, b) end)

do we need OSR exit points for these? I noticed putByVal didn't add one, but I don't see why the non-direct version would but not the direct version.

> JSTests/ChangeLog:21
> +        * stress/async-await-syntax.js:

TODO: remove JS spamy symbols from change list

[Caitlin Potter (:caitp)]

Created attachment 380560 [details]
hopefully fix llint32_64

[Caitlin Potter (:caitp)]

Created attachment 380757 [details]
Patch for landing, hopefully

This last one implements the changes from https://github.com/tc39/proposal-class-fields/issues/263

[EWS Watchlist]

Comment on attachment 380757 [details]
Patch for landing, hopefully

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

New failing tests:
http/wpt/resource-timing/rt-resources-per-worker.html

[EWS Watchlist]

Created attachment 380826 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Ross Kirsling]

Comment on attachment 380757 [details]
Patch for landing, hopefully

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

I'm not in a position to be the ultimate reviewer on this, but here are some things I noticed!

(BTW: "Patch for landing" is usually what we call a resubmission of a patch that received "r+ with comments" which is now being sent directly to the commit queue. In particular, it's the default description when using `webkit-patch land-safely`.)

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3268
> +    emitSetFunctionNameIfNeededImpl(valueNode, value, [=]() { return emitLoad(newTemporary(), ident); });

Seems like we probably don't want to capture by value here.

> Source/JavaScriptCore/parser/Lexer.cpp:-2413
> -        // Hashbang is only permitted at the start of the source text.

This comment might be worth preserving.

> Source/JavaScriptCore/parser/NodeConstructors.h:256
> +        , m_isPrivate(!!(type & Private))

The !! shouldn't be required (here and below).

> Source/JavaScriptCore/parser/Nodes.h:2253
> +        enum Type { Name, PrivateName, ComputedName };

I think this ought to be an enum class.

> Source/JavaScriptCore/parser/Parser.cpp:2988
> +                failIfTrue(classScope->declareLexicalVariable(ident, true) != DeclarationResult::Valid, "Failed to declare lexical variable for computed class field. FIXME: This should never happen!");

Hmm, if this should never happen, it should probably be an assertion and not a user-facing error.

> Source/JavaScriptCore/parser/Parser.cpp:3119
> +    m_token.m_type = EOFTOK; // Hack to make parsing valid

Can you elaborate? Seems surprising since EOF *would* be an error here, hehe.

> Source/JavaScriptCore/parser/Parser.h:1233
> +            ASSERT(i < m_scopeStack.size());

I know this is mimicking stuff above, but is this assertion even hittable? `i` needed to have been positive in order to enter the loop...

> Source/JavaScriptCore/parser/Parser.h:1269
> +        ASSERT(i < m_scopeStack.size() && m_scopeStack.size());

Similarly, I think the latter conjunct is redundant here.

> Source/JavaScriptCore/parser/VariableEnvironment.h:127
> +    VariableEnvironment()
> +    { }

Style: this can be on one line

> Source/JavaScriptCore/parser/VariableEnvironment.h:132
> +    { }

Style (here and right below): each brace gets its own line (like in NodeConstructors.h)

> Source/JavaScriptCore/parser/VariableEnvironment.h:240
> +        RareData()
> +        { }
> +        RareData(RareData&& other)
> +            : m_privateNames(WTFMove(other.m_privateNames))
> +        { }

Style: ditto to the previous two

> Source/JavaScriptCore/runtime/JSSymbolTableObject.h:65
> -    
> +

You could probably drop this file since it's just whitespace changes.

> JSTests/stress/class-fields-harmony.js:6
> +// Copyright 2017 the V8 project authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.

I think if we import a test file wholesale like this, we at least need to insert the license contents here.

[Caitlin Potter (:caitp)]

Created attachment 381427 [details]
Patch for thoroughly scrutinizing

[Caitlin Potter (:caitp)]

Comment on attachment 380757 [details]
Patch for landing, hopefully

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

I think I've taken care of all of these comments, hopefully the new patch description will be seen as more deserving of a look.

In the meantime, still working on improving JIT and tier-up support for the new operations.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3268
>> +    emitSetFunctionNameIfNeededImpl(valueNode, value, [=]() { return emitLoad(newTemporary(), ident); });
> 
> Seems like we probably don't want to capture by value here.

Done

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4411
> +        RefPtr<RegisterID> scope = generator.emitResolveScope(generator.newTemporary(), var);

op_resolve_scope seems to not be doing our work properly here. We want to dynamically resolve the correct lexical scope, but end up using the global object. I don't think this was always the case, since it causes some previously passing tests to fail. I am having trouble finding exactly what was changed that might have broken this, though. These variables are treated as GlobalProperty variables, and should not be.

I'm considering options to fix this, but any suggestions would be welcome.

>> Source/JavaScriptCore/parser/NodeConstructors.h:256
>> +        , m_isPrivate(!!(type & Private))
> 
> The !! shouldn't be required (here and below).

Opted to just get rid of the field

>> Source/JavaScriptCore/parser/Nodes.h:2253
>> +        enum Type { Name, PrivateName, ComputedName };
> 
> I think this ought to be an enum class.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:2988
>> +                failIfTrue(classScope->declareLexicalVariable(ident, true) != DeclarationResult::Valid, "Failed to declare lexical variable for computed class field. FIXME: This should never happen!");
> 
> Hmm, if this should never happen, it should probably be an assertion and not a user-facing error.

Agreed, done

>> Source/JavaScriptCore/parser/Parser.cpp:3119
>> +    m_token.m_type = EOFTOK; // Hack to make parsing valid
> 
> Can you elaborate? Seems surprising since EOF *would* be an error here, hehe.

So, this function is called by `parseInner()` --- parseInner normally expects the entire SourceCode to be consumed, but in this case, we take the entire class SourceCode but only parse little chunks of it (the Syntax is already guaranteed to be valid, but the entire thing isn't consumed). So because of this, we cheat and trick parseInner into believing the entire SourceCode was consumed, and parsing wasn't aborted at any point.

Anyways, I've added a more detailed explanation/comment of why this is done.

>> Source/JavaScriptCore/parser/Parser.h:1233
>> +            ASSERT(i < m_scopeStack.size());
> 
> I know this is mimicking stuff above, but is this assertion even hittable? `i` needed to have been positive in order to enter the loop...

Removed the assert

>> Source/JavaScriptCore/parser/Parser.h:1269
>> +        ASSERT(i < m_scopeStack.size() && m_scopeStack.size());
> 
> Similarly, I think the latter conjunct is redundant here.

Thanks, removed

>> Source/JavaScriptCore/parser/VariableEnvironment.h:127
>> +    { }
> 
> Style: this can be on one line

Done

>> Source/JavaScriptCore/parser/VariableEnvironment.h:132
>> +    { }
> 
> Style (here and right below): each brace gets its own line (like in NodeConstructors.h)

Done

>> Source/JavaScriptCore/parser/VariableEnvironment.h:240
>> +        { }
> 
> Style: ditto to the previous two

Done

>> Source/JavaScriptCore/runtime/JSSymbolTableObject.h:65
>> +
> 
> You could probably drop this file since it's just whitespace changes.

Done

>> JSTests/stress/class-fields-harmony.js:6
>> +// found in the LICENSE file.
> 
> I think if we import a test file wholesale like this, we at least need to insert the license contents here.

Done

[EWS Watchlist]

Comment on attachment 381427 [details]
Patch for thoroughly scrutinizing

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

New failing tests:
http/tests/misc/repeat-open-cancel.html

[EWS Watchlist]

Created attachment 381457 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Caitlin Potter (:caitp)]

Created attachment 381711 [details]
Patch for Thoroughly Scrutinizing

Updated with all the JSGlobalObject*/CallFrame* instead of ExecState* stuff

[Caio Lima]

Comment on attachment 381711 [details]
Patch for Thoroughly Scrutinizing

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

I left some comments, majority of them are minor changes.

> Source/JavaScriptCore/ChangeLog:96
> +        a Symbol object with a no-descriptor PrivateSymbolImpl.

It would be very informative here a piece of bytecode of class evaluation.

> Source/JavaScriptCore/ChangeLog:101
> +        'get_by_val_direct'.

I think it is worth it mention that we don't fully support JIT of `get_by_val_direct`.

> Source/JavaScriptCore/bytecode/PutByValFlags.h:42
> +    PutByValPrivateFieldPut = PutByValPrivateName,

What's the main reason for having this alias here? Maybe it is unecessary.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:-781
> -

Oops. I think this was not intentional.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1145
> +                    classFieldsInitializer = m_codeBlock->classFieldsInitializer();

I think we have tests to cover this, right? A test calling `super()` from an inner function would verify this path.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:764
> +    auto length = node.isPrivate() ? description.length() : 1;

Is there any case where a computed field node can be a private? Maybe I'm missing something here.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:854
> +        // TODO: only load the PrivateSymbol once for unary ++ / --

nit: TODOs should be a FIXME with a bug to resolve it.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:883
> +        // TODO: only load the PrivateSymbol once for unary ++ / --

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:906
> +    return emitPutProperty(generator, base, value, thisValue);

Can we use use a default value of `emitPutProperty` as `nullptr` here?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4398
> +            generator.emitSetFunctionNameIfNeeded(m_assign, value.get(), *m_ident);

We should have tests covering this.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4461
> +    bool hasInstanceFields = this->hasInstanceFields();

I think it would be better have something like `ClassFieldsInitializer classFieldInitializer = this->hasInstanceFields() ? ClassFieldsInitializer::Needed : ClassFieldsInitializer::NotNeeded` here.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4468
> +        if (hasInstanceFields)

here: `metadata->setClassFieldsInitializer(classFieldInitializer);`

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4474
>  

and here only use `classFieldInitializer`. I personally don't like this cast from boolean to `ClassFieldsInitializer`, since it makes obscure the result this cast.

> Source/JavaScriptCore/jit/JITOperations.cpp:746
> +

Why do we have 2 different functions that are doing almost exactly the same thing? I think we should refactor this to re-use code.

> Source/JavaScriptCore/parser/Parser.cpp:841
> +        failIfTrue(match(PRIVATENAME), "Cannot parse variable declaration");

I think we should have better error message here, like `PrivateNames can't be used to declare variables`.

> Source/JavaScriptCore/parser/Parser.cpp:1298
> +                failIfTrue(match(PRIVATENAME), "Cannot parse this destructuring pattern");

Ditto.

> JSTests/stress/class-fields-function-name.js:17
> +assert(a.baz.name == undefined);

We should add private names as well.

> JSTests/test262/config.yaml:-19
> -    - class-fields-private

I think we need to figure out a way to enable `useClassFields` flag when we don't skip those. Otherwise, we will fail Test262 bot.

[Caitlin Potter (:caitp)]

Comment on attachment 381711 [details]
Patch for Thoroughly Scrutinizing

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

>> Source/JavaScriptCore/bytecode/PutByValFlags.h:42
>> +    PutByValPrivateFieldPut = PutByValPrivateName,
> 
> What's the main reason for having this alias here? Maybe it is unecessary.

It documents the uses of the other flags, so that a reader will know why they exist. It puts intent forward

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1145
>> +                    classFieldsInitializer = m_codeBlock->classFieldsInitializer();
> 
> I think we have tests to cover this, right? A test calling `super()` from an inner function would verify this path.

class-fields-harmony.js exercises this path

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:764
>> +    auto length = node.isPrivate() ? description.length() : 1;
> 
> Is there any case where a computed field node can be a private? Maybe I'm missing something here.

looks vestigial, removed

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:854
>> +        // TODO: only load the PrivateSymbol once for unary ++ / --
> 
> nit: TODOs should be a FIXME with a bug to resolve it.

comment looks vestigial, removed

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:883
>> +        // TODO: only load the PrivateSymbol once for unary ++ / --
> 
> Ditto.

ditto

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:906
>> +    return emitPutProperty(generator, base, value, thisValue);
> 
> Can we use use a default value of `emitPutProperty` as `nullptr` here?

The reason this is written this way is so ReadModifyDotNode case initialize `thisValue` in the get operation, and reuse it in the put operation. I prefer to not use default-null pointers, they get confusing. Technically, the design isn't fully needed, since currently `emitPutProperty` never needs to initialize `thisValue` --- but I prefer it if they work consistently, that makes the code overall easier to understand, even if it is slightly unnecessary for the put case.

This could be rewritten to use pointers and avoid overloads, but again, I don't think it's worth it. What might be a bit nicer is renaming the version with `thisValue` to `emit###PropertyWithThisValue` or something, so that it's easier to see that the overloaded version is being used.I dunno if anybody agrees with that assessment, I am sure someone is uneasy about the way `thisValue` is initialized by `emitGetProperty()`. WDYT?

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4398
>> +            generator.emitSetFunctionNameIfNeeded(m_assign, value.get(), *m_ident);
> 
> We should have tests covering this.

class-fields-harmony.js has some coverage of this

>> Source/JavaScriptCore/jit/JITOperations.cpp:746
>> +
> 
> Why do we have 2 different functions that are doing almost exactly the same thing? I think we should refactor this to re-use code.

Because we are trying to fit into the design of the existing IC code, which makes it tricky to do things like add additional parameters. Of course there could be a 3rd impl function, probably a template function to avoid having ternary statements all over the place --- I don't think it would really be an improvement

>> Source/JavaScriptCore/parser/Parser.cpp:841
>> +        failIfTrue(match(PRIVATENAME), "Cannot parse variable declaration");
> 
> I think we should have better error message here, like `PrivateNames can't be used to declare variables`.

ack

>> Source/JavaScriptCore/parser/Parser.cpp:1298
>> +                failIfTrue(match(PRIVATENAME), "Cannot parse this destructuring pattern");
> 
> Ditto.

I'm not 100% sure this error is ever visible anyways, don't we try to reparse as an object literal if it fails?

>> JSTests/stress/class-fields-function-name.js:17
>> +assert(a.baz.name == undefined);
> 
> We should add private names as well.

ack

>> JSTests/test262/config.yaml:-19
>> -    - class-fields-private
> 
> I think we need to figure out a way to enable `useClassFields` flag when we don't skip those. Otherwise, we will fail Test262 bot.

On our Igalia EWS we should be able to change the test run command to `JSC_useClassFields=1 test262-runner ...` easily enough, afaik. Or supply the flag some other way. Not something we can or should worry about in this bug, though.

[Caio Lima]

Comment on attachment 381711 [details]
Patch for Thoroughly Scrutinizing

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

>>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:906
>>> +    return emitPutProperty(generator, base, value, thisValue);
>> 
>> Can we use use a default value of `emitPutProperty` as `nullptr` here?
> 
> The reason this is written this way is so ReadModifyDotNode case initialize `thisValue` in the get operation, and reuse it in the put operation. I prefer to not use default-null pointers, they get confusing. Technically, the design isn't fully needed, since currently `emitPutProperty` never needs to initialize `thisValue` --- but I prefer it if they work consistently, that makes the code overall easier to understand, even if it is slightly unnecessary for the put case.
> 
> This could be rewritten to use pointers and avoid overloads, but again, I don't think it's worth it. What might be a bit nicer is renaming the version with `thisValue` to `emit###PropertyWithThisValue` or something, so that it's easier to see that the overloaded version is being used.I dunno if anybody agrees with that assessment, I am sure someone is uneasy about the way `thisValue` is initialized by `emitGetProperty()`. WDYT?

I do t have a strong opinion here. I’m biased to follow the same approach we have to emitGetById that will use “with_this” version of “thisValue” is present. I’m ok leaving the code as it is if the definition of “thisValue” is deterministic. Can you confirm that we will never have junk value into “thisValue”?

>>> JSTests/test262/config.yaml:-19
>>> -    - class-fields-private
>> 
>> I think we need to figure out a way to enable `useClassFields` flag when we don't skip those. Otherwise, we will fail Test262 bot.
> 
> On our Igalia EWS we should be able to change the test run command to `JSC_useClassFields=1 test262-runner ...` easily enough, afaik. Or supply the flag some other way. Not something we can or should worry about in this bug, though.

But I’m not sure if we are able to set the flag to Apple Test262 bots like https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20Test262%20%28Tests%29.

[Ross Kirsling]

As of r251588, test262/config.yaml has a mapping from test262 features to JSC feature flags. :)

[Caitlin Potter (:caitp)]

Created attachment 382314 [details]
Patch for Thoroughly Scrutinizing

Added test262 config.yaml changes and addressed a few more nits.

[Caitlin Potter (:caitp)]

Comment on attachment 382314 [details]
Patch for Thoroughly Scrutinizing

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

> Source/ThirdParty/ANGLE/ChangeLog:3
> +        [JSC] Add support for instance class fields

I'm not sure where this got pulled in from, so... guess I'll make another one.

[Caitlin Potter (:caitp)]

Created attachment 382318 [details]
Patch for Thoroughly Scrutinizing

Added test262 config.yaml changes and addressed a few more nits, and hopefully fixed the previous issue o_o.

[Caitlin Potter (:caitp)]

Created attachment 383468 [details]
Patch for Thoroughly Scrutinizing

Periodic rebase --- Changed to use the LinkTimeConstants mechanic from https://bugs.webkit.org/show_bug.cgi?id=153792 which probably generates marginally better bytecode now.

[Caitlin Potter (:caitp)]

Created attachment 384358 [details]
Part 1: Public Instance Fields

[Caitlin Potter (:caitp)]

Created attachment 384359 [details]
Part 2: Private Instance Fields

[Caitlin Potter (:caitp)]

The public field and private field parts of the patch have been split apart (as requested by Yusuke offline).

I'm hoping this will help with reviewing a bit, but I'd still prefer it if the changes can be signed off on and landed in a single patch.

[Caio Lima]

Comment on attachment 384358 [details]
Part 1: Public Instance Fields

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

LGTM overall. I left some comments.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4319
> +        Variable var = generator.variable(*m_ident);

Looking to this code and seeing `m_ident` being used on computed names is counter intuitive IMHO. I think a comment here explaining how we use the `m_ident` would be very valuable.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1958
> +    bbaeq JSCell::m_type[t0], SymbolType, .opToPropertyKeySlow

Is there a reason why we always fallback to slow path on String case?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2098
> +    bbaeq JSCell::m_type[t0], SymbolType, .opToPropertyKeySlow

ditto

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:702
> +    JSValue primitive = toPrimitive(globalObject, PreferString);

I think it is good have a test to cover the case of `PreferString`. We can have a computed property with both `valueOf` and `toString` methods and assert that `toString` is called and `valueOf` is never called.

[Caitlin Potter (:caitp)]

Comment on attachment 384358 [details]
Part 1: Public Instance Fields

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

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1958
>> +    bbaeq JSCell::m_type[t0], SymbolType, .opToPropertyKeySlow
> 
> Is there a reason why we always fallback to slow path on String case?

We don't. CellType is 0, StringType is 1, and SymbolType is 2. The idea is "Branch if the source cell's JSType > SymbolType" (inferring that, if everything is working properly, the source is a String or Symbol and not a misc JSCell).

Admittedly, since this is bbaeq, we're also taking the slow case for Symbols. I had fixed this to use bba in llint64, but forgot to do that here.

[Caitlin Potter (:caitp)]

Comment on attachment 384358 [details]
Part 1: Public Instance Fields

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

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1958
>> +    bbaeq JSCell::m_type[t0], SymbolType, .opToPropertyKeySlow
> 
> Is there a reason why we always fallback to slow path on String case?

We don't. CellType is 0, StringType is 1, and SymbolType is 2. The idea is "Branch if the source cell's JSType > SymbolType" (inferring that, if everything is working properly, the source is a String or Symbol and not a misc JSCell).

Admittedly, since this is bbaeq, we're also taking the slow case for Symbols. I had fixed this to use bba in llint64, but forgot to do that here.

[Caitlin Potter (:caitp)]

Created attachment 384927 [details]
Part 1: Public Instance Fields (rev2)

Addressed some of Caio's comments

[Caitlin Potter (:caitp)]

Created attachment 384930 [details]
Part 2: Private Instance Fields (rev2)

Rebased since last week

[Ross Kirsling]

Comment on attachment 384927 [details]
Part 1: Public Instance Fields (rev2)

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

A couple of nits from me.

> Source/JavaScriptCore/bytecode/ExecutableInfo.h:34
> +enum class ClassFieldsInitializer : uint8_t { NotNeeded, Needed };

Hmm, this name really is kind of surprising for a type that's effectively a boolean...though I guess SuperBinding is like this too.
Adding, say, `Requirement` to the end admittedly might be kind of long, but there'd also be precedent for leaning into the booleanness and doing something like NeedsClassFieldInitializer { No, Yes }.

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:61
> +    const auto& instanceFieldLocations = optionalInstanceFieldLocations ? *optionalInstanceFieldLocations : Vector<JSTextPosition>();

Style nit: I think there's preference for using optional.value() instead of *optional.

[Ross Kirsling]

Comment on attachment 384930 [details]
Part 2: Private Instance Fields (rev2)

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

> Source/JavaScriptCore/ChangeLog:34
> +        put_by_val_direct  <receiver, propably 'this'>, <symbol>, <value>, <PutByValPrivateName>

`propably` may not be a word, but I kind of want it to be. :P

> Source/JavaScriptCore/bytecode/PutByValFlags.h:33
> +enum PutByValFlags : int32_t {

I want to say `enum class PutByValFlags : uint8_t`, but I see PutByIdFlags is like this too...

> Source/JavaScriptCore/parser/Lexer.cpp:943
> +    bool isPrivateIdentifier = m_current == '@' && m_parsingBuiltinFunction;

I think "private" was the wrong word for this one to begin with -- how about isBuiltinName?

> Source/JavaScriptCore/parser/Nodes.h:184
> +        virtual bool isPrivateLocation() const { return false; }

Hmm, is it worth adding this to ExpressionNodes across the board if it's just `isDotAccessorNode() && isPrivateName()`?

(I don't mean that it hurts anything so much as it feels weirdly non-orthogonal; it's not true of *all* DotAccessorNodes and it *cannot* be true of other location types.)

> Source/JavaScriptCore/parser/Parser.cpp:4793
> +bool Parser<LexerType>::addPrivateNameUsedIfNeeded(const Identifier* ident)

I think this means "add in a used state" but it's kind of hard to parse.

I was also surprised that the return value here indicates not whether we needed to do work, but whether we ran into trouble on the way (meaning that the "if needed" is wrapped in an "if possible").

Perhaps `ensurePrivateNameUsed` would cover both points? I don't believe "add" lends any information, but "ensure" indicates what we might fail to do.

> Source/JavaScriptCore/parser/VariableEnvironment.h:91
> +struct PrivateNameEntry {

Augh, it's a shame that this whole thing is basically just OptionSet<PrivateNameTraits>. I suppose it'd be tricky to update one without the other though...

> Source/JavaScriptCore/runtime/JSScope.cpp:344
> +            for (auto end = privateNames.end(), iter = privateNames.begin(); iter != end; ++iter)

Any reason this couldn't be a range-based for loop?

> Source/JavaScriptCore/runtime/SymbolTable.cpp:188
> +            for (; iter != end; ++iter)

Ditto.

[Yusuke Suzuki]

Comment on attachment 384927 [details]
Part 1: Public Instance Fields (rev2)

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

Thanks for splitting. It is easy to review to me. First, I would like to ask you a design question.

> Source/JavaScriptCore/ChangeLog:16
> +        In summary, class fields are initialized by a synthetic JSFunction. In its unlinked state,
> +        the UnlinkedFunctionExecutable for the function includes an ordered list of JSTokenLocations
> +        pointing to the start of each class field in the class. Each of these fields are parsed and
> +        included as DefineFieldNodes, which implement the appropriate DefineField behaviour in the
> +        proposal. This synthetic function is only created, and only loaded, if there are class fields
> +        present.

Why not initializing fields in the constructor? Creating a new JSFunction and calling it is (1) slow, and (2) memory consuming.
Can you clarify the reason in ChangeLog?

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:295
> +    case op_to_property_key:

Can you add DFG/FTL implementation too?

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1612
> +    addSlowCase(jump());

Can you implement the fast path for StringType and SymbolType?

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1966
> +    bba JSCell::m_type[t0], SymbolType, .opToPropertyKeySlow

We also have `CellType` and which can be used for any other use cases.
Can you do,

loadb JSCell::m_type[t0], t2
beq t2, SymbolType, .done
bneq t2, StringType, .slow
.done:
return (t1, t0)
...

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2099
> +    btqnz t0, notCellMask, .opToPropertyKeySlow
> +    bbaeq JSCell::m_type[t0], SymbolType, .opToPropertyKeySlow
> +    return(t0)

Ditto

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:694
> +ALWAYS_INLINE JSValue JSValue::toPropertyKey(JSGlobalObject* globalObject, JSValueTag) const

Let's rename it to `toPropertyKeyValue` instead of using JSValueTag, since it is difficult to know the difference between `Identifier JSValue::toPropertyKey`.

[Caitlin Potter (:caitp)]

Comment on attachment 384927 [details]
Part 1: Public Instance Fields (rev2)

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

Thanks very much, I’ll get to addressing these points in a few hours.

>> Source/JavaScriptCore/ChangeLog:16
>> +        present.
> 
> Why not initializing fields in the constructor? Creating a new JSFunction and calling it is (1) slow, and (2) memory consuming.
> Can you clarify the reason in ChangeLog?

I’ll get to the rest of this later today, but:

It’s non-trivial to do this.

1) in either case, we still need to do the weird “go back and parse the fields again”, but now the fields AST lives in a different parser arena, and fixing that is a bit tricky. Beyond that, it only really makes sense to do this for base classes. For extended classes, this might need to be done in the constructors, or in a nested arrow or eval. Beyond that, the scope of initialization is slightly different from the scope of the constructor, with no access to “arguments”, and no access to variables within the constructor’s body.

Because of the complexity, it wasn’t considered worthwhile to spend time on this initially. It might be something worth doing later.

I believe this is explained in the unified patch ChangeLog, and I still think the unified patch should be the one which is landed eventually, but O’ll add some points about this here shortly.

[Caitlin Potter (:caitp)]

Created attachment 385494 [details]
Part 1: Public Instance Fields (rev3)

[Caitlin Potter (:caitp)]

Created attachment 385496 [details]
Part 2: Private Instance Fields (rev3)

[Caitlin Potter (:caitp)]

Comment on attachment 384927 [details]
Part 1: Public Instance Fields (rev2)

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

>> Source/JavaScriptCore/bytecode/ExecutableInfo.h:34
>> +enum class ClassFieldsInitializer : uint8_t { NotNeeded, Needed };
> 
> Hmm, this name really is kind of surprising for a type that's effectively a boolean...though I guess SuperBinding is like this too.
> Adding, say, `Requirement` to the end admittedly might be kind of long, but there'd also be precedent for leaning into the booleanness and doing something like NeedsClassFieldInitializer { No, Yes }.

I've gone ahead and changed it to `NeedsClassFieldInitializer`

>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:61
>> +    const auto& instanceFieldLocations = optionalInstanceFieldLocations ? *optionalInstanceFieldLocations : Vector<JSTextPosition>();
> 
> Style nit: I think there's preference for using optional.value() instead of *optional.

Done

>> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:295
>> +    case op_to_property_key:
> 
> Can you add DFG/FTL implementation too?

This is in progress (https://github.com/caitp/webkit/commit/a761d5559156023adb1680aaef48e5c6fba35a36), I'll get some help with this next week --- but I don't think this should be a blocker for landing, ditto for the other new opcode. It's much easier to add DFG/FTL support separately, and not having them in the initial commit shouldn't slow down any existing code AFAIK.

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:1612
>> +    addSlowCase(jump());
> 
> Can you implement the fast path for StringType and SymbolType?

Done

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1966
>> +    bba JSCell::m_type[t0], SymbolType, .opToPropertyKeySlow
> 
> We also have `CellType` and which can be used for any other use cases.
> Can you do,
> 
> loadb JSCell::m_type[t0], t2
> beq t2, SymbolType, .done
> bneq t2, StringType, .slow
> .done:
> return (t1, t0)
> ...

Done

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2099
>> +    return(t0)
> 
> Ditto

Done ditto

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:694
>> +ALWAYS_INLINE JSValue JSValue::toPropertyKey(JSGlobalObject* globalObject, JSValueTag) const
> 
> Let's rename it to `toPropertyKeyValue` instead of using JSValueTag, since it is difficult to know the difference between `Identifier JSValue::toPropertyKey`.

Done

[Caitlin Potter (:caitp)]

Comment on attachment 384930 [details]
Part 2: Private Instance Fields (rev2)

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

>> Source/JavaScriptCore/ChangeLog:34
>> +        put_by_val_direct  <receiver, propably 'this'>, <symbol>, <value>, <PutByValPrivateName>
> 
> `propably` may not be a word, but I kind of want it to be. :P

I've corrected the typo.

>> Source/JavaScriptCore/bytecode/PutByValFlags.h:33
>> +enum PutByValFlags : int32_t {
> 
> I want to say `enum class PutByValFlags : uint8_t`, but I see PutByIdFlags is like this too...

I agree, but it is bitfield-ish and that's always annoying with enum classes, it's probably simpler to stick with the approach used in PutById because of that. At least, that was my rationale

>> Source/JavaScriptCore/parser/Lexer.cpp:943
>> +    bool isPrivateIdentifier = m_current == '@' && m_parsingBuiltinFunction;
> 
> I think "private" was the wrong word for this one to begin with -- how about isBuiltinName?

Sure, done

>> Source/JavaScriptCore/parser/Nodes.h:184
>> +        virtual bool isPrivateLocation() const { return false; }
> 
> Hmm, is it worth adding this to ExpressionNodes across the board if it's just `isDotAccessorNode() && isPrivateName()`?
> 
> (I don't mean that it hurts anything so much as it feels weirdly non-orthogonal; it's not true of *all* DotAccessorNodes and it *cannot* be true of other location types.)

I don't mind removing it, but it gets a little bit wordier. The spec proposal has a similar abstraction (IsPrivateReference) that this maps directly to, so having it exist in the same way it.

If you'd prefer to change it in favor of the wordier version, I have no problem with that.

>> Source/JavaScriptCore/parser/Parser.cpp:4793
>> +bool Parser<LexerType>::addPrivateNameUsedIfNeeded(const Identifier* ident)
> 
> I think this means "add in a used state" but it's kind of hard to parse.
> 
> I was also surprised that the return value here indicates not whether we needed to do work, but whether we ran into trouble on the way (meaning that the "if needed" is wrapped in an "if possible").
> 
> Perhaps `ensurePrivateNameUsed` would cover both points? I don't believe "add" lends any information, but "ensure" indicates what we might fail to do.

Sounds good to me, done

>> Source/JavaScriptCore/runtime/JSScope.cpp:344
>> +            for (auto end = privateNames.end(), iter = privateNames.begin(); iter != end; ++iter)
> 
> Any reason this couldn't be a range-based for loop?

Done

[Caio Lima]

(In reply to Caitlin Potter (:caitp) from comment #175)
> Comment on attachment 384927 [details]
> Part 1: Public Instance Fields (rev2)
> 
> >> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:295
> >> +    case op_to_property_key:
> > 
> > Can you add DFG/FTL implementation too?
> 
> This is in progress
> (https://github.com/caitp/webkit/commit/
> a761d5559156023adb1680aaef48e5c6fba35a36), I'll get some help with this next
> week --- but I don't think this should be a blocker for landing, ditto for
> the other new opcode. It's much easier to add DFG/FTL support separately,
> and not having them in the initial commit shouldn't slow down any existing
> code AFAIK.

I agree with Caitlin here. I think it would be better handling DFG/FTL of `to_property_key` in a separate patch. IMHO, including DFG/FTL will make it harder to be reviewed and it is very likely that it will make this patch wait longer.

BTW, there is a new version of the patch since it was reviewed last week. Could anybody take a look on it? I'm personally interested in get some feedback from private fields implementation, since it is the same foundation we are using to implement private methods and accessors.

[Caio Lima]

Comment on attachment 385494 [details]
Part 1: Public Instance Fields (rev3)

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:740
> +    RefPtr<RegisterID> propertyExpr;

I think it would be very valuable explain here why a computed field has a `node.name()`. It is counter intuitive.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:750
> +    generator.emitProfileType(propertyName, var, position(), JSTextPosition(-1, position().offset + 1, -1));

I don't see we using this on DFG so far. Is this profiling still necessary?

> Source/JavaScriptCore/parser/Parser.cpp:4792
> +            semanticFailIfFalse(currentScope()->isFunction() || isFunctionEvalContextType || isClassFieldInitializer, "new.target is only valid inside functions");

Do we have test case covering this?

> Source/JavaScriptCore/parser/Parser.cpp:4795
> +                semanticFailIfFalse(!closestOrdinaryFunctionScope->isGlobalCodeScope() || isFunctionEvalContextType || isClassFieldInitializer, "new.target is not valid inside arrow functions in global code");

Ditto.

[Yusuke Suzuki]

Comment on attachment 385494 [details]
Part 1: Public Instance Fields (rev3)

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

Second round of review. Looks nice overall! But putting r- since we need that sizeof() of critical classes must be the same.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:420
> +    unsigned m_needsClassFieldInitializer : 1;

Can you ensure this does not increase the sizeof(UnlinkedCodeBlock)? This is strong requirement.

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:222
> +    Optional<Vector<JSTextPosition>> instanceFieldLocations() const
> +    {
> +        if (m_rareData)
> +            return m_rareData->m_instanceFieldLocations;
> +        return WTF::nullopt;
> +    }

It is copying Vector<>. Can you just return a pointer to `Vector<JSTextPosition>` instead?

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:259
> +    unsigned m_needsClassFieldInitializer : 1;

I think this makes sizeof(UnlinkedFunctionExecutable) larger, and it immediately consumes super very large size of memory in some website including Gmail. Please put this flag without increasing sizeof(UnlinkedFunctionExecutable).
This is strong requirement. I think you can put this after m_isGeneratedFromCache.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3459
> +    dst = tempDestination(dst);

Should not accept nullptr here. Caller of this function should ensure it. Just making this as

OpToPropertyKey::emit(this, dst, src);
return dst;

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:962
> +            func = generator.emitLoadDerivedConstructor();

Let's avoid generator.emitLoadDerivedConstructor if instance-fields do not exist.

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:295
> +    case op_to_property_key:

File FIXME for DFG handling for this. We need DFG and FTL implementation when enabling class-fields.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:364
> +    int dst = bytecode.m_dst.offset();
> +    int src = bytecode.m_src.offset();

Use VirtualRegister.

> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:350
> +    int dst = bytecode.m_dst.offset();
> +    int src = bytecode.m_src.offset();

Ditto.

> Source/JavaScriptCore/parser/Nodes.h:2239
> +        void emitBytecode(BytecodeGenerator&, RegisterID* destination = 0) override;

Use `= nullptr` for new code.

> Source/JavaScriptCore/parser/Parser.cpp:3049
> +        const Identifier* ident = 0;

Use `nullptr` for new code.

> Source/JavaScriptCore/runtime/CodeCache.h:293
>      bool isStrictMode = rootNode->features() & StrictModeFeature;

I think using if constexpr is easier to read than using needsClassFieldInitializer(executable).

NeedsClassFieldInitializer needsClassFieldInitializer = NeedsClassFieldInitializer::No;
if constexpr (std::is_same_v<ExecutableType, DirectEvalExecutable>)
    needsClassFieldInitializer = executable->needsClassFieldInitializer();

> Source/JavaScriptCore/runtime/DirectEvalExecutable.cpp:73
>      : EvalExecutable(globalObject, source, inStrictContext, derivedContextType, isArrowFunctionContext, evalContextType)

Let's propagate needsClassFieldInitializer to EvalExecutable and initialize it correctly in initializer-list, instead of initializing it inside the constructor.

[Caitlin Potter (:caitp)]

Created attachment 387356 [details]
Part 1: Public Instance Fields (rev4)

[Caitlin Potter (:caitp)]

Comment on attachment 385494 [details]
Part 1: Public Instance Fields (rev3)

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

I believe I've addressed each of these in the latest patch.

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:420
>> +    unsigned m_needsClassFieldInitializer : 1;
> 
> Can you ensure this does not increase the sizeof(UnlinkedCodeBlock)? This is strong requirement.

done

>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:222
>> +    }
> 
> It is copying Vector<>. Can you just return a pointer to `Vector<JSTextPosition>` instead?

It is, but it's also pretty rare. We weren't sure what would be preferred, since Optional is more elegant and there's some precedent for using it :) I guess we'll switch back to using the pointer. Done

>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:259
>> +    unsigned m_needsClassFieldInitializer : 1;
> 
> I think this makes sizeof(UnlinkedFunctionExecutable) larger, and it immediately consumes super very large size of memory in some website including Gmail. Please put this flag without increasing sizeof(UnlinkedFunctionExecutable).
> This is strong requirement. I think you can put this after m_isGeneratedFromCache.

Yep, it looks like that very narrowly fits. Done.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3459
>> +    dst = tempDestination(dst);
> 
> Should not accept nullptr here. Caller of this function should ensure it. Just making this as
> 
> OpToPropertyKey::emit(this, dst, src);
> return dst;

Done

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:740
>> +    RefPtr<RegisterID> propertyExpr;
> 
> I think it would be very valuable explain here why a computed field has a `node.name()`. It is counter intuitive.

I've added a comment, and I believe this is also mentioned in the Changelog

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:750
>> +    generator.emitProfileType(propertyName, var, position(), JSTextPosition(-1, position().offset + 1, -1));
> 
> I don't see we using this on DFG so far. Is this profiling still necessary?

Removed it and the DFG/FTL tests [not included in this patch] still pass, done

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:962
>> +            func = generator.emitLoadDerivedConstructor();
> 
> Let's avoid generator.emitLoadDerivedConstructor if instance-fields do not exist.

Done

>> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:295
>> +    case op_to_property_key:
> 
> File FIXME for DFG handling for this. We need DFG and FTL implementation when enabling class-fields.

Done

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:364
>> +    int src = bytecode.m_src.offset();
> 
> Use VirtualRegister.

Done

>> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:350
>> +    int src = bytecode.m_src.offset();
> 
> Ditto.

Done

>> Source/JavaScriptCore/parser/Nodes.h:2239
>> +        void emitBytecode(BytecodeGenerator&, RegisterID* destination = 0) override;
> 
> Use `= nullptr` for new code.

done

>> Source/JavaScriptCore/parser/Parser.cpp:3049
>> +        const Identifier* ident = 0;
> 
> Use `nullptr` for new code.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:4792
>> +            semanticFailIfFalse(currentScope()->isFunction() || isFunctionEvalContextType || isClassFieldInitializer, "new.target is only valid inside functions");
> 
> Do we have test case covering this?

It's covered in JSTests/stress/class-fields-harmony.js (line 403)

>> Source/JavaScriptCore/parser/Parser.cpp:4795
>> +                semanticFailIfFalse(!closestOrdinaryFunctionScope->isGlobalCodeScope() || isFunctionEvalContextType || isClassFieldInitializer, "new.target is not valid inside arrow functions in global code");
> 
> Ditto.

This is also included in the class-fields-harmony.js test (line 373)

>> Source/JavaScriptCore/runtime/CodeCache.h:293
>>      bool isStrictMode = rootNode->features() & StrictModeFeature;
> 
> I think using if constexpr is easier to read than using needsClassFieldInitializer(executable).
> 
> NeedsClassFieldInitializer needsClassFieldInitializer = NeedsClassFieldInitializer::No;
> if constexpr (std::is_same_v<ExecutableType, DirectEvalExecutable>)
>     needsClassFieldInitializer = executable->needsClassFieldInitializer();

Done

>> Source/JavaScriptCore/runtime/DirectEvalExecutable.cpp:73
>>      : EvalExecutable(globalObject, source, inStrictContext, derivedContextType, isArrowFunctionContext, evalContextType)
> 
> Let's propagate needsClassFieldInitializer to EvalExecutable and initialize it correctly in initializer-list, instead of initializing it inside the constructor.

Done

[Caitlin Potter (:caitp)]

Created attachment 387361 [details]
Part 1: Public Instance Fields (rev4)

[Caitlin Potter (:caitp)]

Created attachment 387363 [details]
Part 2: Private Instance Fields (rev4)

[Yusuke Suzuki]

Comment on attachment 387361 [details]
Part 1: Public Instance Fields (rev4)

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

r=me with comments. Nice.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:86
> +    if (info.needsClassFieldInitializer() == NeedsClassFieldInitializer::Yes) {
> +        createRareDataIfNecessary();
> +        m_rareData->m_needsClassFieldInitializer = static_cast<unsigned>(NeedsClassFieldInitializer::Yes);
> +    }

Nice

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:217
> +    Vector<JSTextPosition>* instanceFieldLocations() const

Yeah, elegant code is nice. But I love performance benefit, so if I need to take one of either, I always take performance benefit :P

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:228
> +        ensureRareData().m_instanceFieldLocations = instanceFieldLocations;

Let's do `WTFMove(instanceFieldLocations)` to avoid copying.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1134
> +                if (constructorKind() == ConstructorKind::Extends || isDerivedConstructorContext()) {
>                      newDerivedContextType = DerivedContextType::DerivedConstructorContext;
> +                    needsClassFieldInitializer = m_codeBlock->needsClassFieldInitializer();
> +                }

This code means that, FunctionMetadataNode::needsClassFieldInitializer could be wrong (Even if it is saying NO, it depends on the upper CodeBlock's value).
So, I don't think FunctionMetadataNode::needsClassFieldInitializer's name is correct.
Please rename FunctionMetadataNode::needsClassFieldInitializer to appropriate one, and ensure it is appropriately set. 
For example,

FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer
And
UnlinkedCodeBlock::needsClassFieldInitializer

> Source/JavaScriptCore/parser/Nodes.cpp:333
> +bool PropertyListNode::shouldCreateLexicalScopeForClass(PropertyListNode* list)

Add FIXME+url to avoid this behavior. While collecting property lists, I think we can calculate it.

> Source/JavaScriptCore/parser/Nodes.cpp:345
> +bool PropertyListNode::hasInstanceFields() const

Ditto.

> Source/JavaScriptCore/parser/Parser.cpp:2964
> +        } else if (Options::useClassFields() && !match(OPENPAREN) && tag == ClassElementTag::Instance && parseMode == SourceParseMode::MethodMode && !isGetter && !isSetter) {

`!isGetter && !isSetter` is not necessary since we have `if (isGetter || isSetter) {`

Put ASSERT(!isGetter && !isSetter) inside brace.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #184)
> Comment on attachment 387361 [details]
> Part 1: Public Instance Fields (rev4)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387361&action=review
> 
> r=me with comments. Nice.
> 
> > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:86
> > +    if (info.needsClassFieldInitializer() == NeedsClassFieldInitializer::Yes) {
> > +        createRareDataIfNecessary();
> > +        m_rareData->m_needsClassFieldInitializer = static_cast<unsigned>(NeedsClassFieldInitializer::Yes);
> > +    }
> 
> Nice
> 
> > Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:217
> > +    Vector<JSTextPosition>* instanceFieldLocations() const
> 
> Yeah, elegant code is nice. But I love performance benefit, so if I need to
> take one of either, I always take performance benefit :P
> 
> > Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:228
> > +        ensureRareData().m_instanceFieldLocations = instanceFieldLocations;
> 
> Let's do `WTFMove(instanceFieldLocations)` to avoid copying.
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1134
> > +                if (constructorKind() == ConstructorKind::Extends || isDerivedConstructorContext()) {
> >                      newDerivedContextType = DerivedContextType::DerivedConstructorContext;
> > +                    needsClassFieldInitializer = m_codeBlock->needsClassFieldInitializer();
> > +                }
> 
> This code means that, FunctionMetadataNode::needsClassFieldInitializer could
> be wrong (Even if it is saying NO, it depends on the upper CodeBlock's
> value).
> So, I don't think FunctionMetadataNode::needsClassFieldInitializer's name is
> correct.
> Please rename FunctionMetadataNode::needsClassFieldInitializer to
> appropriate one, and ensure it is appropriately set. 
> For example,
> 
> FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer
> And
> UnlinkedCodeBlock::needsClassFieldInitializer
> 
> > Source/JavaScriptCore/parser/Nodes.cpp:333
> > +bool PropertyListNode::shouldCreateLexicalScopeForClass(PropertyListNode* list)
> 
> Add FIXME+url to avoid this behavior. While collecting property lists, I
> think we can calculate it.
> 
> > Source/JavaScriptCore/parser/Nodes.cpp:345
> > +bool PropertyListNode::hasInstanceFields() const
> 
> Ditto.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:2964
> > +        } else if (Options::useClassFields() && !match(OPENPAREN) && tag == ClassElementTag::Instance && parseMode == SourceParseMode::MethodMode && !isGetter && !isSetter) {
> 
> `!isGetter && !isSetter` is not necessary since we have `if (isGetter ||
> isSetter) {`
> 
> Put ASSERT(!isGetter && !isSetter) inside brace.

And before landing, please check jsc-armv7's new failure.

[Caitlin Potter (:caitp)]

Comment on attachment 387361 [details]
Part 1: Public Instance Fields (rev4)

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

> And before landing, please check jsc-armv7's new failure.

I'm a bit stumped on the class-fields-harmony.js failure in the arm7 run.

>>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:86
>>> +    }
>> 
>> Nice
> 
> And before landing, please check jsc-armv7's new failure.

Just for the record, I've been looking into this for the past few hours, but haven't been able to reproduce the failure using https://github.com/caitp/webkit/commit/afef4270fa6e2d140b75a074742e95f383424727.

Unfortunately, checking on the latest master branch has been difficult. Our buildbox cross compiler is failing to build with or without my patch applied.

In the case of the class-fields-harmony.js.default failure on the bot, the failure in question appears to not use any of the newly introduced opcodes, but it's difficult to verify this at the moment.

>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:228
>> +        ensureRareData().m_instanceFieldLocations = instanceFieldLocations;
> 
> Let's do `WTFMove(instanceFieldLocations)` to avoid copying.

Done (when next version is uploaded)

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1134
>> +                }
> 
> This code means that, FunctionMetadataNode::needsClassFieldInitializer could be wrong (Even if it is saying NO, it depends on the upper CodeBlock's value).
> So, I don't think FunctionMetadataNode::needsClassFieldInitializer's name is correct.
> Please rename FunctionMetadataNode::needsClassFieldInitializer to appropriate one, and ensure it is appropriately set. 
> For example,
> 
> FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer
> And
> UnlinkedCodeBlock::needsClassFieldInitializer

What this is really saying is, "this code is either a constructor which will need to initialize class fields, or is an arrow function nested within a derived class constructor which will need to initialize instance fields after `super()`"

It is potentially a false-positive, if there is no super-call within the arrow function, but it's not possible to be a false negative, AFAIK.

There might be a better name, like "needsClassFieldInitializeDuringConstructOrSuperCall" or something. But it's a bit wordy (and not that informative/helpful, IMHO). Do you think a comment would help explain this better?

>> Source/JavaScriptCore/parser/Parser.cpp:2964
>> +        } else if (Options::useClassFields() && !match(OPENPAREN) && tag == ClassElementTag::Instance && parseMode == SourceParseMode::MethodMode && !isGetter && !isSetter) {
> 
> `!isGetter && !isSetter` is not necessary since we have `if (isGetter || isSetter) {`
> 
> Put ASSERT(!isGetter && !isSetter) inside brace.

Done

[Caitlin Potter (:caitp)]

(In reply to Caitlin Potter (:caitp) from comment #186)
> Comment on attachment 387361 [details]
> Part 1: Public Instance Fields (rev4)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387361&action=review
> 
> > And before landing, please check jsc-armv7's new failure.
> 
> I'm a bit stumped on the class-fields-harmony.js failure in the arm7 run.
> 
> >>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:86
> >>> +    }
> >> 
> >> Nice
> > 
> > And before landing, please check jsc-armv7's new failure.
> 
> Just for the record, I've been looking into this for the past few hours, but
> haven't been able to reproduce the failure using
> https://github.com/caitp/webkit/commit/
> afef4270fa6e2d140b75a074742e95f383424727.
> 
> Unfortunately, checking on the latest master branch has been difficult. Our
> buildbox cross compiler is failing to build with or without my patch applied.
> 
> In the case of the class-fields-harmony.js.default failure on the bot, the
> failure in question appears to not use any of the newly introduced opcodes,
> but it's difficult to verify this at the moment.

This might not have come across as clearly as intended.

That commit builds using the ARM cross compiler toolchain, and runs the test without fail (does not seem to be flaky). The patch on the bug is that commit rebased over master and with the changelog added on.

However, when the diff is applied over the latest master branch, the build fails, so I can't comment on whether the test is broken there or not.

[Yusuke Suzuki]

Comment on attachment 387361 [details]
Part 1: Public Instance Fields (rev4)

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

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1134
>>> +                }
>> 
>> This code means that, FunctionMetadataNode::needsClassFieldInitializer could be wrong (Even if it is saying NO, it depends on the upper CodeBlock's value).
>> So, I don't think FunctionMetadataNode::needsClassFieldInitializer's name is correct.
>> Please rename FunctionMetadataNode::needsClassFieldInitializer to appropriate one, and ensure it is appropriately set. 
>> For example,
>> 
>> FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer
>> And
>> UnlinkedCodeBlock::needsClassFieldInitializer
> 
> What this is really saying is, "this code is either a constructor which will need to initialize class fields, or is an arrow function nested within a derived class constructor which will need to initialize instance fields after `super()`"
> 
> It is potentially a false-positive, if there is no super-call within the arrow function, but it's not possible to be a false negative, AFAIK.
> 
> There might be a better name, like "needsClassFieldInitializeDuringConstructOrSuperCall" or something. But it's a bit wordy (and not that informative/helpful, IMHO). Do you think a comment would help explain this better?

I think we should use `needsClassFieldInitializeDuringConstructOrSuperCall` for FunctionMetadataNode, because `needsClassFieldInitializer` name is incorrectt for FunctionMetadataNode :)

[Yusuke Suzuki]

Comment on attachment 387361 [details]
Part 1: Public Instance Fields (rev4)

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

>>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1134
>>>> +                }
>>> 
>>> This code means that, FunctionMetadataNode::needsClassFieldInitializer could be wrong (Even if it is saying NO, it depends on the upper CodeBlock's value).
>>> So, I don't think FunctionMetadataNode::needsClassFieldInitializer's name is correct.
>>> Please rename FunctionMetadataNode::needsClassFieldInitializer to appropriate one, and ensure it is appropriately set. 
>>> For example,
>>> 
>>> FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer
>>> And
>>> UnlinkedCodeBlock::needsClassFieldInitializer
>> 
>> What this is really saying is, "this code is either a constructor which will need to initialize class fields, or is an arrow function nested within a derived class constructor which will need to initialize instance fields after `super()`"
>> 
>> It is potentially a false-positive, if there is no super-call within the arrow function, but it's not possible to be a false negative, AFAIK.
>> 
>> There might be a better name, like "needsClassFieldInitializeDuringConstructOrSuperCall" or something. But it's a bit wordy (and not that informative/helpful, IMHO). Do you think a comment would help explain this better?
> 
> I think we should use `needsClassFieldInitializeDuringConstructOrSuperCall` for FunctionMetadataNode, because `needsClassFieldInitializer` name is incorrectt for FunctionMetadataNode :)

Ah, no, needsClassFieldInitializeDuringConstructOrSuperCall FunctionMetadataNode is also wrong. If it is false, don't we need class-field-initializattion? No. So this name is incorrect.
I think FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer is better name. And keep UnlinkedCodeBlock::needsClassFieldInitializer's name as is.

[Caitlin Potter (:caitp)]

Created attachment 387818 [details]
Part 1: Public Instance Fields (patch for landing)

[Caitlin Potter (:caitp)]

Comment on attachment 387361 [details]
Part 1: Public Instance Fields (rev4)

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

Last patch is ready to go, I'll submit when EWS is finished.

>>>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1134
>>>>> +                }
>>>> 
>>>> This code means that, FunctionMetadataNode::needsClassFieldInitializer could be wrong (Even if it is saying NO, it depends on the upper CodeBlock's value).
>>>> So, I don't think FunctionMetadataNode::needsClassFieldInitializer's name is correct.
>>>> Please rename FunctionMetadataNode::needsClassFieldInitializer to appropriate one, and ensure it is appropriately set. 
>>>> For example,
>>>> 
>>>> FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer
>>>> And
>>>> UnlinkedCodeBlock::needsClassFieldInitializer
>>> 
>>> What this is really saying is, "this code is either a constructor which will need to initialize class fields, or is an arrow function nested within a derived class constructor which will need to initialize instance fields after `super()`"
>>> 
>>> It is potentially a false-positive, if there is no super-call within the arrow function, but it's not possible to be a false negative, AFAIK.
>>> 
>>> There might be a better name, like "needsClassFieldInitializeDuringConstructOrSuperCall" or something. But it's a bit wordy (and not that informative/helpful, IMHO). Do you think a comment would help explain this better?
>> 
>> I think we should use `needsClassFieldInitializeDuringConstructOrSuperCall` for FunctionMetadataNode, because `needsClassFieldInitializer` name is incorrectt for FunctionMetadataNode :)
> 
> Ah, no, needsClassFieldInitializeDuringConstructOrSuperCall FunctionMetadataNode is also wrong. If it is false, don't we need class-field-initializattion? No. So this name is incorrect.
> I think FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer is better name. And keep UnlinkedCodeBlock::needsClassFieldInitializer's name as is.

OK, per the discussion on IRC, I have come to agree and have made this change.

I've opted to go with a boolean rather than an enum, in order to make conversion between this and the enum slightly simpler.

[Caitlin Potter (:caitp)]

Created attachment 387827 [details]
Part 1: Public Instance Fields (patch for landing)

[Ross Kirsling]

(Weird. Your cq+ didn't actually cause the patch to be sent to the commit queue somehow. I thought it might be because r? was still set, but clearing that didn't help...)

[Ross Kirsling]

(In reply to Ross Kirsling from comment #193)
> (Weird. Your cq+ didn't actually cause the patch to be sent to the commit
> queue somehow. I thought it might be because r? was still set, but clearing
> that didn't help...)

...or wait, yes it did! Guess it just took a moment.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 387827 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 387827 [details]
Part 1: Public Instance Fields (patch for landing)

Clearing flags on attachment: 387827

Committed r254653: <https://trac.webkit.org/changeset/254653>

[Caitlin Potter (:caitp)]

Created attachment 387927 [details]
Part 2: Private Instance Fields (rev5)

[Saam Barati]

(In reply to Caitlin Potter (:caitp) from comment #197)
> Created attachment 387927 [details]
> Part 2: Private Instance Fields (rev5)

I think it is better for this to have its own bugzilla. I think we tend to have 1:1 bugzilla and commit for almost all changes, and especially for big changes.

[Caitlin Potter (:caitp)]

Moved review of part2 (private class fields) to https://bugs.webkit.org/show_bug.cgi?id=206431

[Xan Lopez]

This landed a while ago, I think we can close the bug?