From 57befc5d5075b90b1267efc0459e8541aa0d9e34 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:38:59 -0700 Subject: [PATCH 01/15] Add rule prefer-find --- .../eslint-plugin/docs/rules/prefer-find.md | 29 +++ packages/eslint-plugin/src/configs/all.ts | 1 + .../src/configs/disable-type-checked.ts | 1 + .../src/configs/stylistic-type-checked.ts | 1 + packages/eslint-plugin/src/rules/index.ts | 2 + .../eslint-plugin/src/rules/prefer-find.ts | 110 +++++++++++ .../tests/rules/prefer-find.test.ts | 176 ++++++++++++++++++ .../tests/schema-snapshots/prefer-find.shot | 14 ++ 8 files changed, 334 insertions(+) create mode 100644 packages/eslint-plugin/docs/rules/prefer-find.md create mode 100644 packages/eslint-plugin/src/rules/prefer-find.ts create mode 100644 packages/eslint-plugin/tests/rules/prefer-find.test.ts create mode 100644 packages/eslint-plugin/tests/schema-snapshots/prefer-find.shot diff --git a/packages/eslint-plugin/docs/rules/prefer-find.md b/packages/eslint-plugin/docs/rules/prefer-find.md new file mode 100644 index 000000000000..1d0b931b965b --- /dev/null +++ b/packages/eslint-plugin/docs/rules/prefer-find.md @@ -0,0 +1,29 @@ +--- +description: 'Enforce the use of Array.prototype.find() over Array.prototype.filter() followed by [0] when looking for a single result.' +--- + +> 🛑 This file is source code, not the primary documentation location! 🛑 +> +> See **https://typescript-eslint.io/rules/prefer-find** for documentation. + +When searching for the first item in an array matching a condition, it may be tempting to use code like `arr.filter(x => x > 0)[0]`. However, it is simpler to use [Array.prototype.find()](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find) instead, `arr.find(x => x > 0)`, which also returns the first entry matching a condition. + +Beware that the two approaches are not quite identical, however! `.find()` will only execute the callback on array elements until it finds a match, whereas `.filter()` executes the callback for all array elements. Therefore, when fixing errors from this rule, be sure that your `.filter()` callbacks do not have side effects. + + + +### ❌ Incorrect + +```ts +[1, 2, 3].filter(x => x > 1)[0]; +``` + +### ✅ Correct + +```ts +[1, 2, 3].find(x => x > 1); +``` + +## When Not To Use It + +If you don't mind `.filter(...)[0]`, you can avoid this rule. diff --git a/packages/eslint-plugin/src/configs/all.ts b/packages/eslint-plugin/src/configs/all.ts index 9881e3397de2..99770f7c03b2 100644 --- a/packages/eslint-plugin/src/configs/all.ts +++ b/packages/eslint-plugin/src/configs/all.ts @@ -118,6 +118,7 @@ export = { 'prefer-destructuring': 'off', '@typescript-eslint/prefer-destructuring': 'error', '@typescript-eslint/prefer-enum-initializers': 'error', + '@typescript-eslint/prefer-find': 'error', '@typescript-eslint/prefer-for-of': 'error', '@typescript-eslint/prefer-function-type': 'error', '@typescript-eslint/prefer-includes': 'error', diff --git a/packages/eslint-plugin/src/configs/disable-type-checked.ts b/packages/eslint-plugin/src/configs/disable-type-checked.ts index 2fe413146c7b..20a9481ea733 100644 --- a/packages/eslint-plugin/src/configs/disable-type-checked.ts +++ b/packages/eslint-plugin/src/configs/disable-type-checked.ts @@ -38,6 +38,7 @@ export = { '@typescript-eslint/no-useless-template-literals': 'off', '@typescript-eslint/non-nullable-type-assertion-style': 'off', '@typescript-eslint/prefer-destructuring': 'off', + '@typescript-eslint/prefer-find': 'off', '@typescript-eslint/prefer-includes': 'off', '@typescript-eslint/prefer-nullish-coalescing': 'off', '@typescript-eslint/prefer-optional-chain': 'off', diff --git a/packages/eslint-plugin/src/configs/stylistic-type-checked.ts b/packages/eslint-plugin/src/configs/stylistic-type-checked.ts index 5c73ae3845b6..f8059fa33311 100644 --- a/packages/eslint-plugin/src/configs/stylistic-type-checked.ts +++ b/packages/eslint-plugin/src/configs/stylistic-type-checked.ts @@ -24,6 +24,7 @@ export = { '@typescript-eslint/no-empty-interface': 'error', '@typescript-eslint/no-inferrable-types': 'error', '@typescript-eslint/non-nullable-type-assertion-style': 'error', + '@typescript-eslint/prefer-find': 'error', '@typescript-eslint/prefer-for-of': 'error', '@typescript-eslint/prefer-function-type': 'error', '@typescript-eslint/prefer-namespace-keyword': 'error', diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index 14c171af990e..ed426770097c 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -102,6 +102,7 @@ import parameterProperties from './parameter-properties'; import preferAsConst from './prefer-as-const'; import preferDestructuring from './prefer-destructuring'; import preferEnumInitializers from './prefer-enum-initializers'; +import preferFind from './prefer-find'; import preferForOf from './prefer-for-of'; import preferFunctionType from './prefer-function-type'; import preferIncludes from './prefer-includes'; @@ -241,6 +242,7 @@ export default { 'prefer-as-const': preferAsConst, 'prefer-destructuring': preferDestructuring, 'prefer-enum-initializers': preferEnumInitializers, + 'prefer-find': preferFind, 'prefer-for-of': preferForOf, 'prefer-function-type': preferFunctionType, 'prefer-includes': preferIncludes, diff --git a/packages/eslint-plugin/src/rules/prefer-find.ts b/packages/eslint-plugin/src/rules/prefer-find.ts new file mode 100644 index 000000000000..439d34ffdaff --- /dev/null +++ b/packages/eslint-plugin/src/rules/prefer-find.ts @@ -0,0 +1,110 @@ +import type { TSESLint } from '@typescript-eslint/utils'; +import { AST_NODE_TYPES } from '@typescript-eslint/utils'; +import * as tsutils from 'ts-api-utils'; + +import { createRule, getParserServices } from '../util'; + +export default createRule({ + name: 'prefer-find', + meta: { + docs: { + description: + 'Enforce the use of Array.prototype.find() over Array.prototype.filter() followed by [0] when looking for a single result', + requiresTypeChecking: true, + recommended: 'stylistic', + }, + fixable: 'code', + messages: { + preferFind: 'Use .filter(...)[0] instead of .find(...)', + preferFindFix: 'Use .filter(...)[0] instead of .find(...)', + }, + schema: [], + type: 'suggestion', + hasSuggestions: true, + }, + + defaultOptions: [], + + create(context) { + const services = getParserServices(context); + const checker = services.program.getTypeChecker(); + + return { + MemberExpression(node): void { + // Check if it looks like <>[0] or <>['0'], but not <>?.[0] + if ( + node.computed && + !node.optional && + node.property.type === AST_NODE_TYPES.Literal && + (node.property.value === 0 || + node.property.value === '0' || + node.property.value === 0n) + ) { + // Check if it looks like <>(...)[0], but not <>?.(...)[0] + if ( + node.object.type === AST_NODE_TYPES.CallExpression && + !node.object.optional + ) { + const objectCallee = node.object.callee; + // Check if it looks like <>.filter(...)[0] or <>['filter'](...)[0], + // or the optional chaining variants. + if (objectCallee.type === AST_NODE_TYPES.MemberExpression) { + const isBracketSyntaxForFilter = objectCallee.computed; + if ( + isBracketSyntaxForFilter + ? objectCallee.property.type === AST_NODE_TYPES.Literal && + objectCallee.property.value === 'filter' + : objectCallee.property.name === 'filter' + ) { + const isOptionalAccessOfFilter = objectCallee.optional; + + const filteredObjectType = checker.getTypeAtLocation( + services.esTreeNodeToTSNodeMap.get(objectCallee.object), + ); + + // We can now report if the object is an array + // or if it's an optional chain on a nullable array. + if ( + checker.isArrayType(filteredObjectType) || + (isOptionalAccessOfFilter && + tsutils + .unionTypeParts(filteredObjectType) + .every( + unionPart => + checker.isArrayType(unionPart) || + tsutils.isIntrinsicNullType(unionPart) || + tsutils.isIntrinsicUndefinedType(unionPart), + )) + ) { + context.report({ + node, + messageId: 'preferFind', + suggest: [ + { + messageId: 'preferFindFix', + fix(fixer): TSESLint.RuleFix[] { + return [ + // Replace .filter with .find + fixer.replaceText( + objectCallee.property, + isBracketSyntaxForFilter ? '"find"' : 'find', + ), + // Get rid of the [0] + fixer.removeRange([ + node.object.range[1], + node.range[1], + ]), + ]; + }, + }, + ], + }); + } + } + } + } + } + }, + }; + }, +}); diff --git a/packages/eslint-plugin/tests/rules/prefer-find.test.ts b/packages/eslint-plugin/tests/rules/prefer-find.test.ts new file mode 100644 index 000000000000..f2e44ae88125 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/prefer-find.test.ts @@ -0,0 +1,176 @@ +import { noFormat, RuleTester } from '@typescript-eslint/rule-tester'; + +import rule from '../../src/rules/prefer-find'; +import { getFixturesRootDir } from '../RuleTester'; + +const rootDir = getFixturesRootDir(); + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + tsconfigRootDir: rootDir, + project: './tsconfig.json', + }, +}); + +ruleTester.run('prefer-find', rule, { + valid: [ + ` + interface JerkCode { + filter(predicate: (item: T) => boolean): JerkCode; + } + + declare const jerkCode: JerkCode; + + jerkCode.filter(item => item === 'aha'); + `, + ` + declare const notNecessarilyAnArray: unknown[] | undefined | null | string; + notNecessarilyAnArray?.filter(item => true)[0]; + `, + // Be sure that we don't try to mess with this case, where the member access + // is not directly occurring on the result of the filter call due to optional + // chaining. + '([]?.filter(f))[0];', + // Be sure that we don't try to mess with this case, since the member access + // should not need to be optional for the cases the rule is concerned with. + '[].filter(() => true)?.[0];', + // Be sure that we don't try to mess with this case, since the function call + // should not need to be optional for the cases the rule is concerned with. + '[].filter?.(() => true)[0];', + ], + + invalid: [ + { + code: ` +declare const arr: string[]; +arr.filter(item => item === 'aha')[0]; + `, + errors: [ + { + line: 3, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindFix', + output: ` +declare const arr: string[]; +arr.find(item => item === 'aha'); + `, + }, + ], + }, + ], + }, + { + code: ` +declare const arr: string[]; +arr.filter(item => item === 'aha')['0']; + `, + errors: [ + { + line: 3, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindFix', + output: ` +declare const arr: string[]; +arr.find(item => item === 'aha'); + `, + }, + ], + }, + ], + }, + { + code: ` +declare const arr: string[]; +arr.filter(item => item === 'aha')[0n]; + `, + errors: [ + { + line: 3, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindFix', + output: ` +declare const arr: string[]; +arr.find(item => item === 'aha'); + `, + }, + ], + }, + ], + }, + { + code: 'const two = [1, 2, 3].filter(item => item === 2)[0];', + errors: [ + { + line: 1, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindFix', + output: `const two = [1, 2, 3].find(item => item === 2);`, + }, + ], + }, + ], + }, + { + code: noFormat`(([] as unknown[]))["filter"] ((item) => { return item === 2 } ) [ 0 ] ;`, + errors: [ + { + line: 1, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindFix', + output: + '(([] as unknown[]))["find"] ((item) => { return item === 2 } ) ;', + }, + ], + }, + ], + }, + { + code: noFormat`(([] as unknown[]))?.["filter"] ((item) => { return item === 2 } ) [ 0 ] ;`, + errors: [ + { + line: 1, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindFix', + output: + '(([] as unknown[]))?.["find"] ((item) => { return item === 2 } ) ;', + }, + ], + }, + ], + }, + { + code: ` +declare const nullableArray: unknown[] | undefined | null; +nullableArray?.filter(item => true)[0]; + `, + errors: [ + { + line: 3, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindFix', + output: ` +declare const nullableArray: unknown[] | undefined | null; +nullableArray?.find(item => true); + `, + }, + ], + }, + ], + }, + ], +}); diff --git a/packages/eslint-plugin/tests/schema-snapshots/prefer-find.shot b/packages/eslint-plugin/tests/schema-snapshots/prefer-find.shot new file mode 100644 index 000000000000..941a7a764176 --- /dev/null +++ b/packages/eslint-plugin/tests/schema-snapshots/prefer-find.shot @@ -0,0 +1,14 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`Rule schemas should be convertible to TS types for documentation purposes prefer-find 1`] = ` +" +# SCHEMA: + +[] + + +# TYPES: + +/** No options declared */ +type Options = [];" +`; From 23933e6957f4ee2f864b8d2bc5fb057591f9cb51 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:00 -0700 Subject: [PATCH 02/15] address lots of stuff --- .../eslint-plugin/docs/rules/prefer-find.md | 16 +- .../src/configs/stylistic-type-checked.ts | 1 - .../eslint-plugin/src/rules/prefer-find.ts | 296 ++++++++++++----- .../tests/rules/prefer-find.test.ts | 309 +++++++++++++++++- packages/utils/src/eslint-utils/nullThrows.ts | 3 +- 5 files changed, 528 insertions(+), 97 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/prefer-find.md b/packages/eslint-plugin/docs/rules/prefer-find.md index 1d0b931b965b..06f09f35aa25 100644 --- a/packages/eslint-plugin/docs/rules/prefer-find.md +++ b/packages/eslint-plugin/docs/rules/prefer-find.md @@ -6,9 +6,17 @@ description: 'Enforce the use of Array.prototype.find() over Array.prototype.fil > > See **https://typescript-eslint.io/rules/prefer-find** for documentation. -When searching for the first item in an array matching a condition, it may be tempting to use code like `arr.filter(x => x > 0)[0]`. However, it is simpler to use [Array.prototype.find()](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find) instead, `arr.find(x => x > 0)`, which also returns the first entry matching a condition. +When searching for the first item in an array matching a condition, it may be tempting to use code like `arr.filter(x => x > 0)[0]`. +However, it is simpler to use [Array.prototype.find()](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find) instead, `arr.find(x => x > 0)`, which also returns the first entry matching a condition. +Because the `.find()` only needs to execute the callback until it finds a match, it's also more efficient. -Beware that the two approaches are not quite identical, however! `.find()` will only execute the callback on array elements until it finds a match, whereas `.filter()` executes the callback for all array elements. Therefore, when fixing errors from this rule, be sure that your `.filter()` callbacks do not have side effects. +:::info + +Beware the the difference in short-circuiting behavior between the approaches. +`.find()` will only execute the callback on array elements until it finds a match, whereas `.filter()` executes the callback for all array elements. +Therefore, when fixing errors from this rule, be sure that your `.filter()` callbacks do not have side effects. + +::: @@ -16,6 +24,8 @@ Beware that the two approaches are not quite identical, however! `.find()` will ```ts [1, 2, 3].filter(x => x > 1)[0]; + +[1, 2, 3].filter(x => x > 1).at(0); ``` ### ✅ Correct @@ -26,4 +36,4 @@ Beware that the two approaches are not quite identical, however! `.find()` will ## When Not To Use It -If you don't mind `.filter(...)[0]`, you can avoid this rule. +If you intentionally use patterns like `.filter(callback)[0]` to execute side effects in `callback` on all array elements, you will want to avoid this rule. diff --git a/packages/eslint-plugin/src/configs/stylistic-type-checked.ts b/packages/eslint-plugin/src/configs/stylistic-type-checked.ts index f8059fa33311..5c73ae3845b6 100644 --- a/packages/eslint-plugin/src/configs/stylistic-type-checked.ts +++ b/packages/eslint-plugin/src/configs/stylistic-type-checked.ts @@ -24,7 +24,6 @@ export = { '@typescript-eslint/no-empty-interface': 'error', '@typescript-eslint/no-inferrable-types': 'error', '@typescript-eslint/non-nullable-type-assertion-style': 'error', - '@typescript-eslint/prefer-find': 'error', '@typescript-eslint/prefer-for-of': 'error', '@typescript-eslint/prefer-function-type': 'error', '@typescript-eslint/prefer-namespace-keyword': 'error', diff --git a/packages/eslint-plugin/src/rules/prefer-find.ts b/packages/eslint-plugin/src/rules/prefer-find.ts index 439d34ffdaff..28f931d8580c 100644 --- a/packages/eslint-plugin/src/rules/prefer-find.ts +++ b/packages/eslint-plugin/src/rules/prefer-find.ts @@ -1,8 +1,16 @@ -import type { TSESLint } from '@typescript-eslint/utils'; +import type { TSESLint, TSESTree } from '@typescript-eslint/utils'; import { AST_NODE_TYPES } from '@typescript-eslint/utils'; +import { getScope, getSourceCode } from '@typescript-eslint/utils/eslint-utils'; +import type { RuleFix, SourceCode } from '@typescript-eslint/utils/ts-eslint'; import * as tsutils from 'ts-api-utils'; -import { createRule, getParserServices } from '../util'; +import { + createRule, + getConstrainedTypeAtLocation, + getParserServices, + getStaticValue, + nullThrows, +} from '../util'; export default createRule({ name: 'prefer-find', @@ -11,12 +19,10 @@ export default createRule({ description: 'Enforce the use of Array.prototype.find() over Array.prototype.filter() followed by [0] when looking for a single result', requiresTypeChecking: true, - recommended: 'stylistic', }, - fixable: 'code', messages: { preferFind: 'Use .filter(...)[0] instead of .find(...)', - preferFindFix: 'Use .filter(...)[0] instead of .find(...)', + preferFindSuggestion: 'Use .filter(...)[0] instead of .find(...)', }, schema: [], type: 'suggestion', @@ -26,84 +32,224 @@ export default createRule({ defaultOptions: [], create(context) { + const globalScope = getScope(context); const services = getParserServices(context); const checker = services.program.getTypeChecker(); - return { - MemberExpression(node): void { - // Check if it looks like <>[0] or <>['0'], but not <>?.[0] - if ( - node.computed && - !node.optional && - node.property.type === AST_NODE_TYPES.Literal && - (node.property.value === 0 || - node.property.value === '0' || - node.property.value === 0n) - ) { - // Check if it looks like <>(...)[0], but not <>?.(...)[0] + function parseIfArrayFilterExpression( + expression: TSESTree.Expression, + ): + | { isBracketSyntaxForFilter: boolean; filterNode: TSESTree.Node } + | undefined { + if (expression.type === AST_NODE_TYPES.SequenceExpression) { + // Only the last expression in the (a, b, [1, 2, 3].filter(condition))[0] matters + const lastExpression = nullThrows( + expression.expressions.at(-1), + 'Expected to have more than zero expressions in a sequence expression', + ); + return parseIfArrayFilterExpression(lastExpression); + } + + if (expression.type === AST_NODE_TYPES.ChainExpression) { + return parseIfArrayFilterExpression(expression.expression); + } + + // Check if it looks like <>(...), but not <>?.(...) + if ( + expression.type === AST_NODE_TYPES.CallExpression && + !expression.optional + ) { + const callee = expression.callee; + // Check if it looks like <>.filter(...) or <>['filter'](...), + // or the optional chaining variants. + if (callee.type === AST_NODE_TYPES.MemberExpression) { + const isBracketSyntaxForFilter = callee.computed; if ( - node.object.type === AST_NODE_TYPES.CallExpression && - !node.object.optional + isBracketSyntaxForFilter + ? getStaticValue(callee.property, globalScope)?.value === 'filter' + : callee.property.name === 'filter' ) { - const objectCallee = node.object.callee; - // Check if it looks like <>.filter(...)[0] or <>['filter'](...)[0], - // or the optional chaining variants. - if (objectCallee.type === AST_NODE_TYPES.MemberExpression) { - const isBracketSyntaxForFilter = objectCallee.computed; - if ( - isBracketSyntaxForFilter - ? objectCallee.property.type === AST_NODE_TYPES.Literal && - objectCallee.property.value === 'filter' - : objectCallee.property.name === 'filter' - ) { - const isOptionalAccessOfFilter = objectCallee.optional; - - const filteredObjectType = checker.getTypeAtLocation( - services.esTreeNodeToTSNodeMap.get(objectCallee.object), - ); - - // We can now report if the object is an array - // or if it's an optional chain on a nullable array. - if ( - checker.isArrayType(filteredObjectType) || - (isOptionalAccessOfFilter && - tsutils - .unionTypeParts(filteredObjectType) - .every( - unionPart => - checker.isArrayType(unionPart) || - tsutils.isIntrinsicNullType(unionPart) || - tsutils.isIntrinsicUndefinedType(unionPart), - )) - ) { - context.report({ - node, - messageId: 'preferFind', - suggest: [ - { - messageId: 'preferFindFix', - fix(fixer): TSESLint.RuleFix[] { - return [ - // Replace .filter with .find - fixer.replaceText( - objectCallee.property, - isBracketSyntaxForFilter ? '"find"' : 'find', - ), - // Get rid of the [0] - fixer.removeRange([ - node.object.range[1], - node.range[1], - ]), - ]; - }, - }, - ], - }); - } - } + const filterNode = callee.property; + const isOptionalAccessOfFilter = callee.optional; + + const filteredObjectType = getConstrainedTypeAtLocation( + services, + callee.object, + ); + + // We can now report if the object is an array + // or if it's an optional chain on a nullable array. + if ( + checker.isArrayType(filteredObjectType) || + checker.isTupleType(filteredObjectType) || + (isOptionalAccessOfFilter && + tsutils + .unionTypeParts(filteredObjectType) + .every( + unionPart => + checker.isArrayType(unionPart) || + checker.isTupleType(unionPart) || + tsutils.isIntrinsicNullType(unionPart) || + tsutils.isIntrinsicUndefinedType(unionPart), + )) + ) { + return { + isBracketSyntaxForFilter, + filterNode, + }; } } } + } + + return undefined; + } + + function getObjectIfArrayAtExpression( + node: TSESTree.CallExpression, + ): TSESTree.Expression | undefined { + if (node.arguments.length === 1) { + const atArgument = getStaticValue(node.arguments[0], globalScope); + if (atArgument != null && isTreatedAsZeroByArrayAt(atArgument.value)) { + const callee = node.callee; + if ( + callee.type === AST_NODE_TYPES.MemberExpression && + !callee.optional && + (callee.computed + ? getStaticValue(callee.property, globalScope)?.value === 'at' + : callee.property.name === 'at') + ) { + return callee.object; + } + } + } + + return undefined; + } + + function isTreatedAsZeroByArrayAt(value: unknown): boolean { + const asNumber = Number(value); + + if (isNaN(asNumber)) { + return true; + } + + return Math.trunc(asNumber) === 0; + } + + function isMemberAccessOfZero( + node: TSESTree.MemberExpressionComputedName, + ): boolean { + const property = getStaticValue(node.property, globalScope); + // Check if it looks like <>[0] or <>['0'], but not <>?.[0] + return ( + !node.optional && + property != null && + isTreatedAsZeroByMemberAccess(property.value) + ); + } + + function isTreatedAsZeroByMemberAccess(value: unknown): boolean { + return String(value) === '0'; + } + + function generateFixToRemoveArrayAccess( + fixer: TSESLint.RuleFixer, + arrayNode: TSESTree.Node, + wholeExpressionBeingFlagged: TSESTree.Node, + sourceCode: SourceCode, + ): RuleFix { + const tokenToStartDeletingFrom = nullThrows( + sourceCode.getTokenAfter(arrayNode, { + // The next `.` or `[` is what we're looking for. + // think of (...).at(0) or (...)[0] or even (...)["at"](0). + filter: token => token.value === '.' || token.value === '[', + }), + 'Expected to find a member access token!', + ); + return fixer.removeRange([ + tokenToStartDeletingFrom.range[0], + wholeExpressionBeingFlagged.range[1], + ]); + } + + return { + CallExpression(node): void { + const object = getObjectIfArrayAtExpression(node); + if (object) { + const filterExpression = parseIfArrayFilterExpression(object); + if (filterExpression) { + context.report({ + node, + messageId: 'preferFind', + suggest: [ + { + messageId: 'preferFindSuggestion', + fix(fixer): TSESLint.RuleFix[] { + const sourceCode = getSourceCode(context); + return [ + fixer.replaceText( + filterExpression.filterNode, + filterExpression.isBracketSyntaxForFilter + ? '"find"' + : 'find', + ), + // get rid of the .at(0) or ['at'](0) + generateFixToRemoveArrayAccess( + fixer, + object, + node, + sourceCode, + ), + ]; + }, + }, + ], + }); + } + } + }, + + // we're always looking for array member access to be "computed", + // i.e. filteredResults[0], since filteredResults.0 isn't a thing. + ['MemberExpression[computed=true]']( + node: TSESTree.MemberExpressionComputedName, + ): void { + if (isMemberAccessOfZero(node)) { + const object = node.object; + const parsedFilterExpression = parseIfArrayFilterExpression(object); + if (parsedFilterExpression) { + context.report({ + node, + messageId: 'preferFind', + suggest: [ + { + messageId: 'preferFindSuggestion', + fix(fixer): TSESLint.RuleFix[] { + const sourceCode = context.sourceCode; + + return [ + // Replace .filter with .find + fixer.replaceText( + parsedFilterExpression.filterNode, + parsedFilterExpression.isBracketSyntaxForFilter + ? '"find"' + : 'find', + ), + // Get rid of the [0] + generateFixToRemoveArrayAccess( + fixer, + object, + node, + sourceCode, + ), + ]; + }, + }, + ], + }); + } + } }, }; }, diff --git a/packages/eslint-plugin/tests/rules/prefer-find.test.ts b/packages/eslint-plugin/tests/rules/prefer-find.test.ts index f2e44ae88125..55f4cd6c8fd6 100644 --- a/packages/eslint-plugin/tests/rules/prefer-find.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-find.test.ts @@ -24,20 +24,36 @@ ruleTester.run('prefer-find', rule, { jerkCode.filter(item => item === 'aha'); `, + ` + declare const arr: readonly string[]; + arr.filter(item => item === 'aha')[1]; + `, + ` + declare const arr: string[]; + arr.filter(item => item === 'aha').at(1); + `, ` declare const notNecessarilyAnArray: unknown[] | undefined | null | string; notNecessarilyAnArray?.filter(item => true)[0]; `, - // Be sure that we don't try to mess with this case, where the member access - // is not directly occurring on the result of the filter call due to optional - // chaining. - '([]?.filter(f))[0];', // Be sure that we don't try to mess with this case, since the member access // should not need to be optional for the cases the rule is concerned with. '[].filter(() => true)?.[0];', + // Be sure that we don't try to mess with this case, since the member access + // should not need to be optional for the cases the rule is concerned with. + '[].filter(() => true)?.at?.(0);', // Be sure that we don't try to mess with this case, since the function call // should not need to be optional for the cases the rule is concerned with. '[].filter?.(() => true)[0];', + '[1, 2, 3].filter(x => x > 0).at(-Infinity);', + ` + declare const arr: string[]; + declare const cond: Parameters['filter']>[0]; + const a = { arr }; + a?.arr.filter(cond).at(1); + `, + "['Just', 'a', 'filter'].filter(x => x.length > 4);", + "['Just', 'a', 'find'].find(x => x.length > 4);", ], invalid: [ @@ -52,7 +68,7 @@ arr.filter(item => item === 'aha')[0]; messageId: 'preferFind', suggestions: [ { - messageId: 'preferFindFix', + messageId: 'preferFindSuggestion', output: ` declare const arr: string[]; arr.find(item => item === 'aha'); @@ -64,8 +80,31 @@ arr.find(item => item === 'aha'); }, { code: ` -declare const arr: string[]; -arr.filter(item => item === 'aha')['0']; +declare const arr: Array; +const zero = 0; +arr.filter(item => item === 'aha')[zero]; + `, + errors: [ + { + line: 4, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +declare const arr: Array; +const zero = 0; +arr.find(item => item === 'aha'); + `, + }, + ], + }, + ], + }, + { + code: ` +declare const arr: readonly string[]; +arr.filter(item => item === 'aha').at(0); `, errors: [ { @@ -73,9 +112,53 @@ arr.filter(item => item === 'aha')['0']; messageId: 'preferFind', suggestions: [ { - messageId: 'preferFindFix', + messageId: 'preferFindSuggestion', output: ` +declare const arr: readonly string[]; +arr.find(item => item === 'aha'); + `, + }, + ], + }, + ], + }, + { + code: ` +declare const arr: ReadonlyArray; +(undefined, arr.filter(item => item === 'aha')).at(0); + `, + errors: [ + { + line: 3, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +declare const arr: ReadonlyArray; +(undefined, arr.find(item => item === 'aha')); + `, + }, + ], + }, + ], + }, + { + code: ` declare const arr: string[]; +const zero = 0; +arr.filter(item => item === 'aha').at(zero); + `, + errors: [ + { + line: 4, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +declare const arr: string[]; +const zero = 0; arr.find(item => item === 'aha'); `, }, @@ -86,7 +169,7 @@ arr.find(item => item === 'aha'); { code: ` declare const arr: string[]; -arr.filter(item => item === 'aha')[0n]; +arr.filter(item => item === 'aha')['0']; `, errors: [ { @@ -94,7 +177,7 @@ arr.filter(item => item === 'aha')[0n]; messageId: 'preferFind', suggestions: [ { - messageId: 'preferFindFix', + messageId: 'preferFindSuggestion', output: ` declare const arr: string[]; arr.find(item => item === 'aha'); @@ -112,7 +195,7 @@ arr.find(item => item === 'aha'); messageId: 'preferFind', suggestions: [ { - messageId: 'preferFindFix', + messageId: 'preferFindSuggestion', output: `const two = [1, 2, 3].find(item => item === 2);`, }, ], @@ -120,16 +203,16 @@ arr.find(item => item === 'aha'); ], }, { - code: noFormat`(([] as unknown[]))["filter"] ((item) => { return item === 2 } ) [ 0 ] ;`, + code: noFormat`const fltr = "filter"; (([] as unknown[]))[fltr] ((item) => { return item === 2 } ) [ 0 ] ;`, errors: [ { line: 1, messageId: 'preferFind', suggestions: [ { - messageId: 'preferFindFix', + messageId: 'preferFindSuggestion', output: - '(([] as unknown[]))["find"] ((item) => { return item === 2 } ) ;', + 'const fltr = "filter"; (([] as unknown[]))["find"] ((item) => { return item === 2 } ) ;', }, ], }, @@ -143,9 +226,9 @@ arr.find(item => item === 'aha'); messageId: 'preferFind', suggestions: [ { - messageId: 'preferFindFix', + messageId: 'preferFindSuggestion', output: - '(([] as unknown[]))?.["find"] ((item) => { return item === 2 } ) ;', + '(([] as unknown[]))?.["find"] ((item) => { return item === 2 } ) ;', }, ], }, @@ -162,7 +245,7 @@ nullableArray?.filter(item => true)[0]; messageId: 'preferFind', suggestions: [ { - messageId: 'preferFindFix', + messageId: 'preferFindSuggestion', output: ` declare const nullableArray: unknown[] | undefined | null; nullableArray?.find(item => true); @@ -172,5 +255,197 @@ nullableArray?.find(item => true); }, ], }, + { + code: '([]?.filter(f))[0];', + errors: [ + { + line: 1, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: '([]?.find(f));', + }, + ], + }, + ], + }, + { + code: ` +declare const objectWithArrayProperty: { arr: unknown[] }; +declare function cond(x: unknown): boolean; +console.log((1, 2, objectWithArrayProperty?.arr['filter'](cond)).at(0)); + `, + errors: [ + { + line: 4, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +declare const objectWithArrayProperty: { arr: unknown[] }; +declare function cond(x: unknown): boolean; +console.log((1, 2, objectWithArrayProperty?.arr["find"](cond))); + `, + }, + ], + }, + ], + }, + { + code: ` +[1, 2, 3].filter(x => x > 0).at(NaN); + `, + errors: [ + { + line: 2, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +[1, 2, 3].find(x => x > 0); + `, + }, + ], + }, + ], + }, + { + code: ` +const idxToLookUp = -0.12635678; +[1, 2, 3].filter(x => x > 0).at(idxToLookUp); + `, + errors: [ + { + line: 3, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +const idxToLookUp = -0.12635678; +[1, 2, 3].find(x => x > 0); + `, + }, + ], + }, + ], + }, + { + code: ` +[1, 2, 3].filter(x => x > 0)[\`at\`](0); + `, + errors: [ + { + line: 2, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +[1, 2, 3].find(x => x > 0); + `, + }, + ], + }, + ], + }, + { + code: ` +declare const arr: string[]; +declare const cond: Parameters['filter']>[0]; +const a = { arr }; +a?.arr + .filter(cond) /* what a bad spot for a comment. Let's make sure + there's some yucky symbols too. [ . ?. <> ' ' \\'] */ + .at('0'); + `, + errors: [ + { + line: 5, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +declare const arr: string[]; +declare const cond: Parameters['filter']>[0]; +const a = { arr }; +a?.arr + .find(cond) /* what a bad spot for a comment. Let's make sure + there's some yucky symbols too. [ . ?. <> ' ' \\'] */ + ; + `, + }, + ], + }, + ], + }, + { + code: ` +const imNotActuallyAnArray = [ + [1, 2, 3], + [2, 3, 4], +] as const; +const butIAm = [4, 5, 6]; +butIAm.push( + // line comment! + ...imNotActuallyAnArray[/* comment */ 'filter' /* another comment */]( + x => x[1] > 0, + ) /**/[\`0\`]!, +); + `, + errors: [ + { + line: 9, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +const imNotActuallyAnArray = [ + [1, 2, 3], + [2, 3, 4], +] as const; +const butIAm = [4, 5, 6]; +butIAm.push( + // line comment! + ...imNotActuallyAnArray[/* comment */ "find" /* another comment */]( + x => x[1] > 0, + ) /**/!, +); + `, + }, + ], + }, + ], + }, + { + code: ` +function actingOnArray(values: T) { + return values.filter(filter => filter === 'filter')[ + /* filter */ -0.0 /* filter */ + ]; +} + `, + errors: [ + { + line: 3, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +function actingOnArray(values: T) { + return values.find(filter => filter === 'filter'); +} + `, + }, + ], + }, + ], + }, ], }); diff --git a/packages/utils/src/eslint-utils/nullThrows.ts b/packages/utils/src/eslint-utils/nullThrows.ts index 1a79b2e09d43..41d1322694a2 100644 --- a/packages/utils/src/eslint-utils/nullThrows.ts +++ b/packages/utils/src/eslint-utils/nullThrows.ts @@ -11,7 +11,7 @@ const NullThrowsReasons = { * Assert that a value must not be null or undefined. * This is a nice explicit alternative to the non-null assertion operator. */ -function nullThrows(value: T | null | undefined, message: string): T { +function nullThrows(value: T, message: string): NonNullable { // this function is primarily used to keep types happy in a safe way // i.e. is used when we expect that a value is never nullish // this means that it's pretty much impossible to test the below if... @@ -19,6 +19,7 @@ function nullThrows(value: T | null | undefined, message: string): T { // so ignore it in coverage metrics. /* istanbul ignore if */ if (value == null) { + console.error(`Non-null Assertion Failed: ${message}`); throw new Error(`Non-null Assertion Failed: ${message}`); } From 7637266883d3ecec91735f8fe4d828db6411070e Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:01 -0700 Subject: [PATCH 03/15] remove console statement --- packages/utils/src/eslint-utils/nullThrows.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/utils/src/eslint-utils/nullThrows.ts b/packages/utils/src/eslint-utils/nullThrows.ts index 41d1322694a2..655ec2084682 100644 --- a/packages/utils/src/eslint-utils/nullThrows.ts +++ b/packages/utils/src/eslint-utils/nullThrows.ts @@ -19,7 +19,6 @@ function nullThrows(value: T, message: string): NonNullable { // so ignore it in coverage metrics. /* istanbul ignore if */ if (value == null) { - console.error(`Non-null Assertion Failed: ${message}`); throw new Error(`Non-null Assertion Failed: ${message}`); } From 1ab0477642a91041045881373b5044f8bdce2d6d Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:01 -0700 Subject: [PATCH 04/15] tweaks --- packages/eslint-plugin/src/rules/prefer-find.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-find.ts b/packages/eslint-plugin/src/rules/prefer-find.ts index 28f931d8580c..695b7b20985c 100644 --- a/packages/eslint-plugin/src/rules/prefer-find.ts +++ b/packages/eslint-plugin/src/rules/prefer-find.ts @@ -77,8 +77,8 @@ export default createRule({ callee.object, ); - // We can now report if the object is an array - // or if it's an optional chain on a nullable array. + // As long as the object is an array or an optional chain on a + // nullable array, this is an Array.prototype.filter expression. if ( checker.isArrayType(filteredObjectType) || checker.isTupleType(filteredObjectType) || @@ -153,10 +153,10 @@ export default createRule({ return String(value) === '0'; } - function generateFixToRemoveArrayAccess( + function generateFixToRemoveArrayElementAccess( fixer: TSESLint.RuleFixer, - arrayNode: TSESTree.Node, - wholeExpressionBeingFlagged: TSESTree.Node, + arrayNode: TSESTree.Expression, + wholeExpressionBeingFlagged: TSESTree.Expression, sourceCode: SourceCode, ): RuleFix { const tokenToStartDeletingFrom = nullThrows( @@ -195,7 +195,7 @@ export default createRule({ : 'find', ), // get rid of the .at(0) or ['at'](0) - generateFixToRemoveArrayAccess( + generateFixToRemoveArrayElementAccess( fixer, object, node, @@ -237,7 +237,7 @@ export default createRule({ : 'find', ), // Get rid of the [0] - generateFixToRemoveArrayAccess( + generateFixToRemoveArrayElementAccess( fixer, object, node, From 26a3d69aef8480a6335b023f341a910c09cceb7b Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:02 -0700 Subject: [PATCH 05/15] extract fix to function --- .../eslint-plugin/src/rules/prefer-find.ts | 34 ++++++++++++------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-find.ts b/packages/eslint-plugin/src/rules/prefer-find.ts index 695b7b20985c..0574136a382b 100644 --- a/packages/eslint-plugin/src/rules/prefer-find.ts +++ b/packages/eslint-plugin/src/rules/prefer-find.ts @@ -173,6 +173,19 @@ export default createRule({ ]); } + function generateFixToReplaceFilterWithFind( + fixer: TSESLint.RuleFixer, + filterExpression: { + isBracketSyntaxForFilter: boolean; + filterNode: TSESTree.Node; + }, + ): TSESLint.RuleFix { + return fixer.replaceText( + filterExpression.filterNode, + filterExpression.isBracketSyntaxForFilter ? '"find"' : 'find', + ); + } + return { CallExpression(node): void { const object = getObjectIfArrayAtExpression(node); @@ -188,11 +201,9 @@ export default createRule({ fix(fixer): TSESLint.RuleFix[] { const sourceCode = getSourceCode(context); return [ - fixer.replaceText( - filterExpression.filterNode, - filterExpression.isBracketSyntaxForFilter - ? '"find"' - : 'find', + generateFixToReplaceFilterWithFind( + fixer, + filterExpression, ), // get rid of the .at(0) or ['at'](0) generateFixToRemoveArrayElementAccess( @@ -217,8 +228,8 @@ export default createRule({ ): void { if (isMemberAccessOfZero(node)) { const object = node.object; - const parsedFilterExpression = parseIfArrayFilterExpression(object); - if (parsedFilterExpression) { + const filterExpression = parseIfArrayFilterExpression(object); + if (filterExpression) { context.report({ node, messageId: 'preferFind', @@ -229,12 +240,9 @@ export default createRule({ const sourceCode = context.sourceCode; return [ - // Replace .filter with .find - fixer.replaceText( - parsedFilterExpression.filterNode, - parsedFilterExpression.isBracketSyntaxForFilter - ? '"find"' - : 'find', + generateFixToReplaceFilterWithFind( + fixer, + filterExpression, ), // Get rid of the [0] generateFixToRemoveArrayElementAccess( From eb90e11c476a699a7befbd22628572fc318ec715 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:03 -0700 Subject: [PATCH 06/15] improve behavior around nulls --- .../eslint-plugin/src/rules/prefer-find.ts | 41 ++++++++++++------- .../tests/rules/prefer-find.test.ts | 2 + 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-find.ts b/packages/eslint-plugin/src/rules/prefer-find.ts index 0574136a382b..0c9b398764b8 100644 --- a/packages/eslint-plugin/src/rules/prefer-find.ts +++ b/packages/eslint-plugin/src/rules/prefer-find.ts @@ -3,6 +3,7 @@ import { AST_NODE_TYPES } from '@typescript-eslint/utils'; import { getScope, getSourceCode } from '@typescript-eslint/utils/eslint-utils'; import type { RuleFix, SourceCode } from '@typescript-eslint/utils/ts-eslint'; import * as tsutils from 'ts-api-utils'; +import type { Type } from 'typescript'; import { createRule, @@ -70,7 +71,6 @@ export default createRule({ : callee.property.name === 'filter' ) { const filterNode = callee.property; - const isOptionalAccessOfFilter = callee.optional; const filteredObjectType = getConstrainedTypeAtLocation( services, @@ -79,20 +79,7 @@ export default createRule({ // As long as the object is an array or an optional chain on a // nullable array, this is an Array.prototype.filter expression. - if ( - checker.isArrayType(filteredObjectType) || - checker.isTupleType(filteredObjectType) || - (isOptionalAccessOfFilter && - tsutils - .unionTypeParts(filteredObjectType) - .every( - unionPart => - checker.isArrayType(unionPart) || - checker.isTupleType(unionPart) || - tsutils.isIntrinsicNullType(unionPart) || - tsutils.isIntrinsicUndefinedType(unionPart), - )) - ) { + if (isArrayish(filteredObjectType)) { return { isBracketSyntaxForFilter, filterNode, @@ -105,6 +92,30 @@ export default createRule({ return undefined; } + /** + * Tells whether the type is a possibly nullable array/tuple or union thereof. + */ + function isArrayish(type: Type): boolean { + let isAtLeastOneArrayishComponent = false; + for (const unionPart of tsutils.unionTypeParts(type)) { + if ( + tsutils.isIntrinsicNullType(unionPart) || + tsutils.isIntrinsicUndefinedType(unionPart) + ) { + // ignore + } else if ( + checker.isArrayType(unionPart) || + checker.isTupleType(unionPart) + ) { + isAtLeastOneArrayishComponent = true; + } else { + return false; + } + } + + return isAtLeastOneArrayishComponent; + } + function getObjectIfArrayAtExpression( node: TSESTree.CallExpression, ): TSESTree.Expression | undefined { diff --git a/packages/eslint-plugin/tests/rules/prefer-find.test.ts b/packages/eslint-plugin/tests/rules/prefer-find.test.ts index 55f4cd6c8fd6..247e8fde8876 100644 --- a/packages/eslint-plugin/tests/rules/prefer-find.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-find.test.ts @@ -54,6 +54,8 @@ ruleTester.run('prefer-find', rule, { `, "['Just', 'a', 'filter'].filter(x => x.length > 4);", "['Just', 'a', 'find'].find(x => x.length > 4);", + 'undefined.filter(x => x)[0];', + 'null?.filter(x => x)[0];', ], invalid: [ From 8d74e4305db82ecd992580f621a8629566f0e5f2 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:03 -0700 Subject: [PATCH 07/15] add comments around array indexing checks --- .../eslint-plugin/src/rules/prefer-find.ts | 8 ++++ .../tests/rules/prefer-find.test.ts | 46 +++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/packages/eslint-plugin/src/rules/prefer-find.ts b/packages/eslint-plugin/src/rules/prefer-find.ts index 0c9b398764b8..e5d281101047 100644 --- a/packages/eslint-plugin/src/rules/prefer-find.ts +++ b/packages/eslint-plugin/src/rules/prefer-find.ts @@ -138,6 +138,10 @@ export default createRule({ return undefined; } + /** + * Implements the algorithm for array indexing by `.at()` method. + * @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/at#parameters + */ function isTreatedAsZeroByArrayAt(value: unknown): boolean { const asNumber = Number(value); @@ -160,6 +164,10 @@ export default createRule({ ); } + /** + * Implements the algorithm for array indexing by member operator. + * @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array#array_indices + */ function isTreatedAsZeroByMemberAccess(value: unknown): boolean { return String(value) === '0'; } diff --git a/packages/eslint-plugin/tests/rules/prefer-find.test.ts b/packages/eslint-plugin/tests/rules/prefer-find.test.ts index 247e8fde8876..4b159a023827 100644 --- a/packages/eslint-plugin/tests/rules/prefer-find.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-find.test.ts @@ -96,6 +96,52 @@ arr.filter(item => item === 'aha')[zero]; output: ` declare const arr: Array; const zero = 0; +arr.find(item => item === 'aha'); + `, + }, + ], + }, + ], + }, + { + code: ` +declare const arr: Array; +const zero = 0n; +arr.filter(item => item === 'aha')[zero]; + `, + errors: [ + { + line: 4, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +declare const arr: Array; +const zero = 0n; +arr.find(item => item === 'aha'); + `, + }, + ], + }, + ], + }, + { + code: ` +declare const arr: Array; +const zero = -0n; +arr.filter(item => item === 'aha')[zero]; + `, + errors: [ + { + line: 4, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +declare const arr: Array; +const zero = -0n; arr.find(item => item === 'aha'); `, }, From 6fb63d8181184a982603f88b039d986715fa671d Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:04 -0700 Subject: [PATCH 08/15] messages were backwards --- packages/eslint-plugin/src/rules/prefer-find.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-find.ts b/packages/eslint-plugin/src/rules/prefer-find.ts index e5d281101047..a0de1edb63e3 100644 --- a/packages/eslint-plugin/src/rules/prefer-find.ts +++ b/packages/eslint-plugin/src/rules/prefer-find.ts @@ -22,8 +22,8 @@ export default createRule({ requiresTypeChecking: true, }, messages: { - preferFind: 'Use .filter(...)[0] instead of .find(...)', - preferFindSuggestion: 'Use .filter(...)[0] instead of .find(...)', + preferFind: 'Use .find(...) instead of .filter(...)[0]', + preferFindSuggestion: 'Use .find(...) instead of .filter(...)[0]', }, schema: [], type: 'suggestion', From 8ac6fdd3692d0c762abd5d51d6934af4bc805a59 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:05 -0700 Subject: [PATCH 09/15] filter syntax --- packages/eslint-plugin/src/rules/prefer-find.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-find.ts b/packages/eslint-plugin/src/rules/prefer-find.ts index a0de1edb63e3..2eff6abefdb9 100644 --- a/packages/eslint-plugin/src/rules/prefer-find.ts +++ b/packages/eslint-plugin/src/rules/prefer-find.ts @@ -179,11 +179,12 @@ export default createRule({ sourceCode: SourceCode, ): RuleFix { const tokenToStartDeletingFrom = nullThrows( - sourceCode.getTokenAfter(arrayNode, { - // The next `.` or `[` is what we're looking for. - // think of (...).at(0) or (...)[0] or even (...)["at"](0). - filter: token => token.value === '.' || token.value === '[', - }), + // The next `.` or `[` is what we're looking for. + // think of (...).at(0) or (...)[0] or even (...)["at"](0). + sourceCode.getTokenAfter( + arrayNode, + token => token.value === '.' || token.value === '[', + ), 'Expected to find a member access token!', ); return fixer.removeRange([ From 9f39e63ba267ed570722f503d2e50f30cb1b4070 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:06 -0700 Subject: [PATCH 10/15] formatting --- packages/eslint-plugin/src/rules/prefer-find.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-find.ts b/packages/eslint-plugin/src/rules/prefer-find.ts index 2eff6abefdb9..0afa11ef85ef 100644 --- a/packages/eslint-plugin/src/rules/prefer-find.ts +++ b/packages/eslint-plugin/src/rules/prefer-find.ts @@ -218,7 +218,7 @@ export default createRule({ suggest: [ { messageId: 'preferFindSuggestion', - fix(fixer): TSESLint.RuleFix[] { + fix: (fixer): TSESLint.RuleFix[] => { const sourceCode = getSourceCode(context); return [ generateFixToReplaceFilterWithFind( @@ -256,9 +256,8 @@ export default createRule({ suggest: [ { messageId: 'preferFindSuggestion', - fix(fixer): TSESLint.RuleFix[] { + fix: (fixer): TSESLint.RuleFix[] => { const sourceCode = context.sourceCode; - return [ generateFixToReplaceFilterWithFind( fixer, From 3b4a611efd3584ff2de4534a11c9e395d532f3c1 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:06 -0700 Subject: [PATCH 11/15] add extra comma operator test --- .../tests/rules/prefer-find.test.ts | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/packages/eslint-plugin/tests/rules/prefer-find.test.ts b/packages/eslint-plugin/tests/rules/prefer-find.test.ts index 4b159a023827..693030eec175 100644 --- a/packages/eslint-plugin/tests/rules/prefer-find.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-find.test.ts @@ -495,5 +495,38 @@ function actingOnArray(values: T) { }, ], }, + { + code: ` +const nestedSequenceAbomination = + (1, + 2, + (1, + 2, + 3, + (1, 2, 3, 4), + (1, 2, 3, 4, 5, [1, 2, 3, 4, 5, 6].filter(x => x % 2 == 0)))['0']); + `, + errors: [ + { + line: 5, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +const nestedSequenceAbomination = + (1, + 2, + (1, + 2, + 3, + (1, 2, 3, 4), + (1, 2, 3, 4, 5, [1, 2, 3, 4, 5, 6].find(x => x % 2 == 0)))); + `, + }, + ], + }, + ], + }, ], }); From 702116c9d31bac7dbcd81e5a87893671b2e202f2 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:07 -0700 Subject: [PATCH 12/15] pr feedback round 2 --- .../eslint-plugin/src/rules/prefer-find.ts | 96 ++++++++++++------- .../tests/rules/prefer-find.test.ts | 2 +- 2 files changed, 64 insertions(+), 34 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-find.ts b/packages/eslint-plugin/src/rules/prefer-find.ts index 0afa11ef85ef..39defaae95b9 100644 --- a/packages/eslint-plugin/src/rules/prefer-find.ts +++ b/packages/eslint-plugin/src/rules/prefer-find.ts @@ -1,10 +1,18 @@ import type { TSESLint, TSESTree } from '@typescript-eslint/utils'; import { AST_NODE_TYPES } from '@typescript-eslint/utils'; import { getScope, getSourceCode } from '@typescript-eslint/utils/eslint-utils'; -import type { RuleFix, SourceCode } from '@typescript-eslint/utils/ts-eslint'; +import type { + RuleFix, + Scope, + SourceCode, +} from '@typescript-eslint/utils/ts-eslint'; import * as tsutils from 'ts-api-utils'; import type { Type } from 'typescript'; +import type { + MemberExpressionComputedName, + MemberExpressionNonComputedName, +} from '../../../types/src/generated/ast-spec'; import { createRule, getConstrainedTypeAtLocation, @@ -22,8 +30,8 @@ export default createRule({ requiresTypeChecking: true, }, messages: { - preferFind: 'Use .find(...) instead of .filter(...)[0]', - preferFindSuggestion: 'Use .find(...) instead of .filter(...)[0]', + preferFind: 'Prefer .find(...) instead of .filter(...)[0].', + preferFindSuggestion: 'Use .find(...) instead of .filter(...)[0].', }, schema: [], type: 'suggestion', @@ -37,13 +45,16 @@ export default createRule({ const services = getParserServices(context); const checker = services.program.getTypeChecker(); + interface FilterExpressionData { + isBracketSyntaxForFilter: boolean; + filterNode: TSESTree.Node; + } + function parseIfArrayFilterExpression( expression: TSESTree.Expression, - ): - | { isBracketSyntaxForFilter: boolean; filterNode: TSESTree.Node } - | undefined { + ): FilterExpressionData | undefined { if (expression.type === AST_NODE_TYPES.SequenceExpression) { - // Only the last expression in the (a, b, [1, 2, 3].filter(condition))[0] matters + // Only the last expression in (a, b, [1, 2, 3].filter(condition))[0] matters const lastExpression = nullThrows( expression.expressions.at(-1), 'Expected to have more than zero expressions in a sequence expression', @@ -65,11 +76,7 @@ export default createRule({ // or the optional chaining variants. if (callee.type === AST_NODE_TYPES.MemberExpression) { const isBracketSyntaxForFilter = callee.computed; - if ( - isBracketSyntaxForFilter - ? getStaticValue(callee.property, globalScope)?.value === 'filter' - : callee.property.name === 'filter' - ) { + if (isStaticMemberAccessOfValue(callee, 'filter', globalScope)) { const filterNode = callee.property; const filteredObjectType = getConstrainedTypeAtLocation( @@ -119,19 +126,20 @@ export default createRule({ function getObjectIfArrayAtExpression( node: TSESTree.CallExpression, ): TSESTree.Expression | undefined { - if (node.arguments.length === 1) { - const atArgument = getStaticValue(node.arguments[0], globalScope); - if (atArgument != null && isTreatedAsZeroByArrayAt(atArgument.value)) { - const callee = node.callee; - if ( - callee.type === AST_NODE_TYPES.MemberExpression && - !callee.optional && - (callee.computed - ? getStaticValue(callee.property, globalScope)?.value === 'at' - : callee.property.name === 'at') - ) { - return callee.object; - } + // .at() should take exactly one argument. + if (node.arguments.length !== 1) { + return undefined; + } + + const atArgument = getStaticValue(node.arguments[0], globalScope); + if (atArgument != null && isTreatedAsZeroByArrayAt(atArgument.value)) { + const callee = node.callee; + if ( + callee.type === AST_NODE_TYPES.MemberExpression && + !callee.optional && + isStaticMemberAccessOfValue(callee, 'at', globalScope) + ) { + return callee.object; } } @@ -195,10 +203,7 @@ export default createRule({ function generateFixToReplaceFilterWithFind( fixer: TSESLint.RuleFixer, - filterExpression: { - isBracketSyntaxForFilter: boolean; - filterNode: TSESTree.Node; - }, + filterExpression: FilterExpressionData, ): TSESLint.RuleFix { return fixer.replaceText( filterExpression.filterNode, @@ -207,6 +212,7 @@ export default createRule({ } return { + // This query will be used to find things like `filteredResults.at(0)`. CallExpression(node): void { const object = getObjectIfArrayAtExpression(node); if (object) { @@ -225,7 +231,7 @@ export default createRule({ fixer, filterExpression, ), - // get rid of the .at(0) or ['at'](0) + // Get rid of the .at(0) or ['at'](0). generateFixToRemoveArrayElementAccess( fixer, object, @@ -241,8 +247,10 @@ export default createRule({ } }, - // we're always looking for array member access to be "computed", - // i.e. filteredResults[0], since filteredResults.0 isn't a thing. + // This query will be used to find things like `filteredResults[0]`. + // + // Note: we're always looking for array member access to be "computed", + // i.e. `filteredResults[0]`, since `filteredResults.0` isn't a thing. ['MemberExpression[computed=true]']( node: TSESTree.MemberExpressionComputedName, ): void { @@ -263,7 +271,7 @@ export default createRule({ fixer, filterExpression, ), - // Get rid of the [0] + // Get rid of the [0]. generateFixToRemoveArrayElementAccess( fixer, object, @@ -281,3 +289,25 @@ export default createRule({ }; }, }); + +/** + * Answers whether the member expression looks like + * `x.memberName`, `x['memberName']`, + * or even `const mn = 'memberName'; x[mn]` (or optional variants thereof). + */ +function isStaticMemberAccessOfValue( + memberExpression: + | MemberExpressionComputedName + | MemberExpressionNonComputedName, + value: string, + scope?: Scope.Scope | undefined, +): boolean { + if (!memberExpression.computed) { + // x.memberName case. + return memberExpression.property.name === value; + } + + // x['memberName'] cases. + const staticValueResult = getStaticValue(memberExpression.property, scope); + return staticValueResult != null && value === staticValueResult.value; +} diff --git a/packages/eslint-plugin/tests/rules/prefer-find.test.ts b/packages/eslint-plugin/tests/rules/prefer-find.test.ts index 693030eec175..56656bebfb9e 100644 --- a/packages/eslint-plugin/tests/rules/prefer-find.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-find.test.ts @@ -22,7 +22,7 @@ ruleTester.run('prefer-find', rule, { declare const jerkCode: JerkCode; - jerkCode.filter(item => item === 'aha'); + jerkCode.filter(item => item === 'aha')[0]; `, ` declare const arr: readonly string[]; From a2a832db663cdb5a75a2661a64b5f8da8d6c95f0 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:08 -0700 Subject: [PATCH 13/15] Fix the the Co-authored-by: auvred <61150013+auvred@users.noreply.github.com> --- packages/eslint-plugin/docs/rules/prefer-find.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/prefer-find.md b/packages/eslint-plugin/docs/rules/prefer-find.md index 06f09f35aa25..62de96826809 100644 --- a/packages/eslint-plugin/docs/rules/prefer-find.md +++ b/packages/eslint-plugin/docs/rules/prefer-find.md @@ -12,7 +12,7 @@ Because the `.find()` only needs to execute the callback until it finds a match, :::info -Beware the the difference in short-circuiting behavior between the approaches. +Beware the difference in short-circuiting behavior between the approaches. `.find()` will only execute the callback on array elements until it finds a match, whereas `.filter()` executes the callback for all array elements. Therefore, when fixing errors from this rule, be sure that your `.filter()` callbacks do not have side effects. From c5442c023e93f064e89195416d98ec81ce5bda60 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:09 -0700 Subject: [PATCH 14/15] fix up imports --- packages/eslint-plugin/src/rules/prefer-find.ts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-find.ts b/packages/eslint-plugin/src/rules/prefer-find.ts index 39defaae95b9..9197c8fa7ef6 100644 --- a/packages/eslint-plugin/src/rules/prefer-find.ts +++ b/packages/eslint-plugin/src/rules/prefer-find.ts @@ -9,10 +9,6 @@ import type { import * as tsutils from 'ts-api-utils'; import type { Type } from 'typescript'; -import type { - MemberExpressionComputedName, - MemberExpressionNonComputedName, -} from '../../../types/src/generated/ast-spec'; import { createRule, getConstrainedTypeAtLocation, @@ -84,8 +80,8 @@ export default createRule({ callee.object, ); - // As long as the object is an array or an optional chain on a - // nullable array, this is an Array.prototype.filter expression. + // As long as the object is a (possibly nullable) array, + // this is an Array.prototype.filter expression. if (isArrayish(filteredObjectType)) { return { isBracketSyntaxForFilter, @@ -297,8 +293,8 @@ export default createRule({ */ function isStaticMemberAccessOfValue( memberExpression: - | MemberExpressionComputedName - | MemberExpressionNonComputedName, + | TSESTree.MemberExpressionComputedName + | TSESTree.MemberExpressionNonComputedName, value: string, scope?: Scope.Scope | undefined, ): boolean { From b3e6db338b0a7b1c341a60d8e3f38d2206e53b7e Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 2 Feb 2024 09:39:09 -0700 Subject: [PATCH 15/15] address intersections of arrays --- .../eslint-plugin/src/rules/prefer-find.ts | 25 +++++++---- .../tests/rules/prefer-find.test.ts | 42 +++++++++++++++++++ 2 files changed, 60 insertions(+), 7 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-find.ts b/packages/eslint-plugin/src/rules/prefer-find.ts index 9197c8fa7ef6..e94f384ecd33 100644 --- a/packages/eslint-plugin/src/rules/prefer-find.ts +++ b/packages/eslint-plugin/src/rules/prefer-find.ts @@ -105,15 +105,26 @@ export default createRule({ tsutils.isIntrinsicNullType(unionPart) || tsutils.isIntrinsicUndefinedType(unionPart) ) { - // ignore - } else if ( - checker.isArrayType(unionPart) || - checker.isTupleType(unionPart) - ) { - isAtLeastOneArrayishComponent = true; - } else { + continue; + } + + // apparently checker.isArrayType(T[] & S[]) => false. + // so we need to check the intersection parts individually. + const isArrayOrIntersectionThereof = tsutils + .intersectionTypeParts(unionPart) + .every( + intersectionPart => + checker.isArrayType(intersectionPart) || + checker.isTupleType(intersectionPart), + ); + + if (!isArrayOrIntersectionThereof) { + // There is a non-array, non-nullish type component, + // so it's not an array. return false; } + + isAtLeastOneArrayishComponent = true; } return isAtLeastOneArrayishComponent; diff --git a/packages/eslint-plugin/tests/rules/prefer-find.test.ts b/packages/eslint-plugin/tests/rules/prefer-find.test.ts index 56656bebfb9e..6b303ea9aeda 100644 --- a/packages/eslint-plugin/tests/rules/prefer-find.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-find.test.ts @@ -528,5 +528,47 @@ const nestedSequenceAbomination = }, ], }, + { + code: ` +declare const arr: { a: 1 }[] & { b: 2 }[]; +arr.filter(f, thisArg)[0]; + `, + errors: [ + { + line: 3, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +declare const arr: { a: 1 }[] & { b: 2 }[]; +arr.find(f, thisArg); + `, + }, + ], + }, + ], + }, + { + code: ` +declare const arr: { a: 1 }[] & ({ b: 2 }[] | { c: 3 }[]); +arr.filter(f, thisArg)[0]; + `, + errors: [ + { + line: 3, + messageId: 'preferFind', + suggestions: [ + { + messageId: 'preferFindSuggestion', + output: ` +declare const arr: { a: 1 }[] & ({ b: 2 }[] | { c: 3 }[]); +arr.find(f, thisArg); + `, + }, + ], + }, + ], + }, ], });