Class fields, part 2 (desugaring)

Class fields, part 2 (desugaring)

This is one 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 the desugaring logic to the parser. It isn't usable without
the (forthcoming) backend changes.

BUG=v8:5367
Committed: https://crrev.com/834aec33918ad645aba3b2662d6ceab056867427
Cr-Commit-Position: refs/heads/master@{#39460}

[bakkot, 2016-09-08 18:28:48]

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

[bakkot, 2016-09-08 20:32:34]

PTAL, after the previous patch. This one is where almost all the logic is.

[bakkot, 2016-09-08 22:37:20]

Description was changed from

==========
Class fields, part 2 (desugaring)

This is one 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 the desugaring logic to the parser. It isn't usable without
the (forthcoming) backend changes.
==========

to

==========
Class fields, part 2 (desugaring)

This is one 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 the desugaring logic to the parser. It isn't usable without
the (forthcoming) backend changes.

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

[Dan Ehrenberg, 2016-09-09 22:54:42]

First round of comments

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

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:154: Expression* this_expr) {
For desugarings like this, even simple ones, could you start with a comment
giving the AST that it will desugar into, based on the parameters?

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:165: Expression* Parser::RewriteSuperCall(Expression*
super_call) {
Please add a comment showing what this desugars into.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:166: if (!allow_harmony_class_fields()) {
Ideally we should also take this path if the class doesn't have any fields,
right? We can't somehow query require_class_field_init here?

If this is a potential future optimization that you don't want to do in this
patch, that's completely fine; maybe a good place for a TODO.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:171: // an AST node (at least, one containing a variable
proxy) in two places.
That's right, an AST node is single-use. You can store results in temporary
variables and use a ternary operator rather than an if statement, but that may
still take a do expression.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4286: ".class-field-" + std::to_string(index) + (is_name ?
"-name" : "-func");
Eh, I guess this works, and it's clean code, but typically in V8 we do this sort
of thing with heap-allocated strings, concatenating them with ConsStrings. Would
that work for you here?

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4290: FunctionLiteral*
Parser::SynthesizeClassFieldInitializer(int count) {
Add a comment showing what this desugars into

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4327: // function(){}; static y = function(){}; }
Function name inference sounds like a ship blocker to me. Are the specs good
about requiring the inference?

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4468: // necessary for non-computed names in full-codegen
What happens when you don't include this on a non-computed name? I'm surprised
by the need for this on non-computed names.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4481: // clear on the difference...
Generally in the parser, you can either use a NewVariableDeclaration/Declare
pair, or call DeclareVariable as a shortcut. DeclareLocal is lower-level, and
you'll need to set up other things if you want to use it directly.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4543:
ast_value_factory()->GetOneByteString(".class-field-initializer");
Put in src/heap-symbols.h

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4546: Variable::NORMAL);  // TODO(bakkot) flags
What flags do you think might be wrong here? This seems fine.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4568: scope());  // TODO(bakkot) declare vs declarelocal
Declare.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4579: // especially in trivial cases
As we discussed off-line, you can use the scope information to determine if
super is called, either in this patch or a follow-on.

[bakkot, 2016-09-12 19:10:11]

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

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:154: Expression* this_expr) {
On 2016/09/09 at 22:54:42, Dan Ehrenberg wrote:
> For desugarings like this, even simple ones, could you start with a comment
giving the AST that it will desugar into, based on the parameters?

Done.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:165: Expression* Parser::RewriteSuperCall(Expression*
super_call) {
On 2016/09/09 at 22:54:41, Dan Ehrenberg wrote:
> Please add a comment showing what this desugars into.

Done.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:166: if (!allow_harmony_class_fields()) {
On 2016/09/09 at 22:54:41, Dan Ehrenberg wrote:
> Ideally we should also take this path if the class doesn't have any fields,
right? We can't somehow query require_class_field_init here?
> 
> If this is a potential future optimization that you don't want to do in this
patch, that's completely fine; maybe a good place for a TODO.

Unfortunately not. The super call might occur before we've seen any class
fields. Added a TODO, though.

A backend could do it by checking if .class-field-initializer is present, but
that's what the emitted code does anyway.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:171: // an AST node (at least, one containing a variable
proxy) in two places.
On 2016/09/09 at 22:54:41, Dan Ehrenberg wrote:
> That's right, an AST node is single-use. You can store results in temporary
variables and use a ternary operator rather than an if statement, but that may
still take a do expression.

At that point the if statement is more efficient anyway, given how we represent
do expressions, so that's what I'm doing.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4286: ".class-field-" + std::to_string(index) + (is_name ?
"-name" : "-func");
On 2016/09/09 at 22:54:41, Dan Ehrenberg wrote:
> Eh, I guess this works, and it's clean code, but typically in V8 we do this
sort of thing with heap-allocated strings, concatenating them with ConsStrings.
Would that work for you here?

No, the return value of this function is used by NewUnresolved, which requires
an AstRawString. Also, I don't know how to do to_string for AstRawStrings.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4290: FunctionLiteral*
Parser::SynthesizeClassFieldInitializer(int count) {
On 2016/09/09 at 22:54:42, Dan Ehrenberg wrote:
> Add a comment showing what this desugars into

Done.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4327: // function(){}; static y = function(){}; }
On 2016/09/09 at 22:54:41, Dan Ehrenberg wrote:
> Are the specs good about requiring the inference?

Nope: https://github.com/tc39/proposal-class-public-fields/issues/48

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4468: // necessary for non-computed names in full-codegen
On 2016/09/09 at 22:54:41, Dan Ehrenberg wrote:
> What happens when you don't include this on a non-computed name? I'm surprised
by the need for this on non-computed names.

That argument fails an IsName check in DefineDataPropertyInLiteral no matter
what form the name takes. Debug info indicates that it appears as a JSObject and
I have absolutely no idea why. This only happens with full-codegen.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4543:
ast_value_factory()->GetOneByteString(".class-field-initializer");
On 2016/09/09 at 22:54:42, Dan Ehrenberg wrote:
> Put in src/heap-symbols.h

Done.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4546: Variable::NORMAL);  // TODO(bakkot) flags
On 2016/09/09 at 22:54:42, Dan Ehrenberg wrote:
> What flags do you think might be wrong here? This seems fine.

Mostly I wasn't sure if kCreatedInitialized was correct.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4568: scope());  // TODO(bakkot) declare vs declarelocal
On 2016/09/09 at 22:54:42, Dan Ehrenberg wrote:
> Declare.

Thanks.

https://codereview.chromium.org/2316233004/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4579: // especially in trivial cases
On 2016/09/09 at 22:54:42, Dan Ehrenberg wrote:
> As we discussed off-line, you can use the scope information to determine if
super is called, either in this patch or a follow-on.

I'm going to make that change in a later patch, since this patch doesn't include
the relevant test.

[Dan Ehrenberg, 2016-09-13 18:15:18]

lgtm

[Dan Ehrenberg, 2016-09-13 21:37:06]

lgtm

Thanks for this fix. With a more descriptive commit message, this patch seems
good to commit.

[bakkot, 2016-09-15 21:16:53]

Description was changed from

==========
Class fields, part 2 (desugaring)

This is one 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 the desugaring logic to the parser. It isn't usable without
the (forthcoming) backend changes.

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

to

==========
Class fields, part 2 (desugaring)

This is one 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 the desugaring logic to the parser. It isn't usable without
the (forthcoming) backend changes.

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

[bakkot, 2016-09-15 21:56:58]

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

[commit-bot: I haz the power, 2016-09-15 21:57:06]

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 21:58:55]

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

[commit-bot: I haz the power, 2016-09-15 21:58:56]

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_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/12857)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  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_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/12821)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/24206)

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

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:25]

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:34]

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

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

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  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_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/12866)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/12858)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  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_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/24207)

