Private fields

[language] Private fields

A WIP implementation of a stage-1 proposal to add private fields to JS classes.
The proposal is at https://github.com/tc39/proposal-private-fields

Not all errors are implemented, but the remaining semantics are.

See design doc for details:
https://docs.google.com/document/d/1WRtNm3ZLNJT1WVr8aq4RJuByYgfuAFAhj20LwTW6JVE/edit?ts=57db03c2#

The feature is staged behind --harmony-private-class-fields

The implementation is purely in the parser plus a single runtime function.
Code like `this.#x` is desugared into approximately `this[#x]`,
where `#x` is the name of a variable in the class's scope which holds a private symbol. Initialization is performed via the same mechanism as public fields.

BUG=v8:5368

[bakkot, 2016-09-10 00:25:37]

bakkot@google.com changed reviewers:
+ adamk@chromium.org, littledan@chromium.org

[bakkot, 2016-09-10 00:25:39]

PTAL after class fields. This patch still needs parser tests, but should
otherwise be approximately ready to go.

[Dan Ehrenberg, 2016-09-10 04:30:29]

A good start.

Does this make sure to throw if the same private field is added to an object
twice? I don't see that logic anywhere.

https://codereview.chromium.org/2329703002/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2329703002/diff/1/src/flag-definitions.h#newc...
src/flag-definitions.h:207: V(harmony_private_class_fields, "harmony private
fields in class literals")
Looks like this patch assumes that, if --harmony-private-class-fields is on,
then --harmony-class-fields is on. To make ClusterFuzz happy, we should set this
up as an explicit dependency, otherwise it'll test one without the other. You
can do this with DEFINE_IMPLICATION.

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1737: BindingPatternUnexpectedToken();
This check makes sense, and reminds me: When you write parser tests, be sure to
have a test that verifies that you can have a private field as part of an
assignment pattern.

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2075: false, *is_computed_name);
Don't you want to pass true for is_computed_name? Maybe you get by anyway
because fields are always treated like computed names.

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3372: raw_name->length());  // TODO(bakkot) check
complicated names
What's the issue here? Non-latin1 names?

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3373: name = "#" + name;
We don't concatenate strings in the parser like this generally.

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3376: // doesn't matter
Yeah, you can in fact have identifier characters which don't fit in latin1 in JS

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3397: return factory()->NewProperty(call, prop, pos);
Great logic here, ends up being very elegant, but long-term I think we want
ThrowIfMissingPrivateField to be either part of the load/store IC (a flag passed
into that), or, it looks like we could pretty easily make a code stub for this
runtime function (based on CodeStubAssembler::TryHasOwnProperty). Include a TODO
explaining the need for optimization here.

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4495: Variable::NORMAL);  // TODO(bakkot) flags, Declare
vs DeclareLocal
I'm confused, where's the actual declaration AST node made? This creates a
rather strange AST, I think. I think Declare is what you want generally.

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4592: for (int i = 0; i <
private_field_symbol_vars->length(); ++i) {
A comment explaining the desugaring you're doing?

https://codereview.chromium.org/2329703002/diff/1/src/runtime/runtime-classes.cc
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/2329703002/diff/1/src/runtime/runtime-classes...
src/runtime/runtime-classes.cc:86: // proxy trap
Proxy traps aren't triggered on private symbol lookups. You could make a DCHECK
failure to ensure that there's no failure related to this. Probably it's worth
adding a test with the super return trick adding private state to a Proxy,
ensuring that it works and also no Proxy traps are hit.

https://codereview.chromium.org/2329703002/diff/1/test/mjsunit/harmony/class-...
File test/mjsunit/harmony/class-privates-delete.js (right):

https://codereview.chromium.org/2329703002/diff/1/test/mjsunit/harmony/class-...
test/mjsunit/harmony/class-privates-delete.js:11: delete #a; // TODO(bakkot)
this should be a syntax error
Yes it should

[bakkot, 2016-09-13 01:05:38]

Addressed some of the comments; I'll address the rest when I add more tests.

https://codereview.chromium.org/2329703002/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2329703002/diff/1/src/flag-definitions.h#newc...
src/flag-definitions.h:207: V(harmony_private_class_fields, "harmony private
fields in class literals")
On 2016/09/10 at 04:30:28, Dan Ehrenberg wrote:
> Looks like this patch assumes that, if --harmony-private-class-fields is on,
then --harmony-class-fields is on. To make ClusterFuzz happy, we should set this
up as an explicit dependency, otherwise it'll test one without the other. You
can do this with DEFINE_IMPLICATION.

It doesn't; they can be used independently. All of the places where both patches
need it check whether either is on. Most of the tests only have
--harmony-private-class-fields, though some have both.

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2075: false, *is_computed_name);
On 2016/09/10 at 04:30:28, Dan Ehrenberg wrote:
> Don't you want to pass true for is_computed_name? Maybe you get by anyway
because fields are always treated like computed names.

It really doesn't matter, yeah. I don't believe property->is_computed_name()
ever gets read for private fields. I've set it to false explicitly to avoid the
pointer dereference, though.

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3372: raw_name->length());  // TODO(bakkot) check
complicated names
On 2016/09/10 at 04:30:28, Dan Ehrenberg wrote:
> What's the issue here? Non-latin1 names?

