New rule: no mixed logical operators

[jfmengels]

I'm proposing a rule that warns when logical operators like && and || are mixed. The purpose is to avoid errors like the one fixed here:

if (a && a.b === 1 || a.b === 2) {
}

In this example, we are mixing && and ||. We are probably expecting not to enter inside the if block if

• a is falsy
• a.b is neither 1 nor 2

but what the actual operation will be is (a && a.b === 1) || a.b === 2. Meaning, if a is undefined, then we will be evaluating a.b === 2, which will create an error.

If we had wrapped our different conditions in parentheses a && (a.b === 1 || a.b === 2), then this would have resulted in the wanted behavior.

Fail

if (a && a.b === 1 || a.b === 2) {
}

a && b || c
a || b && c

Pass

a && b && c
a || b || c
a && (b || c)
a || (b && c)

I think the rule is pretty generic and can help avoid "easy" mistakes, and therefore has it's place in ESLint. I don't mind adding it to a custom plugin otherwise. I'd be happy to write the rule :)

[pedrottimark]

The last passing example is at an intersection of rules. Not sure if that is considered a conflict, or not.

a || (b && c)

• incorrect code for no-extra-parens: ["error", "all"] EDIT: default option
• correct code for no-extra-parens: ["error", "all", { "nestedBinaryExpressions": false }]

The exception option allows extra parentheses. I think you are proposing a rule which requires them when a logical expression contains operators with different precedence.

[pedrottimark]

@jfmengels Was thinking about some more neutrally-named rules like object-curly-spacing which have options in both directions. Which lead to a thought: You might research if there is a JSCS rule which does what you need, because that would put your request on the merger-compatibility track in ESLint.

[jfmengels]

I think you are proposing a rule which requires them when a logical expression contains operators with different precedence.

Ah, I though that the logical AND and the logical OR had the same precedence, but you're right, they don't (OR has lower precedence). I was not thinking it that far ahead, simply trying to handle this simple case. I did wonder whether the rule should also apply to operators like addition/multiplication, but didn't want to make it way more complicated than it should be (though open to discussion on this).

You might research if there is a JSCS rule which does what you need

I'm not too familiar with JSCS' rules, but could unfortunately not find any resembling this.

The difference between this rule and object-curly-spacing or no-extra-parens, is that it's to prevent errors, while the other two are styling rules (note: did you see the potentially confusing use of both "and" and "or" in this sentence? :p)

[pedrottimark]

For my own learning, I will search for a rule which has options to disallow, require, or allow something.

Was just reading about no-extra-parens on Friday as part my next batch for consistency editing. The exception options, made me try to remember (unsuccessfully so far) where I read advice that if you need to research operator precedence to know if an expression is correct, add the unnecessary parentheses so the next reader doesn’t have to look it up (or worse, guesses a wrong meaning).

[mysticatea]

I'm a champion.
I think this stylistic rule is enough general for core. I remember mixed logical expression caused a bug in ESLint before.

This rule is opposite of a part of no-extra-parens, a bit similar to warp-iife rule.
I vote to a new rule: wrap-nested-logical-expression or something like.

This is not a JSCS compatibility issue.

[nzakas]

Isn't this just an issue of order of operations rather than logical operator mixing?

[jfmengels]

I vote to a new rule: wrap-nested-logical-expression or something like.

That would work for me 👍

Isn't this just an issue of order of operations rather than logical operator mixing?

Hmm, in a way I guess... But even if you're ordering operations like a || b && c && d || e as you wished for without parens, wrapping them makes for less error-prone and more readable code. In the case of the example I linked to in the issue, some expressions were wrapped, but in the end, the whole expression looked something like a && b || c || d || e because it was not wrapped well enough, and therefore introduced a bug.

[nzakas]

@mysticatea if this is opposite of no-extra-parens, how can we make it work without confusing users?

@jfmengels are you willing to create this rule if it's accepted? (See http://eslint.org/docs/developer-guide/contributing/new-rules)

We still need three 👍s from the team to implement.

[jfmengels]

are you willing to create this rule if it's accepted?

Absolutely.

I understand the conflict with no-extra-parens. If it doesn't get accepted, I'll push for it to get integrated into eslint-plugin-xo, or create a new plugin.

[mysticatea]

@nzakas no-extra-parens has nestedBinaryExpressions option, and it doesn't conflict with this proposal. So, we can describe about nestedBinaryExpressions in rule's document.

