eslint: Should we extend no-constant-condition to catch comparisons that are constant due to type mismatch?

That’s a really good point, thanks for clarifying.

I can work on this. Busy this weekend, but can pick it up next week.

@mdjermanovic Thanks for breaking it down like this. Thinking about it in this way, I realize that there are a actually five things that our rule does more aggressively than no-constant-condition. Maybe it would be easier to consider them individually?

5 Proposals for making no-constant-condition more aggressive

1. Consider short circuiting operations (|| , &&) to be conditions @mdjermanovic seems ok with adding this behind an option
2. Consider any comparison binary expression (==, !==) to be a “condition”. This would warn: myTestFunc({} == false)
3. Consider null conditions, not just boolean conditions ( {} ?? foo, !foo == null)
4. Follow identifiers to their assignment – if there is only one – to catch things like: const foo = {}; foo ? a : b Update: Moved to #13776 13776
5. Infer the return value of JSX ESLint has made a decision not to do this

If anyone feels strongly about breaking any of these questions out into their own issues just let me know and I’ll do that.

To clarify the motivations - Jordan has already done some initial explorations of this on the Facebook codebase and found that there were several hundred violations.

All of them were due to user error.

Some were due to a simple misunderstanding of how operator precedence worked:

const a = 1;
const b = nullOrNumber();
const fallback = 3;

const result = a + b ?? fallback;
// + has a higher precedence than ??, so it is executed first
// meaning this expression becomes (alwaysNumber ?? fallback) - i.e. the nullish coalesce is never evaluated

// The developer actually intended the code to be:
const result = a + (b ?? fallback);

function foo(arg) {
  if (!arg == null) {
    // ! has a higher precedence than ==, so it is evalutated first
    // meaning that this becomes boolean == null, which is a constantly false condition
  }
}

Some of them were due to deeper misunderstanding of how language features work:

const result = <MyComponent /> ?? <Fallback />;
// The developer made the assumption that if MyComponent's render returns null,
// then the JSX also returns null. This is not the case as the result of a JSX expression
// is not at all related to the return value of the render function

In most cases you can apply the same reasoning to == null/=== null checks, as the same mistakes apply there.

We figured if we’ve found several hundred errors in the Facebook codebase, then likely people outside of Facebook have made the same errors.

Some of these examples Jordan mentioned are covered by the no-unsafe-negation lint rule, however this rule just checks negations in binary operations.

Some might be covered by the no-mixed-operators lint rule, however:

• some people can’t use this due to conflicts with prettier which removes unnecessary parentheses.
• some cases (namely all those including ??) aren’t caught with the default rule config.

1. Consider short circuiting operations (|| , &&) to be conditions @mdjermanovic seems ok with adding this behind an option

I’d be willing to champion this, it looks like a good fit for no-constant-condition.

As @captbaritone noted, the rule would then cover all places where value of Boolean(expression) affects control flow:

• if (expression) {}
• while (expression) {}
• do {} while (expression)
• for (foo; expression; bar) {}
• expression ? foo : bar
• expression || foo
• expression && foo

If that value is constant, then there is some dead or useless code, which indicates an error.

The same code that already checks the first 5 cases would check the left of ||/&&, so this seems like a logical and easy addition to the rule.

1. Consider any comparison binary expression (==, !==) to be a “condition”. This would warn: myTestFunc({} == false)

There is a similarity, but I think this doesn’t fit the “check the control flow condition” purpose of no-constant-condition, so this looks to me more like a separate rule.

1. Consider null conditions, not just boolean conditions ( {} ?? foo, !foo == null)

I’ll have to think some more about ??, but !foo == null also wouldn’t fit no-constant-condition in my opinion.

1. Follow identifiers to their assignment – if there is only one – to catch things like: const foo = {}; foo ? a : b

This can be a separate proposal for an enhancement in no-constant-condition, and also something to consider for new rules.

For example, is it safe for rules to assume that methods like Object.prototype.valueOf and Object.prototype.toString have not been overwritten?

Yes, we can assume that all built-in objects from the specification (Object, Object.prototype, etc.) have not been overwritten and that they are exactly the same as defined in the specification.

This assumption would be required in order to assert that {someKey: someValue} == false is constant. It seems like a reasonable assumption to make to me but I just wanted to double check.

If we are sure that the object literal doesn’t include either of "valueOf", "toString" or non-computed & non-shorthand "__proto__", then we can assume that this expression is constant since it always evaluates to false. Otherwise, it might be true as well.

const getZero = () => 0;
const __proto__ = { valueOf: getZero };

({ valueOf: getZero } == false); // true

({ toString: getZero } == false); // true

({ __proto__: __proto__ } == false); // true

Secondly, is it safe for us to assume that globals like Boolean and String have not been globally redefined?

Yes, if Boolean is a reference to global variable, then we can assume that it is the original Boolean. Same for all other globals defined in the specification.

We discussed this in today’s TSC meeting and want to add the rule to ESLint! We decided to name it no-constant-binary-expression since the it’s the expression that’s always evaluating the same way in these cases. Marking as accepted.

TSC Summary: This proposal seeks to add three checks into no-constant-condition:

1. Consider short circuiting operations (|| , &&) to be conditions
2. Consider any comparison binary expression (==, !==) to be a “condition”. This would warn: myTestFunc({} == false)
3. Consider null conditions, not just boolean conditions ( {} ?? foo, !foo == null)

There is consensus that proposals 2 and 3 do not belong in no-constant-condition. @nzakas proposes that all three proposals be included in a new no-constant-binary-operand rule.

TSC Question: Shall we accept this as a new rule proposal for no-constant-binary-operand?

I’d like to make a different proposal. What if we added a new rule called no-constant-binary-operand? I think that better describes proposals 1-3 (and maybe 4?), which we can now group together in one rule, and we can leave no-constant-condition as-is.

@captbaritone Sorry, everyone is pretty strapped lately. I’ll make sure this gets discussed one way or another next week.

you don’t need to.

• 1 + null === 1
• 1 + undefined === NaN
• 1 + '' === '1'
• 1 + false === 1
• 1 + 1 === 2
• 1 + {} === '1[object Object]'
• etc

You know that if there is a + operator, the result will be a string, or a number. If there is a -///*, then the result will be a number.

Thus you know that these operators will never result in a nullish value. From that it follows that for 1 + nullOrNumber ?? 2, the nullish coalesce is never evaluated, as the + is evaluated first, resulting in a string/number - which is never nullish.