Mark child for force warning if assigned implicit key

When React.Children assigns an implicit key we should still warn when the
original element should've had a key since if was in an array.

This marks such elements as having failed validation even if they have a
key so that the error can later be logged in the renderer.

Author: sebmarkbage

packages/react-client/src/ReactFlightClient.js

@@ -610,13 +610,13 @@ function createElement(
     // Unfortunately, _store is enumerable in jest matchers so for equality to
     // work, I need to keep it or make _store non-enumerable in the other file.
     element._store = ({}: {
-      validated?: boolean,
+      validated?: number,
     });
     Object.defineProperty(element._store, 'validated', {
       configurable: false,
       enumerable: false,
       writable: true,
-      value: true, // This element has already been validated on the server.
+      value: 1, // This element has already been validated on the server.
     });
     // debugInfo contains Server Component debug information.
     Object.defineProperty(element, '_debugInfo', {

packages/react-reconciler/src/ReactChildFiber.js

@@ -114,7 +114,11 @@ if (__DEV__) {
     if (child === null || typeof child !== 'object') {
       return;
     }
-    if (!child._store || child._store.validated || child.key != null) {
+    if (
+      !child._store ||
+      ((child._store.validated || child.key != null) &&
+        child._store.validated !== 2)
+    ) {
       return;
     }
 
@@ -126,7 +130,7 @@ if (__DEV__) {
     }
 
     // $FlowFixMe[cannot-write] unable to narrow type from mixed to writable object
-    child._store.validated = true;
+    child._store.validated = 1;
 
     const componentName = getComponentNameFromFiber(returnFiber);
 
@@ -171,6 +175,9 @@ if (__DEV__) {
     }
 
     // We create a fake Fiber for the child to log the stack trace from.
+    // TODO: Refactor the warnForMissingKey calls to happen after fiber creation
+    // so that we can get access to the fiber that will eventually be created.
+    // That way the log can show up associated with the right instance in DevTools.
     const fiber = createFiberFromElement((child: any), returnFiber.mode, 0);
     fiber.return = returnFiber;
 

packages/react/src/ReactChildren.js

@@ -207,24 +207,38 @@ function mapIntoArray(
           // The `if` statement here prevents auto-disabling of the safe
           // coercion ESLint rule, so we must manually disable it below.
           // $FlowFixMe[incompatible-type] Flow incorrectly thinks React.Portal doesn't have a key
-          if (mappedChild.key && (!child || child.key !== mappedChild.key)) {
-            checkKeyStringCoercion(mappedChild.key);
+          if (mappedChild.key != null) {
+            if (!child || child.key !== mappedChild.key) {
+              checkKeyStringCoercion(mappedChild.key);
+            }
           }
         }
-        mappedChild = cloneAndReplaceKey(
+        const newChild = cloneAndReplaceKey(
           mappedChild,
           // Keep both the (mapped) and old keys if they differ, just as
           // traverseAllChildren used to do for objects as children
           escapedPrefix +
             // $FlowFixMe[incompatible-type] Flow incorrectly thinks React.Portal doesn't have a key
-            (mappedChild.key && (!child || child.key !== mappedChild.key)
+            (mappedChild.key != null &&
+            (!child || child.key !== mappedChild.key)
               ? escapeUserProvidedKey(
                   // $FlowFixMe[unsafe-addition]
                   '' + mappedChild.key, // eslint-disable-line react-internal/safe-string-coercion
                 ) + '/'
               : '') +
             childKey,
         );
+        if (__DEV__) {
+          if (nameSoFar !== '' && mappedChild.key == null) {
+            // We need to validate that this child should have had a key before assigning it one.
+            if (!newChild._store.validated) {
+              // We mark this child as having failed validation but we let the actual renderer
+              // print the warning later.
+              newChild._store.validated = 2;
+            }
+          }
+        }
+        mappedChild = newChild;
       }
       array.push(mappedChild);
     }

packages/react/src/__tests__/ReactChildren-test.js

@@ -301,7 +301,7 @@ describe('ReactChildren', () => {
     ]);
   });
 
-  it('should be called for each child in an iterable without keys', () => {
+  it('should be called for each child in an iterable without keys', async () => {
     const threeDivIterable = {
       '@@iterator': function () {
         let i = 0;
@@ -323,11 +323,6 @@ describe('ReactChildren', () => {
       return kid;
     });
 
-    let instance;
-    expect(() => (instance = <div>{threeDivIterable}</div>)).toErrorDev(
-      'Warning: Each child in a list should have a unique "key" prop.',
-    );
-
     function assertCalls() {
       expect(callback).toHaveBeenCalledTimes(3);
       expect(callback).toHaveBeenCalledWith(<div />, 0);
@@ -336,7 +331,18 @@ describe('ReactChildren', () => {
       callback.mockClear();
     }
 
+    let instance;
+    expect(() => {
+      instance = <div>{threeDivIterable}</div>;
+    }).toErrorDev(
+      // With the flag on this doesn't warn eagerly but only when rendered
+      gate(flag => flag.enableOwnerStacks)
+        ? []
+        : ['Warning: Each child in a list should have a unique "key" prop.'],
+    );
+
     React.Children.forEach(instance.props.children, callback, context);
+
     assertCalls();
 
     const mappedChildren = React.Children.map(
@@ -350,6 +356,16 @@ describe('ReactChildren', () => {
       <div key=".1" />,
       <div key=".2" />,
     ]);
+
+    const container = document.createElement('div');
+    const root = ReactDOMClient.createRoot(container);
+    await expect(async () => {
+      await act(() => {
+        root.render(instance);
+      });
+    }).toErrorDev(
+      'Warning: Each child in a list should have a unique "key" prop.',
+    );
   });
 
   it('should be called for each child in an iterable with keys', () => {
@@ -953,7 +969,7 @@ describe('ReactChildren', () => {
   });
 
   it('should render React.lazy after suspending', async () => {
-    const lazyElement = React.lazy(async () => ({default: <div />}));
+    const lazyElement = React.lazy(async () => ({default: <div key="hi" />}));
     function Component() {
       return React.Children.map([lazyElement], c =>
         React.cloneElement(c, {children: 'hi'}),
@@ -969,7 +985,7 @@ describe('ReactChildren', () => {
   });
 
   it('should render cached Promises after suspending', async () => {
-    const promise = Promise.resolve(<div />);
+    const promise = Promise.resolve(<div key="hi" />);
     function Component() {
       return React.Children.map([promise], c =>
         React.cloneElement(c, {children: 'hi'}),

packages/react/src/jsx/ReactJSXElement.js

@@ -342,7 +342,7 @@ function ReactElement(
       configurable: false,
       enumerable: false,
       writable: true,
-      value: false,
+      value: 0,
     });
     // debugInfo contains Server Component debug information.
     Object.defineProperty(element, '_debugInfo', {
@@ -1073,7 +1073,7 @@ function validateChildKeys(node, parentType) {
     } else if (isValidElement(node)) {
       // This element was passed in a valid location.
       if (node._store) {
-        node._store.validated = true;
+        node._store.validated = 1;
       }
     } else {
       const iteratorFn = getIteratorFn(node);
@@ -1133,7 +1133,7 @@ function validateExplicitKey(element, parentType) {
     if (!element._store || element._store.validated || element.key != null) {
       return;
     }
-    element._store.validated = true;
+    element._store.validated = 1;
 
     const currentComponentErrorInfo = getCurrentComponentErrorInfo(parentType);
     if (ownerHasKeyUseWarning[currentComponentErrorInfo]) {

packages/shared/ReactElementType.js

@@ -23,7 +23,7 @@ export type ReactElement = {
   _owner: any,
 
   // __DEV__
-  _store: {validated: boolean, ...},
+  _store: {validated: 0 | 1 | 2, ...}, // 0: not validated, 1: validated, 2: force fail
   _debugInfo: null | ReactDebugInfo,
   _debugStack: Error,
   _debugTask: null | ConsoleTask,