typescript-eslint/typescript-eslintPublic

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

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

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

JoshuaKGoldberg merged 22 commits intotypescript-eslint:mainfromyeonjuan:fix-8674

Aug 12, 2024

Merged

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

JoshuaKGoldberg merged 22 commits intotypescript-eslint:mainfromyeonjuan:fix-8674

Aug 12, 2024

Conversation

Copy link

Contributor

yeonjuan commentedMar 17, 2024

PR Checklist

Addresses an existing open issue:fixesEnhancement: [no-unsafe-return] Disallow return Promise<any> #8674
That issue was marked asaccepting prs
Steps inContributing were taken

Overview

Copy link

Contributor

typescript-eslintbot commentedMar 17, 2024

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.

yeonjuan marked this pull request as draft

March 17, 2024 07:52

Copy link

netlifybot commentedMar 17, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`006d16c`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/669e79e1b9ebfd00092dabdb
😎 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: 100 (🟢 up 6 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 90 (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 yourNetlify site configuration.

Copy link

nx-cloudbot commentedMar 17, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit006d16c. 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

Sent with 💌 fromNxCloud.

yeonjuan added2 commits

April 10, 2024 17:13

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

c48ff42

add testcases

2cf6004

yeonjuan force-pushed thefix-8674 branch from708f449 to2cf6004Compare

April 10, 2024 08:13

yeonjuan marked this pull request as ready for review

April 10, 2024 08:30

kirkwaiblinger requested changes

Apr 21, 2024

View reviewed changes

Copy link

Member

kirkwaiblinger left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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 returnany, but whose type evaluates toany after beingAwaited? Feel free to ignore this if it's a hassle to implement. But, my thought would be something like "Unsafe return of typePromise<any> that is equivalent to simply returningany 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!

packages/eslint-plugin/tests/rules/no-unsafe-return.test.ts OutdatedShow resolvedHide resolved

packages/eslint-plugin/tests/rules/no-unsafe-return.test.tsShow resolvedHide resolved

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

Apr 21, 2024

kirkwaiblinger reviewed

Apr 21, 2024

View reviewed changes

packages/eslint-plugin/tests/rules/no-unsafe-return.test.ts

		messageId:'unsafeReturn',
		line:4,
		column:3,
		endColumn:16,

Copy link

Member

kirkwaiblingerApr 21, 2024

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

yeonjuan commentedApr 25, 2024

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"

Copy link

Member

kirkwaiblinger commentedApr 25, 2024

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"

👍 👍

Merge branch 'main' into fix-8674

7edaed9

# Conflicts:#packages/eslint-plugin/tests/rules/no-unsafe-return.test.ts

Copy link

codecovbot commentedApr 30, 2024•
edited
Loading

Codecov Report

Attention: Patch coverage is71.42857% with8 lines in your changes missing coverage. Please review.

Project coverage is 88.42%. Comparing base(d32d8f4) to head(006d16c).
Report is 158 commits behind head on main.

Files	Patch %	Lines
packages/type-utils/src/predicates.ts	0.00%	8 Missing⚠️

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

Flag	Coverage Δ
unittest	`88.42% <71.42%> (-0.03%)`	⬇️

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

Files	Coverage Δ
...ckages/eslint-plugin/src/rules/no-unsafe-return.ts	`97.43% <100.00%> (+0.66%)`	⬆️
packages/type-utils/src/predicates.ts	`21.21% <0.00%> (-2.13%)`	⬇️

apply reviews

e84d6ab

yeonjuan force-pushed thefix-8674 branch from2e35637 toe84d6abCompare

May 1, 2024 16:33

Copy link

ContributorAuthor

yeonjuan commentedMay 1, 2024

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

yeonjuan added2 commits

May 2, 2024 23:09

Merge branch 'main' into fix-8674

52c6715

fix lint errors

f24b746

yeonjuan force-pushed thefix-8674 branch from600e3dc tof24b746Compare

May 2, 2024 15:14

yeonjuan added2 commits

May 3, 2024 01:01

refactor

014cd48

add type

6390a95

yeonjuan requested a review fromkirkwaiblinger

May 3, 2024 11:15

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

May 3, 2024

Copy link

ContributorAuthor

yeonjuan commentedMay 3, 2024

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

bradzacher added the enhancementNew feature or request label

May 28, 2024

kirkwaiblinger requested changes

Jun 11, 2024

View reviewed changes

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

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

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 🙂

packages/eslint-plugin/src/rules/no-unsafe-return.ts Outdated

		?'any'
		:anyType===AnyType.AnyArray
		?'any[]'
		:'Promise<any>',

Copy link

Member

kirkwaiblingerJun 11, 2024

Choose a reason for hiding this comment

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

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}}.

packages/type-utils/src/getAwaitedType.ts OutdatedShow resolvedHide resolved

packages/type-utils/src/predicates.ts OutdatedShow resolvedHide resolved

