Bug 194435 – [ESNext] Implement private accessors

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caio Lima]

Private accessors are specified on https://tc39.github.io/proposal-private-methods/.

[Caio Lima]

Created attachment 370695 [details]
WIP - Patch

This Patch is dependent on https://bugs.webkit.org/show_bug.cgi?id=194434. WIP

[Caio Lima]

Created attachment 373127 [details]
WIP - Patch

This Patch doesn't apply to trunk, since it is based on top of https://bugs.webkit.org/show_bug.cgi?id=194434

[Caio Lima]

Created attachment 391861 [details]
WIP - Patch

[Caio Lima]

Created attachment 391866 [details]
WIP - Patch

[Caio Lima]

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

[Caio Lima]

Created attachment 419845 [details]
Patch

[Caio Lima]

Created attachment 419866 [details]
Patch

[Caio Lima]

Comment on attachment 419866 [details]
Patch

I've found a couple of bugs when using private identifiers instead of common identifiers.

[Caio Lima]

Created attachment 419911 [details]
Patch

[Caio Lima]

Created attachment 419994 [details]
Patch

[Caio Lima]

Comment on attachment 419994 [details]
Patch

r- because I've found another bug on postfix/prefix nodes.

[Caio Lima]

Created attachment 420052 [details]
WIP Patch

[Caio Lima]

Comment on attachment 420052 [details]
WIP Patch

This is adding initial support for private accessors and postfix/prefix nodes. Tests are still needed.

[Caio Lima]

Created attachment 420127 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 420127 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/ChangeLog:37
> +        new_object         dst:loc12, inlineCapacity:2 // this is the object to store getter and setter pair

Can we have a FIXME about using GetterSetter instead?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3000
> +            if (!excludedNames.contains(entry.key.get())) {
> +                result.add(entry.key, entry.value);
>                  excludedNames.add(entry.key.get());
> +            }

contains & add is duplicate cost. Let's do,

auto addResult = excludedNames.add(entry.key.get());
if (addResult.isNewEntry)
    result.add(entry.key, entry.value);

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:583
> +    typedef std::pair<PropertyNode*, PropertyNode*> GetterSetterPair;
> +    typedef HashMap<UniquedStringImpl*, GetterSetterPair, IdentifierRepHash> GetterSetterMap;

Use `using` instead for new code.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:588
> +    for (; p; p = p->m_next) {

Let's not use p variable here. This is used for the latter iteration. So, let's move

PropertyListNode* p = this;
RegisterID* dst = nullptr;

just before // Fast case: this loop just handles regular value properties.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:601
> +        if (!(p->m_node->type() & (PropertyNode::PrivateGetter | PropertyNode::PrivateSetter)))
> +            continue;
> +
> +        // We group private getters and setters to store them in a object
> +        GetterSetterPair pair(p->m_node, static_cast<PropertyNode*>(nullptr));
> +        GetterSetterMap::AddResult result = privateAccessorMap.add(p->m_node->name()->impl(), pair);
> +        auto& resultPair = result.iterator->value;
> +        // If the map already contains an element with node->name(),
> +        // we need to store this node in the second part.
> +        if (!result.isNewEntry)
> +            resultPair.second = p->m_node;
> +        continue;
> +    }

Can we collect this information into PropertyListNode when parsing?
private getter / setter is not so frequently seen. We should not iterate every time.
Like,

if (hasPrivateGettersOrSetters()) {
    GetterSetterMap privateAccessorMap;
    for (; p; p = p->m_next) {
        ...
    }
    ...
    for (auto& it : privateAccessorMap) {
        ...
    }
}

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:603
> +    // Then we declare private accessos

accessos => accessors.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:609
> +            dst = pair.first->isInstanceClassProperty() ? prototype : dstOrConstructor;

