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

New rule: Disallow mixes of different operators (no-mixed-operators) #566

Closed
feross opened this Issue Jul 13, 2016 · 17 comments

Comments

7 participants
@feross
Copy link
Member

feross commented Jul 13, 2016

http://eslint.org/docs/rules/no-mixed-operators

What do folks think about this rule?

I've always found it super confusing when multiple operators with similar precedence are used without explicit parens. For example:

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

// GOOD
var foo = (a && b) || c || d

// GOOD
var foo = a && (b || c || d)

There are currently a lot of repos that fail when this rule is enabled:

1..424
# tests 424
# pass  394
# fail  30

Maybe we can limit it a bit to make it more palatable? There are lots of possible errors. These were pretty common:

  • Unexpected mix of '&&' and '||'.
  • Unexpected mix of '<<' and '|'.
  • Unexpected mix of '*' and '+'.
  • Unexpected mix of '/' and '+'.

The last two are basic algebra order-of-operations that folks usually learn in elementary/middle school, so maybe we can skip warning for those? And the warnings involving << happen on modules that are super low-level where we might be able to assume the programmer knows what they're doing.

If we just warn when '&&' and '||' are used together without explicit parens, then we get a more reasonable number of warnings:

1..424
# tests 424
# pass  408
# fail  16

Curious to get people's thoughts. Is this generally helpful, or just a problem that I have? 😉

@feross feross added the question label Jul 13, 2016

@sotojuan

This comment has been minimized.

Copy link

sotojuan commented Jul 14, 2016

👍 Pretty sure this is taught as a common best practice anyway. I'm all for disallowing && and || together only.

@nathanph

This comment has been minimized.

Copy link

nathanph commented Jul 14, 2016

I agree with @sotojuan. Programmers who know what they're doing are probably already parenthesizing their statements. IMO it's naive to assume that someone writing low-level code is more likely to know what they're doing.

Even with relatively simple operations parenthesizing helps a ton.

// This is difficult to read, not complicated.
const foo = 3 + 4 / 5 + 3 + 5 * 4 + 6

// This is much easier to read.
const foo = 3 + (4 / 5) + 3 + (5 * 4) + 6
@dcousens

This comment has been minimized.

Copy link
Member

dcousens commented Jul 15, 2016

Agreed, if mixing: parenthesis.

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

yoshuawuyts commented Jul 16, 2016

Yup, I still mess this up every now and then haha

@feross feross added this to the standard v9 milestone Aug 19, 2016

@feross feross added enhancement and removed question labels Aug 19, 2016

@feross

This comment has been minimized.

Copy link
Member Author

feross commented Aug 19, 2016

Targeting this for standard v9.

feross added a commit to standard/eslint-config-standard that referenced this issue Feb 8, 2017

@feross

This comment has been minimized.

Copy link
Member Author

feross commented Feb 8, 2017

This will be part of the standard v9 beta.

We will check all operators except for bitwise operators, since it's very common in low-level modules to omit parens when dealing with binary operators. Maybe we can consider making this stricter in a future version, but I doubt we'll want to. The rule is already very aggressive.

    "no-mixed-operators": ["error", {
        "groups": [
            ["+", "-", "*", "/", "%", "**"],
            ["==", "!=", "===", "!==", ">", ">=", "<", "<="],
            ["&&", "||"],
            ["in", "instanceof"]
        ],
        "allowSamePrecedence": true
    }],

Ecosystem effect is pretty large, but I looked at each failing example and agree that parens ought to be used in each instance to improve clarity. Many of the errors were in my own repos.

1..422
# tests 422
# pass  395
# fail  27

@feross feross closed this Feb 8, 2017

@feross

This comment has been minimized.

Copy link
Member Author

feross commented Feb 9, 2017

After fixing repos I control, ecosystem impact looks better, but still substantial.

1..422
# tests 422
# pass  402
# fail  20
@timoxley

This comment has been minimized.

Copy link
Contributor

timoxley commented Feb 9, 2017

I do use this pattern a bit: var a = obj && obj.prop || defaultValue

Edit: just read this more closely:

We will check all operators except for binary operators

So IIUC the above pattern would not be affected. If so, LGTM

Edit 2: lol you wrote "binary" and I read "boolean" but you meant "bitwise"

@feross

This comment has been minimized.

Copy link
Member Author

feross commented Feb 9, 2017

Oh, sorry, I meant bitwise operators, like |, &, etc. (corrected!). With this rule, you'd have to write: var a = (obj && obj.prop) || defaultValue

Personally, I find it confusing to read that line without the parens, even though I know I've written lines like that before. It's harder to visually parse what's going on when reading it, IMO

@timoxley

This comment has been minimized.

Copy link
Contributor

timoxley commented Feb 9, 2017

Yeah ok, the parens aren't too bad.

I note some irony since in the past I've seen it argued, possibly by me, that explicit semicolons everywhere is as superfluous as explicit parens everywhere. "Just learn these simple operator precedence rules, it's fine. Really!" I realise now that it's a false equivalence though, operator precedence has a great many more rules to remember than ASI.

@feross

This comment has been minimized.

Copy link
Member Author

feross commented Feb 9, 2017

operator precedence has a great many more rules to remember than ASI.

This is so true. 😄

Btw, this is just for the beta release, so worst case if it sucks we can revert before release.

feross added a commit to standard/eslint-config-standard that referenced this issue Feb 9, 2017

@mafintosh

This comment has been minimized.

Copy link
Contributor

mafintosh commented Feb 10, 2017

Not being able to use simple math with +, -, /, * without having to force parens seems annoying (i write a lot of math code). Bit-wise ops are confusing though in terms of preference!

@mafintosh

This comment has been minimized.

Copy link
Contributor

mafintosh commented Feb 10, 2017

I can already hear my first grade math teacher yelling at me for not knowing the basic operation order

@feross

This comment has been minimized.

Copy link
Member Author

feross commented Feb 10, 2017

@mafintosh We could turn it off for +, -, /, * since I agree that basic arithmetic order is well-understood. But on longer lines it's still helpful, e.g.

 return value - Math.floor((value - min) / range) * range

vs.

 return value - (Math.floor((value - min) / range) * range)
@mafintosh

This comment has been minimized.

Copy link
Contributor

mafintosh commented Feb 10, 2017

I don't personally find that helpful but i get your point.

Stuff like this is incredible hard to parse though and i mess up ALL the time so I'd def be okay with enforcing it there

if (a & 1 === 0) { // same as a & (1 === 0)
}
@mafintosh

This comment has been minimized.

Copy link
Contributor

mafintosh commented Feb 10, 2017

@feross A more common mistake I've seen is something like

return value - a + b

Where someone ment

return value - (a + b)

So just forcing them all over the place for the classic ops makes it harder to question if someone ment to add them or not imo

@feross

This comment has been minimized.

Copy link
Member Author

feross commented Feb 10, 2017

@mafintosh The rule currently doesn't warn about operators that have the exact same precedence, so return value - a + b would be allowed. Not sure I followed your point -- does this make it better/worse for you?

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.