From ae2032a8ba4b3445f9d39284bbca381c7971fc98 Mon Sep 17 00:00:00 2001 From: Dominik Rowicki Date: Mon, 23 May 2022 17:11:22 +0200 Subject: [PATCH 1/4] feat(eslint-plugin): warn when spreading promises --- .../docs/rules/no-misused-promises.md | 59 ++++++++++++++++++- .../src/rules/no-misused-promises.ts | 34 ++++++++++- .../tests/rules/no-misused-promises.test.ts | 55 +++++++++++++++++ 3 files changed, 143 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-misused-promises.md b/packages/eslint-plugin/docs/rules/no-misused-promises.md index b3d5dcbe0cb..24e66dda7df 100644 --- a/packages/eslint-plugin/docs/rules/no-misused-promises.md +++ b/packages/eslint-plugin/docs/rules/no-misused-promises.md @@ -13,9 +13,9 @@ See [`no-floating-promises`](./no-floating-promises.md) for detecting unhandled ## Rule Details -This rule accepts a single option which is an object with `checksConditionals` -and `checksVoidReturn` properties indicating which types of misuse to flag. -Both are enabled by default. +This rule accepts a single option which is an object with `checksConditionals`, +`checksVoidReturn`, and `checkSpreads` properties indicating which types of +misuse to flag. All are enabled by default. ## Options @@ -24,6 +24,7 @@ type Options = [ { checksConditionals?: boolean; checksVoidReturn?: boolean | ChecksVoidReturnOptions; + checkSpreads?: boolean; }, ]; @@ -39,6 +40,7 @@ const defaultOptions: Options = [ { checksConditionals: true, checksVoidReturn: true, + checkSpreads: true, }, ]; ``` @@ -101,6 +103,21 @@ For example, if you don't mind that passing a `() => Promise` to a `() => } ``` +### `"checkSpreads"` + +If you don't want to check object spreads, you can add this configuration: + +```json +{ + "@typescript-eslint/no-misused-promises": [ + "error", + { + "checkSpreads": false + } + ] +} +``` + ### `checksConditionals: true` Examples of code for this rule with `checksConditionals: true`: @@ -212,6 +229,42 @@ eventEmitter.on('some-event', () => { +### `checkSpreads: true` + +Examples of code for this rule with `checkSpreads: true`: + + + +#### ❌ Incorrect + +```ts +const getData = () => someAsyncOperation({ myArg: 'foo' }); + +return { foo: 42, ...getData() }; + +const getData2 = async () => { + await someAsyncOperation({ myArg: 'foo' }); +}; + +return { foo: 42, ...getData2() }; +``` + +#### ✅ Correct + +```ts +const getData = () => someAsyncOperation({ myArg: 'foo' }); + +return { foo: 42, ...(await getData()) }; + +const getData2 = async () => { + await someAsyncOperation({ myArg: 'foo' }); +}; + +return { foo: 42, ...(await getData2()) }; +``` + + + ## When Not To Use It If you do not use Promises in your codebase or are not concerned with possible diff --git a/packages/eslint-plugin/src/rules/no-misused-promises.ts b/packages/eslint-plugin/src/rules/no-misused-promises.ts index 03b166ec49b..8f7d84633c7 100644 --- a/packages/eslint-plugin/src/rules/no-misused-promises.ts +++ b/packages/eslint-plugin/src/rules/no-misused-promises.ts @@ -8,6 +8,7 @@ type Options = [ { checksConditionals?: boolean; checksVoidReturn?: boolean | ChecksVoidReturnOptions; + checksSpreads?: boolean; }, ]; @@ -25,7 +26,8 @@ type MessageId = | 'voidReturnVariable' | 'voidReturnProperty' | 'voidReturnReturnValue' - | 'voidReturnAttribute'; + | 'voidReturnAttribute' + | 'spread'; function parseChecksVoidReturn( checksVoidReturn: boolean | ChecksVoidReturnOptions | undefined, @@ -75,6 +77,7 @@ export default util.createRule({ voidReturnAttribute: 'Promise-returning function provided to attribute where a void return was expected.', conditional: 'Expected non-Promise value in a boolean conditional.', + spread: 'Expected a non-Promise value to be spreaded in an object.', }, schema: [ { @@ -99,6 +102,9 @@ export default util.createRule({ }, ], }, + checksSpreads: { + type: 'boolean', + }, }, }, ], @@ -108,10 +114,11 @@ export default util.createRule({ { checksConditionals: true, checksVoidReturn: true, + checksSpreads: true, }, ], - create(context, [{ checksConditionals, checksVoidReturn }]) { + create(context, [{ checksConditionals, checksVoidReturn, checksSpreads }]) { const parserServices = util.getParserServices(context); const checker = parserServices.program.getTypeChecker(); @@ -153,6 +160,10 @@ export default util.createRule({ } : {}; + const spreadChecks: TSESLint.RuleListener = { + SpreadElement: checkSpread, + }; + function checkTestConditional(node: { test: TSESTree.Expression | null; }): void { @@ -376,9 +387,28 @@ export default util.createRule({ } } + function checkSpread(node: TSESTree.SpreadElement): void { + const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node); + + if ( + node.argument.type === AST_NODE_TYPES.CallExpression && + tsutils.isThenableType( + checker, + tsNode.expression, + checker.getTypeAtLocation(tsNode.expression), + ) + ) { + context.report({ + messageId: 'spread', + node: node.argument, + }); + } + } + return { ...(checksConditionals ? conditionalChecks : {}), ...(checksVoidReturn ? voidReturnChecks : {}), + ...(checksSpreads ? spreadChecks : {}), }; }, }); 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 518b7c1d6b5..2bd834ee9c2 100644 --- a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts @@ -316,6 +316,34 @@ const _ = {}} />; `, filename: 'react.tsx', }, + ` +console.log({ ...(await Promise.resolve({ key: 42 })) }); + `, + ` +const getData = Promise.resolve({ key: 42 }); + +console.log({ + someData: 42, + ...(await getData()), +}); + `, + { + code: ` +console.log({ ...Promise.resolve({ key: 42 }) }); + `, + options: [{ checksSpreads: false }], + }, + { + code: ` +const getData = Promise.resolve({ key: 42 }); + +console.log({ + someData: 42, + ...getData(), +}); + `, + options: [{ checksSpreads: false }], + }, ], invalid: [ @@ -870,5 +898,32 @@ it('', async () => {}); }, ], }, + { + code: ` +console.log({ ...Promise.resolve({ key: 42 }) }); + `, + errors: [ + { + line: 2, + messageId: 'spread', + }, + ], + }, + { + code: ` +const getData = () => Promise.resolve({ key: 42 }); + +console.log({ + someData: 42, + ...getData(), +}); + `, + errors: [ + { + line: 6, + messageId: 'spread', + }, + ], + }, ], }); From 3083b01f7dd90f8deeaaefccde1b40168965af59 Mon Sep 17 00:00:00 2001 From: Dominik Rowicki Date: Mon, 23 May 2022 17:49:16 +0200 Subject: [PATCH 2/4] feat(eslint-plugin): fix typo --- .../docs/rules/no-misused-promises.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-misused-promises.md b/packages/eslint-plugin/docs/rules/no-misused-promises.md index 24e66dda7df..203aefc178e 100644 --- a/packages/eslint-plugin/docs/rules/no-misused-promises.md +++ b/packages/eslint-plugin/docs/rules/no-misused-promises.md @@ -14,7 +14,7 @@ See [`no-floating-promises`](./no-floating-promises.md) for detecting unhandled ## Rule Details This rule accepts a single option which is an object with `checksConditionals`, -`checksVoidReturn`, and `checkSpreads` properties indicating which types of +`checksVoidReturn`, and `checksSpreads` properties indicating which types of misuse to flag. All are enabled by default. ## Options @@ -24,7 +24,7 @@ type Options = [ { checksConditionals?: boolean; checksVoidReturn?: boolean | ChecksVoidReturnOptions; - checkSpreads?: boolean; + checksSpreads?: boolean; }, ]; @@ -40,7 +40,7 @@ const defaultOptions: Options = [ { checksConditionals: true, checksVoidReturn: true, - checkSpreads: true, + checksSpreads: true, }, ]; ``` @@ -103,7 +103,7 @@ For example, if you don't mind that passing a `() => Promise` to a `() => } ``` -### `"checkSpreads"` +### `"checksSpreads"` If you don't want to check object spreads, you can add this configuration: @@ -112,7 +112,7 @@ If you don't want to check object spreads, you can add this configuration: "@typescript-eslint/no-misused-promises": [ "error", { - "checkSpreads": false + "checksSpreads": false } ] } @@ -229,9 +229,9 @@ eventEmitter.on('some-event', () => { -### `checkSpreads: true` +### `checksSpreads: true` -Examples of code for this rule with `checkSpreads: true`: +Examples of code for this rule with `checksSpreads: true`: From bcdbc121816068c00148465c6a1325218c7e7abf Mon Sep 17 00:00:00 2001 From: Dominik Rowicki Date: Tue, 24 May 2022 09:48:05 +0200 Subject: [PATCH 3/4] feat(eslint-plugin): handle logical expressions --- .../src/rules/no-misused-promises.ts | 21 ++++++----- .../tests/rules/no-misused-promises.test.ts | 35 +++++++++++++++++++ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-misused-promises.ts b/packages/eslint-plugin/src/rules/no-misused-promises.ts index 8f7d84633c7..cf291820642 100644 --- a/packages/eslint-plugin/src/rules/no-misused-promises.ts +++ b/packages/eslint-plugin/src/rules/no-misused-promises.ts @@ -390,14 +390,7 @@ export default util.createRule({ function checkSpread(node: TSESTree.SpreadElement): void { const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node); - if ( - node.argument.type === AST_NODE_TYPES.CallExpression && - tsutils.isThenableType( - checker, - tsNode.expression, - checker.getTypeAtLocation(tsNode.expression), - ) - ) { + if (isSometimesThenable(checker, tsNode.expression)) { context.report({ messageId: 'spread', node: node.argument, @@ -413,6 +406,18 @@ export default util.createRule({ }, }); +function isSometimesThenable(checker: ts.TypeChecker, node: ts.Node): boolean { + const type = checker.getTypeAtLocation(node); + + for (const subType of tsutils.unionTypeParts(checker.getApparentType(type))) { + if (tsutils.isThenableType(checker, node, subType)) { + return true; + } + } + + return false; +} + // Variation on the thenable check which requires all forms of the type (read: // alternates in a union) to be thenable. Otherwise, you might be trying to // check if something is defined or undefined and get caught because one of the 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 2bd834ee9c2..029bec906b6 100644 --- a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts @@ -326,6 +326,14 @@ console.log({ someData: 42, ...(await getData()), }); + `, + ` +declare const condition: boolean; + +console.log({ ...(condition && (await Promise.resolve({ key: 42 }))) }); +console.log({ ...(condition || (await Promise.resolve({ key: 42 }))) }); +console.log({ ...(condition ? {} : await Promise.resolve({ key: 42 })) }); +console.log({ ...(condition ? await Promise.resolve({ key: 42 }) : {}) }); `, { code: ` @@ -344,6 +352,17 @@ console.log({ `, options: [{ checksSpreads: false }], }, + { + code: ` +declare const condition: boolean; + +console.log({ ...(condition && Promise.resolve({ key: 42 })) }); +console.log({ ...(condition || Promise.resolve({ key: 42 })) }); +console.log({ ...(condition ? {} : Promise.resolve({ key: 42 })) }); +console.log({ ...(condition ? Promise.resolve({ key: 42 }) : {}) }); + `, + options: [{ checksSpreads: false }], + }, ], invalid: [ @@ -925,5 +944,21 @@ console.log({ }, ], }, + { + code: ` +declare const condition: boolean; + +console.log({ ...(condition && Promise.resolve({ key: 42 })) }); +console.log({ ...(condition || Promise.resolve({ key: 42 })) }); +console.log({ ...(condition ? {} : Promise.resolve({ key: 42 })) }); +console.log({ ...(condition ? Promise.resolve({ key: 42 }) : {}) }); + `, + errors: [ + { line: 4, messageId: 'spread' }, + { line: 5, messageId: 'spread' }, + { line: 6, messageId: 'spread' }, + { line: 7, messageId: 'spread' }, + ], + }, ], }); From b41256c19b16412a6636e2eca183ddcc3f22ae44 Mon Sep 17 00:00:00 2001 From: Dominik Rowicki Date: Tue, 24 May 2022 10:16:00 +0200 Subject: [PATCH 4/4] feat(eslint-plugin): test spreading promises in arrays --- .../tests/rules/no-misused-promises.test.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) 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 029bec906b6..24c9d4285d6 100644 --- a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts @@ -334,6 +334,9 @@ console.log({ ...(condition && (await Promise.resolve({ key: 42 }))) }); console.log({ ...(condition || (await Promise.resolve({ key: 42 }))) }); console.log({ ...(condition ? {} : await Promise.resolve({ key: 42 })) }); console.log({ ...(condition ? await Promise.resolve({ key: 42 }) : {}) }); + `, + ` +console.log([...(await Promise.resolve(42))]); `, { code: ` @@ -363,6 +366,13 @@ console.log({ ...(condition ? Promise.resolve({ key: 42 }) : {}) }); `, options: [{ checksSpreads: false }], }, + { + code: ` +// This is invalid Typescript, but it shouldn't trigger this linter specifically +console.log([...Promise.resolve(42)]); + `, + options: [{ checksSpreads: false }], + }, ], invalid: [