Non-one-byte names, in particular, although it looks like this particular line
is ok.

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3373: name = "#" + name;
On 2016/09/10 at 04:30:28, Dan Ehrenberg wrote:
> We don't concatenate strings in the parser like this generally.

I'm not sure it can be avoided; we need an AstRawString here.

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3376: // doesn't matter
On 2016/09/10 at 04:30:28, Dan Ehrenberg wrote:
> Yeah, you can in fact have identifier characters which don't fit in latin1 in
JS

The reason I think it might not matter is that the variables using these names
are totally synthetic. It's valuable to have the name match the field name only
if the name will be visible somewhere; currently it is, because of the
ReferenceError thrown at runtime when referring to an unbound name, but that can
change.

If I understand correctly, this will convert two-byte names to one-byte, which
will mangle the name in a deterministic way, and it'll function correctly. But
this may not be the case. If not, I'm not sure how to deal with this.

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3397: return factory()->NewProperty(call, prop, pos);
On 2016/09/10 at 04:30:28, Dan Ehrenberg wrote:
> Great logic here, ends up being very elegant, but long-term I think we want
ThrowIfMissingPrivateField to be either part of the load/store IC (a flag passed
into that), or, it looks like we could pretty easily make a code stub for this
runtime function (based on CodeStubAssembler::TryHasOwnProperty). Include a TODO
explaining the need for optimization here.

Added a TODO.

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4495: Variable::NORMAL);  // TODO(bakkot) flags, Declare
vs DeclareLocal
On 2016/09/10 at 04:30:28, Dan Ehrenberg wrote:
> I'm confused, where's the actual declaration AST node made? This creates a
rather strange AST, I think. I think Declare is what you want generally.

