From 0bb293b703663e2e78969be7b9685db258e40023 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 30 Sep 2022 13:47:23 -0500 Subject: [PATCH 1/5] feat(eslint-plugin): Check 'rest' parameters in no-misused-promises Fixes #4015 This extends 'no-misued-promises' with support for checking variadic arguments passed to a 'rest' parameter. If a function is declared with an argument like '...handlers: Array<() => void>', we now check if the type argument to `Array` is a void-returning function, and if so, check if any of the variadic arguments return a Promise. --- .../src/rules/no-misused-promises.ts | 45 +++++++++++++++---- .../tests/rules/no-misused-promises.test.ts | 24 ++++++++++ 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-misused-promises.ts b/packages/eslint-plugin/src/rules/no-misused-promises.ts index 2c7f1eeb259..fdf13f14ed6 100644 --- a/packages/eslint-plugin/src/rules/no-misused-promises.ts +++ b/packages/eslint-plugin/src/rules/no-misused-promises.ts @@ -213,13 +213,13 @@ export default util.createRule({ 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; } @@ -486,10 +486,13 @@ function isFunctionParam( return false; } -// Get the positions of parameters which are void functions (and not also +// Get the positions of arguments which are void functions (and not also // 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') when determining +// if trailing arguments are candidates. +function voidFunctionArguments( checker: ts.TypeChecker, node: ts.CallExpression | ts.NewExpression, ): Set { @@ -507,17 +510,41 @@ function voidFunctionParams( : subType.getConstructSignatures(); for (const signature of signatures) { for (const [index, parameter] of signature.parameters.entries()) { - const type = checker.getTypeOfSymbolAtLocation( + const decl = parameter.getDeclarations()?.[0]; + let type = checker.getTypeOfSymbolAtLocation( parameter, node.expression, ); + + const indices = [index]; + // If this is a array 'rest' parameter, add all of the remaining parameter indices. + // Note - we currently do not support 'spread' arguments + if ( + decl && + ts.isParameter(decl) && + decl.dotDotDotToken && + checker.isArrayType(type) && + node.arguments + ) { + for (let i = index + 1; i < node.arguments.length; i++) { + indices.push(i); + } + // Unwrap 'Array' to 'MaybeVoidFunction', + // so that we'll handle it in the same way as a non-rest + // 'param: MaybeVoidFunction' + type = checker.getTypeArguments(type)[0]; + } + if (isThenableReturningFunctionType(checker, node.expression, type)) { - thenableReturnIndices.add(index); + indices.forEach(index => thenableReturnIndices.add(index)); } else if ( - !thenableReturnIndices.has(index) && isVoidReturningFunctionType(checker, node.expression, type) ) { - voidReturnIndices.add(index); + indices.forEach(index => { + if (!thenableReturnIndices.has(index)) { + voidReturnIndices.add(index); + } + }); } } } diff --git a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts index 55ba429cdb0..340f64bb898 100644 --- a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts @@ -970,5 +970,29 @@ console.log({ ...(condition ? Promise.resolve({ key: 42 }) : {}) }); { line: 7, messageId: 'spread' }, ], }, + { + code: ` +type MyUnion = (() => void) | boolean; + +function restPromises(first: Boolean, ...callbacks: Array<() => void>): void {} +function restUnion(first: string, ...callbacks: Array): void {} + +restPromises( + true, + () => Promise.resolve(true), + () => Promise.resolve(null), + () => true, + () => Promise.resolve('Hello'), +); + +restUnion('Testing', false, () => Promise.resolve(true)); + `, + errors: [ + { line: 9, messageId: 'voidReturnArgument' }, + { line: 10, messageId: 'voidReturnArgument' }, + { line: 12, messageId: 'voidReturnArgument' }, + { line: 15, messageId: 'voidReturnArgument' }, + ], + }, ], }); From aac48e9487fc2a44a4692fbe5d03d65be81814f3 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 4 Oct 2022 10:37:12 -0500 Subject: [PATCH 2/5] Address review comments --- .../src/rules/no-misused-promises.ts | 100 +++++++++++++----- .../tests/rules/no-misused-promises.test.ts | 96 +++++++++++++++-- 2 files changed, 161 insertions(+), 35 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-misused-promises.ts b/packages/eslint-plugin/src/rules/no-misused-promises.ts index fdf13f14ed6..58051c660f2 100644 --- a/packages/eslint-plugin/src/rules/no-misused-promises.ts +++ b/packages/eslint-plugin/src/rules/no-misused-promises.ts @@ -486,6 +486,25 @@ function isFunctionParam( return false; } +function checkThenableOrVoid( + checker: ts.TypeChecker, + node: ts.CallExpression | ts.NewExpression, + type: ts.Type, + index: number, + thenableReturnIndices: Set, + voidReturnIndices: Set, +): 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 // thenable functions). These are the candidates for the void-return check at // the current call site. @@ -496,6 +515,11 @@ function voidFunctionArguments( checker: ts.TypeChecker, node: ts.CallExpression | ts.NewExpression, ): Set { + // '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(); + } const thenableReturnIndices = new Set(); const voidReturnIndices = new Set(); const type = checker.getTypeAtLocation(node.expression); @@ -510,41 +534,59 @@ function voidFunctionArguments( : subType.getConstructSignatures(); for (const signature of signatures) { for (const [index, parameter] of signature.parameters.entries()) { - const decl = parameter.getDeclarations()?.[0]; + const decl = parameter.valueDeclaration; let type = checker.getTypeOfSymbolAtLocation( parameter, node.expression, ); - const indices = [index]; - // If this is a array 'rest' parameter, add all of the remaining parameter indices. - // Note - we currently do not support 'spread' arguments - if ( - decl && - ts.isParameter(decl) && - decl.dotDotDotToken && - checker.isArrayType(type) && - node.arguments - ) { - for (let i = index + 1; i < node.arguments.length; i++) { - indices.push(i); - } - // Unwrap 'Array' to 'MaybeVoidFunction', - // so that we'll handle it in the same way as a non-rest - // 'param: MaybeVoidFunction' - type = checker.getTypeArguments(type)[0]; - } - - if (isThenableReturningFunctionType(checker, node.expression, type)) { - indices.forEach(index => thenableReturnIndices.add(index)); - } else if ( - isVoidReturningFunctionType(checker, node.expression, type) - ) { - indices.forEach(index => { - if (!thenableReturnIndices.has(index)) { - voidReturnIndices.add(index); + // If this is a array 'rest' parameter, check all of the argument indicies + // 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' 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. + } + } else { + checkThenableOrVoid( + checker, + node, + type, + index, + thenableReturnIndices, + voidReturnIndices, + ); } } } diff --git a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts index 340f64bb898..b3a8d3aa7a7 100644 --- a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts @@ -373,6 +373,41 @@ console.log([...Promise.resolve(42)]); `, options: [{ checksSpreads: false }], }, + ` +function spreadAny(..._args: any): void {} +function spreadArrayAny(..._args: Array): void {} +function spreadArrayUnknown(..._args: Array): void {} +function spreadArrayFuncPromise( + ..._args: Array<() => Promise> +): 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: [ @@ -972,10 +1007,7 @@ console.log({ ...(condition ? Promise.resolve({ key: 42 }) : {}) }); }, { code: ` -type MyUnion = (() => void) | boolean; - function restPromises(first: Boolean, ...callbacks: Array<() => void>): void {} -function restUnion(first: string, ...callbacks: Array): void {} restPromises( true, @@ -984,15 +1016,67 @@ restPromises( () => 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): 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), +); `, errors: [ - { line: 9, messageId: 'voidReturnArgument' }, - { line: 10, messageId: 'voidReturnArgument' }, - { line: 12, messageId: 'voidReturnArgument' }, + { line: 14, messageId: 'voidReturnArgument' }, { line: 15, messageId: 'voidReturnArgument' }, + { line: 18, messageId: 'voidReturnArgument' }, + { line: 21, messageId: 'voidReturnArgument' }, ], }, + { + code: ` +class TakesVoidCb { + constructor(first: string, ...args: Array<() => void>); +} + +new TakesVoidCb(); +new TakesVoidCb( + 'Testing', + () => {}, + () => Promise.resolve(true), +); + `, + errors: [{ line: 10, messageId: 'voidReturnArgument' }], + }, ], }); From 5f915512ccbdcde6e4290066980fad25d91e0ba6 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 4 Oct 2022 12:51:35 -0500 Subject: [PATCH 3/5] Fix spelling --- packages/eslint-plugin/src/rules/no-misused-promises.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/no-misused-promises.ts b/packages/eslint-plugin/src/rules/no-misused-promises.ts index 58051c660f2..890cea62ffc 100644 --- a/packages/eslint-plugin/src/rules/no-misused-promises.ts +++ b/packages/eslint-plugin/src/rules/no-misused-promises.ts @@ -540,7 +540,7 @@ function voidFunctionArguments( node.expression, ); - // If this is a array 'rest' parameter, check all of the argument indicies + // 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 From 0f01987182e9a9ddd8030f517edfdb8fe074e791 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 5 Oct 2022 09:35:58 -0500 Subject: [PATCH 4/5] Address additional review comments --- .../src/rules/no-misused-promises.ts | 11 +++----- .../tests/rules/no-misused-promises.test.ts | 27 +++++++++++++------ 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-misused-promises.ts b/packages/eslint-plugin/src/rules/no-misused-promises.ts index 890cea62ffc..7afb62785b5 100644 --- a/packages/eslint-plugin/src/rules/no-misused-promises.ts +++ b/packages/eslint-plugin/src/rules/no-misused-promises.ts @@ -486,7 +486,7 @@ function isFunctionParam( return false; } -function checkThenableOrVoid( +function checkThenableOrVoidArgument( checker: ts.TypeChecker, node: ts.CallExpression | ts.NewExpression, type: ts.Type, @@ -551,7 +551,7 @@ function voidFunctionArguments( // 'param: MaybeVoidFunction' type = checker.getTypeArguments(type)[0]; for (let i = index; i < node.arguments.length; i++) { - checkThenableOrVoid( + checkThenableOrVoidArgument( checker, node, type, @@ -565,7 +565,7 @@ function voidFunctionArguments( // 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( + checkThenableOrVoidArgument( checker, node, typeArgs[i - index], @@ -574,12 +574,9 @@ function voidFunctionArguments( 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. } } else { - checkThenableOrVoid( + checkThenableOrVoidArgument( checker, node, type, diff --git a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts index b3a8d3aa7a7..b7c803a99d9 100644 --- a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts @@ -397,11 +397,13 @@ spreadArrayFuncPromise( () => Promise.resolve(undefined), ); `, + // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting ` class TakeCallbacks { constructor(...callbacks: Array<() => void>) {} } +new TakeCallbacks; new TakeCallbacks(); new TakeCallbacks( () => 1, @@ -1035,19 +1037,28 @@ restUnion('Testing', false, () => Promise.resolve(true)); { code: ` function restTupleOne(first: string, ...callbacks: [() => void]): void {} - +restTupleOne('My string', () => Promise.resolve(1)); + `, + errors: [{ line: 3, messageId: 'voidReturnArgument' }], + }, + { + code: ` function restTupleTwo( first: boolean, ...callbacks: [undefined, () => void, undefined] ): void {} +restTupleTwo(true, undefined, () => Promise.resolve(true), undefined); + `, + errors: [{ line: 7, messageId: 'voidReturnArgument' }], + }, + { + code: ` 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), @@ -1057,18 +1068,18 @@ restTupleFour( ); `, errors: [ - { line: 14, messageId: 'voidReturnArgument' }, - { line: 15, messageId: 'voidReturnArgument' }, - { line: 18, messageId: 'voidReturnArgument' }, - { line: 21, messageId: 'voidReturnArgument' }, + { line: 9, messageId: 'voidReturnArgument' }, + { line: 12, messageId: 'voidReturnArgument' }, ], }, { + // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting code: ` class TakesVoidCb { constructor(first: string, ...args: Array<() => void>); } +new TakesVoidCb; new TakesVoidCb(); new TakesVoidCb( 'Testing', @@ -1076,7 +1087,7 @@ new TakesVoidCb( () => Promise.resolve(true), ); `, - errors: [{ line: 10, messageId: 'voidReturnArgument' }], + errors: [{ line: 11, messageId: 'voidReturnArgument' }], }, ], }); From 3a3bb21c9ffbd97318504c1289e968350bd213cc Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 5 Oct 2022 20:34:04 -0400 Subject: [PATCH 5/5] nit: split up tests a bit, and add comments about () --- .../tests/rules/no-misused-promises.test.ts | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts index b7c803a99d9..e333e1f4e67 100644 --- a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts @@ -375,28 +375,37 @@ console.log([...Promise.resolve(42)]); }, ` function spreadAny(..._args: any): void {} -function spreadArrayAny(..._args: Array): void {} -function spreadArrayUnknown(..._args: Array): void {} -function spreadArrayFuncPromise( - ..._args: Array<() => Promise> -): void {} spreadAny( true, () => Promise.resolve(1), () => Promise.resolve(false), ); + `, + ` +function spreadArrayAny(..._args: Array): void {} + spreadArrayAny( true, () => Promise.resolve(1), () => Promise.resolve(false), ); + `, + ` +function spreadArrayUnknown(..._args: Array): void {} + spreadArrayUnknown(() => Promise.resolve(true), 1, 2); + +function spreadArrayFuncPromise( + ..._args: Array<() => Promise> +): void {} + spreadArrayFuncPromise( () => Promise.resolve(undefined), () => Promise.resolve(undefined), ); `, + // Prettier adds a () but this tests arguments being undefined, not [] // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting ` class TakeCallbacks { @@ -1073,6 +1082,7 @@ restTupleFour( ], }, { + // Prettier adds a () but this tests arguments being undefined, not [] // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting code: ` class TakesVoidCb {