Fix #4878: Compile error when using destructuring with a splat or expansion in an array #4879
Conversation
test/assignment.coffee
Outdated
| eqJS ''' | ||
| foo = (list) -> | ||
| [first, ..., last] = list | ||
| ''', ''' |
There was a problem hiding this comment.
Let’s please avoid eqJS tests whenever possible, they break unnecessarily with minor updates to the output. You could instead use:
fn = (list) ->
[first, ..., last] = list
[first, last]
arrayEq [3, 5], fn([3, 4, 5])and so on.
There was a problem hiding this comment.
This example would compile on master.
This PR fixes the issue where destructuring is the last line.
foo = (list) ->
[ first, ..., last ] = listBut, anyway, you're right. I'll change the tests.
src/nodes.coffee
Outdated
| isSplat = splats.length | ||
| isExpans = expans.length | ||
| isSplat = splats.length > 0 | ||
| isExpans = expans.length > 0 |
There was a problem hiding this comment.
Could splats or expans possibly be undefined? This seems safer (and slightly more performant):
isSplat = splats?.length isnt 0
isExpans = expans?.length isnt 0
test/assignment.coffee
Outdated
| f4 = (list) -> | ||
| ([first, ...rest] = list); rest | ||
|
|
||
| arrayEq f1(arr), ['d'] |
There was a problem hiding this comment.
In 2.1.1, f1(arr) returns [ 'a', 'b', 'c', 'd' ]
There was a problem hiding this comment.
In 2.1.1 this
f1 = (list) ->
[first, ..., last] = listcompiles into
f1 = function(list) {
var first, last;
return first = list[0], last = list[list.length - 1], list;
};So, the returned value is list.
In 2.2.0 it's compiled to
f1 = function(list) {
var first, last;
return [first] = list, [last] = slice.call(list, -1);
};The returned value is `[last]'.
There was a problem hiding this comment.
Yes, but [first, ..., last] = list is shorthand for return [first, ..., last] = list, which looks to me like it should return the full array. As in, returning just [last] is wrong.
do -> [a, b, c] = [1, 2, 3]
# returns [1, 2, 3]There was a problem hiding this comment.
Yup. Sorry, my mistake. Not enough coffee in the system 😪
There was a problem hiding this comment.
Not enough coffee in the system
Isn’t that why you’re here? 😝
test/assignment.coffee
Outdated
|
|
||
| foo = (list) -> | ||
| ret = | ||
| if list.length |
There was a problem hiding this comment.
I don’t like using list.length being nonzero as a proxy for truthiness. If you’re evaluating that list.length isnt 0, you should be explicit about it. I think it’s a lot more readable than just list.length or other hacks like !!.
There was a problem hiding this comment.
Right. I copied the example from #4878.
|
@GeoffreyBooth the last commit fixes the returned value, but I just found another issue, which I'll fix later today. |
|
No worries. The tests we have at least prove that this syntax compiles; it would be nice to also somehow prove that |
|
@GeoffreyBooth the “issue” I noticed before is a false alarm. foo = (list) ->
ret =
if list.length > 0
[first, ..., last] = list
[first, last]
else
[]
foo = function(list) {
var first, last, ret;
return ret = list.length > 0 ? (([first] = list, [last] = slice.call(list, -1), list), [first, last]) : [];
};I thought that this part |
2 participants
Fixes #4878.