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: add rule logical-assignment-operators #16102

Merged
merged 37 commits into from Sep 16, 2022

Conversation

DMartens
Copy link
Contributor

@DMartens DMartens commented Jul 5, 2022

Fixes #13689

Prerequisites checklist

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

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

Fixes #13689

What changes did you make? (Give an overview)

Added the rule according to the proposal
Compared to the proposal the if test condition also checks for !!value and Boolean(value) and the check if the problem is fixable also checks for with blocks.
I used the existing operator-shorthand as a template for the documentation and wording of the messages.

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

The most important thing is more fix scenarios. I tried to cover all I could think of (comments, parenthesis, whether it should be autofixed).
Also the logic of "isUndefined" is duplicated as ast-utils only has a "isNullOrUndefined", maybe this should be moved to ast-utils.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 5, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • login: DMartens / name: fnx (e7cb5b1)

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon feature This change adds a new feature to ESLint labels Jul 5, 2022
@netlify
Copy link

netlify bot commented Jul 5, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 180155d
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/631874f03d8b9400080c72e4

@mdjermanovic mdjermanovic added rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jul 5, 2022
Copy link
Member

@nzakas nzakas left a comment

Thanks for doing this! I left a few small comments but otherwise looks really good. Would like @mdjermanovic to review before merging.

docs/src/rules/logical-assignment-operators.md Outdated Show resolved Hide resolved
docs/src/rules/logical-assignment-operators.md Outdated Show resolved Hide resolved
docs/src/rules/logical-assignment-operators.md Outdated Show resolved Hide resolved
docs/src/rules/logical-assignment-operators.md Outdated Show resolved Hide resolved
docs/src/rules/logical-assignment-operators.md Outdated Show resolved Hide resolved
lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
lib/rules/logical-assignment-operators.js Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Very nice work! I left a few comments about things that should be fixed in the code that I noticed on the first pass.

lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
@DMartens
Copy link
Contributor Author

DMartens commented Jul 12, 2022

Thanks for the reviews. I pushed the suggested fixes / features as separate commits for easier reviewing.
Some notes:

  • Currently it is not possible to overwrite undefined and then use the assignment pattern (eg. undefined = undefined || 0)
  • I tried different situations but the continuation problem for the if pattern is only encountered when the previous statement ends with a closing parenthesis and the assignment target is parenthesized. So I targeted this specifically but there may be other contexts which could trigger the continuation problem
  • I chose to add parenthesis based on the precedence for the logical pattern instead of adding parenthesis when the logical expression is not an expression statement

lib/rules/index.js Outdated Show resolved Hide resolved
lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member

mdjermanovic commented Jul 16, 2022

@DMartens it seems that #16102 (comment) hasn't been addressed. For example, the following if can be safely autofixed to a.b &&= foo; but the current version provides a suggestion.

/* eslint logical-assignment-operators: ["error", "always", { enforceForIfStatements: true }] */

(a) => {
    if (a.b) a.b = foo;
}

@DMartens
Copy link
Contributor Author

DMartens commented Jul 18, 2022

@mdjermanovic I fixed this locally. But I have two questions regarding when to fix/suggest the fix:

  1. In the proposal the canBeFixed is linked which also allows fixing for a.b = a.b || c but according to the proposal this should be a suggestion.
  2. I think super and meta properties (eg. import.meta) are also save to fix (eg. import.meta.hot = import.meta.hot || false)

@mdjermanovic
Copy link
Member

mdjermanovic commented Jul 18, 2022

  1. In the proposal the canBeFixed is linked which also allows fixing for a.b = a.b || c but according to the proposal this should be a suggestion.

This is different from operator-assignment because logical assignment operators have short-circuiting behavior. In particular, if a.b is truthy, then a.b ||= c doesn't assign to a.b while a.b = a.b || c does assign to a.b, so they are not equivalent.

2. I think super and meta properties (eg. import.meta) are also save to fix (eg. import.meta.hot = import.meta.hot || false)

I agree, it seems that these cases can be safely autofixed.

@nzakas
Copy link
Member

nzakas commented Jul 30, 2022

@DMartens are you still working on this?

lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
@DMartens
Copy link
Contributor Author

DMartens commented Sep 6, 2022

Thanks for the thorough review. All the suggestions should be implemented as separate commits.

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

