Bug 177592 – [ESNext] Private methods in class: syntax

• Status: NEW
• Component: JavaScriptCore

[GSkachkov]

Following code should be parsed correctly
```
class Counter extends HTMLElement {
  #xValue = 0;

  get #x() { return #xValue; }
  set #x(value) {
    this.#xValue = value; 
    window.requestAnimationFrame(this.#render.bind(this));
  }

  #clicked() {
    this.#x++;
  }

  constructor() {
    super();
    this.onclick = this.#clicked.bind(this);
  }

  connectedCallback() { this.#render(); }

  #render() {
    this.textContent = this.#x.toString();
  }
}
window.customElements.define('num-counter', Counter);
```

[Yusuke Suzuki]

We should use PrivateSymbolImpl in WTF. This is prepared for this purpose ;)
And since they can just become Identifier, we should use get_by_id/put_by_id for these private fields. Then, we can just use existing mechanism.

[Saam Barati]

I think what’s tricky here is if you create the class twice, it should have a different #xValue Symbol. So we need a way of storing and loading the valid symbol. Therefore, we can’t simply use get_by_id for “this.#field”

[Yusuke Suzuki]

(In reply to Saam Barati from comment #2)
> I think what’s tricky here is if you create the class twice, it should have
> a different #xValue Symbol. So we need a way of storing and loading the
> valid symbol. Therefore, we can’t simply use get_by_id for “this.#field”

Oh, nice catch. So, rough implementation would be,

1. Storing FixedArray<Symbol> to Class function.
2. Adding a bytecode to load Symbol from class function
3. put get_by_val with this symbol
4. Symbol should be one wrapping PrivateSymbolImpl

[Saam Barati]

(In reply to Yusuke Suzuki from comment #3)
> (In reply to Saam Barati from comment #2)
> > I think what’s tricky here is if you create the class twice, it should have
> > a different #xValue Symbol. So we need a way of storing and loading the
> > valid symbol. Therefore, we can’t simply use get_by_id for “this.#field”
> 
> Oh, nice catch. So, rough implementation would be,
> 
> 1. Storing FixedArray<Symbol> to Class function.
> 2. Adding a bytecode to load Symbol from class function
Yeah, with the necessary type checks to ensure you aren't loading another class's symbols. We should find a way to constant fold the
get_by_val into GetById/GetByOffset if the callee is a constant inside the DFG.

> 3. put get_by_val with this symbol
> 4. Symbol should be one wrapping PrivateSymbolImpl

[GSkachkov]

(In reply to Saam Barati from comment #4)
> (In reply to Yusuke Suzuki from comment #3)
> > (In reply to Saam Barati from comment #2)
> > > I think what’s tricky here is if you create the class twice, it should have
> > > a different #xValue Symbol. So we need a way of storing and loading the
> > > valid symbol. Therefore, we can’t simply use get_by_id for “this.#field”
> > 
> > Oh, nice catch. So, rough implementation would be,
> > 
> > 1. Storing FixedArray<Symbol> to Class function.
> > 2. Adding a bytecode to load Symbol from class function
> Yeah, with the necessary type checks to ensure you aren't loading another
> class's symbols. We should find a way to constant fold the
> get_by_val into GetById/GetByOffset if the callee is a constant inside the
> DFG.
> 
> > 3. put get_by_val with this symbol
> > 4. Symbol should be one wrapping PrivateSymbolImpl
Thanks, will be useful soon :-) Still playing with syntax, 
will need to ask question to author of the spec.

[GSkachkov]

Created attachment 325085 [details]
Patch

WiP

[Build Bot]

Attachment 325085 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Lexer.h:374:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 325085 [details]
Patch

Attachment 325085 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5005695

New failing tests:
js/dom/invalid-syntax-for-function.html

[Build Bot]

Created attachment 325090 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 325085 [details]
Patch

Attachment 325085 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5005902

New failing tests:
js/dom/invalid-syntax-for-function.html

[Build Bot]

Created attachment 325095 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 325085 [details]
Patch

Attachment 325085 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5006551

New failing tests:
js/dom/invalid-syntax-for-function.html

[Build Bot]

Created attachment 325107 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Build Bot]

Comment on attachment 325085 [details]
Patch

Attachment 325085 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5006907

New failing tests:
js/dom/invalid-syntax-for-function.html

[Build Bot]

Created attachment 325109 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[GSkachkov]

Created attachment 325288 [details]
Patch

WiP. Correct  has to be used during access to the privateMethods/privateProperties

[GSkachkov]

WiP. Correct `this` has to be used during access any values within privateMethods

[Build Bot]

Attachment 325288 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:4832:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:4833:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/parser/Lexer.h:374:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 325288 [details]
Patch

Attachment 325288 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/5032835

New failing tests:
stress/class-private-methods.js.ftl-no-cjit-b3o1
stress/class-private-methods.js.no-cjit-collect-continuously
stress/class-private-methods.js.no-llint
stress/class-private-methods.js.dfg-maximal-flush-validate-no-cjit
stress/class-private-methods.js.no-cjit-validate-phases
stress/class-private-methods.js.ftl-eager-no-cjit-b3o1
stress/class-private-methods.js.ftl-eager
stress/class-private-methods.js.no-ftl
stress/class-private-methods.js.dfg-eager
stress/class-private-methods.js.dfg-eager-no-cjit-validate
stress/class-private-methods.js.ftl-no-cjit-no-inline-validate
stress/class-private-methods.js.ftl-no-cjit-no-put-stack-validate
stress/class-private-methods.js.default
stress/class-private-methods.js.ftl-no-cjit-small-pool
stress/class-private-methods.js.ftl-eager-no-cjit
stress/class-private-methods.js.ftl-no-cjit-validate-sampling-profiler