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

feat: handle logical assignment in no-self-assign #14152

Merged
merged 3 commits into from Dec 30, 2021

Conversation

Zzzen
Copy link
Contributor

@Zzzen Zzzen commented Mar 1, 2021

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-self-assign

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 default behavior

Please provide some example code that this change will affect:

a &= a;  
a |= a;
a &&= a;
a ||= a;
a ??= a;

What does the rule currently do for this code?
no errors are reported

What will the rule do after it's changed?
all lines will be reported.

What changes did you make? (Give an overview)

currently, only = is handled in no-self-assign. But other logical assignment operators work roughly the same as =.

a &&= a is a && (a = a), which is essentially a noop.

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

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Mar 1, 2021
@mdjermanovic mdjermanovic added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion new syntax This issue is related to new syntax that has reached stage 4 rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Mar 1, 2021
@mdjermanovic
Copy link
Member

Hi @Zzzen, thanks for the PR!

We listed adding support for &&=, ||=, and ??= operators to the no-self-assign rule as maybe in #13569. We're not yet sure if that change makes sense, so I marked this as "evaluating".

As for &= and |=, I think that's different as it involves a calculation before the assignment, and it doesn't have to be a noop (should be for integers, but e.g, 1.5 | 1.5 is 1, null | null is 0, etc.), so those checks probably don't belong to no-self-assign.

@Zzzen
Copy link
Contributor Author

Zzzen commented Mar 2, 2021

Ooops, did not notice the issue. IMO, even though |= may have side effects on non-integers, a |= a is likely a typo and should be disallowed like how object properties are handled.

@mdjermanovic mdjermanovic mentioned this pull request Oct 1, 2021
12 tasks
@mdjermanovic mdjermanovic changed the title Fix: handle logical assignment in no-self-assign feat: handle logical assignment in no-self-assign Oct 26, 2021
@eslint-github-bot
Copy link

Hi @Zzzen!, 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

@github-actions
Copy link

Oops! It looks like we lost track of this pull request. What do we want to do here? This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Dec 25, 2021
@mdjermanovic mdjermanovic added this to Feedback Needed in Triage Dec 26, 2021
@mdjermanovic
Copy link
Member

@eslint/eslint-tsc thoughts about this proposal for the no-self-assign rule?

  • Should it report a &&= a, a ||= a, and a ??= a? I think this makes sense since these are essentially the same as a = a.
  • Should it report a &= a, and a |= a? I think this doesn't belong to this rule, for the reasons described in this comment.

@mdjermanovic mdjermanovic added enhancement This change enhances an existing feature of ESLint feature This change adds a new feature to ESLint labels Dec 26, 2021
@nzakas
Copy link
Member

nzakas commented Dec 28, 2021

@mdjermanovic i agree with your reasoning.

@btmills
Copy link
Member

btmills commented Dec 28, 2021

@mdjermanovic I agree as well.

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 29, 2021
@btmills btmills moved this from Feedback Needed to Pull Request Opened in Triage Dec 29, 2021
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

@Zzzen sorry for the delay, the change for &&=, ||=, and ??= is accepted now.

Can you also update the documentation for this rule?

lib/rules/no-self-assign.js Outdated Show resolved Hide resolved
tests/lib/rules/no-self-assign.js Outdated Show resolved Hide resolved
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 30, 2021

CLA Signed

The committers are authorized under a signed CLA.

@Zzzen
Copy link
Contributor Author

Zzzen commented Dec 30, 2021

👌 updated

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 6802a54 into eslint:main Dec 30, 2021
Triage automation moved this from Pull Request Opened to Complete Dec 30, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 29, 2022
@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 Jun 29, 2022
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 feature This change adds a new feature to ESLint new syntax This issue is related to new syntax that has reached stage 4 rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

Successfully merging this pull request may close these issues.

None yet

4 participants