Implement duplicate key warnings

We keep known keys in a set in development. There is an annoying special case where we know we'll check it again because we break out of the loop early.

One test in the tree hook regresses to the failing state because it checks that the tree hook works without a Set available, but we started using Set in this code. It is not essential and we can clean this up later when we decide how to deal with polyfills.

Author: gaearon

scripts/fiber/tests-failing.txt

@@ -62,6 +62,7 @@ src/renderers/shared/__tests__/ReactPerf-test.js
 
 src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js
 * can be retrieved by ID
+* works
 
 src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js
 * gets recorded during an update

scripts/fiber/tests-passing-except-dev.txt

@@ -111,7 +111,6 @@ src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js
 * reports update counts
 * does not report top-level wrapper as a root
 * registers inlined text nodes
-* works
 
 src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js
 * gets recorded for host roots
@@ -133,10 +132,6 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js
 * gets reported when a child is inserted
 * gets reported when a child is removed
 
-src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js
-* warns for duplicated keys
-* warns for duplicated keys with component stack info
-
 src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js
 * should correctly determine if a component is mounted
 * should correctly determine if a null component is mounted
@@ -148,7 +143,6 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js
 * should disallow nested render calls
 
 src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
-* should warn for duplicated keys with component stack info
 * should warn for using maps as children with owner info
 
 src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js

scripts/fiber/tests-passing.txt

@@ -1279,6 +1279,10 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js
 * gets ignored if the type has not changed
 * gets ignored if new html is equal
 
+src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js
+* warns for duplicated keys
+* warns for duplicated keys with component stack info
+
 src/renderers/shared/shared/__tests__/ReactComponent-test.js
 * should throw when supplying a ref outside of render method
 * should warn when children are mutated during render
@@ -1421,6 +1425,7 @@ src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
 * should replace children with different constructors
 * should NOT replace children with different owners
 * should replace children with different keys
+* should warn for duplicated keys with component stack info
 * should reorder bailed-out children
 
 src/renderers/shared/shared/__tests__/ReactMultiChildReconcile-test.js

src/renderers/shared/fiber/ReactChildFiber.js

@@ -37,6 +37,12 @@ var emptyObject = require('emptyObject');
 var getIteratorFn = require('getIteratorFn');
 var invariant = require('invariant');
 
+if (__DEV__) {
+  var ReactComponentTreeHook = require('ReactComponentTreeHook');
+  var { getStackAddendumByFiber } = ReactComponentTreeHook;
+  var warning = require('warning');
+}
+
 const {
   cloneFiber,
   createFiberFromElement,
@@ -541,6 +547,44 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
     return null;
   }
 
+  function warnOnDuplicateKey(
+    child : mixed,
+    returnFiber : Fiber,
+    knownKeys : Set<string>
+  ) {
+    if (__DEV__) {
+      if (child == null || typeof child !== 'object') {
+        return;
+      }
+      switch (child.$$typeof) {
+        // They have keys.
+        case REACT_ELEMENT_TYPE:
+        case REACT_COROUTINE_TYPE:
+        case REACT_YIELD_TYPE:
+        case REACT_PORTAL_TYPE:
+          const key = child.key;
+          if (typeof key !== 'string') {
+            return;
+          }
+          if (knownKeys.has(key)) {
+            warning(
+              false,
+              'Encountered two children with the same key, ' +
+              '`%s`. Child keys must be unique; when two children share a key, ' +
+              'only the first child will be used.%s',
+              key,
+              getStackAddendumByFiber(returnFiber)
+            );
+          } else {
+            knownKeys.add(key);
+          }
+          return;
+        default:
+          return;
+      }
+    }
+  }
+
   function reconcileChildrenArray(
     returnFiber : Fiber,
     currentFirstChild : ?Fiber,
@@ -569,6 +613,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
     let resultingFirstChild : ?Fiber = null;
     let previousNewFiber : ?Fiber = null;
 
+    let knownKeysInDev = null;
+
     let oldFiber = currentFirstChild;
     let lastPlacedIndex = 0;
     let newIdx = 0;
@@ -582,10 +628,15 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
           nextOldFiber = oldFiber.sibling;
         }
       }
