diff --git a/packages/eslint-plugin/docs/rules/only-throw-error.mdx b/packages/eslint-plugin/docs/rules/only-throw-error.mdx index 49d980a127a2..e30f67a0869b 100644 --- a/packages/eslint-plugin/docs/rules/only-throw-error.mdx +++ b/packages/eslint-plugin/docs/rules/only-throw-error.mdx @@ -123,6 +123,11 @@ interface Options { | string )[]; + /** + * Whether to allow rethrowing caught values that are not `Error` objects. + */ + allowRethrowing?: boolean; + /** * Whether to always allow throwing values typed as `any`. */ @@ -136,6 +141,7 @@ interface Options { const defaultOptions: Options = { allow: [], + allowRethrowing: false, allowThrowingAny: true, allowThrowingUnknown: true, }; diff --git a/packages/eslint-plugin/src/rules/no-floating-promises.ts b/packages/eslint-plugin/src/rules/no-floating-promises.ts index 8e91c1987b62..a19d057c545c 100644 --- a/packages/eslint-plugin/src/rules/no-floating-promises.ts +++ b/packages/eslint-plugin/src/rules/no-floating-promises.ts @@ -10,7 +10,6 @@ import { createRule, getOperatorPrecedence, getParserServices, - getStaticMemberAccessValue, isBuiltinSymbolLike, OperatorPrecedence, readonlynessOptionsDefaults, @@ -18,6 +17,11 @@ import { skipChainExpression, typeMatchesSomeSpecifier, } from '../util'; +import { + parseCatchCall, + parseFinallyCall, + parseThenCall, +} from '../util/promiseUtils'; export type Options = [ { @@ -336,38 +340,23 @@ export default createRule({ // If the outer expression is a call, a `.catch()` or `.then()` with // rejection handler handles the promise. - const { callee } = node; - if (callee.type === AST_NODE_TYPES.MemberExpression) { - const methodName = getStaticMemberAccessValue(callee, context); - const catchRejectionHandler = - methodName === 'catch' && node.arguments.length >= 1 - ? node.arguments[0] - : undefined; - if (catchRejectionHandler) { - if (isValidRejectionHandler(catchRejectionHandler)) { + const promiseHandlingMethodCall = + parseCatchCall(node, context) ?? parseThenCall(node, context); + if (promiseHandlingMethodCall != null) { + const onRejected = promiseHandlingMethodCall.onRejected; + if (onRejected != null) { + if (isValidRejectionHandler(onRejected)) { return { isUnhandled: false }; } return { isUnhandled: true, nonFunctionHandler: true }; } + return { isUnhandled: true }; + } - const thenRejectionHandler = - methodName === 'then' && node.arguments.length >= 2 - ? node.arguments[1] - : undefined; - if (thenRejectionHandler) { - if (isValidRejectionHandler(thenRejectionHandler)) { - return { isUnhandled: false }; - } - return { isUnhandled: true, nonFunctionHandler: true }; - } + const promiseFinallyCall = parseFinallyCall(node, context); - // `x.finally()` is transparent to resolution of the promise, so check `x`. - // ("object" in this context is the `x` in `x.finally()`) - const promiseFinallyObject = - methodName === 'finally' ? callee.object : undefined; - if (promiseFinallyObject) { - return isUnhandledPromise(checker, promiseFinallyObject); - } + if (promiseFinallyCall != null) { + return isUnhandledPromise(checker, promiseFinallyCall.object); } // All other cases are unhandled. diff --git a/packages/eslint-plugin/src/rules/only-throw-error.ts b/packages/eslint-plugin/src/rules/only-throw-error.ts index ada9779653a3..187e6ac5e5b1 100644 --- a/packages/eslint-plugin/src/rules/only-throw-error.ts +++ b/packages/eslint-plugin/src/rules/only-throw-error.ts @@ -1,25 +1,30 @@ import type { TSESTree } from '@typescript-eslint/utils'; import { AST_NODE_TYPES } from '@typescript-eslint/utils'; +import { isThenableType } from 'ts-api-utils'; import * as ts from 'typescript'; import type { TypeOrValueSpecifier } from '../util'; import { createRule, + findVariable, getParserServices, isErrorLike, isTypeAnyType, isTypeUnknownType, typeMatchesSomeSpecifier, typeOrValueSpecifiersSchema, + nullThrows, } from '../util'; +import { parseCatchCall, parseThenCall } from '../util/promiseUtils'; export type MessageIds = 'object' | 'undef'; export type Options = [ { allow?: TypeOrValueSpecifier[]; + allowRethrowing?: boolean; allowThrowingAny?: boolean; allowThrowingUnknown?: boolean; }, @@ -48,6 +53,11 @@ export default createRule({ ...typeOrValueSpecifiersSchema, description: 'Type specifiers that can be thrown.', }, + allowRethrowing: { + type: 'boolean', + description: + 'Whether to allow rethrowing caught values that are not `Error` objects.', + }, allowThrowingAny: { type: 'boolean', description: @@ -65,6 +75,7 @@ export default createRule({ defaultOptions: [ { allow: [], + allowRethrowing: true, allowThrowingAny: true, allowThrowingUnknown: true, }, @@ -72,6 +83,65 @@ export default createRule({ create(context, [options]) { const services = getParserServices(context); const allow = options.allow; + + function isRethrownError(node: TSESTree.Node): boolean { + if (node.type !== AST_NODE_TYPES.Identifier) { + return false; + } + + const scope = context.sourceCode.getScope(node); + + const smVariable = nullThrows( + findVariable(scope, node), + `Variable ${node.name} should exist in scope manager`, + ); + + const variableDefinitions = smVariable.defs.filter( + def => def.isVariableDefinition, + ); + if (variableDefinitions.length !== 1) { + return false; + } + const def = smVariable.defs[0]; + + // try { /* ... */ } catch (x) { throw x; } + if (def.node.type === AST_NODE_TYPES.CatchClause) { + return true; + } + + // promise.catch(x => { throw x; }) + // promise.then(onFulfilled, x => { throw x; }) + if ( + def.node.type === AST_NODE_TYPES.ArrowFunctionExpression && + def.node.params.length >= 1 && + def.node.params[0] === def.name && + def.node.parent.type === AST_NODE_TYPES.CallExpression + ) { + const callExpression = def.node.parent; + + const parsedPromiseHandlingCall = + parseCatchCall(callExpression, context) ?? + parseThenCall(callExpression, context); + if (parsedPromiseHandlingCall != null) { + const { object, onRejected } = parsedPromiseHandlingCall; + if (onRejected === def.node) { + const tsObjectNode = services.esTreeNodeToTSNodeMap.get( + object, + ) as ts.Expression; + + // make sure we're actually dealing with a promise + if ( + isThenableType(services.program.getTypeChecker(), tsObjectNode) + ) { + return true; + } + } + } + } + + return false; + } + function checkThrowArgument(node: TSESTree.Node): void { if ( node.type === AST_NODE_TYPES.AwaitExpression || @@ -80,6 +150,10 @@ export default createRule({ return; } + if (options.allowRethrowing && isRethrownError(node)) { + return; + } + const type = services.getTypeAtLocation(node); if (typeMatchesSomeSpecifier(type, allow, services.program)) { diff --git a/packages/eslint-plugin/src/util/promiseUtils.ts b/packages/eslint-plugin/src/util/promiseUtils.ts new file mode 100644 index 000000000000..3cbbec7a6fa6 --- /dev/null +++ b/packages/eslint-plugin/src/util/promiseUtils.ts @@ -0,0 +1,131 @@ +import type { TSESTree } from '@typescript-eslint/utils'; +import type { RuleContext } from '@typescript-eslint/utils/ts-eslint'; + +import { AST_NODE_TYPES } from '@typescript-eslint/utils'; + +import { getStaticMemberAccessValue } from './misc'; + +/** + * Parses a syntactically possible `Promise.then()` call. Does not check the + * type of the callee. + */ +export function parseThenCall( + node: TSESTree.CallExpression, + context: RuleContext, +): + | { + onFulfilled?: TSESTree.Expression | undefined; + onRejected?: TSESTree.Expression | undefined; + object: TSESTree.Expression; + } + | undefined { + if (node.callee.type === AST_NODE_TYPES.MemberExpression) { + const methodName = getStaticMemberAccessValue(node.callee, context); + if (methodName === 'then') { + if (node.arguments.length >= 1) { + if (node.arguments[0].type === AST_NODE_TYPES.SpreadElement) { + return { + object: node.callee.object, + }; + } + + if (node.arguments.length >= 2) { + if (node.arguments[1].type === AST_NODE_TYPES.SpreadElement) { + return { + object: node.callee.object, + onFulfilled: node.arguments[0], + }; + } + + return { + object: node.callee.object, + onFulfilled: node.arguments[0], + onRejected: node.arguments[1], + }; + } + return { + object: node.callee.object, + onFulfilled: node.arguments[0], + }; + } + return { + object: node.callee.object, + }; + } + } + + return undefined; +} + +/** + * Parses a syntactically possible `Promise.catch()` call. Does not check the + * type of the callee. + */ +export function parseCatchCall( + node: TSESTree.CallExpression, + context: RuleContext, +): + | { + onRejected?: TSESTree.Expression | undefined; + object: TSESTree.Expression; + } + | undefined { + if (node.callee.type === AST_NODE_TYPES.MemberExpression) { + const methodName = getStaticMemberAccessValue(node.callee, context); + if (methodName === 'catch') { + if (node.arguments.length >= 1) { + if (node.arguments[0].type === AST_NODE_TYPES.SpreadElement) { + return { + object: node.callee.object, + }; + } + + return { + object: node.callee.object, + onRejected: node.arguments[0], + }; + } + return { + object: node.callee.object, + }; + } + } + + return undefined; +} + +/** + * Parses a syntactically possible `Promise.finally()` call. Does not check the + * type of the callee. + */ +export function parseFinallyCall( + node: TSESTree.CallExpression, + context: RuleContext, +): + | { + object: TSESTree.Expression; + onFinally?: TSESTree.Expression | undefined; + } + | undefined { + if (node.callee.type === AST_NODE_TYPES.MemberExpression) { + const methodName = getStaticMemberAccessValue(node.callee, context); + if (methodName === 'finally') { + if (node.arguments.length >= 1) { + if (node.arguments[0].type === AST_NODE_TYPES.SpreadElement) { + return { + object: node.callee.object, + }; + } + return { + object: node.callee.object, + onFinally: node.arguments[0], + }; + } + return { + object: node.callee.object, + }; + } + } + + return undefined; +} diff --git a/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts b/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts index 609c091eaae5..a983a40f1368 100644 --- a/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts @@ -5493,5 +5493,101 @@ await (>{}); }, ], }, + { + code: ` +Promise.reject('foo').then(); + `, + errors: [ + { + messageId: 'floatingVoid', + suggestions: [ + { + messageId: 'floatingFixVoid', + output: ` +void Promise.reject('foo').then(); + `, + }, + { + messageId: 'floatingFixAwait', + output: ` +await Promise.reject('foo').then(); + `, + }, + ], + }, + ], + }, + { + code: ` +Promise.reject('foo').finally(); + `, + errors: [ + { + messageId: 'floatingVoid', + suggestions: [ + { + messageId: 'floatingFixVoid', + output: ` +void Promise.reject('foo').finally(); + `, + }, + { + messageId: 'floatingFixAwait', + output: ` +await Promise.reject('foo').finally(); + `, + }, + ], + }, + ], + }, + { + code: ` +Promise.reject('foo').finally(...[], () => {}); + `, + errors: [ + { + messageId: 'floatingVoid', + suggestions: [ + { + messageId: 'floatingFixVoid', + output: ` +void Promise.reject('foo').finally(...[], () => {}); + `, + }, + { + messageId: 'floatingFixAwait', + output: ` +await Promise.reject('foo').finally(...[], () => {}); + `, + }, + ], + }, + ], + }, + { + code: ` +Promise.reject('foo').then(...[], () => {}); + `, + errors: [ + { + messageId: 'floatingVoid', + suggestions: [ + { + messageId: 'floatingFixVoid', + output: ` +void Promise.reject('foo').then(...[], () => {}); + `, + }, + { + messageId: 'floatingFixAwait', + output: ` +await Promise.reject('foo').then(...[], () => {}); + `, + }, + ], + }, + ], + }, ], }); diff --git a/packages/eslint-plugin/tests/rules/only-throw-error.test.ts b/packages/eslint-plugin/tests/rules/only-throw-error.test.ts index c7d52c6a1e1c..a85d65cd85e5 100644 --- a/packages/eslint-plugin/tests/rules/only-throw-error.test.ts +++ b/packages/eslint-plugin/tests/rules/only-throw-error.test.ts @@ -189,6 +189,60 @@ throw new Map(); }, ], }, + { + code: ` +try { +} catch (e) { + throw e; +} + `, + options: [ + { + allowRethrowing: true, + allowThrowingAny: false, + allowThrowingUnknown: false, + }, + ], + }, + { + code: ` +try { +} catch (eOuter) { + try { + if (Math.random() > 0.5) { + throw eOuter; + } + } catch (eInner) { + if (Math.random() > 0.5) { + throw eOuter; + } else { + throw eInner; + } + } +} + `, + options: [ + { + allowRethrowing: true, + allowThrowingAny: false, + allowThrowingUnknown: false, + }, + ], + }, + { + code: ` +Promise.reject('foo').catch(e => { + throw e; +}); + `, + options: [ + { + allowRethrowing: true, + allowThrowingAny: false, + allowThrowingUnknown: false, + }, + ], + }, ], invalid: [ { @@ -553,5 +607,143 @@ throw new UnknownError(); }, ], }, + { + code: ` +let x = 1; +Promise.reject('foo').catch(e => { + throw x; +}); + `, + errors: [ + { + messageId: 'object', + }, + ], + options: [ + { + allowRethrowing: true, + allowThrowingAny: false, + allowThrowingUnknown: false, + }, + ], + }, + { + code: ` +Promise.reject('foo').catch((...e) => { + throw e; +}); + `, + errors: [ + { + messageId: 'object', + }, + ], + options: [ + { + allowRethrowing: true, + allowThrowingAny: false, + allowThrowingUnknown: false, + }, + ], + }, + { + code: ` +declare const x: any[]; +Promise.reject('foo').catch(...x, e => { + throw e; +}); + `, + errors: [ + { + messageId: 'object', + }, + ], + options: [ + { + allowRethrowing: true, + allowThrowingAny: false, + allowThrowingUnknown: false, + }, + ], + }, + { + code: ` +declare const x: any[]; +Promise.reject('foo').then(...x, e => { + throw e; +}); + `, + errors: [ + { + messageId: 'object', + }, + ], + options: [ + { + allowRethrowing: true, + allowThrowingAny: false, + allowThrowingUnknown: false, + }, + ], + }, + { + code: ` +declare const onFulfilled: any; +declare const x: any[]; +Promise.reject('foo').then(onFulfilled, ...x, e => { + throw e; +}); + `, + errors: [ + { + messageId: 'object', + }, + ], + options: [ + { + allowRethrowing: true, + allowThrowingAny: false, + allowThrowingUnknown: false, + }, + ], + }, + { + code: ` +Promise.reject('foo').then((...e) => { + throw e; +}); + `, + errors: [ + { + messageId: 'object', + }, + ], + options: [ + { + allowRethrowing: true, + allowThrowingAny: false, + allowThrowingUnknown: false, + }, + ], + }, + { + code: ` +Promise.reject('foo').then(e => { + throw globalThis; +}); + `, + errors: [ + { + messageId: 'object', + }, + ], + options: [ + { + allowRethrowing: true, + allowThrowingAny: false, + allowThrowingUnknown: false, + }, + ], + }, ], }); diff --git a/packages/eslint-plugin/tests/schema-snapshots/only-throw-error.shot b/packages/eslint-plugin/tests/schema-snapshots/only-throw-error.shot index 8727d31475d7..9bcf6d14e382 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/only-throw-error.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/only-throw-error.shot @@ -100,6 +100,10 @@ }, "type": "array" }, + "allowRethrowing": { + "description": "Whether to allow rethrowing caught values that are not `Error` objects.", + "type": "boolean" + }, "allowThrowingAny": { "description": "Whether to always allow throwing values typed as `any`.", "type": "boolean" @@ -136,6 +140,8 @@ type Options = [ } | string )[]; + /** Whether to allow rethrowing caught values that are not `Error` objects. */ + allowRethrowing?: boolean; /** Whether to always allow throwing values typed as `any`. */ allowThrowingAny?: boolean; /** Whether to always allow throwing values typed as `unknown`. */