feat(eslint-plugin): [no-unsafe-return] check promise any#8693
feat(eslint-plugin): [no-unsafe-return] check promise any#8693JoshuaKGoldberg merged 22 commits intotypescript-eslint:mainfrom
Conversation
|
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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
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!
| messageId: 'unsafeReturn', | ||
| line: 4, | ||
| column: 3, | ||
| endColumn: 16, |
There was a problem hiding this comment.
(nit, optional) If you're gonna include the endColumn, might as well include the endLine too for consistency. Here and elsewhere
@kirkwaiblinger I agree, this is great, but how about making the error message a little more concise? |
👍 👍 |
# Conflicts: # packages/eslint-plugin/tests/rules/no-unsafe-return.test.ts
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8693 +/- ##
==========================================
- Coverage 88.45% 88.42% -0.03%
==========================================
Files 422 422
Lines 14695 14717 +22
Branches 4298 4308 +10
==========================================
+ Hits 12998 13014 +16
- Misses 1372 1378 +6
Partials 325 325
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
CI is failing, I created a separate PR to fix it. #9037 |
|
@kirkwaiblinger Thank you for your review. I added test cases and improved messsage. |
There was a problem hiding this comment.
Hi @yeonjuan!
First off, I want to say I am very sorry for how long it's taken me to get to another review pass on this!
This is great stuff. A few more tweaks and questions for iteration, but I feel like we're getting close 🙂
| ? 'any' | ||
| : anyType === AnyType.AnyArray | ||
| ? 'any[]' | ||
| : 'Promise<any>', |
There was a problem hiding this comment.
Nit - not your fault, but this now runs into an "a" vs "an" issue in the generated string. I get
Unsafe return of an `Promise<any>` typed value
Would you change the wording a bit so that it's grammatically consistent?
For example, Unsafe return of a value of type {{type}}.
| Safe, | ||
| } | ||
| /** | ||
| * @returns `AnyType.Any` if the type is `any`, `AnyType.AnyArray` if the type is `any[]` or `readonly any[]`, |
There was a problem hiding this comment.
nit - comment will need updating
There was a problem hiding this comment.
Only blocker remaining is resolution of #8693 (comment), I think
| */ | ||
| getTypeOfPropertyOfType(type: Type, propertyName: string): Type | undefined; | ||
| /** | ||
| * Return the awaited type of the given type. |
There was a problem hiding this comment.
Maybe add a TODO that this should no longer be necessary in a future TS version and link the TS issue?
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
| { | ||
| code: ` | ||
| declare const value: Promise<any>; | ||
| function foo() { |
There was a problem hiding this comment.
I thought this is supposed to be valid, since it's not an async function, according to #8674 (comment). Did that decision get changed?
There was a problem hiding this comment.
Note also that the error message for this case "Unsafe return of an Promise<any> typed value." is problematic, same as #8693 (comment)
There was a problem hiding this comment.
I've fixed! I had forgotten about those comments.
There was a problem hiding this comment.
Left a couple questions
PR Checklist
Overview