[CS2] Fix #4260 and #1349: Splat error with soak properties or expressions

[zdenko]

This is a fix for #4260 and #1349 when invalid JS is compiled if Splat contains soak accessor (?.), or if statement.

Before:

// [a if b...]
[
  ...  if (b) {
    a;
  }
];

// [a?.b...]
[
  ...  if (typeof a !== "undefined" && a !== null) {
    a.b;
  }
]

// arr.push(error.data?.errors...)
arr.push(...if ((ref = error.data) != null) {
  ref.errors;
});

After:

// [a if b...]
[...(b ? a : [])]

// [a?.b...]
[...(typeof a !== "undefined" && a !== null ? a.b : [])]

// arr.push(error.data?.errors...)
arr.push(...((ref = error.data) != null ? ref.errors : []));

[GeoffreyBooth]

@zdenko This should get some tests.

[GeoffreyBooth]

I’ve added some tests based on #4260 and #1349. Hopefully this PR is ready now. @lydell or @connec, do you care to take a look?

Technically this is a tiny breaking change, so it would be better to slip this in before 2.0.0:

console.log [a?.b...]
# Old: console.log([...typeof a !== "undefined" && a !== null ? a.b : void 0]);
# New: console.log([...(typeof a !== "undefined" && a !== null ? a.b : [])]);

Previously the else case was undefined, whereas now it’s [].

Interestingly, the tests I added fail in the CS1 version of this PR. I’m not sure we should bother fixing this in 1.x.

[connec]

Should we add tests for the leading splat variants?

[GeoffreyBooth]

Thanks @connec. Any other notes?

[connec]

Getting a bit pedantic now, but could we add spaced prefix ... as well? Other than that, this LGTM 👍

[zdenko]

could we add spaced prefix ... as well

Test added.

[connec]

Absolutely last, final thing: this could also do with a spaced prefixed version.

[zdenko]

Added. Sorry, forgot to save the changes.

[connec]

It does seem kind of funky that [(a?.b)...] still compiles to [...(typeof a !== "undefined" && a !== null ? a.b : void 0)] (e.g. parens break the change from void 0 -> []), but I think it's good that [(a ? undefined)...] compiles as written.

It's still possible to generate invalid output by wrapping an if statement in parens and then making a soaked access:

[(a if b)?.c ...]
# var ref;
#
# [
#   ...  if ((ref = (b ? a : void 0)) != null) {
#     ref.c;
#   }
# ];

I feel like this should be fixable by doing something with o.level when compiling the name of the splat (LEVEL_LIST maybe?).

[zdenko]

@connec I've made some improvements and fixed the issue with wrapped if statement in parens.

[(a if c)?.b...]
###
[...((ref = (c ? a : void 0)) != null ? ref.b : [])]
###

[zdenko]

@connec

parens break the change from void 0 -> []

Why do you think parens are breaking the change from void 0 -> []?

Even this [a?.b] compiles with void 0, e.g. [typeof a !== "undefined" && a !== null ? a.b : void 0].
If a is undefined the result would be [undefined], and in that case the array won't be empty, e.g. [undefined].length is 1.

[connec]

I just mean that

[(a?.b)...]

Compiles to

[...(typeof a !== "undefined" && a !== null ? a.b : void 0)]

[GeoffreyBooth]

@connec I’m not sure I follow your last comment. Have all your concerns been addressed?

There have been so many bugfixes lately that I think we’re going to release a 2.0.0-beta5 before going straight to 2.0.0.

[connec]

I've taken a closer look and there are still a couple of problems:

