Fix async machinery for finally clauses (issues 22445, 22300).

Fix async machinery for finally clauses (issues 22445, 22300).

R=hausner@google.com

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

[regis, 2015-03-03 22:42:15]

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

[Ivan Posva, 2015-03-03 22:57:56]

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

[Ivan Posva, 2015-03-03 22:57:57]

DBC (Matthias is doing the full review) -ip

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

https://codereview.chromium.org/974073002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8969: SaveExceptionAndStacktrace(async_code,
Please add comment what is being saved here and where this code is in relation
to the rest.

[hausner, 2015-03-03 23:19:12]

LGTM with comments/question. Nice cleanup and refactoring.

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

https://codereview.chromium.org/974073002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:6038: LocalVariable* saved_exception_var =
try_scope->LocalLookupVariable(
Nit: the initializer value for the variable may now fit on one line, so the line
can wrap after the =. Dirro below.

https://codereview.chromium.org/974073002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:6164: LocalVariable* context_var =
try_scope->LocalLookupVariable(
Nit: line break after =

https://codereview.chromium.org/974073002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8709:
SaveExceptionAndStacktrace(current_block_->statements,
Strictly speaking, this is only necessary if the finally block contains an await
or yield statement that suspends execution of the function. Correct? If so, it
might be worth adding a comment that we might want to "optimize" this at some
time in the future.

https://codereview.chromium.org/974073002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8955: if (current != NULL) {
Can this be simplified to if (is_async && (current != NULL))?

[regis, 2015-03-03 23:47:09]

PTAL

Matthias, there is a question for you in a new comment.

Thanks,
Regis

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

https://codereview.chromium.org/974073002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:6038: LocalVariable* saved_exception_var =
try_scope->LocalLookupVariable(
On 2015/03/03 23:19:11, hausner wrote:
> Nit: the initializer value for the variable may now fit on one line, so the
line
> can wrap after the =. Dirro below.

It does not.

https://codereview.chromium.org/974073002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:6164: LocalVariable* context_var =
try_scope->LocalLookupVariable(
On 2015/03/03 23:19:11, hausner wrote:
> Nit: line break after =

ditto

https://codereview.chromium.org/974073002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8709:
SaveExceptionAndStacktrace(current_block_->statements,
On 2015/03/03 23:19:11, hausner wrote:
> Strictly speaking, this is only necessary if the finally block contains an
await
> or yield statement that suspends execution of the function. Correct? If so, it
> might be worth adding a comment that we might want to "optimize" this at some
> time in the future.

Yes, I added a comment and a TODO(hausner) :-)

I also fixed a potential bug:
The call to RestoreSavedTryContext is only done if there is an outer try block,
but the saving of the exception variables is unconditional (even without an
outer try).

https://codereview.chromium.org/974073002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8955: if (current != NULL) {
On 2015/03/03 23:19:11, hausner wrote:
> Can this be simplified to if (is_async && (current != NULL))?

Done.

https://codereview.chromium.org/974073002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8969: SaveExceptionAndStacktrace(async_code,
On 2015/03/03 22:57:57, Ivan Posva wrote:
> Please add comment what is being saved here and where this code is in relation
> to the rest.

Done.

https://codereview.chromium.org/974073002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:9534: innermost_function().IsSyncGenerator()) {
Matthias, is the list above correct?

[regis, 2015-03-03 23:54:45]

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

https://codereview.chromium.org/974073002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:9534: innermost_function().IsSyncGenerator()) {
On 2015/03/03 23:47:09, regis wrote:
> Matthias, is the list above correct?

OK, I'm submitting, but we should verify all the locations where we check
against a list of innermost functions.

[regis, 2015-03-03 23:55:25]

Committed patchset #2 (id:20001) manually as r44210 (presubmit successful).

[hausner, 2015-03-03 23:59:40]

Good catch!

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

https://codereview.chromium.org/974073002/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:9534: innermost_function().IsSyncGenerator()) {
On 2015/03/03 23:47:09, regis wrote:
> Matthias, is the list above correct?

Good catch. It seems that we also need to include async* functions and closures,
so the if should be

if (innermost_function().IsAsyncClosure() ||
       innermost_function().IsAsyncFunction() ||
       innermost_function().IsSyncGenClosure() ||
       innermost_function().IsSyncGenerator() ||
       innermost_function().IsAsyncGenerator() ||
       innermost_function().IsAsyncGenClosure()) {