Bug 194095

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.

Created attachment 400054 [details] Add support for static public fields

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?

Created attachment 401133 [details] Add support for static public fields, v3. v3.

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.

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.

Created attachment 404819 [details] Add support for static public fields, v5 v5, rebased against master.

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?

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.

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

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.

Created attachment 414161 [details] Add support for static public fields, v8 v8, use new makePrivateIdentifier method when parsing.

Comment on attachment 414161 [details] Add support for static public fields, v8 View in context: https://bugs.webkit.org/attachment.cgi?id=414161&action=review I'll comment about some quick things. I'll review it tomorrow or Monday. Can you put `r?` when requesting a review? > JSTests/ChangeLog:1 > +2020-06-05 Xan López <xan@igalia.com> Let's update the date. > JSTests/stress/class-fields-harmony.js:34 > +load("./harmony-support.js"); Let's put JSTests/stress/resources/harmony-support.js under resources. Otherwise, harmony-support.js is executed as one test. > JSTests/stress/class-fields-static-harmony.js:1 > +//@ requireOptions("--usePublicClassFields=1") Use 'usePublicStaticClassFields' > JSTests/stress/class-fields-static-harmony.js:36 > +load("./harmony-support.js"); Ditto. > Source/JavaScriptCore/ChangeLog:1 > +2020-06-05 Xan López <xan@igalia.com> Let's update the date. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:186 > Hmm, I cannot expand the diff in this file. Can you rebase the patch?

Created attachment 414165 [details] Add support for static public fields, v9 v9, move harmony support file to resources/ add flag to enable/disable only static public fields, use it in the parser and in the tests

Comment on attachment 414165 [details] Add support for static public fields, v9 View in context: https://bugs.webkit.org/attachment.cgi?id=414165&action=review Overall looks good. But I put r- since I have several questions about possible bugs. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:827 > + generator.emitJumpIfFalse(generator.emitBinaryOp<OpEq>(generator.newTemporary(), prototypeString.get(), propertyName.get(), OperandTypes(ResultType::stringType(), ResultType::stringType())), validPropertyNameLabel.get()); Use OpStrictEq. > Source/JavaScriptCore/parser/Parser.cpp:2885 > + unsigned nextInstanceComputedFieldID = 0, nextStaticComputedFieldID = 0; Use unsigned nextInstanceComputedFieldID = 0; unsigned nextStaticComputedFieldID = 0; we do not use comma typically here. > Source/JavaScriptCore/parser/Parser.cpp:3080 > + bool isStaticField = false; Why is it defined outside of for loop? What happens if the first field is static and next field is not static (I think in that case, isStaticField boolean becomes wrong). Can you add a test about this case? > Source/JavaScriptCore/parser/Parser.cpp:3098 > + if (match(RESERVED_IF_STRICT) && *m_token.m_data.ident == m_vm.propertyNames->staticKeyword) { > + ident = m_token.m_data.ident; > + ASSERT(ident); > + next(); > + if (match(SEMICOLON) || match (EQUAL) || match(CLOSEBRACE) || m_lexer->hasLineTerminatorBeforeToken()) > + goto validField; > + isStaticField = true; > + } Let's set ident only when it is valid field, and check ident with nullptr. And then, avoid using goto. Like if () { auto* staticIdentifier = m_token.m_data.ident; ASSERT(staticIdent); next(); if (match(SEMICOLON) || match (EQUAL) || match(CLOSEBRACE) || m_lexer->hasLineTerminatorBeforeToken()) ident = staticIdentifier; else isStaticField = true; } if (!ident) { switch (...) { ... } > Source/JavaScriptCore/parser/Parser.cpp:3139 > + validField: Let's avoid using goto.

ACK to everything, save for one comment: (In reply to Yusuke Suzuki from comment #15) > we do not use comma typically here. > > > Source/JavaScriptCore/parser/Parser.cpp:3080 > > + bool isStaticField = false; > > Why is it defined outside of for loop? What happens if the first field is > static and next field is not static (I think in that case, isStaticField > boolean becomes wrong). > Can you add a test about this case? This method will only parse either a vector of static fields or a vector of instance fields, never a mix. So the case you mention cannot happen (there's all sorts of tests with fields in every possible order and combinations). I have added a comment and added the boolean inside the loop, so this is more clear already within the method itself.

Created attachment 414315 [details] Add support for static public fields, v10 v10, address review comments

Comment on attachment 414315 [details] Add support for static public fields, v10 r=me, awesome!

Committed r269922: <https://trac.webkit.org/changeset/269922> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414315 [details].

<rdar://problem/71501769>

*** Bug 219397 has been marked as a duplicate of this bug. ***