[no-constant-condition] all literal should be treated like boolean literal

[Zzzen]

What rule do you want to change?
no-constant-condition

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

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

Please provide some example code that this change will affect:

// Unexpected constant condition. eslintno-constant-condition
if (true || this.loginpage) {
    console.log('1');
}

// no error reported
if (1 || this.loginpage) {
    console.log('1');
}

What does the rule currently do for this code?
Only the first if statement is reported

What will the rule do after it's changed?
The second if statement will be reported too.

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

[yeonjuan]

Online Demo

/*eslint no-constant-condition: "error"*/

if (this.loginpage ||true) {} // error
if (this.loginpage || 1) {} // error
if (this.loginpage || '1') {} // error

if (true || this.loginpage) {} // error

if (1 || this.loginpage) {} // no error
if ('1' || this.loginpage) {} // no error

Hi, @Zzzen. :). It seems a bug to me.
I think the truthy literal on the left side of || should be reported.

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

so, maybe, it should be fixed rather than adding new options?
Let’s hear from others.

[anikethsaha]

I agree with above comment.

if (1 || this.loginpage) {} // this should be an error cause its always truthy
if ('1' || this.loginpage) {} // this should be an error , always truthy

[Zzzen]

Because I'm not sure what's the rationale behind these lines. 😂

'str' || a should have been invalid.

eslint/tests/lib/rules/no-constant-condition.js

Line 76 in fdfa364

// #11181, string literals

"if('str' || a){}",
"if('str1' && a){}",
"if(a && 'str'){}",
"if('str' || abc==='str'){}",

[Zzzen]

Do we have any rules for if (false || a){}? It's likely a mistake ( just like an unused expression ), although not a constant condition.

[anikethsaha]

I think it should be reported as well with an unused condition as if (false || a){} is equal to if (a){}. It can be done in this rule itself instead of a separate rule.

[Zzzen]

It would be a big breaking change.

[mdjermanovic]

Do we have any rules for if (false || a){}? It's likely a mistake ( just like an unused expression ), although not a constant condition.

We don't have such a rule. It might be worth exploring whether an option in no-constant-condition to treat LHS of a logical operator as condition makes sense.

[mdjermanovic]

I believe the only reason to not report 1 || foo as a constant condition in if (1 || foo); would be to avoid overlapping with another rule (or an option in this rule) that would report 1 in 1 || foo.

In that case, true || foo in if (true || foo); shouldn't be reported as well.

foo || 1 is different because it can be a valid code, depending on the context.

Even if we add such a rule or option at some point, I don't think that the mentioned overlapping would be a big problem, so I'd vote to accept this issue as a bug and fix it in a semver-minor release.

In addition to ||, the actual behavior with && also seems to be inconsistent:

/* eslint no-constant-condition: error */

if (false && a); // error

if (void null && a); // error!

if (null && a); // no error

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.