Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Update: report double extra parens in no-extra-parens (fixes #12127) (#…
…12697)

* Fix: report double extra parens in no-extra-parens (fixes #12127)

* rename function, add comments

* review feedback

* add more test cases

* apply cond-assign-exception to test node only

* add more checking for double parens

* move unaryExpression and exponentiation check

* change comments

* rebase checkClass, add test case for regression

* add invalid test case

* add double parens check for for-of, sequence expression
  • Loading branch information
yeonjuan authored and kaicataldo committed Jan 16, 2020
1 parent 02fcc05 commit ae959b6
Show file tree
Hide file tree
Showing 2 changed files with 211 additions and 51 deletions.
131 changes: 82 additions & 49 deletions lib/rules/no-extra-parens.js
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
* (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 @@ -370,17 +392,13 @@ module.exports = {
}

/**
* Evaluate Unary update
* Evaluate a argument of the node.
* @param {ASTNode} node node to evaluate
* @returns {void}
* @private
*/
function checkUnaryUpdate(node) {
if (node.type === "UnaryExpression" && node.argument.type === "BinaryExpression" && node.argument.operator === "**") {
return;
}

if (hasExcessParens(node.argument) && precedence(node.argument) >= precedence(node)) {
function checkArgumentWithPrecedence(node) {
if (hasExcessParensWithPrecedence(node.argument, precedence(node))) {
report(node.argument);
}
}
Expand Down Expand Up @@ -411,7 +429,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 @@ -428,7 +446,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 @@ -443,15 +461,26 @@ module.exports = {
const leftPrecedence = precedence(node.left);
const rightPrecedence = precedence(node.right);
const isExponentiation = node.operator === "**";
const shouldSkipLeft = (NESTED_BINARY && (node.left.type === "BinaryExpression" || node.left.type === "LogicalExpression")) ||
node.left.type === "UnaryExpression" && isExponentiation;
const shouldSkipLeft = NESTED_BINARY && (node.left.type === "BinaryExpression" || node.left.type === "LogicalExpression");
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 (
!(node.left.type === "UnaryExpression" && isExponentiation) &&
(leftPrecedence > prec || (leftPrecedence === prec && !isExponentiation)) ||
isParenthesisedTwice(node.left)
) {
report(node.left);
}
}
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 @@ -484,11 +513,7 @@ module.exports = {
* @returns {void}
*/
function checkSpreadOperator(node) {
const hasExtraParens = precedence(node.argument) >= PRECEDENCE_OF_ASSIGNMENT_EXPR
? hasExcessParens(node.argument)
: hasDoubleExcessParens(node.argument);

if (hasExtraParens) {
if (hasExcessParensWithPrecedence(node.argument, PRECEDENCE_OF_ASSIGNMENT_EXPR)) {
report(node.argument);
}
}
Expand Down Expand Up @@ -650,7 +675,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 @@ -673,18 +698,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 @@ -701,25 +722,27 @@ 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)) {
return;
}

if (hasExcessParens(node.test) && precedence(node.test) >= precedence({ type: "LogicalExpression", operator: "||" })) {
if (
!isCondAssignException(node) &&
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 @@ -756,9 +779,19 @@ module.exports = {
tokensToIgnore.add(firstLeftToken);
}
}
if (!(node.type === "ForOfStatement" && node.right.type === "SequenceExpression") && hasExcessParens(node.right)) {

if (node.type === "ForOfStatement") {
const hasExtraParens = node.right.type === "SequenceExpression"
? hasDoubleExcessParens(node.right)
: hasExcessParens(node.right);

if (hasExtraParens) {
report(node.right);
}
} else if (hasExcessParens(node.right)) {
report(node.right);
}

if (hasExcessParens(node.left)) {
report(node.left);
}
Expand Down Expand Up @@ -910,18 +943,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 All @@ -944,8 +974,10 @@ module.exports = {
},

SequenceExpression(node) {
const precedenceOfNode = precedence(node);

node.expressions
.filter(e => hasExcessParens(e) && precedence(e) >= precedence(node))
.filter(e => hasExcessParensWithPrecedence(e, precedenceOfNode))
.forEach(report);
},

Expand All @@ -969,16 +1001,17 @@ module.exports = {
}
},

UnaryExpression: checkUnaryUpdate,
UpdateExpression: checkUnaryUpdate,
AwaitExpression: checkUnaryUpdate,
UnaryExpression: checkArgumentWithPrecedence,
UpdateExpression: checkArgumentWithPrecedence,
AwaitExpression: checkArgumentWithPrecedence,

VariableDeclarator(node) {
if (node.init && hasExcessParens(node.init) &&
precedence(node.init) >= PRECEDENCE_OF_ASSIGNMENT_EXPR &&
if (
node.init && hasExcessParensWithPrecedence(node.init, PRECEDENCE_OF_ASSIGNMENT_EXPR) &&

// RegExp literal is allowed to have parens (#1589)
!(node.init.type === "Literal" && node.init.regex)) {
// RegExp literal is allowed to have parens (#1589)
!(node.init.type === "Literal" && node.init.regex)
) {
report(node.init);
}
},
Expand Down Expand Up @@ -1023,7 +1056,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

0 comments on commit ae959b6

Please sign in to comment.