Skip to content

fix(eslint-plugin): [promise-function-async] handle function overloading #10304

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.

Sign up for GitHub

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
merged 14 commits into from
Dec 1, 2024
Merged

fix(eslint-plugin): [promise-function-async] handle function overloading #10304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9ba3bf7
initial implementation
ronami Nov 7, 2024
5619a87
remove unnecessary comment
ronami Nov 7, 2024
5f79c06
rework the code to use a single signature
ronami Nov 7, 2024
1c0d222
reduce changes footprint
ronami Nov 7, 2024
294711e
reduce changes footprint further
ronami Nov 7, 2024
481a136
use nullThrows rather than an unreachable conditional
ronami Nov 8, 2024
75ffdc5
revert implementation to check every function signature
ronami Nov 16, 2024
b6aeec1
add missing test
ronami Nov 16, 2024
2b0f1a7
remove unnecessary export syntax from tests
ronami Nov 16, 2024
bb9eeec
correct a comment
ronami Nov 16, 2024
53cdca6
fix unknown => any on a test
ronami Nov 16, 2024
d433280
move code that requires type checking to run after the static ast checks
ronami Nov 27, 2024
a67b11c
refactor code to not use a double-negative check
ronami Nov 27, 2024
8f790ba
remove the unnecessary double check of allowAny
ronami Nov 27, 2024
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
156 changes: 82 additions & 74 deletions packages/eslint-plugin/src/rules/promise-function-async.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,26 +115,6 @@ export default createRule<Options, MessageIds>({
| TSESTree.FunctionDeclaration
| TSESTree.FunctionExpression,
): void {
const signatures = services.getTypeAtLocation(node).getCallSignatures();
if (!signatures.length) {
return;
}
const returnType = checker.getReturnTypeOfSignature(signatures[0]);

if (
!containsAllTypesByName(
returnType,
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
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
return;
}

if (node.parent.type === AST_NODE_TYPES.TSAbstractMethodDefinition) {
// Abstract method can't be async
return;
Expand All @@ -149,7 +129,21 @@ export default createRule<Options, MessageIds>({
return;
}

if (isTypeFlagSet(returnType, ts.TypeFlags.Any | ts.TypeFlags.Unknown)) {
const signatures = services.getTypeAtLocation(node).getCallSignatures();
if (!signatures.length) {
return;
}

const returnTypes = signatures.map(signature =>
checker.getReturnTypeOfSignature(signature),
);

if (
!allowAny &&
returnTypes.some(type =>
isTypeFlagSet(type, ts.TypeFlags.Any | ts.TypeFlags.Unknown),
)
) {
// Report without auto fixer because the return type is unknown
return context.report({
loc: getFunctionHeadLoc(node, context.sourceCode),
Expand All @@ -158,67 +152,81 @@ export default createRule<Options, MessageIds>({
});
}

context.report({
loc: getFunctionHeadLoc(node, context.sourceCode),
node,
messageId: 'missingAsync',
fix: fixer => {
if (
node.parent.type === AST_NODE_TYPES.MethodDefinition ||
(node.parent.type === AST_NODE_TYPES.Property && node.parent.method)
) {
// this function is a class method or object function property shorthand
const method = node.parent;

// the token to put `async` before
let keyToken = nullThrows(
context.sourceCode.getFirstToken(method),
NullThrowsReasons.MissingToken('key token', 'method'),
);

// if there are decorators then skip past them
if (
// require all potential return types to be promise/any/unknown
returnTypes.every(type =>
containsAllTypesByName(
type,
true,
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,
),
)
) {
context.report({
loc: getFunctionHeadLoc(node, context.sourceCode),
node,
messageId: 'missingAsync',
fix: fixer => {
if (
method.type === AST_NODE_TYPES.MethodDefinition &&
method.decorators.length
node.parent.type === AST_NODE_TYPES.MethodDefinition ||
(node.parent.type === AST_NODE_TYPES.Property &&
node.parent.method)
) {
const lastDecorator =
method.decorators[method.decorators.length - 1];
keyToken = nullThrows(
context.sourceCode.getTokenAfter(lastDecorator),
NullThrowsReasons.MissingToken('key token', 'last decorator'),
);
}
// this function is a class method or object function property shorthand
const method = node.parent;

// if current token is a keyword like `static` or `public` then skip it
while (
keyToken.type === AST_TOKEN_TYPES.Keyword &&
keyToken.range[0] < method.key.range[0]
) {
keyToken = nullThrows(
context.sourceCode.getTokenAfter(keyToken),
NullThrowsReasons.MissingToken('token', 'keyword'),
// the token to put `async` before
let keyToken = nullThrows(
context.sourceCode.getFirstToken(method),
NullThrowsReasons.MissingToken('key token', 'method'),
);
}

// check if there is a space between key and previous token
const insertSpace = !context.sourceCode.isSpaceBetween(
nullThrows(
context.sourceCode.getTokenBefore(keyToken),
NullThrowsReasons.MissingToken('token', 'keyword'),
),
keyToken,
);
// if there are decorators then skip past them
if (
method.type === AST_NODE_TYPES.MethodDefinition &&
method.decorators.length
) {
const lastDecorator =
method.decorators[method.decorators.length - 1];
keyToken = nullThrows(
context.sourceCode.getTokenAfter(lastDecorator),
NullThrowsReasons.MissingToken('key token', 'last decorator'),
);
}

// if current token is a keyword like `static` or `public` then skip it
while (
keyToken.type === AST_TOKEN_TYPES.Keyword &&
keyToken.range[0] < method.key.range[0]
) {
keyToken = nullThrows(
context.sourceCode.getTokenAfter(keyToken),
NullThrowsReasons.MissingToken('token', 'keyword'),
);
}

// check if there is a space between key and previous token
const insertSpace = !context.sourceCode.isSpaceBetween(
nullThrows(
context.sourceCode.getTokenBefore(keyToken),
NullThrowsReasons.MissingToken('token', 'keyword'),
),
keyToken,
);

let code = 'async ';
if (insertSpace) {
code = ` ${code}`;
let code = 'async ';
if (insertSpace) {
code = ` ${code}`;
}
return fixer.insertTextBefore(keyToken, code);
}
return fixer.insertTextBefore(keyToken, code);
}

return fixer.insertTextBefore(node, 'async ');
},
});
return fixer.insertTextBefore(node, 'async ');
},
});
}
}

return {
Expand Down
101 changes: 101 additions & 0 deletions packages/eslint-plugin/tests/rules/promise-function-async.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,52 @@ async function asyncFunctionReturningUnion(p: boolean) {
return p ? Promise.resolve(5) : 5;
}
`,
`
function overloadingThatCanReturnPromise(): Promise<number>;
function overloadingThatCanReturnPromise(a: boolean): number;
function overloadingThatCanReturnPromise(
a?: boolean,
): Promise<number> | number {
return Promise.resolve(5);
}
`,
`
function overloadingThatCanReturnPromise(a: boolean): number;
function overloadingThatCanReturnPromise(): Promise<number>;
function overloadingThatCanReturnPromise(
a?: boolean,
): Promise<number> | number {
return Promise.resolve(5);
}
`,
`
function a(): Promise<void>;
function a(x: boolean): void;
function a(x?: boolean) {
if (x == null) return Promise.reject(new Error());
throw new Error();
}
`,
{
code: `
function overloadingThatIncludeUnknown(): number;
function overloadingThatIncludeUnknown(a: boolean): unknown;
function overloadingThatIncludeUnknown(a?: boolean): unknown | number {
return Promise.resolve(5);
}
`,
options: [{ allowAny: true }],
},
{
code: `
function overloadingThatIncludeAny(): number;
function overloadingThatIncludeAny(a: boolean): any;
function overloadingThatIncludeAny(a?: boolean): any | number {
return Promise.resolve(5);
}
`,
options: [{ allowAny: true }],
},
],
invalid: [
{
Expand Down Expand Up @@ -788,5 +834,60 @@ async function promiseInUnionWithoutExplicitReturnType(p: boolean) {
}
`,
},
{
code: `
function overloadingThatCanReturnPromise(): Promise<number>;
function overloadingThatCanReturnPromise(a: boolean): Promise<string>;
function overloadingThatCanReturnPromise(
a?: boolean,
): Promise<number | string> {
return Promise.resolve(5);
}
`,
errors: [
{
messageId,
},
],
output: `
function overloadingThatCanReturnPromise(): Promise<number>;
function overloadingThatCanReturnPromise(a: boolean): Promise<string>;
async function overloadingThatCanReturnPromise(
a?: boolean,
): Promise<number | string> {
return Promise.resolve(5);
}
`,
},
{
code: `
function overloadingThatIncludeAny(): number;
function overloadingThatIncludeAny(a: boolean): any;
function overloadingThatIncludeAny(a?: boolean): any | number {
return Promise.resolve(5);
}
`,
errors: [
{
messageId,
},
],
options: [{ allowAny: false }],
},
{
code: `
function overloadingThatIncludeUnknown(): number;
function overloadingThatIncludeUnknown(a: boolean): unknown;
function overloadingThatIncludeUnknown(a?: boolean): unknown | number {
return Promise.resolve(5);
}
`,
errors: [
{
messageId,
},
],
options: [{ allowAny: false }],
},
],
});
Loading