Fix error handling in dart2js async-await

Fix error handling in dart2js async-await

BUG= dartbug.com/22372
R=floitsch@google.com

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

[sigurdm, 2015-02-13 13:49:02]

sigurdm@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2015-02-13 15:30:07]

LGTM with comments.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/rewrite_async.dart (right):

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:13: import
'package:_internal/compiler/js_lib/shared/async_await_error_codes.dart'
I think we import it relative for embedded names. Please verify and do the same.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:14: as error_codes;
not 100% about the naming convention of imports. With or without "_".
(just make sure it's the right one)

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:134: String yieldNextName;
nextWhenCanceledName ?

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:139: /// The thrown error (it might
need to be rethrown after a finally block).
currentErrorName ?

/// If we are in the process of handling an error, stores the current error.
String currentErrorName;

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:180: /// - The [bodyName]
The body function ([bodyName]). ?

Otherwise it sounds too much like we pass in a string.

Same for the others.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:582: "return
#thenHelper($returnValue, ${error_codes.SUCCESS}, "
Don't use "$" if you don't need to.

This is a value. Pass it in as expression.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:591: "return #streamHelper(null,
${error_codes.SUCCESS}, "
ditto.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:603: "return
#thenHelper($storedErrorName, ${error_codes.ERROR}, "
ditto.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:718: ///   var handler = 8; //
Outside try-blocks go to the rethrow label.
Handler is slightly weird, because we also use it for finallies.

For now it's probably ok to mention this at the sites where we assign a finally
as if it was a handler.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:752: ///             handler = 5; //
If we throw in this handler, do the finally
?
// If the handler throws, do the finally ...
// ... and rethrow.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:752: ///             handler = 5; //
If we throw in this handler, do the finally
?
// If the handler throws, do the finally ...
// ... and rethrow.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:845: if (isSyncStar) {
Not necessarily this CL, but explain why this is different.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:874: // fallthrough
Why fall through?
Is it to avoid repeating the $gotoName = $handlerName line? Or do you also need
the storedErrorName ?

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:955: "thenHelper": isAsync ?
asyncHelper : streamHelper,
rename "thenHelper" to "asyncHelper" ?

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:1634: // And from that finally block,
go to all enclosing finally blocks until
If the catch-part throws visit the finally block first (if there is one).
Then visit all surrounding finallies, until there is a catch-handler or the
'rethrow' label if there is none.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:1759: /// distinguish.
distinguish them.

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right):

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3498: // The error will be a
js-error.
is a

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3553: dynamic /* int | js
function */ asyncBody,
bodyFunctionOrErrorCode ?

It's long, but not used often anyway.

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3555: if (asyncBody ==
async_error_codes.SUCCESS) {
nit.
use identical, or invert the order.
The asyncBody is marked as dynamic (which the inferrer cannot infer). This means
that it currently needs to get an interceptor, ...
If you switch to "identical" or invert the order, it produces much better code.

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3559: } else if (asyncBody ==
async_error_codes.ERROR) {
ditto.

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3560: // The error will be a
js-error.
is

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3631: JS('', '#(#,
null)',helperCallback, async_error_codes.SUCCESS);
missing space.

https://codereview.chromium.org/925973002/diff/20001/tests/language/await_thr...
File tests/language/await_throw_in_catch_test.dart (right):

https://codereview.chromium.org/925973002/diff/20001/tests/language/await_thr...
tests/language/await_throw_in_catch_test.dart:16: throw "Error2";
before this line.
Expect.isFalse(finallyRun);

https://codereview.chromium.org/925973002/diff/20001/tests/language/await_thr...
tests/language/await_throw_in_catch_test.dart:16: throw "Error2";
Add boolean to make sure we entered this catch.
Or better: don't use booleans but integers to make sure the blocks are not
executed multiple times. (also for finallyRun).

https://codereview.chromium.org/925973002/diff/20001/tests/language/await_thr...
tests/language/await_throw_in_catch_test.dart:56: Expect.equals("Error2", e);
indentation.
ditto for the others.

https://codereview.chromium.org/925973002/diff/20001/tests/language/await_thr...
tests/language/await_throw_in_catch_test.dart:56: Expect.equals("Error2", e);
Make sure this 'catch' is executed.
ditto for the others.

https://codereview.chromium.org/925973002/diff/20001/tests/language/await_thr...
tests/language/await_throw_in_catch_test.dart:72: }
Add a few more tests:

