Class fields, part 3 (backends)

Class fields, part 3 (backends)

This is the final part of a WIP implementation of the stage-2 proposal
to add fields to classes:
https://github.com/tc39/proposal-class-public-fields

See design doc:
https://docs.google.com/document/d/1WRtNm3ZLNJT1WVr8aq4RJuByYgfuAFAhj20LwTW6JVE/

The feature is available behind --harmony-class-fields.

This patch adds support for the feature to all backends except
Crankshaft, which does not implement classes. The backend changes
required are fairly small and identical across backends. It also adds
tests, since with this patch the feature is functional.

There are three things backends do in this patch:
- assign the class being constructed to a hidden variable, so that
  static field initializers can have the correct home object and receiver
  (which is necessary if the static field initializers are called while
  the class is being created, instead of immediately after)
- evaluate the (potentially computed) names of non-static fields and
  store those names in variables, to be referenced when instantiating the
  class
- evaluate the initializers of static fields and define the corresponding
  property on the class to hold the resulting value.

BUG=v8:5367

[bakkot, 2016-09-09 17:20:48]

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

[bakkot, 2016-09-09 17:21:33]

Description was changed from

==========
Class fields, part 3 (backends)

This is the final part of a WIP implementation of the stage-2 proposal
to add fields to classes:
https://github.com/tc39/proposal-class-public-fields

This adds support for the feature to all backends except Cranckshaft,
which does not implement classes. The backend changes required are
fairly small and identical across backends.


BUG=v8:5367
==========

to

==========
Class fields, part 3 (backends)

This is the final part of a WIP implementation of the stage-2 proposal
to add fields to classes:
https://github.com/tc39/proposal-class-public-fields

This adds support for the feature to all backends except Cranckshaft,
which does not implement classes. The backend changes required are
fairly small and identical across backends. It also adds tests, since
with this patch the feature is functional.


BUG=v8:5367
==========

[bakkot, 2016-09-09 17:22:19]

PTAL, after previous two patches are done (or concurrently).

[Dan Ehrenberg, 2016-09-10 04:06:54]

The mechanism here and the code implementing it seems reasonable to me, but
better comments could go a long way to making this maintainable. It'd also be
nice if you could give an overview of the three things that go into the backend
in the commit message, and why they're there (namely, the ordering of the
construction of the object and how that impacts binding, evaluating the computed
property names and static field values).

https://codereview.chromium.org/2330473002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2330473002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1498: // to me)
Seems OK; it'd be good to note that this is motivated by semantics under
discussion at https://github.com/tc39/proposal-class-public-fields/issues/50 .
From off-line discussion, sounds like you have reasons for structuring it this
way, so maybe reword the comment to explain what it's doing exactly (rather than
"shouldn't be necessary") and how it ties into the parser. It'd also help if you
wrote out the desugaring in pseudo-JS. The feedback vector seems to be used
analogously to elsewhere, so no need to comment on that.

https://codereview.chromium.org/2330473002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1503: VisitVariableAssignment(variable,
Token::INIT,  // TODO(bakkot) tokens
What is this TODO?

https://codereview.chromium.org/2330473002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1563: // This just does the ToName call on
the field name and stores it in a
Was Melissa O'Neill at Mudd when you went there? One thing that she told me that
really stuck was to never write just 'this', always write 'this <something>',
otherwise what you're saying becomes unintelligible.

It'd be much clearer to say something like, "the parser overloads the value of
this property with a variable which is used to store the possibly-computed name
of this property; desugar the result to assign the resulting key to the variable
for later access by the generated initializer function". If you could also write
out what the desugaring is in pseudo-JS, that would help.

[bakkot, 2016-09-12 20:45:06]

Description was changed from

==========
Class fields, part 3 (backends)

This is the final part of a WIP implementation of the stage-2 proposal
to add fields to classes:
https://github.com/tc39/proposal-class-public-fields

This adds support for the feature to all backends except Cranckshaft,
which does not implement classes. The backend changes required are
fairly small and identical across backends. It also adds tests, since
with this patch the feature is functional.


BUG=v8:5367
==========

to

==========
Class fields, part 3 (backends)

This is the final part of a WIP implementation of the stage-2 proposal
to add fields to classes:
https://github.com/tc39/proposal-class-public-fields

This adds support for the feature to all backends except Crankshaft,
which does not implement classes. The backend changes required are
fairly small and identical across backends. It also adds tests, since
with this patch the feature is functional.


