Bug 219181 – [JSC] Implement private static method

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

[Yusuke Suzuki]

...

[Radar WebKit Bug Importer]

<rdar://problem/71754440>

[Caio Lima]

Created attachment 420525 [details]
Patch

[Caio Lima]

Created attachment 420629 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 420629 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2817
> +        RefPtr<RegisterID> condition = newTemporary();
> +        emitEqualityOp<OpStricteq>(condition.get(), base, brand);
> +        emitJumpIfTrue(condition.get(), brandCheckOkLabel.get());

Please follow https://bugs.webkit.org/show_bug.cgi?id=221927 style

> Source/JavaScriptCore/parser/Parser.cpp:3059
> +                    auto declarationResult = classScope->declarePrivateSetter(*ident, tag);
> +                    semanticFailIfTrue(declarationResult & DeclarationResult::InvalidDuplicateDeclaration, "Declared private setter with an already used name");
> +                    if (tag == ClassElementTag::Static) {
> +                        semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private static setter if there is a non-static private getter with used name");
> +                        declaresStaticPrivateAccessor = true;
> +                    } else {
> +                        semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private non-static setter if there is a static private getter with used name");
> +                        declaresPrivateAccessor = true;
> +                    }
>                      type = static_cast<PropertyNode::Type>(type | PropertyNode::PrivateSetter);
>                  } else {
> -                    semanticFailIfTrue(classScope->declarePrivateGetter(*ident) & DeclarationResult::InvalidDuplicateDeclaration, "Declared private getter with an already used name");
> -                    declaresPrivateAccessor = true;
> +                    auto declarationResult = classScope->declarePrivateGetter(*ident, tag);
> +                    semanticFailIfTrue(declarationResult & DeclarationResult::InvalidDuplicateDeclaration, "Declared private getter with an already used name");
> +                    if (tag == ClassElementTag::Static) {
> +                        semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private static getter if there is a non-static private setter with used name");
> +                        declaresStaticPrivateAccessor = true;
> +                    } else {
> +                        semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private non-static getter if there is a static private setter with used name");
> +                        declaresPrivateAccessor = true;
> +                    }

Both looks almost identical. We should remove `if (isSetter)` branching and pass "getter" / "setter" as a parameter to the error message.

auto declarationResult = classScope->declarePrivateGetter(*ident, tag);
semanticFailIfTrue(declarationResult & DeclarationResult::InvalidDuplicateDeclaration, "Declared private ", (isSetter ? "setter" : "getter"), " with an already used name");
if (tag == ClassElementTag::Static) {
    semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private static ", (isSetter ? "setter" : "getter"), "if there is a non-static private setter with used name");
    declaresStaticPrivateAccessor = true;
} else {
    semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private non-static ", (isSetter ? "setter" : "getter"), " if there is a static private setter with used name");
    declaresPrivateAccessor = true;
}

> Source/JavaScriptCore/parser/Parser.h:518
>          return result;

Can you explain why declarePrivateMethod requires 

        useVariable(&ident, false);
        addClosedVariableCandidateUnconditionally(ident.impl());

and the others are not?

> Source/JavaScriptCore/parser/Parser.h:555
> +    DeclarationResultMask declarePrivateSetter(const Identifier& ident, ClassElementTag tag)
>      {
>          ASSERT(m_allowsLexicalDeclarations);
>          DeclarationResultMask result = DeclarationResult::Valid;
> -        bool addResult = m_lexicalVariables.declarePrivateSetter(ident);
> +        auto addResult = tag == ClassElementTag::Static ? m_lexicalVariables.declareStaticPrivateSetter(ident) : m_lexicalVariables.declarePrivateSetter(ident);
>  
> -        if (!addResult) {
> +        if (addResult == VariableEnvironment::PrivateDeclarationResult::DuplicatedName) {
>              result |= DeclarationResult::InvalidDuplicateDeclaration;
>              return result;
>          }
>  
> +        if (addResult == VariableEnvironment::PrivateDeclarationResult::InvalidStaticNonStatic) {
> +            result |= DeclarationResult::InvalidPrivateStaticNonStatic;
> +            return result;
> +        }
> +
>          return result;
>      }
>  
> -    DeclarationResultMask declarePrivateGetter(const Identifier& ident)
> +    DeclarationResultMask declarePrivateGetter(const Identifier& ident, ClassElementTag tag)
>      {
>          ASSERT(m_allowsLexicalDeclarations);
>          DeclarationResultMask result = DeclarationResult::Valid;
> -        bool addResult = m_lexicalVariables.declarePrivateGetter(ident);
> +        auto addResult = tag == ClassElementTag::Static ? m_lexicalVariables.declareStaticPrivateGetter(ident) : m_lexicalVariables.declarePrivateGetter(ident);
>  
> -        if (!addResult) {
> +        if (addResult == VariableEnvironment::PrivateDeclarationResult::DuplicatedName) {
>              result |= DeclarationResult::InvalidDuplicateDeclaration;
>              return result;
>          }
>  
> +        if (addResult == VariableEnvironment::PrivateDeclarationResult::InvalidStaticNonStatic) {
> +            result |= DeclarationResult::InvalidPrivateStaticNonStatic;
> +            return result;
> +        }
> +

These functions are almost identical. Can you extract the common part out from them?

[Caio Lima]

Comment on attachment 420629 [details]
Patch

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

Thanks a lot for the review

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2817
>> +        emitJumpIfTrue(condition.get(), brandCheckOkLabel.get());
> 
> Please follow https://bugs.webkit.org/show_bug.cgi?id=221927 style

Done.

>> Source/JavaScriptCore/parser/Parser.cpp:3059
>> +                    }
> 
> Both looks almost identical. We should remove `if (isSetter)` branching and pass "getter" / "setter" as a parameter to the error message.
> 
> auto declarationResult = classScope->declarePrivateGetter(*ident, tag);
> semanticFailIfTrue(declarationResult & DeclarationResult::InvalidDuplicateDeclaration, "Declared private ", (isSetter ? "setter" : "getter"), " with an already used name");
> if (tag == ClassElementTag::Static) {
>     semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private static ", (isSetter ? "setter" : "getter"), "if there is a non-static private setter with used name");
>     declaresStaticPrivateAccessor = true;
> } else {
>     semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private non-static ", (isSetter ? "setter" : "getter"), " if there is a static private setter with used name");
>     declaresPrivateAccessor = true;
> }

Done. I needed to add a couple of ternary per message, but we then removed a lot of duplicated code.

>> Source/JavaScriptCore/parser/Parser.h:518
>>          return result;
> 
> Can you explain why declarePrivateMethod requires 
> 
>         useVariable(&ident, false);
>         addClosedVariableCandidateUnconditionally(ident.impl());
> 
> and the others are not?

This is a leftover from an old implementation of `VariableEnvironment::declarePrivateMethod` that was not setting the `VariableEnvyronmentEntry` properly. I just removed it, since it's unnecessary.

>> Source/JavaScriptCore/parser/Parser.h:555
>> +
> 
> These functions are almost identical. Can you extract the common part out from them?

Done.

[Caio Lima]

Created attachment 420828 [details]
Patch

[EWS]

Committed r273107: <https://commits.webkit.org/r273107>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420828 [details].