Breaking: Check assignment targets in no-extra-parens

[mdjermanovic]

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

[X] Changes an existing rule

What rule do you want to change?

no-extra-parens

Does this change cause the rule to produce more or fewer warnings?

more

How will the change be implemented? (New option, new default behavior, etc.)?

new default behavior

Please provide some example code that this change will affect:

// left-hand side of assignments
(a) = b;
(a) += b;
(a.b) = c;

// left-hand side of default assignments
[(a) = b] = [];
[(a.b) = c] = [];
({ a: (b) = c } = {});
({ a: (b.c) = d } = {});

// array pattern elements and rest element
[(a)] = [];
[(a.b)] = [];
[...(a)] = [];
[...(a.b)] = [];

// object pattern property values and rest property
({ a: (b) } = {});
({ a: (b.c) } = {});
({ ...(a) } = {});   // these currently aren't allowed in acorn, there is an open issue
({ ...(a.b) } = {});

What does the rule currently do for this code?

No errors.

What will the rule do after it's changed?

All lines will be errors.

What changes did you make? (Give an overview)

Check missed nodes.

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

Core rules usually don't assume that the syntax can be invalid, and a function like canBeAssignmentTarget shouldn't be necessary. But, default assignments (in particular, parenthesised assignments that look like default assignments) seem to a be a common problem for parsers.

For instance, the following simple examples are actually invalid syntax, but interpreted as a valid code by acorn, espree, babel, typescript-eslint and the majority of parsers that can be found on the ast-explorer site:

(a = b) = c; // invalid syntax

[(a = b) = c] = []; // invalid syntax

Removing these parens would change semantics, and that's why all checks added in this PR use the canBeAssignmentTarget function. This is unusual, but seems to be a safe solution at this moment. Is it okay?

Also, please verify that the examples in the test cases are valid syntax.

[kaicataldo]

I think this change makes sense. Do we want to add this in v7.0.0 since it will increase warnings?

[mdjermanovic]

Rebased

[kaicataldo]

Isn't this syntax supported by Espree already? Or am I misunderstanding what this means?

const { ...foo } = bar;

[mdjermanovic]

Isn't this syntax supported by Espree already? Or am I misunderstanding what this means?

const { ...foo } = bar;

That works, but Espree doesn't allow parens around the target in rest properties. The missing test cases should be something like this:

({ ...(a) } = bar);
({ ...(a.b) } = bar);

These parens seem to be a valid syntax in assignments (not in declarations), but that isn't supported by Espree.

So, the part of this PR that removes those parens is untestable with the default parser. That's the new RestElement(node) handler. It should work (most likely well but untested) with babel-eslint and @typescript-eslint/parser which both allow parens there.

The same code in RestElement(node) also removes parens around rest elements in array patterns:

[...(a)] = []

That works in Espree and has test cases.

Maybe we should add some code to restrict RestElement(node) for now to work with rest elements in array patterns only?

[kaicataldo]

Understood, thanks! As long as the rule doesn't crash, I think it makes sense to consider this an upstream bug in Espree/Acorn and fix it there.

[mdjermanovic]

Just tested with @typescript-eslint/parser and it seems to be working fine. The rule removes extra parens around a and a.b:

/* eslint no-extra-parens: ["error"]*/

({ ...(a) } = {});
({ ...(a.b) } = {});

It turned out that nothing happens with babel-eslint. It's still ExperimentalRestProperty there. The rule doesn't report extra parens but also doesn't crash. These nodes are just ignored.

Also added a commit with TODO tests to make sure we don't forget what's this about.

It seems that this issue has been already reported at acornjs/acorn#872

[mdjermanovic]

I think this change makes sense. Do we want to add this in v7.0.0 since it will increase warnings?

Some changes of a similar kind like #11952, #11973, #11909 were semver-minor.

On the other hand #12302, which looked like a minor change at the moment, based on feedback ended up with a subsequent option in #12436.

So it indeed might be safe to add this within v.7.0.0 releases. Perhaps we should label this as Breaking?

[kaicataldo]

So it indeed might be safe to add this within v.7.0.0 releases. Perhaps we should label this as Breaking?

I think this makes sense. Might as well take advantage of doing a major release soon :)

[mdjermanovic]

Changed the title (there are now multiple commits) to Breaking:

That's how I understood, feel free to change it back on deploy if I was wrong :-)

[ljharb]

Instead of making it breaking, could it be an option whose default is flipped in the next major?

[mdjermanovic]

Maybe breaking with the option to restore the previous behavior. All other options in this rule are true (= doesn't prevent warnings) by default.

If there is a real need for someone to allow parens just in these particular cases?

[ljharb]

No idea, but it feels like it’s always better to ship everything as minor behind an option so that majors are primarily just flipping defaults - it gives a way to test the new stuff without a major, and an easy way to avoid the breakage and thus allow those impacted to trivially upgrade to the new major.

[btmills]

LGTM. Like @kaicataldo, I'm in favor of merging this as-is without an option since we're in a major release cycle. If something comes up where we'd need to allow this, we can add an option retroactively.

[ljharb]

Yes but you won’t backport it retroactively, and the whole point is to maximize how far back the change is supported.