Let's use block local variable instead of dst.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:628
> +            generator.emitDirectPutById(getterSetterObj.get(), setterOrGetterIdent, value.get());
> +        }
>  
> +        if (pair.second) {
> +            dst = pair.second->isInstanceClassProperty() ? prototype : dstOrConstructor;
> +            RefPtr<RegisterID> value = generator.emitNode(pair.second->m_assign);
> +            if (pair.second->needsSuperBinding())
> +                emitPutHomeObject(generator, value.get(), dst);
> +            auto setterOrGetterIdent = pair.second->m_type & PropertyNode::PrivateGetter
> +                ? generator.propertyNames().builtinNames().getPrivateName()
> +                : generator.propertyNames().builtinNames().setPrivateName();
> +            generator.emitDirectPutById(getterSetterObj.get(), setterOrGetterIdent, value.get());
> +        }

They are largely duplicate. Please add lambda to dedupe code.

if (pair.first)
    xxxxx(pair.first);
if (pair.second)
    xxxxx(pair.second);

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:983
>              RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Use RefPtr<RegisterID>.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:993
> +            RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Use `RefPtr<RegisterID> privateBrandSymbol`.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1008
> +            RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1010
> +            generator.emitThrowTypeError("Trying to access a not defined private getter");

"Trying to access an undefined private getter"

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1048
> +            RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1068
>              generator.emitThrowTypeError("Trying to access a not defined private setter");

"Trying to access an undefined private setter"

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2413
> +            RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Ditto. Please check the same things too.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2424
> +        RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2435
> +            generator.emitThrowTypeError("Trying to access a not defined private getter");

"Trying to access an undefined private getter"

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2453
> +        generator.emitThrowTypeError("Trying to access a not defined private getter");

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2702
> +            RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2706
> +            generator.emitThrowTypeError("Trying to access a not defined private setter");

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2713
> +        RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2723
> +            generator.emitThrowTypeError("Trying to access a not defined private getter");

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2741
> +        generator.emitThrowTypeError("Trying to access a not defined private getter");

Ditto.

> Source/JavaScriptCore/parser/VariableEnvironment.cpp:129
> +        if (currentEntry.isSetter() || currentEntry.isMethod() || !currentEntry.isGetter())

Is `currentEntry.isSetter() || currentEntry.isMethod()` necessary?

