IcedCoffeeScript 3, lots of love.

zapu commented 8 years ago

Brought back old iced and iced_advanced tests, added plumbing to Cakefile to execute async tests.

Code generation fixes. Removed @bound = true when emitting inner generator closure for functions with await, as it broke contexts. Fixed passed_deferral and iterator scoping by forcing scoped declaration with { param: true } options.

Added autocb support, sort of. Still unsure how to apply it cleanly without patching a lot of coffee-script code. autocb support is definitely the messiest of this pull request. It would be cool not to ditch the functionality, though, because of compatibility with current iced code.

Added arguments support, broken right now - _arguments temp variable is properly created with freeVariable but Literals themselves do not reference variable created, instead they use hardcoded _arguments.

Right now awaits in expressions do not work (this makes "loops respect autocbs" and "can await in expressions" fail, possible others?). Emitted closure is not iced-transformed. Does ast walking fail here?

maxtaco commented 8 years ago

Thanks for this progress, @zapu.

The big concern with autocb is that (1) it was a cause of most of the ICS bugs; and (2) if it complicates the patch too much, maintenance will be a drag going forward.

Also, I thought in the current iced2 implementation, awaits in expression do not work --- which test fails? I couldn't find "can await in expressions" in the tests, can you send me a link?

Thanks!

zapu commented 8 years ago

Thanks for looking into my patches!

Looks like I might have been mistaken about awaits in expressions.

Consider the following code (there is a test "loops respect autocbs" where something similar happens):

bar = (autocb) ->
  for i in [0..10]
    await setTimeout defer(), 1
    ok = true

bar ->
    console.log arguments

I assumed autocb would be called with the entire for as a parameter. This is indeed the case in my iced3 work, as it just looks for last non-comment expression in block to turn into autocb call. But that's not what happens in iced2, for reasons yet unknown to me. Could you shed some light on it?

Because of this confusion, I added a test in iced.coffee in this PR.

atest 'can await in expressions', (cb) ->
  res =
    for [1..10]
      await delay defer()

  cb true, {}

But if it's not legal iced, it's fine!

autocb is indeed quite a patch and gave me some headaches. It took most of my time on it, actually. But most of the tests depend on autocb and I wanted to get the tests running as they were, before moving forward with other features.

IMO, autocb is really cool feature, although quite a "magic" one. I don't really like where behavior of the language is based on identifier names alone (this is probably my biggest pet peeve in scala, AFAIR their operator precedence depends on operator name). I'd try to keep it and work out the most minimal patch to get it to work, preferably some ast transformations instead of patching individual functions. That's what I was going for in this PR, too. But I'm sure it can be more minimal.

zapu commented 8 years ago

Also please see this pull request: https://github.com/maxtaco/iced-runtime/pull/5

It brings down number of failing test to 16, if my Cakefile counts them correctly - which it doesn't, in some cases, but I'm unsure if that bug is manifesting itself here.

zapu commented 8 years ago

After last commits, we are down to 11 failing tests :confetti_ball: (before: 16 failures). Mostly in iced library (Rendezvous, etc.), I have not looked into that yet. We have one regression in assignment.coffee though :sob:

Overview:

Improved autocb generation. It will descend into Blocks and try to transform last non-comment node to Return, unless it contains Await. Created Return (or Returns that were already in there) are compiled to autocb calls.
arguments in iced functions support. Kind of quick and dirty, needs more tests to see where it fails.
Compilation error when await is used in expressions. Based on what iced2 does, hopefully I did not miss anything. "await expression assertions 1" passes now.
I tried to reorganize new methods a bit in hopes for cleaner rebases with coffeescript mainline later.

While working on those, I noticed not-friendly behavior of autocbs when someone actually decides to call it - either by mistake or on purpose. Observe:

func = (autocb) ->
    autocb()

func = (autocb) ->
    autocb()
    return

func = (autocb) ->
    return autocb()
    return 10

Emits:

// Generated by IcedCoffeeScript 108.0.9
(function() {
  var func;

  func = function(autocb) {
    autocb(autocb());
    return;
  };

  func = function(autocb) {
    autocb();
    autocb();
    return;
  };

  func = function(autocb) {
    autocb(autocb());
    return;
    autocb(10);
    return;
  };

}).call(this);

I managed to get the same behavior in iced3 branch, but I believe it's far from optimal (another autocb gotcha, huh?). What it should be doing, IMO, is

not transform autocb() into autocb(autocb()); return; - maybe the user wants to call the callback and dispatch some more work after. They shouldn't use autocb then, though.
not autogenerate autocb(); return; when autocb() is already the last statement in block.

or:

emit an error when user tries to call autocb manually. Or when, e.g., they try to reassign autocb, too? Maybe autocb should be prohibited as literal anywhere when compilation is in "await + autocb" scope.

maxtaco commented 8 years ago

To answer the first question, regarding:

bar = (autocb) ->
  for i in [0..10]
    await setTimeout defer(), 1
    ok = true

bar ->
    console.log arguments

We previously in iced2 had a very hairy patch to CS in which we got something like this work. You could also do things like:

x = await foo defer _

Which was just shorthand for:

await foo defer y
x = y

And then you could compose to get:

x = for i in [0...10]
   await foo defer _

All of this worked in about 80% of cases, but there were some in which things didn't work or didn't behave as you would expect, and I threw up my hands and stripped out all of the code. For instance:

if await foo defer _ 
  console.log "hi"

This didn't work, and major surgery was required to get it working. Also, I hated the fact the control flow wasn't explicit:

x = [ (await bar defer _), (await foo defer_)]

Do those happen in parallel, or in serial? If in serial, in DFS or BFS order? I just decided it wasn't worth it, and made the decision that awaits or any of their ancestors in the AST are statements rather than expressions (like try, break and return). That made everything so much easier to think about, and the patch much much smaller and easier to maintain.

So getting back to your example with autocb, you seem to be going down that whole path again --- treating awaited statements as expression. I'd really like to avoid it.

Are you sure I can't convince you that we should drop autocb? With escape short circuiters, I really don't find a need for it.

zapu commented 8 years ago

Thanks for explaining this. autocb and "awaits as expression" were giving me problems already and I haven't even bumped into half of the things that would have became an issue.

I've submitted a new pull request with autocb removed and some other fixes.

zapu commented 8 years ago

Hm, was one commit cherry picked from this PR? I think it should be entirely obsolete. This was the dead-end branch in which I was re-implemeting all the autocb quirks and probably introducing more. Am I missing something?

maxtaco / coffee-script

IcedCoffeeScript 3, lots of love. #166