WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
194435
[ESNext] Implement private accessors
https://bugs.webkit.org/show_bug.cgi?id=194435
Summary
[ESNext] Implement private accessors
Caio Lima
Reported
2019-02-08 05:44:28 PST
Private accessors are specified on
https://tc39.github.io/proposal-private-methods/
.
Attachments
WIP - Patch
(39.46 KB, patch)
2019-05-27 10:00 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
WIP - Patch
(45.70 KB, patch)
2019-06-28 10:19 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
WIP - Patch
(44.91 KB, patch)
2020-02-27 07:23 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
WIP - Patch
(44.90 KB, patch)
2020-02-27 07:36 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(55.22 KB, patch)
2021-02-10 08:03 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(56.16 KB, patch)
2021-02-10 10:59 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(54.98 KB, patch)
2021-02-10 15:08 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(56.88 KB, patch)
2021-02-11 08:47 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
WIP Patch
(67.72 KB, patch)
2021-02-11 15:29 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(73.82 KB, patch)
2021-02-12 08:39 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(75.41 KB, patch)
2021-02-15 12:44 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Caio Lima
Comment 1
2019-05-27 10:00:12 PDT
Created
attachment 370695
[details]
WIP - Patch This Patch is dependent on
https://bugs.webkit.org/show_bug.cgi?id=194434
. WIP
Caio Lima
Comment 2
2019-06-28 10:19:15 PDT
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
Comment 3
2020-02-27 07:23:31 PST
Created
attachment 391861
[details]
WIP - Patch
Caio Lima
Comment 4
2020-02-27 07:36:41 PST
Created
attachment 391866
[details]
WIP - Patch
Caio Lima
Comment 5
2020-11-20 02:49:13 PST
***
Bug 219182
has been marked as a duplicate of this bug. ***
Caio Lima
Comment 6
2021-02-10 08:03:03 PST
Created
attachment 419845
[details]
Patch
Caio Lima
Comment 7
2021-02-10 10:59:29 PST
Created
attachment 419866
[details]
Patch
Caio Lima
Comment 8
2021-02-10 13:05:26 PST
Comment on
attachment 419866
[details]
Patch I've found a couple of bugs when using private identifiers instead of common identifiers.
Caio Lima
Comment 9
2021-02-10 15:08:21 PST
Created
attachment 419911
[details]
Patch
Caio Lima
Comment 10
2021-02-11 08:47:33 PST
Created
attachment 419994
[details]
Patch
Caio Lima
Comment 11
2021-02-11 13:46:00 PST
Comment on
attachment 419994
[details]
Patch r- because I've found another bug on postfix/prefix nodes.
Caio Lima
Comment 12
2021-02-11 15:29:05 PST
Created
attachment 420052
[details]
WIP Patch
Caio Lima
Comment 13
2021-02-11 15:30:17 PST
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
Comment 14
2021-02-12 08:39:00 PST
Created
attachment 420127
[details]
Patch
Yusuke Suzuki
Comment 15
2021-02-14 01:45:40 PST
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 16
2021-02-15 12:42:55 PST
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
Comment 17
2021-02-15 12:44:00 PST
Created
attachment 420350
[details]
Patch
EWS
Comment 18
2021-02-15 14:40:31 PST
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
Comment 19
2021-02-15 14:41:14 PST
<
rdar://problem/74366369
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug