Skip to content

Commit

Permalink
Update: warn about mixing ternary and logical operators (fixes #11704) (
Browse files Browse the repository at this point in the history 
#12001)

* ternary operators added to tule

* ternary operators added

* cases added for ternary operator mixed with logical

* documentation updated

* hard coded string message removed and in place added the operator..

* test cases fixed

* added the option to check for ternary operator and extra tests

* documentation and tests updated

* check for test or left node moved to a sperate function

* wrong comment. Has been updated
  • Loading branch information
@karthikkp @aladdin-add
karthikkp authored and aladdin-add committed Aug 13, 2019
1 parent 11be2f8 commit 1aff8fc
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 16 deletions.
32 changes: 30 additions & 2 deletions docs/rules/no-mixed-operators.md
View file
Expand Up @@ -5,6 +5,8 @@ This rule warns when different operators are used consecutively without parenthe

```js
var foo = a && b || c || d; /*BAD: Unexpected mix of '&&' and '||'.*/
var foo = a && b ? c : d; /*BAD: Unexpected mix of '&&' and '?:'.*/
var foo = (a && b) ? c : d; /*GOOD*/
var foo = (a && b) || c || d; /*GOOD*/
var foo = a && (b || c || d); /*GOOD*/
```
Expand All @@ -23,10 +25,21 @@ will generate
1:18 Unexpected mix of '&&' and '||'. (no-mixed-operators)
```

```js
var foo = a && b ? c : d;
```

will generate

```sh
1:13 Unexpected mix of '&&' and '?:'. (no-mixed-operators)
1:18 Unexpected mix of '&&' and '?:'. (no-mixed-operators)
```


## Rule Details

This rule checks `BinaryExpression` and `LogicalExpression`.
This rule checks `BinaryExpression`, `LogicalExpression` and `ConditionalExpression`.

This rule may conflict with [no-extra-parens](no-extra-parens.md) rule.
If you use both this and [no-extra-parens](no-extra-parens.md) rule together, you need to use the `nestedBinaryExpressions` option of [no-extra-parens](no-extra-parens.md) rule.
Expand Down Expand Up @@ -75,7 +88,8 @@ var foo = (a + b) * c;

This rule has 2 options.

* `groups` (`string[][]`) - specifies operator groups to be checked. The `groups` option is a list of groups, and a group is a list of binary operators. Default operator groups are defined as arithmetic, bitwise, comparison, logical, and relational operators.
* `groups` (`string[][]`) - specifies operator groups to be checked. The `groups` option is a list of groups, and a group is a list of binary operators. Default operator groups are defined as arithmetic, bitwise, comparison, logical, and relational operators. Note: Ternary operator(?:) can be part of any group and by default is allowed to be mixed with other operators.

* `allowSamePrecedence` (`boolean`) - specifies whether to allow mixed operators if they are of equal precedence. Default is `true`.

### groups
Expand All @@ -87,6 +101,7 @@ The following operators can be used in `groups` option:
* Comparison Operators: `"=="`, `"!="`, `"==="`, `"!=="`, `">"`, `">="`, `"<"`, `"<="`
* Logical Operators: `"&&"`, `"||"`
* Relational Operators: `"in"`, `"instanceof"`
* Ternary Operator: `?:`

Now, consider the following group configuration: `{"groups": [["&", "|", "^", "~", "<<", ">>", ">>>"], ["&&", "||"]]}`.
There are 2 groups specified in this configuration: bitwise operators and logical operators.
Expand All @@ -102,6 +117,12 @@ var foo = a && b < 0 || c > 0 || d + 1 === 0;
var foo = a & b | c;
```

```js
/*eslint no-mixed-operators: ["error", {"groups": [["&&", "||", "?:"]]}]*/

var foo = a || b ? c : d;
```

Examples of **correct** code for this rule with `{"groups": [["&", "|", "^", "~", "<<", ">>", ">>>"], ["&&", "||"]]}` option:

```js
Expand All @@ -118,6 +139,13 @@ var foo = a + (b * c);
var foo = (a + b) * c;
```

```js
/*eslint no-mixed-operators: ["error", {"groups": [["&&", "||", "?:"]]}]*/

var foo = (a || b) ? c : d;
var foo = a || (b ? c : d);
```

### allowSamePrecedence

Examples of **correct** code for this rule with `{"allowSamePrecedence": true}` option:
Expand Down
61 changes: 48 additions & 13 deletions lib/rules/no-mixed-operators.js
View file
Expand Up @@ -20,12 +20,14 @@ const BITWISE_OPERATORS = ["&", "|", "^", "~", "<<", ">>", ">>>"];
const COMPARISON_OPERATORS = ["==", "!=", "===", "!==", ">", ">=", "<", "<="];
const LOGICAL_OPERATORS = ["&&", "||"];
const RELATIONAL_OPERATORS = ["in", "instanceof"];
const TERNARY_OPERATOR = ["?:"];
const ALL_OPERATORS = [].concat(
ARITHMETIC_OPERATORS,
BITWISE_OPERATORS,
COMPARISON_OPERATORS,
LOGICAL_OPERATORS,
RELATIONAL_OPERATORS
RELATIONAL_OPERATORS,
TERNARY_OPERATOR
);
const DEFAULT_GROUPS = [
ARITHMETIC_OPERATORS,
Expand All @@ -34,7 +36,7 @@ const DEFAULT_GROUPS = [
LOGICAL_OPERATORS,
RELATIONAL_OPERATORS
];
const TARGET_NODE_TYPE = /^(?:Binary|Logical)Expression$/u;
const TARGET_NODE_TYPE = /^(?:Binary|Logical|Conditional)Expression$/u;

/**
* Normalizes options.
Expand Down Expand Up @@ -65,6 +67,18 @@ function includesBothInAGroup(groups, left, right) {
return groups.some(group => group.indexOf(left) !== -1 && group.indexOf(right) !== -1);
}

/**
* Checks whether the given node is a conditional expression and returns the test node else the left node.
*
* @param {ASTNode} node - A node which can be a BinaryExpression or a LogicalExpression node.
* This parent node can be BinaryExpression, LogicalExpression
* , or a ConditionalExpression node
* @returns {ASTNode} node the appropriate node(left or test).
*/
function getChildNode(node) {
return node.type === "ConditionalExpression" ? node.test : node.left;
}

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -121,7 +135,7 @@ module.exports = {
const b = node.parent;

return (
!includesBothInAGroup(options.groups, a.operator, b.operator) ||
!includesBothInAGroup(options.groups, a.operator, b.type === "ConditionalExpression" ? "?:" : b.operator) ||
(
options.allowSamePrecedence &&
astUtils.getPrecedence(a) === astUtils.getPrecedence(b)
Expand All @@ -139,12 +153,25 @@ module.exports = {
* @returns {boolean} `true` if the node was mixed.
*/
function isMixedWithParent(node) {

return (
node.operator !== node.parent.operator &&
!astUtils.isParenthesised(sourceCode, node)
);
}

/**
* Checks whether the operator of a given node is mixed with a
* conditional expression.
*
* @param {ASTNode} node - A node to check. This is a conditional
* expression node
* @returns {boolean} `true` if the node was mixed.
*/
function isMixedWithConditionalParent(node) {
return !astUtils.isParenthesised(sourceCode, node) && !astUtils.isParenthesised(sourceCode, node.test);
}

/**
* Gets the operator token of a given node.
*
Expand All @@ -153,7 +180,7 @@ module.exports = {
* @returns {Token} The operator token of the node.
*/
function getOperatorToken(node) {
return sourceCode.getTokenAfter(node.left, astUtils.isNotClosingParenToken);
return sourceCode.getTokenAfter(getChildNode(node), astUtils.isNotClosingParenToken);
}

/**
Expand All @@ -167,13 +194,13 @@ module.exports = {
*/
function reportBothOperators(node) {
const parent = node.parent;
const left = (parent.left === node) ? node : parent;
const right = (parent.left !== node) ? node : parent;
const left = (getChildNode(parent) === node) ? node : parent;
const right = (getChildNode(parent) !== node) ? node : parent;
const message =
"Unexpected mix of '{{leftOperator}}' and '{{rightOperator}}'.";
const data = {
leftOperator: left.operator,
rightOperator: right.operator
leftOperator: left.operator || "?:",
rightOperator: right.operator || "?:"
};

context.report({
Expand All @@ -198,17 +225,25 @@ module.exports = {
* @returns {void}
*/
function check(node) {
if (TARGET_NODE_TYPE.test(node.parent.type) &&
isMixedWithParent(node) &&
!shouldIgnore(node)
) {
reportBothOperators(node);
if (TARGET_NODE_TYPE.test(node.parent.type)) {
if (node.parent.type === "ConditionalExpression" && !shouldIgnore(node) && isMixedWithConditionalParent(node.parent)) {
reportBothOperators(node);
} else {
if (TARGET_NODE_TYPE.test(node.parent.type) &&
isMixedWithParent(node) &&
!shouldIgnore(node)
) {
reportBothOperators(node);
}
}
}

}

return {
BinaryExpression: check,
LogicalExpression: check

};
}
};
45 changes: 44 additions & 1 deletion tests/lib/rules/no-mixed-operators.js
View file
Expand Up @@ -47,7 +47,18 @@ ruleTester.run("no-mixed-operators", rule, {
{
code: "a * b / c",
options: [{ allowSamePrecedence: true }]
}
},
{
code: "(a || b) ? c : d",
options: [{ groups: [["&&", "||", "?:"]] }]
},
{
code: "a || (b ? c : d)",
options: [{ groups: [["&&", "||", "?:"]] }]
},
"a || (b ? c : d)",
"(a || b) ? c : d",
"a || b ? c : d"
],
invalid: [
{
Expand Down Expand Up @@ -110,6 +121,38 @@ ruleTester.run("no-mixed-operators", rule, {
{ column: 3, message: "Unexpected mix of '*' and '/'." },
{ column: 7, message: "Unexpected mix of '*' and '/'." }
]
},
{
code: "a || b ? c : d",
options: [{ groups: [["&&", "||", "?:"]] }],
errors: [
{ column: 3, message: "Unexpected mix of '||' and '?:'." },
{ column: 8, message: "Unexpected mix of '||' and '?:'." }
]
},
{
code: "a && b ? 1 : 2",
options: [{ groups: [["&&", "||", "?:"]] }],
errors: [
{ column: 3, message: "Unexpected mix of '&&' and '?:'." },
{ column: 8, message: "Unexpected mix of '&&' and '?:'." }
]
},
{
code: "x ? a && b : 0",
options: [{ groups: [["&&", "||", "?:"]] }],
errors: [
{ column: 3, message: "Unexpected mix of '?:' and '&&'." },
{ column: 7, message: "Unexpected mix of '?:' and '&&'." }
]
},
{
code: "x ? 0 : a && b",
options: [{ groups: [["&&", "||", "?:"]] }],
errors: [
{ column: 3, message: "Unexpected mix of '?:' and '&&'." },
{ column: 11, message: "Unexpected mix of '?:' and '&&'." }
]
}
]
});

0 comments on commit 1aff8fc

Please sign in to comment.