Skip to content

Commit 68311ee

Browse files
ronamironami
authored
fix(eslint-plugin): [promise-function-async] handle function overloading (typescript-eslint#10304)
* initial implementation * remove unnecessary comment * rework the code to use a single signature * reduce changes footprint * reduce changes footprint further * use nullThrows rather than an unreachable conditional * revert implementation to check every function signature * add missing test * remove unnecessary export syntax from tests * correct a comment * fix unknown => any on a test * move code that requires type checking to run after the static ast checks * refactor code to not use a double-negative check * remove the unnecessary double check of allowAny
1 parent 05617d2 commit 68311ee

File tree

2 files changed

+183
-74
lines changed

2 files changed

+183
-74
lines changed

‎packages/eslint-plugin/src/rules/promise-function-async.ts

Lines changed: 82 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -115,26 +115,6 @@ export default createRule<Options, MessageIds>({
115115
| TSESTree.FunctionDeclaration
116116
| TSESTree.FunctionExpression,
117117
): void {
118-
const signatures = services.getTypeAtLocation(node).getCallSignatures();
119-
if (!signatures.length) {
120-
return;
121-
}
122-
const returnType = checker.getReturnTypeOfSignature(signatures[0]);
123-
124-
if (
125-
!containsAllTypesByName(
126-
returnType,
127-
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
128-
allowAny!,
129-
allAllowedPromiseNames,
130-
// 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).
131-
node.returnType == null,
132-
)
133-
) {
134-
// Return type is not a promise
135-
return;
136-
}
137-
138118
if (node.parent.type === AST_NODE_TYPES.TSAbstractMethodDefinition) {
139119
// Abstract method can't be async
140120
return;
@@ -149,7 +129,21 @@ export default createRule<Options, MessageIds>({
149129
return;
150130
}
151131

152-
if (isTypeFlagSet(returnType, ts.TypeFlags.Any | ts.TypeFlags.Unknown)) {
132+
const signatures = services.getTypeAtLocation(node).getCallSignatures();
133+
if (!signatures.length) {
134+
return;
135+
}
136+
137+
const returnTypes = signatures.map(signature =>
138+
checker.getReturnTypeOfSignature(signature),
139+
);
140+
141+
if (
142+
!allowAny &&
143+
returnTypes.some(type =>
144+
isTypeFlagSet(type, ts.TypeFlags.Any | ts.TypeFlags.Unknown),
145+
)
146+
) {
153147
// Report without auto fixer because the return type is unknown
154148
return context.report({
155149
loc: getFunctionHeadLoc(node, context.sourceCode),
@@ -158,67 +152,81 @@ export default createRule<Options, MessageIds>({
158152
});
159153
}
160154

161-
context.report({
162-
loc: getFunctionHeadLoc(node, context.sourceCode),
163-
node,
164-
messageId: 'missingAsync',
165-
fix: fixer => {
166-
if (
167-
node.parent.type === AST_NODE_TYPES.MethodDefinition ||
168-
(node.parent.type === AST_NODE_TYPES.Property && node.parent.method)
169-
) {
170-
// this function is a class method or object function property shorthand
171-
const method = node.parent;
172-
173-
// the token to put `async` before
174-
let keyToken = nullThrows(
175-
context.sourceCode.getFirstToken(method),
176-
NullThrowsReasons.MissingToken('key token', 'method'),
177-
);
178-
179-
// if there are decorators then skip past them
155+
if (
156+
// require all potential return types to be promise/any/unknown
157+
returnTypes.every(type =>
158+
containsAllTypesByName(
159+
type,
160+
true,
161+
allAllowedPromiseNames,
162+
// 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).
163+
node.returnType == null,
164+
),
165+
)
166+
) {
167+
context.report({
168+
loc: getFunctionHeadLoc(node, context.sourceCode),
169+
node,
170+
messageId: 'missingAsync',
171+
fix: fixer => {
180172
if (
181-
method.type === AST_NODE_TYPES.MethodDefinition &&
182-
method.decorators.length
173+
node.parent.type === AST_NODE_TYPES.MethodDefinition ||
174+
(node.parent.type === AST_NODE_TYPES.Property &&
175+
node.parent.method)
183176
) {
184-
const lastDecorator =
185-
method.decorators[method.decorators.length - 1];
186-
keyToken = nullThrows(
187-
context.sourceCode.getTokenAfter(lastDecorator),
188-
NullThrowsReasons.MissingToken('key token', 'last decorator'),
189-
);
190-
}
177+
// this function is a class method or object function property shorthand
178+
const method = node.parent;
191179

192-
// if current token is a keyword like `static` or `public` then skip it
193-
while (
194-
keyToken.type === AST_TOKEN_TYPES.Keyword &&
195-
keyToken.range[0] < method.key.range[0]
196-
) {
197-
keyToken = nullThrows(
198-
context.sourceCode.getTokenAfter(keyToken),
199-
NullThrowsReasons.MissingToken('token', 'keyword'),
180+
// the token to put `async` before
181+
let keyToken = nullThrows(
182+
context.sourceCode.getFirstToken(method),
183+
NullThrowsReasons.MissingToken('key token', 'method'),
200184
);
201-
}
202185

203-
// check if there is a space between key and previous token
204-
const insertSpace = !context.sourceCode.isSpaceBetween(
205-
nullThrows(
206-
context.sourceCode.getTokenBefore(keyToken),
207-
NullThrowsReasons.MissingToken('token', 'keyword'),
208-
),
209-
keyToken,
210-
);
186+
// if there are decorators then skip past them
187+
if (
188+
method.type === AST_NODE_TYPES.MethodDefinition &&
189+
method.decorators.length
190+
) {
191+
const lastDecorator =
192+
method.decorators[method.decorators.length - 1];
193+
keyToken = nullThrows(
194+
context.sourceCode.getTokenAfter(lastDecorator),
195+
NullThrowsReasons.MissingToken('key token', 'last decorator'),
196+
);
197+
}
198+
199+
// if current token is a keyword like `static` or `public` then skip it
200+
while (
201+
keyToken.type === AST_TOKEN_TYPES.Keyword &&
202+
keyToken.range[0] < method.key.range[0]
203+
) {
204+
keyToken = nullThrows(
205+
context.sourceCode.getTokenAfter(keyToken),
206+
NullThrowsReasons.MissingToken('token', 'keyword'),
207+
);
208+
}
209+
210+
// check if there is a space between key and previous token
211+
const insertSpace = !context.sourceCode.isSpaceBetween(
212+
nullThrows(
213+
context.sourceCode.getTokenBefore(keyToken),
214+
NullThrowsReasons.MissingToken('token', 'keyword'),
215+
),
216+
keyToken,
217+
);
211218

212-
let code = 'async ';
213-
if (insertSpace) {
214-
code = ` ${code}`;
219+
let code = 'async ';
220+
if (insertSpace) {
221+
code = ` ${code}`;
222+
}
223+
return fixer.insertTextBefore(keyToken, code);
215224
}
216-
return fixer.insertTextBefore(keyToken, code);
217-
}
218225

219-
return fixer.insertTextBefore(node, 'async ');
220-
},
221-
});
226+
return fixer.insertTextBefore(node, 'async ');
227+
},
228+
});
229+
}
222230
}
223231

224232
return {

‎packages/eslint-plugin/tests/rules/promise-function-async.test.ts

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,52 @@ async function asyncFunctionReturningUnion(p: boolean) {
182182
return p ? Promise.resolve(5) : 5;
183183
}
184184
`,
185+
`
186+
function overloadingThatCanReturnPromise(): Promise<number>;
187+
function overloadingThatCanReturnPromise(a: boolean): number;
188+
function overloadingThatCanReturnPromise(
189+
a?: boolean,
190+
): Promise<number> | number {
191+
return Promise.resolve(5);
192+
}
193+
`,
194+
`
195+
function overloadingThatCanReturnPromise(a: boolean): number;
196+
function overloadingThatCanReturnPromise(): Promise<number>;
197+
function overloadingThatCanReturnPromise(
198+
a?: boolean,
199+
): Promise<number> | number {
200+
return Promise.resolve(5);
201+
}
202+
`,
203+
`
204+
function a(): Promise<void>;
205+
function a(x: boolean): void;
206+
function a(x?: boolean) {
207+
if (x == null) return Promise.reject(new Error());
208+
throw new Error();
209+
}
210+
`,
211+
{
212+
code: `
213+
function overloadingThatIncludeUnknown(): number;
214+
function overloadingThatIncludeUnknown(a: boolean): unknown;
215+
function overloadingThatIncludeUnknown(a?: boolean): unknown | number {
216+
return Promise.resolve(5);
217+
}
218+
`,
219+
options: [{ allowAny: true }],
220+
},
221+
{
222+
code: `
223+
function overloadingThatIncludeAny(): number;
224+
function overloadingThatIncludeAny(a: boolean): any;
225+
function overloadingThatIncludeAny(a?: boolean): any | number {
226+
return Promise.resolve(5);
227+
}
228+
`,
229+
options: [{ allowAny: true }],
230+
},
185231
],
186232
invalid: [
187233
{
@@ -788,5 +834,60 @@ async function promiseInUnionWithoutExplicitReturnType(p: boolean) {
788834
}
789835
`,
790836
},
837+
{
838+
code: `
839+
function overloadingThatCanReturnPromise(): Promise<number>;
840+
function overloadingThatCanReturnPromise(a: boolean): Promise<string>;
841+
function overloadingThatCanReturnPromise(
842+
a?: boolean,
843+
): Promise<number | string> {
844+
return Promise.resolve(5);
845+
}
846+
`,
847+
errors: [
848+
{
849+
messageId,
850+
},
851+
],
852+
output: `
853+
function overloadingThatCanReturnPromise(): Promise<number>;
854+
function overloadingThatCanReturnPromise(a: boolean): Promise<string>;
855+
async function overloadingThatCanReturnPromise(
856+
a?: boolean,
857+
): Promise<number | string> {
858+
return Promise.resolve(5);
859+
}
860+
`,
861+
},
862+
{
863+
code: `
864+
function overloadingThatIncludeAny(): number;
865+
function overloadingThatIncludeAny(a: boolean): any;
866+
function overloadingThatIncludeAny(a?: boolean): any | number {
867+
return Promise.resolve(5);
868+
}
869+
`,
870+
errors: [
871+
{
872+
messageId,
873+
},
874+
],
875+
options: [{ allowAny: false }],
876+
},
877+
{
878+
code: `
879+
function overloadingThatIncludeUnknown(): number;
880+
function overloadingThatIncludeUnknown(a: boolean): unknown;
881+
function overloadingThatIncludeUnknown(a?: boolean): unknown | number {
882+
return Promise.resolve(5);
883+
}
884+
`,
885+
errors: [
886+
{
887+
messageId,
888+
},
889+
],
890+
options: [{ allowAny: false }],
891+
},
791892
],
792893
});

0 commit comments

Comments
 (0)