1. There is a simpler fix for adding parens. Firstly, Splat should not have a compileToFragments method, but rather should have a compileNode method. With just that rename, the output for [a?.b...] changes to:

   [...typeof a !== "undefined" && a !== null ? a.b : void 0]

   The reason the above rename works as it does is because the default implementation of compileToFragments accepts a second argument for the current level, which it merges with the options before calling compileNode. In Arr::compileNode, each entry in the array is compiled with compileToFragments o, LEVEL_LIST, which was being ignored as the compileToFragments definition in Splat wasn't using it. By renaming Splat::compileToFragments to Splat::compileNode we get that functionality back.

   Secondly, with correct use of the LEVEL_* flags we can add the required parenthesis. Since ... is an operator, the name should really be compiled with LEVEL_OP, if we do this (e.g. @name.compileToFragments o, LEVEL_OP) then we get the desired compilation:

   [ @makeCode('...'), @name.compileToFragments o, LEVEL_OP ]
   # ...(typeof a !== "undefined" && a !== null ? a : void 0)

   In summary:

   diff --git a/src/nodes.coffee b/src/nodes.coffee
   index 677d4df0..b13468f8 100644
   --- a/src/nodes.coffee
   +++ b/src/nodes.coffee
   @@ -2872,9 +2872,9 @@ exports.Splat = class Splat extends Base
      assigns: (name) ->
        @name.assigns name
   
   -  compileToFragments: (o) ->
   +  compileNode: (o) ->
        [ @makeCode('...')
   -      @name.compileToFragments(o)... ]
   +      @name.compileToFragments(o, LEVEL_OP)... ]
   
      unwrap: -> @name
   
   

2. The switching of void 0 to [] is not robust. We can't just replace all the fragments, since that means things like the following will not compile correctly:

   [ f(a?.b)... ]
   # [...f(typeof a !== "undefined" && a !== null ? a.b : [])]
   # Note that `[]` is a function argument

   More generally, any subexpression containing an If whose alternative is void 0 will be replaced with [], which is incorrect. What we would need in order to support this would be some way of identifying all the 'possible return expressions' for the Splat's @name, and in the case that they are not defined they should become new Arr. Unfortunately there is currently no mechanism in the compiler for figuring out these 'possible return expressions' afaik, and walking the tree in Splat::compileNode is probably too much for this case.

   The same mechanism would be needed to implement undefined/null values in interpolated expressions should return "" #1406 (and its most recent duplicate that I can't find atm).

[GeoffreyBooth]

The switching of void 0 to [] is not robust. We can’t just replace all the fragments, since that means things like the following will not compile correctly

Any idea how to do this properly then? My first thought is to take the expression that toss parentheses around it followed by or [], e.g.:

[ f(a?.b)... ]
# Gets mutated in nodes.coffee, before compileNode/compileToFragments, to:
[ ( ( f(a?.b) ) or [] )... ]
# And *then* compiled.

This will result in a noisy output:

[...(f(typeof a !== "undefined" && a !== null ? a.b : void 0)) || []];

but it seems to work. What do you think?

[zdenko]

@connec you're right about the LEVEL_OP and compileNode. LEVEL_OP can be also used in current @compileToFragments, e.g.

if not (@name instanceof Value) or not @isAssignable()
-  @name = new Value new Parens @name
-  fragments = @name.compileToFragments o
+  fragments = @name.compileToFragments o, LEVEL_OP
else
  fragments = @name.compileToFragments 0
 
[ @makeCode('...')
      fragments... ]    

I've tried to find a 'smart' way to output [] instead of void 0, and, since there is no mechanism to specify return expression, replacing the fragment seemed as a good call.

I did some changes and add additional tests.
Basically, in Splat, void 0 is replaced with [] only for [a?.b...] and f(a?.b...), i.e. where Arr is expected.

[a?.b...]
###
[...(typeof a !== "undefined" && a !== null ? a.b : [])]
###

[ f(a?.b)...]
###
[...f(typeof a !== "undefined" && a !== null ? a.b : void 0)]
###

arr.push(a?.b...)
###
arr.push(...(typeof a !== "undefined" && a !== null ? a.b : []))
###

arr.push(f(a?.b)...)
###
arr.push(...f(typeof a !== "undefined" && a !== null ? a.b : void 0))
###

[connec]

@GeoffreyBooth that would be more robust I think... basically checking if @name instanceof Value and any properties are .soak, and if so wrapping name with new Op '?', @name, new Arr...

@zdenko I still think the method in Splat should be compileNode, rather than compileToFragments, as is the case with almost all the other nodes. I'm not sure why it was originally implemented in compileToFragments.

Re: [] replacement - I'm not sure if I'm really a fan of this feature in general. It feels to me very similar to the issues asking that undefined be treated as an empty string in interpolations. It seems odd to me that [a?.b...] would compile differently to [(a?.b)...], or that the latter would not evaluate a?.b before applying the Splat. If it was part of the Splat operators behaviour to treat null/undefined as an array (e.g. [undefined...]) then that would make sense, but is also kind of weird I think.

[GeoffreyBooth]

Re [] replacement . . . at first I was in favor of it, but now I’m not so sure. You’re right in that I feel like it’s a corollary to undefined in a string interpolation (see #1406 and its duplicates). So I thought, well, now that ES2015 has template literals, how do they handle undefined in an interpolation?

`a${undefined}b` // → "aundefinedb"

In other words, just like CoffeeScript does. So what about splats/rest?

[...undefined]   // Uncaught TypeError: undefined is not iterable

Now granted, maybe both of these cases are scenarios where CoffeeScript could improve upon JavaScript, but that’s not the goal of this PR; and that’s not a goal of CoffeeScript 2, which is trying to hew closer to ES2015’s way of doing things (like the unfortunate change regarding function parameter default values).

So I guess what I’m saying is, we should get rid of the “replace undefined with []” code, that’s buggy anyway, from this PR. And if that’s a feature we want to add, it should get its own discussion and separate PR.

[zdenko]

Agree with the above.
I started to work on an alternative version by adding the .altVal property to Access.
In Splat I'm checking the tree and property altVal: "[]" is added to Access with .soak. This value is then checked in Value::unfoldSoak and passed into If.
Although it's working, it got me started to think of other cases (I wasn't familiar with #1406), where similar behavior would be expected. It seems too much work.
And last but not least, this will also break existing code out there, where one already took care of passing proper value, e.g.arr.push(a?.b ? [c]...)

[GeoffreyBooth]

I think this might be one of those cases where we prefer something to behave as we might assume the user always wants it to, e.g. undefined in an interpolation always becoming an empty string; but others out there might very much want the runtime to throw an exception, because they don’t want undefined magically transforming into an empty string or an empty array. Kind of like how people want type checking; it’s a feature that the runtime throws an exception if a variable is undefined rather than the string or array/iterable that you expect it to be. If it’s potentially valid that the variable could be undefined, well, then you should catch that case; the runtime shouldn’t try to catch it for you. I think that’s just a general principle of JavaScript.

@zdenko if you were to implement the “convert to []“ approach, I think the method in @connec’s comment is probably the easiest way to go. But I don’t think it’s a good idea, as it deviates from the behavior people expect in JavaScript.

I made one little change to use LEVEL_OP per @connec’s comment above. The tests all still pass. I think this is good now?

[zdenko]

@GeoffreyBooth I think LEVEL_OP is not needed in Splat::compileNode, e.g. tests pass. I guess it doesn't hurt though.

Regarding the "convert []" functionality, I'm not sure this is the way to go. I think it should be up to the user to take care of the correct value, .e.g [a?.b ? []...], foo(a?.b ? "") ...

Anyway, converting to [] in Splat should go in separate PR.

[connec]

This patch looks really clean to me now 👍 (though moving the constructor threw me for a second 😛)

It would be interesting to consider swapping undefined for [] in a separate PR, but @zdenko's example of providing an explicit fallback (arr.push(a?.b ? [c]...)) would need to work also.

[GeoffreyBooth]

Thanks @connec. Any opinion on whether we should also merge this into 1.x? We haven’t had any other PRs into that line since 1.12.7, I’m not sure this is significant enough to warrant 1.12.8. Though we could merge it into the 1.x branch and just let that branch sit there unreleased until someday there might be enough patches to make it worth releasing a 1.12.8, or let people use 1.x-latest indefinitely by installing that branch directly from GitHub.

Or we could just say no more 1.x releases, period, and leave it at that.

[connec]

@GeoffreyBooth #4643 - but you've already seen that ;)

I'm happy with merging it to 1.x I think, it seems like a pretty simple backwards compatible fix. Whether it's enough to be released I'm not sure. Personally I don't mind tiny releases with simple fixes, but I don't know what the process is.