Bug 194434 - [ESNext] Implement private methods
Summary: [ESNext] Implement private methods
Status: NEW
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:
Depends on: 174212 213372
Blocks: 194435
  Show dependency treegraph
 
Reported: 2019-02-08 05:43 PST by Caio Lima
Modified: 2020-10-22 13:34 PDT (History)
12 users (show)

See Also:


Attachments
WIP - Patch (80.57 KB, patch)
2019-05-27 08:55 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (122.68 KB, patch)
2019-06-27 13:48 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (115.62 KB, patch)
2019-10-14 00:24 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
WIP - Patch (116.88 KB, patch)
2020-02-13 13:12 PST, Caio Lima
no flags Details | Formatted Diff | Diff
WIP - Patch (116.66 KB, patch)
2020-03-13 06:09 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
WIP - Patch (120.48 KB, patch)
2020-09-07 12:11 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (124.07 KB, patch)
2020-10-20 12:06 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (124.05 KB, patch)
2020-10-21 06:15 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (141.62 KB, patch)
2020-10-22 13:30 PDT, Caio Lima
ticaiolima: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2019-02-08 05:43:24 PST
The proposal https://tc39.github.io/proposal-private-methods/ is on Stage 3.
Comment 1 Caio Lima 2019-05-27 08:55:23 PDT
Created attachment 370694 [details]
WIP - Patch

This patch is dependent on changes from https://bugs.webkit.org/show_bug.cgi?id=174212.
Comment 2 Caio Lima 2019-06-27 13:48:10 PDT
Created attachment 373050 [details]
Patch

This Patch does not apply to tree, since it is a diff from patch on https://bugs.webkit.org/show_bug.cgi?id=174212.
Comment 3 Caio Lima 2019-10-14 00:24:27 PDT
Created attachment 380859 [details]
Patch

This patch is the diff of private methods on top of class fields patch.
Comment 4 Ross Kirsling 2019-10-15 05:57:35 PDT
Comment on attachment 380859 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3027
> +    if (!m_privateNamesStack.size())
> +        return;

Could this be an assertion?

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:-304
> -

You could probably drop this file if it's just whitespace changes.

> Source/JavaScriptCore/runtime/JSSymbolTableObject.h:45
> -    
> +

Ditto.
Comment 5 Caio Lima 2020-02-13 13:12:15 PST
Created attachment 390683 [details]
WIP - Patch

This patch is the diff on top of current private fields patch on https://bugs.webkit.org/show_bug.cgi?id=206431
Comment 6 Caio Lima 2020-03-13 06:09:32 PDT
Created attachment 393477 [details]
WIP - Patch
Comment 7 Caio Lima 2020-09-07 12:11:50 PDT
Created attachment 408191 [details]
WIP - Patch
Comment 8 Caio Lima 2020-10-20 12:06:17 PDT
Created attachment 411896 [details]
Patch
Comment 9 Caio Lima 2020-10-21 06:15:16 PDT
Created attachment 411978 [details]
Patch
Comment 10 Caitlin Potter (:caitp) 2020-10-21 12:56:35 PDT
Comment on attachment 411978 [details]
Patch

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

I think it might be worth adding some tests which try to tier up the brand check and still are able to throw via OSRExit or generic slow path, to be sure that it's ok that the result of GetPrivateName doesn't get used

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2731
> +RegisterID* BytecodeGenerator::emitGetPrivateName(RegisterID* dst, RegisterID* base, RegisterID* property)

Oops 😬, nice

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2802
> +    // This is going to trhow TypeError if brand is not present

nit: throw

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2803
> +    emitGetPrivateName(newTemporary(), base, brandSymbol);

Not really actionable right now, but just thinking: In cases where there are multiple instances of the same class, we will throw on the GetFromScope before here. It will be a better error in some ways (it will show the field name), but worse in other ways (won't explain what it's trying to do).

Also, I'm a bit worried that the result of the GetPrivateName is not going to be USED anywhere, and we might decide to throw it out if the operation isn't considered effectful.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2960
> +    if (!m_privateNamesStack.size())

nit: Right now, the only caller is guarding on `hasPrivateNames`. Does it make sense to just ASSERT() this and make sure it's only called in places it makes sense? (ditto above)

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:903
> +        if (generator.isPrivateMethod(identifier())) {

Maybe it would make sense to rename BaseDotNode::isPrivateField() to make this clearer, because it seems weird that `isPrivateField()` and `isPrivateMethod()` can both be true

> Source/JavaScriptCore/parser/VariableEnvironment.h:104
> +    bool isPrivateAccess() const { return isMethod(); }

I find the name is a bit confusing --- Can we call this `is[Private]MethodOrAccessor` instead?

"private access" reads like it applies to any private member access, to me.
Comment 11 Caitlin Potter (:caitp) 2020-10-21 14:47:08 PDT
Comment on attachment 411978 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2803
>> +    emitGetPrivateName(newTemporary(), base, brandSymbol);
> 
> Not really actionable right now, but just thinking: In cases where there are multiple instances of the same class, we will throw on the GetFromScope before here. It will be a better error in some ways (it will show the field name), but worse in other ways (won't explain what it's trying to do).
> 
> Also, I'm a bit worried that the result of the GetPrivateName is not going to be USED anywhere, and we might decide to throw it out if the operation isn't considered effectful.

This turns out to be wrong, I was thinking `@privateBrandName` would be different for each instance of the class, but it looks like that's not the case.
Comment 12 Caio Lima 2020-10-22 13:30:14 PDT
Created attachment 412125 [details]
Patch
Comment 13 Caio Lima 2020-10-22 13:34:29 PDT
(In reply to Caitlin Potter (:caitp) from comment #11)
> Comment on attachment 411978 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411978&action=review
> 
> >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2803
> >> +    emitGetPrivateName(newTemporary(), base, brandSymbol);
> > 
> > Not really actionable right now, but just thinking: In cases where there are multiple instances of the same class, we will throw on the GetFromScope before here. It will be a better error in some ways (it will show the field name), but worse in other ways (won't explain what it's trying to do).
> > 
> > Also, I'm a bit worried that the result of the GetPrivateName is not going to be USED anywhere, and we might decide to throw it out if the operation isn't considered effectful.
> 
> This turns out to be wrong, I was thinking `@privateBrandName` would be
> different for each instance of the class, but it looks like that's not the
> case.

It is different for each evaluation of a class. For instances of the same class evaluation, it's the same. Also, `GetPrivateName` has `MustGenerate` flag, so it DCE won't be able to remove it.