Update: check identifier in array pattern in id-length (fixes #12832)

[yeonjuan]

Prerequisites checklist

• I have read the contributing guidelines.
• The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

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:

What changes did you make? (Give an overview)

Change the id-length rule to check an identifier in the array pattern.

const [a] = arr;

Is there anything you'd like reviewers to focus on?

This PR will fix #12832.
It will generate more errors so I labeled this pr as Update.

[kaicataldo]

LGTM, thanks!

[mdjermanovic]

By the code it seems that this rule aims to report only variable creations (declarations, params etc.), not all references, maybe we should do that for array patterns as well?

[a] = foo; // shouldn't be error?

Though, RestElement and AssignmentPattern would be already false positives in a similar way (and there are other false positives/false negatives).

[yeonjuan]

@mdjermanovic
Thanks for the review :)

By the code it seems that this rule aims to report only variable creations

Could you please explain it more?

Online Demo

/*eslint id-length: "error"*/

var a = 4; // Error
a = 5; // No error

// Object Pattern
var {c} = foo; // Error
({c} = foo); // Error

// AssignmentPattern
var [d = b] = foo; // Error
[d = b] = foo; // Error

// RestElement
var [...d] = foo; // Error
[...d] = foo; // Error

// No Error in v6.8.0 but both Errors in the PR version.
var [a] = foo;
[a] = foo; 

In my understanding. this rule seems to report the syntax which can make a new identifier if there isn't. (maybe LHS lookup?)

// without declaration
[a] = [3];   // it creates an identifier 'a`
({b} = { b: 3 });   // it creates an identifier 'b`

From this point of view, the below cases also need to be warned.

a = 5;  // No error in the current version.
[a] = 5;
({a} = {});

I would like to hear your opinion :)

[kaicataldo]

Given that other ExpressionStatement nodes generate errors, I think it makes sense for this option to be consistent with the current behavior. Could we land this now and create a follow-up issue to discuss this futher?

[mdjermanovic]

A problem is that the current behavior is already inconsistent.

I'm not sure if someone may complain about why [a] = foo; becomes an error now, since a = foo[0]; isn't. Also, a simple a = foo; isn't an error.

By looking at the actual and the initial version of the code, I think that a = foo; is intentionally no error, while the fact that [a = foo] = bar;, [...a] = foo;, and ({ a } = foo); are all errors could be an oversight. The last one is same as a = foo.a;, which isn't an error. Unfortunately, there are no tests for either of these.

[mdjermanovic]

In the docs, Examples of correct code for this rule with a minimum of 4: var { prop: [x] } = {}; should be also invalid now?

Also, Examples of correct code for this rule with the { "min": 4 } option: var { prop: [x] } = {};

Hm, these two sections look pretty much same? The first one has text "This rule has a shorthand integer option for the "min" object property". It could be for an option that wasn't implemented?

[yeonjuan]

@mdjermanovic

In the docs, Examples of correct code for this rule with a minimum of 4: var { prop: [x] } = {}; should be also invalid now?
Also, Examples of correct code for this rule with the { "min": 4 } option: var { prop: [x] } = {};

Yes, I agree. I'll update the documentation.

Hm, these two sections look pretty much same? The first one has text "This rule has a shorthand integer option for the "min" object property". It could be for an option that wasn't implemented?

It seems so. I think it should be removed because it is not supported actually.

Thanks :)

[kaicataldo]

I think merging #12881 created merge conflicts here - sorry!

[yeonjuan]

@kaicataldo That's no problem 😄 I knew it

[mdjermanovic]

LGTM, thanks!

Non-blockers:

• Some examples in correct { "min": 4 } are incorrect, but that's another bug.
• I still think the intention was to not report left side of assignments unless it's an assignment to a property with invalid length.