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): [no-floating-promises] flag result of .map(async) #7897

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/eslint-plugin/docs/rules/no-floating-promises.md
Expand Up @@ -17,6 +17,13 @@ Valid ways of handling a Promise-valued statement include:
- Calling its `.then()` with two arguments
- Calling its `.catch()` with one argument

This rule also reports when an Array containing Promises is created and not properly handled. The main way to resolve this is by using one of the Promise concurrency methods to create a single Promise, then handling that according to the procedure above. These methods include:

- `Promise.all()`,
- `Promise.allSettled()`,
- `Promise.any()`
- `Promise.race()`

:::tip
`no-floating-promises` only detects unhandled Promise _statements_.
See [`no-misused-promises`](./no-misused-promises.md) for detecting code that provides Promises to _logical_ locations such as if statements.
Expand All @@ -40,6 +47,8 @@ returnsPromise().then(() => {});
Promise.reject('value').catch();

Promise.reject('value').finally();

[1, 2, 3].map(async x => x + 1);
```

### ✅ Correct
Expand All @@ -59,6 +68,8 @@ returnsPromise().then(
Promise.reject('value').catch(() => {});

await Promise.reject('value').finally(() => {});

await Promise.all([1, 2, 3].map(async x => x + 1));
```

## Options
Expand Down Expand Up @@ -104,3 +115,7 @@ If you do not use Promise-like values in your codebase, or want to allow them to
## Related To

- [`no-misused-promises`](./no-misused-promises.md)

## Further Reading

- ["Using Promises" MDN documentation](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises). Note especially the sections on [Promise rejection events](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises#promise_rejection_events) and [Composition](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises#composition).
75 changes: 65 additions & 10 deletions packages/eslint-plugin/src/rules/no-floating-promises.ts
kirkwaiblinger marked this conversation as resolved.
Show resolved Hide resolved
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
Expand Up @@ -23,7 +23,9 @@ type MessageId =
| 'floatingUselessRejectionHandler'
| 'floatingUselessRejectionHandlerVoid'
| 'floatingFixAwait'
| 'floatingFixVoid';
| 'floatingFixVoid'
| 'floatingPromiseArray'
| 'floatingPromiseArrayVoid';

const messageBase =
'Promises must be awaited, end with a call to .catch, or end with a call to .then with a rejection handler.';
Expand All @@ -35,6 +37,13 @@ const messageBaseVoid =
const messageRejectionHandler =
'A rejection handler that is not a function will be ignored.';

const messagePromiseArray =
"An array of Promises may be unintentional. Consider handling the promises' fulfillment or rejection with Promise.all or similar.";
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

const messagePromiseArrayVoid =
"An array of Promises may be unintentional. Consider handling the promises' fulfillment or rejection with Promise.all or similar," +
' or explicitly marking the expression as ignored with the `void` operator.';

export default createRule<Options, MessageId>({
name: 'no-floating-promises',
meta: {
Expand All @@ -54,6 +63,8 @@ export default createRule<Options, MessageId>({
messageBase + ' ' + messageRejectionHandler,
floatingUselessRejectionHandlerVoid:
messageBaseVoid + ' ' + messageRejectionHandler,
floatingPromiseArray: messagePromiseArray,
floatingPromiseArrayVoid: messagePromiseArrayVoid,
},
schema: [
{
Expand Down Expand Up @@ -97,13 +108,18 @@ export default createRule<Options, MessageId>({
expression = expression.expression;
}

const { isUnhandled, nonFunctionHandler } = isUnhandledPromise(
checker,
expression,
);
const { isUnhandled, nonFunctionHandler, promiseArray } =
isUnhandledPromise(checker, expression);

if (isUnhandled) {
if (options.ignoreVoid) {
if (promiseArray) {
context.report({
node,
messageId: options.ignoreVoid
? 'floatingPromiseArrayVoid'
: 'floatingPromiseArray',
});
} else if (options.ignoreVoid) {
context.report({
node,
messageId: nonFunctionHandler
Expand Down Expand Up @@ -206,7 +222,11 @@ export default createRule<Options, MessageId>({
function isUnhandledPromise(
checker: ts.TypeChecker,
node: TSESTree.Node,
): { isUnhandled: boolean; nonFunctionHandler?: boolean } {
): {
isUnhandled: boolean;
nonFunctionHandler?: boolean;
promiseArray?: boolean;
} {
// First, check expressions whose resulting types may not be promise-like
if (node.type === AST_NODE_TYPES.SequenceExpression) {
// Any child in a comma expression could return a potentially unhandled
Expand All @@ -229,8 +249,16 @@ export default createRule<Options, MessageId>({
return isUnhandledPromise(checker, node.argument);
}

const tsNode = services.esTreeNodeToTSNodeMap.get(node);

// Check the type. At this point it can't be unhandled if it isn't a promise
if (!isPromiseLike(checker, services.esTreeNodeToTSNodeMap.get(node))) {
// or array thereof.

if (isPromiseArray(checker, tsNode)) {
return { isUnhandled: true, promiseArray: true };
}

if (!isPromiseLike(checker, tsNode)) {
return { isUnhandled: false };
}

Expand Down Expand Up @@ -296,12 +324,39 @@ export default createRule<Options, MessageId>({
},
});

function isPromiseArray(checker: ts.TypeChecker, node: ts.Node): boolean {
const type = checker.getTypeAtLocation(node);
for (const ty of tsutils
.unionTypeParts(type)
.map(t => checker.getApparentType(t))) {
if (checker.isArrayType(ty)) {
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
const arrayType = checker.getTypeArguments(ty)[0];
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
if (isPromiseLike(checker, node, arrayType)) {
return true;
}
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
}

if (checker.isTupleType(ty)) {
for (const tupleElementType of checker.getTypeArguments(ty)) {
if (isPromiseLike(checker, node, tupleElementType)) {
return true;
}
}
}
}
return false;
}

// Modified from tsutils.isThenable() to only consider thenables which can be
// rejected/caught via a second parameter. Original source (MIT licensed):
//
// https://github.com/ajafff/tsutils/blob/49d0d31050b44b81e918eae4fbaf1dfe7b7286af/util/type.ts#L95-L125
function isPromiseLike(checker: ts.TypeChecker, node: ts.Node): boolean {
const type = checker.getTypeAtLocation(node);
function isPromiseLike(
checker: ts.TypeChecker,
node: ts.Node,
type?: ts.Type,
): boolean {
type ??= checker.getTypeAtLocation(node);
for (const ty of tsutils.unionTypeParts(checker.getApparentType(type))) {
const then = ty.getProperty('then');
if (then === undefined) {
Expand Down
161 changes: 158 additions & 3 deletions packages/eslint-plugin/tests/rules/no-floating-promises.test.ts
Expand Up @@ -222,7 +222,7 @@ async function test() {
`
async function test() {
class Thenable {
then(callback: () => {}): Thenable {
then(callback: () => void): Thenable {
return new Thenable();
}
}
Expand Down Expand Up @@ -264,7 +264,7 @@ async function test() {
`
async function test() {
class CatchableThenable {
then(callback: () => {}, callback: () => {}): CatchableThenable {
then(callback: () => void, callback: () => void): CatchableThenable {
return new CatchableThenable();
}
}
Expand Down Expand Up @@ -475,6 +475,35 @@ Promise.reject()
.finally(() => {});
`,
},
{
code: `
await Promise.all([Promise.resolve(), Promise.resolve()]);
`,
},
{
code: `
declare const promiseArray: Array<Promise<unknown>>;
void promiseArray;
`,
},
{
code: `
[Promise.reject(), Promise.reject()].then(() => {});
`,
},
{
// Expressions aren't checked by this rule, so this just becomes an array
// of number | undefined, which is fine regardless of the ignoreVoid setting.
code: `
[1, 2, void Promise.reject(), 3];
`,
options: [{ ignoreVoid: false }],
},
{
code: `
['I', 'am', 'just', 'an', 'array'];
`,
},
],

invalid: [
Expand Down Expand Up @@ -997,7 +1026,7 @@ async function test() {
code: `
async function test() {
class CatchableThenable {
then(callback: () => {}, callback: () => {}): CatchableThenable {
then(callback: () => void, callback: () => void): CatchableThenable {
return new CatchableThenable();
}
}
Expand Down Expand Up @@ -1651,5 +1680,131 @@ Promise.reject(new Error('message')).finally(() => {});
`,
errors: [{ line: 2, messageId: 'floatingVoid' }],
},
{
code: `
function _<T, S extends Array<T | Promise<T>>>(
maybePromiseArray: S | undefined,
): void {
maybePromiseArray?.[0];
}
`,
errors: [{ line: 5, messageId: 'floatingVoid' }],
},
{
code: `
[1, 2, 3].map(() => Promise.reject());
`,
errors: [{ line: 2, messageId: 'floatingPromiseArrayVoid' }],
},
{
code: `
declare const array: unknown[];
array.map(() => Promise.reject());
`,
errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }],
},
{
code: `
declare const promiseArray: Array<Promise<unknown>>;
void promiseArray;
`,
options: [{ ignoreVoid: false }],
errors: [{ line: 3, messageId: 'floatingPromiseArray' }],
},
{
code: `
[1, 2, Promise.reject(), 3];
`,
errors: [{ line: 2, messageId: 'floatingPromiseArrayVoid' }],
},
{
code: `
[1, 2, Promise.reject().catch(() => {}), 3];
`,
errors: [{ line: 2, messageId: 'floatingPromiseArrayVoid' }],
},
{
code: `
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test case from the issue report

const data = ['test'];
data.map(async () => {
await new Promise((_res, rej) => setTimeout(rej, 1000));
});
`,
errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }],
},
{
code: `
function _<T, S extends Array<T | Array<T | Promise<T>>>>(
maybePromiseArrayArray: S | undefined,
): void {
maybePromiseArrayArray?.[0];
}
`,
errors: [{ line: 5, messageId: 'floatingPromiseArrayVoid' }],
},
{
code: `
function f<T extends Array<Promise<number>>>(a: T): void {
a;
}
`,
errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }],
},
{
code: `
declare const a: Array<Promise<number>> | undefined;
a;
`,
errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }],
},
{
code: `
function f<T extends Array<Promise<number>>>(a: T | undefined): void {
a;
}
`,
errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }],
},
{
code: `
[Promise.reject()] as const;
`,
errors: [{ line: 2, messageId: 'floatingPromiseArrayVoid' }],
},
{
code: `
declare function cursed(): [Promise<number>, Promise<string>];
cursed();
`,
errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }],
},
{
code: `
[
'Type Argument number ',
1,
'is not',
Promise.resolve(),
'but it still is flagged',
] as const;
`,
errors: [{ line: 2, messageId: 'floatingPromiseArrayVoid' }],
},
{
code: `
declare const arrayOrPromiseTuple:
| Array<number>
| [number, number, Promise<unknown>, string];
arrayOrPromiseTuple;
`,
errors: [{ line: 5, messageId: 'floatingPromiseArrayVoid' }],
},
{
code: `
declare const okArrayOrPromiseArray: Array<number> | Array<Promise<unknown>>;
okArrayOrPromiseArray;
`,
errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }],
},
],
});