try {
  try {
    throw;
  } catch(e) {
    throw;
  }
} catch(e) {
  X
}

try {
  try {
    throw;
  } catch(e) {
    throw;
  }
} finally {
  X
}

try {
  try {
    throw;
  } catch(e) {
    throw;
  } finally {
    throw;
  }
} catch(e) {
  X
}

try {
  try {
    throw;
  } catch(e) {
    throw;
  } finally {
    throw;
  }
} finally {
  X
}

Instead of 'throw' also have 'return', 'continue' and 'break'.
Add the same tests with "catch" and "finally" blocks.
Test with more nested finallies.
Test with loops around the try/catch.

[sigurdm, 2015-02-16 16:18:27]

New patchsets have been uploaded after l-g-t-m from floitsch@google.com

[floitsch, 2015-02-16 18:26:36]

Still LGTM.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/rewrite_async.dart (right):

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:46: /// The label of the catch
handler of a [js.Try] or a [js.Fun] or [js.Catch].
Not sure I understand the comment.
is it ([js.Try] or a [js.Fun] or [js.Catch]), or
(the catch handler of a [js.Try]), or a [js.Fun] or [js.Catch].?

In any case it's not clear.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:57: /// The jump-targets include the
function for
the function, representing the target of a return.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:61: /// an uncaught error.
That sounds like the function is used either for the "return" or for rethrowing
uncaught errors. correct?

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:65: List<js.Node>
targetsCatchesAndFinallies = new List<js.Node>();
/// A stack of all (surrounding) jump targets.
///
/// Jump targets are:
///
/// * The function, signalling a return or uncaught throw.
/// * Loops.
/// * LabeledStatements (also used for 'continue' when attached to loops).
/// * Try statements, for catch and finally handlers.
///
/// When jumping to a target it is necessary to visit all finallies that
/// are on the way to target (i.e. more nested than the jump target).
List<js.Node> jumpTargetStack = new List<js.Node>();

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:178: /// - The [completerName]
nit: the completer object [completerName]

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:184: /// - The [completerName]
ditto.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:302: currentErrorName =
freshName("storedError");
freshName("currentError")

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:967: $bodyName,
nit: indentation.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:1631: handlerLabels[node.catchPart] =
uncaughtLabel;
This needs a comment.
Could you delay this until line 1658?

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:1693: // enclosingFinallies can be
empty if
Unfinished comment.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:1803: /// Also a
[nextWhenCanceledName] is set up to
Unfinished sentence.

[sigurdm, 2015-02-17 08:41:39]

New patchsets have been uploaded after l-g-t-m from floitsch@google.com

[sigurdm, 2015-02-17 08:43:11]

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/rewrite_async.dart (right):

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:13: import
'package:_internal/compiler/js_lib/shared/async_await_error_codes.dart'
On 2015/02/13 15:30:06, floitsch wrote:
> I think we import it relative for embedded names. Please verify and do the
same.

At least in js_backend/js_backend.dart it is imported as a package-import.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:14: as error_codes;
On 2015/02/13 15:30:07, floitsch wrote:
> not 100% about the naming convention of imports. With or without "_".
> (just make sure it's the right one)

The style guide says lower_case_underscores.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:134: String yieldNextName;
On 2015/02/13 15:30:06, floitsch wrote:
> nextWhenCanceledName ?

Better, Donde

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:139: /// The thrown error (it might
need to be rethrown after a finally block).
On 2015/02/13 15:30:06, floitsch wrote:
> currentErrorName ?
> 
> /// If we are in the process of handling an error, stores the current error.
> String currentErrorName;

Done.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:180: /// - The [bodyName]
On 2015/02/13 15:30:06, floitsch wrote:
> The body function ([bodyName]). ?
> 
> Otherwise it sounds too much like we pass in a string.
> 
> Same for the others.

Done.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:582: "return
#thenHelper($returnValue, ${error_codes.SUCCESS}, "
On 2015/02/13 15:30:06, floitsch wrote:
> Don't use "$" if you don't need to.
> 
> This is a value. Pass it in as expression.

Done.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:591: "return #streamHelper(null,
${error_codes.SUCCESS}, "
On 2015/02/13 15:30:06, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:603: "return
#thenHelper($storedErrorName, ${error_codes.ERROR}, "
On 2015/02/13 15:30:06, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:718: ///   var handler = 8; //
Outside try-blocks go to the rethrow label.
On 2015/02/13 15:30:06, floitsch wrote:
> Handler is slightly weird, because we also use it for finallies.
> 
> For now it's probably ok to mention this at the sites where we assign a
finally
> as if it was a handler.

