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 label Mar 1, 2021
@mdjermanovic mdjermanovic added evaluating new syntax rule and removed triage labels Mar 1, 2021
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Mar 1, 2021

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

@eslint-github-bot eslint-github-bot bot commented Oct 26, 2021

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

@github-actions github-actions bot commented Dec 25, 2021

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

@mdjermanovic mdjermanovic commented Dec 26, 2021

@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 feature labels Dec 26, 2021
@nzakas
Copy link
Member

@nzakas nzakas commented Dec 28, 2021

@mdjermanovic i agree with your reasoning.

@btmills
Copy link
Member

@btmills btmills commented Dec 28, 2021

@mdjermanovic I agree as well.

@nzakas nzakas added accepted and removed enhancement evaluating 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

@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 linux-foundation-easycla bot commented Dec 30, 2021

CLA Signed

The committers are authorized under a signed CLA.

@Zzzen
Copy link
Contributor Author

@Zzzen Zzzen commented Dec 30, 2021

👌 updated

Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 6802a54 into eslint:main Dec 30, 2021
14 checks passed
Triage automation moved this from Pull Request Opened to Complete Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted feature new syntax rule
Projects
Triage
Complete
Development

Successfully merging this pull request may close these issues.

None yet

4 participants