[CS2] Fix handling of parameters that are function calls

[GeoffreyBooth]

Fixes #4406. Also revises output of generator functions to be function* rather than function *, to follow accepted idiom.

@connec, any notes?

[lydell]

I have a little question, but I don't think it should hold up a merge.

[lydell]

So a call does not count as "complex"? What happens if we change .isComplex() to also check for calls instead?

[GeoffreyBooth]

I just couldn’t remember what isComplex meant 😄 But I did remember that we documented it here, which says:

If node.isComplex() is false, it is safe to use node more than once. Otherwise you need to store the value of node in a variable and output that variable several times instead. Kind of like this: 5 is not complex. returnFive() is complex. It’s all about “do we need to cache this expression?”

So there it is—returnFive() is exactly our use case. Thanks for the suggestion!

[lydell]

Nice!

[connec]

Bit of a nit-pick but the check for @value? is redundant (@value instanceof Call doesn't depend on the existence of @value).

[connec]

Neat fix 👍 made one small change request 😄

[GeoffreyBooth]

Thanks @connec, I simplified it. I guess this is ready for merge?

@lydell, how would you feel about renaming isComplex to needsCaching?

[lydell]

@lydell, how would you feel about renaming isComplex to needsCaching?

That's a really good idea! However, I'm sure there's one or two places where .isComplex() is used to check for something else – perhaps wrongly. If you feel like it, go through each of the 40 occurrences of isComplex in nodes.coffee and see if it makes sense.

[connec]

Ugh, pity I didn't think of this sooner: this should probably just be @name.isComplex() or @value.isComplex(), the following is still broken:

i = 0
({ a = ++i }, b = ++i) ->

It generates:

// Generated by CoffeeScript 2.0.0-alpha
var i;

i = 0;

(function(arg, b = ++i) { // 'b' will be set first (b === 1)
  var a, ref;
  a = (ref = arg.a) != null ? ref : ++i; // 'a' will be set second (a === 2)
});