A few small suggestions, then LGTM.

tests/lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
tests/lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
tests/lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
lib/rules/logical-assignment-operators.js Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks! Great work.

I'll leave this open for a few days in case someone else wants to review it before merging.

Copy link
Contributor

@snitin315 snitin315 left a comment

Great work on this. Thank you for contributing

nzakas
nzakas approved these changes Sep 16, 2022
Copy link
Member

@nzakas nzakas left a comment

LGTM

@nzakas nzakas merged commit b0d72c9 into eslint:main Sep 16, 2022
18 checks passed
@amitbeck
Copy link

amitbeck commented Sep 17, 2022

This rule is a great addition, but it's a shame that it didn't end up being named logical-operator-assignment to align with operator-assignment, as suggested in #13689 (comment).

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 21, 2022

This rule is a great addition, but it's a shame that it didn't end up being named logical-operator-assignment to align with operator-assignment, as suggested in #13689 (comment).

@amitbeck thanks for the input! We considered your suggestion but decided on the logical-assignment-operators rule name because "logical assignment operators" is the term used in the proposal and the specification.

@amitbeck
Copy link

amitbeck commented Sep 21, 2022

This rule is a great addition, but it's a shame that it didn't end up being named logical-operator-assignment to align with operator-assignment, as suggested in #13689 (comment).

@amitbeck thanks for the input! We considered your suggestion but decided on the logical-assignment-operators rule name because "logical assignment operators" is the term used in the proposal and the specification.

@mdjermanovic Good to know, thanks for the clarification. It still bothers me that the other rule's name is misaligned with this one but nothing can truly be perfect 🥲

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Sep 27, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.23.1` -> `8.24.0`](https://renovatebot.com/diffs/npm/eslint/8.23.1/8.24.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.24.0`](https://github.com/eslint/eslint/releases/tag/v8.24.0)

[Compare Source](eslint/eslint@v8.23.1...v8.24.0)

#### Features

-   [`1729f9e`](eslint/eslint@1729f9e) feat: account for `sourceType: "commonjs"` in the strict rule ([#&#8203;16308](eslint/eslint#16308)) (Milos Djermanovic)
-   [`b0d72c9`](eslint/eslint@b0d72c9) feat: add rule logical-assignment-operators ([#&#8203;16102](eslint/eslint#16102)) (fnx)
-   [`f02bcd9`](eslint/eslint@f02bcd9) feat: `array-callback-return` support `findLast` and `findLastIndex` ([#&#8203;16314](eslint/eslint#16314)) (Sosuke Suzuki)

#### Documentation

-   [`2c152ff`](eslint/eslint@2c152ff) docs: note false positive `Object.getOwnPropertyNames` in prefer-reflect ([#&#8203;16317](eslint/eslint#16317)) (AnnAngela)
-   [`bf7bd88`](eslint/eslint@bf7bd88) docs: fix warn severity description for new config files ([#&#8203;16324](eslint/eslint#16324)) (Nitin Kumar)
-   [`8cc0bbe`](eslint/eslint@8cc0bbe) docs: use more clean link syntax ([#&#8203;16309](eslint/eslint#16309)) (Percy Ma)
-   [`6ba269e`](eslint/eslint@6ba269e) docs: fix typo ([#&#8203;16288](eslint/eslint#16288)) (jjangga0214)

#### Chores

-   [`131e646`](eslint/eslint@131e646) chore: Upgrade [@&#8203;humanwhocodes/config-array](https://github.com/humanwhocodes/config-array) for perf ([#&#8203;16339](eslint/eslint#16339)) (Nicholas C. Zakas)
-   [`504fe59`](eslint/eslint@504fe59) perf: switch from object spread to `Object.assign` when merging globals ([#&#8203;16311](eslint/eslint#16311)) (Milos Djermanovic)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMDIuMSIsInVwZGF0ZWRJblZlciI6IjMyLjIwMi4zIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1560
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@nzakas
Copy link
Member

nzakas commented Oct 6, 2022

@DMartens we'd like to pay you for this contribution, but I couldn't find an email address to contact you at. Can you please email contact (at) eslint (dot) org so we can give you instructions? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion contributor pool feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New rule: Prefer logical assignment operators
5 participants