Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update: check logical assignment in no-constant-condition #13946

Merged
merged 2 commits into from Jan 2, 2021

Conversation

mdjermanovic
Copy link
Member

Prerequisites checklist

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?

@mdjermanovic mdjermanovic added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 21, 2020
@mdjermanovic mdjermanovic mentioned this pull request Dec 21, 2020
12 tasks
@btmills
Copy link
Member

btmills commented Dec 28, 2020

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
Copy link
Member Author

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?

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also update the examples in the documentation?

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 31, 2020
@mdjermanovic
Copy link
Member Author

Can you also update the examples in the documentation?

Added examples for &&= and ||=.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@btmills
Copy link
Member

btmills commented Jan 1, 2021

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.

@mdjermanovic mdjermanovic merged commit e128e77 into master Jan 2, 2021
@mdjermanovic mdjermanovic deleted the logicalassignment-noconstantcondition branch January 2, 2021 02:12
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jul 2, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants