refactor(core): warn about duplicated keys when using built-in @for (angular#55243)

This commit a new check that warn users about duplicated keys detected given
a tracking expression and a collection iterated over with @for. Duplicated keys
should be avoided as those are more expensive to manage and can result in
incorrect UI display.

PR Close angular#55243

Author: pkozlowski-opensource

goldens/public-api/core/errors.md

@@ -79,6 +79,8 @@ export const enum RuntimeErrorCode {
     // (undocumented)
     INVALID_SKIP_HYDRATION_HOST = -504,
     // (undocumented)
+    LOOP_TRACK_DUPLICATE_KEYS = 955,
+    // (undocumented)
     MISSING_DOCUMENT = 210,
     // (undocumented)
     MISSING_GENERATED_DEF = 906,

packages/core/src/errors.ts

@@ -124,6 +124,9 @@ export const enum RuntimeErrorCode {
   // Output()
   OUTPUT_REF_DESTROYED = 953,
 
+  // Repeater errors
+  LOOP_TRACK_DUPLICATE_KEYS = 955,
+
   // Runtime dependency tracker errors
   RUNTIME_DEPS_INVALID_IMPORTED_TYPE = 1000,
   RUNTIME_DEPS_ORPHAN_COMPONENT = 1001,

packages/core/src/render3/list_reconciliation.ts

@@ -7,8 +7,11 @@
  */
 
 import {TrackByFunction} from '../change_detection';
+import {formatRuntimeError, RuntimeErrorCode} from '../errors';
 import {assertNotSame} from '../util/assert';
 
+import {stringifyForError} from './util/stringify_utils';
+
 /**
  * A type representing the live collection to be reconciled with any new (incoming) collection. This
  * is an adapter class that makes it possible to work with different internal data structures,
@@ -61,6 +64,16 @@ function valuesMatching<V>(
   return 0;
 }
 
+function recordDuplicateKeys(keyToIdx: Map<unknown, Set<number>>, key: unknown, idx: number): void {
+  const idxSoFar = keyToIdx.get(key);
+
+  if (idxSoFar !== undefined) {
+    idxSoFar.add(idx);
+  } else {
+    keyToIdx.set(key, new Set([idx]));
+  }
+}
+
 /**
  * The live collection reconciliation algorithm that perform various in-place operations, so it
  * reflects the content of the new (incoming) collection.
@@ -93,13 +106,20 @@ export function reconcile<T, V>(
   let liveStartIdx = 0;
   let liveEndIdx = liveCollection.length - 1;
 
+  const duplicateKeys = ngDevMode ? new Map<unknown, Set<number>>() : undefined;
+
   if (Array.isArray(newCollection)) {
     let newEndIdx = newCollection.length - 1;
 
     while (liveStartIdx <= liveEndIdx && liveStartIdx <= newEndIdx) {
       // compare from the beginning
       const liveStartValue = liveCollection.at(liveStartIdx);
       const newStartValue = newCollection[liveStartIdx];
+
+      if (ngDevMode) {
+        recordDuplicateKeys(duplicateKeys!, trackByFn(liveStartIdx, newStartValue), liveStartIdx);
+      }
+
       const isStartMatching =
           valuesMatching(liveStartIdx, liveStartValue, liveStartIdx, newStartValue, trackByFn);
       if (isStartMatching !== 0) {
@@ -114,6 +134,11 @@ export function reconcile<T, V>(
       // TODO(perf): do _all_ the matching from the end
       const liveEndValue = liveCollection.at(liveEndIdx);
       const newEndValue = newCollection[newEndIdx];
+
+      if (ngDevMode) {
+        recordDuplicateKeys(duplicateKeys!, trackByFn(newEndIdx, newEndValue), newEndIdx);
+      }
+
       const isEndMatching =
           valuesMatching(liveEndIdx, liveEndValue, newEndIdx, newEndValue, trackByFn);
       if (isEndMatching !== 0) {
@@ -188,6 +213,11 @@ export function reconcile<T, V>(
     while (!newIterationResult.done && liveStartIdx <= liveEndIdx) {
       const liveValue = liveCollection.at(liveStartIdx);
       const newValue = newIterationResult.value;
+
+      if (ngDevMode) {
+        recordDuplicateKeys(duplicateKeys!, trackByFn(liveStartIdx, newValue), liveStartIdx);
+      }
+
       const isStartMatching =
           valuesMatching(liveStartIdx, liveValue, liveStartIdx, newValue, trackByFn);
       if (isStartMatching !== 0) {
@@ -244,6 +274,31 @@ export function reconcile<T, V>(
   detachedItems?.forEach(item => {
     liveCollection.destroy(item);
   });
+
+  // report duplicate keys (dev mode only)
+  if (ngDevMode) {
+    let duplicatedKeysMsg = [];
+    for (const [key, idxSet] of duplicateKeys!) {
+      if (idxSet.size > 1) {
+        const idx = [...idxSet].sort((a, b) => a - b);
+        for (let i = 1; i < idx.length; i++) {
+          duplicatedKeysMsg.push(
+              `key "${stringifyForError(key)}" at index "${idx[i - 1]}" and "${idx[i]}"`);
+        }
+      }
+    }
+
+    if (duplicatedKeysMsg.length > 0) {
+      const message = formatRuntimeError(
+          RuntimeErrorCode.LOOP_TRACK_DUPLICATE_KEYS,
+          'The provided track expression resulted in duplicated keys for a given collection. ' +
+              'Adjust the tracking expression such that it uniquely identifies all the items in the collection. ' +
+              'Duplicated keys were: \n' + duplicatedKeysMsg.join(', \n') + '.');
+
+      // tslint:disable-next-line:no-console
+      console.warn(message);
+    }
+  }
 }
 
 function attachPreviouslyDetached<T, V>(

packages/core/test/acceptance/control_flow_for_spec.ts

@@ -38,6 +38,19 @@ describe('control flow - for', () => {
     expect(fixture.nativeElement.textContent).toBe('3(0)|2(1)|1(2)|');
   });
 
+  it('should loop over iterators that can be iterated over only once', () => {
+    @Component({
+      template: '@for ((item of items.keys()); track $index) {{{item}}|}',
+    })
+    class TestComponent {
+      items = new Map([['a', 1], ['b', 2], ['c', 3]]);
+    }
+
+    const fixture = TestBed.createComponent(TestComponent);
+    fixture.detectChanges();
+    expect(fixture.nativeElement.textContent).toBe('a|b|c|');
+  });
+
   it('should work correctly with trackBy index', () => {
     @Component({
       template: '@for ((item of items); track idx; let idx = $index) {{{item}}({{idx}})|}',
@@ -273,6 +286,95 @@ describe('control flow - for', () => {
       fixture.detectChanges();
       expect(context).toBe(fixture.componentInstance);
     });
+
+    it('should warn about duplicated keys when using arrays', () => {
+      @Component({
+        template: `@for (item of items; track item) {{{item}}}`,
+      })
+      class TestComponent {
+        items = ['a', 'b', 'a', 'c', 'a'];
+      }
+
+      spyOn(console, 'warn');
+
+      const fixture = TestBed.createComponent(TestComponent);
+      fixture.detectChanges();
+      expect(fixture.nativeElement.textContent).toBe('abaca');
+      expect(console.warn).toHaveBeenCalledTimes(1);
+      expect(console.warn)
+          .toHaveBeenCalledWith(jasmine.stringContaining(
+              `NG0955: The provided track expression resulted in duplicated keys for a given collection.`));
+      expect(console.warn)
+          .toHaveBeenCalledWith(jasmine.stringContaining(
+              `Adjust the tracking expression such that it uniquely identifies all the items in the collection. `));
+      expect(console.warn)
+          .toHaveBeenCalledWith(jasmine.stringContaining(`key "a" at index "0" and "2"`));
+      expect(console.warn)
+          .toHaveBeenCalledWith(jasmine.stringContaining(`key "a" at index "2" and "4"`));
+    });
+
+    it('should warn about duplicated keys when using iterables', () => {
+      @Component({
+        template: `@for (item of items.values(); track item) {{{item}}}`,
+      })
+      class TestComponent {
+        items = new Map([[1, 'a'], [2, 'b'], [3, 'a'], [4, 'c'], [5, 'a']]);
+      }
+
+      spyOn(console, 'warn');
+
+      const fixture = TestBed.createComponent(TestComponent);
+      fixture.detectChanges();
+      expect(fixture.nativeElement.textContent).toBe('abaca');
+      expect(console.warn).toHaveBeenCalledTimes(1);
+      expect(console.warn)
+          .toHaveBeenCalledWith(jasmine.stringContaining(
+              `NG0955: The provided track expression resulted in duplicated keys for a given collection.`));
+      expect(console.warn)
+          .toHaveBeenCalledWith(jasmine.stringContaining(
+              `Adjust the tracking expression such that it uniquely identifies all the items in the collection. `));
+      expect(console.warn)
+          .toHaveBeenCalledWith(jasmine.stringContaining(`key "a" at index "0" and "2"`));
+      expect(console.warn)
+          .toHaveBeenCalledWith(jasmine.stringContaining(`key "a" at index "2" and "4"`));
+    });
+
+    it('should warn about duplicate keys when keys are expressed as symbols', () => {
+      const value = Symbol('a');
+
+      @Component({
+        template: `@for (item of items.values(); track item) {}`,
+      })
+      class TestComponent {
+        items = new Map([[1, value], [2, value]]);
+      }
+
+      spyOn(console, 'warn');
+
+      const fixture = TestBed.createComponent(TestComponent);
+      fixture.detectChanges();
+      expect(console.warn)
+          .toHaveBeenCalledWith(jasmine.stringContaining(`Symbol(a)" at index "0" and "1".`));
+    });
+
+    it('should warn about duplicate keys iterating over the new collection only', () => {
+      @Component({
+        template: `@for (item of items; track item) {}`,
+      })
+      class TestComponent {
+        items = [1, 2, 3];
+      }
+
+      spyOn(console, 'warn');
+
+      const fixture = TestBed.createComponent(TestComponent);
+      fixture.detectChanges();
+      expect(console.warn).not.toHaveBeenCalled();
+
+      fixture.componentInstance.items = [4, 5, 6];
+      fixture.detectChanges();
+      expect(console.warn).not.toHaveBeenCalled();
+    });
   });
 
   describe('list diffing and view operations', () => {