Implement async* functions in VM

Implement async* functions in VM

R=iposva@google.com

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

[Ivan Posva, 2015-02-23 08:23:35]

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

[Ivan Posva, 2015-02-23 08:23:36]

First round of comments/questions...

-Ivan

https://codereview.chromium.org/944893005/diff/1/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/944893005/diff/1/runtime/lib/core_patch.dart#...
runtime/lib/core_patch.dart:50: controller.addStream(stream as Stream,
cancelOnError: false);
Why is there an "as Stream" here?

https://codereview.chromium.org/944893005/diff/1/runtime/vm/flow_graph_builde...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/944893005/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:1489: const intptr_t old_ctx_level,
old_ctx_level is not used in this function. Why is it being passed?

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

https://codereview.chromium.org/944893005/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:6286: // Create the parameter list for the body closure of
a sync generator:
sync?

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

https://codereview.chromium.org/944893005/diff/1/runtime/vm/symbols.h#newcode50
runtime/vm/symbols.h:50: V(ControllerStream, "stream")                          
                     \
Stream instead of ControllerStream?

[hausner, 2015-02-23 17:12:01]

Thank you. I realized that async* is more complex than what's in this change. 

Since the await operator can be used in async* functions, I need to add all the
async support to async* as well.

https://codereview.chromium.org/944893005/diff/1/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/944893005/diff/1/runtime/lib/core_patch.dart#...
runtime/lib/core_patch.dart:50: controller.addStream(stream as Stream,
cancelOnError: false);
On 2015/02/23 08:23:36, Ivan Posva wrote:
> Why is there an "as Stream" here?

Because the spec says it is a runtime error if yield* returns a value that is
not a stream. The type annotation on the parameter does not fire in non-checked
mode. The as-operator does a type check even in non-checked mode.

https://codereview.chromium.org/944893005/diff/1/runtime/vm/flow_graph_builde...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/944893005/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:1489: const intptr_t old_ctx_level,
On 2015/02/23 08:23:36, Ivan Posva wrote:
> old_ctx_level is not used in this function. Why is it being passed?

Copy-paste error. I think I will need it though since I'll have to add most of
the implicit variables of an async method in async*.

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

https://codereview.chromium.org/944893005/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:6286: // Create the parameter list for the body closure of
a sync generator:
On 2015/02/23 08:23:36, Ivan Posva wrote:
> sync?

Yes.

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

https://codereview.chromium.org/944893005/diff/1/runtime/vm/symbols.h#newcode50
runtime/vm/symbols.h:50: V(ControllerStream, "stream")                          
                     \
On 2015/02/23 08:23:36, Ivan Posva wrote:
> Stream instead of ControllerStream?

Ok. In some cases we concatenate class and method name (e.g. FutureThen) but we
don't do it consistently and I think it's not necessary. Done.

[kevmoo, 2015-02-23 18:37:29]

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

[kevmoo, 2015-02-23 18:37:30]

DBC

https://codereview.chromium.org/944893005/diff/1/tests/language/language.status
File tests/language/language.status (right):

https://codereview.chromium.org/944893005/diff/1/tests/language/language.stat...
tests/language/language.status:13: ### asyncstar_concat_test: Fail # Issue 21404
Please remove these sections, don't just comment them out

https://codereview.chromium.org/944893005/diff/1/tests/language/language.stat...
tests/language/language.status:47: ### async_await_syntax_test/a09a:
CompileTimeError # Issue 21404
ditto

https://codereview.chromium.org/944893005/diff/1/tests/language/language.stat...
tests/language/language.status:59: ### async_await_syntax_test/a03a:
CompileTimeError # Issue 21404
ditto

[hausner, 2015-02-23 18:42:18]

Thanks.

https://codereview.chromium.org/944893005/diff/1/tests/language/language.status
File tests/language/language.status (right):

https://codereview.chromium.org/944893005/diff/1/tests/language/language.stat...
tests/language/language.status:13: ### asyncstar_concat_test: Fail # Issue 21404
On 2015/02/23 18:37:30, kevmoo wrote:
> Please remove these sections, don't just comment them out

Yes, will do.

https://codereview.chromium.org/944893005/diff/1/tests/language/language.stat...
tests/language/language.status:47: ### async_await_syntax_test/a09a:
CompileTimeError # Issue 21404
On 2015/02/23 18:37:30, kevmoo wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/944893005/diff/1/tests/language/language.stat...
tests/language/language.status:59: ### async_await_syntax_test/a03a:
CompileTimeError # Issue 21404
On 2015/02/23 18:37:30, kevmoo wrote:
> ditto

