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

Update: report double extra parens in no-extra-parens (fixes #12127) #12697

Merged
merged 11 commits into from Jan 16, 2020
Merged

Update: report double extra parens in no-extra-parens (fixes #12127) #12697

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7f70bd0
Fix: report double extra parens in no-extra-parens (fixes #12127)
yeonjuan Dec 21, 2019
a424855
rename function, add comments
yeonjuan Dec 24, 2019
000952d
review feedback
yeonjuan Dec 26, 2019
582aa86
add more test cases
yeonjuan Dec 27, 2019
26aa479
apply cond-assign-exception to test node only
yeonjuan Dec 27, 2019
ff04296
add more checking for double parens
yeonjuan Dec 28, 2019
e964722
move unaryExpression and exponentiation check
yeonjuan Jan 5, 2020
88dfc2b
change comments
yeonjuan Jan 5, 2020
f7abbf0
rebase checkClass, add test case for regression
yeonjuan Jan 7, 2020
8e3bc05
add invalid test case
yeonjuan Jan 7, 2020
7b00475
add double parens check for for-of, sequence expression
yeonjuan Jan 12, 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
79 changes: 52 additions & 27 deletions lib/rules/no-extra-parens.js
View file
Open in desktop
Expand Up @@ -169,6 +169,28 @@ module.exports = {
return ruleApplies(node) && isParenthesisedTwice(node);
}

/**
* Determines if a node that is expected to be parenthesised is surrounded by
yeonjuan marked this conversation as resolved.
Show resolved Hide resolved
* (potentially) invalid extra parentheses with considering precedence level of the node.
* If the preference level of the node is not higher or equal to precedence lower limit, it also checks
* whether the node is surrounded by parentheses twice or not.
* @param {ASTNode} node The node to be checked.
* @param {number} precedenceLowerLimit The lower limit of precedence.
* @returns {boolean} True if the node is has an unexpected extra pair of parentheses.
* @private
*/
function hasExcessParensWithPrecedence(node, precedenceLowerLimit) {
if (ruleApplies(node) && isParenthesised(node)) {
if (
precedence(node) >= precedenceLowerLimit ||
isParenthesisedTwice(node)
) {
return true;
}
}
return false;
}

/**
* Determines if a node test expression is allowed to have a parenthesised assignment
* @param {ASTNode} node The node to be checked.
Expand Down Expand Up @@ -411,7 +433,7 @@ module.exports = {
function checkCallNew(node) {
const callee = node.callee;

if (hasExcessParens(callee) && precedence(callee) >= precedence(node)) {
if (hasExcessParensWithPrecedence(callee, precedence(node))) {
const hasNewParensException = callee.type === "NewExpression" && !isNewExpressionWithParens(callee);

if (
Expand All @@ -429,7 +451,7 @@ module.exports = {
}
}
node.arguments
.filter(arg => hasExcessParens(arg) && precedence(arg) >= PRECEDENCE_OF_ASSIGNMENT_EXPR)
.filter(arg => hasExcessParensWithPrecedence(arg, PRECEDENCE_OF_ASSIGNMENT_EXPR))
.forEach(report);
}

Expand All @@ -448,11 +470,22 @@ module.exports = {
node.left.type === "UnaryExpression" && isExponentiation;
const shouldSkipRight = NESTED_BINARY && (node.right.type === "BinaryExpression" || node.right.type === "LogicalExpression");

if (!shouldSkipLeft && hasExcessParens(node.left) && (leftPrecedence > prec || (leftPrecedence === prec && !isExponentiation))) {
report(node.left);
if (!shouldSkipLeft && hasExcessParens(node.left)) {
if (
(leftPrecedence > prec || (leftPrecedence === prec && !isExponentiation)) ||
isParenthesisedTwice(node.left)
) {
report(node.left);
}
}
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
if (!shouldSkipRight && hasExcessParens(node.right) && (rightPrecedence > prec || (rightPrecedence === prec && isExponentiation))) {
report(node.right);

if (!shouldSkipRight && hasExcessParens(node.right)) {
if (
(rightPrecedence > prec || (rightPrecedence === prec && isExponentiation)) ||
isParenthesisedTwice(node.right)
) {
report(node.right);
}
}
}

Expand Down Expand Up @@ -651,7 +684,7 @@ module.exports = {
return {
ArrayExpression(node) {
node.elements
.filter(e => e && hasExcessParens(e) && precedence(e) >= PRECEDENCE_OF_ASSIGNMENT_EXPR)
.filter(e => e && hasExcessParensWithPrecedence(e, PRECEDENCE_OF_ASSIGNMENT_EXPR))
.forEach(report);
},

Expand All @@ -674,18 +707,14 @@ module.exports = {
if (astUtils.isOpeningParenToken(tokenBeforeFirst) && astUtils.isOpeningBraceToken(firstBodyToken)) {
tokensToIgnore.add(firstBodyToken);
}
if (hasExcessParens(node.body) && precedence(node.body) >= PRECEDENCE_OF_ASSIGNMENT_EXPR) {
if (hasExcessParensWithPrecedence(node.body, PRECEDENCE_OF_ASSIGNMENT_EXPR)) {
report(node.body);
}
}
},

AssignmentExpression(node) {
if (isReturnAssignException(node)) {
return;
}

if (hasExcessParens(node.right) && precedence(node.right) >= precedence(node)) {
if (!isReturnAssignException(node) && hasExcessParensWithPrecedence(node.right, precedence(node))) {
report(node.right);
}
},
Expand All @@ -702,25 +731,24 @@ module.exports = {

ClassBody(node) {
node.body
.filter(member => member.type === "MethodDefinition" && member.computed &&
member.key && hasExcessParens(member.key) && precedence(member.key) >= PRECEDENCE_OF_ASSIGNMENT_EXPR)
.filter(member => member.type === "MethodDefinition" && member.computed && member.key)
.filter(member => hasExcessParensWithPrecedence(member.key, PRECEDENCE_OF_ASSIGNMENT_EXPR))
.forEach(member => report(member.key));
},

ConditionalExpression(node) {
if (isReturnAssignException(node)) {
if (isReturnAssignException(node) || isCondAssignException(node)) {
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
return;
}

if (hasExcessParens(node.test) && precedence(node.test) >= precedence({ type: "LogicalExpression", operator: "||" })) {
if (hasExcessParensWithPrecedence(node.test, precedence({ type: "LogicalExpression", operator: "||" }))) {
report(node.test);
}

if (hasExcessParens(node.consequent) && precedence(node.consequent) >= PRECEDENCE_OF_ASSIGNMENT_EXPR) {
if (hasExcessParensWithPrecedence(node.consequent, PRECEDENCE_OF_ASSIGNMENT_EXPR)) {
report(node.consequent);
}

if (hasExcessParens(node.alternate) && precedence(node.alternate) >= PRECEDENCE_OF_ASSIGNMENT_EXPR) {
if (hasExcessParensWithPrecedence(node.alternate, PRECEDENCE_OF_ASSIGNMENT_EXPR)) {
report(node.alternate);
}
},
Expand Down Expand Up @@ -911,18 +939,15 @@ module.exports = {

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

return value && hasExcessParens(value) && precedence(value) >= PRECEDENCE_OF_ASSIGNMENT_EXPR;
}).forEach(property => report(property.value));
.filter(property => property.value && hasExcessParensWithPrecedence(property.value, PRECEDENCE_OF_ASSIGNMENT_EXPR))
.forEach(property => report(property.value));
},

Property(node) {
if (node.computed) {
const { key } = node;

if (key && hasExcessParens(key) && precedence(key) >= PRECEDENCE_OF_ASSIGNMENT_EXPR) {
if (key && hasExcessParensWithPrecedence(key, PRECEDENCE_OF_ASSIGNMENT_EXPR)) {
report(key);
}
}
Expand Down Expand Up @@ -1024,7 +1049,7 @@ module.exports = {
AssignmentPattern(node) {
const { right } = node;

if (right && hasExcessParens(right) && precedence(right) >= PRECEDENCE_OF_ASSIGNMENT_EXPR) {
if (right && hasExcessParensWithPrecedence(right, PRECEDENCE_OF_ASSIGNMENT_EXPR)) {
report(right);
}
}
Expand Down
108 changes: 106 additions & 2 deletions tests/lib/rules/no-extra-parens.js
View file
Open in desktop
Expand Up @@ -250,17 +250,22 @@ ruleTester.run("no-extra-parens", rule, {
{ code: "var a = (b = c)", options: ["functions"] },
{ code: "_ => (a = 0)", options: ["functions"] },

// ["all", {conditionalAssign: false}] enables extra parens around conditional assignments
// ["all", { conditionalAssign: false }] enables extra parens around conditional assignments
{ code: "while ((foo = bar())) {}", options: ["all", { conditionalAssign: false }] },
{ code: "if ((foo = bar())) {}", options: ["all", { conditionalAssign: false }] },
{ code: "do; while ((foo = bar()))", options: ["all", { conditionalAssign: false }] },
{ code: "for (;(a = b););", options: ["all", { conditionalAssign: false }] },
{ code: "var a = ((b = c)) ? foo : bar;", options: ["all", { conditionalAssign: false }] },

// ["all", { nestedBinaryExpressions: false }] enables extra parens around conditional assignments
{ code: "a + (b * c)", options: ["all", { nestedBinaryExpressions: false }] },
{ code: "(a * b) + c", options: ["all", { nestedBinaryExpressions: false }] },
{ code: "(a * b) / c", options: ["all", { nestedBinaryExpressions: false }] },
{ code: "a || (b && c)", options: ["all", { nestedBinaryExpressions: false }] },
{ code: "a + ((b * c))", options: ["all", { nestedBinaryExpressions: false }] },
{ code: "((a * b)) + c", options: ["all", { nestedBinaryExpressions: false }] },
{ code: "((a * b)) / c", options: ["all", { nestedBinaryExpressions: false }] },
{ code: "a || ((b && c))", options: ["all", { nestedBinaryExpressions: false }] },

// ["all", { returnAssign: false }] enables extra parens around expressions returned by return statements
{ code: "function a(b) { return b || c; }", options: ["all", { returnAssign: false }] },
Expand All @@ -276,6 +281,8 @@ ruleTester.run("no-extra-parens", rule, {
{ code: "b => { return (b = 1) };", options: ["all", { returnAssign: false }] },
{ code: "b => { return (b = c) || (b = d) };", options: ["all", { returnAssign: false }] },
{ code: "b => { return c ? (d = b) : (e = b) };", options: ["all", { returnAssign: false }] },
{ code: "function a(b) { return ((b = 1)); }", options: ["all", { returnAssign: false }] },
{ code: "b => ((b = 1));", options: ["all", { returnAssign: false }] },

// https://github.com/eslint/eslint/issues/3653
"(function(){}).foo(), 1, 2;",
Expand Down Expand Up @@ -352,6 +359,8 @@ ruleTester.run("no-extra-parens", rule, {
{ code: "foo in (bar in baz)", options: ["all", { nestedBinaryExpressions: false }] },
{ code: "foo + (bar + baz)", options: ["all", { nestedBinaryExpressions: false }] },
{ code: "foo && (bar && baz)", options: ["all", { nestedBinaryExpressions: false }] },
{ code: "((foo instanceof bar)) instanceof baz", options: ["all", { nestedBinaryExpressions: false }] },
{ code: "((foo in bar)) in baz", options: ["all", { nestedBinaryExpressions: false }] },

// https://github.com/eslint/eslint/issues/9019
"(async function() {});",
Expand Down Expand Up @@ -452,7 +461,9 @@ ruleTester.run("no-extra-parens", rule, {

// ["all", { enforceForSequenceExpressions: false }]
{ code: "(a, b)", options: ["all", { enforceForSequenceExpressions: false }] },
{ code: "((a, b))", options: ["all", { enforceForSequenceExpressions: false }] },
{ code: "(foo(), bar());", options: ["all", { enforceForSequenceExpressions: false }] },
{ code: "((foo(), bar()));", options: ["all", { enforceForSequenceExpressions: false }] },
{ code: "if((a, b)){}", options: ["all", { enforceForSequenceExpressions: false }] },
{ code: "while ((val = foo(), val < 10));", options: ["all", { enforceForSequenceExpressions: false }] },

Expand All @@ -463,6 +474,7 @@ ruleTester.run("no-extra-parens", rule, {
{ code: "(new foo(bar)).baz", options: ["all", { enforceForNewInMemberExpressions: false }] },
{ code: "(new foo.bar()).baz", options: ["all", { enforceForNewInMemberExpressions: false }] },
{ code: "(new foo.bar()).baz()", options: ["all", { enforceForNewInMemberExpressions: false }] },
{ code: "((new foo.bar())).baz()", options: ["all", { enforceForNewInMemberExpressions: false }] },

"let a = [ ...b ]",
"let a = { ...b }",
Expand Down Expand Up @@ -2035,6 +2047,98 @@ ruleTester.run("no-extra-parens", rule, {
"SequenceExpression",
1,
{ parserOptions: { ecmaVersion: 2020 } }
)
),

// https://github.com/eslint/eslint/issues/12127
{
code: "[1, ((2, 3))];",
output: "[1, (2, 3)];",
errors: [{ messageId: "unexpected" }]
},
{
code: "const foo = () => ((bar, baz));",
output: "const foo = () => (bar, baz);",
errors: [{ messageId: "unexpected" }]
},
{
code: "foo = ((bar, baz));",
output: "foo = (bar, baz);",
errors: [{ messageId: "unexpected" }]
},
{
code: "foo + ((bar + baz));",
output: "foo + (bar + baz);",
errors: [{ messageId: "unexpected" }]
},
{
code: "((foo + bar)) + baz;",
output: "(foo + bar) + baz;",
errors: [{ messageId: "unexpected" }]
},
{
code: "foo * ((bar + baz));",
output: "foo * (bar + baz);",
errors: [{ messageId: "unexpected" }]
},
{
code: "((foo + bar)) * baz;",
output: "(foo + bar) * baz;",
errors: [{ messageId: "unexpected" }]
},
{
code: "new A(((foo, bar)))",
output: "new A((foo, bar))",
errors: [{ messageId: "unexpected" }]
},
{
code: "class A{ [((foo, bar))]() {} }",
output: "class A{ [(foo, bar)]() {} }",
errors: [{ messageId: "unexpected" }]
},
{
code: "new ((A, B))()",
output: "new (A, B)()",
errors: [{ messageId: "unexpected" }]
},
{
code: "((foo, bar)) ? bar : baz;",
output: "(foo, bar) ? bar : baz;",
errors: [{ messageId: "unexpected" }]
},
{
code: "((f ? o : o)) ? bar : baz;",
output: "(f ? o : o) ? bar : baz;",
errors: [{ messageId: "unexpected" }]
},
{
code: "((f = oo)) ? bar : baz;",
output: "(f = oo) ? bar : baz;",
errors: [{ messageId: "unexpected" }]
},
{
code: "foo ? ((bar, baz)) : baz;",
output: "foo ? (bar, baz) : baz;",
errors: [{ messageId: "unexpected" }]
},
{
code: "foo ? bar : ((bar, baz));",
output: "foo ? bar : (bar, baz);",
errors: [{ messageId: "unexpected" }]
},
{
code: "function foo(bar = ((baz1, baz2))) {}",
output: "function foo(bar = (baz1, baz2)) {}",
errors: [{ messageId: "unexpected" }]
},
{
code: "var foo = { bar: ((baz1, baz2)) };",
output: "var foo = { bar: (baz1, baz2) };",
errors: [{ messageId: "unexpected" }]
},
{
code: "var foo = { [((bar1, bar2))]: baz };",
output: "var foo = { [(bar1, bar2)]: baz };",
errors: [{ messageId: "unexpected" }]
}
]
});