[bakkot, 2016-09-15 23:00:38]

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:00:45]

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:06:30]

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

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

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

[bakkot, 2016-09-15 23:07:51]

The CQ bit was unchecked by bakkot@google.com

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

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:09:50]

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:38:40]

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

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

Dry run: This issue passed the CQ dry run.

[bakkot, 2016-09-16 00:43:16]

The CQ bit was checked by bakkot@google.com

[bakkot, 2016-09-16 00:43:16]

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/2316233004/#ps100001
(title: "rebase harder")

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

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 00:46:10]

Committed patchset #6 (id:100001)

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

Description was changed from

==========
Class fields, part 2 (desugaring)

This is one 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 the desugaring logic to the parser. It isn't usable without
the (forthcoming) backend changes.

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

to

==========
Class fields, part 2 (desugaring)

This is one 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 the desugaring logic to the parser. It isn't usable without
the (forthcoming) backend changes.

BUG=v8:5367

Committed: https://crrev.com/834aec33918ad645aba3b2662d6ceab056867427
Cr-Commit-Position: refs/heads/master@{#39460}
==========

[commit-bot: I haz the power, 2016-09-16 00:47:02]

Patchset 6 (id:??) landed as
https://crrev.com/834aec33918ad645aba3b2662d6ceab056867427
Cr-Commit-Position: refs/heads/master@{#39460}