Acknowledged.

[hausner, 2015-02-24 00:56:34]

hausner@google.com changed reviewers:
+ lrn@google.com
- kevmoo@google.com

[hausner, 2015-02-24 00:56:35]

Looking for next round of comments please. This version allows await inside
async* functions.

I would appreciate if Lasse could check the _AsyncStarStreamController code in
core_patch.dart to see whether it makes sense.

The last test case in asyncstar_yield[star]_test.dart hangs with this change. I
am probably not yet correctly implementing the case when yielding a value to a
stream that got cancelled.

[Lasse Reichstein Nielsen, 2015-02-24 13:12:34]

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:31: bool isCancelled = false;
"isCanceled"
I've recently been informed that one "l" is US English spelling :(

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:41: if (isCancelled) return true;
Since you are using a single-subscription controller, and you don't start
running until onListen, you should be able to use !controller.hasListener as
equivalent to isCancelled, and save the cancel callback.

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:43: if (true) {
if (true) ?

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:45: scheduleMicrotask(asyncStarBody);
If the stream subscription is paused, you should not reschedule the body until
the subscription is resumed.
I think this would work:
  if (!controller.isPaused) scheduleMicrotask(asyncStarBody);
  return true;

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:57: // has been cancelled?
If controller.stream has been canceled or if the argument stream has been
canceled?

In the latter case, the addStream will return a future which will be completed
soon (or is already completed, I don't remember - there is no practical
difference).

If the controller.stream has been canceled, adding to it throws, so checking
isCancelled first is fine, but then the asyncStarBody function should start
exiting, as if by a return, so you do need to schedule it.
That is, the behavior of asyncStartBody, when called as a microtask, depends on
whether the subscription has been canceled. 

Maybe let asyncStartBody take a boolean (optional?) argument, and when you pass
it false, it starts exiting due to the cancel.

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:76: controller.close();
If the subscription was paused, you will need to schedule the remainder of the
asyncStartBody, so it can do clean up (execute finallies). 
There won't be an "onResume" call if you cancel in a paused state, it goes
directly to "onCancel", and controller.isPaused will return false in the
canceled state, so you will need to keep the isPaused (or probably isScheduled)
state manually.

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:91: scheduleMicrotask(asyncStarBody);
You can get two onResumes in a row if someone does:
  subscription.resume();
  subscription.pause();
  subscription.resume();
when there are no buffered elements.
This code would schedule the body twice in that case.
You probably need to keep track of whether it has already been scheduled, and
not reschedule (an _isScheduled flag).


I think rescheduling immediately is better than my implementation in an earlier
mail where I kept checking whether the subscription was still paused. The async*
function should continue executing the first time it's resumed, and not check
again until the next yield.

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:96: isCancelled = true;
If the subscription was paused, you will need to schedule the remainder of the
asyncStartBody, so it can do clean up (execute finallies). 
There won't be an "onResume" call if you cancel in a paused state, it goes
directly to "onCancel", and controller.isPaused will return false in the
canceled state, so you will need to keep the isPaused (or probably _isScheduled)
state manually.

[hausner, 2015-02-25 21:59:32]

This version implements async* to the same level as async and sync. That is, the
feature is essentially complete, but shows the same bug as the other features
when yield or async is used in the catch or finally portion of a try statement.
That bug is independently being investigated, so I consider this a CL that is a
check-in candidate.

[hausner, 2015-02-25 22:07:41]

Responding to Lasse's comments.

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:31: bool isCancelled = false;
On 2015/02/24 13:12:34, Lasse Reichstein Nielsen wrote:
> "isCanceled"
> I've recently been informed that one "l" is US English spelling :(

I'm using controller.hasListener now. Apparently the spelling with one l is
recent American English. Both spellings are still in use.

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:41: if (isCancelled) return true;
On 2015/02/24 13:12:34, Lasse Reichstein Nielsen wrote:
> Since you are using a single-subscription controller, and you don't start
> running until onListen, you should be able to use !controller.hasListener as
> equivalent to isCancelled, and save the cancel callback.

Done.

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:43: if (true) {
On 2015/02/24 13:12:34, Lasse Reichstein Nielsen wrote:
> if (true) ?

That was a way to have the dart2js and the spec-compliant behavior in the code
at the same time. I'm implementing the dart2js version now, since apparently the
spec is going to change to conform to that behavior (suspending the generator
after each yield, whether the listener is paused or not.)

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:45: scheduleMicrotask(asyncStarBody);
On 2015/02/24 13:12:34, Lasse Reichstein Nielsen wrote:
> If the stream subscription is paused, you should not reschedule the body until
> the subscription is resumed.
> I think this would work:
>   if (!controller.isPaused) scheduleMicrotask(asyncStarBody);
>   return true;

Done.

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:57: // has been cancelled?
On 2015/02/24 13:12:34, Lasse Reichstein Nielsen wrote:
> If controller.stream has been canceled or if the argument stream has been
> canceled?
> 
> In the latter case, the addStream will return a future which will be completed
> soon (or is already completed, I don't remember - there is no practical
> difference).
> 
> If the controller.stream has been canceled, adding to it throws, so checking
> isCancelled first is fine, but then the asyncStarBody function should start
> exiting, as if by a return, so you do need to schedule it.
> That is, the behavior of asyncStartBody, when called as a microtask, depends
on
> whether the subscription has been canceled. 
> 
> Maybe let asyncStartBody take a boolean (optional?) argument, and when you
pass
> it false, it starts exiting due to the cancel.

Done.

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:76: controller.close();
On 2015/02/24 13:12:34, Lasse Reichstein Nielsen wrote:
> If the subscription was paused, you will need to schedule the remainder of the
> asyncStartBody, so it can do clean up (execute finallies). 
> There won't be an "onResume" call if you cancel in a paused state, it goes
> directly to "onCancel", and controller.isPaused will return false in the
> canceled state, so you will need to keep the isPaused (or probably
isScheduled)
> state manually.

Done. One thing that is still sup-optimal: the controller does not know whether
the body is already sheduled to run again, so in the worst case it gets
scheduled twice after the stream is cancelled.

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:91: scheduleMicrotask(asyncStarBody);
On 2015/02/24 13:12:34, Lasse Reichstein Nielsen wrote:
> You can get two onResumes in a row if someone does:
>   subscription.resume();
>   subscription.pause();
>   subscription.resume();
> when there are no buffered elements.
> This code would schedule the body twice in that case.
> You probably need to keep track of whether it has already been scheduled, and
> not reschedule (an _isScheduled flag).
> 
> 
> I think rescheduling immediately is better than my implementation in an
earlier
> mail where I kept checking whether the subscription was still paused. The
async*
> function should continue executing the first time it's resumed, and not check
> again until the next yield.

Acknowledged.

https://codereview.chromium.org/944893005/diff/20001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:96: isCancelled = true;
On 2015/02/24 13:12:34, Lasse Reichstein Nielsen wrote:
> If the subscription was paused, you will need to schedule the remainder of the
> asyncStartBody, so it can do clean up (execute finallies). 
> There won't be an "onResume" call if you cancel in a paused state, it goes
> directly to "onCancel", and controller.isPaused will return false in the
> canceled state, so you will need to keep the isPaused (or probably
_isScheduled)
> state manually.

See comment above.

[Lasse Reichstein Nielsen, 2015-02-26 14:54:52]

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:42: // Adds element to steam, returns true iff the
caller should suspend
suspend -> terminate/return from?

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:46: // If stream is cancelled, tell caller to exit
the async generator.
The spec says that you should pause first if the stream is paused, and then
terminate the generator if the stream is cancelled before it is resumed.
That means that the boolean saying whether to exit the generator isn't known
until either the onCancel or the onResume callback, whichever comes first.
That's why I suggest that the asyncStartBody takes a boolean argument, instead
of just having the add method returning a bool (which bight also be fine,
because it quickly handles the case where the subscription is cancelled before
the yield, but might also be spec violating if the yield is specced to always
yield control before continuing - while(true)try{yield 42;}finally(continue;}
would be an an uninterruptible loop if a cancelled subscription made the yield
try to exit immediately).

This implementation will "work" (generate the same stream events), but it will
run non-finally generator code after the subscription is canceled, which
shouldn't happen, and it will run until it hits the *next* yield, which may
cause visible side effects.
So, if you do:

 import "dart:async";
 main() async {
   var c1 = new Completer();
   var c2 = new Completer();
   var c3 = new Completer();
   var stream = () async* { 
     print("*1"); 
     yield 1; 
     c1.complete(); 
     await c2.future; 
     c3.complete();
     print("*2"); 
     yield 2; 
     print("*3");
   } ();
   var sub = stream.listen(print);
   await c1.future;  // prints "*1", "1"
   sub.pause();
   c2.complete();    // prints "*2"
   await c3.future;
   sub.cancel();     // should not print 2 or "*3"
 }
 
you will also print "*3". The current dart2js implementation only prints "*1",
"1" and "*2".

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:79: if (!controller.hasListener) return true;
I think an uncaught exception reaching the stream can only happen if control has
already left tbe body of the generator (that's why it's uncaught). A yield
statement cannot add an error to the stream.
That should mean that the return value is irrelevant. Returning here is still
fine, it just doesn't have to be "true".

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:85: 
It returns null if the controller isn't paused - which as argued above is fine,
but might as well make it a void function.

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:107: scheduleMicrotask(asyncStarBody);
This is still error-prone if you get another resume (or just a resume) before
the already scheduled body is run.

I would move this into a helper function:
  scheduleBody();
which is:
  void scheduleBody() {
    if (_isScheduled || controller.isPaused || _isAdding) return;
    _isScheduled = true;
    scheduleMicrotask(runBody);
  }
  // could have two versions of runBody if you want to send a bool back.
  void runBody() {      
    _isScheduled = false;
    asyncStarBody();
  }
with a corresponding flag at the top.
That definitely ensures that you only schedule the body once until it has run
again.

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:112: if (!isClosed) {
You can use controller.isClosed. It's set to true when you call
controller.close().

[hausner, 2015-02-26 21:54:08]

Responding to Lasse's comments.

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:42: // Adds element to steam, returns true iff the
caller should suspend
On 2015/02/26 14:54:52, Lasse Reichstein Nielsen wrote:
> suspend -> terminate/return from?

Correct.

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:46: // If stream is cancelled, tell caller to exit
the async generator.
On 2015/02/26 14:54:52, Lasse Reichstein Nielsen wrote:
> The spec says that you should pause first if the stream is paused, and then
> terminate the generator if the stream is cancelled before it is resumed.
> That means that the boolean saying whether to exit the generator isn't known
> until either the onCancel or the onResume callback, whichever comes first.
> That's why I suggest that the asyncStartBody takes a boolean argument, instead
> of just having the add method returning a bool (which bight also be fine,
> because it quickly handles the case where the subscription is cancelled before
> the yield, but might also be spec violating if the yield is specced to always
> yield control before continuing - while(true)try{yield 42;}finally(continue;}
> would be an an uninterruptible loop if a cancelled subscription made the yield
> try to exit immediately).
> 
> This implementation will "work" (generate the same stream events), but it will
> run non-finally generator code after the subscription is canceled, which
> shouldn't happen, and it will run until it hits the *next* yield, which may
> cause visible side effects.
> So, if you do:
> 
>  import "dart:async";
>  main() async {
>    var c1 = new Completer();
>    var c2 = new Completer();
>    var c3 = new Completer();
>    var stream = () async* { 
>      print("*1"); 
>      yield 1; 
>      c1.complete(); 
>      await c2.future; 
>      c3.complete();
>      print("*2"); 
>      yield 2; 
>      print("*3");
>    } ();
>    var sub = stream.listen(print);
>    await c1.future;  // prints "*1", "1"
>    sub.pause();
>    c2.complete();    // prints "*2"
>    await c3.future;
>    sub.cancel();     // should not print 2 or "*3"
>  }
>  
> you will also print "*3". The current dart2js implementation only prints "*1",
> "1" and "*2".

I am leaving this as a TODO for later. The body closure already has 3 (optional)
parameters that are used if there is an await in the async* code. I don't want
to make things more complex at this point. 

As a note: instead of passing a parameter to the closure body, I think it would
also be ok to add a getter to the AsyncStarStreamController that returns whether
the stream is cancelled. Then, the compiler could generate this code for a yield
e statement:

  controller.add(e);
  suspend_generator;
  if (controller.isCancelled) return;

Would that work?

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:79: if (!controller.hasListener) return true;
On 2015/02/26 14:54:52, Lasse Reichstein Nielsen wrote:
> I think an uncaught exception reaching the stream can only happen if control
has
> already left tbe body of the generator (that's why it's uncaught). A yield
> statement cannot add an error to the stream.
> That should mean that the return value is irrelevant. Returning here is still
> fine, it just doesn't have to be "true".

Yes, this method is only called from the implicit try-catch around the async*
body. In that case, control flow is on its way out of the generator. The only
thing that will happen after this call is the invocation of the close() method
from the finally clause.

Made addError() a void function and removed the code to schedule the body again.

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:85: 
On 2015/02/26 14:54:52, Lasse Reichstein Nielsen wrote:
> It returns null if the controller isn't paused - which as argued above is
fine,
> but might as well make it a void function.

Done.

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:107: scheduleMicrotask(asyncStarBody);
On 2015/02/26 14:54:52, Lasse Reichstein Nielsen wrote:
> This is still error-prone if you get another resume (or just a resume) before
> the already scheduled body is run.
> 
> I would move this into a helper function:
>   scheduleBody();
> which is:
>   void scheduleBody() {
>     if (_isScheduled || controller.isPaused || _isAdding) return;
>     _isScheduled = true;
>     scheduleMicrotask(runBody);
>   }
>   // could have two versions of runBody if you want to send a bool back.
>   void runBody() {      
>     _isScheduled = false;
>     asyncStarBody();
>   }
> with a corresponding flag at the top.
> That definitely ensures that you only schedule the body once until it has run
> again.

Ok, done. I was thinking about this possibility yesterday but wanted to avoid
the extra function call. In the grand scheme of things, that is probably not
worth worrying about.

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:112: if (!isClosed) {
On 2015/02/26 14:54:52, Lasse Reichstein Nielsen wrote:
> You can use controller.isClosed. It's set to true when you call
> controller.close().

Done.

[Lasse Reichstein Nielsen, 2015-02-27 07:51:12]

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/944893005/diff/40001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:46: // If stream is cancelled, tell caller to exit
the async generator.
On 2015/02/26 21:54:07, hausner wrote:
>   controller.add(e);
>   suspend_generator;
>   if (controller.isCancelled) return;
> 
> Would that work?

Looks fine. It basically what I did in an attempt to rewrite async* to async
(no-start) code. The suspend_generator will suspend either until a microtask,
or, if paused, until the resume.
(I assume this is pre-transformed pseudo code).

[hausner, 2015-02-28 00:18:50]

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

[hausner, 2015-02-28 00:21:32]

This version has Regis' async fix for try-catch-finally merged in.

I do not know why asyncstar_throw_in_catch_test.dart hangs in the second test
case (foo2()). I'm investigating, but maybe it's ok to check this in as is,
warts and all?

[Ivan Posva, 2015-03-03 01:05:52]

LGTM -ip

https://codereview.chromium.org/944893005/diff/80001/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/944893005/diff/80001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:46: void scheduleGenerator() {
Why does this method have to be public? Same question for all of the other
methods and fields in this class.

https://codereview.chromium.org/944893005/diff/80001/runtime/lib/core_patch.d...
runtime/lib/core_patch.dart:63: // if (controller.isCanelled) return;
isCanceled

https://codereview.chromium.org/944893005/diff/80001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/944893005/diff/80001/runtime/vm/object.h#newc...
runtime/vm/object.h:2155: bool IsAsyncFunction() const {
Can you please comment what these different function checkers test for? For
example in which case does IsGenerator() return true?

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

https://codereview.chromium.org/944893005/diff/100001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6422: if (params->parameters->length() == 0) {
We should assert that params->parameters->length() is 0 or 1.

https://codereview.chromium.org/944893005/diff/100001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6547: //   var :controller;
//   var :async_op;?

https://codereview.chromium.org/944893005/diff/100001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6639: //   var :await_context_var;
// var :async_op;

https://codereview.chromium.org/944893005/diff/100001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6660:
closure_body->scope()->LookupVariable(Symbols::Controller(), false);
Force capture :async_op?

https://codereview.chromium.org/944893005/diff/100001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6725: // TODO(hausner): Is the temporary expression
necessary?
Remove TODO

[hausner, 2015-03-03 17:50:16]

Committed patchset #7 (id:120001) manually as r44185 (presubmit successful).

[hausner, 2015-03-03 17:51:29]

Thank you. async* has landed.

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

https://codereview.chromium.org/944893005/diff/100001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6422: if (params->parameters->length() == 0) {
On 2015/03/03 01:05:52, Ivan Posva wrote:
> We should assert that params->parameters->length() is 0 or 1.

Done.

https://codereview.chromium.org/944893005/diff/100001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6547: //   var :controller;
On 2015/03/03 01:05:52, Ivan Posva wrote:
> //   var :async_op;?

Done.

https://codereview.chromium.org/944893005/diff/100001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6639: //   var :await_context_var;
On 2015/03/03 01:05:52, Ivan Posva wrote:
> // var :async_op;

Done.

https://codereview.chromium.org/944893005/diff/100001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6660:
closure_body->scope()->LookupVariable(Symbols::Controller(), false);
On 2015/03/03 01:05:52, Ivan Posva wrote:
> Force capture :async_op?

Done.

https://codereview.chromium.org/944893005/diff/100001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6725: // TODO(hausner): Is the temporary expression
necessary?
On 2015/03/03 01:05:52, Ivan Posva wrote:
> Remove TODO

Done.