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: treat unknown nodes as having the lowest precedence #17302

Merged
merged 5 commits into from Jun 28, 2023

Conversation

bradzacher
Copy link
Contributor

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?

@bradzacher bradzacher requested a review from a team as a code owner June 22, 2023 05:58
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Jun 22, 2023
@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jun 22, 2023
@mdjermanovic
Copy link
Member

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:

const requiresOuterParenthesis = logical.parent.type !== "ExpressionStatement" &&
(astUtils.getPrecedence({ type: "AssignmentExpression" }) < astUtils.getPrecedence(logical.parent));

astUtils.getPrecedence(parent) >= PRECEDENCE_OF_EXPONENTIATION_EXPR &&

@mdjermanovic mdjermanovic added the rule Relates to ESLint's core rules label Jun 22, 2023
@bradzacher
Copy link
Contributor Author

bradzacher commented Jun 24, 2023

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

For the logical-assignment-operators case....

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 d as number doesn't need parentheses! If it needed to be wrapped then the expression wouldn't have ever been parsed as an assignment expression and thus the rule wouldn't have fired on the code.

EG annotated the rule will only match on this case if it's:

a.b.c || (a.b.c = d as number)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LogicalExpression
          ^^^^^^^^^^^^^^^^^^^  AssignmentExpression
                  ^^^^^^^^^^^  unknown node

If intstead it was parsed as

a.b.c || (a.b.c = d as number)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ LogicalExpression
          ^^^^^^^^^^^^^^^^^^^  unknown node
          ^^^^^^^^^            AssignmentExpression

Then the rule wouldn't ever fire because the selector specifically looks for right.type = AssignmentExpression.

Which is why I didn't touch the logic here - because it's already working correctly!

I.e. for a.b.c || (a.b.c = d as number) the suggestion is a.b.c ||= d as number
and for a.b.c || (a.b.c = (d as number)) the suggestion is a.b.c ||= (d as number)
Both of which are syntactically correct.


For the prefer-exponentiation-operator case - yup this was an unhandled case - fixed.

@mdjermanovic
Copy link
Member

In order for you to hit this case you need code like this:

a.b.c || (a.b.c = d as number)

The code I linked checks logical expression's parent to determine whether the resulting assignment expression (||=) should be parenthesized in relation to that node. For example, let's say there's an unknown expression type SomethingExpression with operator something that happens to have precedence lower than || but higher than ||=.

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 

@bradzacher
Copy link
Contributor Author

bradzacher commented Jun 26, 2023

I see what you're saying - I just don't have an example of any code where the code is parsed like that.
The only examples I have parse as

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.
the only syntax with lower precedence than logicals are: spreads, yield, ternaries, assignments
the only syntax with lower precedence than assignments are: spreads, yield

So the only way I could create a test case for your example is to fabricate a hypothetical AST.
Happy to change the code (latest commit does) - I just don't have a test for it.

@mdjermanovic
Copy link
Member

So the only way I could create a test case for your example is to fabricate a hypothetical AST.
Happy to change the code (latest commit does) - I just don't have a test for it.

In that case, there's no need for a test case for this example. The code change LGTM.

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.

Just a small change to sync code with ast in one example, then LGTM.