BUG=v8:5367
==========

[bakkot, 2016-09-12 23:04:11]

Description was changed from

==========
Class fields, part 3 (backends)

This is the final part of a WIP implementation of the stage-2 proposal
to add fields to classes:
https://github.com/tc39/proposal-class-public-fields

This adds support for the feature to all backends except Crankshaft,
which does not implement classes. The backend changes required are
fairly small and identical across backends. It also adds tests, since
with this patch the feature is functional.


BUG=v8:5367
==========

to

==========
Class fields, part 3 (backends)

This is the final part of a WIP implementation of the stage-2 proposal
to add fields to classes:
https://github.com/tc39/proposal-class-public-fields

This adds support for the feature to all backends except Crankshaft,
which does not implement classes. The backend changes required are
fairly small and identical across backends. It also adds tests, since
with this patch the feature is functional.

There are three things backends do in this patch:
- assign the class being constructed to a hidden variable, so that
  static field initializers can have the correct home object and receiver
  (which is necessary if the static field initializers are called while
  the class is being created, instead of immediately after)
- evaluate the (potentially computed) names of non-static fields and
  store those names in variables, to be referenced when instantiating the
  class
- evaluate the initializers of static fields and define the corresponding
  property on the class to hold the resulting value.

BUG=v8:5367
==========

[bakkot, 2016-09-12 23:05:15]

> It'd also be nice if you could give an overview of the three things that go
into the backend in the commit message, and why they're there (namely, the
ordering of the construction of the object and how that impacts binding,
evaluating the computed property names and static field values).

Edited the commit message to be more informative.

https://codereview.chromium.org/2330473002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2330473002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1498: // to me)
On 2016/09/10 at 04:06:54, Dan Ehrenberg wrote:
> Seems OK; it'd be good to note that this is motivated by semantics under
discussion at https://github.com/tc39/proposal-class-public-fields/issues/50 .
From off-line discussion, sounds like you have reasons for structuring it this
way, so maybe reword the comment to explain what it's doing exactly (rather than
"shouldn't be necessary") and how it ties into the parser. It'd also help if you
wrote out the desugaring in pseudo-JS. The feedback vector seems to be used
analogously to elsewhere, so no need to comment on that.

Added a more reasonable comment mentioning that this is only necessary to
implement a particular semantics which are not what is currently specified.

https://codereview.chromium.org/2330473002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1503: VisitVariableAssignment(variable,
Token::INIT,  // TODO(bakkot) tokens
On 2016/09/10 at 04:06:54, Dan Ehrenberg wrote:
> What is this TODO?

Oh, I just wasn't sure if Token::INIT or Token::ASSIGN was proper here. Looking
more into the code, it doesn't appear to matter.

https://codereview.chromium.org/2330473002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1563: // This just does the ToName call on
the field name and stores it in a
On 2016/09/10 at 04:06:54, Dan Ehrenberg wrote:
> Was Melissa O'Neill at Mudd when you went there? One thing that she told me
that really stuck was to never write just 'this', always write 'this
<something>', otherwise what you're saying becomes unintelligible.
> 
> It'd be much clearer to say something like, "the parser overloads the value of
this property with a variable which is used to store the possibly-computed name
of this property; desugar the result to assign the resulting key to the variable
for later access by the generated initializer function". If you could also write
out what the desugaring is in pseudo-JS, that would help.

I've written a more reasonable if slightly wordy comment here also.

[Dan Ehrenberg, 2016-09-13 18:25:12]

lgtm

[bakkot, 2016-09-15 21:17:12]

Description was changed from

==========
Class fields, part 3 (backends)

This is the final part of a WIP implementation of the stage-2 proposal
to add fields to classes:
https://github.com/tc39/proposal-class-public-fields

This adds support for the feature to all backends except Crankshaft,
which does not implement classes. The backend changes required are
fairly small and identical across backends. It also adds tests, since
with this patch the feature is functional.

There are three things backends do in this patch:
- assign the class being constructed to a hidden variable, so that
  static field initializers can have the correct home object and receiver
  (which is necessary if the static field initializers are called while
  the class is being created, instead of immediately after)
- evaluate the (potentially computed) names of non-static fields and
  store those names in variables, to be referenced when instantiating the
  class
- evaluate the initializers of static fields and define the corresponding
  property on the class to hold the resulting value.

BUG=v8:5367
==========

to

==========
Class fields, part 3 (backends)

