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): add new rule no-misused-promises #612

Merged
merged 9 commits into from Jul 16, 2019
1 change: 1 addition & 0 deletions packages/eslint-plugin/README.md
Expand Up @@ -153,6 +153,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: | | |
Expand Down
120 changes: 120 additions & 0 deletions packages/eslint-plugin/docs/rules/no-misused-promises.md
@@ -0,0 +1,120 @@
# 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 the `"conditional"` check:

```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 the `"void-return"` check:

```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 a single `checks`
property indicating which types of misuse to flag. As of right now, there are
two checks available ("conditional", "void-return"), which are both enabled by
default.

If you only want conditionals to be checked, your configuration will look like
this:

```json
{
"@typescript-eslint/no-misused-promises": [
"error",
{
"checks": ["conditional"]
}
]
}
```

Likewise, if you only want to check for functions that return promises where a
void return is expected, you can configure the rule like this:

```json
{
"@typescript-eslint/no-misused-promises": [
"error",
{
"checks": ["void-return"]
}
]
}
```

## 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)
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Expand Up @@ -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';
Expand Down Expand Up @@ -87,6 +88,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,
Expand Down
254 changes: 254 additions & 0 deletions packages/eslint-plugin/src/rules/no-misused-promises.ts
@@ -0,0 +1,254 @@
import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';
import * as tsutils from 'tsutils';
import ts from 'typescript';

import * as util from '../util';

type Options = [
{
checks: ('conditional' | 'void-return')[];
princjef marked this conversation as resolved.
Show resolved Hide resolved
}
];

export default util.createRule<Options, 'conditional' | 'voidReturn'>({
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: {
checks: {
type: 'array',
items: {
type: 'string',
enum: ['conditional', 'void-return'],
},
},
},
},
],
type: 'problem',
},
defaultOptions: [
{
checks: ['conditional', 'void-return'],
},
],

create(context, [{ checks }]) {
const parserServices = util.getParserServices(context);
const checker = parserServices.program.getTypeChecker();

const conditionalChecks: TSESLint.RuleListener = {
ConditionalExpression(node) {
princjef marked this conversation as resolved.
Show resolved Hide resolved
checkConditional(node.test);
},
DoWhileStatement(node) {
checkConditional(node.test);
},
ForStatement(node) {
if (node.test) {
checkConditional(node.test);
}
},
IfStatement(node) {
checkConditional(node.test);
},
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(node) {
checkConditional(node.test);
},
};

const voidReturnChecks: TSESLint.RuleListener = {
CallExpression(node) {
checkArguments(node);
},
NewExpression(node) {
checkArguments(node);
},
};

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 {
...(checks.includes('conditional') ? conditionalChecks : {}),
...(checks.includes('void-return') ? 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);

outer: for (const subType of tsutils.unionTypeParts(
princjef marked this conversation as resolved.
Show resolved Hide resolved
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);
for (const subType of tsutils.unionTypeParts(thenType)) {
for (const signature of subType.getCallSignatures()) {
if (
signature.parameters.length !== 0 &&
isFunctionParam(checker, signature.parameters[0], node)
) {
continue outer;
}
}
}

// If no flavors of the then property are thenable, we don't consider the
// overall type to be thenable
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<number>();
const thenableReturnIndices = new Set<number>();
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;
}