From 88d65afecabcc638c7c5c060b93c252f47d4f8e6 Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Mon, 9 Sep 2024 18:30:02 +0900 Subject: [PATCH 1/3] feat(eslint-plugin): [no-misused-promises] check array predicate return --- .../src/rules/no-misused-promises.ts | 18 +++++++++ .../src/rules/no-unnecessary-condition.ts | 23 +++-------- packages/eslint-plugin/src/util/index.ts | 1 + .../util/isArrayMethodCallWithPredicate.ts | 35 ++++++++++++++++ .../tests/rules/no-misused-promises.test.ts | 40 +++++++++++++++++++ .../rules/no-unnecessary-condition.test.ts | 2 + 6 files changed, 101 insertions(+), 18 deletions(-) create mode 100644 packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts diff --git a/packages/eslint-plugin/src/rules/no-misused-promises.ts b/packages/eslint-plugin/src/rules/no-misused-promises.ts index c6e0978f1cb0..a9fa38f5f0ab 100644 --- a/packages/eslint-plugin/src/rules/no-misused-promises.ts +++ b/packages/eslint-plugin/src/rules/no-misused-promises.ts @@ -6,6 +6,7 @@ import * as ts from 'typescript'; import { createRule, getParserServices, + isArrayMethodCallWithPredicate, isFunction, isRestParameterDeclaration, nullThrows, @@ -175,6 +176,7 @@ export default createRule({ checkConditional(node.argument, true); }, WhileStatement: checkTestConditional, + 'CallExpression > MemberExpression': checkArrayPredicates, }; checksVoidReturn = parseChecksVoidReturn(checksVoidReturn); @@ -322,6 +324,22 @@ export default createRule({ } } + function checkArrayPredicates(node: TSESTree.MemberExpression): void { + const parent = node.parent; + if (parent.type === AST_NODE_TYPES.CallExpression) { + const callback = parent.arguments.at(0); + if (callback && isArrayMethodCallWithPredicate(services, parent)) { + const type = services.esTreeNodeToTSNodeMap.get(callback); + if (returnsThenable(checker, type)) { + context.report({ + messageId: 'conditional', + node, + }); + } + } + } + } + function checkArguments( node: TSESTree.CallExpression | TSESTree.NewExpression, ): void { diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 8f193dde47fa..4152353d3f4e 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -9,6 +9,7 @@ import { getParserServices, getTypeName, getTypeOfPropertyOfName, + isArrayMethodCallWithPredicate, isIdentifier, isNullableType, isTypeAnyType, @@ -457,26 +458,12 @@ export default createRule({ checkNode(node.test); } - const ARRAY_PREDICATE_FUNCTIONS = new Set([ - 'filter', - 'find', - 'some', - 'every', - ]); - function isArrayPredicateFunction(node: TSESTree.CallExpression): boolean { - const { callee } = node; - return ( - // looks like `something.filter` or `something.find` - callee.type === AST_NODE_TYPES.MemberExpression && - callee.property.type === AST_NODE_TYPES.Identifier && - ARRAY_PREDICATE_FUNCTIONS.has(callee.property.name) && - // and the left-hand side is an array, according to the types - (nodeIsArrayType(callee.object) || nodeIsTupleType(callee.object)) - ); - } function checkCallExpression(node: TSESTree.CallExpression): void { // If this is something like arr.filter(x => /*condition*/), check `condition` - if (isArrayPredicateFunction(node) && node.arguments.length) { + if ( + isArrayMethodCallWithPredicate(services, node) && + node.arguments.length + ) { const callback = node.arguments[0]; // Inline defined functions if ( diff --git a/packages/eslint-plugin/src/util/index.ts b/packages/eslint-plugin/src/util/index.ts index 58f34597653b..b13b5855231d 100644 --- a/packages/eslint-plugin/src/util/index.ts +++ b/packages/eslint-plugin/src/util/index.ts @@ -21,6 +21,7 @@ export * from './scopeUtils'; export * from './types'; export * from './isAssignee'; export * from './getFixOrSuggest'; +export * from './isArrayMethodCallWithPredicate'; // this is done for convenience - saves migrating all of the old rules export * from '@typescript-eslint/type-utils'; diff --git a/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts b/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts new file mode 100644 index 000000000000..eb27158618fd --- /dev/null +++ b/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts @@ -0,0 +1,35 @@ +import { getConstrainedTypeAtLocation } from '@typescript-eslint/type-utils'; +import type { + ParserServicesWithTypeInformation, + TSESTree, +} from '@typescript-eslint/utils'; +import { AST_NODE_TYPES } from '@typescript-eslint/utils'; +import * as tsutils from 'ts-api-utils'; + +const ARRAY_PREDICATE_FUNCTIONS = new Set([ + 'filter', + 'find', + 'findIndex', + 'findLast', + 'findLastIndex', + 'some', + 'every', +]); + +export function isArrayMethodCallWithPredicate( + services: ParserServicesWithTypeInformation, + node: TSESTree.CallExpression, +): boolean { + if ( + node.callee.type === AST_NODE_TYPES.MemberExpression && + node.callee.property.type === AST_NODE_TYPES.Identifier && + ARRAY_PREDICATE_FUNCTIONS.has(node.callee.property.name) + ) { + const checker = services.program.getTypeChecker(); + const type = getConstrainedTypeAtLocation(services, node.callee.object); + return tsutils + .typeParts(type) + .some(t => checker.isArrayType(t) || checker.isTupleType(t)); + } + return false; +} diff --git a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts index 072e1444fea0..dd6a08c1c46f 100644 --- a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts @@ -1047,6 +1047,10 @@ interface MyInterface extends MyCall, MyIndex, MyConstruct, MyMethods { 'const notAFn3: boolean = true;', 'const notAFn4: { prop: 1 } = { prop: 1 };', 'const notAFn5: {} = {};', + ` +const array: number[] = [1, 2, 3]; +array.filter(a => a > 1); + `, ], invalid: [ @@ -2269,5 +2273,41 @@ interface MyInterface extends MyCall, MyIndex, MyConstruct, MyMethods { }, ], }, + { + code: ` +declare function isTruthy(value: unknown): Promise; +[0, 1, 2].filter(isTruthy); + `, + errors: [ + { + line: 3, + messageId: 'conditional', + }, + ], + }, + { + code: ` +const array: number[] = []; +array.every(() => Promise.resolve(true)); + `, + errors: [ + { + line: 3, + messageId: 'conditional', + }, + ], + }, + { + code: ` +const tuple: [number, number, number] = [1, 2, 3]; +tuple.find(() => Promise.resolve(false)); + `, + errors: [ + { + line: 3, + messageId: 'conditional', + }, + ], + }, ], }); diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts index b8a9ab9981d9..fd80c94b28ed 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -1282,11 +1282,13 @@ function truthy() { function falsy() {} [1, 3, 5].filter(truthy); [1, 2, 3].find(falsy); +[1, 2, 3].findLastIndex(falsy); `, output: null, errors: [ ruleError(6, 18, 'alwaysTruthyFunc'), ruleError(7, 16, 'alwaysFalsyFunc'), + ruleError(8, 25, 'alwaysFalsyFunc'), ], }, // Supports generics From bf86e6a78eed27baeab51f08467b72e8fb012bcd Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Mon, 9 Sep 2024 19:15:11 +0900 Subject: [PATCH 2/3] refactor --- packages/eslint-plugin/docs/rules/no-misused-promises.mdx | 5 +++++ packages/eslint-plugin/src/rules/no-misused-promises.ts | 6 ++++-- .../docs-eslint-output-snapshots/no-misused-promises.shot | 6 ++++++ .../eslint-plugin/tests/rules/no-misused-promises.test.ts | 7 ++++--- 4 files changed, 19 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-misused-promises.mdx b/packages/eslint-plugin/docs/rules/no-misused-promises.mdx index bc8ae0f33b6e..f4fd23fec126 100644 --- a/packages/eslint-plugin/docs/rules/no-misused-promises.mdx +++ b/packages/eslint-plugin/docs/rules/no-misused-promises.mdx @@ -134,6 +134,8 @@ if (promise) { const val = promise ? 123 : 456; +[1, 2, 3].filter(() => promise); + while (promise) { // Do something } @@ -152,6 +154,9 @@ if (await promise) { const val = (await promise) ? 123 : 456; +const returnVal = await promise; +[1, 2, 3].filter(() => returnVal); + while (await promise) { // Do something } diff --git a/packages/eslint-plugin/src/rules/no-misused-promises.ts b/packages/eslint-plugin/src/rules/no-misused-promises.ts index a9fa38f5f0ab..fe0e826ce44d 100644 --- a/packages/eslint-plugin/src/rules/no-misused-promises.ts +++ b/packages/eslint-plugin/src/rules/no-misused-promises.ts @@ -32,6 +32,7 @@ interface ChecksVoidReturnOptions { type MessageId = | 'conditional' + | 'predicate' | 'spread' | 'voidReturnArgument' | 'voidReturnAttribute' @@ -92,6 +93,7 @@ export default createRule({ voidReturnVariable: 'Promise-returning function provided to variable where a void return was expected.', conditional: 'Expected non-Promise value in a boolean conditional.', + predicate: 'Expected a non-Promise value to be returned.', spread: 'Expected a non-Promise value to be spreaded in an object.', }, schema: [ @@ -332,8 +334,8 @@ export default createRule({ const type = services.esTreeNodeToTSNodeMap.get(callback); if (returnsThenable(checker, type)) { context.report({ - messageId: 'conditional', - node, + messageId: 'predicate', + node: callback, }); } } diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-misused-promises.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-misused-promises.shot index 17d39d27c875..8c9f4989d46b 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-misused-promises.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-misused-promises.shot @@ -14,6 +14,9 @@ if (promise) { const val = promise ? 123 : 456; ~~~~~~~ Expected non-Promise value in a boolean conditional. +[1, 2, 3].filter(() => promise); + ~~~~~~~~~~~~~ Expected a non-Promise value to be returned. + while (promise) { ~~~~~~~ Expected non-Promise value in a boolean conditional. // Do something @@ -34,6 +37,9 @@ if (await promise) { const val = (await promise) ? 123 : 456; +const returnVal = await promise; +[1, 2, 3].filter(() => returnVal); + while (await promise) { // Do something } diff --git a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts index dd6a08c1c46f..653236007eb5 100644 --- a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts @@ -2281,7 +2281,7 @@ declare function isTruthy(value: unknown): Promise; errors: [ { line: 3, - messageId: 'conditional', + messageId: 'predicate', }, ], }, @@ -2293,7 +2293,7 @@ array.every(() => Promise.resolve(true)); errors: [ { line: 3, - messageId: 'conditional', + messageId: 'predicate', }, ], }, @@ -2302,10 +2302,11 @@ array.every(() => Promise.resolve(true)); const tuple: [number, number, number] = [1, 2, 3]; tuple.find(() => Promise.resolve(false)); `, + options: [{ checksConditionals: true }], errors: [ { line: 3, - messageId: 'conditional', + messageId: 'predicate', }, ], }, From 9f161d4882725d1122661dd27713101d1839b869 Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Mon, 16 Sep 2024 04:37:26 +0900 Subject: [PATCH 3/3] fix review --- .../src/rules/no-misused-promises.ts | 5 +++- .../src/rules/no-unnecessary-condition.ts | 2 +- .../util/isArrayMethodCallWithPredicate.ts | 30 ++++++++++++------- .../tests/rules/no-misused-promises.test.ts | 12 ++++++++ 4 files changed, 36 insertions(+), 13 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-misused-promises.ts b/packages/eslint-plugin/src/rules/no-misused-promises.ts index fe0e826ce44d..9edd31141e0a 100644 --- a/packages/eslint-plugin/src/rules/no-misused-promises.ts +++ b/packages/eslint-plugin/src/rules/no-misused-promises.ts @@ -330,7 +330,10 @@ export default createRule({ const parent = node.parent; if (parent.type === AST_NODE_TYPES.CallExpression) { const callback = parent.arguments.at(0); - if (callback && isArrayMethodCallWithPredicate(services, parent)) { + if ( + callback && + isArrayMethodCallWithPredicate(context, services, parent) + ) { const type = services.esTreeNodeToTSNodeMap.get(callback); if (returnsThenable(checker, type)) { context.report({ diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index bf231aac8425..e3e3a983d386 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -462,7 +462,7 @@ export default createRule({ function checkCallExpression(node: TSESTree.CallExpression): void { // If this is something like arr.filter(x => /*condition*/), check `condition` if ( - isArrayMethodCallWithPredicate(services, node) && + isArrayMethodCallWithPredicate(context, services, node) && node.arguments.length ) { const callback = node.arguments[0]; diff --git a/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts b/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts index eb27158618fd..746e9003722c 100644 --- a/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts +++ b/packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts @@ -4,8 +4,11 @@ import type { TSESTree, } from '@typescript-eslint/utils'; import { AST_NODE_TYPES } from '@typescript-eslint/utils'; +import type { RuleContext } from '@typescript-eslint/utils/ts-eslint'; import * as tsutils from 'ts-api-utils'; +import { getStaticMemberAccessValue } from './misc'; + const ARRAY_PREDICATE_FUNCTIONS = new Set([ 'filter', 'find', @@ -17,19 +20,24 @@ const ARRAY_PREDICATE_FUNCTIONS = new Set([ ]); export function isArrayMethodCallWithPredicate( + context: RuleContext, services: ParserServicesWithTypeInformation, node: TSESTree.CallExpression, ): boolean { - if ( - node.callee.type === AST_NODE_TYPES.MemberExpression && - node.callee.property.type === AST_NODE_TYPES.Identifier && - ARRAY_PREDICATE_FUNCTIONS.has(node.callee.property.name) - ) { - const checker = services.program.getTypeChecker(); - const type = getConstrainedTypeAtLocation(services, node.callee.object); - return tsutils - .typeParts(type) - .some(t => checker.isArrayType(t) || checker.isTupleType(t)); + if (node.callee.type !== AST_NODE_TYPES.MemberExpression) { + return false; } - return false; + + const staticAccessValue = getStaticMemberAccessValue(node.callee, context); + + if (!staticAccessValue || !ARRAY_PREDICATE_FUNCTIONS.has(staticAccessValue)) { + return false; + } + + const checker = services.program.getTypeChecker(); + const type = getConstrainedTypeAtLocation(services, node.callee.object); + return tsutils + .unionTypeParts(type) + .flatMap(part => tsutils.intersectionTypeParts(part)) + .some(t => checker.isArrayType(t) || checker.isTupleType(t)); } diff --git a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts index 653236007eb5..0c6b33582825 100644 --- a/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts +++ b/packages/eslint-plugin/tests/rules/no-misused-promises.test.ts @@ -2288,6 +2288,18 @@ declare function isTruthy(value: unknown): Promise; { code: ` const array: number[] = []; +array.every(() => Promise.resolve(true)); + `, + errors: [ + { + line: 3, + messageId: 'predicate', + }, + ], + }, + { + code: ` +const array: (string[] & { foo: 'bar' }) | (number[] & { bar: 'foo' }) = []; array.every(() => Promise.resolve(true)); `, errors: [