fix(change_detector): fix NaN move detection in collections

Closes #1136

Closes #1149

Author: vicb

lib/change_detection/dirty_checking_change_detector.dart

@@ -514,19 +514,10 @@ class DirtyCheckingRecord<H> implements Record<H>, WatchRecord<H> {
     }
 
     var last = currentValue;
-    if (!identical(last, current)) {
-      if (last is String && current is String &&
-          last == current) {
-        // This is false change in strings we need to recover, and pretend it
-        // is the same. We save the value so that next time identity will pass
-        currentValue = current;
-      } else if (last is num && last.isNaN && current is num && current.isNaN) {
-        // we need this for the compiled JavaScript since in JS NaN !== NaN.
-      } else {
-        previousValue = last;
-        currentValue = current;
-        return true;
-      }
+    if (!_looseIdentical(last, current)) {
+      previousValue = currentValue;
+      currentValue = current;
+      return true;
     }
     return false;
   }
@@ -620,14 +611,10 @@ class _MapChangeRecord<K, V> implements MapChangeRecord<K, V> {
       var newSeqRecord;
       if (oldSeqRecord != null && key == oldSeqRecord.key) {
         newSeqRecord = oldSeqRecord;
-        if (!identical(value, oldSeqRecord._currentValue)) {
+        if (!_looseIdentical(value, oldSeqRecord._currentValue)) {
           var prev = oldSeqRecord._previousValue = oldSeqRecord._currentValue;
           oldSeqRecord._currentValue = value;
-          if (!((value is String && prev is String && value == prev) ||
-                (value is num && value.isNaN && prev is num && prev.isNaN))) {
-            // Check string by value rather than reference
-            _addToChanges(oldSeqRecord);
-          }
+          _addToChanges(oldSeqRecord);
         }
       } else {
         seqChanged = true;
@@ -955,7 +942,7 @@ class _CollectionChangeRecord<V> implements CollectionChangeRecord<V> {
       _length = list.length;
       for (int index = 0; index < _length; index++) {
         var item = list[index];
-        if (record == null || !identical(item, record.item)) {
+        if (record == null || !_looseIdentical(record.item, item)) {
           record = mismatch(record, item, index);
           maybeDirty = true;
         } else if (maybeDirty) {
@@ -967,7 +954,7 @@ class _CollectionChangeRecord<V> implements CollectionChangeRecord<V> {
     } else {
       int index = 0;
       for (var item in collection) {
-        if (record == null || !identical(item, record.item)) {
+        if (record == null || !_looseIdentical(record.item, item)) {
           record = mismatch(record, item, index);
           maybeDirty = true;
         } else if (maybeDirty) {
@@ -1037,19 +1024,6 @@ class _CollectionChangeRecord<V> implements CollectionChangeRecord<V> {
    * - [index] is the position of the item in the collection
    */
   ItemRecord<V> mismatch(ItemRecord<V> record, item, int index) {
-    if (record != null) {
-      if (item is String && record.item is String && record.item == item) {
-        // this is false change in strings we need to recover, and pretend it is
-        // the same. We save the value so that next time identity can pass
-        return record..item = item;
-      }
-
-      if (item is num && (item as num).isNaN && record.item is num && (record.item as num).isNaN) {
-        // we need this for JavaScript since in JS NaN !== NaN.
-        return record;
-      }
-    }
-
     // The previous record after which we will append the current one.
     ItemRecord<V> previousRecord;
 
@@ -1340,7 +1314,8 @@ class _DuplicateItemRecordList {
       record._prevDup = null;
     } else {
       // adding a duplicate [ItemRecord] to the list
-      assert(record.item == _head.item);
+      assert(record.item ==  _head.item ||
+             record.item is num && record.item.isNaN && _head.item is num && _head.item.isNaN);
       if (insertBefore == null) {
         _tail._nextDup = record;
         record._prevDup = _tail;
@@ -1367,10 +1342,10 @@ class _DuplicateItemRecordList {
     ItemRecord record;
     for (record = _head; record != null; record = record._nextDup) {
       if ((afterIndex == null || afterIndex < record.currentIndex) &&
-          identical(record.item, item)) {
-        return record;
+           _looseIdentical(record.item, item)) {
+          return record;
+        }
       }
-    }
     return null;
   }
 
@@ -1411,10 +1386,11 @@ class _DuplicateItemRecordList {
  * The list of duplicates is implemented by [_DuplicateItemRecordList].
  */
 class DuplicateMap {
+  static final nanKey = const Object();
   final map = new HashMap<dynamic, _DuplicateItemRecordList>();
 
   void put(ItemRecord record, [ItemRecord insertBefore = null]) {
-    var key = record.item;
+    var key = _getKey(record.item);
     _DuplicateItemRecordList duplicates = map[key];
     if (duplicates == null) {
       duplicates = map[key] = new _DuplicateItemRecordList();
@@ -1429,9 +1405,10 @@ class DuplicateMap {
    * Use case: `[a, b, c, a, a]` if we are at index `3` which is the second `a` then asking if we
    * have any more `a`s needs to return the last `a` not the first or second.
    */
-  ItemRecord get(key, [int afterIndex]) {
+  ItemRecord get(value, [int afterIndex]) {
+    var key = _getKey(value);
     _DuplicateItemRecordList recordList = map[key];
-    return recordList == null ? null : recordList.get(key, afterIndex);
+    return recordList == null ? null : recordList.get(value, afterIndex);
   }
 
   /**
@@ -1440,10 +1417,11 @@ class DuplicateMap {
    * The list of duplicates also is removed from the map if it gets empty.
    */
   ItemRecord remove(ItemRecord record) {
-    assert(map.containsKey(record.item));
-    _DuplicateItemRecordList recordList = map[record.item];
+    var key = _getKey(record.item);
+    assert(map.containsKey(key));
+    _DuplicateItemRecordList recordList = map[key];
     // Remove the list of duplicates when it gets empty
-    if (recordList.remove(record)) map.remove(record.item);
+    if (recordList.remove(record)) map.remove(key);
     return record;
   }
 
@@ -1453,5 +1431,32 @@ class DuplicateMap {
     map.clear();
   }
 
+  /// Required to handle num.NAN as a Map value
+  dynamic _getKey(value) => value is num && value.isNaN ? nanKey : value;
+
   String toString() => "DuplicateMap($map)";
 }
+
+/**
+ * Returns whether the [dst] and [src] are loosely identical:
+ * * true when the value are identical,
+ * * true when both values are equal strings,
+ * * true when both values are NaN
+ *
+ * If both values are equal string, src is assigned to dst.
+ */
+bool _looseIdentical(dst, src) {
+  if (identical(dst, src)) return true;
+
+  if (dst is String && src is String && dst == src) {
+    // this is false change in strings we need to recover, and pretend it is the same. We save the
+    // value so that next time identity can pass
+    dst = src;
+    return true;
+  }
+
+  //  we need this for JavaScript since in JS NaN !== NaN.
+  if (dst is num && (dst as num).isNaN && src is num && (src as num).isNaN) return true;
+
+  return false;
+}

test/change_detection/dirty_checking_change_detector_spec.dart

@@ -390,6 +390,21 @@ void testWithGetterFactory(FieldGetterFactory getterFactory) {
         expect(detector.collectChanges().moveNext()).toEqual(false);
       });
 
+      it('should detect [NaN] moves', () {
+        var list = [double.NAN, double.NAN];
+        detector..watch(list, null, null)..collectChanges();
+
+        list..clear()..addAll(['foo', double.NAN, double.NAN]);
+        var iterator = detector.collectChanges();
+        expect(iterator.moveNext()).toEqual(true);
+        expect(iterator.current.currentValue, toEqualCollectionRecord(
+            collection: ['foo[null -> 0]', 'NaN[0 -> 1]', 'NaN[1 -> 2]'],
+            previous: ['NaN[0 -> 1]', 'NaN[1 -> 2]'],
+            additions: ['foo[null -> 0]'],
+            moves: ['NaN[0 -> 1]', 'NaN[1 -> 2]'],
+            removals: []));
+      });
+
       it('should remove and add same item', () {
         var list = ['a', 'b', 'c'];
         var record = detector.watch(list, null, 'handler');