Skip to content

feat(eslint-plugin): [promise-function-async] check for promises in implicit return types #6330

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion packages/eslint-plugin/docs/rules/promise-function-async.md
Original file line number Diff line number Diff line change
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,15 @@ const arrowFunctionReturnsPromise = async () => Promise.resolve('value');
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<string> {
return p ? 'value' : Promise.resolve('value');
}

async function functionReturnsUnionWithPromiseImplicitly(p: boolean) {
return p ? 'value' : Promise.resolve('value');
}
```
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/promise-function-async.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ export default util.createRule<Options, MessageIds>({
returnType,
allowAny!,
allAllowedPromiseNames,
// 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
Expand Down
34 changes: 34 additions & 0 deletions packages/eslint-plugin/tests/rules/promise-function-async.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,23 @@ abstract class Test {
}
`,
},
`
function promiseInUnionWithExplicitReturnType(
p: boolean,
): Promise<number> | number {
return p ? Promise.resolve(5) : 5;
}
`,
`
function explicitReturnWithPromiseInUnion(): Promise<number> | number {
return 5;
}
`,
`
async function asyncFunctionReturningUnion(p: boolean) {
return p ? Promise.resolve(5) : 5;
}
`,
],
invalid: [
{
Expand Down Expand Up @@ -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;
}
`,
},
],
});
20 changes: 14 additions & 6 deletions packages/type-utils/src/containsAllTypesByName.ts
Original file line number Diff line number Diff line change
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 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 containsAllTypesByName(
type: ts.Type,
allowAny: boolean,
allowedNames: Set<string>,
matchAnyInstead = false,
): 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 =>
containsAllTypesByName(t, allowAny, allowedNames, matchAnyInstead);

if (isUnionOrIntersectionType(type)) {
return type.types.every(t =>
containsAllTypesByName(t, allowAny, allowedNames),
);
return matchAnyInstead
? type.types.some(predicate)
: type.types.every(predicate);
}

const bases = type.getBaseTypes();

return (
typeof bases !== 'undefined' &&
bases.length > 0 &&
bases.every(t => containsAllTypesByName(t, allowAny, allowedNames))
(matchAnyInstead
? bases.some(predicate)
: bases.length > 0 && bases.every(predicate))
);
}