This is the final part of a WIP implementation of the stage-2 proposal
to add fields to classes:
https://github.com/tc39/proposal-class-public-fields

See design doc:
https://docs.google.com/document/d/1WRtNm3ZLNJT1WVr8aq4RJuByYgfuAFAhj20LwTW6JVE/

This adds support for the feature to all backends except Crankshaft,
which does not implement classes. The backend changes required are
fairly small and identical across backends. It also adds tests, since
with this patch the feature is functional.

There are three things backends do in this patch:
- assign the class being constructed to a hidden variable, so that
  static field initializers can have the correct home object and receiver
  (which is necessary if the static field initializers are called while
  the class is being created, instead of immediately after)
- evaluate the (potentially computed) names of non-static fields and
  store those names in variables, to be referenced when instantiating the
  class
- evaluate the initializers of static fields and define the corresponding
  property on the class to hold the resulting value.

BUG=v8:5367
==========

[bakkot, 2016-09-15 21:33:27]

Description was changed from

==========
Class fields, part 3 (backends)

This is the final part of a WIP implementation of the stage-2 proposal
to add fields to classes:
https://github.com/tc39/proposal-class-public-fields

See design doc:
https://docs.google.com/document/d/1WRtNm3ZLNJT1WVr8aq4RJuByYgfuAFAhj20LwTW6JVE/

This adds support for the feature to all backends except Crankshaft,
which does not implement classes. The backend changes required are
fairly small and identical across backends. It also adds tests, since
with this patch the feature is functional.

There are three things backends do in this patch:
- assign the class being constructed to a hidden variable, so that
  static field initializers can have the correct home object and receiver
  (which is necessary if the static field initializers are called while
  the class is being created, instead of immediately after)
- evaluate the (potentially computed) names of non-static fields and
  store those names in variables, to be referenced when instantiating the
  class
- evaluate the initializers of static fields and define the corresponding
  property on the class to hold the resulting value.

BUG=v8:5367
==========

to

==========
Class fields, part 3 (backends)

This is the final part of a WIP implementation of the stage-2 proposal
to add fields to classes:
https://github.com/tc39/proposal-class-public-fields

See design doc:
https://docs.google.com/document/d/1WRtNm3ZLNJT1WVr8aq4RJuByYgfuAFAhj20LwTW6JVE/

The feature is available behind --harmony-class-fields.

This adds support for the feature to all backends except Crankshaft,
which does not implement classes. The backend changes required are
fairly small and identical across backends. It also adds tests, since
with this patch the feature is functional.

There are three things backends do in this patch:
- assign the class being constructed to a hidden variable, so that
  static field initializers can have the correct home object and receiver
  (which is necessary if the static field initializers are called while
  the class is being created, instead of immediately after)
- evaluate the (potentially computed) names of non-static fields and
  store those names in variables, to be referenced when instantiating the
  class
- evaluate the initializers of static fields and define the corresponding
  property on the class to hold the resulting value.

BUG=v8:5367
==========

[bakkot, 2016-09-15 21:34:04]

Description was changed from

==========
Class fields, part 3 (backends)

This is the final part of a WIP implementation of the stage-2 proposal
to add fields to classes:
https://github.com/tc39/proposal-class-public-fields

See design doc:
https://docs.google.com/document/d/1WRtNm3ZLNJT1WVr8aq4RJuByYgfuAFAhj20LwTW6JVE/

The feature is available behind --harmony-class-fields.

This adds support for the feature to all backends except Crankshaft,
which does not implement classes. The backend changes required are
fairly small and identical across backends. It also adds tests, since
with this patch the feature is functional.

There are three things backends do in this patch:
- assign the class being constructed to a hidden variable, so that
  static field initializers can have the correct home object and receiver
  (which is necessary if the static field initializers are called while
  the class is being created, instead of immediately after)
- evaluate the (potentially computed) names of non-static fields and
  store those names in variables, to be referenced when instantiating the
  class
- evaluate the initializers of static fields and define the corresponding
  property on the class to hold the resulting value.

BUG=v8:5367
==========

to

==========
Class fields, part 3 (backends)

This is the final part of a WIP implementation of the stage-2 proposal
to add fields to classes:
https://github.com/tc39/proposal-class-public-fields

See design doc:
https://docs.google.com/document/d/1WRtNm3ZLNJT1WVr8aq4RJuByYgfuAFAhj20LwTW6JVE/