Acknowledged. I mentioned it in visitTry.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:752: ///             handler = 5; //
If we throw in this handler, do the finally
On 2015/02/13 15:30:07, floitsch wrote:
> ?
> // If the handler throws, do the finally ...
> // ... and rethrow.

Done.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:752: ///             handler = 5; //
If we throw in this handler, do the finally
On 2015/02/13 15:30:06, floitsch wrote:
> ?
> // If the handler throws, do the finally ...
> // ... and rethrow.

Acknowledged.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:845: if (isSyncStar) {
On 2015/02/13 15:30:06, floitsch wrote:
> Not necessarily this CL, but explain why this is different.

Added todo

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:874: // fallthrough
On 2015/02/13 15:30:06, floitsch wrote:
> Why fall through?
> Is it to avoid repeating the $gotoName = $handlerName line? Or do you also
need
> the storedErrorName ?

Only for the handler. I changed the code, so it does not fall through anymore.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:955: "thenHelper": isAsync ?
asyncHelper : streamHelper,
On 2015/02/13 15:30:07, floitsch wrote:
> rename "thenHelper" to "asyncHelper" ?

Done.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:1634: // And from that finally block,
go to all enclosing finally blocks until
On 2015/02/13 15:30:06, floitsch wrote:
> If the catch-part throws visit the finally block first (if there is one).
> Then visit all surrounding finallies, until there is a catch-handler or the
> 'rethrow' label if there is none.

Done.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:1759: /// distinguish.
On 2015/02/13 15:30:06, floitsch wrote:
> distinguish them.

Done.

https://codereview.chromium.org/925973002/diff/20001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:1769:
setErrorHandler(enclosingFinallyLabels.removeLast());
I realized that I forget to reset the error-handler after the yield.

So now I will instead have all the finally-labels in nextWhenCanceledName and
pop from that before the switch.

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right):

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3498: // The error will be a
js-error.
On 2015/02/13 15:30:07, floitsch wrote:
> is a

Done.

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3553: dynamic /* int | js
function */ asyncBody,
On 2015/02/13 15:30:07, floitsch wrote:
> bodyFunctionOrErrorCode ?
> 
> It's long, but not used often anyway.

Good point. I will use identical.

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3555: if (asyncBody ==
async_error_codes.SUCCESS) {
On 2015/02/13 15:30:07, floitsch wrote:
> nit.
> use identical, or invert the order.
> The asyncBody is marked as dynamic (which the inferrer cannot infer). This
means
> that it currently needs to get an interceptor, ...
> If you switch to "identical" or invert the order, it produces much better
code.

Done.

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3559: } else if (asyncBody ==
async_error_codes.ERROR) {
On 2015/02/13 15:30:07, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3560: // The error will be a
js-error.
On 2015/02/13 15:30:07, floitsch wrote:
> is

Done.

https://codereview.chromium.org/925973002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3631: JS('', '#(#,
null)',helperCallback, async_error_codes.SUCCESS);
On 2015/02/13 15:30:07, floitsch wrote:
> missing space.

Done.

https://codereview.chromium.org/925973002/diff/20001/tests/language/await_thr...
File tests/language/await_throw_in_catch_test.dart (right):

https://codereview.chromium.org/925973002/diff/20001/tests/language/await_thr...
tests/language/await_throw_in_catch_test.dart:16: throw "Error2";
On 2015/02/13 15:30:07, floitsch wrote:
> Add boolean to make sure we entered this catch.
> Or better: don't use booleans but integers to make sure the blocks are not
> executed multiple times. (also for finallyRun).

Done.

https://codereview.chromium.org/925973002/diff/20001/tests/language/await_thr...
tests/language/await_throw_in_catch_test.dart:16: throw "Error2";
On 2015/02/13 15:30:07, floitsch wrote:
> before this line.
> Expect.isFalse(finallyRun);

Done.

https://codereview.chromium.org/925973002/diff/20001/tests/language/await_thr...
tests/language/await_throw_in_catch_test.dart:56: Expect.equals("Error2", e);
On 2015/02/13 15:30:07, floitsch wrote:
> Make sure this 'catch' is executed.
> ditto for the others.

Done.