+      const newChild = newChildren[newIdx];
+      if (__DEV__) {
+        knownKeysInDev = knownKeysInDev || new Set();
+        warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
+      }
       const newFiber = updateSlot(
         returnFiber,
         oldFiber,
-        newChildren[newIdx],
+        newChild,
         priority
       );
       if (!newFiber) {
@@ -596,6 +647,18 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
         if (!oldFiber) {
           oldFiber = nextOldFiber;
         }
+        if (__DEV__) {
+          if (
+            knownKeysInDev != null &&
+            newChild != null &&
+            typeof newChild.key === 'string'
+          ) {
+            // Since we break out of this loop without incrementing newIdx,
+            // we will look at this child again in the next loop.
+            // Forget we checked it to avoid a false positive for duplicate key.
+            knownKeysInDev.delete(newChild.key);
+          }
+        }
         break;
       }
       if (shouldTrackSideEffects) {
@@ -630,9 +693,14 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
       // If we don't have any more existing children we can choose a fast path
       // since the rest will all be insertions.
       for (; newIdx < newChildren.length; newIdx++) {
+        const newChild = newChildren[newIdx];
+        if (__DEV__) {
+          knownKeysInDev = knownKeysInDev || new Set();
+          warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
+        }
         const newFiber = createChild(
           returnFiber,
-          newChildren[newIdx],
+          newChild,
           priority
         );
         if (!newFiber) {
@@ -655,11 +723,16 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
 
     // Keep scanning and use the map to restore deleted items as moves.
     for (; newIdx < newChildren.length; newIdx++) {
+      const newChild = newChildren[newIdx];
+      if (__DEV__) {
+        knownKeysInDev = knownKeysInDev || new Set();
+        warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
+      }
       const newFiber = updateFromMap(
         existingChildren,
         returnFiber,
         newIdx,
-        newChildren[newIdx],
+        newChild,
         priority
       );
       if (newFiber) {
@@ -705,6 +778,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
     let resultingFirstChild : ?Fiber = null;
     let previousNewFiber : ?Fiber = null;
 
+    let knownKeysInDev = null;
+
     let oldFiber = currentFirstChild;
     let lastPlacedIndex = 0;
     let newIdx = 0;
@@ -720,10 +795,15 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
           nextOldFiber = oldFiber.sibling;
         }
       }
+      const newChild = step.value;
+      if (__DEV__) {
+        knownKeysInDev = knownKeysInDev || new Set();
+        warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
+      }
       const newFiber = updateSlot(
         returnFiber,
         oldFiber,
-        step.value,
+        newChild,
         priority
       );
       if (!newFiber) {
@@ -734,6 +814,18 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
         if (!oldFiber) {
           oldFiber = nextOldFiber;
         }
+        if (__DEV__) {
+          if (
+            knownKeysInDev != null &&
+            newChild != null &&
+            typeof newChild.key === 'string'
+          ) {
+            // Since we break out of this loop without incrementing newIdx,
+            // we will look at this child again in the next loop.
+            // Forget we checked it to avoid a false positive for duplicate key.
+            knownKeysInDev.delete(newChild.key);
+          }
+        }
         break;
       }
       if (shouldTrackSideEffects) {
@@ -768,9 +860,14 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
       // If we don't have any more existing children we can choose a fast path
       // since the rest will all be insertions.
       for (; !step.done; newIdx++, step = newChildren.next()) {
+        const newChild = step.value;
+        if (__DEV__) {
+          knownKeysInDev = knownKeysInDev || new Set();
+          warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
+        }
         const newFiber = createChild(
           returnFiber,
-          step.value,
+          newChild,
           priority
         );
         if (!newFiber) {
@@ -793,11 +890,16 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
 
     // Keep scanning and use the map to restore deleted items as moves.
     for (; !step.done; newIdx++, step = newChildren.next()) {
+      const newChild = step.value;
+      if (__DEV__) {
+        knownKeysInDev = knownKeysInDev || new Set();
+        warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
+      }
       const newFiber = updateFromMap(
         existingChildren,
         returnFiber,
         newIdx,
-        step.value,
+        newChild,
         priority
       );
       if (newFiber) {

src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js

@@ -70,8 +70,7 @@ describe('ReactChildReconciler', () => {
     ReactTestUtils.renderIntoDocument(<GrandParent />);
 
     expectDev(console.error.calls.count()).toBe(1);
-    expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
-      'Warning: flattenChildren(...): ' +
+    expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toContain(
       'Encountered two children with the same key, `1`. ' +
       'Child keys must be unique; when two children share a key, ' +
       'only the first child will be used.\n' +

src/renderers/shared/shared/__tests__/ReactMultiChild-test.js

@@ -186,8 +186,7 @@ describe('ReactMultiChild', () => {
       );
 
       expectDev(console.error.calls.count()).toBe(1);
-      expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
-        'Warning: flattenChildren(...): ' +
+      expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toContain(
         'Encountered two children with the same key, `1`. ' +
         'Child keys must be unique; when two children share a key, ' +
         'only the first child will be used.\n' +