[CS2] Fix #4631: Expansion that becomes rest parameter causes runtime error

[zdenko]

Closes #4631.
After added support for the spread dots on the left side, dots in array slice, e.g. ([a...b]) were misinterpreted and the code was compiled to an implicit call. The error occurred when there was a space around the dots.

Example:

# wrong
arr[a ... b] => arr[a(...b)]

# correct
arr[a ... b] => arr.slice(a, b)

[GeoffreyBooth]

LGTM. @helixbass? @connec?

[connec]

My only suggestion is to add a regression test.

Also, does arr[(a ...b)] work as expected (e.g. arr[a(...b)]?)

[zdenko]

@connec Tests are added. Detection of the implicit call with dots on the left is improved as I also found few margin error cases, i.e. { a:1, b ... }

[helixbass]

@zdenko why and @tag(i + 2) in IMPLICIT_CALL? Seems a little arbitrary and tests don't fail without it

Also there's trailing whitespace in the preceding line. I'd recommend setting up your editor to highlight trailing whitespace so it's obvious

[zdenko]

@helixbass tests don't fail because there are no many tests for spred dots on the left side.
@tag(i +2) is needed to catch cases like { a:1, b ... } (note the space after the b).
In this case tag (b) is in IMPLICIT_FUNC and nextTag is ..., so I need to check@tag (i + 2). Otherwise, the rewriter will compile this into
[IDENTIFIER b] [CALL_START (] [... ...] [CALL_END )].

[connec]

Maybe we should add a test for it?

[zdenko]

I was thinking the same, but I'm not sure what would be a better option: a) put all tests for spread dots in one file, or b) add test cases in files (e.g. arrays, assignment, functions,...).

Basically, we need to cover all cases with spread dots preceding by space:

{ a: 1, b ... }

{
  a:1
  b ...
}

f = (a ...) ->

[
  a
  b ...
]

I would opt for option (a).

[connec]

I guess with the current test layout you'd need to put some in:

• arrays (array spreads)
• assignment (array/object rest destructuring)
• functions (argument rest arguments)
• function_invocation (spread arguments)
• objects (object spreads) - though there are no object spread tests there currently?
• ranges (disambiguating spreads from ranges)
• slicing_and_splicing (disambiguating spreads from slices and splices)

That is quite spread out, but probably the easiest thing to do for now. We may want a new rests_and_spreads test file where we can move all the rest/spread tests so all these cases are in one place.

[GeoffreyBooth]

@helixbass or @connec, any further notes?

[helixbass]

@GeoffreyBooth my comment still stands. No tests fail when removing and @tag(i + 2) in IMPLICIT_CALL and without diving fully into this myself I'm not super sold by @zdenko's reply

My comment was largely prompted by it seeming like using IMPLICIT_CALL there couldn't really make semantic sense - quoting the source, IMPLICIT_CALL is: If preceded by an IMPLICIT_FUNC, indicates a function invocation. The way it's being used here, it's not (immediately) preceded by an IMPLICIT_FUNC (but rather by ...), so I don't see how there could be a grammatical basis for using IMPLICIT_CALL here rather than something more specific. But I'm not prepared to evaluate that more fully until there are tests giving some sort of explanation of why it's there at all

[zdenko]

@GeoffreyBooth, @helixbass I'm preparing tests for these cases. It shouldn't take much longer.

[GeoffreyBooth]

@zdenko thanks for all the extra tests!

What about @helixbass’ comment above, about not needing and @tag(i + 2) in IMPLICIT_CALL?

Also do you mind taking a look at #4260? It seems possibly related to this.

[zdenko]

@GeoffreyBooth new tests are cases for and @tag(i + 2) in IMPLICIT_CALL.
When left side spread was added it didn't occur to me that people might put space before/after dots.
One example of this is #4631, e.g. array ranges [a ... b].
So, when I checked other possible places with spread dots, I've found that rewriter misinterpreted implicit calls when dots followed IMPLICIT_FUNC tag.
Example:

obj = {a:1, p ..., b:2}

({
    a: p
    b
    r ...
 }) ->

In these cases, rewriter would compile p ..., into p(..., {b:2}) and r ... into r(...).

You might also try these in the current branch:

coffee -te '({a, p ..., b:2}) ->'
coffee -te '(a, r ...) ->'
coffee -te 'obj1 = {a, f() ...}'

Perhaps I didn't explain the need for @tag(i + 2) check well, or I was too terse.
Basically, checking of @tag(i + 2) is not needed for recognizing the implicit call, but to prevent misinterpretation in other cases, i.e. when the code is not implicit func call.
So, in the current branch, this will work as expected:

f ...     a             #  f(...a) 
f() ...   b             #  f()(...b) 
f? ...    c             #  if (typeof f === "function") { f(...c) }
h[0] ...  d             #  h[0](...d) 

I don't think there were any test cases before with space before/after the dots, so this is the reason no test failed.

[GeoffreyBooth]

@helixbass what do you think?

This is the last PR holding up 2.0.0.

[helixbass]

@zdenko @GeoffreyBooth ok I see how it's being used. It's really the set of tokens that can start something that comes after a left-... grammatically. I think reusing the name IMPLICIT_CALL makes it a little confusing for the reader. But I don't know a better name and the tokens in IMPLICIT_CALL seem close enough to the right set of tokens to get the job done

Found another object spread destructuring bug and just opened as #4651 so probably want to fix that too before 2.0.0?

[GeoffreyBooth]

Thanks for that other ticket @helixbass. In the interest of taking one thing at a time, do you think we can merge this PR in? And #4651 can get its own PR.

[helixbass]

@GeoffreyBooth ya I didn't mean to imply that this should wait, they're unrelated