Skip to content

Commit

Permalink
fix(eslint-plugin): [no-floating-promises] false negative calling .th…
Browse files Browse the repository at this point in the history 
…en with second argument undefined (#6881)

* fix(eslint-plugin): [no-floating-promises] False negative calling .then with second argument undefined (Issue #6850)

* use getTypeAtLocation correctly

* clean up duplicate

* correct mistake in IIFE abbreviation

* purge the word invocated from the codebase
  • Loading branch information
@kirkwaiblinger
kirkwaiblinger authored Jul 15, 2023
1 parent d740de3 commit 606a52c
Show file tree
Hide file tree
Showing 4 changed files with 274 additions and 48 deletions.
2 changes: 1 addition & 1 deletion packages/eslint-plugin/docs/rules/no-floating-promises.md
View file
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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;
} 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
Loading

0 comments on commit 606a52c

Please sign in to comment.