Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 14 commits into from
13 changes: 12 additions & 1 deletion lib/coffee-script/grammar.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions lib/coffee-script/lexer.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 11 additions & 3 deletions lib/coffee-script/nodes.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

93 changes: 56 additions & 37 deletions lib/coffee-script/parser.js

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion src/grammar.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,8 @@ grammar =
o 'FORIN Expression BY Expression', -> source: $2, step: $4
o 'FORIN Expression WHEN Expression BY Expression', -> source: $2, guard: $4, step: $6
o 'FORIN Expression BY Expression WHEN Expression', -> source: $2, step: $4, guard: $6
o 'FORFROM Expression', -> source: $2, from: yes
o 'FORFROM Expression WHEN Expression', -> source: $2, guard: $4, from: yes
]

Switch: [
Expand Down Expand Up @@ -643,7 +645,7 @@ operators = [
['nonassoc', 'INDENT', 'OUTDENT']
['right', 'YIELD']
['right', '=', ':', 'COMPOUND_ASSIGN', 'RETURN', 'THROW', 'EXTENDS']
['right', 'FORIN', 'FOROF', 'BY', 'WHEN']
['right', 'FORIN', 'FOROF', 'FORFROM', 'BY', 'WHEN']
['right', 'IF', 'ELSE', 'FOR', 'WHILE', 'UNTIL', 'LOOP', 'SUPER', 'CLASS']
['left', 'POST_IF']
]
Expand Down
8 changes: 6 additions & 2 deletions src/lexer.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ exports.Lexer = class Lexer
@indents = [] # The stack of all current indentation levels.
@ends = [] # The stack for pairing up tokens.
@tokens = [] # Stream of parsed tokens in the form `['TYPE', value, location data]`.
@seenFor = no # Used to recognize FORIN and FOROF tokens.
@seenFor = no # Used to recognize FORIN, FOROF and FORFROM tokens.

@chunkLine =
opts.line or 0 # The start line for the current @chunk.
Expand Down Expand Up @@ -121,7 +121,11 @@ exports.Lexer = class Lexer
'PROPERTY'
else
'IDENTIFIER'


if tag is 'IDENTIFIER' and @seenFor and id is 'from'
Copy link
Collaborator

Choose a reason for hiding this comment

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

add FROM to RELATIONS instead (~L939)

Copy link
Author

@atg atg Sep 12, 2016

Choose a reason for hiding this comment

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

from is not really a relation like in, of or instanceof. It can't be used as an infix operator. You can do (x in y) and (x of y) but not (x from y).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough. It's still a bit weird to have this part disjointed from the others.

Copy link
Author

Choose a reason for hiding this comment

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

Well in and of are reserved keywords, whereas from is a context-sensitive identifier. So the lexer can't handle them all in the same way.

$ coffee -te 'from'
[IDENTIFIER from] [TERMINATOR \n]

$ coffee -te 'in'
[RELATION in] [TERMINATOR \n]

I've merged the if tag is 'IDENTIFIER' conditionals into one, maybe that helps it look cleaner.

tag = 'FORFROM'
@seenFor = no

if tag is 'IDENTIFIER' and (id in JS_KEYWORDS or id in COFFEE_KEYWORDS)
tag = id.toUpperCase()
if tag is 'WHEN' and @tag() in LINE_BREAK
Expand Down
11 changes: 8 additions & 3 deletions src/nodes.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -2095,13 +2095,15 @@ exports.For = class For extends While
@body = Block.wrap [body]
@own = !!source.own
@object = !!source.object
[@name, @index] = [@index, @name] if @object
@from = !!source.from
[@name, @index] = [@index, @name] if @object or @from
@index.error 'index cannot be a pattern matching expression' if @index instanceof Value
@range = @source instanceof Value and @source.base instanceof Range and not @source.properties.length
@pattern = @name instanceof Value
@index.error 'indexes do not apply to range loops' if @range and @index
Copy link
Collaborator

@vendethiel vendethiel Sep 12, 2016

Choose a reason for hiding this comment

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

this can be an issue:

$ ./bin/coffee -bce '1 for a from [1..5]'
[stdin]:1:7: error: indexes do not apply to range loops
1 for a from [1..5]
      ^
$ ./bin/coffee -bce '1 for a from ([1..5])'                                                                                                                                                                             // Generated by CoffeeScript 1.10.0
var a;

for (a of [1, 2, 3, 4, 5]) {

  1;
}

Copy link
Author

Choose a reason for hiding this comment

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

OK, I've made @range conditional on @from not being enabled and added some tests for range support.

@name.error 'cannot pattern match over range loops' if @range and @pattern
@name.error 'cannot use own with for-in' if @own and not @object
@name.error 'cannot use index with for-from' if @from and @name
@returns = false

children: ['body', 'source', 'guard', 'step']
Expand All @@ -2121,7 +2123,7 @@ exports.For = class For extends While
scope.find(name) if name and not @pattern
scope.find(index) if index
rvar = scope.freeVariable 'results' if @returns
ivar = (@object and index) or scope.freeVariable 'i', single: true
ivar = ((@object or @from) and index) or scope.freeVariable 'i', single: true
kvar = (@range and name) or index or ivar
kvarAssign = if kvar isnt ivar then "#{kvar} = " else ""
if @step and not @range
Expand All @@ -2142,7 +2144,7 @@ exports.For = class For extends While
svar = ref
if name and not @pattern
namePart = "#{name} = #{svar}[#{kvar}]"
if not @object
if not (@object or @from)
defPart += "#{@tab}#{step};\n" if step isnt stepVar
down = stepNum < 0
lvar = scope.freeVariable 'len' unless @step and stepNum? and down
Expand Down Expand Up @@ -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

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

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

Choose a reason for hiding this comment

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

👍

bodyFragments = body.compileToFragments merge(o, indent: idt1), LEVEL_TOP
if bodyFragments and (bodyFragments.length > 0)
bodyFragments = [].concat @makeCode("\n"), bodyFragments, @makeCode("\n")
Expand Down
18 changes: 18 additions & 0 deletions test/arrays.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -107,3 +107,21 @@ test "regex interpolation in array", ->
eq 2, arr.length
eq 'ab', arr[0].source
eq 'value', arr[1].key


test "for-from loops over Array", ->
array1 = [50, 30, 70, 20]
array2 = []
for x from array1
array2.push(x)
arrayEq array1, array2


test "for-from comprehensions over Array", ->

array1 = (x + 10 for x from [10, 20, 30])
ok array1.join(' ') is '20 30 40'

array2 = (x for x from [30, 41, 57] when x %% 3 == 0)
ok array2.join(' ') is '30 57'

7 changes: 7 additions & 0 deletions test/compilation.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

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

Please put these tests in error_messages.coffee.

cantCompile "x for x, i from [1, 2, 3]"

test "own is not supported in for-from loops", ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should also go in error_messages.coffee.

cantCompile "x for own x from [1, 2, 3]"

38 changes: 38 additions & 0 deletions test/generators.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -250,3 +250,41 @@ test "yield handles 'this' correctly", ->
ok z.done is false

throws -> y.next new Error "boom"


test "for-from comprehensions over generators", ->
array1 = [50, 30, 70, 20]
gen = -> yield from array1

array2 = []
array3 = []
array4 = []

iterator = gen()
for x from iterator
array2.push(x)
break if x is 30

for x from iterator
array3.push(x)

for x from iterator
array4.push(x)

arrayEq array2, [50, 30]
arrayEq array3, [70, 20]
arrayEq array4, []


test "for-from comprehensions over generators", ->

gen = ->
yield from [30, 41, 51, 60]

iterator = gen()
array1 = (x for x from iterator when x %% 2 == 1)
array2 = (x for x from iterator)

ok array1.join(' ') is '41 51'
ok array2.length is 0