packages/type-utils/src/predicates.ts Outdated

		Safe,
		}
		/**
		*@returns `AnyType.Any` if the type is `any`, `AnyType.AnyArray` if the type is `any[]` or `readonly any[]`,

Copy link

Member

kirkwaiblingerJun 11, 2024

Choose a reason for hiding this comment

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

nit - comment will need updating

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

Jun 11, 2024

improve messages

25daa68

yeonjuan mentioned this pull request

Jul 12, 2024

Expose TypeChecker.getAwaitedType for public apimicrosoft/TypeScript#59256

Closed

6 tasks

kirkwaiblinger requested changes

Jul 15, 2024

View reviewed changes

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

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

Only blocker remaining is resolution of#8693 (comment), I think

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

Jul 15, 2024

apply review

74e16c4

yeonjuan force-pushed thefix-8674 branch from7828c3e to74e16c4Compare

July 15, 2024 16:21

yeonjuan added2 commits

July 16, 2024 01:33

fix tests

8e5a19b

change to use internal getAwaitedType

0d0a86a

kirkwaiblinger reviewed

Jul 17, 2024

View reviewed changes

packages/type-utils/typings/typescript.d.ts

		*/
		getTypeOfPropertyOfType(type:Type,propertyName:string):Type\|undefined;
		/**
		* Return the awaited type of the given type.

Copy link

Member

kirkwaiblingerJul 17, 2024

Choose a reason for hiding this comment

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

Maybe add a TODO that this should no longer be necessary in a future TS version and link the TS issue?

Copy link

ContributorAuthor

yeonjuanJul 18, 2024

Choose a reason for hiding this comment

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

Sure!fcca5ec

add todo comment

fcca5ec

JoshuaKGoldberg reviewed

Jul 20, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/no-unsafe-return.mdx OutdatedShow resolvedHide resolved

JoshuaKGoldberg previously approved these changes

Jul 20, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Yay! 🚀

JoshuaKGoldberg added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label

Jul 20, 2024

yeonjuan dismissedJoshuaKGoldberg’sstale review via2542bd5

July 20, 2024 14:07

Update packages/eslint-plugin/docs/rules/no-unsafe-return.mdx

2542bd5

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

kirkwaiblinger reviewed

Jul 22, 2024

View reviewed changes

packages/eslint-plugin/tests/rules/no-unsafe-return.test.ts Outdated

		{
		code:`
		declare const value: Promise<any>;
		function foo() {

Copy link

Member

kirkwaiblingerJul 22, 2024

Choose a reason for hiding this comment

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

I thought this is supposed to be valid, since it's not anasync function, according to#8674 (comment). Did that decision get changed?

Copy link

Member

kirkwaiblingerJul 22, 2024

Choose a reason for hiding this comment

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

Note also that the error message for this case "Unsafe return of anPromise<any> typed value." is problematic, same as#8693 (comment)

Copy link

ContributorAuthor

yeonjuanJul 22, 2024•
edited
Loading

Choose a reason for hiding this comment

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

I've fixed! I had forgotten about those comments.

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

Jul 22, 2024

auvred previously approved these changes

Jul 22, 2024

View reviewed changes

Copy link

Member

auvred left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Looks great!

UPD: There seems to be a merge conflict with#9540.

kirkwaiblinger requested changes

Jul 22, 2024

View reviewed changes

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

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

Left a couple questions

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

Jul 22, 2024

apply reviews

ccc2e97

yeonjuan dismissedauvred’sstale review viaccc2e97

July 22, 2024 15:15

Merge branch 'main' into fix-8674

006d16c

JoshuaKGoldberg removed the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label

Jul 29, 2024

yeonjuan requested a review fromkirkwaiblinger

August 1, 2024 13:34

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

Aug 1, 2024

kirkwaiblinger approved these changes

Aug 1, 2024

View reviewed changes

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

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

Yay!

kirkwaiblinger added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label

Aug 1, 2024

JoshuaKGoldberg approved these changes

Aug 12, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

JoshuaKGoldberg merged commitc20bd2f intotypescript-eslint:main

Aug 12, 2024

github-actionsbot locked asresolvedand limited conversation to collaborators

Aug 20, 2024

Labels

1 approval

>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge

enhancement

New feature or request

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

yeonjuan commentedMar 17, 2024

PR Checklist

Overview

Uh oh!

typescript-eslintbot commentedMar 17, 2024

Uh oh!

netlifybot commentedMar 17, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

✅ Deploy Preview fortypescript-eslint ready!

Uh oh!

nx-cloudbot commentedMar 17, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

kirkwaiblinger left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kirkwaiblingerApr 21, 2024

Choose a reason for hiding this comment

Uh oh!

yeonjuan commentedApr 25, 2024

Uh oh!

kirkwaiblinger commentedApr 25, 2024

Uh oh!

codecovbot commentedApr 30, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yeonjuan commentedMay 1, 2024

Uh oh!

yeonjuan commentedMay 3, 2024

Uh oh!

kirkwaiblinger left a comment

Choose a reason for hiding this comment

Uh oh!

kirkwaiblingerJun 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kirkwaiblingerJun 11, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger left a comment

Choose a reason for hiding this comment

Uh oh!

kirkwaiblingerJul 17, 2024

Choose a reason for hiding this comment

Uh oh!

yeonjuanJul 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

kirkwaiblingerJul 22, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblingerJul 22, 2024

Choose a reason for hiding this comment

Uh oh!

yeonjuanJul 22, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auvred left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger left a comment

netlifybot commentedMar 17, 2024•
edited
Loading

nx-cloudbot commentedMar 17, 2024•
edited
Loading

kirkwaiblinger left a comment•
edited
Loading

codecovbot commentedApr 30, 2024•
edited
Loading

yeonjuanJul 22, 2024•
edited
Loading

auvred left a comment•
edited
Loading