feat(eslint-plugin): [no-unsafe-return] check promise any

[yeonjuan]

PR Checklist

• Addresses an existing open issue: fixes Enhancement: [no-unsafe-return] Disallow return Promise<any> #8674
• That issue was marked as accepting prs
• Steps in Contributing were taken

Overview

[typescript-eslint]

Thanks for the PR, @yeonjuan!

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.

[netlify]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	6390a95
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6634c6f7b5fe0f0008c6842b
😎 Deploy Preview	https://deploy-preview-8693--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 97 (no change from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 6390a95. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

✅ Successfully ran 31 targets

• nx test eslint-plugin --coverage=false
• nx test eslint-plugin
• nx test type-utils --coverage=false
• nx run types:build
• nx test typescript-estree --coverage=false
• nx test repo-tools --coverage=false
• nx test utils --coverage=false
• nx test visitor-keys --coverage=false
• nx run repo-tools:postinstall-script
• nx test typescript-eslint --coverage=false
• nx test scope-manager --coverage=false
• nx test rule-schema-to-typescript-types --coverage=false
• nx test eslint-plugin-internal --coverage=false
• nx run-many --target=build --parallel --exclude=website --exclude=website-eslint
• nx test parser --coverage=false
• nx test ast-spec --coverage=false
• nx run-many --target=lint --parallel --exclude eslint-plugin
• nx test type-utils
• nx test visitor-keys
• nx test utils
• nx test typescript-eslint
• nx test eslint-plugin-internal
• nx test rule-schema-to-typescript-types
• nx test scope-manager
• nx test typescript-estree
• nx test parser
• nx test repo-tools
• nx run-many --target=typecheck --parallel
• nx test ast-spec
• nx run integration-tests:test
• nx run repo-tools:generate-configs

---

Sent with 💌 from NxCloud.

[kirkwaiblinger]

A good start! Curious to see what ends up changing in order to support the added test cases.

And a thought - these are realllly tricky. I spent a lot of time myself getting confused over whether the code I was trying out actually should pass or fail 😆.

So, I wonder if we can help the user out by supplying a custom error message for the async cases that don't directly return any, but whose type evaluates to any after being Awaited? Feel free to ignore this if it's a hassle to implement. But, my thought would be something like "Unsafe return of type Promise<any> that is equivalent to simply returning any within an async function." I'm curious for your thoughts. And, as said, it's completely ok if you would prefer to ignore this idea!

[kirkwaiblinger]

Question - why do we need this test? As written, this is illegal TS; TS demands you must use Promise<any>.

My thought is - if we do need to test it, indicate this explicitly with @ts-expect-error comment. If not, we can remove.

[kirkwaiblinger]

A couple test cases....

In particular a bit of tightening around #8674 (comment) is in order.

And I've thrown in a couple annoying (and, admittedly, somewhat artificial) test cases to tighten up getAwaitedType, to try to help it match Awaited<T>, which will collapse Awaited<Promise<Promise<Promise<T>>>> to T

// (passes)
// should be valid; ok to return any in async function that returns Promise<any>.
async function f(): Promise<any> {
  return {} as any;
}

// (passes)
// should be invalid; not ok to return any in async function that returns type narrower than Promise<any>
async function f(): Promise<object> {
  return {} as any;
}

// (fails)
// should be invalid; can't return any in sync function returning a Promise (since Promise != any).
function f(): Promise<any> {
  return {} as any;
}

// (passes)
// should be invalid; can't return any in sync function returning a Promise (since Promise != any).
function f(): Promise<object> {
  return {} as any;
}

// (fails)
// should be valid; ok to return Promise<any> in function returning object.
function f(): object {
  return Promise.resolve({} as any);
}

// (passes)
// should be invalid; returning T<any> to function returning T<object>.
function f(): Promise<object> {
  return Promise.resolve<any>({});
}

// (passes)
// should be invalid; returning T<any> to function returning T<object>
function f(): Promise<object> {
  return Promise.resolve<Promise<Promise<any>>>({} as any);
}

// (passes)
// should be invalid; returning value that awaits to any in async function 
// with return type narrower than Promise<any>. 
async function f(): Promise<object> {
  return Promise.resolve<Promise<Promise<any>>>({} as Promise<any>);
}

// (fails)
// should be invalid; returning value that awaits to any in async function
// with return type narrower than Promise<any>. 
async function f(): Promise<object> {
  return {} as Promise<Promise<Promise<Promise<any>>>>
}

// (fails)
// should be invalid; returning value that awaits to any in async function
// without return type. 
async function f() {
  return {} as Promise<Promise<Promise<Promise<any>>>>
}

[kirkwaiblinger]

PS

• apologies if I accidentally included a few test cases that you've already written
• i wouldn't be wholly surprised if I got the valid/invalid wrong for a few of these, so feel free to push back if some of them seem incorrect

[kirkwaiblinger]

(nit, optional) If you're gonna include the endColumn, might as well include the endLine too for consistency. Here and elsewhere

[yeonjuan]

So, I wonder if we can help the user out by supplying a custom error message for the async cases that don't directly return any, but whose type evaluates to any after being Awaited? Feel free to ignore this if it's a hassle to implement. But, my thought would be something like "Unsafe return of type Promise that is equivalent to simply returning any within an async function." I'm curious for your thoughts. And, as said, it's completely ok if you would prefer to ignore this idea!

@kirkwaiblinger I agree, this is great, but how about making the error message a little more concise? "Unsafe return of type Promise<any> typed value"

[kirkwaiblinger]

So, I wonder if we can help the user out by supplying a custom error message for the async cases that don't directly return any, but whose type evaluates to any after being Awaited? Feel free to ignore this if it's a hassle to implement. But, my thought would be something like "Unsafe return of type Promise that is equivalent to simply returning any within an async function." I'm curious for your thoughts. And, as said, it's completely ok if you would prefer to ignore this idea!

@kirkwaiblinger I agree, this is great, but how about making the error message a little more concise? "Unsafe return of type Promise<any> typed value"

👍 👍

[codecov]

Codecov Report

Attention: Patch coverage is 88.88889% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 87.24%. Comparing base (68739b2) to head (1dcbe73).
Report is 69 commits behind head on main.

❗ Current head 1dcbe73 differs from pull request most recent head 6390a95

Please upload reports for the commit 6390a95 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8693      +/-   ##
==========================================
- Coverage   87.40%   87.24%   -0.16%     
==========================================
  Files         260      251       -9     
  Lines       12608    12341     -267     
  Branches     3937     3885      -52     
==========================================
- Hits        11020    10767     -253     
+ Misses       1313     1304       -9     
+ Partials      275      270       -5     

Flag	Coverage Δ
unittest	87.24% <88.88%> (-0.16%)	⬇️

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

Files	Coverage Δ
packages/type-utils/src/predicates.ts	23.72% <ø> (+0.39%)	⬆️
...ckages/eslint-plugin/src/rules/no-unsafe-return.ts	95.52% <88.88%> (-1.09%)	⬇️

... and 59 files with indirect coverage changes

[yeonjuan]

CI is failing, I created a separate PR to fix it. #9037

[yeonjuan]

@kirkwaiblinger Thank you for your review. I added test cases and improved messsage.