Fix #4875: Asynchronous iterators

[zdenko]

This PR adds support for asynchronous iterators (#4875)
It's still WIP and needs tests.

readLines = (path) ->
  file = await fileOpen path
  try
    while not file.EOF
      yield await file.readLine()
  finally
    await file.close()

foo = ->
  res = []
  for await x from readLines
    res.push x
  res

compiles to

var foo, readLines;
readLines = async function*(path) {
  var file, results;
  file = (await fileOpen(path));
  try {
    results = [];
    while (!file.EOF) {
      results.push((yield (await file.readLine())));
    }
    return results;
  } finally {
    await file.close();
  }
};

foo = async function() {
  var res, x;
  res = [];
  for await (x of readLines) {
    res.push(x);
  }
  return res;
};

[vendethiel]

please move this kind of code to the AST nodes

[zdenko]

OK. That code was already there, I just added 'await' parts.
But, I agree. It belongs to the nodes.

[GeoffreyBooth]

Marking this as WIP because it lacks tests. You could convert the old error messages ones to be the basis of some new async iterators tests. Thanks for tackling this!

[dyoder]

This is looking good…thank you @zdenko! And to @vendethiel for the review.

[dyoder]

I wonder…creating an array of results here is potentially dangerous since async iterators will often, perhaps even typically, be used with an “infinite” collection, ex: events. Of course, not doing that would be unexpected. Perhaps it's something we can document?

[dyoder]

I guess that's no different than sync generators, though.

[rdeforest]

I made a test for this and it failed 'cake test' because it needed --harmony. When I ran cake build:watch:harmony without my test, a different test ("classes with value'd constructors") failed with the error "TypeError: Class constructors may only return object or undefined".

In order that my work not go to waste I'll include it in my post here:

succeedLater = (val, ms) ->
  new Promise (resolve, reject) ->
    setTimeout (-> resolve val), ms

failLater    = (val, ms) ->
  new Promise (resolve, reject) ->
    setTimeout (-> reject new Error val), ms

asyncIterator = (iterations, operation) ->
  for i in [1 .. iterations]
    yield await operation()

test "success loop finishes", ->
  a = await do ->
    for await result from asyncIterator 5, -> succeedLater 'ok', 10
      result

  eq a.length, 5
  eq -1, a.findIndex (x) -> x isnt 'ok'

test "failure loop aborts", ->
  try
    a = do ->
      for await result from asyncIterator 5, -> failLater 'err', 10
        ok false

    a.catch (err) ->
      eq 'err', err.message

  catch err
    ok false

(Edited to add failure test)

[GeoffreyBooth]

I don’t know if this is already on this branch by now, but earlier this was tagged WIP because we need to add something to Cakefile similar to the feature detection for async function support, to test for async iterator support; and the async iterator tests would be in their own file that only gets loaded when supported. It’s a pattern we’re going to need to increasingly follow as we add support for more newer features.

[rdeforest]

I see this in Cakefile:

  unless global.supportsAsync # Except for async tests, if async isn’t supported.
    files = files.filter (filename) -> filename isnt 'async.coffee'

This seems like it wouldn't scale. Is there already work underway on generalizing this kind of thing? I didn't see a tracking issue for it. I'd love to take the project on if nobody else is doing it already.

[GeoffreyBooth]

@rdeforest That would be great. Maybe do it as a separate PR?

The codebase assumes support for all features supported by Node 6, as that’s our minimum supported version. So there aren’t that many features added to ES since then that need special casing. Async iterators, exponentiation operator (see #4881) and rewriting the modules tests to really run rather than do string comparisons. I’m not sure there are too many more; and in a year or two when there might be, we’ll probably bump up our minimum Node version to 8 or so.

[rdeforest]

I'll take a stab at it and submit a new PR. In the interest of 'one change at a time' I'll base it on master and not including the tests I posted above. I'm currently unemployed and procrastinating so I should have something today or tomorrow.

[GeoffreyBooth]

P.S. Take a look at the 1 branch Cakefile: https://github.com/jashkenas/coffeescript/blob/1/Cakefile#L403-L422

There were more special cases in the 1.x days, when we were supporting Node 0.8+.

[GeoffreyBooth]

@zdenko I think we have the testing feature detection worked out, so the next step for this PR is to create a tests/async_iterators.coffee and put the async iterator tests in there. They should be normal tests, not string comparisons.

[GeoffreyBooth]

Can you please explain all these grammar changes and why they’re necessary (or an improvement)?

[zdenko]

@vendethiel suggested moving some code from grammar into nodes, e.g.

 o 'ForStart ForSource',  ->
        $2[prop] = $1[prop] for prop in ['await', 'awaitTag', 'own', 'ownTag']

to which I agreed, and then reworked For in nodes.
It's just a cleanup.

[GeoffreyBooth]

Ah, okay. Maybe next time that should be its own PR, so it’s easier to track the changes that relate to the cleanup as opposed to adding support for async iterators, but I have no objection.

[GeoffreyBooth]

This feels a lot less readable than if @isGenerator and @isAsync. Why was this changed?

[zdenko]

Becuase I'm checking if this is async iterator for await x from y.
if @isGenerator and @isAsync was used to check if both condition above returned true and throw an error. Since functions can now contain yield and await this condition isn't required.
Besides, I think we can't use @isAsync here, e.g. if node instanceof For and @isAwait here, as @isAwait will be false.

[GeoffreyBooth]

Okay, anyone else have any notes? @vendethiel?