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.

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

Update: enforceForLogicalOperands no-extra-boolean-cast (fixes #12137) #12734

Merged
merged 15 commits into from Feb 14, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
50 changes: 50 additions & 0 deletions docs/rules/no-extra-boolean-cast.md
Expand Up @@ -68,3 +68,53 @@ function foo() {

var foo = bar ? !!baz : !!bat;
```

## Options

This rule has an object option:

* `"enforceForLogicalOperands"` when set to `true`, checks whether the extra boolean is contained within a logical expression. Default is `false`, meaning that this rule by default does not warn about extra booleans inside logical expression.

mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
### enforceForLogicalOperands

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

```js
/*eslint no-extra-boolean-cast: ["error", {"enforceForLogicalOperands": true}]*/
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved

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

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

var foo = new Boolean(!!bar || baz)


if (Boolean(foo || bar)) {
// ...
}
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
```

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

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

if (foo || bar) {
//...
}

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

var foo = new Boolean(bar || baz)


if (foo || bar) {
// ...
}
```
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
88 changes: 70 additions & 18 deletions lib/rules/no-extra-boolean-cast.js
Expand Up @@ -26,7 +26,16 @@ module.exports = {
url: "https://eslint.org/docs/rules/no-extra-boolean-cast"
},

schema: [],
schema: [{
type: "object",
properties: {
enforceForLogicalOperands: {
type: "boolean",
default: false
}
},
additionalProperties: false
}],
fixable: "code",

messages: {
Expand All @@ -47,23 +56,67 @@ module.exports = {
"ForStatement"
];

/**
* Check if a node is a Boolean function or constructor.
* @param {ASTNode} node the node
* @returns {boolean} If the node is in the context
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
*/
function isFunctionOrConstructorContext(node) {
return (node.type === "CallExpression" || node.type === "NewExpression") &&
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to ensure that Boolean isn't a shadowed global variable?

Copy link
Member

Choose a reason for hiding this comment

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

That should be also done in CallExpression(node).

Though I think that both global Boolean fixes can be a separate PR because it's an existing issue (this function was extracted from UnaryExpression(node)).

Copy link
Member

Choose a reason for hiding this comment

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

Separating it out into a separate PR sounds good to me!

node.callee.type === "Identifier" &&
node.callee.name === "Boolean";


}
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved

/**
* 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" &&
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
(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
* @param {ASTNode} parent Its parent
* @returns {boolean} If it is in a boolean context
*/
function isInBooleanContext(node, parent) {
function isBooleanContext(node) {
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved

// Boolean(<bool>) and new Boolean(<bool>)
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
return (
(BOOLEAN_NODE_TYPES.indexOf(parent.type) !== -1 &&
node === parent.test) ||
(BOOLEAN_NODE_TYPES.indexOf(node.parent.type) !== -1 &&
node === node.parent.test) ||

// !<bool>
(parent.type === "UnaryExpression" &&
parent.operator === "!")
(node.parent.type === "UnaryExpression" &&
node.parent.operator === "!")
);
}

/**
* Checks whether the node is a context that should report an error
* Acts recursively if it is in a logical context
* @param {ASTNode} node the node
* @returns {boolean} If the node is in one of the flagged contexts
*/
function isFlaggedContext(node) {
return isBooleanContext(node) ||
isFunctionOrConstructorContext(node.parent) ||
(isLogicalContext(node.parent) &&

// For nested logical statements
isFlaggedContext(node.parent, node.parent.parent)
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
);
}


/**
* Check if a node has comments inside.
* @param {ASTNode} node The node to check.
Expand All @@ -76,23 +129,18 @@ module.exports = {
return {
UnaryExpression(node) {
const ancestors = context.getAncestors(),
parent = ancestors.pop(),
grandparent = ancestors.pop();
parent = ancestors.pop();

mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved

// Exit early if it's guaranteed not to match
if (node.operator !== "!" ||
parent.type !== "UnaryExpression" ||
parent.operator !== "!") {
parent.type !== "UnaryExpression" ||
parent.operator !== "!") {
return;
}

if (isInBooleanContext(parent, grandparent) ||

// Boolean(<bool>) and new Boolean(<bool>)
((grandparent.type === "CallExpression" || grandparent.type === "NewExpression") &&
grandparent.callee.type === "Identifier" &&
grandparent.callee.name === "Boolean")
) {
if (isFlaggedContext(parent)) {
context.report({
node: parent,
messageId: "unexpectedNegation",
Expand All @@ -110,6 +158,10 @@ module.exports = {
prefix = " ";
}

if (astUtils.getPrecedence(node.argument) < astUtils.getPrecedence(parent)) {
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
return fixer.replaceText(parent, `(${prefix + sourceCode.getText(node.argument)})`);
mdjermanovic marked this conversation as resolved.
Show resolved Hide resolved
}

return fixer.replaceText(parent, prefix + sourceCode.getText(node.argument));
}
});
Expand All @@ -122,7 +174,7 @@ module.exports = {
return;
}

if (isInBooleanContext(node, parent)) {
if (isFlaggedContext(node)) {
context.report({
node,
messageId: "unexpectedCall",
Expand Down