New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Breaking: Check assignment targets in no-extra-parens #12490
Conversation
I think this change makes sense. Do we want to add this in v7.0.0 since it will increase warnings? |
f0a8739
to
6939bfa
Compare
Rebased |
tests/lib/rules/no-extra-parens.js
Outdated
invalid("({ a: (b) } = {})", "({ a: b } = {})", "Identifier"), | ||
invalid("({ a: (b.c) } = {})", "({ a: b.c } = {})", "MemberExpression"), | ||
|
||
// TODO: add tests for the RestElement in object patterns when it becomes supported in espree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this syntax supported by Espree already? Or am I misunderstanding what this means?
const { ...foo } = bar;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 |
I think this makes sense. Might as well take advantage of doing a major release soon :) |
Changed the title (there are now multiple commits) to That's how I understood, feel free to change it back on deploy if I was wrong :-) |
Instead of making it breaking, could it be an option whose default is flipped in the next major? |
Maybe breaking with the option to restore the previous behavior. All other options in this rule are If there is a real need for someone to allow parens just in these particular cases? |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Yes but you won’t backport it retroactively, and the whole point is to maximize how far back the change is supported. |
* Update: Check assignment targets in no-extra-parens * Add TODO tests
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:
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:
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.