Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Breaking: Check assignment targets in no-extra-parens #12490

Merged
merged 2 commits into from Jan 17, 2020
Merged

Breaking: Check assignment targets in no-extra-parens #12490

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6939bfa
Update: Check assignment targets in no-extra-parens
mdjermanovic Oct 24, 2019
4258017
Add TODO tests
mdjermanovic Jan 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 43 additions & 1 deletion lib/rules/no-extra-parens.js
View file
Open in desktop
Expand Up @@ -342,6 +342,17 @@ module.exports = {
return node.type === "CallExpression" && node.callee.type === "FunctionExpression";
}

/**
* Determines if the given node can be the assignment target in destructuring or the LHS of an assignment.
* This is to avoid an autofix that could change behavior because parsers mistakenly allow invalid syntax,
* such as `(a = b) = c` and `[(a = b) = c] = []`. Ideally, this function shouldn't be necessary.
* @param {ASTNode} [node] The node to check
* @returns {boolean} `true` if the given node can be a valid assignment target
*/
function canBeAssignmentTarget(node) {
return node && (node.type === "Identifier" || node.type === "MemberExpression");
}

/**
* Report the node
* @param {ASTNode} node node to evaluate
Expand Down Expand Up @@ -679,6 +690,12 @@ module.exports = {
.forEach(report);
},

ArrayPattern(node) {
node.elements
.filter(e => canBeAssignmentTarget(e) && hasExcessParens(e))
.forEach(report);
},

ArrowFunctionExpression(node) {
if (isReturnAssignException(node)) {
return;
Expand All @@ -705,6 +722,10 @@ module.exports = {
},

AssignmentExpression(node) {
if (canBeAssignmentTarget(node.left) && hasExcessParens(node.left)) {
report(node.left);
}

if (!isReturnAssignException(node) && hasExcessParensWithPrecedence(node.right, precedence(node))) {
report(node.right);
}
Expand Down Expand Up @@ -947,6 +968,15 @@ module.exports = {
.forEach(property => report(property.value));
},

ObjectPattern(node) {
node.properties
.filter(property => {
const value = property.value;

return canBeAssignmentTarget(value) && hasExcessParens(value);
}).forEach(property => report(property.value));
},

Property(node) {
if (node.computed) {
const { key } = node;
Expand All @@ -957,6 +987,14 @@ module.exports = {
}
},

RestElement(node) {
const argument = node.argument;

if (canBeAssignmentTarget(argument) && hasExcessParens(argument)) {
report(argument);
}
},

ReturnStatement(node) {
const returnToken = sourceCode.getFirstToken(node);

Expand Down Expand Up @@ -1054,7 +1092,11 @@ module.exports = {
},

AssignmentPattern(node) {
const { right } = node;
const { left, right } = node;

if (canBeAssignmentTarget(left) && hasExcessParens(left)) {
report(left);
}

if (right && hasExcessParensWithPrecedence(right, PRECEDENCE_OF_ASSIGNMENT_EXPR)) {
report(right);
Expand Down
19 changes: 19 additions & 0 deletions tests/lib/rules/no-extra-parens.js
View file
Open in desktop
Expand Up @@ -1403,6 +1403,25 @@ ruleTester.run("no-extra-parens", rule, {
invalid("const [a = (b)] = []", "const [a = b] = []", "Identifier"),
invalid("const {a = (b)} = {}", "const {a = b} = {}", "Identifier"),

// LHS of assigments/Assignment targets
invalid("(a) = b", "a = b", "Identifier"),
invalid("(a.b) = c", "a.b = c", "MemberExpression"),
invalid("(a) += b", "a += b", "Identifier"),
invalid("(a.b) >>= c", "a.b >>= c", "MemberExpression"),
invalid("[(a) = b] = []", "[a = b] = []", "Identifier"),
invalid("[(a.b) = c] = []", "[a.b = c] = []", "MemberExpression"),
invalid("({ a: (b) = c } = {})", "({ a: b = c } = {})", "Identifier"),
invalid("({ a: (b.c) = d } = {})", "({ a: b.c = d } = {})", "MemberExpression"),
invalid("[(a)] = []", "[a] = []", "Identifier"),
invalid("[(a.b)] = []", "[a.b] = []", "MemberExpression"),
invalid("[,(a),,] = []", "[,a,,] = []", "Identifier"),
invalid("[...(a)] = []", "[...a] = []", "Identifier"),
invalid("[...(a.b)] = []", "[...a.b] = []", "MemberExpression"),
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
Copy link
Member

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;

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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


// https://github.com/eslint/eslint/issues/11706 (also in valid[])
{
code: "for ((a = (b in c)); ;);",
Expand Down