Skip to content
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

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

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 18 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,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(
ericbf marked this conversation as resolved.
Show resolved Hide resolved
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
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
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
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))
);
}