Wrap ClassLiterals in DoExpressions instead of giving them BlockScopes.

Wrap ClassLiterals in DoExpressions instead of giving them BlockScopes.

This slightly simplifies scope handling. It also makes it possible to
implement some potential future changes to classes purely in the parser
by adding additional code to the DoExpression.

This is a portion of https://codereview.chromium.org/2142333002/, which
probably isn't going through in full.

Committed: https://crrev.com/c2bcfc31454bfa95e146cb6002fb6a9a142181f7
Cr-Commit-Position: refs/heads/master@{#38035}

[bakkot, 2016-07-22 19:35:59]

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

[commit-bot: I haz the power, 2016-07-22 19:36:03]

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

[bakkot, 2016-07-22 19:46:03]

Description was changed from

==========
Wrap ClassLiterals in DoExpressions instead of giving them BlockScopes.

This slightly simplifies scope handling. It also makes it possible to implement
some potential future changes to classes purely in the parser by adding
additional code to the DoExpression.
==========

to

==========
Wrap ClassLiterals in DoExpressions instead of giving them BlockScopes.

This slightly simplifies scope handling. It also makes it possible to
implement some potential future changes to classes purely in the parser
by adding additional code to the DoExpression.

This is a portion of https://codereview.chromium.org/2142333002/, which
probably isn't going through in full.
==========

[bakkot, 2016-07-22 19:48:23]

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

[commit-bot: I haz the power, 2016-07-22 19:48:31]

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

[bakkot, 2016-07-22 19:54:32]

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

[commit-bot: I haz the power, 2016-07-22 20:00:00]

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

[commit-bot: I haz the power, 2016-07-22 20:00:01]

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...)

[bakkot, 2016-07-22 20:00:55]

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

[commit-bot: I haz the power, 2016-07-22 20:01:01]

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-07-22 20:22:58]

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

[commit-bot: I haz the power, 2016-07-22 20:22:58]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-07-22 23:35:47]

adamk@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[adamk, 2016-07-22 23:35:47]

+mstarzinger for compile and interpreter

[adamk, 2016-07-22 23:37:53]

This seems like a good idea to me; note that I just landed a change that'll
probably conflict with all your parameter-initializer-rewriter changes, sorry
about that. Please ping back when you've merged, as that's the only file I have
concerns over.

[bakkot, 2016-07-23 01:14:16]

On 2016/07/22 at 23:37:53, adamk wrote:
> This seems like a good idea to me; note that I just landed a change that'll
probably conflict with all your parameter-initializer-rewriter changes, sorry
about that. Please ping back when you've merged, as that's the only file I have
concerns over.

Done.

I've re-introduced old_scope to the parameter-initializer-rewriter, because this
change relied on it: this change causes the rewriter to recur into blocks even
when they have a scope, so as to not miss VariableProxies which are nested in do
expressions. old_scope is then necessary to avoid reparenting nested scopes.

This was certainly necessary with the old rewriter. It may not be now. The only
test case I had which required this change to the rewriter made use of
temporaries.

I was expecting something like '((a, b = do {let x = 0; a+x})=>b)(0)' to fail
without this change, but that doesn't seem to be the case. Adam, if you can
confirm that recurring into blocks which have associated scopes is never
necessary (even when those blocks may be associated with do expressions) I'll
get rid of old_scope again.

[marja, 2016-07-25 08:02:41]

src/parsing/parser* LGTM, as the changes there are straightforward.

adamk@ can review parsing/pattern-rewriter.cc as I'm not familiar with that code

[Michael Starzinger, 2016-07-25 14:49:54]

LGTM on everything aside from the parser. Just one nit left. Thanks!

https://codereview.chromium.org/2176653003/diff/60001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/2176653003/diff/60001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1449: {
nit: This block looks like it was only introduced for the NestedClassLiteral.
Can we remove it and outdent everything one level?

[Michael Starzinger, 2016-07-25 14:56:47]

One more comment.

https://codereview.chromium.org/2176653003/diff/60001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2176653003/diff/60001/src/ast/ast.h#newcode2739
src/ast/ast.h:2739: BailoutId ExitId() { return BailoutId(local_id(2)); }
The first three bailout ids should no longer be required. Can we drop them and
also decreate the number of ids in {num_ids} accordingly?

[adamk, 2016-07-25 17:19:28]

On 2016/07/23 01:14:16, bakkot wrote:
> On 2016/07/22 at 23:37:53, adamk wrote:
> > This seems like a good idea to me; note that I just landed a change that'll
> probably conflict with all your parameter-initializer-rewriter changes, sorry
> about that. Please ping back when you've merged, as that's the only file I
have
> concerns over.
> 
> Done.
> 
> I've re-introduced old_scope to the parameter-initializer-rewriter, because
this
> change relied on it: this change causes the rewriter to recur into blocks even
> when they have a scope, so as to not miss VariableProxies which are nested in
do
> expressions. old_scope is then necessary to avoid reparenting nested scopes.
> 
> This was certainly necessary with the old rewriter. It may not be now. The
only
> test case I had which required this change to the rewriter made use of
> temporaries.
> 
> I was expecting something like '((a, b = do {let x = 0; a+x})=>b)(0)' to fail
> without this change, but that doesn't seem to be the case. Adam, if you can
> confirm that recurring into blocks which have associated scopes is never
> necessary (even when those blocks may be associated with do expressions) I'll
> get rid of old_scope again.

We now do a better job of handling arrow functions. For the test case you list,
I think we handle everything correctly before calling the scope rewriter:

- The do-expression's block scope is already reparented to have the arrow
function's scope as its outer_scope
- The temporary created as part of the do-expression is already moved to the
arrow function, so there's no need to dig down into it

The only use of the scope rewriter now is for the case of a sloppy eval in a
parameter, which causes us to add an extra varblock scope that needs to be
inserted below the function scope. However, I'm not at all sure how sloppy eval
works inside a do-expression. And I already know that var declarations inside
do-expressions don't work properly. So if we don't have a test case motivating
the change, I'd suggest leaving the scope rewriter untouched in this patch.

[bakkot, 2016-07-25 18:34:19]

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

[commit-bot: I haz the power, 2016-07-25 18:34:28]

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

[bakkot, 2016-07-25 18:51:48]

On 2016/07/25 at 17:19:28, adamk wrote:
> On 2016/07/23 01:14:16, bakkot wrote:
> > On 2016/07/22 at 23:37:53, adamk wrote:
> > > This seems like a good idea to me; note that I just landed a change
that'll
> > probably conflict with all your parameter-initializer-rewriter changes,
sorry
> > about that. Please ping back when you've merged, as that's the only file I
have
> > concerns over.
> > 
> > Done.
> > 
> > I've re-introduced old_scope to the parameter-initializer-rewriter, because
this
> > change relied on it: this change causes the rewriter to recur into blocks
even
> > when they have a scope, so as to not miss VariableProxies which are nested
in do
> > expressions. old_scope is then necessary to avoid reparenting nested scopes.
> > 
> > This was certainly necessary with the old rewriter. It may not be now. The
only
> > test case I had which required this change to the rewriter made use of
> > temporaries.
> > 
> > I was expecting something like '((a, b = do {let x = 0; a+x})=>b)(0)' to
fail
> > without this change, but that doesn't seem to be the case. Adam, if you can
> > confirm that recurring into blocks which have associated scopes is never
> > necessary (even when those blocks may be associated with do expressions)
I'll
> > get rid of old_scope again.
> 
> We now do a better job of handling arrow functions. For the test case you
list, I think we handle everything correctly before calling the scope rewriter:
> 
> - The do-expression's block scope is already reparented to have the arrow
function's scope as its outer_scope
> - The temporary created as part of the do-expression is already moved to the
arrow function, so there's no need to dig down into it
> 
> The only use of the scope rewriter now is for the case of a sloppy eval in a
parameter, which causes us to add an extra varblock scope that needs to be
inserted below the function scope. However, I'm not at all sure how sloppy eval
works inside a do-expression. And I already know that var declarations inside
do-expressions don't work properly. So if we don't have a test case motivating
the change, I'd suggest leaving the scope rewriter untouched in this patch.

Ah, good. I've put it back as it was.

[bakkot, 2016-07-25 18:52:04]

https://codereview.chromium.org/2176653003/diff/60001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2176653003/diff/60001/src/ast/ast.h#newcode2739
src/ast/ast.h:2739: BailoutId ExitId() { return BailoutId(local_id(2)); }
On 2016/07/25 at 14:56:47, Michael Starzinger wrote:
> The first three bailout ids should no longer be required. Can we drop them and
also decreate the number of ids in {num_ids} accordingly?

Done. Also edited GetIdForProperty, which I am *pretty* sure also refers to the
number of ids required.

https://codereview.chromium.org/2176653003/diff/60001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/2176653003/diff/60001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1449: {
On 2016/07/25 at 14:49:54, Michael Starzinger wrote:
> nit: This block looks like it was only introduced for the NestedClassLiteral.
Can we remove it and outdent everything one level?

Done.

[commit-bot: I haz the power, 2016-07-25 19:03:59]

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

[commit-bot: I haz the power, 2016-07-25 19:04:00]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-07-25 19:14:07]

lgtm

[bakkot, 2016-07-25 19:14:37]

The CQ bit was checked by bakkot@google.com

[bakkot, 2016-07-25 19:14:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2176653003/#ps80001
(title: "address comments")

[commit-bot: I haz the power, 2016-07-25 19:14:46]

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-07-25 19:16:28]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-25 19:21:55]

Description was changed from

==========
Wrap ClassLiterals in DoExpressions instead of giving them BlockScopes.

This slightly simplifies scope handling. It also makes it possible to
implement some potential future changes to classes purely in the parser
by adding additional code to the DoExpression.

This is a portion of https://codereview.chromium.org/2142333002/, which
probably isn't going through in full.
==========

to

==========
Wrap ClassLiterals in DoExpressions instead of giving them BlockScopes.

This slightly simplifies scope handling. It also makes it possible to
implement some potential future changes to classes purely in the parser
by adding additional code to the DoExpression.

This is a portion of https://codereview.chromium.org/2142333002/, which
probably isn't going through in full.

Committed: https://crrev.com/c2bcfc31454bfa95e146cb6002fb6a9a142181f7
Cr-Commit-Position: refs/heads/master@{#38035}
==========

[commit-bot: I haz the power, 2016-07-25 19:21:57]

Patchset 5 (id:??) landed as
https://crrev.com/c2bcfc31454bfa95e146cb6002fb6a9a142181f7
Cr-Commit-Position: refs/heads/master@{#38035}

[Dan Ehrenberg, 2017-04-28 08:33:22]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2849773002/ by littledan@chromium.org.

The reason for reverting is: Blamed for
https://bugs.chromium.org/p/chromium/issues/detail?id=716265.