The feature is available behind --harmony-class-fields.

This patch adds support for the feature to all backends except
Crankshaft, which does not implement classes. The backend changes
required are fairly small and identical across backends. It also adds
tests, since with this patch the feature is functional.

There are three things backends do in this patch:
- assign the class being constructed to a hidden variable, so that
  static field initializers can have the correct home object and receiver
  (which is necessary if the static field initializers are called while
  the class is being created, instead of immediately after)
- evaluate the (potentially computed) names of non-static fields and
  store those names in variables, to be referenced when instantiating the
  class
- evaluate the initializers of static fields and define the corresponding
  property on the class to hold the resulting value.

BUG=v8:5367
==========

[bakkot, 2016-09-15 22:31:28]

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

[commit-bot: I haz the power, 2016-09-15 22:31:33]

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

[bakkot, 2016-09-15 22:31:36]

The CQ bit was unchecked by bakkot@google.com

[bakkot, 2016-09-15 22:46:29]

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

[commit-bot: I haz the power, 2016-09-15 22:46:34]

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 22:48:47]

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

[commit-bot: I haz the power, 2016-09-15 22:48:48]

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_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12867)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[bakkot, 2016-09-15 22:49:16]

The CQ bit was unchecked by bakkot@google.com

[bakkot, 2016-09-15 23:19:31]

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

[commit-bot: I haz the power, 2016-09-15 23:19:40]

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

[bakkot, 2016-09-15 23:21:18]

The CQ bit was unchecked by bakkot@google.com

[bakkot, 2016-09-15 23:23:19]

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

[commit-bot: I haz the power, 2016-09-15 23:23:27]

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 23:37:21]

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

[commit-bot: I haz the power, 2016-09-15 23:37:22]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[bakkot, 2016-09-16 00:12:29]

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

[commit-bot: I haz the power, 2016-09-16 00:12:43]

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-16 01:21:08]

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

[commit-bot: I haz the power, 2016-09-16 01:21:09]

Dry run: This issue passed the CQ dry run.

[bakkot, 2016-09-16 01:22:13]

The CQ bit was checked by bakkot@google.com

[bakkot, 2016-09-16 01:22:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2330473002/#ps120001
(title: "arguments")

[commit-bot: I haz the power, 2016-09-16 01:22:18]

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-16 01:26:27]

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

[commit-bot: I haz the power, 2016-09-16 01:26:28]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/24225)

[Dan Ehrenberg, 2016-09-16 01:31:00]

littledan@chromium.org changed reviewers:
+ beneder@chromium.org

[Dan Ehrenberg, 2016-09-16 01:31:13]

Ping Benedikt, could you do a backend review?

[rmcilroy, 2016-09-16 09:33:34]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[rmcilroy, 2016-09-16 09:33:36]

https://codereview.chromium.org/2330473002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2330473002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1584: continue;
Drive-by - could you add an example of class fields to test-bytecode-generator
so that we have expected bytecode for these patterns.

[bakkot, 2016-09-16 18:59:53]

https://codereview.chromium.org/2330473002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2330473002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1584: continue;
On 2016/09/16 at 09:33:35, rmcilroy wrote:
> Drive-by - could you add an example of class fields to test-bytecode-generator
so that we have expected bytecode for these patterns.

Done.

[rmcilroy, 2016-09-19 10:06:08]

https://codereview.chromium.org/2330473002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2330473002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1584: continue;
On 2016/09/16 18:59:53, bakkot wrote:
> On 2016/09/16 at 09:33:35, rmcilroy wrote:
> > Drive-by - could you add an example of class fields to
test-bytecode-generator
> so that we have expected bytecode for these patterns.
> 
> Done.

Thanks!

[Benedikt Meurer, 2016-09-20 03:20:37]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org, mstarzinger@chromium.org

[Benedikt Meurer, 2016-09-20 03:20:38]

bmeurer@chromium.org changed required reviewers:
+ mstarzinger@chromium.org

[Benedikt Meurer, 2016-09-20 03:20:38]

LGTM (although I'm not an expert here). Adding Michi as reviewer for graph
builder changes.

[Michael Starzinger, 2016-09-20 08:50:56]

LGTM. Thanks!

[Dan Ehrenberg, 2016-09-20 15:38:35]

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

[commit-bot: I haz the power, 2016-09-20 15:38:48]

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-20 15:52:18]

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

[commit-bot: I haz the power, 2016-09-20 15:52:19]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)