maxtaco / coffee-script

IcedCoffeeScript
http://maxtaco.github.com/coffee-script
MIT License
726 stars 58 forks source link

Fix deferred loops, issue #162 and #89 and more #164

Closed zapu closed 8 years ago

zapu commented 9 years ago

(all those issues were only in iced loops)

To observe the issues, try the script I created:

https://gist.github.com/zapu/6c05772716331713b2c2

It executes different loops with and without await/defer and compares results.

Loops like 'for i in array by s' did not use step parameter at all. Attempted fix adds support for any kind of step parameter, negative or positive. Optimization was added that triggers if step is a literal number. This was reported in issue #162 by @vird and #89 by @davidbau.

Loops that used ranges 'for i in [a..b] by s' were also incorrectly handled, that is, their behavior was different than coffee-script one. So one could have a loop that behaves differently when await/defer was added to the loop body. Code generation was altered to match coffee-script's behavior. Also step value is now saved to local variable '_step'. This fixes an issue when provided step is a function call - before, the call block would have been executed every iteration.

I also added tests for different loops to iced.coffee. The test also checks for behavior of normal loops, which seems redundant here. There are already tests for that in comprehensions.coffee so I tell me if I should remove them - they were mostly useful in process of discovering this issue and might be obsolete now.

zapu commented 9 years ago

Ideally, compileNode and icedCompileIced should share code for creation of step and condition, but since normal loop generator aims to create very dense code, it seems hard to do.

maxtaco commented 8 years ago

Thanks @zapu, great work!

maxtaco commented 8 years ago

BTW, the iced3 branch is the future. It emits ES6 with a lot less of a patch, and achieves the above goal of sharing much much more code. There are a few todos -- the ugliest is ES6to5 conversion, but if we can skip that, then it's nearly ready. I just did a rebase (after about ~2 months of collecting dust), and it applied nearly cleanly, which is great news.

zapu commented 8 years ago

I've been looking into ES6 generation as well. I even started my own little branch and tried to implement it myself, based on your findings and ideas:

https://github.com/zapu/coffeescript/commit/db770a8db21de756e716deb44f5b4ed3072e9bea

It mostly works, but I didn't bother to implement all of the slot types and there are probably some obscure edge cases I did not even test. I did not manage to do top-level awaits as well, it looks doable at es6 side (but needs wrapping everything in generator block), but probably needs some more coffee compiler trickery.

First I wanted to do iced translation as a separate pass, kind of like current icedTransform in

fragments = icedTransform(parser.parse(tokens), options).compileToFragments options

but even in separate file, and not touch rest of the compiler, which would make it trivial to plug into any revision of coffee-script (as long as parser doesn't change, probably). But it seemed like I was reimplementing a lot of tree walking that is already there in nodes.coffee. So I abandoned it and decided to see how much work and how invasive would modifying nodes.coffee be.

I will try to contribute to iced3 branch from now on. How are you planning to do 6to5 translation? I was testing some outputs with https://babeljs.io/ and things seem to work! And of course the output works out of the box in latest node.

maxtaco commented 8 years ago

I've heard from the team here that babel is the way to go... But as you say, maybe it's worth ignoring, since we have coverage with out-of-the-box node and the major browsers. If you really want to work with legacy browsers, you can supply the 6to5 yourself?

zapu commented 8 years ago

What's the issue here, though? I assume source maps can give some troubles, but it will be even worse when people do the 6to5 of generated code themselves. Please confirm if I'm understanding this correctly.

maxtaco commented 8 years ago

Source maps and also just the plumbing from the command line is all.

maxtaco commented 8 years ago

But otherwise, no big issue, you are right.

zapu commented 8 years ago

"Merging" the source map seems tricky but I have no experience in that. I'll try this week!

Thanks for getting back to me!

maxtaco commented 8 years ago

Awesome! Let me know if I can help in any way

BTW, you'll also need to refer to the iced3 branch of iced-runtime to use what's currently there.

maxtaco commented 8 years ago

(And thanks in advance!)