From 101927a646dd0b4b2c2b88699c07c2f5d704f001 Mon Sep 17 00:00:00 2001 From: Rel1cx Date: Thu, 11 Jun 2026 06:50:59 +0800 Subject: [PATCH] refactor(rules/no-array-index-key, rules/no-missing-key): improve key detection - no-array-index-key: replace name-based index tracking with scope-based resolution, fixing shadowed identifier false positives; also detect Number() coercion, ??/||/ternary fallbacks, and string-literal 'key' props in createElement calls - no-missing-key: track nested Children.toArray with a depth counter, support flatMap, and check conditional/logical branches in array literals without double-reporting nested arrays --- .../src/rules/no-array-index-key/CHANGELOG.md | 10 + .../src/rules/no-array-index-key/lib.ts | 123 ++++++------ .../no-array-index-key/no-array-index-key.mdx | 82 ++++++++ .../no-array-index-key.spec.ts | 73 +++++++ .../no-array-index-key/no-array-index-key.ts | 187 ++++++++---------- .../src/rules/no-missing-key/CHANGELOG.md | 9 + .../rules/no-missing-key/no-missing-key.mdx | 70 +++++++ .../no-missing-key/no-missing-key.spec.ts | 54 +++++ .../rules/no-missing-key/no-missing-key.ts | 131 ++++++------ 9 files changed, 522 insertions(+), 217 deletions(-) diff --git a/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/CHANGELOG.md b/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/CHANGELOG.md index 08de3c4497..1c8ffe36da 100644 --- a/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/CHANGELOG.md +++ b/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Replaced name-based index parameter tracking with scope-based resolution, so identifiers that shadow an index parameter are no longer misreported. + +### Added + +- Detect `Number(index)` coercion, in addition to `String(index)` and `index.toString()`. +- Detect index usage in `??`, `||`, and ternary fallback branches of a `key` value (e.g. `key={item.id ?? index}`). +- Detect string-literal `'key'` properties in `createElement`/`cloneElement` props objects. + ## [5.6.0] - 2026-04-29 ### Fixed diff --git a/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/lib.ts b/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/lib.ts index 82ea88c132..0c67f975e5 100644 --- a/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/lib.ts +++ b/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/lib.ts @@ -1,7 +1,9 @@ -import { Extract, isOneOf } from "@eslint-react/ast"; +import { Check, Extract, isOneOf } from "@eslint-react/ast"; import * as core from "@eslint-react/core"; import type { RuleContext } from "@eslint-react/eslint"; +import { DefinitionType } from "@typescript-eslint/scope-manager"; import { AST_NODE_TYPES as AST, type TSESTree } from "@typescript-eslint/types"; +import { findVariable } from "@typescript-eslint/utils/ast-utils"; import type { ReportDescriptor } from "@typescript-eslint/utils/ts-eslint"; /** @@ -16,68 +18,71 @@ export function report(context: RuleContext) { }; } -export function getIndexParamPosition(methodName: string) { - switch (methodName) { - case "every": - case "filter": - case "find": - case "findIndex": - case "findLast": - case "findLastIndex": - case "flatMap": - case "forEach": - case "map": - case "some": - return 1; - case "reduce": - case "reduceRight": - return 2; - default: - return -1; - } -} +/** + * Iterator-like methods that pass the item's index to their callback, + * mapped to the position of the index parameter in the callback's parameter list. + * `map` and `forEach` also cover `Children.map` and `Children.forEach`, + * whose callback is the second argument instead of the first. + */ +const INDEX_PARAM_POSITIONS = new Map([ + ["every", 1], + ["filter", 1], + ["find", 1], + ["findIndex", 1], + ["findLast", 1], + ["findLastIndex", 1], + ["flatMap", 1], + ["forEach", 1], + ["map", 1], + ["reduce", 2], + ["reduceRight", 2], + ["some", 1], +]); -// Gets the name of the index parameter from a map-like function's callback -// e.g., in `data.map((item, index) => ...)` it returns 'index' -export function getMapIndexParamName(context: RuleContext, node: TSESTree.CallExpression): string | null { - const callee = Extract.unwrap(node.callee); - if (callee.type !== AST.MemberExpression) { - return null; - } - if (callee.property.type !== AST.Identifier) { - return null; - } - const { name } = callee.property; - // Determines the position of the index parameter for array methods like 'map', 'forEach', etc - const indexPosition = getIndexParamPosition(name); - if (indexPosition === -1) { - return null; - } - // The callback function is the first argument, or the second for `React.Children` methods - const callbackArg = node.arguments[ - core.isChildrenMap(context, callee) || core.isChildrenForEach(context, callee) - ? 1 - : 0 - ]; - if (callbackArg == null) { - return null; - } - if (!isOneOf([AST.ArrowFunctionExpression, AST.FunctionExpression])(callbackArg)) { - return null; - } - const { params } = callbackArg; - if (params.length < indexPosition + 1) { - return null; +/** + * Checks whether an identifier is a reference to the index parameter of an + * iterator-like method's callback (e.g. `i` in `items.map((item, i) => ...)`). + * @param context The ESLint rule context. + * @param node The identifier to check. + * @returns `true` if the identifier resolves to an array index parameter. + */ +export function isArrayIndexReference(context: RuleContext, node: TSESTree.Identifier): boolean { + const variable = findVariable(context.sourceCode.getScope(node), node); + const def = variable?.defs.at(0); + if (def == null || def.type !== DefinitionType.Parameter) return false; + const callback = def.node; + if (!isOneOf([AST.ArrowFunctionExpression, AST.FunctionExpression])(callback)) return false; + // The argument node as it appears in the call, including any TS type wrappers around the callback + let argument: TSESTree.Node = callback; + while (Check.isTypeExpression(argument.parent)) { + argument = argument.parent; } - const param = params.at(indexPosition); - - return param != null && "name" in param - ? param.name - : null; + const call = argument.parent; + if (call.type !== AST.CallExpression) return false; + const callee = Extract.unwrap(call.callee); + if (callee.type !== AST.MemberExpression) return false; + if (callee.property.type !== AST.Identifier) return false; + const indexPosition = INDEX_PARAM_POSITIONS.get(callee.property.name); + if (indexPosition == null) return false; + // The callback is the first argument, or the second for `Children.map`/`Children.forEach` + const callbackPosition = core.isChildrenMap(context, callee) || core.isChildrenForEach(context, callee) + ? 1 + : 0; + if (call.arguments[callbackPosition] !== argument) return false; + // The resolved binding must be the parameter at the index position itself, + // possibly wrapped in a default value assignment (e.g. `(item, i = 0) => ...`) + const param = callback.params[indexPosition]; + if (param == null) return false; + return param === def.name + || (param.type === AST.AssignmentPattern && param.left === def.name); } -// Recursively collects all identifiers from a binary expression -// e.g., for `a + b + c`, it returns identifiers for a, b, and c +/** + * Recursively collects all identifiers from a binary expression. + * e.g., for `a + b + c`, it returns identifiers for a, b, and c. + * @param side The binary expression (or one of its sides) to collect from. + * @returns The identifiers found in the expression. + */ export function getIdentifiersFromBinaryExpression( side: TSESTree.BinaryExpression["left"], ): readonly TSESTree.Identifier[] { diff --git a/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/no-array-index-key.mdx b/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/no-array-index-key.mdx index 1dc087d0a8..aa52b28edb 100644 --- a/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/no-array-index-key.mdx +++ b/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/no-array-index-key.mdx @@ -68,6 +68,88 @@ function MyComponent({ items }: MyComponentProps) { } ``` +### Deriving the key from the index + +Wrapping the index in a string or a conversion does not make it a stable identity — the key still changes whenever the list is reordered. The rule also reports indexes used in template literals, string concatenation, and `String()`, `Number()`, or `.toString()` conversions. + +```tsx +// Problem: The key is still derived from the array index +interface MyComponentProps { + items: { id: string; name: string }[]; +} + +function MyComponent({ items }: MyComponentProps) { + return ( + + ); +} +``` + +```tsx +// Recommended: Derive the key from the data itself +interface MyComponentProps { + items: { id: string; name: string }[]; +} + +function MyComponent({ items }: MyComponentProps) { + return ( + + ); +} +``` + +### Falling back to the index + +Using the index as a fallback when an identifier is missing still gives some items an unstable identity. The rule checks every branch a key may evaluate to, including `??`, `||`, and ternary expressions. + +```tsx +// Problem: Items without an id silently fall back to the index +interface MyComponentProps { + items: { id?: string; name: string }[]; +} + +function MyComponent({ items }: MyComponentProps) { + return ( + + ); +} +``` + +```tsx +// Recommended: Assign a stable id when the item is created, not at render time +interface MyComponentProps { + items: { id: string; name: string }[]; +} + +function createItem(name: string) { + return { id: crypto.randomUUID(), name }; +} + +function MyComponent({ items }: MyComponentProps) { + return ( + + ); +} +``` + ### Rendering nested lists When rendering nested lists, both the outer and inner items need stable keys. Using the item's own identifier ensures React can correctly track elements at every level. diff --git a/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/no-array-index-key.spec.ts b/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/no-array-index-key.spec.ts index 2343af0bd2..303098ac2f 100644 --- a/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/no-array-index-key.spec.ts +++ b/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/no-array-index-key.spec.ts @@ -255,6 +255,41 @@ ruleTester.run(RULE_NAME, rule, { code: tsx`foo.map((value, index) => )`, errors: [{ messageId: "default" }], }, + { + code: tsx`foo.map((bar, i) => )`, + errors: [{ messageId: "default" }], + }, + { + code: tsx`foo.map((bar, i) => )`, + errors: [{ messageId: "default" }], + }, + { + code: tsx`foo.map((bar, i) => )`, + errors: [{ messageId: "default" }], + }, + { + code: tsx`foo.map((bar, i = 0) => )`, + errors: [{ messageId: "default" }], + }, + { + code: tsx`foo.map((bar, i) => bar.items.map((baz) => ))`, + errors: [{ messageId: "default" }], + }, + { + // The identifier resolves to the nearest binding: the inner map's index + code: tsx`foo.map((bar, i) => bar.items.map((item, i) => ))`, + errors: [{ messageId: "default" }], + }, + { + // Recursion reaches the index through chained logical branches + code: tsx`foo.map((bar, i) => )`, + errors: [{ messageId: "default" }], + }, + { + // String-literal 'key' property is treated like the identifier form + code: tsx`foo.map((bar, i) => React.createElement('Foo', { 'key': i }))`, + errors: [{ messageId: "default" }], + }, ], valid: [ // https://github.com/oxc-project/oxc/issues/21110 @@ -303,5 +338,43 @@ ruleTester.run(RULE_NAME, rule, { ); } `, + // The identifier shadows the index parameter with a different value + tsx` + foo.map((bar, i) => { + return bar.items.map((item) => { + const i = item.id; + return ; + }); + }) + `, + // The identifier is not an index parameter of an iterator-like callback + tsx` + const i = getId(); + foo.map((bar) => ); + `, + tsx`foo.customMap((bar, i) => )`, + // The 'key' is derived from item data, not the array index + tsx`foo.map((bar, i) => )`, + tsx`foo.map((bar, i) => )`, + // Only the parameter at the index position counts, not other callback params + tsx`foo.map((bar, i) => )`, + tsx`foo.reduce((acc, bar, i) => acc.concat(), [])`, + // The function must be at the callback position of the call + tsx`foo.map(notCallback, (bar, i) => )`, + // Only inline callbacks are tracked, not function references + tsx` + function renderItem(bar, i) { + return ; + } + foo.map(renderItem); + `, + // The index in a ternary test is not part of the key's value + tsx`foo.map((bar, i) => 0 ? bar.id : bar.name} />)`, + // Only direct template interpolations are checked, not nested expressions + tsx`foo.map((bar, i) => )`, + // Only 'toString', 'String' and 'Number' conversions are tracked + tsx`foo.map((bar, i) => )`, + // Computed 'key' properties are not checked + tsx`foo.map((bar, i) => React.createElement('Foo', { ['key']: i }))`, ], }); diff --git a/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/no-array-index-key.ts b/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/no-array-index-key.ts index 9fb4b55a1b..76b1a67cac 100644 --- a/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/no-array-index-key.ts +++ b/plugins/eslint-plugin-react-x/src/rules/no-array-index-key/no-array-index-key.ts @@ -4,8 +4,7 @@ import * as core from "@eslint-react/core"; import { type RuleContext, type RuleFeature, type RuleListener } from "@eslint-react/eslint"; import { AST_NODE_TYPES as AST, type TSESTree } from "@typescript-eslint/types"; import type { ReportDescriptor } from "@typescript-eslint/utils/ts-eslint"; -import { isMatching } from "ts-pattern"; -import { getIdentifiersFromBinaryExpression, getMapIndexParamName, report } from "./lib"; +import { getIdentifiersFromBinaryExpression, isArrayIndexReference, report } from "./lib"; export const RULE_NAME = "no-array-index-key"; @@ -13,6 +12,9 @@ export const RULE_FEATURES = [] as const satisfies RuleFeature[]; export type MessageID = "default"; +/** Global functions that convert their first argument while preserving its identity as a key. */ +const COERCION_FUNCTIONS = new Set(["Number", "String"]); + export default createRule<[], MessageID>({ meta: { type: "suggestion", @@ -29,14 +31,24 @@ export default createRule<[], MessageID>({ defaultOptions: [], }); +/** + * Gets the value of an object property named 'key' (e.g. in `createElement('div', { key: ... })`). + * @param property The object literal member to inspect. + * @returns The value of the 'key' property, or `null` if the member is not one. + */ +function getKeyPropValue(property: TSESTree.ObjectLiteralElement): TSESTree.Node | null { + if (property.type !== AST.Property || property.computed) return null; + const isKeyName = (property.key.type === AST.Identifier && property.key.name === "key") + || (property.key.type === AST.Literal && property.key.value === "key"); + return isKeyName ? property.value : null; +} + export function create(context: RuleContext): RuleListener { - // A stack to keep track of index parameter names from nested map calls - const indexParamNames: Array = []; + type Descriptor = ReportDescriptor & { node: TSESTree.Node }; - // Checks if a given node is an identifier that matches a known array index parameter name + // Checks if a given node is an identifier that resolves to an array index parameter function isArrayIndex(node: TSESTree.Node): node is TSESTree.Identifier { - return node.type === AST.Identifier - && indexParamNames.some((name) => name != null && name === node.name); + return node.type === AST.Identifier && isArrayIndexReference(context, node); } // Checks if a call expression is `React.createElement` or `React.cloneElement` @@ -44,117 +56,88 @@ export function create(context: RuleContext): RuleListener { return core.isCreateElementCall(context, node) || core.isCloneElementCall(context, node); } - // Generates report descriptors for various ways an array index can be used as a key - function getReportDescriptors(node: TSESTree.Node): ReportDescriptor[] { + /** + * Checks an expression used as a 'key' value, recursing into the branches + * it may evaluate to and reporting every array index it is derived from. + * @param node The key value expression to check. + * @returns The report descriptors for the violations found. + */ + function checkKeyExpression(node: TSESTree.Node): Descriptor[] { switch (node.type) { // Case: key={index} - case AST.Identifier: { - if (indexParamNames.some((name) => name != null && name === node.name)) { - return [{ - messageId: "default", - node, - }]; - } - return []; - } - // Case: key={`foo-${index}`} or key={'foo' + index} + case AST.Identifier: + return isArrayIndex(node) + ? [{ messageId: "default", node }] + : []; + // Case: key={`foo-${index}`} + // Note: only direct interpolations are checked; composite expressions + // like `${item.type + index}` derive the key from item data as well case AST.TemplateLiteral: - case AST.BinaryExpression: { - const descriptors: ReportDescriptor[] = []; - const expressions = node.type === AST.TemplateLiteral - ? node.expressions - : getIdentifiersFromBinaryExpression(node); - for (const expression of expressions) { - if (isArrayIndex(expression)) { - descriptors.push({ - messageId: "default", - node: expression, - }); - } - } - return descriptors; - } + return node.expressions + .filter(isArrayIndex) + .map((expression) => ({ messageId: "default", node: expression })); + // Case: key={'foo' + index} + case AST.BinaryExpression: + return getIdentifiersFromBinaryExpression(node) + .filter(isArrayIndex) + .map((identifier) => ({ messageId: "default", node: identifier })); + // Case: key={cond ? index : id} + case AST.ConditionalExpression: + return [ + ...checkKeyExpression(node.consequent), + ...checkKeyExpression(node.alternate), + ]; + // Case: key={id || index} + case AST.LogicalExpression: + return [ + ...checkKeyExpression(node.left), + ...checkKeyExpression(node.right), + ]; // Case: key={index.toString()} or key={String(index)} case AST.CallExpression: { const callee = Extract.unwrap(node.callee); - switch (true) { - // Case: key={index.toString()} - case callee.type === AST.MemberExpression - && callee.property.type === AST.Identifier - && callee.property.name === "toString" - && isArrayIndex(callee.object): { - return [{ - messageId: "default", - node: callee.object, - }]; - } - // Case: key={String(index)} - case callee.type === AST.Identifier - && callee.name === "String" - && node.arguments[0] != null - && isArrayIndex(node.arguments[0]): { - return [{ - messageId: "default", - node: node.arguments[0], - }]; - } + // Case: key={index.toString()} + if ( + callee.type === AST.MemberExpression + && callee.property.type === AST.Identifier + && callee.property.name === "toString" + && isArrayIndex(callee.object) + ) { + return [{ messageId: "default", node: callee.object }]; + } + // Case: key={String(index)} or key={Number(index)} + const argument = node.arguments.at(0); + if ( + callee.type === AST.Identifier + && COERCION_FUNCTIONS.has(callee.name) + && argument != null + && isArrayIndex(argument) + ) { + return [{ messageId: "default", node: argument }]; } + return []; } + default: + return []; } - return []; } return { + // Handles 'key' props in `createElement` and `cloneElement` calls CallExpression(node) { - // Push the index parameter name (if any) to the stack - indexParamNames.push(getMapIndexParamName(context, node)); - if (node.arguments.length === 0) { - return; - } - // Check for array index usage in `createElement` and `cloneElement` calls - if (!isCreateOrCloneElementCall(node)) { - return; - } + if (!isCreateOrCloneElementCall(node)) return; const [, props] = node.arguments; - if (props?.type !== AST.ObjectExpression) { - return; - } - for (const prop of props.properties) { - // Find the 'key' prop - if (!isMatching({ key: { name: "key" } })(prop)) { - continue; - } - if (!("value" in prop)) { - continue; - } - // Check its value and report if it uses an array index - for (const descriptor of getReportDescriptors(prop.value)) { - report(context)(descriptor); - } + if (props?.type !== AST.ObjectExpression) return; + for (const property of props.properties) { + const value = getKeyPropValue(property); + if (value == null) continue; + checkKeyExpression(value).forEach(report(context)); } }, - // When exiting a CallExpression, pop the index name from the stack to manage scope - "CallExpression:exit"() { - indexParamNames.pop(); - }, - // Handles JSX attributes - JSXAttribute(node) { - // Check only for the 'key' attribute - if (node.name.name !== "key") { - return; - } - // Ignore if we are not inside a map-like call - if (indexParamNames.length === 0) { - return; - } - // The key's value must be an expression container (ex: key={...}) - if (node.value?.type !== AST.JSXExpressionContainer) { - return; - } - // Check the expression and report if it uses an array index - for (const descriptor of getReportDescriptors(node.value.expression)) { - report(context)(descriptor); - } + // Handles 'key' attributes in JSX elements + "JSXAttribute[name.name='key']"(node: TSESTree.JSXAttribute) { + if (node.value?.type !== AST.JSXExpressionContainer) return; + checkKeyExpression(node.value.expression).forEach(report(context)); }, }; } diff --git a/plugins/eslint-plugin-react-x/src/rules/no-missing-key/CHANGELOG.md b/plugins/eslint-plugin-react-x/src/rules/no-missing-key/CHANGELOG.md index 406841c425..ff1a72ff2b 100644 --- a/plugins/eslint-plugin-react-x/src/rules/no-missing-key/CHANGELOG.md +++ b/plugins/eslint-plugin-react-x/src/rules/no-missing-key/CHANGELOG.md @@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fixed checks being incorrectly resumed inside an outer `Children.toArray()` after a nested `Children.toArray()` call exits, by tracking nesting depth instead of a boolean flag. + +### Added + +- Check `.flatMap()` callbacks in addition to `.map()` and `Array.from()`. +- Check conditional (`? :`) and logical (`&&`, `||`, `??`) branches of elements in array literals. + ## [5.7.8] - 2026-05-14 ### Fixed diff --git a/plugins/eslint-plugin-react-x/src/rules/no-missing-key/no-missing-key.mdx b/plugins/eslint-plugin-react-x/src/rules/no-missing-key/no-missing-key.mdx index 954a3b2868..f708bf8739 100644 --- a/plugins/eslint-plugin-react-x/src/rules/no-missing-key/no-missing-key.mdx +++ b/plugins/eslint-plugin-react-x/src/rules/no-missing-key/no-missing-key.mdx @@ -33,6 +33,8 @@ React needs keys to identify items in the list. If you don’t specify a key, Re ### Missing key in list rendering +The rule checks the values produced by iterator callbacks of `map`, `flatMap`, and `Array.from`. + ```tsx // Problem: When a key is missing, React uses the array index as the default key, which can lead to state mismatch or performance issues interface MyComponentProps { @@ -65,6 +67,74 @@ function MyComponent({ items }: MyComponentProps) { } ``` +### Missing key in conditionally rendered items + +When a list item is rendered conditionally, every branch that produces an element needs a key. The rule checks all branches an item may evaluate to, including `? :`, `&&`, `||`, and `??` expressions. + +```tsx +// Problem: Each branch of the conditional produces a list item without a key +interface MyComponentProps { + items: { id: number; name: string; pinned: boolean }[]; +} + +function MyComponent({ items }: MyComponentProps) { + return ( +
    + {items.map((item) => ( + item.pinned + ? + : + ))} +
+ ); +} +``` + +```tsx +// Recommended: Provide a key in every branch +interface MyComponentProps { + items: { id: number; name: string; pinned: boolean }[]; +} + +function MyComponent({ items }: MyComponentProps) { + return ( +
    + {items.map((item) => ( + item.pinned + ? + : + ))} +
+ ); +} +``` + +### Missing key on elements in an array literal + +Elements placed directly in an array literal become list items just like elements returned from `map`, so each of them needs a key as well. + +```tsx +// Problem: Elements in an array literal are rendered as a list +function MyComponent() { + return [ +
, + , +