10000 fix(eslint-plugin): [use-unknown-in-catch-callback-variable] flag sec… · jakebailey/typescript-eslint@75a09a8 · GitHub
[go: up one dir, main page]
Skip to content

Commit 75a09a8

Browse files
abrahamguoabrahamguo
authored
fix(eslint-plugin): [use-unknown-in-catch-callback-variable] flag second argument of .then (typescript-eslint#9059)
* add failing case * WIP writing conditionals * WIP * WIP * finish logic, theoretically * add output * WIP adding tests * finish tests * account for too few arguments * add more tests * make messages more specific * update docs * message data is mandatory * update docs snapshot * remove spread arg checking * change false to undefined * refactor * add clarifying comment * clean up docs * re-lint * refactor * clarify comment * small tweaks * add missing scope
1 parent fd57da9 commit 75a09a8

File tree

4 files changed

+208
-217
lines changed

4 files changed

+208
-217
lines changed

‎packages/eslint-plugin/docs/rules/use-unknown-in-catch-callback-variable.mdx

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
---
2-
description: 'Enforce typing arguments in `.catch()` callbacks as `unknown`.'
2+
description: 'Enforce typing arguments in Promise rejection callbacks as `unknown`.'
33
---
44

55
import Tabs from '@theme/Tabs';
@@ -9,7 +9,7 @@ import TabItem from '@theme/TabItem';
99
>
1010
> See **https://typescript-eslint.io/rules/use-unknown-in-catch-callback-variable** for documentation.
1111
12-
This rule enforces that you always use the `unknown` type for the parameter of a `Promise.prototype.catch()` callback.
12+
This rule enforces that you always use the `unknown` type for the parameter of a Promise rejection callback.
1313

1414
<Tabs>
1515
<TabItem value="❌ Incorrect">
@@ -26,6 +26,15 @@ Promise.reject(new Error('I will reject!')).catch((err: any) => {
2626
Promise.reject(new Error('I will reject!')).catch((err: Error) => {
2727
console.log(err);
2828
});
29+
30+
Promise.reject(new Error('I will reject!')).then(
31+
result => {
32+
console.log(result);
33+
},
34+
err => {
35+
console.log(err);
36+
},
37+
);
2938
```
3039

3140
</TabItem>
@@ -70,7 +79,7 @@ Promise.reject(x).catch((err: unknown) => {
7079
```
7180

7281
:::info
73-
There is actually a way to have the `catch()` callback variable use the `unknown` type _without_ an explicit type annotation at the call sites, but it has the drawback that it involves overriding global type declarations.
82+
There is actually a way to have the `catch()` and `then()` callback variables use the `unknown` type _without_ an explicit type annotation at the call sites, but it has the drawback that it involves overriding global type declarations.
7483
For example, the library [better-TypeScript-lib](https://github.com/uhyo/better-typescript-lib) sets this up globally for your project (see [the relevant lines in the better-TypeScript-lib source code](https://github.com/uhyo/better-typescript-lib/blob/c294e177d1cc2b1d1803febf8192a4c83a1fe028/lib/lib.es5.d.ts#L635) for details on how).
7584

7685
For further reading on this, you may also want to look into
@@ -81,4 +90,4 @@ For further reading on this, you may also want to look into
8190

8291
If your codebase is not yet able to enable `useUnknownInCatchVariables`, it likely would be similarly difficult to enable this rule.
8392

84-
If you have modified the global type declarations in order to make `catch()` callbacks use the `unknown` type without an explicit type annotation, you do not need this rule.
93+
If you have modified the global type declarations in order to make `then()` and `catch()` callbacks use the `unknown` type without an explicit type annotation, you do not need this rule.

‎packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts

Lines changed: 77 additions & 121 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
1+
import type { Scope } from '@typescript-eslint/scope-manager';
12
import type { TSESLint, TSESTree } from '@typescript-eslint/utils';
23
import { AST_NODE_TYPES } from '@typescript-eslint/utils';
3-
import type {
4-
ReportDescriptor,
5-
Scope,
6-
} from '@typescript-eslint/utils/ts-eslint';
4+
import type { ReportDescriptor } from '@typescript-eslint/utils/ts-eslint';
75
import * as tsutils from 'ts-api-utils';
86
import type * as ts from 'typescript';
97

@@ -18,7 +16,6 @@ import {
1816

1917
type MessageIds =
2018
| 'useUnknown'
21-
| 'useUnknownSpreadArgs'
2219
| 'useUnknownArrayDestructuringPattern'
2320
| 'useUnknownObjectDestructuringPattern'
2421
| 'addUnknownTypeAnnotationSuggestion'
@@ -27,14 +24,27 @@ type MessageIds =
2724
| 'wrongRestTypeAnnotationSuggestion';
2825

2926
const useUnknownMessageBase =
30-
'Prefer the safe `: unknown` for a catch callback variable.';
27+
'Prefer the safe `: unknown` for a `{{method}}`{{append}} callback variable.';
28+
29+
/**
30+
* `x.memberName` => 'memberKey'
31+
*
32+
* `const mk = 'memberKey'; x[mk]` => 'memberKey'
33+
*
34+
* `const mk = 1234; x[mk]` => 1234
35+
*/
36+
const getStaticMemberAccessKey = (
37+
{ computed, property }: TSESTree.MemberExpression,
38+
scope: Scope,
39+
): { value: unknown } | null =>
40+
computed ? getStaticValue(property, scope) : { value: property.name };
3141

3242
export default createRule<[], MessageIds>({
3343
name: 'use-unknown-in-catch-callback-variable',
3444
meta: {
3545
docs: {
3646
description:
37-
'Enforce typing arguments in `.catch()` callbacks as `unknown`',
47+
'Enforce typing arguments in Promise rejection callbacks as `unknown`',
3848
requiresTypeChecking: true,
3949
recommended: 'strict',
4050
},
@@ -45,13 +55,10 @@ export default createRule<[], MessageIds>({
4555
useUnknownObjectDestructuringPattern: `${
4656
useUnknownMessageBase
4757
} The thrown error may be nullable, or may not have the expected shape.`,
48-
useUnknownSpreadArgs: `${
49-
useUnknownMessageBase
50-
} The argument list may contain a handler that does not use \`unknown\` for the catch callback variable.`,
5158
addUnknownTypeAnnotationSuggestion:
52-
'Add an explicit `: unknown` type annotation to the catch variable.',
59+
'Add an explicit `: unknown` type annotation to the rejection callback variable.',
5360
addUnknownRestTypeAnnotationSuggestion:
54-
'Add an explicit `: [unknown]` type annotation to the catch rest variable.',
61+
'Add an explicit `: [unknown]` type annotation to the rejection callback rest variable.',
5562
wrongTypeAnnotationSuggestion:
5663
'Change existing type annotation to `: unknown`.',
5764
wrongRestTypeAnnotationSuggestion:
@@ -65,27 +72,8 @@ export default createRule<[], MessageIds>({
6572
defaultOptions: [],
6673

6774
create(context) {
68-
const services = getParserServices(context);
69-
const checker = services.program.getTypeChecker();
70-
71-
function isPromiseCatchAccess(node: TSESTree.Expression): boolean {
72-
if (
73-
!(
74-
node.type === AST_NODE_TYPES.MemberExpression &&
75-
isStaticMemberAccessOfValue(node, 'catch')
76-
)
77-
) {
78-
return false;
79-
}
80-
81-
const objectTsNode = services.esTreeNodeToTSNodeMap.get(node.object);
82-
const tsNode = services.esTreeNodeToTSNodeMap.get(node);
83-
return tsutils.isThenableType(
84-
checker,
85-
tsNode,
86-
checker.getTypeAtLocation(objectTsNode),
87-
);
88-
}
75+
const { program, esTreeNodeToTSNodeMap } = getParserServices(context);
76+
const checker = program.getTypeChecker();
8977

9078
function isFlaggableHandlerType(type: ts.Type): boolean {
9179
for (const unionPart of tsutils.unionTypeParts(type)) {
@@ -125,59 +113,20 @@ export default createRule<[], MessageIds>({
125113
return false;
126114
}
127115

128-
/**
129-
* If passed an ordinary expression, this will check it as expected.
130-
*
131-
* If passed a spread element, it treats it as the union of unwrapped array/tuple type.
132-
*/
133-
function shouldFlagArgument(
134-
node: TSESTree.Expression | TSESTree.SpreadElement,
135-
): boolean {
136-
const argument = services.esTreeNodeToTSNodeMap.get(node);
116+
function shouldFlagArgument(node: TSESTree.Expression): boolean {
117+
const argument = esTreeNodeToTSNodeMap.get(node);
137118
const typeOfArgument = checker.getTypeAtLocation(argument);
138119
return isFlaggableHandlerType(typeOfArgument);
139120
}
140121

141-
function shouldFlagMultipleSpreadArgs(
142-
argumentsList: TSE 1C6A STree.CallExpressionArgument[],
143-
): boolean {
144-
// One could try to be clever about unpacking fixed length tuples and stuff
145-
// like that, but there's no need, since this is all invalid use of `.catch`
146-
// anyway at the end of the day. Instead, we'll just check whether any of the
147-
// possible args types would violate the rule on its own.
148-
return argumentsList.some(argument => shouldFlagArgument(argument));
149-
}
150-
151-
function shouldFlagSingleSpreadArg(node: TSESTree.SpreadElement): boolean {
152-
const spreadArgs = services.esTreeNodeToTSNodeMap.get(node.argument);
153-
154-
const spreadArgsType = checker.getTypeAtLocation(spreadArgs);
155-
156-
if (checker.isArrayType(spreadArgsType)) {
157-
const arrayType = checker.getTypeArguments(spreadArgsType)[0];
158-
return isFlaggableHandlerType(arrayType);
159-
}
160-
161-
if (checker.isTupleType(spreadArgsType)) {
162-
const firstType = checker.getTypeArguments(spreadArgsType).at(0);
163-
if (!firstType) {
164-
// empty spread args. Suspect code, but not a problem for this rule.
165-
return false;
166-
}
167-
return isFlaggableHandlerType(firstType);
168-
}
169-
170-
return true;
171-
}
172-
173122
/**
174123
* Analyzes the syntax of the catch argument and makes a best effort to pinpoint
175124
* why it's reporting, and to come up with a suggested fix if possible.
176125
*
177126
* This function is explicitly operating under the assumption that the
178127
* rule _is reporting_, so it is not guaranteed to be sound to call otherwise.
179128
*/
180-
function refineReportForNormalArgumentIfPossible(
129+
function refineReportIfPossible(
181130
argument: TSESTree.Expression,
182131
): undefined | Partial<ReportDescriptor<MessageIds>> {
183132
// Only know how to be helpful if a function literal has been provided.
@@ -288,68 +237,75 @@ export default createRule<[], MessageIds>({
288237
}
289238

290239
return {
291-
CallExpression(node): void {
292-
if (node.arguments.length === 0 || !isPromiseCatchAccess(node.callee)) {
240+
CallExpression({ arguments: args, callee }): void {
241+
if (callee.type !== AST_NODE_TYPES.MemberExpression) {
293242
return;
294243
}
295244

296-
const firstArgument = node.arguments[0];
245+
const staticMemberAccessKey = getStaticMemberAccessKey(
246+
callee,
247+
context.sourceCode.getScope(callee),
248+
);
249+
if (!staticMemberAccessKey) {
250+
return;
251+
}
297252

298-
// Deal with some special cases around spread element args.
299-
// promise.catch(...handlers), promise.catch(...handlers, ...moreHandlers).
300-
if (firstArgument.type === AST_NODE_TYPES.SpreadElement) {
301-
if (node.arguments.length === 1) {
302-
if (shouldFlagSingleSpreadArg(firstArgument)) {
303-
context.report({
304-
node: firstArgument,
305-
messageId: 'useUnknown',
306-
});
307-
}
308-
} else if (shouldFlagMultipleSpreadArgs(node.arguments)) {
309-
context.report({
310-
node,
311-
messageId: 'useUnknownSpreadArgs',
312-
});
313-
}
253+
const promiseMethodInfo = (
254+
[
255+
{ method: 'catch', append: '', argIndexToCheck: 0 },
256+
{ method: 'then', append: ' rejection', argIndexToCheck: 1 },
257+
] satisfies {
258+
method: string;
259+
append: string;
260+
argIndexToCheck: number;
261+
}[]
262+
).find(({ method }) => staticMemberAccessKey.value === method);
263+
if (!promiseMethodInfo) {
264+
return;
265+
}
266+
267+
// Need to be enough args to check
268+
const { argIndexToCheck, ...data } = promiseMethodInfo;
269+
if (args.length < argIndexToCheck + 1) {
314270
return;
315271
}
316272

317-
// First argument is an "ordinary" argument (i.e. not a spread argument)
273+
// Argument to check, and all arguments before it, must be "ordinary" arguments (i.e. no spread arguments)
318274
// promise.catch(f), promise.catch(() => {}), promise.catch(<expression>, <<other-args>>)
319-
if (shouldFlagArgument(firstArgument)) {
275+
const argsToCheck = args.slice(0, argIndexToCheck + 1);
276+
if (
277+
argsToCheck.some(({ type }) => type === AST_NODE_TYPES.SpreadElement)
278+
) {
279+
return;
280+
}
281+
282+
if (
283+
!tsutils.isThenableType(
284+
checker,
285+
esTreeNodeToTSNodeMap.get(callee),
286+
checker.getTypeAtLocation(esTreeNodeToTSNodeMap.get(callee.object)),
287+
)
288+
) {
289+
return;
290+
}
291+
292+
// the `some` check above has already excluded `SpreadElement`, so we are safe to assert the same
293+
const node = argsToCheck[argIndexToCheck] as Exclude<
294+
(typeof argsToCheck)[number],
295+
TSESTree.SpreadElement
296+
>;
297+
if (shouldFlagArgument(node)) {
320298
// We are now guaranteed to report, but we have a bit of work to do
321299
// to determine exactly where, and whether we can fix it.
322-
const overrides =
323-
refineReportForNormalArgumentIfPossible(firstArgument);
300+
const overrides = refineReportIfPossible(node);
324301
context.report({
325-
node: firstArgument,
302+
node,
326303
messageId: 'useUnknown',
304+
data,
327305
...overrides,
328306
});
329307
}
330308
},
331309
};
332310
},
333311
});
334-
335-
/**
336-
* Answers whether the member expression looks like
337-
* `x.memberName`, `x['memberName']`,
338-
* or even `const mn = 'memberName'; x[mn]` (or optional variants thereof).
339-
*/
340-
function isStaticMemberAccessOfValue(
341-
memberExpression:
342-
| TSESTree.MemberExpressionComputedName
343-
| TSESTree.MemberExpressionNonComputedName,
344-
value: string,
345-
scope?: Scope.Scope | undefined,
346-
): boolean {
347-
if (!memberExpression.computed) {
348-
// x.memberName case.
349-
return memberExpression.property.name === value;
350-
}
351-
352-
// x['memberName'] cases.
353-
const staticValueResult = getStaticValue(memberExpression.property, scope);
354-
return staticValueResult != null && value === staticValueResult.value;
355-
}

‎packages/eslint-plugin/tests/docs-eslint-output-snapshots/use-unknown-in-catch-callback-variable.shot

Lines changed: 13 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)
0