Interesting bugs caught by no-constant-binary-expression

In ESLint v8.14.0 I contributed a new core rule called no-constant-binary-expression which has surprised me with the wide variety of subtle and interesting bugs it has been able to detect.

In this post I’ll explain what the rule does and share some examples of real bugs it has detected in popular open source projects such as Material UI, Webpack, VS Code, and Firefox as well as a few interesting bugs that it found internally at Meta. I hope these examples will convince you to try enabling the rule in the projects you work on!

What does no-constant-binary-expression do?

The rule checks for comparisons (==, !==, etc) where the outcome cannot vary at runtime, and logical expressions (&&, ??, ||) which will either always or never short-circuit.

For example:

• +x == null will always be false, because + will coerce x into a number, and a number is never nullish.
• { ...foo } || DEFAULT will never return DEFAULT because objects are always truthy.

Both of these are examples of expressions that look like they can affect the way the program evaluates, but in reality, do not.

This rule originally started as just an attempt to detect unnecessary null checks. However, as I worked on it, I realized useless null checks were just a special case of a broader category: useless code. Eventually it clicked for me: developers don’t intend to write useless code, and code that does not match the developer’s intent is by definition a bug. Therefore, any useless code you can detect is a bug.

This realization was confirmed for me when I ran the first version of the rule against our code base at Meta, and it detected a wide variety of subtle and interesting bugs which had made it through code review.

Real world bugs found with no-constant-binary-expressions

In this section I’ll share a number of types of bugs that this rule can catch. Each includes at least one concrete example detected in a popular open source project. My choice to include real examples here is not to shame anyone or any project, but to drive home the fact that these are errors that any team can easily make.

Confusing operator precedence

The most common class of bug the rule finds is places where developers misunderstood the precedence of operators, particularly unary operators like !, + and typeof.

if (!whitelist.has(specifier.imported.name) == null) {
  return;
}

From Material UI (also: VS Code 1, 2, Webpack, Mozilla)

Confusing ?? and || precedence

When trying to define default values, people get confused with expressions like a === b ?? c and assume it will be parsed as a === (b ?? c). When in actuality it will be parsed as (a === b) ?? c.

shouldShowWelcome() {
  return this.viewModel?.welcomeExperience === WelcomeExperience.ForWorkspace ?? true;
}

From VS Code

Aside: Observing how frequently developers get confused by operator precedence inspired me to experiment with a VS Code extension to visually clarify how precedence gets interpreted.

Expecting objects to be compared by value

Developers coming from other languages where structures are compared by value, rather than by reference, can easily fall into the trap of thinking they can do things like test if an object is empty by comparing with a newly created empty object. Or course in JavaScript, objects are compared by reference, and no value can ever be equal to a newly constructed object literal.

In this example, hasData will always be set to true because data can never be referentially equal to a newly created object.

hasData = hasData || data !== {};

From Firefox (also: Firefox)

Expecting empty objects to be false or null

Another common categrory of JavaScript error is expecting empty objects to be nullish or falsy. This is likely an easy mistake to make for folks coming from a language like Python where empty lists and dictionaries are falsy.

const newConfigValue = { ...configProfiles } ?? {};

From VS Code (also: VS Code 1, 2)

Is it >= or =>?

I’ve only seen this particular typo once, but I wanted to include it because it’s a great example of the unexpected types of bugs this rule can catch.

Here, the developer meant to test if a value was greater than or equal to zero (>= 0), but accidentally reversed the order of the characters and created an arrow function that returned 0 && startWidth <= 1!

assert(startWidth => 0 && startWidth <= 1);

From Mozilla

Other errors caught by no-constant-binary-expression

The above five categories of errors are not exhaustive. When I originally ran the first version of this rule on our (very) large monorepo at Meta, it found over 500 issues. While many fell into the categories outlined above, there was also a long-tail of other interesting bugs. Some highlights include:

• Thinking || allows for set operations: states.includes('VALID' || 'IN_PROGRESS')
• Thinking primitive functions pass through nulls: Number(x) == null
• Not knowing primitive constructors return boxed primitives: new Number(x) === 10

I never would have set out to lint for these specific issues individually, but by simply trying to identify anything “useless” we were able to find and correct them.

Conclusion

As you’ve now seen no-constant-binary-expression is capable of detecting a variety of different types of bugs. The rule accomplishes this not because it’s programmed to look for those specific issues, but because all those bugs have one thing in common: they manifest as useless code. Because developers generally don’t intend to write useless code, detecting useless code generally results in detecting bugs.

If you’ve found these examples compelling, please consider enabling no-constant-binary-expression in your ESLint config:

// eslintrc
module.exports = {
  rules: {
    // Requires eslint >= v8.14.0
    "no-constant-binary-expression": "error"
  }
}

If you do, and it finds bugs, I’d love to hear about them!

Thanks to Brad Zacher for the original observation which inspired this work and the suggestion to propose it as a new core rule. And thanks to Milos Djermanovic for significant contributions during code review.