Add a `for .. from ..` loop for generators, see #3832 #4306

atg · 2016-09-12T01:19:08Z

This implements a for x from g loop as argued for in this comment.

Currently CoffeeScript supports generators using yield and yield from, but it doesn't support a way to use these generators like for (x of g) in ES6.

CoffeeScript already has for in (for Arrays) and for of (for Objects), however users expect both to generate es5-compatible code. It's impossible to modify either of these syntaxes to have all three of es5 compatibility, efficiency and support for generators/iterables.

That means the choice is either a compile-time flag or a new loop syntax. A compile-time flag is likely to create subtle bugs: if the code is expected to be compiled in es6 mode but is actually compiled in es5 mode, then a for in loop over an iterator will be a normal for loop for (var i = 0; i < iterator.length; i++). Iterators are objects, and .length on an iterator is usually 0. The loop will therefore silently fail to run, with no error message.

That leaves a new loop syntax.

The best argument for introducing a new for loop syntax for generators is that they are fundamentally different from either arrays or objects. Iterating over a generator mutates that generator, the next time you iterate over it, it will be empty. Generators cannot be iterated over in reverse order (because they are not indexed). Generators can be infinite.

So aside from being the only option, a new syntax has the benefit of making it clear from the code which loops are meant for Arrays, and which loops are meant for generators (or any iterable).

As for the specific syntax, for x from y is a nice complement to yield from y.

lydell · 2016-09-12T05:18:53Z

I like this. 👍

vendethiel · 2016-09-12T08:15:13Z

src/nodes.coffee

@@ -2178,6 +2180,9 @@ exports.For = class For extends While
    if @object
      forPartFragments   = [@makeCode("#{kvar} in #{svar}")]
      guardPart = "\n#{idt1}if (!#{utility 'hasProp', o}.call(#{svar}, #{kvar})) continue;" if @own
+    if @from
+      forPartFragments   = [@makeCode("#{kvar} of #{svar}")]
+      guardPart = "\n"


You mean the guardPart? Yeah, I'll delete that.

atg · 2016-09-12T19:38:06Z

I'm actually starting to understand how For#compileNode works now. I've refactored my first try so that @index and @name are not flipped, and added support for patterns.

jashkenas · 2016-09-26T20:52:08Z

I like this too. @lydell should we merge it?

lydell · 2016-09-26T21:36:38Z

I get this error when running the tests:

$ npm run test-harmony

> [email protected] test-harmony /home/lydell/forks/coffeescript
> node --harmony ./bin/cake test

failed 1 and passed 755 tests in 10.75 seconds 

  for-from comprehensions over generators 
  AssertionError: Expected  to deep equal 70,20
  ...

  function () {
    var array1, array2, array3, array4, gen, iterator, x;
    array1 = [50, 30, 70, 20];
    gen = function*() {
      return (yield* array1);
    };
    array2 = [];
    array3 = [];
    array4 = [];
    iterator = gen();
    for (x of iterator) {
      array2.push(x);
      if (x === 30) {
        break;
      }
    }
    for (x of iterator) {
      array3.push(x);
    }
    for (x of iterator) {
      array4.push(x);
    }
    arrayEq(array2, [50, 30]);
    arrayEq(array3, [70, 20]);
    return arrayEq(array4, []);
  }

lydell · 2016-09-29T16:23:54Z

@atg I'd like to merge this too. Could you please look into the failing test whenever you get the time? :)

Jamesernator · 2016-10-03T00:44:20Z

I'd like to merge this too. Could you please look into the failing test whenever you get the time? :)

I took a look at this, it seems like there's a bug in V8 with resuming iterators in a for-of loop (it failed in both Chrome and Node), it works fine on SpiderMonkey though.

atg · 2016-10-29T00:14:40Z

Oops, didn't see the updates. I'll see if I can get this up to date.

atg · 2016-10-29T02:57:33Z

@lydell OK the branch is conflict-free now.

The test is ok insofar as CoffeeScript is generating the right code. But the code doesn't run as expected.

As @Jamesernator said, the test works in Firefox but not in v8. So I tried in Safari, which agrees with v8 that the test is wrong.

I dug into the spec, and it's difficult to understand, but from what I can determine my test & spidermonkey are wrong and v8/nitro are doing the right thing. This is the relevant section.

