Bug 219181 - [JSC] Implement private static method
Summary: [JSC] Implement private static method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Lima
URL:
Keywords: InRadar
Depends on: 194434 194435
Blocks:
  Show dependency treegraph
 
Reported: 2020-11-19 15:07 PST by Yusuke Suzuki
Modified: 2021-02-18 15:48 PST (History)
11 users (show)

See Also:


Attachments
Patch (58.04 KB, patch)
2021-02-16 12:48 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (58.05 KB, patch)
2021-02-17 04:57 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (58.04 KB, patch)
2021-02-18 07:30 PST, Caio Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-11-19 15:07:23 PST
...
Comment 1 Radar WebKit Bug Importer 2020-11-26 15:08:27 PST
<rdar://problem/71754440>
Comment 2 Caio Lima 2021-02-16 12:48:24 PST
Created attachment 420525 [details]
Patch
Comment 3 Caio Lima 2021-02-17 04:57:58 PST
Created attachment 420629 [details]
Patch
Comment 4 Yusuke Suzuki 2021-02-17 23:51:10 PST
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?
Comment 5 Caio Lima 2021-02-18 07:29:41 PST
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.
Comment 6 Caio Lima 2021-02-18 07:30:32 PST
Created attachment 420828 [details]
Patch
Comment 7 EWS 2021-02-18 15:48:34 PST
Committed r273107: <https://commits.webkit.org/r273107>

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