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

Update: warn about mixing ternary and logical operators (fixes #11704) #12001

Merged
merged 10 commits into from Aug 13, 2019
Merged

Update: warn about mixing ternary and logical operators (fixes #11704) #12001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b01d47f
ternary operators added to tule
karthikkp Jul 16, 2019
0ae2e91
ternary operators added
karthikkp Jul 16, 2019
dc2e975
cases added for ternary operator mixed with logical
karthikkp Jul 17, 2019
be4a213
documentation updated
karthikkp Jul 17, 2019
849caa7
hard coded string message removed and in place added the operator..
karthikkp Jul 17, 2019
885abd2
test cases fixed
karthikkp Jul 17, 2019
b6cafb4
added the option to check for ternary operator and extra tests
karthikkp Jul 24, 2019
ed76433
documentation and tests updated
karthikkp Jul 25, 2019
269f309
check for test or left node moved to a sperate function
karthikkp Jul 25, 2019
03dcd51
wrong comment. Has been updated
karthikkp Jul 26, 2019
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
32 changes: 30 additions & 2 deletions docs/rules/no-mixed-operators.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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 '&&'." }
]
}
]
});