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

feat: add enforceForInnerExpressions option to no-extra-boolean-cast #18222

Merged
merged 14 commits into from May 6, 2024
+1,457 −216
Merged

feat: add enforceForInnerExpressions option to no-extra-boolean-cast #18222

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ce5ac38
fix: [no-extra-boolean-cast] inspect comma expressions and ?? express…
kirkwaiblinger Mar 21, 2024
554caaf
check ternaries recursively too
kirkwaiblinger Mar 21, 2024
c60535b
pr feedback
kirkwaiblinger Mar 22, 2024
c5c1630
Merge branch 'main' into no-extra-boolean-cast-inspect-more-expressions
kirkwaiblinger Apr 26, 2024
8326217
change docs
kirkwaiblinger Apr 26, 2024
822ec6c
add option for recursive expression checks
kirkwaiblinger Apr 26, 2024
e24feeb
better wording
kirkwaiblinger Apr 26, 2024
c327282
format
kirkwaiblinger Apr 26, 2024
464d921
enforceForInnerExpressions
kirkwaiblinger May 1, 2024
6b5b3f3
feedback
kirkwaiblinger May 1, 2024
4af5bd5
nice, anyOf is clever about additionalProperties
kirkwaiblinger May 1, 2024
c775385
test cov
kirkwaiblinger May 2, 2024
c8a1dc3
test coverage
kirkwaiblinger May 2, 2024
ba74952
docs
kirkwaiblinger May 2, 2024
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
19 changes: 19 additions & 0 deletions lib/rules/no-extra-boolean-cast.js
View file
Open in desktop
Expand Up @@ -115,6 +115,23 @@ module.exports = {
return isInFlaggedContext(node.parent);
}

if (node.parent.type === "ConditionalExpression" && (node.parent.consequent === node || node.parent.alternate === node)) {
return isInFlaggedContext(node.parent);
}

/*
* Check last expression only in a sequence, i.e. if ((1, 2, Boolean(3))) {}, since
* the others don't affect the result of the expression.
*/
if (node.parent.type === "SequenceExpression" && node.parent.expressions.at(-1) === node) {
return isInFlaggedContext(node.parent);
}

// Check the right hand side of a `??` operator.
if (node.parent.type === "LogicalExpression" && node.parent.operator === "??" && node.parent.right === node) {
return isInFlaggedContext(node.parent);
}

