Bug 194095 - [JSC] Add support for static public class fields
Summary: [JSC] Add support for static public class fields
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 174212 194098
Blocks: 214297
  Show dependency treegraph
 
Reported: 2019-01-31 10:24 PST by Xan Lopez
Modified: 2020-10-27 05:20 PDT (History)
11 users (show)

See Also:


Attachments
Class fields static (private and public) (17.91 KB, patch)
2019-06-28 06:47 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Add support for static public fields (35.33 KB, patch)
2020-05-22 09:49 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Add support for static public fields, v3. (69.50 KB, patch)
2020-06-05 02:57 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Add support for static public fields, v4 (72.72 KB, patch)
2020-07-12 02:46 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Add support for static public fields, v5 (72.69 KB, patch)
2020-07-21 06:52 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Add support for static public fields, v6 (72.41 KB, patch)
2020-07-28 04:34 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Add support for static public fields, v7 (69.86 KB, patch)
2020-10-27 05:20 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xan Lopez 2019-01-31 10:24:34 PST
Proposal (currently at stage 3): https://tc39.github.io/proposal-static-class-features/

This would land after bug #174212 lands.
Comment 1 Xan Lopez 2019-06-28 06:47:54 PDT
Created attachment 373113 [details]
Class fields static (private and public)

WIP in patch.

This is enough to pass all current test262 tests, so it should be good for an early review if someone feels like it. It needs the code in bug #174212.
Comment 2 Xan Lopez 2020-05-22 09:49:07 PDT
Created attachment 400054 [details]
Add support for static public fields
Comment 3 Caio Lima 2020-05-25 08:56:32 PDT
Comment on attachment 400054 [details]
Add support for static public fields

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

I think it is necesssary to have some stress test cases included in this patch, since most of our bots don't run Test262. It is also important to have a test case asserting that the case listed on https://bugs.webkit.org/show_bug.cgi?id=209675 can pass.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:796
> +    generator.emitJumpIfFalse(generator.emitBinaryOp<OpEq>(generator.newTemporary(), generator.emitLoad(nullptr, JSValue(generator.addStringConstant(generator.propertyNames().prototype))), propertyName, OperandTypes(ResultType::stringType(), ResultType::stringType())), validPropertyNameLabel.get());

I personally think this is very hard to read. Also, I'm not 100% sure if this quite safe if we don't use `RefPtr<>`. Do you mind making it something like:

```
RefPtr<RegisterID> prototypeString = generator.emitLoad(nullptr, generator.propertyNames().prototype);
RefPtr<RegisterID> isValidPropertyName = generator.emitBinaryOp<OpEq>(generator.newTemporary(), prototypeString, propertyName, OperandTypes(ResultType::stringType(), ResultType::stringType()))
generator.emitJumpIfFalse(isValidPropertyName, validPropertyNameLabel.get());
```

> Source/JavaScriptCore/parser/ASTBuilder.h:408
> +    DefineFieldNode* createDefineField(const JSTokenLocation& location, const Identifier* ident, ExpressionNode* initializer, DefineFieldNode::Type type, bool isStatic)

Could you turn `bool isStatic` into an enum?

> Source/JavaScriptCore/parser/Nodes.h:2295
> +        DefineFieldNode(const JSTokenLocation&, const Identifier*, ExpressionNode*, Type, bool);

Ditto

