From 28a131dfe1e785e791deda04ff99cd46ec0594e4 Mon Sep 17 00:00:00 2001 From: Jeff Principe Date: Tue, 16 Jul 2019 07:48:31 -0700 Subject: [PATCH] feat(eslint-plugin): add new rule no-misused-promises (#612) * feat(eslint-plugin): add new rule no-misused-promises * chore: review feedback * chore: regenerate plugin configs --- packages/eslint-plugin/README.md | 1 + .../docs/rules/no-misused-promises.md | 119 ++++++++ packages/eslint-plugin/src/configs/all.json | 1 + packages/eslint-plugin/src/configs/base.json | 4 +- .../src/configs/recommended.json | 2 - packages/eslint-plugin/src/rules/index.ts | 2 + .../src/rules/no-misused-promises.ts | 253 ++++++++++++++++ .../tests/rules/no-misused-promises.test.ts | 282 ++++++++++++++++++ .../experimental-utils/src/ts-eslint/Rule.ts | 1 + 9 files changed, 662 insertions(+), 3 deletions(-) create mode 100644 packages/eslint-plugin/docs/rules/no-misused-promises.md create mode 100644 packages/eslint-plugin/src/rules/no-misused-promises.ts create mode 100644 packages/eslint-plugin/tests/rules/no-misused-promises.test.ts diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index e958c1a01f2..a4d7be3ce5d 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -152,6 +152,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e | [`@typescript-eslint/no-inferrable-types`](./docs/rules/no-inferrable-types.md) | Disallows explicit type declarations for variables or parameters initialized to a number, string, or boolean | :heavy_check_mark: | :wrench: | | | [`@typescript-eslint/no-magic-numbers`](./docs/rules/no-magic-numbers.md) | Disallows magic numbers | | | | | [`@typescript-eslint/no-misused-new`](./docs/rules/no-misused-new.md) | Enforce valid definition of `new` and `constructor` | :heavy_check_mark: | | | +| [`@typescript-eslint/no-misused-promises`](./docs/rules/no-misused-promises.md) | Avoid using promises in places not designed to handle them | | | :thought_balloon: | | [`@typescript-eslint/no-namespace`](./docs/rules/no-namespace.md) | Disallow the use of custom TypeScript modules and namespaces | :heavy_check_mark: | | | | [`@typescript-eslint/no-non-null-assertion`](./docs/rules/no-non-null-assertion.md) | Disallows non-null assertions using the `!` postfix operator | :heavy_check_mark: | | | | [`@typescript-eslint/no-object-literal-type-assertion`](./docs/rules/no-object-literal-type-assertion.md) | Forbids an object literal to appear in a type assertion expression | :heavy_check_mark: | | | diff --git a/packages/eslint-plugin/docs/rules/no-misused-promises.md b/packages/eslint-plugin/docs/rules/no-misused-promises.md new file mode 100644 index 00000000000..65e3084ae26 --- /dev/null +++ b/packages/eslint-plugin/docs/rules/no-misused-promises.md @@ -0,0 +1,119 @@ +# Avoid using promises in places not designed to handle them (no-misused-promises) + +This rule forbids using promises in places where the Typescript compiler +allows them but they are not handled properly. These situations can often arise +due to a missing `await` keyword or just a misunderstanding of the way async +functions are handled/awaited. + +## Rule Details + +Examples of **incorrect** code for this rule with `checksConditionals: true`: + +```ts +const promise = Promise.resolve('value'); + +if (promise) { + // Do something +} + +const val = promise ? 123 : 456; + +while (promise) { + // Do something +} +``` + +Examples of **incorrect** code for this rule with `checksVoidReturn: true`: + +```ts +[1, 2, 3].forEach(async value => { + await doSomething(value); +}); + +new Promise(async (resolve, reject) => { + await doSomething(); + resolve(); +}); + +const eventEmitter = new EventEmitter(); +eventEmitter.on('some-event', async () => { + await doSomething(); +}); +``` + +Examples of **correct** code for this rule: + +```ts +const promise = Promise.resolve('value'); + +if (await promise) { + // Do something +} + +const val = (await promise) ? 123 : 456; + +while (await promise) { + // Do something +} + +for (const value of [1, 2, 3]) { + await doSomething(value); +} + +new Promise((resolve, reject) => { + // Do something + resolve(); +}); + +const eventEmitter = new EventEmitter(); +eventEmitter.on('some-event', () => { + doSomething(); +}); +``` + +## Options + +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 + +If you don't want functions that return promises where a void return is +expected to be checked, your configuration will look like this: + +```json +{ + "@typescript-eslint/no-misused-promises": [ + "error", + { + "checksVoidReturn": false + } + ] +} +``` + +Likewise, if you don't want to check conditionals, you can configure the rule +like this: + +```json +{ + "@typescript-eslint/no-misused-promises": [ + "error", + { + "checksConditionals": false + } + ] +} +``` + +## When Not To Use It + +If you do not use Promises in your codebase or are not concerned with possible +misuses of them outside of what the Typescript compiler will check. + +## Related to + +- [`no-floating-promises`]('./no-floating-promises.md') + +## Further Reading + +- [Typescript void function assignability](https://github.com/Microsoft/TypeScript/wiki/FAQ#why-are-functions-returning-non-void-assignable-to-function-returning-void) diff --git a/packages/eslint-plugin/src/configs/all.json b/packages/eslint-plugin/src/configs/all.json index 5fd043c4c11..10831d4b246 100644 --- a/packages/eslint-plugin/src/configs/all.json +++ b/packages/eslint-plugin/src/configs/all.json @@ -36,6 +36,7 @@ "no-magic-numbers": "off", "@typescript-eslint/no-magic-numbers": "error", "@typescript-eslint/no-misused-new": "error", + "@typescript-eslint/no-misused-promises": "error", "@typescript-eslint/no-namespace": "error", "@typescript-eslint/no-non-null-assertion": "error", "@typescript-eslint/no-object-literal-type-assertion": "error", diff --git a/packages/eslint-plugin/src/configs/base.json b/packages/eslint-plugin/src/configs/base.json index 9b6931ad616..6f56100a6ae 100644 --- a/packages/eslint-plugin/src/configs/base.json +++ b/packages/eslint-plugin/src/configs/base.json @@ -3,5 +3,7 @@ "parserOptions": { "sourceType": "module" }, - "plugins": ["@typescript-eslint"] + "plugins": [ + "@typescript-eslint" + ] } diff --git a/packages/eslint-plugin/src/configs/recommended.json b/packages/eslint-plugin/src/configs/recommended.json index d26fd25d01c..0e6d6d9f665 100644 --- a/packages/eslint-plugin/src/configs/recommended.json +++ b/packages/eslint-plugin/src/configs/recommended.json @@ -24,13 +24,11 @@ "@typescript-eslint/no-non-null-assertion": "error", "@typescript-eslint/no-object-literal-type-assertion": "error", "@typescript-eslint/no-parameter-properties": "error", - "@typescript-eslint/no-triple-slash-reference": "error", "no-unused-vars": "off", "@typescript-eslint/no-unused-vars": "warn", "no-use-before-define": "off", "@typescript-eslint/no-use-before-define": "error", "@typescript-eslint/no-var-requires": "error", - "@typescript-eslint/prefer-interface": "error", "@typescript-eslint/prefer-namespace-keyword": "error", "@typescript-eslint/type-annotation-spacing": "error" } diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index 00107aa57a8..73f34ae380f 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -27,6 +27,7 @@ import noForInArray from './no-for-in-array'; import noInferrableTypes from './no-inferrable-types'; import noMagicNumbers from './no-magic-numbers'; import noMisusedNew from './no-misused-new'; +import noMisusedPromises from './no-misused-promises'; import noNamespace from './no-namespace'; import noNonNullAssertion from './no-non-null-assertion'; import noObjectLiteralTypeAssertion from './no-object-literal-type-assertion'; @@ -89,6 +90,7 @@ export default { 'no-inferrable-types': noInferrableTypes, 'no-magic-numbers': noMagicNumbers, 'no-misused-new': noMisusedNew, + 'no-misused-promises': noMisusedPromises, 'no-namespace': noNamespace, 'no-non-null-assertion': noNonNullAssertion, 'no-object-literal-type-assertion': noObjectLiteralTypeAssertion, diff --git a/packages/eslint-plugin/src/rules/no-misused-promises.ts b/packages/eslint-plugin/src/rules/no-misused-promises.ts new file mode 100644 index 00000000000..7b1181ff179 --- /dev/null +++ b/packages/eslint-plugin/src/rules/no-misused-promises.ts @@ -0,0 +1,253 @@ +import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils'; +import * as tsutils from 'tsutils'; +import ts from 'typescript'; + +import * as util from '../util'; + +type Options = [ + { + checksConditionals?: boolean; + checksVoidReturn?: boolean; + } +]; + +export default util.createRule({ + name: 'no-misused-promises', + meta: { + docs: { + description: 'Avoid using promises in places not designed to handle them', + category: 'Best Practices', + recommended: false, + }, + messages: { + voidReturn: + 'Promise returned in function argument where a void return was expected.', + conditional: 'Expected non-Promise value in a boolean conditional.', + }, + schema: [ + { + type: 'object', + properties: { + checksConditionals: { + type: 'boolean', + }, + checksVoidReturn: { + type: 'boolean', + }, + }, + }, + ], + type: 'problem', + }, + defaultOptions: [ + { + checksConditionals: true, + checksVoidReturn: true, + }, + ], + + create(context, [{ checksConditionals, checksVoidReturn }]) { + const parserServices = util.getParserServices(context); + const checker = parserServices.program.getTypeChecker(); + + const conditionalChecks: TSESLint.RuleListener = { + ConditionalExpression: checkTestConditional, + DoWhileStatement: checkTestConditional, + ForStatement: checkTestConditional, + IfStatement: checkTestConditional, + LogicalExpression(node) { + // We only check the lhs of a logical expression because the rhs might + // be the return value of a short circuit expression. + checkConditional(node.left); + }, + UnaryExpression(node) { + if (node.operator === '!') { + checkConditional(node.argument); + } + }, + WhileStatement: checkTestConditional, + }; + + const voidReturnChecks: TSESLint.RuleListener = { + CallExpression: checkArguments, + NewExpression: checkArguments, + }; + + function checkTestConditional(node: { test: TSESTree.Expression | null }) { + if (node.test) { + checkConditional(node.test); + } + } + + function checkConditional(node: TSESTree.Expression) { + const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node); + if (isAlwaysThenable(checker, tsNode)) { + context.report({ + messageId: 'conditional', + node, + }); + } + } + + function checkArguments( + node: TSESTree.CallExpression | TSESTree.NewExpression, + ) { + const tsNode = parserServices.esTreeNodeToTSNodeMap.get< + ts.CallExpression | ts.NewExpression + >(node); + const voidParams = voidFunctionParams(checker, tsNode); + if (voidParams.size === 0) { + return; + } + + for (const [index, argument] of node.arguments.entries()) { + if (!voidParams.has(index)) { + continue; + } + + const tsNode = parserServices.esTreeNodeToTSNodeMap.get(argument); + if (returnsThenable(checker, tsNode as ts.Expression)) { + context.report({ + messageId: 'voidReturn', + node: argument, + }); + } + } + } + + return { + ...(checksConditionals ? conditionalChecks : {}), + ...(checksVoidReturn ? voidReturnChecks : {}), + }; + }, +}); + +// 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 +// branches is thenable. +function isAlwaysThenable(checker: ts.TypeChecker, node: ts.Node) { + const type = checker.getTypeAtLocation(node); + + for (const subType of tsutils.unionTypeParts(checker.getApparentType(type))) { + const thenProp = subType.getProperty('then'); + + // If one of the alternates has no then property, it is not thenable in all + // cases. + if (thenProp === undefined) { + return false; + } + + // We walk through each variation of the then property. Since we know it + // exists at this point, we just need at least one of the alternates to + // be of the right form to consider it thenable. + const thenType = checker.getTypeOfSymbolAtLocation(thenProp, node); + let hasThenableSignature = false; + for (const subType of tsutils.unionTypeParts(thenType)) { + for (const signature of subType.getCallSignatures()) { + if ( + signature.parameters.length !== 0 && + isFunctionParam(checker, signature.parameters[0], node) + ) { + hasThenableSignature = true; + break; + } + } + + // We only need to find one variant of the then property that has a + // function signature for it to be thenable. + if (hasThenableSignature) { + break; + } + } + + // If no flavors of the then property are thenable, we don't consider the + // overall type to be thenable + if (!hasThenableSignature) { + return false; + } + } + + // If all variants are considered thenable (i.e. haven't returned false), we + // consider the overall type thenable + return true; +} + +function isFunctionParam( + checker: ts.TypeChecker, + param: ts.Symbol, + node: ts.Node, +): boolean { + const type: ts.Type | undefined = checker.getApparentType( + checker.getTypeOfSymbolAtLocation(param, node), + ); + for (const subType of tsutils.unionTypeParts(type)) { + if (subType.getCallSignatures().length !== 0) { + return true; + } + } + return false; +} + +// Get the positions of parameters 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( + checker: ts.TypeChecker, + node: ts.CallExpression | ts.NewExpression, +) { + const voidReturnIndices = new Set(); + const thenableReturnIndices = new Set(); + const type = checker.getTypeAtLocation(node.expression); + + for (const subType of tsutils.unionTypeParts(type)) { + // Standard function calls and `new` have two different types of signatures + const signatures = ts.isCallExpression(node) + ? subType.getCallSignatures() + : subType.getConstructSignatures(); + for (const signature of signatures) { + for (const [index, parameter] of signature.parameters.entries()) { + const type = checker.getTypeOfSymbolAtLocation( + parameter, + node.expression, + ); + for (const subType of tsutils.unionTypeParts(type)) { + for (const signature of subType.getCallSignatures()) { + const returnType = signature.getReturnType(); + if (tsutils.isTypeFlagSet(returnType, ts.TypeFlags.Void)) { + voidReturnIndices.add(index); + } else if ( + tsutils.isThenableType(checker, node.expression, returnType) + ) { + thenableReturnIndices.add(index); + } + } + } + } + } + } + + // If a certain positional argument accepts both thenable and void returns, + // a promise-returning function is valid + for (const thenable of thenableReturnIndices) { + voidReturnIndices.delete(thenable); + } + + return voidReturnIndices; +} + +// Returns true if the expression is a function that returns a thenable +function returnsThenable(checker: ts.TypeChecker, node: ts.Expression) { + const type = checker.getApparentType(checker.getTypeAtLocation(node)); + + for (const subType of tsutils.unionTypeParts(type)) { + for (const signature of subType.getCallSignatures()) { + const returnType = signature.getReturnType(); + if (tsutils.isThenableType(checker, node, returnType)) { + return true; + } + } + } + + return false; +} diff --git a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts new file mode 100644 index 00000000000..5d4c7fcf4b8 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts @@ -0,0 +1,282 @@ +import rule from '../../src/rules/no-misused-promises'; +import { RuleTester, getFixturesRootDir } from '../RuleTester'; + +const rootDir = getFixturesRootDir(); + +const ruleTester = new RuleTester({ + parserOptions: { + ecmaVersion: 2018, + tsconfigRootDir: rootDir, + project: './tsconfig.json', + }, + parser: '@typescript-eslint/parser', +}); + +ruleTester.run('no-misused-promises', rule, { + valid: [ + `if (true) {}`, + { + code: `if (Promise.resolve()) {}`, + options: [{ checksConditionals: false }], + }, + ` +if (true) {} +else if (false) {} +else {} +`, + { + code: ` +if (Promise.resolve()) {} +else if (Promise.resolve()) {} +else {} +`, + options: [{ checksConditionals: false }], + }, + `for (;;) {}`, + `for (let i; i < 10; i++) {}`, + { + code: `for (let i; Promise.resolve(); i++) {}`, + options: [{ checksConditionals: false }], + }, + `do {} while (true);`, + { + code: `do {} while (Promise.resolve())`, + options: [{ checksConditionals: false }], + }, + `while (true) {}`, + { + code: `while (Promise.resolve()) {}`, + options: [{ checksConditionals: false }], + }, + `true ? 123 : 456`, + { + code: `Promise.resolve() ? 123 : 456`, + options: [{ checksConditionals: false }], + }, + `if (!true) {}`, + { + code: `if (!Promise.resolve()) {}`, + options: [{ checksConditionals: false }], + }, + `(await Promise.resolve()) || false`, + { + code: `Promise.resolve() || false`, + options: [{ checksConditionals: false }], + }, + `(true && await Promise.resolve()) || false`, + { + code: `(true && Promise.resolve()) || false`, + options: [{ checksConditionals: false }], + }, + `false || (true && Promise.resolve())`, + ` +async function test() { + if (await Promise.resolve()) {} +} +`, + ` +async function test() { + const mixed: Promise | undefined = Promise.resolve(); + if (mixed) { + await mixed; + } +} +`, + `if (~Promise.resolve()) {}`, + ` +interface NotQuiteThenable { + then(param: string): void; + then(): void; +} +const value: NotQuiteThenable = { then() {} }; +if (value) {} +`, + `[1, 2, 3].forEach(val => {});`, + { + code: `[1, 2, 3].forEach(async val => {});`, + options: [{ checksVoidReturn: false }], + }, + `new Promise((resolve, reject) => resolve());`, + { + code: `new Promise(async (resolve, reject) => resolve());`, + options: [{ checksVoidReturn: false }], + }, + `Promise.all(['abc', 'def'].map(async val => { await val; }))`, + ` +const fn: (arg: () => Promise | void) => void = () => {}; +fn(() => Promise.resolve()); +`, + ], + + invalid: [ + { + code: `if (Promise.resolve()) {}`, + errors: [ + { + line: 1, + messageId: 'conditional', + }, + ], + }, + { + code: ` +if (Promise.resolve()) {} +else if (Promise.resolve()) {} +else {} +`, + errors: [ + { + line: 2, + messageId: 'conditional', + }, + { + line: 3, + messageId: 'conditional', + }, + ], + }, + { + code: `for (let i; Promise.resolve(); i++) {}`, + errors: [ + { + line: 1, + messageId: 'conditional', + }, + ], + }, + { + code: `do {} while (Promise.resolve())`, + errors: [ + { + line: 1, + messageId: 'conditional', + }, + ], + }, + { + code: `while (Promise.resolve()) {}`, + errors: [ + { + line: 1, + messageId: 'conditional', + }, + ], + }, + { + code: `Promise.resolve() ? 123 : 456`, + errors: [ + { + line: 1, + messageId: 'conditional', + }, + ], + }, + { + code: `if (!Promise.resolve()) {}`, + errors: [ + { + line: 1, + messageId: 'conditional', + }, + ], + }, + { + code: `Promise.resolve() || false`, + errors: [ + { + line: 1, + messageId: 'conditional', + }, + ], + }, + { + code: `(true && Promise.resolve()) || false`, + errors: [ + { + line: 1, + messageId: 'conditional', + }, + ], + }, + { + code: ` +[Promise.resolve(), Promise.reject()].forEach( + async val => { await val; } +); +`, + errors: [ + { + line: 3, + messageId: 'voidReturn', + }, + ], + }, + { + code: ` +new Promise(async (resolve, reject) => { + await Promise.resolve(); + resolve(); +}); +`, + errors: [ + { + line: 2, + messageId: 'voidReturn', + }, + ], + }, + { + code: ` +const fnWithCallback = (arg: string, cb: (err: any, res: string) => void) => { + cb(null, arg); +}; + +fnWithCallback('val', async (err, res) => { + await res; +}); +`, + errors: [ + { + line: 6, + messageId: 'voidReturn', + }, + ], + }, + { + code: ` +const fnWithCallback = (arg: string, cb: (err: any, res: string) => void) => { + cb(null, arg); +}; + +fnWithCallback('val', (err, res) => Promise.resolve(res)); +`, + errors: [ + { + line: 6, + messageId: 'voidReturn', + }, + ], + }, + { + code: ` +const fnWithCallback = (arg: string, cb: (err: any, res: string) => void) => { + cb(null, arg); +}; + +fnWithCallback('val', (err, res) => { + if (err) { + return 'abc'; + } else { + return Promise.resolve(res); + } +}); +`, + errors: [ + { + line: 6, + messageId: 'voidReturn', + }, + ], + }, + ], +}); diff --git a/packages/experimental-utils/src/ts-eslint/Rule.ts b/packages/experimental-utils/src/ts-eslint/Rule.ts index 2759a3d33d2..bc546e25cae 100644 --- a/packages/experimental-utils/src/ts-eslint/Rule.ts +++ b/packages/experimental-utils/src/ts-eslint/Rule.ts @@ -270,6 +270,7 @@ interface RuleListener { JSXSpreadChild?: RuleFunction; JSXText?: RuleFunction; LabeledStatement?: RuleFunction; + LogicalExpression?: RuleFunction; MemberExpression?: RuleFunction; MetaProperty?: RuleFunction; MethodDefinition?: RuleFunction;