Switched to that. This has the side effect of making duplicate property name
declaration an early SyntaxError: `class { #a; #a; }` produces "SyntaxError:
Identifier '#a' has already been declared". This isn't quite per spec, but
sounds like it probably should be:
https://github.com/tc39/proposal-private-fields/issues/48

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4592: for (int i = 0; i <
private_field_symbol_vars->length(); ++i) {
On 2016/09/10 at 04:30:28, Dan Ehrenberg wrote:
> A comment explaining the desugaring you're doing?

Done.

[bakkot, 2016-09-13 22:05:04]

> Does this make sure to throw if the same private field is added to an object
twice? I don't see that logic anywhere.

No, it actually allows that. Added a test that it's allowed and a TODO to fix
it. I wouldn't be surprised if that changes depending on the outcome of the
discussion of the similar question for public fields, though.

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2329703002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1737: BindingPatternUnexpectedToken();
On 2016/09/10 at 04:30:28, Dan Ehrenberg wrote:
> This check makes sense, and reminds me: When you write parser tests, be sure
to have a test that verifies that you can have a private field as part of an
assignment pattern.

Done.

https://codereview.chromium.org/2329703002/diff/1/src/runtime/runtime-classes.cc
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/2329703002/diff/1/src/runtime/runtime-classes...
src/runtime/runtime-classes.cc:86: // proxy trap
On 2016/09/10 at 04:30:28, Dan Ehrenberg wrote:
> Proxy traps aren't triggered on private symbol lookups. You could make a
DCHECK failure to ensure that there's no failure related to this. Probably it's
worth adding a test with the super return trick adding private state to a Proxy,
ensuring that it works and also no Proxy traps are hit.

Added such a test.

[Dan Ehrenberg, 2016-09-15 20:53:55]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 20:54:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 20:55:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 20:55:50]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/24202)

[Dan Ehrenberg, 2016-09-15 21:26:58]

https://codereview.chromium.org/2329703002/diff/20001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2329703002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3376: // doesn't matter
I'm looking forward to your fix here.

https://codereview.chromium.org/2329703002/diff/40001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2329703002/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:2066: EmptyClassLiteralProperty));  // TODO(bakkot)
IdentifierPart
Actually I like using IdentifierName here; I think I'll change the spec. (This
disallows, e.g., #0).

