Revert no-unnecessary-key to remove constant key checking, closes #1436 (#1439)

Author: Rel1cx

apps/website/content/docs/rules/overview.mdx

@@ -235,7 +235,7 @@ full: true
     | [`no-set-state-in-component-did-update`](no-set-state-in-component-did-update)     | 1️⃣ 1️⃣ |           | Disallows calling `this.setState` in `componentDidUpdate` outside functions such as callbacks                                 |          |
     | [`no-set-state-in-component-will-update`](no-set-state-in-component-will-update)   | 1️⃣ 1️⃣ |           | Disallows calling `this.setState` in `componentWillUpdate` outside functions such as callbacks                                |          |
     | [`no-string-refs`](no-string-refs)                                                 | 2️⃣ 2️⃣ |    `🔄`   | Replaces string refs with callback refs                                                                                       | >=16.3.0 |
-    | [`no-unnecessary-key`](no-unnecessary-key)                                         | 0️⃣ 0️⃣ |    `🧪`   | Disallows unnecessary `key` props on elements                                                                                 |          |
+    | [`no-unnecessary-key`](no-unnecessary-key)                                         | 0️⃣ 0️⃣ |    `🧪`   | Disallows unnecessary `key` props on nested child elements when rendering lists                                               |          |
     | [`no-unnecessary-use-callback`](no-unnecessary-use-callback)                       | 0️⃣ 1️⃣ |    `🧪`   | Disallows unnecessary usage of `useCallback`                                                                                  |          |
     | [`no-unnecessary-use-memo`](no-unnecessary-use-memo)                               | 0️⃣ 1️⃣ |    `🧪`   | Disallows unnecessary usage of `useMemo`                                                                                      |          |
     | [`no-unnecessary-use-ref`](no-unnecessary-use-ref)                                 | 0️⃣ 0️⃣ |    `🧪`   | Disallows unnecessary usage of `useRef`                                                                                       |          |

packages/plugins/eslint-plugin-react-x/src/rules/no-duplicate-key.mdx