tests/lib/rules/no-extra-boolean-cast.js Outdated Show resolved Hide resolved
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. Just waiting for @mdjermanovic to verify his changes.

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
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 ef6e24e into eslint:main Jun 28, 2023
17 checks passed
@bradzacher bradzacher deleted the low-precedence-for-unknown-nodes branch June 30, 2023 03:14
json-derulo added a commit to json-derulo/angular-ecmascript-intl that referenced this pull request Jul 4, 2023
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
(#&#8203;1[eslint/eslint#17328))
(Milos Djermanovic)
-
[`4c50400`](https://togithub.com/eslint/eslint/commit/4c5040022639ae804c15b366afc6e64982bd8ae3)
feat: add `ecmaVersion: 2024`, regexp `v` flag parsing
(#&#8203;1[eslint/eslint#17324))
(Milos Djermanovic)
-
[`4d411e4`](https://togithub.com/eslint/eslint/commit/4d411e4c7063274d6d346f1b7ee46f7575d0bbd2)
feat: add ternaryOperandBinaryExpressions option to no-extra-parens rule
(#&#8203;1[eslint/eslint#17270))
(Percy Ma)
-
[`c8b1f4d`](https://togithub.com/eslint/eslint/commit/c8b1f4d61a256727755d561bf53f889b6cd712e0)
feat: Move `parserServices` to `SourceCode`
(#&#8203;1[eslint/eslint#17311))
(Milos Djermanovic)
-
[`ef6e24e`](https://togithub.com/eslint/eslint/commit/ef6e24e42670f321d996948623846d9caaedac99)
feat: treat unknown nodes as having the lowest precedence
(#&#8203;1[eslint/eslint#17302))
(Brad Zacher)
-
[`1866e1d`](https://togithub.com/eslint/eslint/commit/1866e1df6175e4ba0ae4a0d88dc3c956bb310035)
feat: allow flat config files to export a Promise
(#&#8203;1[eslint/eslint#17301))
(Milos Djermanovic)

#### Bug Fixes

-
[`a36bcb6`](https://togithub.com/eslint/eslint/commit/a36bcb67f26be42c794797d0cc9948b9cfd4ff71)
fix: no-unused-vars false positive with logical assignment operators
(#&#8203;1[eslint/eslint#17320))
(Gweesin Chan)
-
[`7620b89`](https://togithub.com/eslint/eslint/commit/7620b891e81c234f30f9dbcceb64a05fd0dde65e)
fix: Remove `no-unused-labels` autofix before potential directives
(#&#8203;1[eslint/eslint#17314))
(Francesco Trotta)
-
[`391ed38`](https://togithub.com/eslint/eslint/commit/391ed38b09bd1a3abe85db65b8fcda980ab3d6f4)
fix: Remove `no-extra-semi` autofix before potential directives
(#&#8203;1[eslint/eslint#17297))
(Francesco Trotta)

#### Documentation

-
[`526e911`](https://togithub.com/eslint/eslint/commit/526e91106e6fe101578e9478a9d7f4844d4f72ac)
docs: resubmit pr 17115 doc changes
(#&#8203;1[eslint/eslint#17291))
(唯然)
-
[`e1314bf`](https://togithub.com/eslint/eslint/commit/e1314bf85a52bb0d05b1c9ca3b4c1732bae22172)
docs: Integration section and tutorial
(#&#8203;1[eslint/eslint#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
[@&#8203;eslint/js](https://togithub.com/eslint/js)[@&#8203;8](https://togithub.com/8).44.0
(#&#8203;1[eslint/eslint#17329))
(Milos Djermanovic)
-
[`a1cb642`](https://togithub.com/eslint/eslint/commit/a1cb6421f9d185901cd99e5f696e912226ef6632)
chore: package.json update for
[@&#8203;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
(#&#8203;1[eslint/eslint#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
(#&#8203;1[eslint/eslint#17323))
(Ziyad El Abid)
-
[`cf88439`](https://togithub.com/eslint/eslint/commit/cf884390ad8071d88eae05df9321100f1770363d)
chore: upgrade optionator@0.9.3
(#&#8203;1[eslint/eslint#17319))
(Milos Djermanovic)
-
[`9718a97`](https://togithub.com/eslint/eslint/commit/9718a9781d69d2c40b68c631aed97700b32c0082)
refactor: remove unnecessary code in `flat-eslint.js`
(#&#8203;1[eslint/eslint#17308))
(Milos Djermanovic)
-
[`f82e56e`](https://togithub.com/eslint/eslint/commit/f82e56e9acfb9562ece76441472d5657d7d5e296)
perf: various performance improvements
(#&#8203;1[eslint/eslint#17135))
(moonlightaria)
-
[`da81e66`](https://togithub.com/eslint/eslint/commit/da81e66e22b4f3d3fe292cf70c388753304deaad)
chore: update eslint-plugin-jsdoc to 46.2.5
(#&#8203;1[eslint/eslint#17245))
(唯然)
-
[`b991640`](https://togithub.com/eslint/eslint/commit/b991640176d5dce4750f7cc71c56cd6f284c882f)
chore: switch eslint-config-eslint to the flat format
(#&#8203;1[eslint/eslint#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>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 26, 2023
@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 Dec 26, 2023
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 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.

Rule Change: [no-extra-parens] assume unknown node types need to be parenthesised if they are parenthesised
3 participants