feat: Add rule no-constant-binary-expression

[captbaritone]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[X] New rule (template)

Please describe what the rule should do:

What category of rule is this? (place an "X" next to just one item)

[X] Warns about a potential error

Provide 2-3 code examples that this rule will warn about:

value === (0 || undefined || null)

!value !== null

someFunction() != null ?? false

x + b ?? 0

Why should this rule be included in ESLint (instead of a plugin)?

Recommended by TSC in #13752.

What changes did you make? (Give an overview)

Adds a rule which is similar in nature to no-constant-condition in that it flags conditions have constant behavior at runtime, since that is highly correlated with developer error. Specifically it checks for:

• && and || logical expressions where the left hand side is either always truthy or always falsy
• ?? logical expressions where the left hand side is either always nullish or always non-nullish
• ==, ===, !== and != binary expressions where one side is either null, undefined, true or false, and the outcome of the comparison can be determined statically.

Is there anything you'd like reviewers to focus on?

The code ended up begin quite verbose with comments, which I personally found helpful during development, but may not be idiomatic in this code base. I'm happy to tone them down if desired.

At its heart this rule boils down to a "best effort". It understands many semantics of the JavaScript language, but it must draw the line somewhere, and those lines are necessarily somewhat arbitrary. I feel pretty good about the lines I ended up setting given the TSC Summary, but I'm open to feedback.

Impact and Representative Examples

I ran this rule on our large monorepo and it uncovered ~450 existing errors. Of those, about one third looked like changes which were intended to be temporary behavior changes, or hard coded feature flags:

const enableFeature = false && shouldEnableFeature();

Personally I think these would merit an /* eslint-disable-next-line */ comment to clarify the motivation, but I could also see us wanting to add an option to permit literal true or false on the left hand side of || or &&.

Below are some representative errors it uncovered, anonymized and reduced to their pure form:

value === (0 || undefined || null)

!value !== null

someFunction() != null ?? false

x + b ?? 0

String(someNumber) ?? 'DEFAULT_VALUE'

typeof someMemberExpression !== undefined

switch (status) {
  case 'A' || 'B' || 'C' || 'D':
    // some code
    break;
  // [...]
}

+value ?? 0

!!someArr.find(predicate) ?? false

Rule Name

Edit @mdjermanovic pointed out that the TSC had proposed a better name, but I missed the comment.

Is no-constant-binary-expression the right name for this rule given that it also applies to the left hand side of logical expressions? I'd rather not re-litigate, but my instinct tells me that the logical expression checks should live in no-constant-condition and this new rule should flag any comparison binary expression whose result is provably static.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Jordan Eldredge (7333916)

[eslint-github-bot]

Hi @captbaritone!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
• There should be a space following the initial tag and colon, for example 'feat: Message'.
• The first letter of the tag should be in lowercase

Read more about contributing to ESLint here

[j-f1]

Is no-constant-binary-operand the right name for this rule given that it also applies to the left hand side of logical expressions?

When first reading through this PR, I thought this rule prohibited constant operands in binary expressions (such as the 1 in x + 1). I assume at least some other people will feel the same way and may skip over it in a list because of that interpretation.

[captbaritone]

I believe all previous feedback has been addressed.

[Swapnull]

Hey, I hope this is ok, but I just noticed you are spelling always and short-circuiting wrong in a few comments. It seems to be fairly consistent, so a find a replace should sort it :)

[mdjermanovic]

Is no-constant-binary-operand the right name for this rule given that it also applies to the left hand side of logical expressions?

Per #13752 (comment), the name should be no-constant-binary-expression.

[captbaritone]

the name should be no-constant-binary-expression

Ah thanks. Will fix.

Edit I've updated the PR, commit and PR description to use the name no-constant-binary-expression. Sorry I missed that comment the first time 'round.

[captbaritone]

@mdjermanovic Thanks for the detailed review with examples of code that would behave give incorrect results. Made it super easy to address the feedback and add the missing tests cases.

Updated and ready for another round.

[captbaritone]

@mdjermanovic Again, great feedback thanks! Ready for another pass.

[captbaritone]

@mdjermanovic Feedback addressed.

[mdjermanovic]

Just one small typo to fix, and then LGTM.

[captbaritone]

@mdjermanovic Fixed

[mdjermanovic]

Great work, thanks!

I'll leave this open for @nzakas to confirm his feedback has been addressed.

[nzakas]

LGTM. I just left a few suggestions for cleaning up the docs and JSDoc.

[captbaritone]

@nzakas Thanks! I've merged committed them all.

[mdjermanovic]

I'll update the doc file for #15782 in a follow-up PR.

[mdjermanovic]

@captbaritone thanks for contributing!