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

[captbaritone]

What rule do you want to change?

no-constant-condition

Does this change cause the rule to produce more or fewer warnings?

More

How will the change be implemented? (New option, new default behavior, etc.)?

New default behavior

Please provide some example code that this change will affect:

(a + b ?? c)

(!foo == null)

(!foo ?? bar)

((a + b) / 2 ?? bar)

const bar = String(foo); (bar == null)

(String(foo.bar) ?? baz)

const bar = <div>Hi!</div>; (bar == null)

(<div>Hi!</div> ?? foo)

("hello" + name ?? "")

([foo?.bar ?? ""] ?? [])

What does the rule currently do for this code?

Nothing

What will the rule do after it's changed?

Warn/Error

Are you willing to submit a pull request to implement this change?

Possibly

No Useless Null Checks

There are some types of comparisons which we can tell statically will always create the same result. One such example is performing a null check on an ObjectExpression. In many cases, these type of coding errors are harmless since they merely guard against edge cases which can't actually exist. However, in other cases they represent a misunderstanding of how the code will behave. For example:

• a + b ?? c Misunderstanding of operator precedence.
• <SomeComponent /> ?? <Fallback /> Incorrect assumption about JSX returning the result of render function

no-constant-condition guards against many of these types of errors but it does not currently try to understand comparisons that are constant due to the statically knowable type of the values being compared.

My question is: should it?

On one hand this seems like these are constant conditions that ESLint can determine, so it should! On the other hand, if ESLint wanted to report all possible constant conditions of this sort, it would end up implementing a type system, which is clearly out of scope for this project.

Speaking of type systems, it's worth noting that many of these errors, type systems (Flow and TypeScript) do not currently report as errors.

Faced with the question of "how far should a rule like this go?" I decided to pick an arbitrary subset of "constant comparisons to null" and write a rule specifically to check for that. You can find it here.

Surprisingly it found hundreds of errors in our (very large) code base, so I believe the rule has some non-trivial value.

I would be curious to hear any maintainer's thoughts on if this would be a good addition to no-constant-condition, of if it seems out of scope in some way.

If the rule that checks for constant null comparisons seems like a good addition, then it might be worth considering other similar checks that could be performed, such as constant boolean comparisons: {} || fallback.

Thanks to @bradzacher for some initial insight which lead to this rule.

[nzakas]

I’m not clear on what your exact proposal is here as not all of the examples in the code will result in null.

Can you describe more concisely what pattern(s) you are looking to add to this rule? Is it just == null? How does the ?? operator fit into this discussion?

[captbaritone]

Thanks for taking a look, and my apologies that the explanation was not clear. The proposal is to add additional logic to the no-constant-condition rule such that it can detect additional cases where a condition's outcome can be statically determined.

As a starting place, I propose that we could detect when the outcome of a comparison to null (either explicitly with == and friends, or implicitly with the ?? operator) cannot vary at runtime due to the statically knowable type of the value being compared to null.

I'll break down a few of the examples and hopefully that can help clarify as well:

• (a + b ?? c)
  • A BinaryExpression with a + operator will always result in either a number or a string. It can never be null, so the ?? c could be removed without affecting the runtime behavior of the program.
• (!foo == null)
  • A UnaryExpression with a ! operator will always result in a boolean. It can never be null, so the whole expression could be replaced with false without affecting the runtime behavior of the program.
• void foo == null (new example)
  • A UnaryExpression with a void operator will always result in a undefined. It will always be loosely equal to null, so the whole expression could be replaced with true without affecting the runtime behavior of the program.

[bradzacher]

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.

[ljharb]

How can you know that nullOrNumber in fact returns null or a number, without type information?

[bradzacher]

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.

[ljharb]

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

[mdjermanovic]

{} || fallback

I think we could say that the left operand of || or && represents a condition, and just run the same isConstant on node.left of a LogicalExpression.

It could be behind an option, and that option can apply to all || and && expressions regardless of the context (not just in test conditions, e.g. const foo = {} || fallback; is also an error).

a + b ?? c

I support this, just not sure would it be better to make a separate rule that could check all ?? and other comparisons to nullish values regardless of the context.

<SomeComponent /> ?? <Fallback />

Per our policy around JSX, ESLint core doesn't assume any specific semantics for JSX. It's a React-specific fact that <SomeComponent /> cannot be a nullish value, so this wouldn't qualify. It could be a rule in eslint-plugin-react.

