Skip to content

Commit

Permalink
[promise-function-async] Only allow unions in explicit return types
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ericbf committed Jan 12, 2023
1 parent 10ce912 commit 3527630
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 10 deletions.
18 changes: 17 additions & 1 deletion packages/eslint-plugin/docs/rules/promise-function-async.md
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -39,4 +45,14 @@ const arrowFunctionReturnsPromise = async () => Promise.resolve('value');
async function functionReturnsPromise() {
return Promise.resolve('value');
}

function functionReturnsUnionWithPromiseExplicitly(
p: boolean,
): string | Promise<string> {
return p ? 'value' : Promise.resolve('value');
}

async function functionReturnsUnionWithPromiseImplicitly(p: boolean) {
return p ? 'value' : Promise.resolve('value');
}
```
4 changes: 3 additions & 1 deletion packages/eslint-plugin/src/rules/promise-function-async.ts
Expand Up @@ -111,10 +111,12 @@ export default util.createRule<Options, MessageIds>({
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
Expand Down
24 changes: 24 additions & 0 deletions packages/eslint-plugin/tests/rules/promise-function-async.test.ts
Expand Up @@ -165,6 +165,13 @@ abstract class Test {
}
`,
},
`
function promiseInUnionWithExplicitReturnType(
p: boolean,
): Promise<number> | number {
return p ? Promise.resolve(5) : 5;
}
`,
],
invalid: [
{
Expand Down Expand Up @@ -752,5 +759,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;
}
`,
},
],
});
Expand Up @@ -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<string>,
mustMatchAll: boolean,
): boolean {
if (isTypeFlagSet(type, ts.TypeFlags.Any | ts.TypeFlags.Unknown)) {
return !allowAny;
Expand All @@ -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))
);
}
2 changes: 1 addition & 1 deletion 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';
Expand Down

0 comments on commit 3527630

Please sign in to comment.