From 19e7528573729b8eebb545bdf51396b82c675eed Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sun, 1 Dec 2024 12:25:50 -0700 Subject: [PATCH 1/4] only flag literal handlers --- .../use-unknown-in-catch-callback-variable.ts | 48 ++++- ...unknown-in-catch-callback-variable.test.ts | 174 ++++++++++++++---- 2 files changed, 182 insertions(+), 40 deletions(-) diff --git a/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts b/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts index a7d2c53b1a2a..7826443e868f 100644 --- a/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts +++ b/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts @@ -100,10 +100,41 @@ export default createRule<[], MessageIds>({ return false; } - function shouldFlagArgument(node: TSESTree.Expression): boolean { - const argument = esTreeNodeToTSNodeMap.get(node); - const typeOfArgument = checker.getTypeAtLocation(argument); - return isFlaggableHandlerType(typeOfArgument); + function collectFlaggedNodes( + node: Exclude, + ): Exclude[] { + switch (node.type) { + case AST_NODE_TYPES.LogicalExpression: + return [ + ...collectFlaggedNodes(node.left), + ...collectFlaggedNodes(node.right), + ]; + case AST_NODE_TYPES.SequenceExpression: + return collectFlaggedNodes( + nullThrows( + node.expressions.at(-1), + 'seuence expression must have multiple expressions', + ), + ); + case AST_NODE_TYPES.ConditionalExpression: + return [ + ...collectFlaggedNodes(node.consequent), + ...collectFlaggedNodes(node.alternate), + ]; + case AST_NODE_TYPES.ArrowFunctionExpression: + case AST_NODE_TYPES.FunctionExpression: + { + const argument = esTreeNodeToTSNodeMap.get(node); + const typeOfArgument = checker.getTypeAtLocation(argument); + if (isFlaggableHandlerType(typeOfArgument)) { + return [node]; + } + } + break; + default: + break; + } + return []; } /** @@ -114,7 +145,7 @@ export default createRule<[], MessageIds>({ * rule _is reporting_, so it is not guaranteed to be sound to call otherwise. */ function refineReportIfPossible( - argument: TSESTree.Expression, + argument: Exclude, ): Partial> | undefined { // Only know how to be helpful if a function literal has been provided. if ( @@ -277,11 +308,12 @@ export default createRule<[], MessageIds>({ } // the `some` check above has already excluded `SpreadElement`, so we are safe to assert the same - const node = argsToCheck[argIndexToCheck] as Exclude< - (typeof argsToCheck)[number], + const argToCheck = argsToCheck[argIndexToCheck] as Exclude< + TSESTree.Node, TSESTree.SpreadElement >; - if (shouldFlagArgument(node)) { + + for (const node of collectFlaggedNodes(argToCheck)) { // We are now guaranteed to report, but we have a bit of work to do // to determine exactly where, and whether we can fix it. const overrides = refineReportIfPossible(node); diff --git a/packages/eslint-plugin/tests/rules/use-unknown-in-catch-callback-variable.test.ts b/packages/eslint-plugin/tests/rules/use-unknown-in-catch-callback-variable.test.ts index aa7cee9f9f6d..13989e5a2f45 100644 --- a/packages/eslint-plugin/tests/rules/use-unknown-in-catch-callback-variable.test.ts +++ b/packages/eslint-plugin/tests/rules/use-unknown-in-catch-callback-variable.test.ts @@ -163,6 +163,21 @@ Promise.resolve().catch(...x); declare const thenArgs: [() => {}, (err: any) => {}]; Promise.resolve().then(...thenArgs); `, + // this is valid, because the `any` is a passed-in handler, not a function literal. + // https://github.com/typescript-eslint/typescript-eslint/issues/9057 + ` +declare const yoloHandler: (x: any) => void; +Promise.reject(new Error('I will reject!')).catch(yoloHandler); + `, + // type assertion is not a function literal. + ` +type InvalidHandler = (arg: any) => void; +Promise.resolve().catch(( + function (err /* awkward spot for comment */) { + throw err; + } +)); + `, ], invalid: [ @@ -347,25 +362,6 @@ Promise.resolve().catch(function (err: unknown /* awkward spot for comment */) { ], }, - { - code: ` -type InvalidHandler = (arg: any) => void; -Promise.resolve().catch(( - function (err /* awkward spot for comment */) { - throw err; - } -)); - `, - errors: [ - { - line: 3, - messageId: 'useUnknown', - // We should _not_ make a suggestion due to the type assertion. - suggestions: null, - }, - ], - }, - { code: ` Promise.resolve().catch(function namedCallback(err: string) { @@ -598,19 +594,6 @@ Promise.reject(new Error('I will reject!')).catch(([err]: [unknown]) => { ], }, - { - code: ` -declare const yoloHandler: (x: any) => void; -Promise.reject(new Error('I will reject!')).catch(yoloHandler); - `, - errors: [ - { - line: 3, - messageId: 'useUnknown', - }, - ], - }, - { code: ` Promise.resolve(' a string ').catch( @@ -748,5 +731,132 @@ Promise.resolve().catch((...{ find }: [unknown]) => { }, ], }, + + { + code: ` +declare const condition: boolean; +Promise.resolve('foo').then(() => {}, condition ? err => {} : err => {}); + `, + errors: [ + { + column: 51, + endColumn: 60, + endLine: 3, + line: 3, + + messageId: 'useUnknown', + + suggestions: [ + { + messageId: 'addUnknownTypeAnnotationSuggestion', + output: ` +declare const condition: boolean; +Promise.resolve('foo').then(() => {}, condition ? (err: unknown) => {} : err => {}); + `, + }, + ], + }, + { + column: 63, + endColumn: 72, + endLine: 3, + line: 3, + + messageId: 'useUnknown', + + suggestions: [ + { + messageId: 'addUnknownTypeAnnotationSuggestion', + output: ` +declare const condition: boolean; +Promise.resolve('foo').then(() => {}, condition ? err => {} : (err: unknown) => {}); + `, + }, + ], + }, + ], + }, + { + code: ` +declare const condition: boolean; +declare const maybeNullishHandler: null | ((err: any) => void); +Promise.resolve('foo').catch( + condition + ? (err => {}, err => {}, maybeNullishHandler) ?? (err => {}) + : (condition && (err => {})) || (err => {}), +); + `, + errors: [ + { + column: 55, + endColumn: 64, + endLine: 6, + line: 6, + + messageId: 'useUnknown', + + suggestions: [ + { + messageId: 'addUnknownTypeAnnotationSuggestion', + output: ` +declare const condition: boolean; +declare const maybeNullishHandler: null | ((err: unknown) => void); +Promise.resolve('foo').catch( + condition + ? (err => {}, err => {}, maybeNullishHandler) ?? ((err: unknown) => {}) + : (condition && (err => {})) || (err => {}), +); + `, + }, + ], + }, + { + column: 22, + endColumn: 31, + endLine: 7, + line: 7, + + messageId: 'useUnknown', + + suggestions: [ + { + messageId: 'addUnknownTypeAnnotationSuggestion', + output: ` +declare const condition: boolean; +declare const maybeNullishHandler: null | (err => void); +Promise.resolve('foo').catch( + condition + ? (err => {}, err => {}, maybeNullishHandler) ?? ((err: unknown) => {}) + : (condition && ((err: unknown) => {})) || (err => {}), +); + `, + }, + ], + }, + { + column: 38, + endColumn: 47, + endLine: 7, + line: 7, + + messageId: 'useUnknown', + + suggestions: [ + { + messageId: 'addUnknownTypeAnnotationSuggestion', + output: ` +declare const condition: boolean; +declare const maybeNullishHandler: null | (err => void); +Promise.resolve('foo').catch( + condition + ? (err => {}, err => {}, maybeNullishHandler) ?? ((err: unknown) => {}) + : (condition && (err => {})) || ((err: unknown) => {}), +); + `, + }, + ], + }, + ], + }, ], }); From 8e2da575d326715a0d8e085fa23475aabb064a75 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sun, 1 Dec 2024 12:36:13 -0700 Subject: [PATCH 2/4] noice --- ...unknown-in-catch-callback-variable.test.ts | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/use-unknown-in-catch-callback-variable.test.ts b/packages/eslint-plugin/tests/rules/use-unknown-in-catch-callback-variable.test.ts index 13989e5a2f45..9be3500b1481 100644 --- a/packages/eslint-plugin/tests/rules/use-unknown-in-catch-callback-variable.test.ts +++ b/packages/eslint-plugin/tests/rules/use-unknown-in-catch-callback-variable.test.ts @@ -740,7 +740,7 @@ Promise.resolve('foo').then(() => {}, condition ? err => {} : err => {}); errors: [ { column: 51, - endColumn: 60, + endColumn: 54, endLine: 3, line: 3, @@ -758,7 +758,7 @@ Promise.resolve('foo').then(() => {}, condition ? (err: unknown) => {} : err => }, { column: 63, - endColumn: 72, + endColumn: 66, endLine: 3, line: 3, @@ -789,7 +789,7 @@ Promise.resolve('foo').catch( errors: [ { column: 55, - endColumn: 64, + endColumn: 58, endLine: 6, line: 6, @@ -800,7 +800,7 @@ Promise.resolve('foo').catch( messageId: 'addUnknownTypeAnnotationSuggestion', output: ` declare const condition: boolean; -declare const maybeNullishHandler: null | ((err: unknown) => void); +declare const maybeNullishHandler: null | ((err: any) => void); Promise.resolve('foo').catch( condition ? (err => {}, err => {}, maybeNullishHandler) ?? ((err: unknown) => {}) @@ -812,7 +812,7 @@ Promise.resolve('foo').catch( }, { column: 22, - endColumn: 31, + endColumn: 25, endLine: 7, line: 7, @@ -823,10 +823,10 @@ Promise.resolve('foo').catch( messageId: 'addUnknownTypeAnnotationSuggestion', output: ` declare const condition: boolean; -declare const maybeNullishHandler: null | (err => void); +declare const maybeNullishHandler: null | ((err: any) => void); Promise.resolve('foo').catch( condition - ? (err => {}, err => {}, maybeNullishHandler) ?? ((err: unknown) => {}) + ? (err => {}, err => {}, maybeNullishHandler) ?? (err => {}) : (condition && ((err: unknown) => {})) || (err => {}), ); `, @@ -835,7 +835,7 @@ Promise.resolve('foo').catch( }, { column: 38, - endColumn: 47, + endColumn: 41, endLine: 7, line: 7, @@ -846,10 +846,10 @@ Promise.resolve('foo').catch( messageId: 'addUnknownTypeAnnotationSuggestion', output: ` declare const condition: boolean; -declare const maybeNullishHandler: null | (err => void); +declare const maybeNullishHandler: null | ((err: any) => void); Promise.resolve('foo').catch( condition - ? (err => {}, err => {}, maybeNullishHandler) ?? ((err: unknown) => {}) + ? (err => {}, err => {}, maybeNullishHandler) ?? (err => {}) : (condition && (err => {})) || ((err: unknown) => {}), ); `, From 2dedf46d1f9b5d5420095704bf66765778f5e4e7 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sun, 1 Dec 2024 12:37:49 -0700 Subject: [PATCH 3/4] tighten types --- .../use-unknown-in-catch-callback-variable.ts | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts b/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts index 7826443e868f..ab0e9fd56d26 100644 --- a/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts +++ b/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts @@ -102,7 +102,7 @@ export default createRule<[], MessageIds>({ function collectFlaggedNodes( node: Exclude, - ): Exclude[] { + ): (TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression)[] { switch (node.type) { case AST_NODE_TYPES.LogicalExpression: return [ @@ -145,18 +145,8 @@ export default createRule<[], MessageIds>({ * rule _is reporting_, so it is not guaranteed to be sound to call otherwise. */ function refineReportIfPossible( - argument: Exclude, + argument: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, ): Partial> | undefined { - // Only know how to be helpful if a function literal has been provided. - if ( - !( - argument.type === AST_NODE_TYPES.ArrowFunctionExpression || - argument.type === AST_NODE_TYPES.FunctionExpression - ) - ) { - return undefined; - } - const catchVariableOuterWithIncorrectTypes = nullThrows( argument.params.at(0), 'There should have been at least one parameter for the rule to have flagged.', From 71b76161ba9af50e6a08bc70ce4814c7e7eb2958 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josh=20Goldberg=20=E2=9C=A8?= Date: Mon, 9 Dec 2024 09:43:40 -0500 Subject: [PATCH 4/4] Update packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts --- .../src/rules/use-unknown-in-catch-callback-variable.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts b/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts index ab0e9fd56d26..0ee671556d13 100644 --- a/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts +++ b/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts @@ -113,7 +113,7 @@ export default createRule<[], MessageIds>({ return collectFlaggedNodes( nullThrows( node.expressions.at(-1), - 'seuence expression must have multiple expressions', + 'sequence expression must have multiple expressions', ), ); case AST_NODE_TYPES.ConditionalExpression: