Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
feat(eslint-plugin): [no-misused-promises] warn when spreading promis…
…es (#5053)

* feat(eslint-plugin): warn when spreading promises

* feat(eslint-plugin): fix typo

* feat(eslint-plugin): handle logical expressions

* feat(eslint-plugin): test spreading promises in arrays

Co-authored-by: Josh Goldberg <me@joshuakgoldberg.com>
  • Loading branch information
papermana and JoshuaKGoldberg committed May 25, 2022
1 parent bc90ce0 commit 61ffa9e
Show file tree
Hide file tree
Showing 3 changed files with 193 additions and 5 deletions.
59 changes: 56 additions & 3 deletions packages/eslint-plugin/docs/rules/no-misused-promises.md
Expand Up @@ -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 `checksSpreads` properties indicating which types of
misuse to flag. All are enabled by default.

## Options

Expand All @@ -24,6 +24,7 @@ type Options = [
{
checksConditionals?: boolean;
checksVoidReturn?: boolean | ChecksVoidReturnOptions;
checksSpreads?: boolean;
},
];

Expand All @@ -39,6 +40,7 @@ const defaultOptions: Options = [
{
checksConditionals: true,
checksVoidReturn: true,
checksSpreads: true,
},
];
```
Expand Down Expand Up @@ -101,6 +103,21 @@ For example, if you don't mind that passing a `() => Promise<void>` to a `() =>
}
```

### `"checksSpreads"`

If you don't want to check object spreads, you can add this configuration:

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

### `checksConditionals: true`

Examples of code for this rule with `checksConditionals: true`:
Expand Down Expand Up @@ -212,6 +229,42 @@ eventEmitter.on('some-event', () => {

<!--/tabs-->

### `checksSpreads: true`

Examples of code for this rule with `checksSpreads: true`:

<!--tabs-->

#### ❌ 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()) };
```

<!--tabs-->

## When Not To Use It

If you do not use Promises in your codebase or are not concerned with possible
Expand Down
39 changes: 37 additions & 2 deletions packages/eslint-plugin/src/rules/no-misused-promises.ts
Expand Up @@ -8,6 +8,7 @@ type Options = [
{
checksConditionals?: boolean;
checksVoidReturn?: boolean | ChecksVoidReturnOptions;
checksSpreads?: boolean;
},
];

Expand All @@ -25,7 +26,8 @@ type MessageId =
| 'voidReturnVariable'
| 'voidReturnProperty'
| 'voidReturnReturnValue'
| 'voidReturnAttribute';
| 'voidReturnAttribute'
| 'spread';

function parseChecksVoidReturn(
checksVoidReturn: boolean | ChecksVoidReturnOptions | undefined,
Expand Down Expand Up @@ -75,6 +77,7 @@ export default util.createRule<Options, MessageId>({
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: [
{
Expand All @@ -99,6 +102,9 @@ export default util.createRule<Options, MessageId>({
},
],
},
checksSpreads: {
type: 'boolean',
},
},
},
],
Expand All @@ -108,10 +114,11 @@ export default util.createRule<Options, MessageId>({
{
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();

Expand Down Expand Up @@ -153,6 +160,10 @@ export default util.createRule<Options, MessageId>({
}
: {};

const spreadChecks: TSESLint.RuleListener = {
SpreadElement: checkSpread,
};

function checkTestConditional(node: {
test: TSESTree.Expression | null;
}): void {
Expand Down Expand Up @@ -376,13 +387,37 @@ export default util.createRule<Options, MessageId>({
}
}

function checkSpread(node: TSESTree.SpreadElement): void {
const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node);

if (isSometimesThenable(checker, tsNode.expression)) {
context.report({
messageId: 'spread',
node: node.argument,
});
}
}

return {
...(checksConditionals ? conditionalChecks : {}),
...(checksVoidReturn ? voidReturnChecks : {}),
...(checksSpreads ? spreadChecks : {}),
};
},
});

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
Expand Down
100 changes: 100 additions & 0 deletions packages/eslint-plugin/tests/rules/no-misused-promises.test.ts
Expand Up @@ -316,6 +316,63 @@ const _ = <Component onEvent={async () => {}} />;
`,
filename: 'react.tsx',
},
`
console.log({ ...(await Promise.resolve({ key: 42 })) });
`,
`
const getData = Promise.resolve({ key: 42 });
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 }) : {}) });
`,
`
console.log([...(await Promise.resolve(42))]);
`,
{
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 }],
},
{
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 }],
},
{
code: `
// This is invalid Typescript, but it shouldn't trigger this linter specifically
console.log([...Promise.resolve(42)]);
`,
options: [{ checksSpreads: false }],
},
],

invalid: [
Expand Down Expand Up @@ -870,5 +927,48 @@ 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',
},
],
},
{
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' },
],
},
],
});

0 comments on commit 61ffa9e

Please sign in to comment.