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.

Sign up for GitHub

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

Jump to bottom

fix(eslint-plugin): [no-floating-promises] false negative calling .then with second argument undefined #6881

Merged
merged 5 commits into from Jul 15, 2023
Merged

fix(eslint-plugin): [no-floating-promises] false negative calling .then with second argument undefined #6881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
41c7c3f
fix(eslint-plugin): [no-floating-promises] False negative calling .th…
kirkwaiblinger Apr 11, 2023
8708fc0
use getTypeAtLocation correctly
kirkwaiblinger Jul 14, 2023
2452b67
clean up duplicate
kirkwaiblinger Jul 14, 2023
0f1a292
correct mistake in IIFE abbreviation
kirkwaiblinger Jul 14, 2023
822ddc6
purge the word invocated from the codebase
kirkwaiblinger Jul 14, 2023
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
2 changes: 1 addition & 1 deletion packages/eslint-plugin/docs/rules/no-floating-promises.md
View file
Open in desktop
Expand Up @@ -83,7 +83,7 @@ With this option set to `true`, and if you are using `no-void`, you should turn

### `ignoreIIFE`

This allows you to skip checking of async IIFEs (Immediately Invocated function Expressions).
This allows you to skip checking of async IIFEs (Immediately Invoked function Expressions).

Examples of **correct** code for this rule with `{ ignoreIIFE: true }`:

Expand Down
142 changes: 105 additions & 37 deletions packages/eslint-plugin/src/rules/no-floating-promises.ts
View file
Open in desktop
JoshuaKGoldberg 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 @@ -15,9 +15,21 @@ type Options = [

type MessageId =
| 'floating'
| 'floatingVoid'
| 'floatingUselessRejectionHandler'
| 'floatingUselessRejectionHandlerVoid'
| 'floatingFixAwait'
| 'floatingFixVoid'
| 'floatingVoid';
| 'floatingFixVoid';

const messageBase =
'Promises must be awaited, end with a call to .catch, or end with a call to .then with a rejection handler.';

const messageBaseVoid =
'Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler' +
' or be explicitly marked as ignored with the `void` operator.';

const messageRejectionHandler =
'A rejection handler that is not a function will be ignored.';

export default util.createRule<Options, MessageId>({
name: 'no-floating-promises',
Expand All @@ -30,13 +42,14 @@ export default util.createRule<Options, MessageId>({
},
hasSuggestions: true,
messages: {
floating:
'Promises must be awaited, end with a call to .catch, or end with a call to .then with a rejection handler.',
floating: messageBase,
floatingFixAwait: 'Add await operator.',
floatingVoid:
'Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler' +
' or be explicitly marked as ignored with the `void` operator.',
floatingVoid: messageBaseVoid,
floatingFixVoid: 'Add void operator to ignore.',
floatingUselessRejectionHandler:
messageBase + ' ' + messageRejectionHandler,
floatingUselessRejectionHandlerVoid:
messageBaseVoid + ' ' + messageRejectionHandler,
},
schema: [
{
Expand All @@ -48,7 +61,7 @@ export default util.createRule<Options, MessageId>({
},
ignoreIIFE: {
description:
'Whether to ignore async IIFEs (Immediately Invocated Function Expressions).',
'Whether to ignore async IIFEs (Immediately Invoked Function Expressions).',
type: 'boolean',
},
},
Expand Down Expand Up @@ -80,11 +93,18 @@ export default util.createRule<Options, MessageId>({
expression = expression.expression;
}

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

if (isUnhandled) {
if (options.ignoreVoid) {
context.report({
node,
messageId: 'floatingVoid',
messageId: nonFunctionHandler
? 'floatingUselessRejectionHandlerVoid'
: 'floatingVoid',
suggest: [
{
messageId: 'floatingFixVoid',
Expand All @@ -110,7 +130,9 @@ export default util.createRule<Options, MessageId>({
} else {
context.report({
node,
messageId: 'floating',
messageId: nonFunctionHandler
? 'floatingUselessRejectionHandler'
: 'floating',
suggest: [
{
messageId: 'floatingFixAwait',
Expand Down Expand Up @@ -168,16 +190,31 @@ export default util.createRule<Options, MessageId>({
);
}

function isValidRejectionHandler(rejectionHandler: TSESTree.Node): boolean {
return (
services.program
.getTypeChecker()
.getTypeAtLocation(
services.esTreeNodeToTSNodeMap.get(rejectionHandler),
)
.getCallSignatures().length > 0
);
}

function isUnhandledPromise(
checker: ts.TypeChecker,
node: TSESTree.Node,
): boolean {
): { isUnhandled: boolean; nonFunctionHandler?: 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
// promise, so we check them all regardless of whether the final returned
// value is promise-like.
return node.expressions.some(item => isUnhandledPromise(checker, item));
return (
node.expressions
.map(item => isUnhandledPromise(checker, item))
.find(result => result.isUnhandled) ?? { isUnhandled: false }
);
}

if (
Expand All @@ -192,24 +229,45 @@ export default util.createRule<Options, MessageId>({

// Check the type. At this point it can't be unhandled if it isn't a promise
if (!isPromiseLike(checker, services.esTreeNodeToTSNodeMap.get(node))) {
return false;
return { isUnhandled: false };
}

if (node.type === AST_NODE_TYPES.CallExpression) {
// If the outer expression is a call, it must be either a `.then()` or
// `.catch()` that handles the promise.
return (
!isPromiseCatchCallWithHandler(node) &&
!isPromiseThenCallWithRejectionHandler(node) &&
!isPromiseFinallyCallWithHandler(node)
);

const catchRejectionHandler = getRejectionHandlerFromCatchCall(node);
if (catchRejectionHandler) {
if (isValidRejectionHandler(catchRejectionHandler)) {
return { isUnhandled: false };
} else {
return { isUnhandled: true, nonFunctionHandler: true };
}
}

const thenRejectionHandler = getRejectionHandlerFromThenCall(node);
if (thenRejectionHandler) {
if (isValidRejectionHandler(thenRejectionHandler)) {
return { isUnhandled: false };
} else {
return { isUnhandled: true, nonFunctionHandler: true };
}
}

if (isPromiseFinallyCallWithHandler(node)) {
return { isUnhandled: false };
}

return { isUnhandled: true };
} else if (node.type === AST_NODE_TYPES.ConditionalExpression) {
// We must be getting the promise-like value from one of the branches of the
// ternary. Check them directly.
return (
isUnhandledPromise(checker, node.alternate) ||
isUnhandledPromise(checker, node.consequent)
);
const alternateResult = isUnhandledPromise(checker, node.alternate);
if (alternateResult.isUnhandled) {
return alternateResult;
} else {
return isUnhandledPromise(checker, node.consequent);
}
} else if (
node.type === AST_NODE_TYPES.MemberExpression ||
node.type === AST_NODE_TYPES.Identifier ||
Expand All @@ -218,18 +276,20 @@ export default util.createRule<Options, MessageId>({
// If it is just a property access chain or a `new` call (e.g. `foo.bar` or
// `new Promise()`), the promise is not handled because it doesn't have the
// necessary then/catch call at the end of the chain.
return true;
return { isUnhandled: true };
} else if (node.type === AST_NODE_TYPES.LogicalExpression) {
return (
isUnhandledPromise(checker, node.left) ||
isUnhandledPromise(checker, node.right)
);
const leftResult = isUnhandledPromise(checker, node.left);
if (leftResult.isUnhandled) {
return leftResult;
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
} else {
return isUnhandledPromise(checker, node.right);
}
}

// We conservatively return false for all other types of expressions because
// we don't want to accidentally fail if the promise is handled internally but
// we just can't tell.
return false;
return { isUnhandled: false };
}
},
});
Expand Down Expand Up @@ -291,26 +351,34 @@ function isFunctionParam(
return false;
}

function isPromiseCatchCallWithHandler(
function getRejectionHandlerFromCatchCall(
expression: TSESTree.CallExpression,
): boolean {
return (
): TSESTree.CallExpressionArgument | undefined {
if (
expression.callee.type === AST_NODE_TYPES.MemberExpression &&
expression.callee.property.type === AST_NODE_TYPES.Identifier &&
expression.callee.property.name === 'catch' &&
expression.arguments.length >= 1
);
) {
return expression.arguments[0];
} else {
return undefined;
}
}

function isPromiseThenCallWithRejectionHandler(
function getRejectionHandlerFromThenCall(
expression: TSESTree.CallExpression,
): boolean {
return (
): TSESTree.CallExpressionArgument | undefined {
if (
expression.callee.type === AST_NODE_TYPES.MemberExpression &&
expression.callee.property.type === AST_NODE_TYPES.Identifier &&
expression.callee.property.name === 'then' &&
expression.arguments.length >= 2
);
) {
return expression.arguments[1];
} else {
return undefined;
}
}

function isPromiseFinallyCallWithHandler(
Expand Down