[es7] implement |do| expressions proposal

[es7] implement |do| expressions proposal

Adds an implementation of "do expression" parsing (https://webcache.googleusercontent.com/search?q=cache:MIGALjqPDNgJ:wiki.ecmascript.org/doku.php%3Fid%3Dstrawman:do_expressions+&cd=1&hl=en&ct=clnk&gl=us).

This feature provides a way to evaluate a block of statements within an expression context, producing the resulting completion value. This is very helpful for implementing certain language features via desugaring.

BUG=v8:4488
LOG=N
R=adamk@chromium.org, bmeurer@chromium.org, rossberg@chromium.org, wingo@igalia.com

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

[caitp (gmail), 2015-10-11 03:10:23]

This is a work in progress, first sort of checkpoint in the prototyping of
do-expressions.

There is _some_ parsing support, for the benefit of testing.

A lot of stuff here is quite a mess, but hopefully myself and others can be
satisfied with it at some point.

Implementation is as follows:

- Parsed as part of an AssignmentExpression, if the first token is 'do'
- Parsed as a harmony-style block, with its own scope.
- The block is rewritten in a fashion similar to eval(), in rewriter.cc
- rewriter.cc needed some hacks to make this work correctly, as the parser does
not have access to an isolate.
- There are 2 new AST nodes, it's likely the CompletionStatement node isn't
really needed, but is currently there to work around ExpressionStatement
visitors always using an EffectContext in full-codegen

I'm sure reviewers will find lots of things to pick at, and it will be lots of
fun, whenever anyone has some time to poke at it =)

[caitp (gmail), 2015-10-11 14:05:05]

Patchset #3 (id:40001) has been deleted

[caitp (gmail), 2015-10-11 14:27:24]

Patchset #3 (id:60001) has been deleted

[caitp (gmail), 2015-10-12 12:58:39]

caitpotter88@gmail.com changed reviewers:
+ adamk@chromium.org, bmeurer@chromium.org, rossberg@chromium.org,
wingo@igalia.com

[caitp (gmail), 2015-10-12 13:03:56]

On 2015/10/11 03:10:23, caitp wrote:
> This is a work in progress, first sort of checkpoint in the prototyping of
> do-expressions.
> 
> There is _some_ parsing support, for the benefit of testing.
> 
> A lot of stuff here is quite a mess, but hopefully myself and others can be
> satisfied with it at some point.
> 
> Implementation is as follows:
> 
> - Parsed as part of an AssignmentExpression, if the first token is 'do'
> - Parsed as a harmony-style block, with its own scope.
> - The block is rewritten in a fashion similar to eval(), in rewriter.cc
> - rewriter.cc needed some hacks to make this work correctly, as the parser
does
> not have access to an isolate.
> - There are 2 new AST nodes, it's likely the CompletionStatement node isn't
> really needed, but is currently there to work around ExpressionStatement
> visitors always using an EffectContext in full-codegen
> 
> I'm sure reviewers will find lots of things to pick at, and it will be lots of
> fun, whenever anyone has some time to poke at it =)

It's been improved a little bit, and should be good enough to implement
ConcatIterableToArray() on top of now, at least.

[rossberg, 2015-10-12 13:52:40]

Excellent!

