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): Check 'rest' parameters in no-misused-promises #5731

Merged
Merged
Show file tree
Hide file tree
Changes from 3 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
95 changes: 82 additions & 13 deletions packages/eslint-plugin/src/rules/no-misused-promises.ts
Expand Up @@ -213,13 +213,13 @@ export default util.createRule<Options, MessageId>({
node: TSESTree.CallExpression | TSESTree.NewExpression,
): void {
const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node);
const voidParams = voidFunctionParams(checker, tsNode);
if (voidParams.size === 0) {
const voidArgs = voidFunctionArguments(checker, tsNode);
if (voidArgs.size === 0) {
return;
}

for (const [index, argument] of node.arguments.entries()) {
if (!voidParams.has(index)) {
if (!voidArgs.has(index)) {
continue;
}

Expand Down Expand Up @@ -486,13 +486,40 @@ function isFunctionParam(
return false;
}

// Get the positions of parameters which are void functions (and not also
function checkThenableOrVoid(
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
checker: ts.TypeChecker,
node: ts.CallExpression | ts.NewExpression,
type: ts.Type,
index: number,
thenableReturnIndices: Set<number>,
voidReturnIndices: Set<number>,
): void {
if (isThenableReturningFunctionType(checker, node.expression, type)) {
thenableReturnIndices.add(index);
} else if (isVoidReturningFunctionType(checker, node.expression, type)) {
// If a certain argument accepts both thenable and void returns,
// a promise-returning function is valid
if (!thenableReturnIndices.has(index)) {
voidReturnIndices.add(index);
}
}
}

// Get the positions of arguments which are void functions (and not also
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
// thenable functions). These are the candidates for the void-return check at
// the current call site.
function voidFunctionParams(
// If the function parameters end with a 'rest' parameter, then we consider
// the array type parameter (e.g. '...args:Array<SomeType>') when determining
// if trailing arguments are candidates.
function voidFunctionArguments(
checker: ts.TypeChecker,
node: ts.CallExpression | ts.NewExpression,
): Set<number> {
// 'new' can be used without any arguments, as in 'let b = new Object;'
// In this case, there are no argument positions to check, so return early.
if (!node.arguments) {
return new Set<number>();
}
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
const thenableReturnIndices = new Set<number>();
const voidReturnIndices = new Set<number>();
const type = checker.getTypeAtLocation(node.expression);
Expand All @@ -507,17 +534,59 @@ function voidFunctionParams(
: subType.getConstructSignatures();
for (const signature of signatures) {
for (const [index, parameter] of signature.parameters.entries()) {
const type = checker.getTypeOfSymbolAtLocation(
const decl = parameter.valueDeclaration;
let type = checker.getTypeOfSymbolAtLocation(
parameter,
node.expression,
);
if (isThenableReturningFunctionType(checker, node.expression, type)) {
thenableReturnIndices.add(index);
} else if (
!thenableReturnIndices.has(index) &&
isVoidReturningFunctionType(checker, node.expression, type)
) {
voidReturnIndices.add(index);

// If this is a array 'rest' parameter, check all of the argument indices
// from the current argument to the end.
// Note - we currently do not support 'spread' arguments - adding support for them
// is tracked in https://github.com/typescript-eslint/typescript-eslint/issues/5744
if (decl && ts.isParameter(decl) && decl.dotDotDotToken) {
if (checker.isArrayType(type)) {
// Unwrap 'Array<MaybeVoidFunction>' to 'MaybeVoidFunction',
// so that we'll handle it in the same way as a non-rest
// 'param: MaybeVoidFunction'
type = checker.getTypeArguments(type)[0];
for (let i = index; i < node.arguments.length; i++) {
checkThenableOrVoid(
checker,
node,
type,
i,
thenableReturnIndices,
voidReturnIndices,
);
}
} else if (checker.isTupleType(type)) {
// Check each type in the tuple - for example, [boolean, () => void] would
// add the index of the second tuple parameter to 'voidReturnIndices'
const typeArgs = checker.getTypeArguments(type);
for (let i = index; i < node.arguments.length; i++) {
checkThenableOrVoid(
checker,
node,
typeArgs[i - index],
i,
thenableReturnIndices,
voidReturnIndices,
);
}
} else {
// We must have an 'any' parameter (e.g. `...args: any`), as anything
// else would be invalid TypeScript. There's nothing for us to check.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
checkThenableOrVoid(
checker,
node,
type,
index,
thenableReturnIndices,
voidReturnIndices,
);
}
}
}
Expand Down
108 changes: 108 additions & 0 deletions packages/eslint-plugin/tests/rules/no-misused-promises.test.ts
Expand Up @@ -373,6 +373,41 @@ console.log([...Promise.resolve(42)]);
`,
options: [{ checksSpreads: false }],
},
`
function spreadAny(..._args: any): void {}
function spreadArrayAny(..._args: Array<any>): void {}
function spreadArrayUnknown(..._args: Array<unknown>): void {}
function spreadArrayFuncPromise(
..._args: Array<() => Promise<undefined>>
): void {}

spreadAny(
true,
() => Promise.resolve(1),
() => Promise.resolve(false),
);
spreadArrayAny(
true,
() => Promise.resolve(1),
() => Promise.resolve(false),
);
spreadArrayUnknown(() => Promise.resolve(true), 1, 2);
spreadArrayFuncPromise(
() => Promise.resolve(undefined),
() => Promise.resolve(undefined),
);
`,
`
class TakeCallbacks {
constructor(...callbacks: Array<() => void>) {}
}

new TakeCallbacks();
new TakeCallbacks(
() => 1,
() => true,
);
`,
],

invalid: [
Expand Down Expand Up @@ -970,5 +1005,78 @@ console.log({ ...(condition ? Promise.resolve({ key: 42 }) : {}) });
{ line: 7, messageId: 'spread' },
],
},
{
code: `
function restPromises(first: Boolean, ...callbacks: Array<() => void>): void {}

restPromises(
true,
() => Promise.resolve(true),
() => Promise.resolve(null),
() => true,
() => Promise.resolve('Hello'),
);
`,
errors: [
{ line: 6, messageId: 'voidReturnArgument' },
{ line: 7, messageId: 'voidReturnArgument' },
{ line: 9, messageId: 'voidReturnArgument' },
],
},
{
code: `
type MyUnion = (() => void) | boolean;

function restUnion(first: string, ...callbacks: Array<MyUnion>): void {}
restUnion('Testing', false, () => Promise.resolve(true));
`,
errors: [{ line: 5, messageId: 'voidReturnArgument' }],
},
{
code: `
function restTupleOne(first: string, ...callbacks: [() => void]): void {}

function restTupleTwo(
first: boolean,
...callbacks: [undefined, () => void, undefined]
): void {}

function restTupleFour(
first: number,
...callbacks: [() => void, boolean, () => void, () => void]
): void;

restTupleOne('My string', () => Promise.resolve(1));
restTupleTwo(true, undefined, () => Promise.resolve(true), undefined);
restTupleFour(
1,
() => Promise.resolve(true),
false,
() => {},
() => Promise.resolve(1),
);
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
`,
errors: [
{ line: 14, messageId: 'voidReturnArgument' },
{ line: 15, messageId: 'voidReturnArgument' },
{ line: 18, messageId: 'voidReturnArgument' },
{ line: 21, messageId: 'voidReturnArgument' },
],
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
},
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
{
code: `
class TakesVoidCb {
constructor(first: string, ...args: Array<() => void>);
}

new TakesVoidCb();
new TakesVoidCb(
'Testing',
() => {},
() => Promise.resolve(true),
);
`,
errors: [{ line: 10, messageId: 'voidReturnArgument' }],
},
],
});