Fix #4787: Destructuring of objects within arrays can generate invalid JavaScript

[zdenko]

Fixes #4787
Destructuring assignment with expansion and object destructuring within array compiles correctly.

[...,{a,b}] = arr

Before:

var a, b;
{a, b} = arr[arr.length - 1];

After:

var a, b;
({a, b} = arr[arr.length - 1]);

[GeoffreyBooth]

This is pretty surgical. @connec, any notes?

[vendethiel]

The first > could be >=?

[GeoffreyBooth]

That sounds like an improvement, yes.

Side note: Can someone who really understands LEVEL_TOP, LEVEL_LIST etc. write up an explanation on one of the wiki pages?

[zdenko]

@vendethiel, @GeoffreyBooth
The condition has basically just left and right side, and could be written as

if o.level > LEVEL_LIST or (o.level is LEVEL_TOP and isLevel and ...)

So, if we change first > to >=, the left side condition will be also true in cases like
a = (b=1) ->, where the level of assignment b=5 is LEVEL_LIST, and will be compiled to
a = function((b=1)) {}.

However, the condition could be simplified a bit and changed to

if o.level > LEVEL_LIST or o.level <= LEVEL_LIST and isValue and @variable.base instanceof Obj and not @nestedLhs and not (@param is yes)

or even to

if o.level > LEVEL_LIST or isValue and @variable.base instanceof Obj and not @nestedLhs and not (@param is yes)

As far as I understood values of the levels, they're used to control precedence of the compiled code fragments.
From the nodes.coffee

# Levels indicate a node's position in the AST. Useful for knowing if
# parens are necessary or superfluous.
LEVEL_TOP    = 1  # ...;
LEVEL_PAREN  = 2  # (...)
LEVEL_LIST   = 3  # [...]
LEVEL_COND   = 4  # ... ? x : y
LEVEL_OP     = 5  # !...
LEVEL_ACCESS = 6  # ...[0]

I guess, it can be confusing sometimes since level with value 1 (LEVEL_TOP) has higher precedence then 2, i.e. 1 > 2 .

[GeoffreyBooth]

Okay, so let’s wrap this up. @zdenko and @vendethiel, what do you think the best check is?

And @zdenko you should probably add a comment explaining the conditional, similar to what you’ve written here.

[vendethiel]

I'd say if o.level > LEVEL_LIST or (isValue and @variable.base instanceof Obj and not @nestedLhs and not @param)

[zdenko]

Agree. It's shorter and IMHO, somehow less confusing.

[connec]

This looks good to me. I'd suggest it could be a bit more readable if the condition was rearranged slightly:

if o.level > LEVEL_LIST or (isValue and @variable.isObject() and not @nestedLhs and not @param)

[GeoffreyBooth]

I appreciate the comment, but can you please explain why parentheses are required for those lower levels?

[zdenko]

@connec @variable.base instaceof Obj is needed for cases like {}.p = 0.
Changing not (@param is yes) to not @param cause two tests to fail, but I can't figure which ones exactly and why ¯\_(ツ)_/¯

@GeoffreyBooth I'm not sure exactly about lower levels. But, it seems that values for LEVEL_COND (e,g. Existence, If,...) and LEVEL_ACCESS (e.g. Slice, In) are already wrapped in parentheses elsewhere, before passing thru the Assign.
This leaves only LEVEL_OP, (e.g. +, -, ...) as in this case a = b - c = 2 which has to be compiled to a = b - (c = 2).
So, changing condition from o.level > LEVEL_LIST to o.level is LEVEL_OP works and all tests pass.
On the other hand, wrapping expressions into another pair of parentheses doesn't hurt either.
Perhaps someone else, more familiar with the internals, could shed some light on this.