Skip to content

Commit

Permalink
feat: add enforceForInnerExpressions option to `no-extra-boolean-ca…
Browse files Browse the repository at this point in the history 
…st` (#18222)

* fix: [no-extra-boolean-cast] inspect comma expressions and ?? expressions.

Fixes #18186

* check ternaries recursively too

* pr feedback

* change docs

* add option for recursive expression checks

* better wording

* format

* enforceForInnerExpressions

* feedback

* nice, anyOf is clever about additionalProperties

* test cov

* test coverage

* docs
  • Loading branch information
@kirkwaiblinger
kirkwaiblinger committed May 6, 2024
1 parent 0de0909 commit db0b174
Show file tree
Hide file tree
Showing 3 changed files with 1,457 additions and 216 deletions.
57 changes: 41 additions & 16 deletions docs/src/rules/no-extra-boolean-cast.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,6 @@ title: no-extra-boolean-cast
rule_type: suggestion
---





In contexts such as an `if` statement's test where the result of the expression will already be coerced to a Boolean, casting to a Boolean via double negation (`!!`) or a `Boolean` call is unnecessary. For example, these `if` statements are equivalent:

```js
Expand Down Expand Up @@ -88,16 +84,18 @@ var foo = bar ? !!baz : !!bat;

This rule has an object option:

* `"enforceForLogicalOperands"` when set to `true`, in addition to checking default contexts, checks whether the extra boolean cast is contained within a logical expression. Default is `false`, meaning that this rule by default does not warn about extra booleans cast inside logical expression.
* `"enforceForInnerExpressions"` when set to `true`, in addition to checking default contexts, checks whether extra boolean casts are present in expressions whose result is used in a boolean context. See examples below. Default is `false`, meaning that this rule by default does not warn about extra booleans cast inside inner expressions.

**Deprecated:** The object property `enforceForLogicalOperands` is deprecated ([eslint#18222](https://github.com/eslint/eslint/pull/18222)). Please use `enforceForInnerExpressions` instead.

### enforceForLogicalOperands
### enforceForInnerExpressions

Examples of **incorrect** code for this rule with `"enforceForLogicalOperands"` option set to `true`:
Examples of **incorrect** code for this rule with `"enforceForInnerExpressions"` option set to `true`:

::: incorrect

```js
/*eslint no-extra-boolean-cast: ["error", {"enforceForLogicalOperands": true}]*/
/*eslint no-extra-boolean-cast: ["error", {"enforceForInnerExpressions": true}]*/

if (!!foo || bar) {
//...
Expand All @@ -107,23 +105,38 @@ while (!!foo && bar) {
//...
}

if ((!!foo || bar) && baz) {
if ((!!foo || bar) && !!baz) {
//...
}

foo && Boolean(bar) ? baz : bat
var foo = new Boolean(!!bar || baz);

var foo = new Boolean(!!bar || baz)
foo && Boolean(bar) ? baz : bat;

const ternaryBranches = Boolean(bar ? !!baz : bat);

const nullishCoalescingOperator = Boolean(bar ?? Boolean(baz));

const commaOperator = Boolean((bar, baz, !!bat));

// another comma operator example
for (let i = 0; console.log(i), Boolean(i < 10); i++) {
// ...
}
```
:::
Examples of **correct** code for this rule with `"enforceForLogicalOperands"` option set to `true`:
Examples of **correct** code for this rule with `"enforceForInnerExpressions"` option set to `true`:
::: correct
```js
/*eslint no-extra-boolean-cast: ["error", {"enforceForLogicalOperands": true}]*/
/*eslint no-extra-boolean-cast: ["error", {"enforceForInnerExpressions": true}]*/

// Note that `||` and `&&` alone aren't a boolean context for either operand
// since the resultant value need not be a boolean without casting.
var foo = !!bar || baz;

if (foo || bar) {
//...
Expand All @@ -137,11 +150,23 @@ if ((foo || bar) && baz) {
//...
}

foo && bar ? baz : bat
var foo = new Boolean(bar || baz);

var foo = new Boolean(bar || baz)
foo && bar ? baz : bat;

var foo = !!bar || baz;
const ternaryBranches = Boolean(bar ? baz : bat);

const nullishCoalescingOperator = Boolean(bar ?? baz);

const commaOperator = Boolean((bar, baz, bat));

// another comma operator example
for (let i = 0; console.log(i), i < 10; i++) {
// ...
}

// comma operator in non-final position
Boolean((Boolean(bar), baz, bat));
```
:::
106 changes: 79 additions & 27 deletions lib/rules/no-extra-boolean-cast.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,28 @@ module.exports = {
},

schema: [{
type: "object",
properties: {
enforceForLogicalOperands: {
type: "boolean",
default: false
anyOf: [
{
type: "object",
properties: {
enforceForInnerExpressions: {
type: "boolean"
}
},
additionalProperties: false
},

// deprecated
{
type: "object",
properties: {
enforceForLogicalOperands: {
type: "boolean"
}
},
additionalProperties: false
}
},
additionalProperties: false
]
}],
fixable: "code",

Expand All @@ -49,6 +63,9 @@ module.exports = {

create(context) {
const sourceCode = context.sourceCode;
const enforceForLogicalOperands = context.options[0]?.enforceForLogicalOperands === true;
const enforceForInnerExpressions = context.options[0]?.enforceForInnerExpressions === true;


// Node types which have a test which will coerce values to booleans.
const BOOLEAN_NODE_TYPES = new Set([
Expand All @@ -72,19 +89,6 @@ module.exports = {
node.callee.name === "Boolean";
}

/**
* Checks whether the node is a logical expression and that the option is enabled
* @param {ASTNode} node the node
* @returns {boolean} if the node is a logical expression and option is enabled
*/
function isLogicalContext(node) {
return node.type === "LogicalExpression" &&
(node.operator === "||" || node.operator === "&&") &&
(context.options.length && context.options[0].enforceForLogicalOperands === true);

}


/**
* Check if a node is in a context where its value would be coerced to a boolean at runtime.
* @param {ASTNode} node The node
Expand Down Expand Up @@ -115,12 +119,51 @@ module.exports = {
return isInFlaggedContext(node.parent);
}

return isInBooleanContext(node) ||
(isLogicalContext(node.parent) &&
/*
* legacy behavior - enforceForLogicalOperands will only recurse on
* logical expressions, not on other contexts.
* enforceForInnerExpressions will recurse on logical expressions
* as well as the other recursive syntaxes.
*/

if (enforceForLogicalOperands || enforceForInnerExpressions) {
if (node.parent.type === "LogicalExpression") {
if (node.parent.operator === "||" || node.parent.operator === "&&") {
return isInFlaggedContext(node.parent);
}

// For nested logical statements
isInFlaggedContext(node.parent)
);
// Check the right hand side of a `??` operator.
if (enforceForInnerExpressions &&
node.parent.operator === "??" &&
node.parent.right === node
) {
return isInFlaggedContext(node.parent);
}
}
}

if (enforceForInnerExpressions) {
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);
}

}

return isInBooleanContext(node);
}


Expand All @@ -147,7 +190,6 @@ module.exports = {
* Determines whether the given node needs to be parenthesized when replacing the previous node.
* It assumes that `previousNode` is the node to be reported by this rule, so it has a limited list
* of possible parent node types. By the same assumption, the node's role in a particular parent is already known.
* For example, if the parent is `ConditionalExpression`, `previousNode` must be its `test` child.
* @param {ASTNode} previousNode Previous node.
* @param {ASTNode} node The node to check.
* @throws {Error} (Unreachable.)
Expand All @@ -157,6 +199,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,9 +217,18 @@ module.exports = {
case "DoWhileStatement":
case "WhileStatement":
case "ForStatement":
case "SequenceExpression":
return false;
case "ConditionalExpression":
return precedence(node) <= precedence(parent);
if (previousNode === parent.test) {
return precedence(node) <= precedence(parent);
}
if (previousNode === parent.consequent || previousNode === parent.alternate) {
return precedence(node) < precedence({ type: "AssignmentExpression" });
}

/* c8 ignore next */
throw new Error("Ternary child must be test, consequent, or alternate.");
case "UnaryExpression":
return precedence(node) < precedence(parent);
case "LogicalExpression":
Expand Down

0 comments on commit db0b174

Please sign in to comment.