> Source/JavaScriptCore/parser/VariableEnvironment.cpp:173
> +bool VariableEnvironment::declarePrivateGetter(const RefPtr<UniquedStringImpl>& identifier)
> +{
> +    if (!m_rareData)
> +        m_rareData = WTF::makeUnique<VariableEnvironment::RareData>();
> +
> +    auto findResult = m_rareData->m_privateNames.find(identifier);
> +
> +    if (findResult == m_rareData->m_privateNames.end()) {
> +        PrivateNameEntry meta(PrivateNameEntry::Traits::IsDeclared | PrivateNameEntry::Traits::IsGetter);
> +
> +        auto entry = VariableEnvironmentEntry();
> +        entry.setIsPrivateGetter();
> +        entry.setIsConst();
> +        entry.setIsCaptured();
> +        m_map.add(identifier, entry);
> +
> +        auto addResult = m_rareData->m_privateNames.add(identifier, meta);
> +        return addResult.isNewEntry;
> +    }

Looks like declarePrivateGetter is largely duplicate to declarePrivateSetter. Could you dedupe the code?

> Source/JavaScriptCore/parser/VariableEnvironment.cpp:177
> +        if (currentEntry.isGetter() || currentEntry.isMethod() || !currentEntry.isSetter())

Is `currentEntry.isGetter() || currentEntry.isMethod()` necessary?

[Caio Lima]

Comment on attachment 420127 [details]
Patch

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

Thank you very much for the review!

>> Source/JavaScriptCore/ChangeLog:37
>> +        new_object         dst:loc12, inlineCapacity:2 // this is the object to store getter and setter pair
> 
> Can we have a FIXME about using GetterSetter instead?

Definitely yes.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3000
>> +            }
> 
> contains & add is duplicate cost. Let's do,
> 
> auto addResult = excludedNames.add(entry.key.get());
> if (addResult.isNewEntry)
>     result.add(entry.key, entry.value);

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:583
>> +    typedef HashMap<UniquedStringImpl*, GetterSetterPair, IdentifierRepHash> GetterSetterMap;
> 
> Use `using` instead for new code.

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:588
>> +    for (; p; p = p->m_next) {
> 
> Let's not use p variable here. This is used for the latter iteration. So, let's move
> 
> PropertyListNode* p = this;
> RegisterID* dst = nullptr;
> 
> just before // Fast case: this loop just handles regular value properties.

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:601
>> +    }
> 
> Can we collect this information into PropertyListNode when parsing?
> private getter / setter is not so frequently seen. We should not iterate every time.
> Like,
> 
> if (hasPrivateGettersOrSetters()) {
>     GetterSetterMap privateAccessorMap;
>     for (; p; p = p->m_next) {
>         ...
>     }
>     ...
>     for (auto& it : privateAccessorMap) {
>         ...
>     }
> }

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:609
>> +            dst = pair.first->isInstanceClassProperty() ? prototype : dstOrConstructor;
> 
> Let's use block local variable instead of dst.

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:628
>> +        }
> 
> They are largely duplicate. Please add lambda to dedupe code.
> 
> if (pair.first)
>     xxxxx(pair.first);
> if (pair.second)
>     xxxxx(pair.second);

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:983
>>              RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());
> 
> Use RefPtr<RegisterID>.

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:993
>> +            RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());
> 
> Use `RefPtr<RegisterID> privateBrandSymbol`.

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1010
>> +            generator.emitThrowTypeError("Trying to access a not defined private getter");
> 
> "Trying to access an undefined private getter"

I changed it, but I'm a bit worried that this can cause some confusion about about JS `undefined` value.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1048
>> +            RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());
> 
> Ditto.

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1068
>>              generator.emitThrowTypeError("Trying to access a not defined private setter");
> 
> "Trying to access an undefined private setter"

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2413
>> +            RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());
> 
> Ditto. Please check the same things too.

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2424
>> +        RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());
> 
> Ditto.

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2435
>> +            generator.emitThrowTypeError("Trying to access a not defined private getter");
> 
> "Trying to access an undefined private getter"

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2453
>> +        generator.emitThrowTypeError("Trying to access a not defined private getter");
> 
> Ditto.

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2702
>> +            RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());
> 
> Ditto.

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2706
>> +            generator.emitThrowTypeError("Trying to access a not defined private setter");
> 
> Ditto.

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2713
>> +        RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());
> 
> Ditto.

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2723
>> +            generator.emitThrowTypeError("Trying to access a not defined private getter");
> 
> Ditto.

Done.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2741
>> +        generator.emitThrowTypeError("Trying to access a not defined private getter");
> 
> Ditto.

Done.

>> Source/JavaScriptCore/parser/VariableEnvironment.cpp:129
>> +        if (currentEntry.isSetter() || currentEntry.isMethod() || !currentEntry.isGetter())
> 
> Is `currentEntry.isSetter() || currentEntry.isMethod()` necessary?

No, removed.

>> Source/JavaScriptCore/parser/VariableEnvironment.cpp:173
>> +    }
> 
> Looks like declarePrivateGetter is largely duplicate to declarePrivateSetter. Could you dedupe the code?

Done.

>> Source/JavaScriptCore/parser/VariableEnvironment.cpp:177
>> +        if (currentEntry.isGetter() || currentEntry.isMethod() || !currentEntry.isSetter())
> 
> Is `currentEntry.isGetter() || currentEntry.isMethod()` necessary?

Removed

[Caio Lima]

Created attachment 420350 [details]
Patch

[EWS]

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

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

[Radar WebKit Bug Importer]

<rdar://problem/74366369>