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

fix(eslint-plugin): [return-await] correct autofixer in binary expression #4401

Merged

Conversation

islandryu
Copy link
Contributor

@islandryu islandryu commented Jan 7, 2022

PR Checklist

Overview

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @islandryu!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@nx-cloud
Copy link

nx-cloud bot commented Jan 7, 2022

☁️ Nx Cloud Report

CI ran the following commands for commit dbf7e4a. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 48 targets

Sent with 💌 from NxCloud.

@netlify
Copy link

netlify bot commented Jan 7, 2022

✔️ Deploy Preview for typescript-eslint ready!

🔨 Explore the source changes: dbf7e4a

🔍 Inspect the deploy log: https://app.netlify.com/sites/typescript-eslint/deploys/61fbf2cedc89960007dc0c8a

😎 Browse the preview: https://deploy-preview-4401--typescript-eslint.netlify.app

@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #4401 (dbf7e4a) into main (63fbbaa) will increase coverage by 1.81%.
The diff coverage is 65.71%.

@@            Coverage Diff             @@
##             main    #4401      +/-   ##
==========================================
+ Coverage   92.49%   94.30%   +1.81%     
==========================================
  Files         346      148     -198     
  Lines       11684     7943    -3741     
  Branches     3335     2535     -800     
==========================================
- Hits        10807     7491    -3316     
+ Misses        608      255     -353     
+ Partials      269      197      -72     
Flag Coverage Δ
unittest 94.30% <65.71%> (+1.81%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...es/eslint-plugin/src/util/getOperatorPrecedence.ts 60.00% <60.00%> (ø)
packages/eslint-plugin/src/rules/return-await.ts 99.06% <100.00%> (+0.05%) ⬆️
packages/utils/src/ts-eslint/Scope.ts
packages/scope-manager/src/lib/es2021.promise.ts
packages/utils/src/eslint-utils/nullThrows.ts
...ckages/utils/src/ts-eslint-scope/PatternVisitor.ts
packages/type-utils/src/requiresQuoting.ts
packages/utils/src/ts-eslint-scope/Reference.ts
packages/typescript-estree/src/ts-estree/index.ts
.../scope-manager/src/variable/ImplicitLibVariable.ts
... and 191 more

@bradzacher bradzacher added the bug Something isn't working label Jan 11, 2022
@bradzacher bradzacher changed the title fix(eslint-plugin): return-await in binary expression fix(eslint-plugin): [return-await] correct autofixer in binary expression Jan 11, 2022
@islandryu islandryu force-pushed the fixReturnAwaitInBinaryExpression branch 2 times, most recently from def7c60 to 4477fb7 Compare January 14, 2022 13:08
@islandryu
Copy link
Contributor Author

I'm sorry.
I did a forcepush by mistake.
I didn't change anything.

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

This is definitely a good start - but the problem is that there are a lot of cases that it won't handle properly.

The problem we're trying to solve here is that of "precedence" - meaning we need to check if the thing on the right has a lower precedence than the await we're inserting. If it has a lower precedence, then it is evaluated after the await. If it has a higher precedence, then it is evaluated before the await.

Luckily this problem is already solved for us, to some degree:
https://github.com/microsoft/TypeScript/blob/af689cc5d527789cbc1d18a7b8aae6fa46051ea7/src/compiler/utilities.ts#L3446-L3783

All of this code is specifically built for TS nodes. Additionally it's all internal to TS (so we can't use it anyways). It would be a good idea for us to maintain a copy of this for ourselves using our AST_NODE_TYPES (and with a non-const enum). We can place this function in the utils package so anyone can consume it easily.


As an example - from the above code we can see that BinaryExpression has a precedence of OperatorPrecedence.Coalesce (or 4), whereas AwaitExpression has a precedence of OperatorPrecedence.Unary (or 16)

This means that AwaitExpression has a higher precedence than BinaryExpression - thus it is evaluated first.
So by evaluating the precedences, we know that before we insert an await we must wrap the child BinaryExpression in parentheses so that it is evaluated correctly.

This system handles all cases correctly and saves us having to manually handle individual cases.

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jan 17, 2022
@islandryu
Copy link
Contributor Author

islandryu commented Jan 18, 2022

@bradzacher
Thanks for the review.
Am I correct in my understanding?

// CallExpression has a higher precedence than awaitExpression, so () is not necessary.
// input
return a();

// expected output
return await a();


// All binaryExpression has a lower precedence than awaitExpression, so () is necessary.
// input
return a() || undefined
return undefined || a()
return undefined ?? a()

// expected output 
return await (a() || undefined)
return await (undefined || a())
return await (undefined ?? a())


// AsExpression has a lower precedence than awaitExpression, so () is necessary.
// input
return a() as T

// expected output
return await (a() as T)

.
.
.

@bradzacher
Copy link
Member

You are correct!
By inspecting the precedence we can determine whether or not we need parentheses at all.

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 19, 2022
@islandryu
Copy link
Contributor Author

I could not test all of getOperatorPrecedence.ts with return-await alone.

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

this is looking good to me - this is awesome!
thanks so much for improving the fixer here and adding utils!

@bradzacher bradzacher merged commit 5fa2fad into typescript-eslint:main Mar 1, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 8, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.13.0` -> `5.14.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.13.0/5.14.0) |
| [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.13.0` -> `5.14.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.13.0/5.14.0) |

---

### Release Notes

<details>
<summary>typescript-eslint/typescript-eslint (@&#8203;typescript-eslint/eslint-plugin)</summary>

### [`v5.14.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#&#8203;5140-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5130v5140-2022-03-07)

[Compare Source](typescript-eslint/typescript-eslint@v5.13.0...v5.14.0)

##### Bug Fixes

-   **eslint-plugin:** \[naming-convention] cover case that requires quotes ([#&#8203;4582](typescript-eslint/typescript-eslint#4582)) ([3ea0947](typescript-eslint/typescript-eslint@3ea0947))
-   **eslint-plugin:** \[no-misused-promises] factor thenable returning function overload signatures ([#&#8203;4620](typescript-eslint/typescript-eslint#4620)) ([56a09e9](typescript-eslint/typescript-eslint@56a09e9))
-   **eslint-plugin:** \[prefer-readonly-parameter-types] handle class sharp private field and member without throwing error ([#&#8203;4343](typescript-eslint/typescript-eslint#4343)) ([a65713a](typescript-eslint/typescript-eslint@a65713a))
-   **eslint-plugin:** \[return-await] correct autofixer in binary expression ([#&#8203;4401](typescript-eslint/typescript-eslint#4401)) ([5fa2fad](typescript-eslint/typescript-eslint@5fa2fad))

##### Features

-   **eslint-plugin:** \[no-misused-promises] add granular options within `checksVoidReturns` ([#&#8203;4623](typescript-eslint/typescript-eslint#4623)) ([1085177](typescript-eslint/typescript-eslint@1085177))

</details>

<details>
<summary>typescript-eslint/typescript-eslint (@&#8203;typescript-eslint/parser)</summary>

### [`v5.14.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#&#8203;5140-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5130v5140-2022-03-07)

[Compare Source](typescript-eslint/typescript-eslint@v5.13.0...v5.14.0)

**Note:** Version bump only for package [@&#8203;typescript-eslint/parser](https://github.com/typescript-eslint/parser)

</details>

---

### Configuration

📅 **Schedule**: 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 these updates 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).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1201
Reviewed-by: 6543 <6543@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[return-await] wrong autofix of return undefined ?? promise to return await undefined ?? promise
2 participants