NotificationsYou must be signed in to change notification settings
Fork2k
Star16.6k

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

atg wants to merge14 commits intojashkenas:masterfromatg:master

Closed

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

atg wants to merge14 commits intojashkenas:masterfromatg:master

Conversation

Copy link

atg commentedSep 12, 2016

This implements afor x from g loop as argued for inthis comment.

Currently CoffeeScript supports generators usingyield andyield from, but it doesn't support a way touse these generators likefor (x of g) in ES6.

CoffeeScript already hasfor in (for Arrays) andfor 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 afor in loop over an iterator will be a normal for loopfor (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 generatormutates 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 toyield from y.

Added support for for-from loop, seejashkenas#3832

4caa6b7

Copy link

Collaborator

lydell commentedSep 12, 2016

I like this. 👍

vendethiel reviewed

Sep 12, 2016

View reviewed changes

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"

Copy link

Collaborator

vendethielSep 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

why this?

Copy link

Author

atgSep 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Collaborator

vendethielSep 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍

atg added3 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

Copy link

Author

atg commentedSep 12, 2016

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

atg added4 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

Copy link

Owner

jashkenas commentedSep 26, 2016

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

jashkenas added the enhancement label

Sep 26, 2016

Copy link

Collaborator

lydell commentedSep 26, 2016

I get this error when running the tests:

$ npm run test-harmony> coffee-script@1.11.0 test-harmony /home/lydell/forks/coffeescript> node --harmony ./bin/cake testfailed 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 mentioned this pull request

Sep 27, 2016

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

Closed

Copy link

Collaborator

lydell commentedSep 29, 2016

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

Copy link

Jamesernator commentedOct 3, 2016•
edited
Loading

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.

Copy link

Author

atg commentedOct 29, 2016

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

for-from: merge with upstream

2267019

Copy link

Author

atg commentedOct 29, 2016•
edited
Loading

@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 withv8 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 toIteratorClose(). 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. ¯_(ツ)_/¯

Copy link

Collaborator

GeoffreyBooth commentedOct 29, 2016

@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.

Copy link

Author

atg commentedOct 29, 2016•
edited
Loading

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

atg added2 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

Copy link

Author

atg commentedOct 29, 2016

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

GeoffreyBooth requested changes

Oct 29, 2016

View reviewed changes

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", ->

Copy link

Collaborator

GeoffreyBoothOct 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Please put these tests inerror_messages.coffee.

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'

Copy link

Collaborator

GeoffreyBoothOct 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Please always useis instead of==.

src/lexer.coffee

		@error "reserved word '#{id}'", length: id.length

		if tag is 'IDENTIFIER'
		if @seenFor and id is 'from'

Copy link

Collaborator

GeoffreyBoothOct 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Author

atgOct 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Not sure. You can copy thetag is 'IDENTIFIER' into each of the ifs, but then theelse 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 tagis'IDENTIFIER'and@seenForand idis'from'tag='FORFROM'@seenFor=noelseif tagis'IDENTIFIER'and (idin JS_KEYWORDSor idin COFFEE_KEYWORDS)andnot (@exportSpecifierListand idin COFFEE_KEYWORDS)tag=id.toUpperCase()if tagis'WHEN'and@tag()in LINE_BREAKtag='LEADING_WHEN'elseif tagis'FOR'@seenFor=yeselseif tagis'UNLESS'tag='IF'elseif tagis'IMPORT'@seenImport=yeselseif tagis'EXPORT'@seenExport=yeselseif tagin UNARYtag='UNARY'elseif tagin RELATIONif tagisnt'INSTANCEOF'and@seenFortag='FOR'+ tag@seenFor=noelsetag='RELATION'if@value()is'!'poppedToken=@tokens.pop()id='!'+ idif tagis'IDENTIFIER'and idin RESERVED@error"reserved word '#{id}'",length:id.length

Copy link

Collaborator

GeoffreyBoothOct 31, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

atg#1

atg added2 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

Copy link

Author

atg commentedNov 1, 2016

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

GeoffreyBooth reviewed

Nov 1, 2016

View reviewed changes

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", ->

Copy link

Collaborator

GeoffreyBoothNov 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This should also go inerror_messages.coffee.

This was referencedNov 4, 2016

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

Closed

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

Merged

GeoffreyBooth added a commit that referenced this pull request

Nov 8, 2016

Add afor .. from ..loop for generators, see#4306,#3832(#4355)

b3896d0

* 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

Copy link

Collaborator