[nzakas]

@mysticatea okay, thanks for explaining.

Again, let's make sure that we think this rule proposal is extremely important to be included in ESLint.

[ilyavolodin]

I think this is a stylistic preferences that is hard to enforce 100% correctly (as in, I, for example, prefer to wrap nested logical operators like that only in a case I have more then 2 of them), I think this might be better as a plugin.

[nzakas]

@eslint/eslint-team we need some more opinions here.

[pedrottimark]

Agree with #6023 (comment) that there would be differences of opinion when a rule should require the extra parentheses.

In the existing doc, I intend to give stronger explanation for benefits of following config which allows extra parens in cases like the original problem:

no-extra-parens: ["error", "all", { "nestedBinaryExpressions": false }]

But leave it to human code reviewer on teams to decide exactly how much they need.

[mysticatea]

• As it caused a bug of ESLint before, a mix of logical expressions can confuse people. So I have ever seen "enclose mixed logical expressions" in some code style guides in Japan. I think this rule is pretty important.
• Indeed, guessing developer's intention is very difficult, but detecting a mix of logical expressions is easy. I made a prototype: http://astexplorer.net/#/7os6gj7FJO

[nzakas]

Okay, @mysticatea feels strongly, do it 👍

[platinumazure]

@nzakas Is this accepted?

[mysticatea]

I'm a champion, but I have not gotten 3 👍s from core team yet. If this is accepted, I will work on this.
@eslint/eslint-team I'm happy if somebody supports me.

[alberto]

Here's another one 👍

[mikesherov]

👍

[alberto]

@michaelficarra expressed disagreement about this, so I think the 3 votes aren't enough in this case.

[michaelficarra]

I don't see how this is specific to logical operators. Any infix operators with mixed precedence can cause an issue if left unparenthesised. For instance, I may intend a + b * c to mean (a + b) * c if I don't consider the precedence difference between + and *. And people actually do make this error, as we've just recently seen. So my understanding of the ask here is a rule that is completely the opposite of no-extra-parens.

[platinumazure]

One could argue that there's a difference in comprehensibility between
(mixed operators for calculating a value) versus (missed operators for
determining flow of control). It seems like it might be slightly more
important to have unambiguous code in a control flow condition.

That said, I agree with @michaelficarra and wonder if no-extra-parens
should be re-envisioned as a rule about precedence enforcement (on a per-operator basis). Although
the rule name would become problematic in that case...
On May 17, 2016 8:56 AM, "Michael Ficarra" notifications@github.com wrote:

I don't see how this is specific to logical operators. Any infix operators
with mixed precedence can cause an issue if left unparenthesised. For
instance, I may intend a + b * c to mean (a + b) * c if I don't consider
the precedence difference between + and *. So my understanding of the ask
here is a rule that is completely the opposite of no-extra-parens.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#6023 (comment)

[nzakas]

Yes, an active dissent means we need to talk through this some more.

[mysticatea]

Thank you, everyone, I'm happy 😄

I did not have the rule's idea for parentheses of arithmetic operators.
I'm OK to generalize this proposal for all binary operators. Then, #6081-like, we can have preferences for each operator.

---

no-mixed-operators

Enclosing complex expressions by parentheses makes more clarity of developer's intention, then the code would get more readability. This rule warns a use of mixed operators in a expression.

var foo = a && b || c || d;    /*error Unexpected a mix of `&&` and `||`.*/

This rule checks BinaryExpression and LogicalExpression.

Options

{
    "no-mixed-operators": ["error", {
        "ignore": []
    }]
}

• ignore (string[]) - specifies operators this rule should ignore. When this rule compares two operators, if either operator is included in this option, this rule ignores it. This value is a list of binary operators, logical operators.

Examples

/*eslint no-mixed-operators: "error" */

// ✘ BAD
var foo = a && b || c || d;
var foo = a + b * c;

// ✔ GOOD
var foo = (a && b) || c || d;
var foo = a && (b || c || d);
var foo = a + (b * c);
var foo = (a + b) * c;

/*eslint no-mixed-operators: ["error", {"ignore": ["+", "-", "*", "/"]}] */

// ✘ BAD
var foo = a && b || c || d;

// ✔ GOOD
var foo = (a && b) || c || d;
var foo = a && (b || c || d);
var foo = a + b * c;
var foo = a + (b * c);
var foo = (a + b) * c;

