yield keyword does not work in for loop expression #3665
Comments
|
This will need to be addressed before the next release goes out. Let's add a failing test case for it — and other situations where we generate "hidden" function wrappers. |
|
I propose in the case that yield or any other es6 feature is detected by the lexer, this tells the compiler to use es6 scoping: if (true){ // run code in block scope
let var1, var2, var3; // declare scope variables here
// compute value within block scope here ...
// return value or set it to variable outside block scope
}Any other ideas? |
|
@GabrielRatener I doubt that approach would fly with Jeremy and it might be more work. |
|
@xixixao I have been puting some thought into alternatives to current function closures that could be used, and this is the best I could come up with. This seems to be a difficult bug to fix. The tricky part is determining when to return or set a value directly, otherwise block scopes with |
|
@GabrielRatener are you sure? Looks right to me. ➜ CoffeeScript (master) ✗ cat generate-test.coffee && ./bin/coffee -bcp generate-test.coffee->
array = [1, 2, 3]
outvar = for i in array
yield i * 2
i * 2// Generated by CoffeeScript 1.8.0
(function*() {
var array, i, outvar;
array = [1, 2, 3];
return outvar = (function*() {
var _i, _len, _results;
_results = [];
for (_i = 0, _len = array.length; _i < _len; _i++) {
i = array[_i];
(yield i) * 2;
_results.push(i * 2);
}
return _results;
})();
}); |
|
@akre54 -- do we have a good test for it already? |
|
coffeescript/test/generators.coffee Line 50 in efca286 |
|
Is that a problem? |
|
Nah it just means I didn't pass anything to the |
|
Now I'm just confused, but whatever. Sounds like this is invalid. |
|
Simpler: |
|
@akre54 @jashkenas Sorry for the erroneous information, the closure created is actually another generator. This still does not work though. In the case of gogo = ->
yield 2
yield 3
a = for i in [1..3]
yield i
6
yield 4
yield a
b = gogo()
console.log b.next()
console.log b.next()
console.log b.next()
console.log b.next()
console.log b.next()
console.log b.next()
console.log b.next()
console.log b.next()This gives the following output: Remember, generator functions return iterators, not the expressions to the right of the return statements they contain. This is still a bug. |
|
@alubbe — What do you think about this wrinkle? |
|
This looks like it has more to do with list comprehensions getting wrapped in a closure than any yield-specific changes. Without the closure the yield would work as intended. I think it's downright odd that assigning the return value of the comprehension to a variable gets a closure ( |
|
@GabrielRatener The simple answer is to call Simplifying your example: gogo = ->
a = for i in [1..3]
yield i
'a'
yield a
yield 'b'
outer = gogo()
console.log first = outer.next()
# { value: {}, done: false } (the generator)
inner = first.value
console.log inner.next()
# { value: 1, done: false }
console.log inner.next()
# { value: 2, done: false }
console.log inner.next()
# { value: 3, done: false }
console.log inner.next()
# { value: [ 'a', 'a', 'a' ], done: true }
console.log outer.next()
# { value: 'b', done: false }
console.log outer.next()
# { value: undefined, done: true } |
|
@akre54 — You're missing the point here. It's not odd that CoffeeScript sometimes has to closure-wrap a complex expression ... that's part of our compilation strategy. It's also not a simple answer to call |
|
Sure. But I don't think you're getting what I'm saying. Why does ->
for i in [1..3]
yield i
'a'
'b'... not get a closure? Keeping the two consistent (dropping the extra closure from assignments) would solve this issue, no? |
|
@akre54 Because the for loop is not being used as an expression in that case. CS compiler detects when statements are used as expressions, and then wraps them in closures accordingly. |
|
No no I got that. What I'm saying is that it used to be that comprehensions assigned as expressions would still live in the same block scope (only wrapped in a for loop) as the outer body, the way that non-expression comprehensions are currently. Reverting to the old behavior would solve this issue with yield, but I imagine there was some reason the change was made in the first place (off the top of my head I can't think of anything and issue search is failing me). I know there was some discussion around explicitly asking for a closure using either the stabby arrow syntax or the |
|
Closure wrapping is used by CS to turn javascript statements into javascript expressions. I'l let someone else give a better explanation. |
|
A comprehension expression in coffeescript compiles to a number of Javascript statements. A closure is needed whenever a comprehension is used where Javascript statements would be invalid (e.g. as part of a larger expression). ###
no closure
###
i for i in [1..2]
-> i for i in [1..2]
-> a ; i for i in [1..2]
###
closure
###
return (i for i in [1..2])
a = (i for i in [1..2])
a = for i in [1..2] then i
(i for i in [1..2])\
.concat\
(i for i in [1..2])Hopefully the last example makes it clear why closures are sometimes needed: when a comprehension is used as a generic expression as part of a more complex expression, the closure is needed to evaluate the comprehension at the right point. The first case where there is no closure seems to be an optimisation: bare comprehension expressions at the end of a function don't need a closure, though it could have one. It seems it would be possible to optimise away the closure in the basic assignment case by compiling var a, i, _i;
a = [];
for (i = _i = 1; _i <= 2; i = ++_i) {
a.push(i);
}However, when the assignment is part of a generic expression |
|
I guess |
|
Hi! First of all it was surprise for me that ... yield 2 * 2 ...is compiled into ... (yield 2) * 2I think in most cases the intention of writing the former is since in JavaScript assert((function*(){ yield 2*2; })().next().value === 4);But it is another story (probably an issue). The other more serious problem that I see is collecting the argument of the Consider already mentioned so = require 'so'
fs = require 'then-fs'
readJSON = so (path) -> JSON.parse yield fs.readFile path, 'utf8'
writeJSON = so (path, value) -> yield fs.writeFile path, JSON.stringify(value), 'utf8'
main = so ->
names = ['a', 'b', 'c']
all = for name in names
path = "#{name}.json"
yield readJSON path # <--- HERE
yield writeJSON 'all.json', all
main().done()@alubbe, what would you expect to see as the result of this program? +1 for |
|
|
|
@GabrielRatener I've opened an issue: #3674 |
|
For what it's worth, CoffeeScript master doesn't support the |
|
@akre54 CS supports |
|
@GabrielRatener, yes exactly! I'm only afraid that @jashkenas said f = ->
a = for i in [1..3]
yield i * 2for I suppose that after 4 iterations of |
|
@GabrielRatener yes of course. right again :) |
|
@Artazor Yes, you are right, probably just a thinko on their part. |
|
Okay, so the order precendence is fixed now, see #3677 I've also tried to add Basically, it should check whether the for loop is occuring in an assignment and whether @jashkenas @michaelficarra Any pointers? |
|
@alubbe Will your PR fix all expression closures containing |
|
We are testing for and can confirm precedence regarding |
|
@alubbe Any progress? |
|
Unfortunately, no. Like I said, I know what needs to be fixed and how, but I have no idea what's the appropriate place within the code base. |
|
Ah, yeah, @alubbe It's not pretty. It might actually be better to have |
|
Appreciate the hard work @alubbe. Thank you. |
|
So, I'm imagining you'll probably want to make the change in the hairy heart of CoffeeScript: You can tell if it's being used as a value or as a statement (what you mean by "is occurring within an assignment") by checking the value of |
|
Notice that |
|
Then I guess I take it back about where this should be done. You'll want to insert the detection / closure-conversion-fixing into |
|
@jashkenas the problem is that we don't currently mark compiler-generated closures, so in the case of nested comprehensions we might need to detect if the outerleast non-generated scope is yielding or not |
|
Also, that For method could really get some lifting on the string generation things :) |
|
All compiler-generated closures are created in that method I've mentioned above. Hopefully the machinery can simply be inserted there. If not, a tag can easily be added. |
|
Should be fixed now - #3734 |
|
@jashkenas @alubbe Unfortunately there are cases, where this issue is still not fixed. Namely, when in switch or for expression This code (minimal for the sake of example): ->
foo = switch
when true then yield =>
bar = (yield @ for item in arr)compiles to this: (function*() {
var bar, foo, item;
foo = (function*() {
switch (false) {
case !true:
return (yield (function(_this) {
return function() {};
})(this));
}
}).call(this);
return bar = (function*() {
var i, len, results;
results = [];
for (i = 0, len = arr.length; i < len; i++) {
item = arr[i];
results.push((yield this));
}
return results;
}).call(this);
});but should compile to this: (function*() {
var bar, foo, item;
foo = (yield* (function*() { // <-- here
switch (false) {
case !true:
return (yield (function(_this) {
return function() {};
})(this));
}
}).call(this));
return bar = (yield* (function*() { // <-- here
var i, len, results;
results = [];
for (i = 0, len = arr.length; i < len; i++) {
item = arr[i];
results.push((yield this));
}
return results;
}).call(this));
});Notice the |
|
Hey @pepkin88 I have found and fixed the issue locally, and will write some more tests over the weekend to check for further issues with Could you create a separate issue for this bug please? |
|
Ok, I will. Created: #3882 |
When one tries to do the following
Inside a gennerator function, the loop gets wrapped in another generator closure and an iterator is returned to
outvarinstead of an array, I presume this is a bug.The text was updated successfully, but these errors were encountered: