Update: check logical assignment in no-constant-condition

[mdjermanovic]

Prerequisites checklist

• I have read the contributing guidelines.

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

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

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:

if (a ||= true);

if (a &&= false);

if (a ||= b || true);

if (a && (b &&= false));

if (!(a ||= 'foo') === true);

if (!(a ||= 'foo') === false);

What does the rule currently do for this code?

no errors

What will the rule do after it's changed?

all conditions will be reported

What changes did you make? (Give an overview)

Added logic for logical assignments.

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

This wasn't included in #13618 as it was waiting for refactoring and bug fixes from #13245. Can we treat this as a semver-minor bug fix?

[btmills]

It's interesting that this rule doesn't currently check non-logical assignment expressions. For example, if (a |= true) in one of your tests is a constant condition, but the rule currently allows it. Had the rule already checked non-logical assignment expressions, I'd say yes, this is a semver-minor change so that this rule supports a new language feature. But since we're not currently checking any assignment expressions, it seems inconsistent to start checking logical assignments in a semver-minor change when adding checks for the existing non-logical assignment operators would be a semver-major change or behind an option.

Should this instead add a checkAssignments option that checks all assignment expressions, logical and non-logical? We could enable it by default in the next major.

[mdjermanovic]

It does check the direct = and other expressions that evaluate to one of the operands, like logical and sequence. It doesn't have the logic for specific arithmetic and bitwise binary operators like | yet, so it skips compound assignments.

For example, this is nearly the same as if (a ||= true);:

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

if (a = a || true); // error

demo

It indeed doesn't account for = and others in isLogicalIdentity, so if (a || (a = true));, which would be fully equivalent, isn't currently an error.

Maybe we could remove this, but leave the rest?

[nzakas]

Can you also update the examples in the documentation?

[mdjermanovic]

Can you also update the examples in the documentation?

Added examples for &&= and ||=.

[nzakas]

LGTM

[btmills]

LGTM

[btmills]

To resolve my question above, we decided in yesterday's TSC meeting to update our semantic versioning policy to allow adding new rule checks for new language features in semver-minor features. That's being done in #13970, and this is semver-minor per that policy. We're not including operators like |= because &&= and ||= are logically more similar to =, &&, and ||, which the rule already handles, than |=, for which the rule doesn't have any logic.