https://codereview.chromium.org/1399893002/diff/100001/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/1399893002/diff/100001/src/ast.cc#newcode1131
src/ast.cc:1131: Variable* AstNodeFactory::NewTemporaryVar(Scope* scope,
This function seems rather redundant.

https://codereview.chromium.org/1399893002/diff/100001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1399893002/diff/100001/src/ast.h#newcode2698
src/ast.h:2698: Scope* scope() { return scope_; }
Why not just have a DoExpression carry a Block, instead of its own scope and
statements? That seems natural, and probably allows a bit more code reuse.
(TryStatement does the same, for example.)

https://codereview.chromium.org/1399893002/diff/100001/src/compiler/ast-graph...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1399893002/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:1674: if (expr->result()->is_assigned()) {
It's better to avoid this case distinction in back-ends and simply make sure in
the rewriter that it always gets assigned, with undefined if necessary.

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

https://codereview.chromium.org/1399893002/diff/100001/src/flag-definitions.h...
src/flag-definitions.h:190: DEFINE_BOOL(do_expression_parsing, false, "do
expression parsing")
No reason not to call this harmony_do_expressions, I think.

https://codereview.chromium.org/1399893002/diff/100001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1399893002/diff/100001/src/preparser.h#newcod...
src/preparser.h:2524: Expect(Token::LBRACE, CHECK_OK);
Can't you just invoke ParseScopedBlock here?

https://codereview.chromium.org/1399893002/diff/100001/src/rewriter.cc
File src/rewriter.cc (left):

https://codereview.chromium.org/1399893002/diff/100001/src/rewriter.cc#oldcode75
src/rewriter.cc:75: DEFINE_AST_VISITOR_SUBCLASS_MEMBERS();
I think Adam is working on a refactoring for separating AstVisitor from Isolate.

[caitp (gmail), 2015-10-12 18:23:00]

Some cleanup

Few things left untouched:
  - the flag name is still the same, but if people really want to change it, I
can agree to that
  - The rewriter is still mangled to not have to depend on Isolate, but if Adam
is working on a fix for that, that will change a lot in a followup

https://codereview.chromium.org/1399893002/diff/100001/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/1399893002/diff/100001/src/ast.cc#newcode1131
src/ast.cc:1131: Variable* AstNodeFactory::NewTemporaryVar(Scope* scope,
On 2015/10/12 13:52:39, rossberg wrote:
> This function seems rather redundant.

Yeah, it's gone. It's more useful to let the NewDoExpression caller pass in the
result variable, since internal uses will likely want to provide their own
instead of allocating yet another temporary.

https://codereview.chromium.org/1399893002/diff/100001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1399893002/diff/100001/src/ast.h#newcode2698
src/ast.h:2698: Scope* scope() { return scope_; }
On 2015/10/12 13:52:39, rossberg wrote:
> Why not just have a DoExpression carry a Block, instead of its own scope and
> statements? That seems natural, and probably allows a bit more code reuse.
> (TryStatement does the same, for example.)

Done

https://codereview.chromium.org/1399893002/diff/100001/src/compiler/ast-graph...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1399893002/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:1674: if (expr->result()->is_assigned()) {
On 2015/10/12 13:52:39, rossberg wrote:
> It's better to avoid this case distinction in back-ends and simply make sure
in
> the rewriter that it always gets assigned, with undefined if necessary.

Done.

It seems like it could potentially save a bit of space if the variable never got
allocated, though.

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

https://codereview.chromium.org/1399893002/diff/100001/src/flag-definitions.h...
src/flag-definitions.h:190: DEFINE_BOOL(do_expression_parsing, false, "do
expression parsing")
On 2015/10/12 13:52:40, rossberg wrote:
> No reason not to call this harmony_do_expressions, I think.

Hmm, I think "harmony" might be a bit of a stretch until other implementors are
on board, and I didn't want to give the impression that it would be safe for
people to start using it in node.js apps or something.

Do we really want to change the name?

https://codereview.chromium.org/1399893002/diff/100001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1399893002/diff/100001/src/preparser.h#newcod...
src/preparser.h:2524: Expect(Token::LBRACE, CHECK_OK);
On 2015/10/12 13:52:40, rossberg wrote:
> Can't you just invoke ParseScopedBlock here?

Done, sort of.

ParseScopedBlock only exists in the full parser, can't be called from
ParserBase. I wound up creating separate implementations in preparser.cc and
parser.cc instead, which isn't ideal.

https://codereview.chromium.org/1399893002/diff/100001/src/rewriter.cc
File src/rewriter.cc (left):

https://codereview.chromium.org/1399893002/diff/100001/src/rewriter.cc#oldcode75
src/rewriter.cc:75: DEFINE_AST_VISITOR_SUBCLASS_MEMBERS();
On 2015/10/12 13:52:40, rossberg wrote:
> I think Adam is working on a refactoring for separating AstVisitor from
Isolate.

If that lands first, that would be helpful.

[adamk, 2015-10-12 19:02:21]

https://codereview.chromium.org/1399893002/diff/100001/src/rewriter.cc
File src/rewriter.cc (left):

https://codereview.chromium.org/1399893002/diff/100001/src/rewriter.cc#oldcode75
src/rewriter.cc:75: DEFINE_AST_VISITOR_SUBCLASS_MEMBERS();
On 2015/10/12 18:23:00, caitp wrote:
> On 2015/10/12 13:52:40, rossberg wrote:
> > I think Adam is working on a refactoring for separating AstVisitor from
> Isolate.
> 
> If that lands first, that would be helpful.

I was hoping to get some answers on my v8-dev thread
(https://groups.google.com/d/msg/v8-dev/NnbGtwMgwOk/WT91HW7SCQAJ) but barring
that I'll be in MUC this week so will find out in person what the best way
forward here is. My refactoring is not likely to be real far off in
functionality from what you've got here, but will obviously involve less code
duplication.

[rossberg, 2015-10-13 10:44:33]

https://codereview.chromium.org/1399893002/diff/100001/src/compiler/ast-graph...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1399893002/diff/100001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:1674: if (expr->result()->is_assigned()) {
On 2015/10/12 18:22:59, caitp wrote:
> On 2015/10/12 13:52:39, rossberg wrote:
> > It's better to avoid this case distinction in back-ends and simply make sure
> in
> > the rewriter that it always gets assigned, with undefined if necessary.
> 
> Done.
> 
> It seems like it could potentially save a bit of space if the variable never
got
> allocated, though.

Such an optimsation comes for free with SSA form.

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

https://codereview.chromium.org/1399893002/diff/100001/src/flag-definitions.h...
src/flag-definitions.h:190: DEFINE_BOOL(do_expression_parsing, false, "do
expression parsing")
On 2015/10/12 18:23:00, caitp wrote:
> On 2015/10/12 13:52:40, rossberg wrote:
> > No reason not to call this harmony_do_expressions, I think.
> 
> Hmm, I think "harmony" might be a bit of a stretch until other implementors
are
> on board, and I didn't want to give the impression that it would be safe for
> people to start using it in node.js apps or something.
> 
> Do we really want to change the name?

It's in the TC39 process, so it's adequate. We have consistently used harmony
for those. So yes, please change.

(As for impressions, we have repeatedly stated that it is _not_ safe to use
features that are not implied by the global --harmony flag, for all sorts of
reasons. Anybody who does so is on their own.)

https://codereview.chromium.org/1399893002/diff/140001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1399893002/diff/140001/src/preparser.h#newcod...
src/preparser.h:1830: Expression ParseDoExpression(bool* ok);
Nit: group this with the Expression parsing functions below.

https://codereview.chromium.org/1399893002/diff/140001/src/rewriter.cc
File src/rewriter.cc (right):

https://codereview.chromium.org/1399893002/diff/140001/src/rewriter.cc#newcode47
src/rewriter.cc:47: // Returns ".result = value"
Why are these public now?

https://codereview.chromium.org/1399893002/diff/140001/src/typing-asm.cc
File src/typing-asm.cc (right):

https://codereview.chromium.org/1399893002/diff/140001/src/typing-asm.cc#newc...
src/typing-asm.cc:438: void AsmTyper::VisitDoExpression(DoExpression* expr) {
This function needs to fail, do-expressions are not legal asm.

https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc#newcode353
src/typing.cc:353: RECURSE(VisitVariableProxy(expr->result()));
This function should set the type of the do-node to that of the result variable.

https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
File test/mjsunit/internal-do-expressions.js (right):

https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
test/mjsunit/internal-do-expressions.js:7: // This feature is not currently on
the standards track. --do-expression-parsing
It is, though still at stage 0.

https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
test/mjsunit/internal-do-expressions.js:14: function TestBasic() {
The most important thing to test is all sorts of statements within a non-trivial
expression context, i.e., one where the operand stack isn't empty:

x + do{...}
f(do{...})

etc. I assume this will not work out of the box, in particular not with
deoptimisation.

[caitp (gmail), 2015-10-13 15:06:00]

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

https://codereview.chromium.org/1399893002/diff/100001/src/flag-definitions.h...
src/flag-definitions.h:190: DEFINE_BOOL(do_expression_parsing, false, "do
expression parsing")
On 2015/10/13 10:44:32, rossberg wrote:
> On 2015/10/12 18:23:00, caitp wrote:
> > On 2015/10/12 13:52:40, rossberg wrote:
> > > No reason not to call this harmony_do_expressions, I think.
> > 
> > Hmm, I think "harmony" might be a bit of a stretch until other implementors
> are
> > on board, and I didn't want to give the impression that it would be safe for
> > people to start using it in node.js apps or something.
> > 
> > Do we really want to change the name?
> 
> It's in the TC39 process, so it's adequate. We have consistently used harmony
> for those. So yes, please change.
> 
> (As for impressions, we have repeatedly stated that it is _not_ safe to use
> features that are not implied by the global --harmony flag, for all sorts of
> reasons. Anybody who does so is on their own.)

Done

https://codereview.chromium.org/1399893002/diff/140001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1399893002/diff/140001/src/preparser.h#newcod...
src/preparser.h:1830: Expression ParseDoExpression(bool* ok);
On 2015/10/13 10:44:32, rossberg wrote:
> Nit: group this with the Expression parsing functions below.

Done.

https://codereview.chromium.org/1399893002/diff/140001/src/rewriter.cc
File src/rewriter.cc (right):

https://codereview.chromium.org/1399893002/diff/140001/src/rewriter.cc#newcode47
src/rewriter.cc:47: // Returns ".result = value"
On 2015/10/13 10:44:33, rossberg wrote:
> Why are these public now?

SetResult is used by the do expression rewriter, instead of duplicating code.
There isn't much point in making them private, tbh.

https://codereview.chromium.org/1399893002/diff/140001/src/typing-asm.cc
File src/typing-asm.cc (right):

https://codereview.chromium.org/1399893002/diff/140001/src/typing-asm.cc#newc...
src/typing-asm.cc:438: void AsmTyper::VisitDoExpression(DoExpression* expr) {
On 2015/10/13 10:44:33, rossberg wrote:
> This function needs to fail, do-expressions are not legal asm.

Done

https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc#newcode353
src/typing.cc:353: RECURSE(VisitVariableProxy(expr->result()));
On 2015/10/13 10:44:33, rossberg wrote:
> This function should set the type of the do-node to that of the result
variable.

Done, I think.. It's not clear that the Proxy's bounds are always set to
anything useful

https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
File test/mjsunit/internal-do-expressions.js (right):

https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
test/mjsunit/internal-do-expressions.js:7: // This feature is not currently on
the standards track. --do-expression-parsing
On 2015/10/13 10:44:33, rossberg wrote:
> It is, though still at stage 0.

Acknowledged.

https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
test/mjsunit/internal-do-expressions.js:14: function TestBasic() {
On 2015/10/13 10:44:33, rossberg wrote:
> The most important thing to test is all sorts of statements within a
non-trivial
> expression context, i.e., one where the operand stack isn't empty:
> 
> x + do{...}
> f(do{...})
> 
> etc. I assume this will not work out of the box, in particular not with
> deoptimisation.

For every case I've tried, it seems to work (although there was an issue in
crankshaft due to bailouts when visiting the `do` block that wasn't handled,
which I'll fix in the next patchset).

It's hard to get enough coverage for this, but I've added some more tests, and
some suggestions for others would be very helpful

[adamk, 2015-10-14 13:06:26]

If you sync to
https://chromium.googlesource.com/v8/v8/+/d317145d05f25232f8c45f5b8ed98e2df66...,
it should be trivial to add a new InitializeAstVisitor() method that takes a
stack_limit (or a Parser) instead of an Isolate.

[caitp (gmail), 2015-10-14 13:37:19]

On 2015/10/14 13:06:26, adamk wrote:
> If you sync to
>
https://chromium.googlesource.com/v8/v8/+/d317145d05f25232f8c45f5b8ed98e2df66...,
> it should be trivial to add a new InitializeAstVisitor() method that takes a
> stack_limit (or a Parser) instead of an Isolate.

done

[rossberg, 2015-10-15 09:58:27]

LGTM

https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc#newcode353
src/typing.cc:353: RECURSE(VisitVariableProxy(expr->result()));
On 2015/10/13 15:06:00, caitp wrote:
> On 2015/10/13 10:44:33, rossberg wrote:
> > This function should set the type of the do-node to that of the result
> variable.
> 
> Done, I think.. It's not clear that the Proxy's bounds are always set to
> anything useful

True, but that is generally true when you try to type JS. ;)

https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
File test/mjsunit/internal-do-expressions.js (right):

https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
test/mjsunit/internal-do-expressions.js:14: function TestBasic() {
On 2015/10/13 15:06:00, caitp wrote:
> On 2015/10/13 10:44:33, rossberg wrote:
> > The most important thing to test is all sorts of statements within a
> non-trivial
> > expression context, i.e., one where the operand stack isn't empty:
> > 
> > x + do{...}
> > f(do{...})
> > 
> > etc. I assume this will not work out of the box, in particular not with
> > deoptimisation.
> 
> For every case I've tried, it seems to work (although there was an issue in
> crankshaft due to bailouts when visiting the `do` block that wasn't handled,
> which I'll fix in the next patchset).
> 
> It's hard to get enough coverage for this, but I've added some more tests, and
> some suggestions for others would be very helpful

Some other scenarios to test:

- Uses of break and continue inside the do (i.e., those that don't escape -- I
think they should work out of the box).
- Uses of throw inside the do, both caught inside or outside the do.
- Various completion values (cf. the existing completion.js).
- Different forms of declarations inside the do (esp. hoisting of var).
- Sloppy eval with var declaration. :)
- Do in some more esoteric positions, like default parameters, decl
initialisers, `return` -- especially do that throws or has var declarations
- Deopts triggered inside the do.
- Conversely, OSR inside the do, i.e., a long-running for-loop that gets
optimised in the middle.

I'm fine with leaving these for follow-up CLs, just dropping a list of
everything that might be problematic and should eventually be tested. :)

[adamk, 2015-10-15 10:59:33]

Definitely would like more tests too: var and function declarations being the
ones I'm most worried about, but also legacy const.

As a side-note, this is going to further complicate the (sadly ongoing, due to
yak-shaving) attempt to fix scoping of defaults in arrow parameter lists.

https://codereview.chromium.org/1399893002/diff/240001/src/ast-literal-reinde...
File src/ast-literal-reindexer.cc (right):

https://codereview.chromium.org/1399893002/diff/240001/src/ast-literal-reinde...
src/ast-literal-reindexer.cc:47: // TODO(caitp): literals in do expressions need
re-indexing too.
Is this meant for a followup? It'd be nice to have a summary in the CL
description of what's known not to work in this patch.

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.cc#newco...
src/preparser.cc:1248: Scope* block_scope = NewScope(scope_, BLOCK_SCOPE);
Why does this need a Scope when we don't use a Scope for
PreParser::ParseBlock()?

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.h#newcod...
src/preparser.h:144: INLINE(bool CheckStackOverflow()) {
I don't think you need this anymore.

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.h#newcod...
src/preparser.h:2428: if (token == Token::DO && allow_harmony_do_expressions())
{
Didn't we just switch on |token|?

https://codereview.chromium.org/1399893002/diff/240001/src/rewriter.h
File src/rewriter.h (right):

https://codereview.chromium.org/1399893002/diff/240001/src/rewriter.h#newcode27
src/rewriter.h:27: // to  ensure identical behaviour of completion result.
Nit: extra space between "to" and "ensure".

[caitp (gmail), 2015-10-15 11:24:17]

https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
File test/mjsunit/internal-do-expressions.js (right):

https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
test/mjsunit/internal-do-expressions.js:14: function TestBasic() {
On 2015/10/15 09:58:27, rossberg wrote:
> On 2015/10/13 15:06:00, caitp wrote:
> > On 2015/10/13 10:44:33, rossberg wrote:
> > > The most important thing to test is all sorts of statements within a
> > non-trivial
> > > expression context, i.e., one where the operand stack isn't empty:
> > > 
> > > x + do{...}
> > > f(do{...})
> > > 
> > > etc. I assume this will not work out of the box, in particular not with
> > > deoptimisation.
> > 
> > For every case I've tried, it seems to work (although there was an issue in
> > crankshaft due to bailouts when visiting the `do` block that wasn't handled,
> > which I'll fix in the next patchset).
> > 
> > It's hard to get enough coverage for this, but I've added some more tests,
and
> > some suggestions for others would be very helpful
> 
> Some other scenarios to test:
> 
> - Uses of break and continue inside the do (i.e., those that don't escape -- I
> think they should work out of the box).
> - Uses of throw inside the do, both caught inside or outside the do.
> - Various completion values (cf. the existing completion.js).
> - Different forms of declarations inside the do (esp. hoisting of var).
> - Sloppy eval with var declaration. :)
> - Do in some more esoteric positions, like default parameters, decl
> initialisers, `return` -- especially do that throws or has var declarations
> - Deopts triggered inside the do.
> - Conversely, OSR inside the do, i.e., a long-running for-loop that gets
> optimised in the middle.
> 
> I'm fine with leaving these for follow-up CLs, just dropping a list of
> everything that might be problematic and should eventually be tested. :)

Ah yes, I'll add more

https://codereview.chromium.org/1399893002/diff/240001/src/ast-literal-reinde...
File src/ast-literal-reindexer.cc (right):

https://codereview.chromium.org/1399893002/diff/240001/src/ast-literal-reinde...
src/ast-literal-reindexer.cc:47: // TODO(caitp): literals in do expressions need
re-indexing too.
On 2015/10/15 10:59:33, adamk wrote:
> Is this meant for a followup? It'd be nice to have a summary in the CL
> description of what's known not to work in this patch.

I guess it's really the same as initializer literals, so maybe they don't need
reindexing

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.cc#newco...
src/preparser.cc:1248: Scope* block_scope = NewScope(scope_, BLOCK_SCOPE);
On 2015/10/15 10:59:33, adamk wrote:
> Why does this need a Scope when we don't use a Scope for
> PreParser::ParseBlock()?

Allocating the temporary result, but it lives in the closure scope anyways so it
may not matter

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.h#newcod...
src/preparser.h:2428: if (token == Token::DO && allow_harmony_do_expressions())
{
On 2015/10/15 10:59:33, adamk wrote:
> Didn't we just switch on |token|?

It can fall through from the above, unfortunately :(

[adamk, 2015-10-15 12:01:25]

https://codereview.chromium.org/1399893002/diff/240001/src/ast-literal-reinde...
File src/ast-literal-reindexer.cc (right):

https://codereview.chromium.org/1399893002/diff/240001/src/ast-literal-reinde...
src/ast-literal-reindexer.cc:47: // TODO(caitp): literals in do expressions need
re-indexing too.
On 2015/10/15 11:24:17, caitp wrote:
> On 2015/10/15 10:59:33, adamk wrote:
> > Is this meant for a followup? It'd be nice to have a summary in the CL
> > description of what's known not to work in this patch.
> 
> I guess it's really the same as initializer literals, so maybe they don't need
> reindexing

Seems like this should be testable one way or the other.

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.cc#newco...
src/preparser.cc:1248: Scope* block_scope = NewScope(scope_, BLOCK_SCOPE);
On 2015/10/15 11:24:17, caitp wrote:
> On 2015/10/15 10:59:33, adamk wrote:
> > Why does this need a Scope when we don't use a Scope for
> > PreParser::ParseBlock()?
> 
> Allocating the temporary result, but it lives in the closure scope anyways so
it
> may not matter

I'd remove it (and maybe just delegate to ParseBlock, though I see that the
PreParser's version of ParseBlock is a bit funny).

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.h#newcod...
src/preparser.h:2428: if (token == Token::DO && allow_harmony_do_expressions())
{
On 2015/10/15 11:24:17, caitp wrote:
> On 2015/10/15 10:59:33, adamk wrote:
> > Didn't we just switch on |token|?
> 
> It can fall through from the above, unfortunately :(

What if you restructured the cases to have returns instead of "result = ...;
break;"? In fact I already see some returns above, so I think that'd make it
overall more consistent.

[caitp (gmail), 2015-10-15 12:34:07]

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.cc#newco...
src/preparser.cc:1248: Scope* block_scope = NewScope(scope_, BLOCK_SCOPE);
On 2015/10/15 12:01:25, adamk wrote:
> On 2015/10/15 11:24:17, caitp wrote:
> > On 2015/10/15 10:59:33, adamk wrote:
> > > Why does this need a Scope when we don't use a Scope for
> > > PreParser::ParseBlock()?
> > 
> > Allocating the temporary result, but it lives in the closure scope anyways
so
> it
> > may not matter
> 
> I'd remove it (and maybe just delegate to ParseBlock, though I see that the
> PreParser's version of ParseBlock is a bit funny).

Wait, I had actually gotten rid of this in an earlier patch set, forgot about
that.

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.h#newcod...
src/preparser.h:2428: if (token == Token::DO && allow_harmony_do_expressions())
{
On 2015/10/15 12:01:25, adamk wrote:
> On 2015/10/15 11:24:17, caitp wrote:
> > On 2015/10/15 10:59:33, adamk wrote:
> > > Didn't we just switch on |token|?
> > 
> > It can fall through from the above, unfortunately :(
> 
> What if you restructured the cases to have returns instead of "result = ...;
> break;"? In fact I already see some returns above, so I think that'd make it
> overall more consistent.

I think doing that will add a lot of unrelated noise to the diff, probably
better to do that in a followup

[adamk, 2015-10-15 12:42:27]

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1399893002/diff/240001/src/preparser.h#newcod...
src/preparser.h:2428: if (token == Token::DO && allow_harmony_do_expressions())
{
On 2015/10/15 12:34:07, caitp wrote:
> On 2015/10/15 12:01:25, adamk wrote:
> > On 2015/10/15 11:24:17, caitp wrote:
> > > On 2015/10/15 10:59:33, adamk wrote:
> > > > Didn't we just switch on |token|?
> > > 
> > > It can fall through from the above, unfortunately :(
> > 
> > What if you restructured the cases to have returns instead of "result = ...;
> > break;"? In fact I already see some returns above, so I think that'd make it
> > overall more consistent.
> 
> I think doing that will add a lot of unrelated noise to the diff, probably
> better to do that in a followup

If you want to wait on it, please add a TODO.

[caitp (gmail), 2015-10-15 14:18:09]

On 2015/10/15 09:58:27, rossberg wrote:
> LGTM
> 
> https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc
> File src/typing.cc (right):
> 
>
https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc#newcode353
> src/typing.cc:353: RECURSE(VisitVariableProxy(expr->result()));
> On 2015/10/13 15:06:00, caitp wrote:
> > On 2015/10/13 10:44:33, rossberg wrote:
> > > This function should set the type of the do-node to that of the result
> > variable.
> > 
> > Done, I think.. It's not clear that the Proxy's bounds are always set to
> > anything useful
> 
> True, but that is generally true when you try to type JS. ;)
> 
>
https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
> File test/mjsunit/internal-do-expressions.js (right):
> 
>
https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
> test/mjsunit/internal-do-expressions.js:14: function TestBasic() {
> On 2015/10/13 15:06:00, caitp wrote:
> > On 2015/10/13 10:44:33, rossberg wrote:
> > > The most important thing to test is all sorts of statements within a
> > non-trivial
> > > expression context, i.e., one where the operand stack isn't empty:
> > > 
> > > x + do{...}
> > > f(do{...})
> > > 
> > > etc. I assume this will not work out of the box, in particular not with
> > > deoptimisation.
> > 
> > For every case I've tried, it seems to work (although there was an issue in
> > crankshaft due to bailouts when visiting the `do` block that wasn't handled,
> > which I'll fix in the next patchset).
> > 
> > It's hard to get enough coverage for this, but I've added some more tests,
and
> > some suggestions for others would be very helpful
> 
> Some other scenarios to test:
> 
> - Uses of break and continue inside the do (i.e., those that don't escape -- I
> think they should work out of the box).
> - Uses of throw inside the do, both caught inside or outside the do.
> - Various completion values (cf. the existing completion.js).
> - Different forms of declarations inside the do (esp. hoisting of var).
> - Sloppy eval with var declaration. :)
> - Do in some more esoteric positions, like default parameters, decl
> initialisers, `return` -- especially do that throws or has var declarations
> - Deopts triggered inside the do.
> - Conversely, OSR inside the do, i.e., a long-running for-loop that gets
> optimised in the middle.
> 
> I'm fine with leaving these for follow-up CLs, just dropping a list of
> everything that might be problematic and should eventually be tested. :)

About hoisting, do we want function declarations to be block-scoped all the
time, or only for strict scopes?

It doesn't make a lot of sense for a function declaration to escape the |do|
block, imo

[adamk, 2015-10-15 14:21:58]

On 2015/10/15 14:18:09, caitp wrote:
> On 2015/10/15 09:58:27, rossberg wrote:
> > LGTM
> > 
> > https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc
> > File src/typing.cc (right):
> > 
> >
>
https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc#newcode353
> > src/typing.cc:353: RECURSE(VisitVariableProxy(expr->result()));
> > On 2015/10/13 15:06:00, caitp wrote:
> > > On 2015/10/13 10:44:33, rossberg wrote:
> > > > This function should set the type of the do-node to that of the result
> > > variable.
> > > 
> > > Done, I think.. It's not clear that the Proxy's bounds are always set to
> > > anything useful
> > 
> > True, but that is generally true when you try to type JS. ;)
> > 
> >
>
https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
> > File test/mjsunit/internal-do-expressions.js (right):
> > 
> >
>
https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
> > test/mjsunit/internal-do-expressions.js:14: function TestBasic() {
> > On 2015/10/13 15:06:00, caitp wrote:
> > > On 2015/10/13 10:44:33, rossberg wrote:
> > > > The most important thing to test is all sorts of statements within a
> > > non-trivial
> > > > expression context, i.e., one where the operand stack isn't empty:
> > > > 
> > > > x + do{...}
> > > > f(do{...})
> > > > 
> > > > etc. I assume this will not work out of the box, in particular not with
> > > > deoptimisation.
> > > 
> > > For every case I've tried, it seems to work (although there was an issue
in
> > > crankshaft due to bailouts when visiting the `do` block that wasn't
handled,
> > > which I'll fix in the next patchset).
> > > 
> > > It's hard to get enough coverage for this, but I've added some more tests,
> and
> > > some suggestions for others would be very helpful
> > 
> > Some other scenarios to test:
> > 
> > - Uses of break and continue inside the do (i.e., those that don't escape --
I
> > think they should work out of the box).
> > - Uses of throw inside the do, both caught inside or outside the do.
> > - Various completion values (cf. the existing completion.js).
> > - Different forms of declarations inside the do (esp. hoisting of var).
> > - Sloppy eval with var declaration. :)
> > - Do in some more esoteric positions, like default parameters, decl
> > initialisers, `return` -- especially do that throws or has var declarations
> > - Deopts triggered inside the do.
> > - Conversely, OSR inside the do, i.e., a long-running for-loop that gets
> > optimised in the middle.
> > 
> > I'm fine with leaving these for follow-up CLs, just dropping a list of
> > everything that might be problematic and should eventually be tested. :)
> 
> About hoisting, do we want function declarations to be block-scoped all the
> time, or only for strict scopes?
> 
> It doesn't make a lot of sense for a function declaration to escape the |do|
> block, imo

I'd say function declarations in do expressions should always be block scoped,
and shouldn't escape the block, even for sloppy mode (there are no legacy
concerns here).

[caitp (gmail), 2015-10-15 14:26:24]

On 2015/10/15 14:21:58, adamk wrote:
> On 2015/10/15 14:18:09, caitp wrote:
> > On 2015/10/15 09:58:27, rossberg wrote:
> > > LGTM
> > > 
> > > https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc
> > > File src/typing.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc#newcode353
> > > src/typing.cc:353: RECURSE(VisitVariableProxy(expr->result()));
> > > On 2015/10/13 15:06:00, caitp wrote:
> > > > On 2015/10/13 10:44:33, rossberg wrote:
> > > > > This function should set the type of the do-node to that of the result
> > > > variable.
> > > > 
> > > > Done, I think.. It's not clear that the Proxy's bounds are always set to
> > > > anything useful
> > > 
> > > True, but that is generally true when you try to type JS. ;)
> > > 
> > >
> >
>
https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
> > > File test/mjsunit/internal-do-expressions.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-...
> > > test/mjsunit/internal-do-expressions.js:14: function TestBasic() {
> > > On 2015/10/13 15:06:00, caitp wrote:
> > > > On 2015/10/13 10:44:33, rossberg wrote:
> > > > > The most important thing to test is all sorts of statements within a
> > > > non-trivial
> > > > > expression context, i.e., one where the operand stack isn't empty:
> > > > > 
> > > > > x + do{...}
> > > > > f(do{...})
> > > > > 
> > > > > etc. I assume this will not work out of the box, in particular not
with
> > > > > deoptimisation.
> > > > 
> > > > For every case I've tried, it seems to work (although there was an issue
> in
> > > > crankshaft due to bailouts when visiting the `do` block that wasn't
> handled,
> > > > which I'll fix in the next patchset).
> > > > 
> > > > It's hard to get enough coverage for this, but I've added some more
tests,
> > and
> > > > some suggestions for others would be very helpful
> > > 
> > > Some other scenarios to test:
> > > 
> > > - Uses of break and continue inside the do (i.e., those that don't escape
--
> I
> > > think they should work out of the box).
> > > - Uses of throw inside the do, both caught inside or outside the do.
> > > - Various completion values (cf. the existing completion.js).
> > > - Different forms of declarations inside the do (esp. hoisting of var).
> > > - Sloppy eval with var declaration. :)
> > > - Do in some more esoteric positions, like default parameters, decl
> > > initialisers, `return` -- especially do that throws or has var
declarations
> > > - Deopts triggered inside the do.
> > > - Conversely, OSR inside the do, i.e., a long-running for-loop that gets
> > > optimised in the middle.
> > > 
> > > I'm fine with leaving these for follow-up CLs, just dropping a list of
> > > everything that might be problematic and should eventually be tested. :)
> > 
> > About hoisting, do we want function declarations to be block-scoped all the
> > time, or only for strict scopes?
> > 
> > It doesn't make a lot of sense for a function declaration to escape the |do|
> > block, imo
> 
> I'd say function declarations in do expressions should always be block scoped,
> and shouldn't escape the block, even for sloppy mode (there are no legacy
> concerns here).

Fair enough --- I think that's a followup commit though, since it needs some
extra logic in ParseFunctionDeclaration

[caitp (gmail), 2015-10-15 16:00:26]

I've added a bunch more tests (though more are needed for sure), and added some
TODO comments to address separately.

[adamk, 2015-10-15 17:06:56]

lgtm as a first shot. Still wishing var declarations weren't allowed :)

[caitp (gmail), 2015-10-15 17:08:22]

On 2015/10/15 17:06:56, adamk wrote:
> lgtm as a first shot. Still wishing var declarations weren't allowed :)

I think we can do that, but it probably needs to be incorporated in the proposal

[adamk, 2015-10-15 17:09:16]

Yeah, sorry, I realize it'd have to be part of the proposal. Just wishful
thinking at the moment.

[caitp (gmail), 2015-10-15 17:10:17]

On 2015/10/15 17:09:16, adamk wrote:
> Yeah, sorry, I realize it'd have to be part of the proposal. Just wishful
> thinking at the moment.

Yep =) Alright, CQing and hopefully will get started on the followups in a
moment

[caitp (gmail), 2015-10-15 17:10:23]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-10-15 17:10:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1399893002/#ps260001
(title: "Add more test cases + some TODOs")

[commit-bot: I haz the power, 2015-10-15 17:10:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1399893002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1399893002/260001

[commit-bot: I haz the power, 2015-10-15 17:11:30]

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

[commit-bot: I haz the power, 2015-10-15 17:11:30]

Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/10602)

[caitp (gmail), 2015-10-15 17:21:09]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-10-15 17:21:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org, adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1399893002/#ps280001
(title: "Rebased")

[commit-bot: I haz the power, 2015-10-15 17:21:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1399893002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1399893002/280001

[commit-bot: I haz the power, 2015-10-15 17:23:23]

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

[commit-bot: I haz the power, 2015-10-15 17:23:24]

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

[caitp (gmail), 2015-10-15 17:24:16]

On 2015/10/15 17:23:24, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_presubmit on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/6721)

I guess I need a TF reviewer to sign off on it too

[rossberg, 2015-10-16 12:09:41]

Benedikt, can you look at the TF parts?

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
File test/mjsunit/harmony/do-expressions.js (right):

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
test/mjsunit/harmony/do-expressions.js:55: for (var i = 0; i < 10; ++i) (do { if
(i === 3) break; });
I'm mildly surprised that this one actually works right now. I bet it goes
haywire when you wrap some expression around the do (though you probably need
more than just return afterwards to observe the mess).

What I guess already works, though, is a break to a target local to the same do,
even if that do has an expression around it.

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
test/mjsunit/harmony/do-expressions.js:64: if ((i & 1) === 0) continue;
Same here.

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
test/mjsunit/harmony/do-expressions.js:210: function TestHoisting() {
Can you add a test with do-var inside a parameter initialiser?

[caitp (gmail), 2015-10-16 12:21:00]

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
File test/mjsunit/harmony/do-expressions.js (right):

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
test/mjsunit/harmony/do-expressions.js:210: function TestHoisting() {
On 2015/10/16 12:09:40, rossberg wrote:
> Can you add a test with do-var inside a parameter initialiser?

I don't expect it to work the way I'd think it would right now ---

EG:

```
function f(a = x, b = do { var x = "foo" }) {
  return a;
}
// a is initialized before hoisted `x` is initialized
assertEquals(undefined, f());

// x is hoisted and can be referenced from outside the function
assertEquals("foo", x);
```

But right now it doesn't end up working this way (x isn't hoisted to the right
declaration scope)

Can I mark this as a TODO? I think it would be fixed by building on top of
Adam's initializer scoping CL

[caitp (gmail), 2015-10-16 12:21:53]

On 2015/10/16 12:21:00, caitp wrote:
>
https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
> File test/mjsunit/harmony/do-expressions.js (right):
> 
>
https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
> test/mjsunit/harmony/do-expressions.js:210: function TestHoisting() {
> On 2015/10/16 12:09:40, rossberg wrote:
> > Can you add a test with do-var inside a parameter initialiser?
> 
> I don't expect it to work the way I'd think it would right now ---
> 
> EG:
> 
> ```
> function f(a = x, b = do { var x = "foo" }) {
>   return a;
> }
> // a is initialized before hoisted `x` is initialized
> assertEquals(undefined, f());
> 
> // x is hoisted and can be referenced from outside the function
> assertEquals("foo", x);
> ```
> 
> But right now it doesn't end up working this way (x isn't hoisted to the right
> declaration scope)
> 
> Can I mark this as a TODO? I think it would be fixed by building on top of
> Adam's initializer scoping CL

Minor edit, `b = do { var x = "foo"; x }`

[rossberg, 2015-10-16 12:37:02]

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
File test/mjsunit/harmony/do-expressions.js (right):

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
test/mjsunit/harmony/do-expressions.js:210: function TestHoisting() {
On 2015/10/16 12:21:00, caitp wrote:
> On 2015/10/16 12:09:40, rossberg wrote:
> > Can you add a test with do-var inside a parameter initialiser?
> 
> I don't expect it to work the way I'd think it would right now ---
> 
> EG:
> 
> ```
> function f(a = x, b = do { var x = "foo" }) {
>   return a;
> }
> // a is initialized before hoisted `x` is initialized
> assertEquals(undefined, f());
> 
> // x is hoisted and can be referenced from outside the function
> assertEquals("foo", x);
> ```
> 
> But right now it doesn't end up working this way (x isn't hoisted to the right
> declaration scope)
> 
> Can I mark this as a TODO? I think it would be fixed by building on top of
> Adam's initializer scoping CL

Sure, TODO is fine, just was curious in how far it works already.

FWIW, the ES6 semantics already is to introduce a separate var scope around each
parameter (remember sloppy eval!). Consequently, I don't think your example is
supposed to work the way you describe it. The expected behaviour consistent with
ES6 would be:
```
let x = 1

function f(a = do { var x = 2 }, b = x) { return b }
assertEquals(1, f())

function g(a = x, b = do { var x = 2 }) { return a }
assertEquals(1, g())

function h({a = do { var x = 2 }, b = x}) { return b }
assertEquals(2, h())

function i({a = x, b = do { var x = 2 }}) { return b }
assertEquals(undefined, i())

function j(a = do { var y = 0 }) {}
assertThrows(() => y, ReferenceError)
```
Essentially, the dynamic behaviour should be mostly like for an eval in place of
the do, except that vars are created immediately, as observed by i().

[caitp (gmail), 2015-10-16 13:12:19]

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
File test/mjsunit/harmony/do-expressions.js (right):

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
test/mjsunit/harmony/do-expressions.js:55: for (var i = 0; i < 10; ++i) (do { if
(i === 3) break; });
On 2015/10/16 12:09:40, rossberg wrote:
> I'm mildly surprised that this one actually works right now. I bet it goes
> haywire when you wrap some expression around the do (though you probably need
> more than just return afterwards to observe the mess).
> 
> What I guess already works, though, is a break to a target local to the same
do,
> even if that do has an expression around it.

From local testing, it seems to be working fine --- or at least isn't causing
any crashes. It's possible that the operand stack isn't being unwound correctly,
though. The behaviour is that the expression isn't applied when the
break/continue occurs. I'm not sure how big of a test it would take to cause a
crash if the operand stack isn't being unwound correctly, or if it depends on
the operation being performed, either.

Either way, will push a bigger test

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
test/mjsunit/harmony/do-expressions.js:210: function TestHoisting() {
Yeah, you're right. Then this doesn't need a TODO because IIRC this already
works, fixing up the tests

[caitp (gmail), 2015-10-16 13:23:03]

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
File test/mjsunit/harmony/do-expressions.js (right):

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
test/mjsunit/harmony/do-expressions.js:210: function TestHoisting() {
On 2015/10/16 12:37:02, rossberg wrote:
> On 2015/10/16 12:21:00, caitp wrote:
> > On 2015/10/16 12:09:40, rossberg wrote:
> > > Can you add a test with do-var inside a parameter initialiser?
> > 
> > I don't expect it to work the way I'd think it would right now ---
> > 
> > EG:
> > 
> > ```
> > function f(a = x, b = do { var x = "foo" }) {
> >   return a;
> > }
> > // a is initialized before hoisted `x` is initialized
> > assertEquals(undefined, f());
> > 
> > // x is hoisted and can be referenced from outside the function
> > assertEquals("foo", x);
> > ```
> > 
> > But right now it doesn't end up working this way (x isn't hoisted to the
right
> > declaration scope)
> > 
> > Can I mark this as a TODO? I think it would be fixed by building on top of
> > Adam's initializer scoping CL
> 
> Sure, TODO is fine, just was curious in how far it works already.
> 
> FWIW, the ES6 semantics already is to introduce a separate var scope around
each
> parameter (remember sloppy eval!). Consequently, I don't think your example is
> supposed to work the way you describe it. The expected behaviour consistent
with
> ES6 would be:
> ```
> let x = 1
> 
> function f(a = do { var x = 2 }, b = x) { return b }
> assertEquals(1, f())
> 
> function g(a = x, b = do { var x = 2 }) { return a }
> assertEquals(1, g())
> 
> function h({a = do { var x = 2 }, b = x}) { return b }
> assertEquals(2, h())

Is this actually right? Each parameter with an initializer gets its own
VariableEnvironment, so the declaration in the initializer for `a` shouldn't be
visible to the declaration for `b`, is that wrong? I would expect this to return
the value for `let x`, regardless of language mode

> 
> function i({a = x, b = do { var x = 2 }}) { return b }
> assertEquals(undefined, i())
> 
> function j(a = do { var y = 0 }) {}
> assertThrows(() => y, ReferenceError)
> ```
> Essentially, the dynamic behaviour should be mostly like for an eval in place
of
> the do, except that vars are created immediately, as observed by i().

[rossberg, 2015-10-16 13:28:28]

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
File test/mjsunit/harmony/do-expressions.js (right):

https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/d...
test/mjsunit/harmony/do-expressions.js:210: function TestHoisting() {
On 2015/10/16 13:23:03, caitp wrote:
> On 2015/10/16 12:37:02, rossberg wrote:
> > function h({a = do { var x = 2 }, b = x}) { return b }
> > assertEquals(2, h())
> 
> Is this actually right? Each parameter with an initializer gets its own
> VariableEnvironment, so the declaration in the initializer for `a` shouldn't
be
> visible to the declaration for `b`, is that wrong? I would expect this to
return
> the value for `let x`, regardless of language mode

Note the curly braces: this is a single parameter destructuring, which is how it
differs from the preceding examples.

[Benedikt Meurer, 2015-10-20 15:41:57]

LGTM for compiler.

[caitp (gmail), 2015-10-20 19:56:57]

On 2015/10/20 15:41:57, Benedikt Meurer wrote:
> LGTM for compiler.

Thanks, I appreciate it. CQing, and hoping it doesn't need to be reverted.

[caitp (gmail), 2015-10-20 19:57:05]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-10-20 19:57:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org, adamk@chromium.org, bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1399893002/#ps360001
(title: "Rebased")

[commit-bot: I haz the power, 2015-10-20 19:57:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1399893002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1399893002/360001

[commit-bot: I haz the power, 2015-10-20 20:11:05]

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

[commit-bot: I haz the power, 2015-10-20 20:11:06]

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

[caitp (gmail), 2015-10-20 20:13:05]

On 2015/10/20 20:11:06, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_mac_rel on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10909)

ah, there's the failures rossberg@ was expecting from the break/continue tests.
Lets see

[caitp (gmail), 2015-10-20 21:45:59]

On 2015/10/20 20:13:05, caitp wrote:
> On 2015/10/20 20:11:06, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   v8_mac_rel on tryserver.v8 (JOB_FAILED,
> > http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10909)
> 
> ah, there's the failures rossberg@ was expecting from the break/continue
tests.
> Lets see

I'm not 100% sure what has changed, but on a fresh build with patch set 16,
tests were fully passing with and without DCHECKs --- now the break/continue
thing crashes crankshaft with/without DCHECKS.

[caitp (gmail), 2015-10-20 22:17:26]

On 2015/10/20 21:45:59, caitp wrote:
> On 2015/10/20 20:13:05, caitp wrote:
> > On 2015/10/20 20:11:06, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   v8_mac_rel on tryserver.v8 (JOB_FAILED,
> > > http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10909)
> > 
> > ah, there's the failures rossberg@ was expecting from the break/continue
> tests.
> > Lets see
> 
> I'm not 100% sure what has changed, but on a fresh build with patch set 16,
> tests were fully passing with and without DCHECKs --- now the break/continue
> thing crashes crankshaft with/without DCHECKS.

And strangely, it doesn't seem to crash on my xeon machine, so that's probably
why I'm not seeing the issue from the main machine I was working from. Crashes
on the i7 though :(

[caitp (gmail), 2015-10-20 22:53:55]

On 2015/10/20 22:17:26, caitp wrote:
> On 2015/10/20 21:45:59, caitp wrote:
> > On 2015/10/20 20:13:05, caitp wrote:
> > > On 2015/10/20 20:11:06, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   v8_mac_rel on tryserver.v8 (JOB_FAILED,
> > > >
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10909)
> > > 
> > > ah, there's the failures rossberg@ was expecting from the break/continue
> > tests.
> > > Lets see
> > 
> > I'm not 100% sure what has changed, but on a fresh build with patch set 16,
> > tests were fully passing with and without DCHECKs --- now the break/continue
> > thing crashes crankshaft with/without DCHECKS.
> 
> And strangely, it doesn't seem to crash on my xeon machine, so that's probably
> why I'm not seeing the issue from the main machine I was working from. Crashes
> on the i7 though :(

With https://gist.github.com/caitp/589dcc90e80ddc37d3ef#file-scaryshaft-diff
applied, 100% of tests are passing locally.

But that change looks suspicious. It might not be so bad since crankshaft is
disappearing at some point, but I don't know.


What do people prefer, doing that, or just deopting?

[Benedikt Meurer (Google), 2015-10-21 02:15:21]

On 2015/10/20 22:53:55, caitp wrote:
> On 2015/10/20 22:17:26, caitp wrote:
> > On 2015/10/20 21:45:59, caitp wrote:
> > > On 2015/10/20 20:13:05, caitp wrote:
> > > > On 2015/10/20 20:11:06, commit-bot: I haz the power wrote:
> > > > > Try jobs failed on following builders:
> > > > >   v8_mac_rel on tryserver.v8 (JOB_FAILED,
> > > > >
> http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10909)
> > > > 
> > > > ah, there's the failures rossberg@ was expecting from the break/continue
> > > tests.
> > > > Lets see
> > > 
> > > I'm not 100% sure what has changed, but on a fresh build with patch set
16,
> > > tests were fully passing with and without DCHECKs --- now the
break/continue
> > > thing crashes crankshaft with/without DCHECKS.
> > 
> > And strangely, it doesn't seem to crash on my xeon machine, so that's
probably
> > why I'm not seeing the issue from the main machine I was working from.
Crashes
> > on the i7 though :(
> 
> With https://gist.github.com/caitp/589dcc90e80ddc37d3ef#file-scaryshaft-diff
> applied, 100% of tests are passing locally.
> 
> But that change looks suspicious. It might not be so bad since crankshaft is
> disappearing at some point, but I don't know.
> 
> 
> What do people prefer, doing that, or just deopting?

Can we please disable do expressions for Crankshaft (in Ast numbering), and only
support them in fullcodegen and TurboFan?

[caitp (gmail), 2015-10-21 02:16:38]

On 2015/10/21 02:15:21, Benedikt Meurer (Google) wrote:
> On 2015/10/20 22:53:55, caitp wrote:
> > On 2015/10/20 22:17:26, caitp wrote:
> > > On 2015/10/20 21:45:59, caitp wrote:
> > > > On 2015/10/20 20:13:05, caitp wrote:
> > > > > On 2015/10/20 20:11:06, commit-bot: I haz the power wrote:
> > > > > > Try jobs failed on following builders:
> > > > > >   v8_mac_rel on tryserver.v8 (JOB_FAILED,
> > > > > >
> > http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10909)
> > > > > 
> > > > > ah, there's the failures rossberg@ was expecting from the
break/continue
> > > > tests.
> > > > > Lets see
> > > > 
> > > > I'm not 100% sure what has changed, but on a fresh build with patch set
> 16,
> > > > tests were fully passing with and without DCHECKs --- now the
> break/continue
> > > > thing crashes crankshaft with/without DCHECKS.
> > > 
> > > And strangely, it doesn't seem to crash on my xeon machine, so that's
> probably
> > > why I'm not seeing the issue from the main machine I was working from.
> Crashes
> > > on the i7 though :(
> > 
> > With https://gist.github.com/caitp/589dcc90e80ddc37d3ef#file-scaryshaft-diff
> > applied, 100% of tests are passing locally.
> > 
> > But that change looks suspicious. It might not be so bad since crankshaft is
> > disappearing at some point, but I don't know.
> > 
> > 
> > What do people prefer, doing that, or just deopting?
> 
> Can we please disable do expressions for Crankshaft (in Ast numbering), and
only
> support them in fullcodegen and TurboFan?

Yes, that's the simplest thing

[caitp (gmail), 2015-10-21 02:31:43]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-10-21 02:31:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rossberg@chromium.org, adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1399893002/#ps380001
(title: "Disable CrankShaft")

[commit-bot: I haz the power, 2015-10-21 02:31:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1399893002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1399893002/380001

[commit-bot: I haz the power, 2015-10-21 02:55:54]

Committed patchset #18 (id:380001)

[commit-bot: I haz the power, 2015-10-21 02:58:43]

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

[adamk, 2015-10-21 09:04:36]

While disabling this for Crankshaft seems fine as a first step, I'd worry about
leaving it that way for long. Unless we want to make this is part of a plan to
get more JS code running through TurboFan.

[caitp (gmail), 2015-10-21 10:16:17]

On 2015/10/21 09:04:36, adamk wrote:
> While disabling this for Crankshaft seems fine as a first step, I'd worry
about
> leaving it that way for long. Unless we want to make this is part of a plan to
> get more JS code running through TurboFan.

I think I know how to properly fix the issue (making the BreakAndContinueInfo
that gets produced include the drop_extra equal to the amount of pushed values
at the time the DoExpression block is reached).
I'm fairly sure that should get it doing the right thing, most likely.

I can try it and see?

[Benedikt Meurer, 2015-10-21 10:30:14]

I'd be really in favor of not adding anything ES6ish/newish to Crankshaft at
all. We decided to explicitly not include for-of in Crankshaft, and I think the
same argument applies here. It's against the TurboFan is the ES6 compiler OKR.

[caitp (gmail), 2015-10-21 10:35:04]

On 2015/10/21 10:30:14, Benedikt Meurer wrote:
> I'd be really in favor of not adding anything ES6ish/newish to Crankshaft at
> all. We decided to explicitly not include for-of in Crankshaft, and I think
the
> same argument applies here. It's against the TurboFan is the ES6 compiler OKR.

The fix is incredibly small (adding an extra `drop_extra` parameter to
VisitBlock, making an extra VisitBlock with no extra argument that just inlines
a call to the other with a value of 0, and calling it with
environment()->push_count() in VisitDoExpression, which was like 2 lines of code
before anyways).

It seems to work beautifully. But if it's too much, we don't need to land it.

[rossberg, 2015-10-21 10:40:21]

On 2015/10/21 10:16:17, caitp wrote:
> On 2015/10/21 09:04:36, adamk wrote:
> > While disabling this for Crankshaft seems fine as a first step, I'd worry
> about
> > leaving it that way for long. Unless we want to make this is part of a plan
to
> > get more JS code running through TurboFan.
> 
> I think I know how to properly fix the issue (making the BreakAndContinueInfo
> that gets produced include the drop_extra equal to the amount of pushed values
> at the time the DoExpression block is reached).
> I'm fairly sure that should get it doing the right thing, most likely.
> 
> I can try it and see?

The plan of record still is not to waste time on Crankshaft for new features.
The current motivation for do-expressions in particular is to use it for
desugaring certain constructs into for-of loops. Those are not supported by
Crankshaft either, so there is little point.

As for dealing with the operand stack, that is gonna be a substantial amount of
work, in particular in full-codegen, with some subtleties. The reason we decided
to ignore control transfer for now is that we don't want to go down that rat
hole just now. It's fine if break/continue/return inside a `do` don't work for
now.

[caitp (gmail), 2015-10-21 10:47:33]

On 2015/10/21 10:40:21, rossberg wrote:
> On 2015/10/21 10:16:17, caitp wrote:
> > On 2015/10/21 09:04:36, adamk wrote:
> > > While disabling this for Crankshaft seems fine as a first step, I'd worry
> > about
> > > leaving it that way for long. Unless we want to make this is part of a
plan
> to
> > > get more JS code running through TurboFan.
> > 
> > I think I know how to properly fix the issue (making the
BreakAndContinueInfo
> > that gets produced include the drop_extra equal to the amount of pushed
values
> > at the time the DoExpression block is reached).
> > I'm fairly sure that should get it doing the right thing, most likely.
> > 
> > I can try it and see?
> 
> The plan of record still is not to waste time on Crankshaft for new features.
> The current motivation for do-expressions in particular is to use it for
> desugaring certain constructs into for-of loops. Those are not supported by
> Crankshaft either, so there is little point.
> 
> As for dealing with the operand stack, that is gonna be a substantial amount
of
> work, in particular in full-codegen, with some subtleties. The reason we
decided
> to ignore control transfer for now is that we don't want to go down that rat
> hole just now. It's fine if break/continue/return inside a `do` don't work for
> now.

The fix I mentioned above isn't really even needed, it's a 1 line change adding
`environment()->push_count()` to the BreakAndContinueInfo in VisitBlock().

I'm not totally sure why this isn't crashing spectacularly in full-codegen
though, since it seems like the stack should be getting mixed up there.

[rossberg, 2015-10-21 10:51:50]

On 2015/10/21 10:47:33, caitp wrote:
> I'm not totally sure why this isn't crashing spectacularly in full-codegen
> though, since it seems like the stack should be getting mixed up there.

Probably because the tests don't exercise the stack very much yet. This is
unlikely to be easy. Let's keep the current work to the minimum we need for
destructuring assignment, and come back to it later.

[marja, 2016-07-13 12:08:03]

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

[marja, 2016-07-13 12:08:05]

This CL is a bit old, but I bumped into this question... maybe you people in cc
can answer? Thanks!

https://codereview.chromium.org/1399893002/diff/380001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1399893002/diff/380001/src/ast.h#newcode3615
src/ast.h:3615: return new (parser_zone_) DoExpression(parser_zone_, block,
result, pos);
Question: why is DoExpression in parser_zone_ and not in local_zone_?

[caitp, 2016-07-13 21:45:46]

On 2016/07/13 12:08:05, marja wrote:
> This CL is a bit old, but I bumped into this question... maybe you people in
cc
> can answer? Thanks!
> 
> https://codereview.chromium.org/1399893002/diff/380001/src/ast.h
> File src/ast.h (right):
> 
> https://codereview.chromium.org/1399893002/diff/380001/src/ast.h#newcode3615
> src/ast.h:3615: return new (parser_zone_) DoExpression(parser_zone_, block,
> result, pos);
> Question: why is DoExpression in parser_zone_ and not in local_zone_?

IIRC, it was necessary because of the delayed rewriting desugaring, so it could
work in lazily parsed functions. Can't recall, though.

[marja, 2016-07-14 09:57:36]

Hmm, I don't understand this. Where's the delayed rewriting desugaring
happening? ParseDoExpression calls Rewriter::Rewrite(Parser* parser,
DoExpression* expr, AstValueFactory* factory) but that's not delayed... And
where do we even get the DoExpression from, since its parent AST nodes will be
in the temp zone and thus discarded.

Afaics, it's potentially worrying that the Block* of the DoExpression will
reside in the temp zone which will then be discarded, so the DoExpression refers
to dead memory.