https://codereview.chromium.org/2329703002/diff/40001/src/runtime/runtime-cla...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/2329703002/diff/40001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:78:
RUNTIME_FUNCTION(Runtime_ThrowIfMissingPrivateField) {
It'd be nice to have a comment somewhere explaining that this runtime function
needs to return the object because it may be used as an LHS

https://codereview.chromium.org/2329703002/diff/40001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:86: // proxy trap
TODO can be rephrased to say that it in fact doesn't.

Maybe add a DCHECK to ensure that the Name is a private symbol, though.

https://codereview.chromium.org/2329703002/diff/40001/src/runtime/runtime-obj...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2329703002/diff/40001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:679: RUNTIME_FUNCTION(Runtime_DefineDataProperty)
{
The change to DefineDataProperty, which works on Proxies, should be backported
to one of the earlier public properties patches. They can also end up running on
Proxies. It'd be nice to mention somewhere in this definition that the point of
splitting off from DefineDataPropertyInLiteral is to work on Proxies. Maybe,
rather than making a new runtime function, the prior one should just have that
small generalization applied to it; I bet it wouldn't be a big perf regression.

https://codereview.chromium.org/2329703002/diff/40001/test/mjsunit/harmony/cl...
File test/mjsunit/harmony/class-privates-errors.js (right):

https://codereview.chromium.org/2329703002/diff/40001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/class-privates-errors.js:16: }
This test is a little incomplete, as you don't actually instantiate Derived in
this test. This looks like a fun infinite loop to me; you may want to make a
base case so you can actually observe that the check currently doesn't occur.

https://codereview.chromium.org/2329703002/diff/40001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/class-privates-errors.js:35: //
https://github.com/tc39/proposal-private-fields/issues/49
I think the only ongoing discussion is whether it should be a syntax error to
list an outer enclosing one on an inner class. If it statically doesn't resolve
to anything at all, I think it should be a SyntaxError, and I haven't heard an
argument the other way.

[bakkot, 2016-09-17 00:11:11]

https://codereview.chromium.org/2329703002/diff/20001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2329703002/diff/20001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3376: // doesn't matter
On 2016/09/15 at 21:26:58, Dan Ehrenberg wrote:
> I'm looking forward to your fix here.

Done.

https://codereview.chromium.org/2329703002/diff/40001/src/runtime/runtime-cla...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/2329703002/diff/40001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:78:
RUNTIME_FUNCTION(Runtime_ThrowIfMissingPrivateField) {
On 2016/09/15 at 21:26:58, Dan Ehrenberg wrote:
> It'd be nice to have a comment somewhere explaining that this runtime function
needs to return the object because it may be used as an LHS

Done.

https://codereview.chromium.org/2329703002/diff/40001/src/runtime/runtime-cla...
src/runtime/runtime-classes.cc:86: // proxy trap
On 2016/09/15 at 21:26:58, Dan Ehrenberg wrote:
> TODO can be rephrased to say that it in fact doesn't.
> 
> Maybe add a DCHECK to ensure that the Name is a private symbol, though.

Done.

https://codereview.chromium.org/2329703002/diff/40001/src/runtime/runtime-obj...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2329703002/diff/40001/src/runtime/runtime-obj...
src/runtime/runtime-object.cc:679: RUNTIME_FUNCTION(Runtime_DefineDataProperty)
{
On 2016/09/15 at 21:26:58, Dan Ehrenberg wrote:
> The change to DefineDataProperty, which works on Proxies, should be backported
to one of the earlier public properties patches. They can also end up running on
Proxies. It'd be nice to mention somewhere in this definition that the point of
splitting off from DefineDataPropertyInLiteral is to work on Proxies. Maybe,
rather than making a new runtime function, the prior one should just have that
small generalization applied to it; I bet it wouldn't be a big perf regression.

It has been; it got copied over here by accident. I'm going to leave it as two
functions for the moment.

[bakkot, 2016-09-17 00:15:29]

Description was changed from

==========
Private fields

A WIP implementation of a stage-1 proposal.

BUG=v8:5368
==========

to

==========
Private fields

A WIP implementation of a stage-1 proposal.
The proposal is at https://github.com/tc39/proposal-private-fields

Not all errors are implemented, but the remaining semantics are.

See design doc for details:
https://docs.google.com/document/d/1WRtNm3ZLNJT1WVr8aq4RJuByYgfuAFAhj20LwTW6J...

The implementation is purely in the parser plus a single runtime function.
Code like `this.#x` is desugared into approximately `this[#x]`,
where `#x` is the name of a variable in the class's scope which holds a private
symbol. Initialization is performed via the same mechanism as public fields.

BUG=v8:5368
==========

[bakkot, 2016-09-17 00:16:34]

Description was changed from

==========
Private fields

A WIP implementation of a stage-1 proposal.
The proposal is at https://github.com/tc39/proposal-private-fields

Not all errors are implemented, but the remaining semantics are.

See design doc for details:
https://docs.google.com/document/d/1WRtNm3ZLNJT1WVr8aq4RJuByYgfuAFAhj20LwTW6J...

The implementation is purely in the parser plus a single runtime function.
Code like `this.#x` is desugared into approximately `this[#x]`,
where `#x` is the name of a variable in the class's scope which holds a private
symbol. Initialization is performed via the same mechanism as public fields.

BUG=v8:5368
==========

to

==========
[language] Private fields

A WIP implementation of a stage-1 proposal to add private fields to JS classes.
The proposal is at https://github.com/tc39/proposal-private-fields

Not all errors are implemented, but the remaining semantics are.

See design doc for details:
https://docs.google.com/document/d/1WRtNm3ZLNJT1WVr8aq4RJuByYgfuAFAhj20LwTW6J...

The feature is staged behind --harmony-private-class-fields

The implementation is purely in the parser plus a single runtime function.
Code like `this.#x` is desugared into approximately `this[#x]`,
where `#x` is the name of a variable in the class's scope which holds a private
symbol. Initialization is performed via the same mechanism as public fields.

BUG=v8:5368
==========

[bakkot, 2016-09-17 00:16:41]

The CQ bit was checked by bakkot@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-17 00:16:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-17 00:18:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-17 00:18:36]

Dry run: Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/9070)