Fix inconsistencies when invoking functions with implicit objects

[dohse]

Modifies AST by moving values from within implicit objects to surrounding function call.

Fixes: #618
Related: #1008 #873 #631

[michaelficarra]

no tests?

[jashkenas]

Hey donjones ... thanks for this patch -- very, very exciting stuff. Not being able to write call a: b, c was definitely a nasty bit of the previous grammar. It's merged to master.

I'm not entirely comfortable with doing the rewriting at CallNode compile time -- it should probably be put as a step in the Rewriter, even if it takes an additional pass to accomplish.

Merged at SHA: 53dc1f2. Thanks again.

[TrevorBurnham]

Quoting satyr at 618:

• f a:b, c is valid, but [a:b, c] isn't.
• f a:b, c+1 still doesn't work.
• f a:b, g c still doesn't work.

The old behavior was actually more consistent.

The question is: Is this purely a special-case hack, or is it a starting point for a more general fix? It should be kept on master only if it's in the second category.

[michaelficarra]

I agree with satyr and TrevorBurnham here: if we can't (or won't) make it consistent and working for those cases, we should revert this.

[dohse]

these cases break the hack, but they are reported as errors at least. it was very easy to shoot yourself into the foot with the old behavior.

f a: 1, b, (c), f a: 1, b {c} and f a: 1, b, [c] are inconsistent to f a: 1, b, c in the old behavior.

what about disabling implicit values in implicit braces in all positions?

i am currently trying to improve the fix. but i need to take another route, because the rewriter is getting in my way.

[dohse]

i now tried a grammar based approach, it just works for one-liners, because the rewriting needs to be changed, (but that code looks pretty scary). For one-liners it should work with every possible combination.

this version is in: https://github.com/donjones/coffee-script/tree/implicit-object-grammar-fix
would be happy to receive comments.

[jashkenas]

Yep -- we'll revert it if we can't make it consistent. And it's definitely going to be on master only, not 1.0-stable -- it's a backwards-incompatible enhancement.

For what it's worth it now handles simple array expansions as well:

list: [a: b, c]

[satyr]

satyr/coco@4c55b05

Simply swapping addImplicit* order did it for Coco.

$ coco -bpe 'f a:b, c+1'
f({
  a: b
}, c + 1);

[ghost]

Sorry I haven't done this yet! I want to run my app (not just the tests) to check if everything is working properly. But, on the date this feature got in, the app was in the middle of a restructuring... Then I got caught up in some other urgent stuff...

Finally things have settled down. Today (and for the next few days) I'll be finishing the restructuring... By tomorrow I should be able to run the relevant tests. And, in a few days, I'll be able to run the whole app to check if this feature works well with the several code variations I've got.

[aeosynth]

Here's another related parse error:

f a: b, c.d

edit: and another:

f a: b, c[d]

[ghost]

(Sorry for taking so much time to come back with my findings -- I've been really busy fixing my app's bugs...)

After checking my source code for some time, I realize that the main (and possibly only) use-cases I have are in this form:

fn(str1[, str2], hash, array or str)

So I guess there's no point in me creating another version of my source code without the { }'s just to test these cases...

[jashkenas]

With the paren/brace merge in the Rewriter, these are all compiling successfully now.