typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star15.7k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

bradzacher merged 5 commits intotypescript-eslint:mainfromislandryu:fixReturnAwaitInBinaryExpression

Mar 1, 2022

Merged

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

bradzacher merged 5 commits intotypescript-eslint:mainfromislandryu:fixReturnAwaitInBinaryExpression

Mar 1, 2022

Conversation

Copy link

Contributor

islandryu commentedJan 7, 2022•
edited
Loading

PR Checklist

Addresses an existing issue:fixes[return-await] wrong autofix ofreturn undefined ?? promise toreturn await undefined ?? promise #4090
That issue was marked asaccepting prs
Steps inCONTRIBUTING.md were taken

Overview

fix(eslint-plugin): return-await in binary expression

4477fb7

Copy link

Contributor

typescript-eslintbot commentedJan 7, 2022

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 visitorsper day.

Copy link

nx-cloudbot commentedJan 7, 2022•
edited
Loading

☁️ Nx Cloud Report

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

📂 See all runs for this branch

✅ Successfully ran 48 targets

Sent with 💌 fromNxCloud.

Copy link

netlifybot commentedJan 7, 2022•
edited
Loading

✔️ Deploy Preview fortypescript-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

Copy link

codecovbot commentedJan 7, 2022•
edited
Loading

Codecov Report

Merging#4401 (dbf7e4a) intomain (63fbbaa) willincrease coverage by1.81%.
The diff coverage is65.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
... and191 more

bradzacher added the bugSomething isn't working label

Jan 11, 2022

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 force-pushed thefixReturnAwaitInBinaryExpression branch 2 times, most recently fromdef7c60 to4477fb7Compare

January 14, 2022 13:08

Copy link

ContributorAuthor

islandryu commentedJan 14, 2022

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

bradzacher requested changes

Jan 17, 2022

View reviewed changes

Copy link

Member

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 theawait 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 ourAST_NODE_TYPES (and with a non-const enum). We can place this function in theutils package so anyone can consume it easily.

As an example - from the above code we can see thatBinaryExpression has a precedence ofOperatorPrecedence.Coalesce (or4), whereasAwaitExpression has a precedence ofOperatorPrecedence.Unary (or16)

This means thatAwaitExpression has a higher precedence thanBinaryExpression - thus it is evaluated first.
So by evaluating the precedences, we know that before we insert anawait we must wrap the childBinaryExpression in parentheses so that it is evaluated correctly.

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

bradzacher added the awaiting responseIssues waiting for a reply from the OP or another party label

Jan 17, 2022

Copy link

ContributorAuthor

islandryu commentedJan 18, 2022•
edited
Loading

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

// CallExpression has a higher precedence than awaitExpression, so () is not necessary.// inputreturna();// expected outputreturnawaita();// All binaryExpression has a lower precedence than awaitExpression, so () is necessary.// inputreturna()||undefinedreturnundefined||a()returnundefined??a()// expected outputreturnawait(a()||undefined)returnawait(undefined||a())returnawait(undefined??a())// AsExpression has a lower precedence than awaitExpression, so () is necessary.// inputreturna()asT// expected outputreturnawait(a()asT)...

Copy link

Member

bradzacher commentedJan 18, 2022

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

fix(eslint-plugin): [return-await] add ( ) to Expressions with lower …

9c30915

…precedence than awaitExpressions

islandryu commented

Jan 19, 2022

View reviewed changes

packages/eslint-plugin/src/util/getOperatorPrecedence.tsShow resolvedHide resolved

islandryu requested a review frombradzacher

January 19, 2022 14:14

bradzacher removed the awaiting responseIssues waiting for a reply from the OP or another party label

Jan 19, 2022

islandryu added2 commits

January 29, 2022 23:08

Merge branch 'main' ofhttps://github.com/islandryu/typescript-eslint…

9a3bf15

…into fixReturnAwaitInBinaryExpression

fix(eslint-plugin): [return-await] add test

6402c6f

Copy link

ContributorAuthor

islandryu commentedJan 30, 2022

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

Merge branch 'main' into fixReturnAwaitInBinaryExpression

dbf7e4a

bradzacher approved these changes

Mar 1, 2022

View reviewed changes

Copy link

Member

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!

packages/eslint-plugin/src/util/getOperatorPrecedence.tsShow resolvedHide resolved

bradzacher merged commit5fa2fad intotypescript-eslint:main

Mar 1, 2022

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request

Mar 8, 2022

Update typescript-eslint monorepo to v5.14.0 (#1201)

ad861c4

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/1201Reviewed-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-actionsbot locked asresolvedand limited conversation to collaborators

May 25, 2022

Labels

bug

Something isn't working

2 participants

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

islandryu commentedJan 7, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

PR Checklist

Overview

Uh oh!

typescript-eslintbot commentedJan 7, 2022

Uh oh!

nx-cloudbot commentedJan 7, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

netlifybot commentedJan 7, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

codecovbot commentedJan 7, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

islandryu commentedJan 14, 2022

Uh oh!

bradzacher left a comment

Choose a reason for hiding this comment

Uh oh!

islandryu commentedJan 18, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

bradzacher commentedJan 18, 2022

Uh oh!

Uh oh!

islandryu commentedJan 30, 2022

Uh oh!

bradzacher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

islandryu commentedJan 7, 2022•
edited
Loading

nx-cloudbot commentedJan 7, 2022•
edited
Loading

netlifybot commentedJan 7, 2022•
edited
Loading

codecovbot commentedJan 7, 2022•
edited
Loading

islandryu commentedJan 18, 2022•
edited
Loading