@@ -130,4 +130,4 @@ function MyComponent() {
 - [`no-array-index-key`](./no-array-index-key)\
   Warns when an array `index` is used as a `key` prop.
 - [`no-unnecessary-key`](./no-unnecessary-key)\
-  Disallows unnecessary `key` props on elements.
+  Disallows unnecessary `key` props on nested child elements when rendering lists.

packages/plugins/eslint-plugin-react-x/src/rules/no-unnecessary-key.mdx

@@ -22,14 +22,12 @@ react-x/no-unnecessary-key
 
 ## Description
 
-Disallows unnecessary `key` props on elements.
+Disallows unnecessary `key` props on nested child elements when rendering lists.
 
 When rendering a list of elements in React, the `key` prop should only be placed on the outermost element for each item in the list. Adding keys to nested child elements is redundant, can cause confusion, and may lead to subtle bugs during refactoring.
 
 For example, if an element with a `key` is wrapped with a `React.Fragment` or another component, the `key` must be moved to the new wrapping element. Forgetting to remove the original `key` from the child element can lead to runtime warnings from React if it's duplicated or simply leave unnecessary code. This rule helps identify and remove these redundant `key` props.
 
-Also, static `key` props on elements that are not part of a _dynamic structure_ (e.g., inside list rendering, conditional rendering, or control flow statements) are unnecessary and should be removed.
-
 ## Examples
 
 ### Failing
@@ -53,17 +51,6 @@ things.map(thing => (
 ))
 ```
 
-```tsx
-// Static key on an element outside of a dynamic structure is unnecessary
-function ComponentWithStaticKey() {
-  return (
-    <div>
-      <MyComponent key="static-key" /> {/* This key is unnecessary */}
-    </div>
-  );
-}
-```
-
 ### Passing
 
 ```tsx
@@ -77,34 +64,6 @@ things.map(thing => (
     <div>{thing.description}</div>
   </React.Fragment>
 ))
-
-// Keys used to re-mount components are allowed
-function ComponentWithDynamicKey({ someValue }) {
-  return (
-    <div>
-      <MyComponent key={someValue} />
-    </div>
-  );
-}
-```
-
-Additionally, keys are allowed in dynamic structures such as conditionals or control flow statements:
-
-```tsx
-function ComponentWithControlFlow({items}) {
-  const elements = [];
-  for (const item of items) {
-    elements.push(<MyComponent key={item.id} />);
-  }
-  return <div>{elements}</div>;
-}
-
-function ComponentWithControlFlow2({cond}) {
-  if (cond) {
-    return <MyComponent key="key" />;
-  }
-  return <MyComponent />;
-}
 ```
 
 ## Implementation

packages/plugins/eslint-plugin-react-x/src/rules/no-unnecessary-key.spec.ts

@@ -5,31 +5,6 @@ import rule, { RULE_NAME } from "./no-unnecessary-key";
 
 ruleTester.run(RULE_NAME, rule, {
   invalid: [
-    {
-      code: tsx`
-        <div key="static-key"></div>
-      `,
-      errors: [{ messageId: "noUnnecessaryKey" }],
-    },
-    {
-      code: tsx`
-        <>
-          <span key="child-1"></span>
-          <span key="child-2"></span>
-        </>
-      `,
-      errors: [{ messageId: "noUnnecessaryKey" }, { messageId: "noUnnecessaryKey" }],
-    },
-    {
-      code: tsx`
-        things.map(thing => {
-          function NestedComponent() {
-            return <span key='foo'><span key='bar' /></span>;
-          }
-        })
-      `,
-      errors: [{ messageId: "noUnnecessaryKey" }, { messageId: "noUnnecessaryKey" }],
-    },
     // Invalid:  unnecessary key on a child element in a map
     {
       code: tsx`
@@ -51,15 +26,6 @@ ruleTester.run(RULE_NAME, rule, {
       `,
       errors: [{ messageId: "noUnnecessaryKey" }, { messageId: "noUnnecessaryKey" }],
     },
-    // TODO: Add support for array literal
-    // Invalid: unnecessary key on a child element in an array literal
-    // {
-    //   code: tsx`
-    //     const elements = [<div key='1'><p key='child' /></div>]
-    //   `,
-    //   errors: [{ messageId: "noUnnecessaryKey" }],
-    // },
-    // Invalid: unnecessary key within an element returned from a function expression
     {
       code: tsx`
         things.map(function(thing) { return <div key={thing.id}><i key='icon' /></div>; })
@@ -73,27 +39,11 @@ ruleTester.run(RULE_NAME, rule, {
       `,
       errors: [{ messageId: "noUnnecessaryKey" }],
     },
-    // Invalid:  unnecessary key with a static value
-    {
-      code: tsx`
-        <span key="static-key"></span>
-      `,
-      errors: [{ messageId: "noUnnecessaryKey" }],
-    },
-    // Invalid: static key in a simple function component
-    {
-      code: tsx`
-        function SimpleComponent() {
-          return <div key="unnecessary" />;
-        }
-      `,
-      errors: [{ messageId: "noUnnecessaryKey" }],
-    },
     // Invalid: deeply nested unnecessary keys
     {
       code: tsx`
         things.map(thing => (
-          <div key={thing. id}>
+          <div key={thing.id}>
             <section>
               <article>
                 <p key="deep-nested">Content</p>
@@ -111,57 +61,6 @@ ruleTester.run(RULE_NAME, rule, {
       `,
       errors: [{ messageId: "noUnnecessaryKey" }],
     },
-    // Invalid: key with template literal value
-    {
-      code: tsx`
-        <div key={\`static-template\`}></div>
-      `,
-      errors: [{ messageId: "noUnnecessaryKey" }],
-    },
-    // Invalid: key with number literal
-    {
-      code: tsx`
-        <div key={123}></div>
-      `,
-      errors: [{ messageId: "noUnnecessaryKey" }],
-    },
-    // Invalid:  key on child with sibling elements (not in list context)
-    {
-      code: tsx`
-        function Component() {
-          return (
-            <div>
-              <span key="first" />
-              <span key="second" />
-            </div>
-          );
-        }
-      `,
-      errors: [{ messageId: "noUnnecessaryKey" }, { messageId: "noUnnecessaryKey" }],
-    },
-    // Invalid: unnecessary key in flatMap callback
-    {
-      code: tsx`
-        things.flatMap(thing => <div key={thing.id}><span key="child" /></div>)
-      `,
-      errors: [{ messageId: "noUnnecessaryKey" }],
-    },
-    // Invalid: key on wrapper and child in reduce
-    // {
-    //   code: tsx`
-    //     items.reduce((acc, item) => [... acc, <div key={item.id}><p key="inner" /></div>], [])
-    //   `,
-    //   errors: [{ messageId: "noUnnecessaryKey" }],
-    // },
-    // Invalid: unnecessary keys in forEach (not returning elements, but still detected)
-    {
-      code: tsx`
-        function Component() {
-          return <div key="outer"><span key="inner" /></div>;
-        }
-      `,
-      errors: [{ messageId: "noUnnecessaryKey" }, { messageId: "noUnnecessaryKey" }],
-    },
     // Invalid: multiple levels of unnecessary keys
     {
       code: tsx`
@@ -322,9 +221,9 @@ ruleTester.run(RULE_NAME, rule, {
       };
     `,
     // Valid: key on element in reduce accumulator
-    // tsx`
-    //   items.reduce((acc, item) => [...acc, <div key={item.id} />], [])
-    // `,
+    tsx`
+      items.reduce((acc, item) => [...acc, <div key={item.id} />], [])
+    `,
     // Valid: key with spread attribute (should be skipped)
     tsx`
       things.map(thing => <div {... props} key={thing.id} />)
@@ -380,5 +279,15 @@ ruleTester.run(RULE_NAME, rule, {
     tsx`
       [...items].map(item => <div key={item.id} />)
     `,
+    // https://github.com/Rel1cx/eslint-react/issues/1436
+    tsx`
+      export default function App() {
+        return [getChild()];
+      }
+
+      function getChild() {
+        return <div key="key">foo</div>;
+      }
+    `,
   ],
 });

packages/plugins/eslint-plugin-react-x/src/rules/no-unnecessary-key.ts

@@ -5,10 +5,8 @@ import {
   getJsxConfigFromContext,
   isJsxFragmentElement,
   isRenderFunctionLoose,
-  useComponentCollector,
 } from "@eslint-react/core";
-import { type RuleContext, type RuleFeature, defineRuleListener } from "@eslint-react/shared";
-import { findEnclosingAssignmentTarget } from "@eslint-react/var";
+import { type RuleContext, type RuleFeature } from "@eslint-react/shared";
 import type { TSESTree } from "@typescript-eslint/types";
 import { AST_NODE_TYPES as T } from "@typescript-eslint/types";
 import type { RuleListener } from "@typescript-eslint/utils/ts-eslint";
@@ -28,7 +26,7 @@ export default createRule<[], MessageID>({
   meta: {
     type: "problem",
     docs: {
-      description: "Disallows unnecessary 'key' props on elements.",
+      description: "Disallows unnecessary 'key' props on nested child elements when rendering lists.",
     },
     messages: {
       noUnnecessaryKey: "Unnecessary `key` prop on this element. {{reason}}",
@@ -47,70 +45,44 @@ export function create(context: RuleContext<MessageID, []>): RuleListener {
     ...getJsxConfigFromContext(context),
     ...getJsxConfigFromAnnotation(context),
   };
-  const { ctx, visitor } = useComponentCollector(context);
-  const constantKeys = new Set<TSESTree.JSXAttribute>();
-  return defineRuleListener(
-    visitor,
-    {
-      JSXAttribute(node: TSESTree.JSXAttribute) {
-        // Check if the attribute is a `key` prop
-        if (node.name.name !== "key") return;
-        const jsxElement = node.parent.parent;
-        // Always allow `<React.Fragment key={...}>` to avoid false positives
-        if (isJsxFragmentElement(context, jsxElement, jsxConfig)) return;
-        // If there is a spread attribute, it's not safe to report an unnecessary key
-        if (jsxElement.openingElement.attributes.some((attr) => attr.type === T.JSXSpreadAttribute)) return;
-        // If inside a render function, skip checking to avoid false positives
-        if (AST.findParentNode(jsxElement, (n) => isRenderFunctionLoose(context, n)) != null) return;
-        // Find the parent `.map()` callback function, if it exists
-        const mapCallback = AST.findParentNode(jsxElement, isArrayMethodCallback);
-        // Check static keys on elements that are not in a map context
-        if (mapCallback == null || AST.findParentNode(jsxElement, AST.isFunction) !== mapCallback) {
-          constantKeys.add(node);
-          return;
-        }
-        // If the `.map()` callback is not in the same scope, exit
-        if (context.sourceCode.getScope(mapCallback) !== context.sourceCode.getScope(jsxElement)) return;
-        // Find the nearest parent that is either the map callback or a JSX element with a `key` prop
-        const keyedElementOrElse = AST.findParentNode(
-          jsxElement,
-          (n) => {
-            // Stop searching if we reach the map callback
-            if (n === mapCallback) return true;
-            // Check if the node is a JSX element with a `key` prop
-            return AST.isJSXElement(n) && getJsxAttribute(context, n)("key") != null;
-          },
-        );
-        // If the search stopped at the map callback, it means no parent element had a key
-        // In this case, the current key is necessary, so we exit
-        if (keyedElementOrElse == null || keyedElementOrElse === mapCallback) return;
-        // Otherwise, a parent element with a `key` was found, so the current `key` is unnecessary
-        context.report({
-          messageId: "noUnnecessaryKey",
-          node,
-          data: { reason: "A parent element already has a `key` prop in the same list rendering context." },
-        });
-      },
-      "Program:exit"(node) {
-        const components = ctx.getAllComponents(node);
-        for (const key of constantKeys) {
-          // Check if the keyed element is inside dynamic structures
-          const isInDynamicStructure = AST.findParentNode(key, (n) =>
-            AST.isConditional(n)
-            || AST.isControlFlow(n)
-            || findEnclosingAssignmentTarget(n) != null
-            || components.some((comp) => comp.node === n && comp.rets.length > 1)) != null;
-          // We cant be sure the key is unnecessary
-          if (isInDynamicStructure) continue;
-          context.report({
-            messageId: "noUnnecessaryKey",
-            node: key,
-            data: { reason: "The `key` prop is not needed outside of dynamic rendering contexts." },
-          });
-        }
-      },
+  return {
+    JSXAttribute(node: TSESTree.JSXAttribute) {
+      // Check if the attribute is a `key` prop
+      if (node.name.name !== "key") return;
+      const jsxElement = node.parent.parent;
+      // Always allow `<React.Fragment key={...}>` to avoid false positives
+      if (isJsxFragmentElement(context, jsxElement, jsxConfig)) return;
+      // If there is a spread attribute, it's not safe to report an unnecessary key
+      if (jsxElement.openingElement.attributes.some((attr) => attr.type === T.JSXSpreadAttribute)) return;
+      // If inside a render function, it's not safe to report an unnecessary key
+      if (AST.findParentNode(jsxElement, (n) => isRenderFunctionLoose(context, n)) != null) return;
+      // Find the parent `.map()` callback function, if it exists
+      const mapCallback = AST.findParentNode(jsxElement, isArrayMethodCallback);
+      // If the element is not in a map context, it's not safe to report an unnecessary key, @see https://github.com/Rel1cx/eslint-react/issues/1436
+      if (mapCallback == null || AST.findParentNode(jsxElement, AST.isFunction) !== mapCallback) return;
+      // If the `.map()` callback is not in the same scope, exit
+      if (context.sourceCode.getScope(mapCallback) !== context.sourceCode.getScope(jsxElement)) return;
+      // Find the nearest parent that is either the map callback or a JSX element with a `key` prop
+      const keyedElementOrElse = AST.findParentNode(
+        jsxElement,
+        (n) => {
+          // Stop searching if we reach the map callback
+          if (n === mapCallback) return true;
+          // Check if the node is a JSX element with a `key` prop
+          return AST.isJSXElement(n) && getJsxAttribute(context, n)("key") != null;
+        },
+      );
+      // If the search stopped at the map callback, it means no parent element had a key
+      // In this case, the current key is necessary, so we exit
+      if (keyedElementOrElse == null || keyedElementOrElse === mapCallback) return;
+      // Otherwise, a parent element with a `key` was found, so the current `key` is unnecessary
+      context.report({
+        messageId: "noUnnecessaryKey",
+        node,
+        data: { reason: "A parent element already has a `key` prop in the same list rendering context." },
+      });
     },
-  );
+  };
 }
 
 export function getArrayMethodCallbackPosition(methodName: string) {