You can see that all paths lead to IteratorClose(). If you loop over a generator object once and break in the middle, then it will be closed forever. Personally I think this is a silly decision because it shuts the door on many cool uses of generators. But arguing with the spec is like arguing with a wall. ¯_(ツ)_/¯

GeoffreyBooth · 2016-10-29T17:59:43Z

@atg so does the test pass (in Node 6.9.1 or 7.0.0, with --harmony)? We can’t merge in a PR with broken tests.

atg · 2016-10-29T18:33:50Z

I'll change the test to accept either the firefox or v8/nitro behaviour as a pass.

See explanation here: jashkenas#4306 (comment)

atg · 2016-10-29T19:00:00Z

$ node --harmony ./bin/cake test
passed 771 tests in 9.13 seconds

GeoffreyBooth · 2016-10-29T20:54:40Z

test/compilation.coffee

@@ -123,3 +123,10 @@ test "#3001: `own` shouldn't be allowed in a `for`-`in` loop", ->

 test "#2994: single-line `if` requires `then`", ->
  cantCompile "if b else x"
+
+test "indexes are not supported in for-from loops", ->


Please put these tests in error_messages.coffee.

GeoffreyBooth · 2016-10-29T20:55:12Z

test/generators.coffee

+  # Different JS engines have different opinions on the value of array3:
+  # https://github.com/jashkenas/coffeescript/pull/4306#issuecomment-257066877
+  # As a temporary measure, either result is accepted.
+  ok array3.length == 0 or array3.join(',') == '70,20'


Please always use is instead of ==.

GeoffreyBooth · 2016-10-29T20:56:39Z

src/lexer.coffee

-      @error "reserved word '#{id}'", length: id.length
-
+    if tag is 'IDENTIFIER'
+      if @seenFor and id is 'from'


Can this be refactored so that we don't branch this if block again? In other words, don't indent the following 15ish lines.

Not sure. You can copy the tag is 'IDENTIFIER' into each of the ifs, but then the else if condition becomes comically long (see below).

I do agree that probably the if at the bottom of the big if (if id in RESERVED) should broken out and returned to the main body of the method.

if tag is 'IDENTIFIER' and @seenFor and id is 'from' tag = 'FORFROM' @seenFor = no else if tag is 'IDENTIFIER' and (id in JS_KEYWORDS or id in COFFEE_KEYWORDS) and not (@exportSpecifierList and id in COFFEE_KEYWORDS) tag = id.toUpperCase() if tag is 'WHEN' and @tag() in LINE_BREAK tag = 'LEADING_WHEN' else if tag is 'FOR' @seenFor = yes else if tag is 'UNLESS' tag = 'IF' else if tag is 'IMPORT' @seenImport = yes else if tag is 'EXPORT' @seenExport = yes else if tag in UNARY tag = 'UNARY' else if tag in RELATION if tag isnt 'INSTANCEOF' and @seenFor tag = 'FOR' + tag @seenFor = no else tag = 'RELATION' if @value() is '!' poppedToken = @tokens.pop() id = '!' + id if tag is 'IDENTIFIER' and id in RESERVED @error "reserved word '#{id}'", length: id.length

atg · 2016-11-01T19:49:52Z

Alright, do it or don't. I got places to be.

GeoffreyBooth · 2016-11-01T20:39:45Z

test/compilation.coffee

@@ -123,3 +123,6 @@ test "#3001: `own` shouldn't be allowed in a `for`-`in` loop", ->

 test "#2994: single-line `if` requires `then`", ->
  cantCompile "if b else x"
+
+test "own is not supported in for-from loops", ->


This should also go in error_messages.coffee.

* Added support for for-from loop, see #3832 * for-from: remove extra newline and add support for ranges * for-from: tidy up the lexer * for-from: add support for patterns * for-from: fix bad alignment * for-from: add two more tests * for-from: fix test "for-from loops over generators" See explanation here: #4306 (comment) * for-from: delete leftover console.log * Refactor the big `if` block in the lexer to be as minimal a change from `master` as we can get away with * Cleanup to make more idiomatic, remove trailing whitespace, minor performance improvements * for-from: move code from one file to another * for-from: clean up whitespace * for-from: lexer bikeshedding * Move "own is not supported in for-from loops" test into error_messages.coffee; improve error message so that "own" is underlined * Revert unnecessary changes, to minimize the lines of code modified by this PR

GeoffreyBooth · 2016-11-08T07:40:42Z

Closed per #4355

