Fix async machinery (issue 22445 and possibly others to be triaged later).

Fix async machinery (issue 22445 and possibly others to be triaged later).

R=hausner@google.com, iposva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=44107

[regis, 2015-02-27 01:48:24]

regis@google.com changed reviewers:
+ hausner@google.com, iposva@google.com

[Ivan Posva, 2015-02-27 10:23:43]

First set of comments and questions.

-Ivan

https://codereview.chromium.org/958243003/diff/1/runtime/vm/ast_transformer.cc
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/958243003/diff/1/runtime/vm/ast_transformer.c...
runtime/vm/ast_transformer.cc:116: //                   existent).
This second half of the comment makes it sound as if the distinction between
try, catch or finally blocks is being done here, but this is just a helper
function.

https://codereview.chromium.org/958243003/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/958243003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:7765: void Parser::CheckAwaitInTryBlock(
CheckAsyncOpInTryBlock as it is either await or yield that need this extra
handling.

https://codereview.chromium.org/958243003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:7782: if (try_blocks_list_->inside_catch() &&
Shouldn't this if be nested inside the one above? Or is there really a case
where you expect to set *outer_try_scope, but not *try_scope?

Also you might want to expand on the fact that the checking for being
inside_catch() here is an optimization as it would not be wrong to restore the
outer value unconditionally. It is just only needed from within a catch (or
finally which is its own "catchall" since you have the potential of leaving the
current block in an unstructured fashion.

https://codereview.chromium.org/958243003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8256: if (saved_exception_var == NULL) {
How could this have already been defined before in this scope? Shouldn't this be
an assertion instead?

https://codereview.chromium.org/958243003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8266: if (saved_stack_trace_var == NULL) {
ditto

[Florian Schneider, 2015-02-27 11:52:52]

fschneider@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2015-02-27 11:52:53]

dbc: pretty complicated stuff - I wonder if we can simplify it by moving stuff
out of ast_transformer.cc into parser.cc. VisitAwaitNode for instance. I don't
see the reason to have this code separated from the parser.

Does it work with arbitrarily nested try-blocks? e.g. something like

try {
  await ...
  try {
     await ...

https://codereview.chromium.org/958243003/diff/1/runtime/vm/ast_transformer.cc
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/958243003/diff/1/runtime/vm/ast_transformer.c...
runtime/vm/ast_transformer.cc:246: node->try_scope(),
How about passing the correct LocalVariable objects instead the scope and
try_index? Maybe this would simplify the code not to have to repeat the lookup
and constructing of the correct variable name here since it is already done in
the parser.

[hausner, 2015-02-27 18:05:32]

LGTM. Nice cleanup step.

https://codereview.chromium.org/958243003/diff/1/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/958243003/diff/1/runtime/vm/ast.h#newcode173
runtime/vm/ast.h:173: outer_try_index_(outer_try_index) { }
I think an await node must always be contained in a try scope (but not
necessarily in an outer try scope). An async or async* has an implicitly created
try-catch around the function body. Maybe it would make sense to assert this
here, or in the parser where you look up the try scope.

https://codereview.chromium.org/958243003/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/958243003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:7765: void Parser::CheckAwaitInTryBlock(
On 2015/02/27 10:23:43, Ivan Posva wrote:
> CheckAsyncOpInTryBlock as it is either await or yield that need this extra
> handling.

DBC: It's not necessarily an async op either. Yield can be in a synchronous
generator. There is really nothing async specific here. This function just
gathers info about the current nesting of try scopes.

[regis, 2015-02-27 21:41:53]

PTAL

Thanks all,
Regis

https://codereview.chromium.org/958243003/diff/1/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/958243003/diff/1/runtime/vm/ast.h#newcode173
runtime/vm/ast.h:173: outer_try_index_(outer_try_index) { }
On 2015/02/27 18:05:31, hausner wrote:
> I think an await node must always be contained in a try scope (but not
> necessarily in an outer try scope). An async or async* has an implicitly
created
> try-catch around the function body. Maybe it would make sense to assert this
> here, or in the parser where you look up the try scope.

The AwaitNode is constructed when parsing both the async function (no wrapping
try) and the async closure (wrapping try). I added an assert in
Parser::CheckAsyncOpInTryBlock.

https://codereview.chromium.org/958243003/diff/1/runtime/vm/ast_transformer.cc
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/958243003/diff/1/runtime/vm/ast_transformer.c...
runtime/vm/ast_transformer.cc:116: //                   existent).
On 2015/02/27 10:23:43, Ivan Posva wrote:
> This second half of the comment makes it sound as if the distinction between
> try, catch or finally blocks is being done here, but this is just a helper
> function.

I've moved the second half of the comment to Parser::CheckAwaitInTryBlock (to be
renamed).

https://codereview.chromium.org/958243003/diff/1/runtime/vm/ast_transformer.c...
runtime/vm/ast_transformer.cc:246: node->try_scope(),
On 2015/02/27 11:52:53, Florian Schneider wrote:
> How about passing the correct LocalVariable objects instead the scope and
> try_index? Maybe this would simplify the code not to have to repeat the lookup
> and constructing of the correct variable name here since it is already done in
> the parser.

I am not sure that all variables are already setup correctly when we construct
the AwaitNode.
But I've added a TODO in Parser::CheckAwaitInTryBlock to check if it would be
possible.

https://codereview.chromium.org/958243003/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/958243003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:7765: void Parser::CheckAwaitInTryBlock(
On 2015/02/27 10:23:43, Ivan Posva wrote:
> CheckAsyncOpInTryBlock as it is either await or yield that need this extra
> handling.

Done. I was just keeping the current style :-)
I think we need a renaming pass over the source, since yield came after await.