Hmm, the option to make comparison group might be wanted. (e.g. bit operators group, logical operators group, ...)

[mysticatea]

I rethought my proposal. The new proposal has groups option instead of ignore option.
In this proposal, we can specify groups of operators we want to prevent mixed uses.
I'd like to make forward with this proposal.

---

no-mixed-operators

Enclosing complex expressions by parentheses makes more clarity of developer's intention, then the code would get more readability. This rule warns a use of mixed operators in an expression.

var foo = a && b || c || d;    /*error Unexpected a mix of `&&` and `||`.*/

This rule checks BinaryExpression and LogicalExpression.

Options

{
    "no-mixed-operators": ["error", {
        "groups": []
    }]
}

• groups (string[][]) - specifies groups to compare operators.
  When this rule compares two operators, if both operators are included in a same group, this rule checks it.
  Otherwise, this rule ignores it.
  
  This value is a list of groups.
  The group is a list of binary operators and logical operators.
  
  Default is a group which includes all operators.

Examples

/*eslint no-mixed-operators: "error" */

// ✘ BAD
var foo = a && b || c > 0 || d + 1 === 0;
var foo = a + b * c;

// ✔ GOOD
var foo = (a && b) || (c > 0) || ((d + 1) === 0);
var foo = a && (b ||  (c > 0) || ((d + 1) === 0));
var foo = a + (b * c);
var foo = (a + b) * c;

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

// ✘ BAD
var foo = a && b || c > 0 || d + 1 === 0;
var foo = a + b * c;

// ✔ GOOD
var foo = (a && b) || c > 0 || d + 1 === 0;
var foo = a && (b || c > 0 || d + 1 === 0);
var foo = a + (b * c);
var foo = (a + b) * c;

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

// ✘ BAD
var foo = a && b || c > 0 || d + 1 === 0;

// ✔ GOOD
var foo = (a && b) || c > 0 || d + 1 === 0;
var foo = a && (b || c > 0 || d + 1 === 0);
var foo = a + b * c;
var foo = a + (b * c);
var foo = (a + b) * c;

[mikesherov]

I really like the groups proposal but I think we may want to have a couple of keywords as shorthand: logical, mathematical, and comparison. IMO, this will make the standard cases people will use easier to configure, considering that a lot of folks know about PEMDAS for mathematics but not the precedent of logical operators nor comparison operators.

[jfmengels]

@mysticatea I'm not sure I got this: In both your examples, would a + b - c be good or bad? I think this should be good, as their precedence is the same.

And i was about to propose the same thing as @mikesherov: keywords/"presets" of operators. I think that a lot of people would not think of adding the ** operator for instance (at least currently).

[mysticatea]

Thank you for the feedbacks!

Presets idea looks great!
But I have a concern. As you know, ** was added in ES2016. So operators can be added in future. I think adding new operators into presets is a breaking change. There is maintenance cost on presets.

ignoreSamePrecedence option (default is false) is fine to me. I'd like to add it.

[michaelficarra]

@mysticatea That's not a breaking change, since they were not using the operator before it was added to the language. You can think of it as the operator always having been in the preset.

[jfmengels]

It is for people who were using ** before upgrading to a version of ESLint who supports it. In this case, it will probably be in there from the start, but if new operators get into JS and some people use low stages of Babel very early on, then some new ESLint errors could appear in their codebase.

[michaelficarra]

@jfmengels No, the rule will support whatever is supported by the default parser. If someone uses a different parser, they need to use different rules. This project cannot be concerned with making each of its rules work with every imaginable parser.

[mikesherov]

presets should definitely include new operators as they're added to the language. exponentiation operator is a mathematical operator. To not have it in the mathematical preset would be confusing.

[mysticatea]

Honestly, I want not to add presets for this time. JS has keyword operators, so presets may be confusable.

{groups: [["logical", "arithmetic", "instanceof"]]}

Instead, documents show categories of operators (copyable).
We can add presets in future, but cannot remove/modify it without breaking.

[mysticatea]

I updated the reference implementation. The default value of options has changed.

May this go forward?

[ilyavolodin]

Marking as accepted since there's a champion and 3 +1s from the team.

[nzakas]

We need to resolve the disagreement before moving forward. @michaelficarra are your concerns addressed?

[michaelficarra]

@nzakas Yes, my concern was that this rule only applied to logical operators. Now that it has been re-envisioned to lint all binary operators, it's fine.

[mysticatea]

Thank you very much! ✨