-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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: treat unknown nodes as having the lowest precedence #17302
feat: treat unknown nodes as having the lowest precedence #17302
Conversation
I think we should keep the current behavior (assuming that unknown nodes have higher precedence and thus require parentheses around known nodes) in the following cases: eslint/lib/rules/logical-assignment-operators.js Lines 373 to 374 in f82e56e
|
For the I looked at this case deeply and I realised that it is impossible to hit for unknown nodes! In order for you to hit this case you need code like this: a.b.c || (a.b.c = d as number) But syntactically we already know that EG annotated the rule will only match on this case if it's:
If intstead it was parsed as
Then the rule wouldn't ever fire because the selector specifically looks for Which is why I didn't touch the logic here - because it's already working correctly! I.e. for For the |
tests/fixtures/parsers/typescript-parsers/boolean-cast-with-assertion.js
Outdated
Show resolved
Hide resolved
tests/fixtures/parsers/typescript-parsers/logical-assignment-with-assertion.js
Outdated
Show resolved
Hide resolved
tests/fixtures/parsers/typescript-parsers/logical-assignment-with-assertion.js
Outdated
Show resolved
Hide resolved
tests/fixtures/parsers/typescript-parsers/logical-assignment-with-assertion.js
Outdated
Show resolved
Hide resolved
tests/fixtures/parsers/typescript-parsers/member-call-expr-with-assertion.js
Outdated
Show resolved
Hide resolved
tests/fixtures/parsers/typescript-parsers/member-call-expr-with-assertion.js
Outdated
Show resolved
Hide resolved
The code I linked checks logical expression's parent to determine whether the resulting assignment expression ( a.b.c || (a.b.c = d) something number
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
SomethingExpression Fix without parentheses would be wrong: a.b.c ||= d something number
^^^^^^^^^^^^^^^^^^
SomethingExpression The correct fix should be: (a.b.c ||= d) something number
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
SomethingExpression |
I see what you're saying - I just don't have an example of any code where the code is parsed like that. a.b.c || (a.b.c = d) as number
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LogicalExpression
^^^^^^^^^^^^^^^^^^^^^ unknown node Which means to get this code to parse in the order you want you need parentheses (a.b.c || (a.b.c = d)) as number
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unknown node
^^^^^^^^^^^^^^^^^^^^^^ LogicalExpression Which then degenerates to a plain old, non special syntax case and is fixed correctly to (a.b.c ||= d) as number I reviewed the precedence of all TS syntax and all of it has a higher precedence than both assignments and logicals. So the only way I could create a test case for your example is to fabricate a hypothetical AST. |
In that case, there's no need for a test case for this example. The code change LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small change to sync code with ast in one example, then LGTM.
tests/fixtures/parsers/typescript-parsers/boolean-cast-with-assertion.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just waiting for @mdjermanovic to verify his changes.
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://togithub.com/eslint/eslint)) | devDependencies | minor | [`8.43.0` -> `8.44.0`](https://renovatebot.com/diffs/npm/eslint/8.43.0/8.44.0) | --- ### Release Notes <details> <summary>eslint/eslint (eslint)</summary> ### [`v8.44.0`](https://togithub.com/eslint/eslint/releases/tag/v8.44.0) [Compare Source](https://togithub.com/eslint/eslint/compare/v8.43.0...v8.44.0) #### Features - [`1766771`](https://togithub.com/eslint/eslint/commit/176677180a4a1209fc192771521c9192e1f67578) feat: add `es2023` and `es2024` environments (#​1[https://github.com/eslint/eslint/issues/17328](https://togithub.com/eslint/eslint/issues/17328)/17328)) (Milos Djermanovic) - [`4c50400`](https://togithub.com/eslint/eslint/commit/4c5040022639ae804c15b366afc6e64982bd8ae3) feat: add `ecmaVersion: 2024`, regexp `v` flag parsing (#​1[https://github.com/eslint/eslint/issues/17324](https://togithub.com/eslint/eslint/issues/17324)/17324)) (Milos Djermanovic) - [`4d411e4`](https://togithub.com/eslint/eslint/commit/4d411e4c7063274d6d346f1b7ee46f7575d0bbd2) feat: add ternaryOperandBinaryExpressions option to no-extra-parens rule (#​1[https://github.com/eslint/eslint/issues/17270](https://togithub.com/eslint/eslint/issues/17270)/17270)) (Percy Ma) - [`c8b1f4d`](https://togithub.com/eslint/eslint/commit/c8b1f4d61a256727755d561bf53f889b6cd712e0) feat: Move `parserServices` to `SourceCode` (#​1[https://github.com/eslint/eslint/issues/17311](https://togithub.com/eslint/eslint/issues/17311)/17311)) (Milos Djermanovic) - [`ef6e24e`](https://togithub.com/eslint/eslint/commit/ef6e24e42670f321d996948623846d9caaedac99) feat: treat unknown nodes as having the lowest precedence (#​1[https://github.com/eslint/eslint/issues/17302](https://togithub.com/eslint/eslint/issues/17302)/17302)) (Brad Zacher) - [`1866e1d`](https://togithub.com/eslint/eslint/commit/1866e1df6175e4ba0ae4a0d88dc3c956bb310035) feat: allow flat config files to export a Promise (#​1[https://github.com/eslint/eslint/issues/17301](https://togithub.com/eslint/eslint/issues/17301)/17301)) (Milos Djermanovic) #### Bug Fixes - [`a36bcb6`](https://togithub.com/eslint/eslint/commit/a36bcb67f26be42c794797d0cc9948b9cfd4ff71) fix: no-unused-vars false positive with logical assignment operators (#​1[https://github.com/eslint/eslint/issues/17320](https://togithub.com/eslint/eslint/issues/17320)/17320)) (Gweesin Chan) - [`7620b89`](https://togithub.com/eslint/eslint/commit/7620b891e81c234f30f9dbcceb64a05fd0dde65e) fix: Remove `no-unused-labels` autofix before potential directives (#​1[https://github.com/eslint/eslint/issues/17314](https://togithub.com/eslint/eslint/issues/17314)/17314)) (Francesco Trotta) - [`391ed38`](https://togithub.com/eslint/eslint/commit/391ed38b09bd1a3abe85db65b8fcda980ab3d6f4) fix: Remove `no-extra-semi` autofix before potential directives (#​1[https://github.com/eslint/eslint/issues/17297](https://togithub.com/eslint/eslint/issues/17297)/17297)) (Francesco Trotta) #### Documentation - [`526e911`](https://togithub.com/eslint/eslint/commit/526e91106e6fe101578e9478a9d7f4844d4f72ac) docs: resubmit pr 17115 doc changes (#​1[https://github.com/eslint/eslint/issues/17291](https://togithub.com/eslint/eslint/issues/17291)/17291)) (唯然) - [`e1314bf`](https://togithub.com/eslint/eslint/commit/e1314bf85a52bb0d05b1c9ca3b4c1732bae22172) docs: Integration section and tutorial (#​1[https://github.com/eslint/eslint/issues/17132](https://togithub.com/eslint/eslint/issues/17132)/17132)) (Ben Perlmutter) - [`19a8c5d`](https://togithub.com/eslint/eslint/commit/19a8c5d84596a9f7f2aa428c1696ba86daf854e6) docs: Update README (GitHub Actions Bot) #### Chores - [`49e46ed`](https://togithub.com/eslint/eslint/commit/49e46edf3c8dc71d691a97fc33b63ed80ae0db0c) chore: upgrade [@​eslint/js](https://togithub.com/eslint/js)[@​8](https://togithub.com/8).44.0 (#​1[https://github.com/eslint/eslint/issues/17329](https://togithub.com/eslint/eslint/issues/17329)/17329)) (Milos Djermanovic) - [`a1cb642`](https://togithub.com/eslint/eslint/commit/a1cb6421f9d185901cd99e5f696e912226ef6632) chore: package.json update for [@​eslint/js](https://togithub.com/eslint/js) release (ESLint Jenkins) - [`840a264`](https://togithub.com/eslint/eslint/commit/840a26462bbf6c27c52c01b85ee2018062157951) test: More test cases for no-case-declarations (#​1[https://github.com/eslint/eslint/issues/17315](https://togithub.com/eslint/eslint/issues/17315)/17315)) (Elian Cordoba) - [`e6e74f9`](https://togithub.com/eslint/eslint/commit/e6e74f9eef0448129dd4775628aba554a2d8c8c9) chore: package.json update for eslint-config-eslint release (ESLint Jenkins) - [`eb3d794`](https://togithub.com/eslint/eslint/commit/eb3d7946e1e9f70254008744dba2397aaa730114) chore: upgrade semver@7.5.3 (#​1[https://github.com/eslint/eslint/issues/17323](https://togithub.com/eslint/eslint/issues/17323)/17323)) (Ziyad El Abid) - [`cf88439`](https://togithub.com/eslint/eslint/commit/cf884390ad8071d88eae05df9321100f1770363d) chore: upgrade optionator@0.9.3 (#​1[https://github.com/eslint/eslint/issues/17319](https://togithub.com/eslint/eslint/issues/17319)/17319)) (Milos Djermanovic) - [`9718a97`](https://togithub.com/eslint/eslint/commit/9718a9781d69d2c40b68c631aed97700b32c0082) refactor: remove unnecessary code in `flat-eslint.js` (#​1[https://github.com/eslint/eslint/issues/17308](https://togithub.com/eslint/eslint/issues/17308)/17308)) (Milos Djermanovic) - [`f82e56e`](https://togithub.com/eslint/eslint/commit/f82e56e9acfb9562ece76441472d5657d7d5e296) perf: various performance improvements (#​1[https://github.com/eslint/eslint/issues/17135](https://togithub.com/eslint/eslint/issues/17135)/17135)) (moonlightaria) - [`da81e66`](https://togithub.com/eslint/eslint/commit/da81e66e22b4f3d3fe292cf70c388753304deaad) chore: update eslint-plugin-jsdoc to 46.2.5 (#​1[https://github.com/eslint/eslint/issues/17245](https://togithub.com/eslint/eslint/issues/17245)/17245)) (唯然) - [`b991640`](https://togithub.com/eslint/eslint/commit/b991640176d5dce4750f7cc71c56cd6f284c882f) chore: switch eslint-config-eslint to the flat format (#​1[https://github.com/eslint/eslint/issues/17247](https://togithub.com/eslint/eslint/issues/17247)/17247)) (唯然) </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, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xNTUuMCIsInVwZGF0ZWRJblZlciI6IjM1LjE1OS4zIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: Renovate Bot <bot@renovateapp.com>
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[x] Changes an existing rule (template)
#17173
What changes did you make? (Give an overview)
This PR changes the
getPrecedence
util so that it assumes all unknown nodes have the lowest possible precedence.This means that lint rules will assume the unknown nodes require parentheses when evaluating or creating fixes.
I have included some test cases for TS code to validate the behaviour works as intended.
Is there anything you'd like reviewers to focus on?