https://codereview.chromium.org/925973002/diff/20001/tests/language/await_thr...
tests/language/await_throw_in_catch_test.dart:72: }
On 2015/02/13 15:30:07, floitsch wrote:
> Add a few more tests:
> 
> try {
>   try {
>     throw;
>   } catch(e) {
>     throw;
>   }
> } catch(e) {
>   X
> }
> 
> try {
>   try {
>     throw;
>   } catch(e) {
>     throw;
>   }
> } finally {
>   X
> }
> 
> try {
>   try {
>     throw;
>   } catch(e) {
>     throw;
>   } finally {
>     throw;
>   }
> } catch(e) {
>   X
> }
> 
> try {
>   try {
>     throw;
>   } catch(e) {
>     throw;
>   } finally {
>     throw;
>   }
> } finally {
>   X
> }
> 
> Instead of 'throw' also have 'return', 'continue' and 'break'.
> Add the same tests with "catch" and "finally" blocks.
> Test with more nested finallies.
> Test with loops around the try/catch.

Done.

[sigurdm, 2015-02-17 10:29:06]

PTAL on the test-changes.

Thanks!

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
File pkg/compiler/lib/src/js/rewrite_async.dart (right):

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:46: /// The label of the catch
handler of a [js.Try] or a [js.Fun] or [js.Catch].
On 2015/02/16 18:26:36, floitsch wrote:
> Not sure I understand the comment.
> is it ([js.Try] or a [js.Fun] or [js.Catch]), or
> (the catch handler of a [js.Try]), or a [js.Fun] or [js.Catch].?
> 
> In any case it's not clear.

I try to explain more in depth.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:57: /// The jump-targets include the
function for
On 2015/02/16 18:26:36, floitsch wrote:
> the function, representing the target of a return.

Done.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:61: /// an uncaught error.
On 2015/02/16 18:26:36, floitsch wrote:
> That sounds like the function is used either for the "return" or for
rethrowing
> uncaught errors. correct?

Yes, it is used for both.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:65: List<js.Node>
targetsCatchesAndFinallies = new List<js.Node>();
On 2015/02/16 18:26:36, floitsch wrote:
> /// A stack of all (surrounding) jump targets.
> ///
> /// Jump targets are:
> ///
> /// * The function, signalling a return or uncaught throw.
> /// * Loops.
> /// * LabeledStatements (also used for 'continue' when attached to loops).
> /// * Try statements, for catch and finally handlers.
> ///
> /// When jumping to a target it is necessary to visit all finallies that
> /// are on the way to target (i.e. more nested than the jump target).
> List<js.Node> jumpTargetStack = new List<js.Node>();

This is not very precise. But might be easier to understand.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:178: /// - The [completerName]
On 2015/02/16 18:26:36, floitsch wrote:
> nit: the completer object [completerName]

Done.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:184: /// - The [completerName]
On 2015/02/16 18:26:36, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:302: currentErrorName =
freshName("storedError");
On 2015/02/16 18:26:36, floitsch wrote:
> freshName("currentError")

Done.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:967: $bodyName,
On 2015/02/16 18:26:36, floitsch wrote:
> nit: indentation.

Done.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:1631: handlerLabels[node.catchPart] =
uncaughtLabel;
On 2015/02/16 18:26:36, floitsch wrote:
> This needs a comment.
Comment added
> Could you delay this until line 1658?

Yes, it belongs better there.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:1693: // enclosingFinallies can be
empty if
On 2015/02/16 18:26:36, floitsch wrote:
> Unfinished comment.

Done.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:1693: // enclosingFinallies can be
empty if
On 2015/02/16 18:26:36, floitsch wrote:
> Unfinished comment.

Done.

https://codereview.chromium.org/925973002/diff/60001/pkg/compiler/lib/src/js/...
pkg/compiler/lib/src/js/rewrite_async.dart:1803: /// Also a
[nextWhenCanceledName] is set up to
On 2015/02/16 18:26:36, floitsch wrote:
> Unfinished sentence.

Done.

https://codereview.chromium.org/925973002/diff/100001/tests/language/sync_gen...
File tests/language/sync_generator3_test.dart (left):

https://codereview.chromium.org/925973002/diff/100001/tests/language/sync_gen...
tests/language/sync_generator3_test.dart:39: s = g().toString();
I believer this test was wrong (the vm currently fails it for another reason).
g().toString() should not be able to throw *and* return something that can be
assigned to s.

[floitsch, 2015-02-17 12:40:59]

Still LGTM.

https://codereview.chromium.org/925973002/diff/140001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right):