https://codereview.chromium.org/958243003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:7765: void Parser::CheckAwaitInTryBlock(
On 2015/02/27 18:05:32, hausner wrote:
> On 2015/02/27 10:23:43, Ivan Posva wrote:
> > CheckAsyncOpInTryBlock as it is either await or yield that need this extra
> > handling.
> 
> DBC: It's not necessarily an async op either. Yield can be in a synchronous
> generator. There is really nothing async specific here. This function just
> gathers info about the current nesting of try scopes.

Acknowledged.

https://codereview.chromium.org/958243003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:7782: if (try_blocks_list_->inside_catch() &&
On 2015/02/27 10:23:43, Ivan Posva wrote:
> Shouldn't this if be nested inside the one above? Or is there really a case
> where you expect to set *outer_try_scope, but not *try_scope?
>
Done. The second function level check would always fail if the first one did,
but you are right that this is clearer and more efficient to nest.

 
> Also you might want to expand on the fact that the checking for being
> inside_catch() here is an optimization as it would not be wrong to restore the
> outer value unconditionally. It is just only needed from within a catch (or
> finally which is its own "catchall" since you have the potential of leaving
the
> current block in an unstructured fashion.

I moved a comment from the helper function to here and updated the portion
explaining finally. The key point is that the try block matching the finally has
already been popped when parsing finally.

https://codereview.chromium.org/958243003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8256: if (saved_exception_var == NULL) {
On 2015/02/27 10:23:43, Ivan Posva wrote:
> How could this have already been defined before in this scope? Shouldn't this
be
> an assertion instead?

No. Note that I moved these variables in the parent scope of where they were
earlier, because these need to be accessible in the finally clause (issue 22595
to be fixed).

Now, several parallel try statements will reuse the variables, as the
non-captured versions of these variables are reused too.
I added a comment here and where the non-captured versions are created.

https://codereview.chromium.org/958243003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8266: if (saved_stack_trace_var == NULL) {
On 2015/02/27 10:23:43, Ivan Posva wrote:
> ditto

ditto

[hausner, 2015-02-27 22:00:55]

Still LGTM.

[Ivan Posva, 2015-02-27 22:15:25]

LGTM -ip

https://codereview.chromium.org/958243003/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/958243003/diff/40001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:7787: if (scope->function_level() ==
current_function_level) {
We need to evaluate the need for this check.

[regis, 2015-02-27 22:21:33]

Committed patchset #3 (id:40001) manually as r44107 (presubmit successful).