[captbaritone]

@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 Follow identifiers to their declaration in no-constant-condition #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.

[nzakas]

Thanks for the thoughtful discussion and extra explanations — this has helped a lot.

I agree that the proposals would provide a lot of value and I think it’s worth exploring the right way to consider them in ESLint.

That said, I think we are trying to stretch the meaning of “condition” a bit too far. The point of the no-constant-condition rule is to watch for cases when a Boolean value in a condition position will never change. I’m having a hard time reconciling that definition with Boolean or equality operators.

Maybe this functionality is better for no-unused-expressions (https://eslint.org/docs/rules/no-unused-expressions)?

[captbaritone]

From my perspective proposal no. 1 fits within no-constant-condition. The short-circuiting nature of logical operators means that the control flow of the program changes "conditionally" based upon the boolean value of left hand side. Ignoring side effects, a || b is equivalent to a ? a : b, and we consider ternary operators a "condition".

If you agree that proposal no. 1 fits inside no-constant-condition, then I think the null coalescing operator (??) would belong there as well since it's also includes a condition that affect control flow. I listed this as part of proposal no. 3 but I think you could pull just ?? into no. 1 if you want.

Proposal no. 4 catches the exact same coding errors that no-constant-condition catches. The only difference is that we look are looking a bit harder. I think no. 4 would fit in no-constant-condition just fine.

That leaves proposal no. 2 (considering constant comparisons). I can see the argument for that belonging outside of no-constant-condition. However, I see no-unused-expression as a rule that tries to detect code which is vestigial or not "under load". Basically, code which has no affect on the behavior of the program. To me this is subtly different than code which has constant affect on the behavior of the program. In no-unused-expression the code could generally be removed without changing the semantics of the program. With the code detected by proposal no. 2 the offending code would need to be replaced by a constant in order to preserve the semantics of the program. In this way, the errors are similar to those surfaces by no-constant-condition.

I think the core maintainers of ESLint are likely best qualified to make the call of where these proposals belong, so I'll gladly defer to you, but these are my thoughts on the matter.

[nzakas]

Let me take a concrete example here:

({}) || fallback;
({}) && fallback

These are both already flagged by no-unused-expressions. I'm guessing the concern is that fallback is currently being flagged by no-unused-expressions, but the desire is to have ({}) flagged?

Just to reiterate: I'm not in favor of adding the proposals to no-constant-condition, but I'm not opposed to finding another home for these checks. We just need to do the difficult work of figuring out where patterns are already being flagged and making sure we're not flagging the same things in two different rules.

[captbaritone]

Looking at your examples, they are being flagged by no-unused-expressions only because the result is not assigned to anything. It notices that the entire expression is unused, which has nothing to do with constant truthiness of {}.

For example, this would not be flagged by no-unused-expression:

var x = ({}) && foo;

Update:

With proposal no. 1 the ObjectExpression would be flagged since it will always be truthy which renders the && useless.

[mdjermanovic]

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.

2. 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.

3. 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.

4. 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.

[captbaritone]

Unless anyone objects in the next day or so, I'll split no. 4 out into its own issue. I think that will simplify this discussion.

Update: I've filed the new issue here: #13776

@mdjermanovic thanks for offering to take the lead on no. 1, just let me know if there's anything I can/should do to continue to advocate for its inclusion.

[nzakas]

I still think it's a stretch to say that short-circuiting logical operators should be considered in the same way that control flow statements are, but I'd like to hear from the rest of the team on that point.

Otherwise, I agree with @mdjermanovic's comments about the other proposals. I do think we probably need to figure out if there are any existing rules where it makes sense to add ?? checks or if it's better to create a new rule.

[captbaritone]

Sounds like the next step is to build consensus among the team as to whether short-circuiting logical operators should be considered control flow statements.

Maybe we need to tag a few people?

[nzakas]

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.

[btmills]

👍 to a new no-constant-binary-operand rule. The data from Facebook's codebase indicates it would likely be generally useful. While it might have some logic (particularly #13776) in common with no-constant-condition, a separate no-constant-binary-operand rule would leave each rule with a distinct purpose.

[nzakas]

@mdjermanovic what do you think about my suggestion for a no-constant-binary-operand rule?

[nzakas]

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?

[btmills]

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.

[captbaritone]

PR is up #15296