add handling of lone commas when destructuring arrays

[justincorrigible]

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Details about the bug and my environment are as seen in the issue

Closes #12756. This handles ignored elements as per MDN

Tell us about your environment

• ESLint Version: 6.8.0
• Node Version: 13.6.0
• NPM Version: 6.13.4
• Parser @typescript-eslint

What changes did you make? (Give an overview)

I've split the checks for ArrayPatterns from ArrayLiterals, as LHS allows ignoring elements, whereas RHS would be invalid code. This results in null nodes using AST, which the existing validation code could not handle coherently. Thus, if there are any commas without element, it doesn't report them. Honestly, I'm not sure my approach is the most adequate, yet it seems to get the job done based on my tests.

I've noticed with this change you can have as many commas in a single line as you want.
I am not sure what space between consecutive commas should be required, if any.
That is to be discussed in a separate issue.

For the time being, my next step will be modifying "array-element-newline" to respond accordingly (pending issue to discuss implementation with the community, will add a link here for reference.)

Is there anything you'd like reviewers to focus on?
I didn't go the whole ten yards checking for other overlapping rules.

[mdjermanovic]

Does the same lone commas problem exist with array expressions as well?

If so, maybe we shouldn't separate logic for ArrayPattern/ArrayExpression, but rather fix both.

[justincorrigible]

Does the same lone commas problem exist with array expressions as well?

The issue is there for those, yes. However, I cannot think of any good use case for anyone to willingly create an array with empty positions.
Not "fixing" the issue for those may help people realise they have those, fulfilling the job.

const foo = ['foo',,'bar']; // shouldn't be allowed

For destructuring assigments it should always be allowed (hence this fix)... but maybe users would want or not to enforce the same thing for array expressions. Perhaps that should be handled as a new option, and contributed in a different PR, rather than assume people will want it off-the-bat (breaking the current, expected functioning of ESLint.)

[mdjermanovic]

The issue is there for those, yes. However, I cannot think of any good use case for anyone to willingly create an array with empty positions.
Not "fixing" the issue for those may help people realise they have those, fulfilling the job.

I agree there is probably no valid use case for empty positions in arrays, but there's a separate rule to disallow them (no-sparse-arrays).

For destructuring assigments it should always be allowed (hence this fix)... but maybe users would want or not to enforce the same thing for array expressions. Perhaps that should be handled as a new option, and contributed in a different PR, rather than assume people will want it off-the-bat (breaking the current, expected functioning of ESLint.)

If this is a bug in the default behavior for ArrayPattern nodes, then it probably makes sense to treat the same thing as a bug in the default behavior for ArrayExpression nodes.

It's good in general to fix separate bugs in separate PRs, but in this particular case it looks like we are adding some extra code just to preserve one bug while fixing another?

[justincorrigible]

there's a separate rule to disallow them (no-sparse-arrays)

That's a good point, as long as there's something to ensure discouraging the use of empty positions RHS, we're good. I'm just not well acquainted with the whole array of rules (bad pun.)

Amending my PR. Stand by. That rule should also be amended to ensure it allows destructuring assignments, perhaps?

[justincorrigible]

Place holder in case we may want to report other things later, based on new options.

[justincorrigible]

@mdjermanovic It seems allowing those RHS empty positions makes a few of the tests fail.

Since this PR is meant to relax the "comma-style" rule, as you suggest, correctly delegating the handling of those empty spaces to "no-sparse-arrays"; I'll remove the tests here, and make a mental note to check/adjust there instead.

[mdjermanovic]

To clarify, comma-style shouldn't delegate handling of empty spaces to no-sparse-arrays. These two rules have a different purpose.

comma-style is about formatting. It should work well with sparse arrays.

Failing tests means that we changed something in the designed behavior of this rule. We should analyze what's failing, is that correct, and then modify those tests and/or add new tests to show the new behavior.

This isn't only about array expressions, the same change would affect similar tests with array patterns. It's just a coincidence that only array expressions' tests had empty elements.

[mdjermanovic]

This particular test doesn't have empty elements. If it's failing, then the change might be incorrect since it modifies behavior for arrays/patterns that don't have empty elements.

A simplified example, this time with an array pattern that doesn't have empty elements:

/*eslint comma-style: ["error", "last", { 
    "exceptions": { "ArrayPattern": false }
}]*/

var [
  foo
  ,
  bar] = [];

This is invalid code by this rule in the actual version - Online Demo

After this change as it's implemented at the moment, this would be a valid code (I think, it would be nice to add a test case). Is this the desired new behavior, i.e., should we ignore any commas on separate lines?

[justincorrigible]

Is this the desired new behavior, i.e., should we ignore any commas on separate lines?

If "comma-style" is enabled, that comma in a new line belongs either with foo (as default 'last'), or bar (if set to 'first'). Meaning your example should indeed fail.
My attempted fix is therefor incomplete.
Will revise and amend it... thanks for pointing it out. 👍

[justincorrigible]

FYI: Still working on this. Just slammed at the 9-5 for the moment.

[kaicataldo]

Thanks for the update :)

[nzakas]

Unfortunately, it looks like this pull request has been abandoned, so closing.

Note: The issue that this PR addresses is accepted (#12756) so anyone interested may submit another PR to implement the change.