return isInBooleanContext(node) ||
(isLogicalContext(node.parent) &&

Expand Down Expand Up @@ -157,6 +174,7 @@ module.exports = {
if (previousNode.parent.type === "ChainExpression") {
return needsParens(previousNode.parent, node);
}

if (isParenthesized(previousNode)) {

// parentheses around the previous node will stay, so there is no need for an additional pair
Expand All @@ -174,6 +192,7 @@ module.exports = {
case "DoWhileStatement":
case "WhileStatement":
case "ForStatement":
case "SequenceExpression":
return false;
case "ConditionalExpression":
return precedence(node) <= precedence(parent);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was assuming that previousNode is parent.test (as in the description of this function on line 167, which we should also update), but now it can be consequent/alternate, in which case only SequenceExpression would need parentheses.

For example:

/* eslint no-extra-boolean-cast: 2 */

if (a ? Boolean(b = c) : Boolean(d = e));

After the autofix:

/* eslint no-extra-boolean-cast: 2 */

if (a ? (b = c) : (d = e));

Parentheses around b = c and d = e are unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one took me a while to figure out trying to make use of the existing infrastructure.
I would have expected to be able to write

                    if (previousNode === parent.test) {
                        return precedence(node) <= precedence(parent);
                    }
                    if (previousNode === parent.consequent || previousNode === parent.alternate) {
                        return precedence(node) < precedence(parent);
                    }

, which essentially just handles the right-associativity, but it seems that eslint's precedence() function differs from MDN's table, in that it treats AssignmentExpressions and similar as lower precedence than ternary, rather than equal... See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_precedence#table, vs

eslint/lib/rules/utils/ast-utils.js

Lines 1509 to 1602 in 239a7e2

getPrecedence(node) {
switch (node.type) {
case "SequenceExpression":
return 0;
case "AssignmentExpression":
case "ArrowFunctionExpression":
case "YieldExpression":
return 1;
case "ConditionalExpression":
return 3;
case "LogicalExpression":
switch (node.operator) {
case "||":
case "??":
return 4;
case "&&":
return 5;
// no default
}
/* falls through */
case "BinaryExpression":
switch (node.operator) {
case "|":
return 6;
case "^":
return 7;
case "&":
return 8;
case "==":
case "!=":
case "===":
case "!==":
return 9;
case "<":
case "<=":
case ">":
case ">=":
case "in":
case "instanceof":
return 10;
case "<<":
case ">>":
case ">>>":
return 11;
case "+":
case "-":
return 12;
case "*":
case "/":
case "%":
return 13;
case "**":
return 15;
// no default
}
/* falls through */
case "UnaryExpression":
case "AwaitExpression":
return 16;
case "UpdateExpression":
return 17;
case "CallExpression":
case "ChainExpression":
case "ImportExpression":
return 18;
case "NewExpression":
return 19;
default:
if (node.type in eslintVisitorKeys) {
return 20;
}
/*
* if the node is not a standard node that we know about, then assume it has the lowest precedence
* this will mean that rules will wrap unknown nodes in parentheses where applicable instead of
* unwrapping them and potentially changing the meaning of the code or introducing a syntax error.
*/
return -1;
}
},

Think that there should be a followup to change to match the MDN version, or is it better to just leave the current system?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can consider changing this in a followup if the current logic is confusing and the new one would be better for practical use, but we'll need to check all rules that use getPrecedence() to see if they would need to be updated.

Expand Down
45 changes: 45 additions & 0 deletions tests/lib/rules/no-extra-boolean-cast.js
View file
Open in desktop
Expand Up @@ -34,6 +34,7 @@ ruleTester.run("no-extra-boolean-cast", rule, {
"for(Boolean(foo);;) {}",
"for(;; Boolean(foo)) {}",
"if (new Boolean(foo)) {}",
"if ((Boolean(1), 2)) {}",
{
code: "var foo = bar || !!baz",
options: [{ enforceForLogicalOperands: true }]
Expand Down Expand Up @@ -2435,6 +2436,50 @@ ruleTester.run("no-extra-boolean-cast", rule, {
ecmaVersion: 2020
},
errors: [{ messageId: "unexpectedCall" }]
},
{
code: "if ((1, 2, Boolean(3))) {}",
output: "if ((1, 2, 3)) {}",
errors: [{ messageId: "unexpectedCall" }]
},
{
code: "if (a ?? Boolean(b)) {}",
output: "if (a ?? b) {}",
options: [{ enforceForLogicalOperands: true }],
errors: [{ messageId: "unexpectedCall" }]
},
{
code: "if (a ?? Boolean(b)) {}",
output: "if (a ?? b) {}",
options: [{ enforceForLogicalOperands: false }],
errors: [{ messageId: "unexpectedCall" }]
},
{
code: "if ((a, b, c ?? (d, e, f ?? Boolean(g)))) {}",
output: "if ((a, b, c ?? (d, e, f ?? g))) {}",
options: [{ enforceForLogicalOperands: false }],
errors: [{ messageId: "unexpectedCall" }]
},
{
code: "if ((a, b, c ?? (d, e, f ?? Boolean(g)))) {}",
output: "if ((a, b, c ?? (d, e, f ?? g))) {}",
options: [{ enforceForLogicalOperands: true }],
errors: [{ messageId: "unexpectedCall" }]
},
{
code: "if (a ? Boolean(b) : c) {}",
output: "if (a ? b : c) {}",
errors: [{ messageId: "unexpectedCall" }]
},
{
code: "if (a ? b : Boolean(c)) {}",
output: "if (a ? b : c) {}",
errors: [{ messageId: "unexpectedCall" }]
},
{
code: "if (a ? b : Boolean(c ? d : e)) {}",
output: "if (a ? b : (c ? d : e)) {}",
errors: [{ messageId: "unexpectedCall" }]
}
]
});