From ea34d2342ed62e56d9e9c1119c1775ca4a9991eb Mon Sep 17 00:00:00 2001 From: Eric Ferreira Date: Thu, 12 Jan 2023 17:19:07 -0500 Subject: [PATCH 1/2] [promise-function-async] Only allow unions in explicit return types When we return a union containing a promise from a function implicitly, it's often a mistake. This commit makes it so if the return type is explicit, any `Promise` in the return type (whether it's part of a union or not) will flag the function. If it is intentional, make the return type explicit. Fixes #6329 --- .../docs/rules/promise-function-async.md | 18 +++++++++- .../src/rules/promise-function-async.ts | 4 ++- .../rules/promise-function-async.test.ts | 34 +++++++++++++++++++ ...lTypesByName.ts => containsTypesByName.ts} | 22 ++++++++---- packages/type-utils/src/index.ts | 2 +- 5 files changed, 70 insertions(+), 10 deletions(-) rename packages/type-utils/src/{containsAllTypesByName.ts => containsTypesByName.ts} (52%) diff --git a/packages/eslint-plugin/docs/rules/promise-function-async.md b/packages/eslint-plugin/docs/rules/promise-function-async.md index e697217dc5d..4ca59ac341e 100644 --- a/packages/eslint-plugin/docs/rules/promise-function-async.md +++ b/packages/eslint-plugin/docs/rules/promise-function-async.md @@ -11,10 +11,12 @@ Ensures that each function is only capable of: - returning a rejected promise, or - throwing an Error object. -In contrast, non-`async` `Promise` - returning functions are technically capable of either. +In contrast, non-`async`, `Promise`-returning functions are technically capable of either. Code that handles the results of those functions will often need to handle both cases, which can get complex. This rule's practice removes a requirement for creating code to handle both cases. +> When functions return unions of `Promise` and non-`Promise` types implicitly, it is usually a mistake—this rule flags those cases. If it is intentional, make the return type explicitly to allow the rule to pass. + ## Examples Examples of code for this rule @@ -29,6 +31,10 @@ const arrowFunctionReturnsPromise = () => Promise.resolve('value'); function functionReturnsPromise() { return Promise.resolve('value'); } + +function functionReturnsUnionWithPromiseImplicitly(p: boolean) { + return p ? 'value' : Promise.resolve('value'); +} ``` ### ✅ Correct @@ -39,4 +45,14 @@ const arrowFunctionReturnsPromise = async () => Promise.resolve('value'); async function functionReturnsPromise() { return Promise.resolve('value'); } + +function functionReturnsUnionWithPromiseExplicitly( + p: boolean, +): string | Promise { + return p ? 'value' : Promise.resolve('value'); +} + +async function functionReturnsUnionWithPromiseImplicitly(p: boolean) { + return p ? 'value' : Promise.resolve('value'); +} ``` diff --git a/packages/eslint-plugin/src/rules/promise-function-async.ts b/packages/eslint-plugin/src/rules/promise-function-async.ts index 4387bc52c9b..e8b9f44739b 100644 --- a/packages/eslint-plugin/src/rules/promise-function-async.ts +++ b/packages/eslint-plugin/src/rules/promise-function-async.ts @@ -111,10 +111,12 @@ export default util.createRule({ const returnType = checker.getReturnTypeOfSignature(signatures[0]); if ( - !util.containsAllTypesByName( + !util.containsTypesByName( returnType, allowAny!, allAllowedPromiseNames, + // Only if a return type is explicitly declared must it match all + Boolean(node.returnType), ) ) { // Return type is not a promise diff --git a/packages/eslint-plugin/tests/rules/promise-function-async.test.ts b/packages/eslint-plugin/tests/rules/promise-function-async.test.ts index ccf99b6afcb..089e6ba617b 100644 --- a/packages/eslint-plugin/tests/rules/promise-function-async.test.ts +++ b/packages/eslint-plugin/tests/rules/promise-function-async.test.ts @@ -165,6 +165,23 @@ abstract class Test { } `, }, + ` +function promiseInUnionWithExplicitReturnType( + p: boolean, +): Promise | number { + return p ? Promise.resolve(5) : 5; +} + `, + ` +function explicitReturnWithPromiseInUnion(): Promise | number { + return 5; +} + `, + ` +async function asyncFunctionReturningUnion(p: boolean) { + return p ? Promise.resolve(5) : 5; +} + `, ], invalid: [ { @@ -752,5 +769,22 @@ const foo = { }, ], }, + { + code: ` +function promiseInUnionWithoutExplicitReturnType(p: boolean) { + return p ? Promise.resolve(5) : 5; +} + `, + errors: [ + { + messageId, + }, + ], + output: ` +async function promiseInUnionWithoutExplicitReturnType(p: boolean) { + return p ? Promise.resolve(5) : 5; +} + `, + }, ], }); diff --git a/packages/type-utils/src/containsAllTypesByName.ts b/packages/type-utils/src/containsTypesByName.ts similarity index 52% rename from packages/type-utils/src/containsAllTypesByName.ts rename to packages/type-utils/src/containsTypesByName.ts index 07aa20dac04..2e57984ac28 100644 --- a/packages/type-utils/src/containsAllTypesByName.ts +++ b/packages/type-utils/src/containsTypesByName.ts @@ -5,13 +5,16 @@ import { isTypeFlagSet } from './typeFlagUtils'; /** * @param type Type being checked by name. + * @param allowAny Whether to consider `any` and `unknown` to match. * @param allowedNames Symbol names checking on the type. - * @returns Whether the type is, extends, or contains all of the allowed names. + * @param mustMatchAll Whether all parts have to match, as opposed to any parts matching. + * @returns Whether the type is, extends, or contains the allowed names (or all matches the allowed names, if mustMatchAll is true). */ -export function containsAllTypesByName( +export function containsTypesByName( type: ts.Type, allowAny: boolean, allowedNames: Set, + mustMatchAll: boolean, ): boolean { if (isTypeFlagSet(type, ts.TypeFlags.Any | ts.TypeFlags.Unknown)) { return !allowAny; @@ -26,16 +29,21 @@ export function containsAllTypesByName( return true; } + const predicate = (t: ts.Type): boolean => + containsTypesByName(t, allowAny, allowedNames, mustMatchAll); + if (isUnionOrIntersectionType(type)) { - return type.types.every(t => - containsAllTypesByName(t, allowAny, allowedNames), - ); + return mustMatchAll + ? type.types.every(predicate) + : type.types.some(predicate); } const bases = type.getBaseTypes(); + return ( typeof bases !== 'undefined' && - bases.length > 0 && - bases.every(t => containsAllTypesByName(t, allowAny, allowedNames)) + (mustMatchAll + ? bases.length > 0 && bases.every(predicate) + : bases.some(predicate)) ); } diff --git a/packages/type-utils/src/index.ts b/packages/type-utils/src/index.ts index dde032e1770..a350d5c8658 100644 --- a/packages/type-utils/src/index.ts +++ b/packages/type-utils/src/index.ts @@ -1,4 +1,4 @@ -export * from './containsAllTypesByName'; +export * from './containsTypesByName'; export * from './getConstrainedTypeAtLocation'; export * from './getContextualType'; export * from './getDeclaration'; From be9531d4b72c45d83ab142f4a833dec398556206 Mon Sep 17 00:00:00 2001 From: Eric Ferreira Date: Fri, 10 Feb 2023 13:05:27 -0500 Subject: [PATCH 2/2] Refrain from renaming the type-util, instead add an optional fourth param. --- .../docs/rules/promise-function-async.md | 1 + .../src/rules/promise-function-async.ts | 6 +++--- ...pesByName.ts => containsAllTypesByName.ts} | 20 +++++++++---------- packages/type-utils/src/index.ts | 2 +- 4 files changed, 15 insertions(+), 14 deletions(-) rename packages/type-utils/src/{containsTypesByName.ts => containsAllTypesByName.ts} (69%) diff --git a/packages/eslint-plugin/docs/rules/promise-function-async.md b/packages/eslint-plugin/docs/rules/promise-function-async.md index 4ca59ac341e..f3095ed1808 100644 --- a/packages/eslint-plugin/docs/rules/promise-function-async.md +++ b/packages/eslint-plugin/docs/rules/promise-function-async.md @@ -46,6 +46,7 @@ async function functionReturnsPromise() { return Promise.resolve('value'); } +// An explicit return type that is not Promise means this function cannot be made async, so it is ignored by the rule function functionReturnsUnionWithPromiseExplicitly( p: boolean, ): string | Promise { diff --git a/packages/eslint-plugin/src/rules/promise-function-async.ts b/packages/eslint-plugin/src/rules/promise-function-async.ts index e8b9f44739b..34fbc7ee672 100644 --- a/packages/eslint-plugin/src/rules/promise-function-async.ts +++ b/packages/eslint-plugin/src/rules/promise-function-async.ts @@ -111,12 +111,12 @@ export default util.createRule({ const returnType = checker.getReturnTypeOfSignature(signatures[0]); if ( - !util.containsTypesByName( + !util.containsAllTypesByName( returnType, allowAny!, allAllowedPromiseNames, - // Only if a return type is explicitly declared must it match all - Boolean(node.returnType), + // If no return type is explicitly set, we check if any parts of the return type match a Promise (instead of requiring all to match). + node.returnType == null, ) ) { // Return type is not a promise diff --git a/packages/type-utils/src/containsTypesByName.ts b/packages/type-utils/src/containsAllTypesByName.ts similarity index 69% rename from packages/type-utils/src/containsTypesByName.ts rename to packages/type-utils/src/containsAllTypesByName.ts index 2e57984ac28..9c3fb522236 100644 --- a/packages/type-utils/src/containsTypesByName.ts +++ b/packages/type-utils/src/containsAllTypesByName.ts @@ -7,14 +7,14 @@ import { isTypeFlagSet } from './typeFlagUtils'; * @param type Type being checked by name. * @param allowAny Whether to consider `any` and `unknown` to match. * @param allowedNames Symbol names checking on the type. - * @param mustMatchAll Whether all parts have to match, as opposed to any parts matching. + * @param matchAnyInstead Whether to instead just check if any parts match, rather than all parts. * @returns Whether the type is, extends, or contains the allowed names (or all matches the allowed names, if mustMatchAll is true). */ -export function containsTypesByName( +export function containsAllTypesByName( type: ts.Type, allowAny: boolean, allowedNames: Set, - mustMatchAll: boolean, + matchAnyInstead = false, ): boolean { if (isTypeFlagSet(type, ts.TypeFlags.Any | ts.TypeFlags.Unknown)) { return !allowAny; @@ -30,20 +30,20 @@ export function containsTypesByName( } const predicate = (t: ts.Type): boolean => - containsTypesByName(t, allowAny, allowedNames, mustMatchAll); + containsAllTypesByName(t, allowAny, allowedNames, matchAnyInstead); if (isUnionOrIntersectionType(type)) { - return mustMatchAll - ? type.types.every(predicate) - : type.types.some(predicate); + return matchAnyInstead + ? type.types.some(predicate) + : type.types.every(predicate); } const bases = type.getBaseTypes(); return ( typeof bases !== 'undefined' && - (mustMatchAll - ? bases.length > 0 && bases.every(predicate) - : bases.some(predicate)) + (matchAnyInstead + ? bases.some(predicate) + : bases.length > 0 && bases.every(predicate)) ); } diff --git a/packages/type-utils/src/index.ts b/packages/type-utils/src/index.ts index a350d5c8658..dde032e1770 100644 --- a/packages/type-utils/src/index.ts +++ b/packages/type-utils/src/index.ts @@ -1,4 +1,4 @@ -export * from './containsTypesByName'; +export * from './containsAllTypesByName'; export * from './getConstrainedTypeAtLocation'; export * from './getContextualType'; export * from './getDeclaration';