Bug 235085 – [JSC] Implement support for class static initialization blocks

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Devin Rousso]

<https://github.com/tc39/proposal-class-static-block>

[Radar WebKit Bug Importer]

<rdar://problem/87421954>

[Rob Palmer]

This feature went to Stage 4 one year ago (August 2021) and is now part of ES2022.

The consequence of not implementing this feature is that many users of the feature are forced to transpile/downlevel their code to ES2021.

This over-downlevelling has the unfortunate side-effect of causing all private fields to be stored in WeakMaps, which tend to be much slower than native private fields.

[Mark Lam]

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

[Caitlin Potter (:caitp)]

FYI, Serakeri has been working on this offline

[Mark Lam]

(In reply to Caitlin Potter (:caitp) from comment #4)
> FYI, Serakeri has been working on this offline

Yijia has also been working on this for a while.  Perhaps we should sync up and avoid duplicating work.

[Yijia Huang]

Created attachment 462669 [details]
Patch

[Yusuke Suzuki]

@Caitlin, Yijia's patch is now uploaded for review / merge.

[Yusuke Suzuki]

Comment on attachment 462669 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1259
> +    RegisterID* ret;

Use `RefPtr<RegisterID>`.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4994
> +        RegisterID* res = generator.emitNewFunctionExpression(generator.finalDestination(dst), this);

Use `RefPtr<RegisterID>`.

> Source/JavaScriptCore/parser/ASTBuilder.h:133
> +    ExpressionNode* makeFunctionCallNode(const JSTokenLocation&, ExpressionNode* func, bool previousBaseWasSuper, ArgumentsNode* args, const JSTextPosition& divotStart, const JSTextPosition& divot, const JSTextPosition& divotEnd, size_t callOrApplyChildDepth, bool isOptionalCall, bool isStaticClassBlockFunctionCall = false);

Avoid using `bool` for parameter since it is hard to know what it means from the callsite. Use enum instead.

> Source/JavaScriptCore/parser/ASTBuilder.h:428
> +    ExpressionNode* createFunctionExpr(const JSTokenLocation& location, const ParserFunctionInfo<ASTBuilder>& functionInfo, bool isStaticBlockFunction = false)

Avoid using `bool` for parameter since it is hard to know what it means from the callsite. Use enum instead.

> Source/JavaScriptCore/parser/ASTBuilder.h:1424
> +ExpressionNode* ASTBuilder::makeFunctionCallNode(const JSTokenLocation& location, ExpressionNode* func, bool previousBaseWasSuper, ArgumentsNode* args, const JSTextPosition& divotStart, const JSTextPosition& divot, const JSTextPosition& divotEnd, size_t callOrApplyChildDepth, bool isOptionalCall, bool isStaticBlockFunctionCall)

Avoid using `bool` for parameter since it is hard to know what it means from the callsite. Use enum instead.

> Source/JavaScriptCore/parser/NodeConstructors.h:412
> +    inline FunctionCallValueNode::FunctionCallValueNode(const JSTokenLocation& location, ExpressionNode* expr, ArgumentsNode* args, const JSTextPosition& divot, const JSTextPosition& divotStart, const JSTextPosition& divotEnd, const bool isStaticBlockFunctionCall)

Avoid using `bool` for parameter since it is hard to know what it means from the callsite. Use enum instead.

> Source/JavaScriptCore/parser/NodeConstructors.h:1034
> +    inline FuncExprNode::FuncExprNode(const JSTokenLocation& location, const Identifier& ident, FunctionMetadataNode* metadata, const SourceCode& source, bool isStaticBlockFunction)

Avoid using `bool` for parameter since it is hard to know what it means from the callsite. Use enum instead.

> Source/JavaScriptCore/parser/Nodes.h:991
> +        FunctionCallValueNode(const JSTokenLocation&, ExpressionNode*, ArgumentsNode*, const JSTextPosition& divot, const JSTextPosition& divotStart, const JSTextPosition& divotEnd, bool isStatic = false);

Avoid using `bool` for parameter since it is hard to know what it means from the callsite. Use enum instead.

> Source/JavaScriptCore/parser/Nodes.h:2343
> +        FuncExprNode(const JSTokenLocation&, const Identifier&, FunctionMetadataNode*, const SourceCode&, bool isStaticBlock = false);

Avoid using `bool` for parameter since it is hard to know what it means from the callsite. Use enum instead.

> Source/JavaScriptCore/parser/Parser.cpp:1899
> +template <class TreeBuilder> TreeStatement Parser<LexerType>::parseBlockStatement(TreeBuilder& context, bool isCatchBlock, bool isStaticBlock)

Avoid using `bool` for parameter since it is hard to know what it means from the callsite. Use enum instead.

> Source/JavaScriptCore/parser/Parser.h:1763
> +    template <class TreeBuilder> TreeStatement parseBlockStatement(TreeBuilder&, bool isCatchBlock = false, bool isStaticBlock = false);

Avoid using `bool` for parameter since it is hard to know what it means from the callsite. Use enum instead.

> Source/JavaScriptCore/parser/SyntaxChecker.h:151
> +    ExpressionType makeFunctionCallNode(const JSTokenLocation&, ExpressionType, bool, int, int, int, int, size_t, bool, bool = false) { return CallExpr; }

Avoid using `bool` for parameter since it is hard to know what it means from the callsite. Use enum instead.

> Source/JavaScriptCore/parser/SyntaxChecker.h:198
> +    ExpressionType createFunctionExpr(const JSTokenLocation&, const ParserFunctionInfo<SyntaxChecker>&, bool = false) { return FunctionExpr; }

Avoid using `bool` for parameter since it is hard to know what it means from the callsite. Use enum instead.

> JSTests/stress/class-static-block.js:1
> +function assert(b) {

Enable test262 too to ensure that they are correctly implemented. See JSTests/test262/config.yaml to enable it for this feature.

[Yijia Huang]

Created attachment 462685 [details]
Patch

[Yijia Huang]

Created attachment 462688 [details]
Patch

[Yijia Huang]

Pull request: https://github.com/WebKit/WebKit/pull/4372

[Mark Lam]

Comment on attachment 462688 [details]
Patch

Taking this bugzilla patch out of review.  A PR has been posted on WebKit Github.  Will continue reviewing there.

[Darin Adler]

Comment on attachment 462688 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4994
> +        RefPtr<RegisterID> homeObject = emitHomeObjectForCallee(generator);
> +        RefPtr<RegisterID> res = generator.emitNewFunctionExpression(generator.finalDestination(dst), this);

Should be able to write just RefPtr here rather than RefPtr<RegisterID>.

Not sure if there are different traditions in this code, but typically in WebKit we prefer words, like result, rather than fragments, like res.

> Source/JavaScriptCore/parser/NodeConstructors.h:256
> +        , m_expression(nullptr)
> +        , m_assign(nullptr)

These could be initialized in the class definition instead of the constructor.

> Source/JavaScriptCore/parser/NodeConstructors.h:260
> +        , m_isOverriddenByDuplicate(false)

This could be initialized in the class definition instead of the constructor.

> Source/JavaScriptCore/parser/NodeConstructors.h:1042
> +        , m_isStaticBlockFunction(false)

This could be initialized in the class definition instead of the constructor.

> Source/JavaScriptCore/parser/Nodes.h:1002
>          ExpressionNode* m_expr;
>          ArgumentsNode* m_args;
> +        bool m_isStaticBlockFunctionCall;

I suggest we initialize these to nullptr and false here in the class definition.

> Source/JavaScriptCore/parser/Nodes.h:2354
> +        bool m_isStaticBlockFunction;

I suggest we initialize this to false here in the class definition.

> Source/JavaScriptCore/parser/Parser.cpp:3101
> +        case OPENBRACE: {

No braces needed here.

> Source/JavaScriptCore/parser/Parser.cpp:3318
> +            case OPENBRACE: {
> +                break;
> +            }

No braces needed here.

[Yijia Huang]

(In reply to Darin Adler from comment #13)

Thanks for your review, Darin. We've already moved our review to the GitHub PR https://github.com/WebKit/WebKit/pull/4372.

[EWS]

Committed 255173@main (c33391001b54): <https://commits.webkit.org/255173@main>

Reviewed commits have been landed. Closing PR #4372 and removing active labels.