Added support for for-from loop, see jashkenas#3832

4caa6b7

vendethiel reviewed Sep 12, 2016
View reviewed changes

atg added 3 commits September 12, 2016 18:19

for-from: remove extra newline and add support for ranges

68f22b0

for-from: tidy up the lexer

07da7ad

for-from: add support for patterns

5361fbf

atg added 4 commits September 12, 2016 20:47

for-from: fix bad alignment

ab10937

for-from: sync with upstream

486ab5f

for-from: sync with upstream (again)

34e83c6

for-from: add two more tests

e759662

jashkenas added the enhancement label Sep 26, 2016

lydell mentioned this pull request Sep 27, 2016

CS2 Discussion: Project: CoffeeScript 2.0.0 coffeescript6/discuss#36

Closed

for-from: merge with upstream

2267019

atg added 2 commits October 29, 2016 19:52

for-from: fix test "for-from loops over generators"

813e107

See explanation here: jashkenas#4306 (comment)

for-from: delete leftover console.log

f279fcf

GeoffreyBooth requested changes Oct 29, 2016

View reviewed changes

atg added 2 commits November 1, 2016 19:34

for-from: move code from one file to another

a4531b0

for-from: clean up whitespace

4be5a4d

for-from: lexer bikeshedding

00fee49

GeoffreyBooth reviewed Nov 1, 2016

View reviewed changes

This was referenced Nov 4, 2016

CS2 Discussion: Project: Progress as of November 2016 coffeescript6/discuss#52

Closed

Add a for .. from .. loop for generators, see #4306, #3832 #4355

Merged

GeoffreyBooth closed this Nov 8, 2016

GeoffreyBooth mentioned this pull request Dec 2, 2016

CS2 Discussion: Project: Progress as of December 2016 coffeescript6/discuss#56

Closed

GeoffreyBooth mentioned this pull request Feb 22, 2017

CS2 Discussion: Project: Progress as of February 2017: 2.0.0-alpha1! coffeescript6/discuss#71

Closed

GeoffreyBooth mentioned this pull request Apr 14, 2017

CS2 Discussion: Project: Progress as of April 2017: 2.0.0-beta1! coffeescript6/discuss#80

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `for .. from ..` loop for generators, see #3832 #4306

Add a `for .. from ..` loop for generators, see #3832 #4306

atg commented Sep 12, 2016

lydell commented Sep 12, 2016

vendethiel Sep 12, 2016

atg Sep 12, 2016

vendethiel Sep 12, 2016

atg commented Sep 12, 2016

jashkenas commented Sep 26, 2016

lydell commented Sep 26, 2016

lydell commented Sep 29, 2016

Jamesernator commented Oct 3, 2016 •

edited

Loading

atg commented Oct 29, 2016

atg commented Oct 29, 2016 •

edited

Loading

GeoffreyBooth commented Oct 29, 2016

atg commented Oct 29, 2016 •

edited

Loading

atg commented Oct 29, 2016

GeoffreyBooth Oct 29, 2016

GeoffreyBooth Oct 29, 2016

GeoffreyBooth Oct 29, 2016

atg Oct 30, 2016

GeoffreyBooth Oct 31, 2016

atg commented Nov 1, 2016

GeoffreyBooth Nov 1, 2016

GeoffreyBooth commented Nov 8, 2016

Add a for .. from .. loop for generators, see #3832 #4306

Add a for .. from .. loop for generators, see #3832 #4306

Conversation

atg commented Sep 12, 2016

lydell commented Sep 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atg commented Sep 12, 2016

jashkenas commented Sep 26, 2016

lydell commented Sep 26, 2016

lydell commented Sep 29, 2016

Jamesernator commented Oct 3, 2016 • edited Loading

atg commented Oct 29, 2016

atg commented Oct 29, 2016 • edited Loading

GeoffreyBooth commented Oct 29, 2016

atg commented Oct 29, 2016 • edited Loading

atg commented Oct 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atg commented Nov 1, 2016

Choose a reason for hiding this comment

GeoffreyBooth commented Nov 8, 2016

Add a `for .. from ..` loop for generators, see #3832 #4306

Add a `for .. from ..` loop for generators, see #3832 #4306

Jamesernator commented Oct 3, 2016 •

edited

Loading

atg commented Oct 29, 2016 •

edited

Loading

atg commented Oct 29, 2016 •

edited

Loading