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

[islandryu]

PR Checklist

• Addresses an existing issue: fixes [return-await] wrong autofix of return undefined ?? promise to return await undefined ?? promise #4090
• That issue was marked as accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

[typescript-eslint]

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]

☁️ 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

• Node 16.x - nx test @typescript-eslint/eslint-plugin-tslint
• Node 16.x - nx test @typescript-eslint/eslint-plugin
• Node 12.x - nx test @typescript-eslint/eslint-plugin-internal
• Node 12.x - nx test @typescript-eslint/eslint-plugin-tslint
• Node 12.x - nx test @typescript-eslint/eslint-plugin
• Node 14.x - nx test @typescript-eslint/eslint-plugin-tslint
• Node 14.x - nx test @typescript-eslint/eslint-plugin
• Node 12.x - nx test @typescript-eslint/eslint-plugin-tslint
• Node 12.x - nx test @typescript-eslint/eslint-plugin
• Node 12.x - nx run-many --target=typecheck --all --parallel
• Node 12.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 16.x - nx test @typescript-eslint/parser
• Node 16.x - nx test @typescript-eslint/type-utils
• Node 16.x - nx test @typescript-eslint/utils
• Node 16.x - nx test @typescript-eslint/scope-manager
• Node 16.x - nx test @typescript-eslint/visitor-keys
• Node 12.x - nx test @typescript-eslint/parser
• Node 16.x - nx test @typescript-eslint/typescript-estree
• Node 12.x - nx test @typescript-eslint/type-utils
• Node 16.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12.x - nx test @typescript-eslint/utils
• Node 12.x - nx test @typescript-eslint/scope-manager
• Node 12.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12.x - nx test @typescript-eslint/visitor-keys
• Node 12.x - nx test @typescript-eslint/typescript-estree
• Node 12.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14.x - nx test @typescript-eslint/parser
• Node 14.x - nx test @typescript-eslint/type-utils
• Node 12.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14.x - nx test @typescript-eslint/utils
• Node 14.x - nx test @typescript-eslint/scope-manager
• Node 12.x - nx test @typescript-eslint/parser
• Node 14.x - nx test @typescript-eslint/visitor-keys
• Node 12.x - nx test @typescript-eslint/type-utils
• Node 14.x - nx test @typescript-eslint/typescript-estree
• Node 12.x - nx test @typescript-eslint/utils
• Node 14.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12.x - nx test @typescript-eslint/scope-manager
• Node 12.x - nx test @typescript-eslint/visitor-keys
• Node 12.x - nx test @typescript-eslint/typescript-estree
• Node 12.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12.x - nx prebuild @typescript-eslint/types
• Node 16.x - nx prebuild @typescript-eslint/types
• Node 12.x - nx prebuild @typescript-eslint/types
• Node 12.x - nx prebuild @typescript-eslint/types
• Node 12.x - nx prebuild @typescript-eslint/types
• Node 12.x - nx prebuild @typescript-eslint/types
• Node 14.x - nx prebuild @typescript-eslint/types

---

Sent with 💌 from NxCloud.

[netlify]

✔️ 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]

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

[islandryu]

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

[bradzacher]

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.

[islandryu]

@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]

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

[islandryu]

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

[bradzacher]

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