https://codereview.chromium.org/925973002/diff/140001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3708: // Rely on [_helper] to
repeatedly return `ITERATION_ENDED`.
_body ?

https://codereview.chromium.org/925973002/diff/140001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3708: // Rely on [_helper] to
repeatedly return `ITERATION_ENDED`.
Do you mean that the iterator doesn't need to save this state, since the [_body]
already does it for you?

https://codereview.chromium.org/925973002/diff/140001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3711: // Rely on [_helper] to
repeatedly return `UNCAUGHT_ERROR`.
_body ?

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
File tests/language/async_throw_in_catch_test.dart (right):

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:29: foo1(Tracer tracer) async {
You could consider removing the "async" in the /// forceAwait case too.
(shouldn't change anything, though).

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:32: // This await  forces dart2js
to rewrite the try into a state machine
nit nit. 2 spaces after "await".

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:85: await new Future.value(3);
/// forceAwait: continue ?

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:132: throw "Error3";
Please add a few tests, where the "finally" does a "return". (I see throws, but
no returns).
('continue' would be nice too, but there are at least some 'break'-in-finally
tests)

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:204: foo10(Tracer tracer) async {
does the same as foo9.

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:341: try {
You could avoid a lot of the boilerplate:

runTest(String expected, String expectedError, fun) async {
  Tracer tracer = new Tracer(expected);
  try {
    await fun(tracer);
  } catch(e) {
    Expect.equals(expectedError, e);
    tracer.add("X");
  }
  tracer.done();
}

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:344: Expect.equals("Error2", e);
You should trace the catching.

[sigurdm, 2015-02-17 14:19:02]

New patchsets have been uploaded after l-g-t-m from floitsch@google.com

[sigurdm, 2015-02-17 14:19:49]

https://codereview.chromium.org/925973002/diff/140001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right):

https://codereview.chromium.org/925973002/diff/140001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3708: // Rely on [_helper] to
repeatedly return `ITERATION_ENDED`.
On 2015/02/17 12:40:58, floitsch wrote:
> Do you mean that the iterator doesn't need to save this state, since the
[_body]
> already does it for you?

Yes

https://codereview.chromium.org/925973002/diff/140001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3708: // Rely on [_helper] to
repeatedly return `ITERATION_ENDED`.
On 2015/02/17 12:40:58, floitsch wrote:
> _body ?

Done.

https://codereview.chromium.org/925973002/diff/140001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:3711: // Rely on [_helper] to
repeatedly return `UNCAUGHT_ERROR`.
On 2015/02/17 12:40:58, floitsch wrote:
> _body ?

Done.

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
File tests/language/async_throw_in_catch_test.dart (right):

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:29: foo1(Tracer tracer) async {
On 2015/02/17 12:40:58, floitsch wrote:
> You could consider removing the "async" in the /// forceAwait case too.
> (shouldn't change anything, though).

I want to check that the async rewrite works properly, even though it leaves
some parts untranslated

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:32: // This await  forces dart2js
to rewrite the try into a state machine
On 2015/02/17 12:40:58, floitsch wrote:
> nit nit. 2 spaces after "await".

Done.

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:85: await new Future.value(3);
On 2015/02/17 12:40:58, floitsch wrote:
> /// forceAwait: continue ?

Done.

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:132: throw "Error3";
On 2015/02/17 12:40:58, floitsch wrote:
> Please add a few tests, where the "finally" does a "return". (I see throws,
but
> no returns).
> ('continue' would be nice too, but there are at least some 'break'-in-finally
> tests)

Done.

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:204: foo10(Tracer tracer) async {
On 2015/02/17 12:40:58, floitsch wrote:
> does the same as foo9.

Removed

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:341: try {
On 2015/02/17 12:40:58, floitsch wrote:
> You could avoid a lot of the boilerplate:
> 
> runTest(String expected, String expectedError, fun) async {
>   Tracer tracer = new Tracer(expected);
>   try {
>     await fun(tracer);
>   } catch(e) {
>     Expect.equals(expectedError, e);
>     tracer.add("X");
>   }
>   tracer.done();
> }

Done.

https://codereview.chromium.org/925973002/diff/140001/tests/language/async_th...
tests/language/async_throw_in_catch_test.dart:344: Expect.equals("Error2", e);
On 2015/02/17 12:40:58, floitsch wrote:
> You should trace the catching.

Done.

[sigurdm, 2015-02-17 14:52:08]

Committed patchset #10 (id:180001) manually as 43812 (presubmit successful).