> Source/JavaScriptCore/parser/Parser.cpp:259
>          else if (parseMode == SourceParseMode::InstanceFieldInitializerMode) {

This is not accurate anymore, since it is also being used for static fields. I think I would prefer create a new `SourceParseMode::StaticFieldInitializerMode` with reason described bellow.

> Source/JavaScriptCore/parser/Parser.cpp:2872
> +    unsigned numInstanceComputedFields = 0, numStaticComputedFields = 1;

I think that a comment here to explain why `numStaticComputedFields` starts with 1 would help a lot. Also, WDYT if we call them as `nextInstanceComputedFieldID` and `nextStaticComputedFieldID`?

> Source/JavaScriptCore/parser/Parser.cpp:2978
> +                unsigned* fieldID = tag == ClassElementTag::Instance ? &numInstanceComputedFields : &numStaticComputedFields;

I'm also in favor of using:

```
if (tag == ClassElementTag::Instance)
    ident = &m_parserArena.identifierArena().makeNumericIdentifier(m_vm, nextInstanceComputedFieldID++);
else
    ident = &m_parserArena.identifierArena().makeNumericIdentifier(m_vm, nextStaticComputedFieldID++);
```

> Source/JavaScriptCore/parser/Parser.cpp:3047
> +template <class TreeBuilder> TreeSourceElements Parser<LexerType>::parseInstanceFieldInitializerSourceElements(TreeBuilder& context, const Vector<JSTextPosition>& classFieldLocations)

This function is also being used to initialize static fields. I think we should rename it to `parseFieldInitializerSourceElements`.

> Source/JavaScriptCore/parser/Parser.cpp:3053
> +    // fields, at 1 for static fields. We'll bump the counter by one

Instead of having this check down there, we could use the `parseMode` to properly initialize `numComputedFields`. I would also rename it as `nextComputedFieldID`.

> Source/JavaScriptCore/runtime/JSFunction.cpp:-759
> -    ASSERT(jsExecutable()->ecmaName().isNull());

Is there a reason to remove this ASSERT?
Comment 4 Xan Lopez 2020-06-05 02:57:46 PDT
Created attachment 401133 [details]
Add support for static public fields, v3.

v3.
Comment 5 Xan Lopez 2020-06-05 03:00:52 PDT
Addressed most of these, commenting inline.

Additional comments:

- There are three failures in the new V8 tests, will look into them now.
- As we discussed I think it would be better to store the computed field keys in something like a list. Will also try to look into this.

(In reply to Caio Lima from comment #3)
> Comment on attachment 400054 [details]
> Add support for static public fields
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400054&action=review
> 
> I think it is necesssary to have some stress test cases included in this
> patch, since most of our bots don't run Test262. It is also important to
> have a test case asserting that the case listed on
> https://bugs.webkit.org/show_bug.cgi?id=209675 can pass.

I imported the V8 tests, like I did with the instance fields. Also refactored a bit both files so they can share the support code.

> 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:796
> > +    generator.emitJumpIfFalse(generator.emitBinaryOp<OpEq>(generator.newTemporary(), generator.emitLoad(nullptr, JSValue(generator.addStringConstant(generator.propertyNames().prototype))), propertyName, OperandTypes(ResultType::stringType(), ResultType::stringType())), validPropertyNameLabel.get());
> 
> I personally think this is very hard to read. Also, I'm not 100% sure if
> this quite safe if we don't use `RefPtr<>`. Do you mind making it something
> like:
> 
> ```
> RefPtr<RegisterID> prototypeString = generator.emitLoad(nullptr,
> generator.propertyNames().prototype);
> RefPtr<RegisterID> isValidPropertyName =
> generator.emitBinaryOp<OpEq>(generator.newTemporary(), prototypeString,
> propertyName, OperandTypes(ResultType::stringType(),
> ResultType::stringType()))
> generator.emitJumpIfFalse(isValidPropertyName, validPropertyNameLabel.get());
> ```

Done, although the exact code you suggested does not work as we discussed offline. Will look into this.

> 
> > Source/JavaScriptCore/parser/ASTBuilder.h:408
> > +    DefineFieldNode* createDefineField(const JSTokenLocation& location, const Identifier* ident, ExpressionNode* initializer, DefineFieldNode::Type type, bool isStatic)
> 
> Could you turn `bool isStatic` into an enum?
> 
> > Source/JavaScriptCore/parser/Nodes.h:2295
> > +        DefineFieldNode(const JSTokenLocation&, const Identifier*, ExpressionNode*, Type, bool);
> 
> Ditto
> 
> > Source/JavaScriptCore/parser/Parser.cpp:259
> >          else if (parseMode == SourceParseMode::InstanceFieldInitializerMode) {

Turns out this was not needed, because the parser code just reparses the fields and figures out which 'mode' it's in (static or instance). We could tell it explicitly, but since it needs to reparse the fields anyway it seemed redundant.

> 
> This is not accurate anymore, since it is also being used for static fields.
> I think I would prefer create a new
> `SourceParseMode::StaticFieldInitializerMode` with reason described bellow.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:2872
> > +    unsigned numInstanceComputedFields = 0, numStaticComputedFields = 1;
> 
> I think that a comment here to explain why `numStaticComputedFields` starts
> with 1 would help a lot. Also, WDYT if we call them as
> `nextInstanceComputedFieldID` and `nextStaticComputedFieldID`?
> 
> > Source/JavaScriptCore/parser/Parser.cpp:2978
> > +                unsigned* fieldID = tag == ClassElementTag::Instance ? &numInstanceComputedFields : &numStaticComputedFields;
> 
> I'm also in favor of using:
> 
> ```
> if (tag == ClassElementTag::Instance)
>     ident = &m_parserArena.identifierArena().makeNumericIdentifier(m_vm,
> nextInstanceComputedFieldID++);
> else
>     ident = &m_parserArena.identifierArena().makeNumericIdentifier(m_vm,
> nextStaticComputedFieldID++);
> ```
> 
> > Source/JavaScriptCore/parser/Parser.cpp:3047
> > +template <class TreeBuilder> TreeSourceElements Parser<LexerType>::parseInstanceFieldInitializerSourceElements(TreeBuilder& context, const Vector<JSTextPosition>& classFieldLocations)
> 
> This function is also being used to initialize static fields. I think we
> should rename it to `parseFieldInitializerSourceElements`.

All done.

> 
> > Source/JavaScriptCore/parser/Parser.cpp:3053
> > +    // fields, at 1 for static fields. We'll bump the counter by one
> 
> Instead of having this check down there, we could use the `parseMode` to
> properly initialize `numComputedFields`. I would also rename it as
> `nextComputedFieldID`.
> 
> > Source/JavaScriptCore/runtime/JSFunction.cpp:-759
> > -    ASSERT(jsExecutable()->ecmaName().isNull());
> 
> Is there a reason to remove this ASSERT?

This was a leftover from a previous way of doing things! Removed.
Comment 6 Xan Lopez 2020-07-12 02:46:55 PDT
Created attachment 404092 [details]
Add support for static public fields, v4

New version.

Fixed two of the failing V8 tests, a few other naming fixes, rebased against master.

There is one remaining V8 failure, but I think this is not a field bug per se, just the new code finding an old issue. Basically:

 let q = { ["z"]: class { static y = this.name } }
 assertEquals(q.z.y, 'z');

This will fail (the value of q.z.y is ''). The reason is that the existing code sets the function name for the class *after* the whole class is generated. The spec says it should happen within the class definition evaluation (see 14.6.14 Runtime Semantics: ClassDefinitionEvaluation). This was perhaps mostly harmless until now, but with static fields the initializer runs before the class evaluation is over, so it finds 'this.name' to be unitialized. This would work for a non-computed property name (z: class { ... }) because in that case the parser itself will already set the proper function name, but for computed property names that does not happen. I will open a follow-up for this issue if we agree it should be fixed in parallel.
Comment 7 Xan Lopez 2020-07-21 06:52:24 PDT
Created attachment 404819 [details]
Add support for static public fields, v5

v5, rebased against master.
Comment 8 Saam Barati 2020-07-21 20:13:50 PDT
Comment on attachment 404819 [details]
Add support for static public fields, v5

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

> Source/JavaScriptCore/ChangeLog:43
> +        following order: b, c, a, d. Since each group of fields is
> +        initialized independently we can trivially retrieve the property
> +        keys for each group just by getting the odd property keys for
> +        static fields (indexes 1 and 3) and the even ones for instance
> +        fields (indexes 0 and 2).

I don't get this. Why do we need property keys of numbers ever? Are we looking up in an array or something?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:823
> +    RefPtr<RegisterID> propertyExpr = generator.emitNode(node.m_expression);
> +    RefPtr<RegisterID> propertyName = generator.emitToPropertyKey(generator.newTemporary(), propertyExpr.get());
> +
> +    Ref<Label> validPropertyNameLabel = generator.newLabel();
> +    RefPtr<RegisterID> prototypeString = generator.emitLoad(nullptr, JSValue(generator.addStringConstant(generator.propertyNames().prototype)));
> +    generator.emitJumpIfFalse(generator.emitBinaryOp<OpEq>(generator.newTemporary(), prototypeString.get(), propertyName.get(), OperandTypes(ResultType::stringType(), ResultType::stringType())), validPropertyNameLabel.get());
> +    generator.emitThrowTypeError("Cannot declare a static field named 'prototype'");
>  
> +    generator.emitLabel(validPropertyNameLabel.get());

this code in emitSaveComputedFieldName is only called for static fields?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4822
> +

nit: revert

> Source/JavaScriptCore/parser/Parser.cpp:2885
> +    unsigned nextInstanceComputedFieldID = 0, nextStaticComputedFieldID = 1;

nit: separate declarations on individual lines..

> Source/JavaScriptCore/parser/Parser.cpp:3087
> +    // Property keys for computed fields start at 0 for instance

this is a confusing model to me. Why do we have numeric identifiers for each field?

> Source/JavaScriptCore/parser/Parser.cpp:3110
> +            if (!nextComputedFieldID)
> +                nextComputedFieldID++;

so the first field dictates if all other fields are static or not static?
Comment 9 Xan Lopez 2020-07-28 04:25:31 PDT
Hi,

thanks for the review.

(In reply to Saam Barati from comment #8)
> Comment on attachment 404819 [details]
> Add support for static public fields, v5
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404819&action=review
> 
> > Source/JavaScriptCore/ChangeLog:43
> > +        following order: b, c, a, d. Since each group of fields is
> > +        initialized independently we can trivially retrieve the property
> > +        keys for each group just by getting the odd property keys for
> > +        static fields (indexes 1 and 3) and the even ones for instance
> > +        fields (indexes 0 and 2).
> 
> I don't get this. Why do we need property keys of numbers ever? Are we
> looking up in an array or something?

Most questions are about this in one way or another, so I'll try to answer them all here.

Computed properties need to be evaluated (and transformed into property keys) during the first class evaluation. *Instance* class fields are initialized later. That means we need some place to store the property keys meanwhile. In the already landed instance class fields patch (https://bugs.webkit.org/show_bug.cgi?id=174212) those property keys were stored in the class scope and could be retrieved through numerical indexes, essentially a poor man's array.

What this patch does is build up on that, splitting the property keys for static and instance fields in even/odd indexes because they need to be read separately. Again, this is a poor man's way of doing two arrays. Also, strictly speaking, we could do without this for static fields, but since we are also using a field initializer it's much easier to reuse the whole infrastructure in place.

At the very least all this could be changed to use proper arrays, I think I mentioned that possibility in an earlier comment. Maybe we could first improve the way this is done for instance fields, and when we are happy with it change this patch to use the same mechanism. Or something else! Open to any suggestion here, hopefully it's easier to iterate the right solution now that this patch passes all tests.
 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:823
> > +    RefPtr<RegisterID> propertyExpr = generator.emitNode(node.m_expression);
> > +    RefPtr<RegisterID> propertyName = generator.emitToPropertyKey(generator.newTemporary(), propertyExpr.get());
> > +
> > +    Ref<Label> validPropertyNameLabel = generator.newLabel();
> > +    RefPtr<RegisterID> prototypeString = generator.emitLoad(nullptr, JSValue(generator.addStringConstant(generator.propertyNames().prototype)));
> > +    generator.emitJumpIfFalse(generator.emitBinaryOp<OpEq>(generator.newTemporary(), prototypeString.get(), propertyName.get(), OperandTypes(ResultType::stringType(), ResultType::stringType())), validPropertyNameLabel.get());
> > +    generator.emitThrowTypeError("Cannot declare a static field named 'prototype'");
> >  
> > +    generator.emitLabel(validPropertyNameLabel.get());
> 
> this code in emitSaveComputedFieldName is only called for static fields?
> 

Good catch, the code used to be like that but this is a mistake, should be guarded for static fields only.
Comment 10 Xan Lopez 2020-07-28 04:34:17 PDT
Created attachment 405349 [details]
Add support for static public fields, v6

v6, fixing the issues mentioned in the review except the ad-hoc array thing for property keys, which needs more discussion. Uploading this to have an up to date "reference patch".
Comment 11 Xan Lopez 2020-10-27 05:20:56 PDT
Created attachment 412412 [details]
Add support for static public fields, v7

v7,
rebased on top of the symbol-as-id patch (still to land), which allows us to remove all the stuff about even/odd ids for computed property keys.