fix(dirty-checking): handle simultaneous mutations and ref changes

This applies to both maps and lists/iterables.

Consider the case when one is watching an expression, say, `items`, that
evaluates a list.  Now consider the case when one both mutates that
list and also updates `items` to point to a different list.  In this
case, the reported changes should be between the earlier observed
collection and the newly observed collection.  The previous
implementation is buggy.  It first observes that the original list was
mutated and then observes that the reference to the list was updated to
a different list.  The changes it reports are the differences between
the mutated list and the new list which is incorrect.

This commit fixes the bug by noticing the case when both a mutation of
the previously tracked sequence and a ref change occurs and resets the
change record to undo the recording of the mutation.

Author: chirayuk

lib/change_detection/change_detection.dart

@@ -114,6 +114,7 @@ abstract class MapChangeRecord<K, V> {
 
   /// A list of [CollectionKeyValue]s which are in the iteration order. */
   KeyValue<K, V> get mapHead;
+  PreviousKeyValue<K, V> get previousMapHead;
   /// A list of changed items.
   ChangedKeyValue<K, V> get changesHead;
   /// A list of new added items.
@@ -145,6 +146,10 @@ abstract class KeyValue<K, V> extends MapKeyValue<K, V> {
   KeyValue<K, V> get nextKeyValue;
 }
 
+abstract class PreviousKeyValue<K, V> extends MapKeyValue<K, V> {
+  PreviousKeyValue<K, V> get previousNextKeyValue;
+}
+
 abstract class AddedKeyValue<K, V> extends MapKeyValue<K, V> {
   AddedKeyValue<K, V> get nextAddedKeyValue;
 }
@@ -172,6 +177,7 @@ abstract class CollectionChangeRecord<V> {
 
   /** A list of [CollectionItem]s which are in the iteration order. */
   CollectionItem<V> get collectionHead;
+  PreviousCollectionItem<V> get previousCollectionHead;
   /** A list of new [AddedItem]s. */
   AddedItem<V> get additionsHead;
   /** A list of [MovedItem]s. */
@@ -211,6 +217,10 @@ abstract class CollectionItem<V> extends CollectionChangeItem<V> {
  * A linked list of new items added to the collection. These items are always in
  * the iteration order of the collection.
  */
+abstract class PreviousCollectionItem<V> extends CollectionChangeItem<V> {
+  PreviousCollectionItem<V> get previousNextItem;
+}
+
 abstract class AddedItem<V> extends CollectionChangeItem<V> {
   AddedItem<V> get nextAddedItem;
 }

lib/change_detection/dirty_checking_change_detector.dart

@@ -426,16 +426,29 @@ class DirtyCheckingRecord<H> implements Record<H>, WatchRecord<H> {
       _getter = null;
       if (obj is Map) {
         if (_mode != _MODE_MAP_) {
-          // Last one was collection as well, don't reset state.
           _mode =  _MODE_MAP_;
           currentValue = new _MapChangeRecord();
         }
+        if (currentValue.isDirty) {
+          // We're dirty because the mapping we tracked by reference mutated.
+          // In addition, our reference has now changed.  We should compare the
+          // previous reported value of that mapping with the one from the
+          // new reference.
+          currentValue._revertToPreviousState();
+        }
+
       } else if (obj is Iterable) {
         if (_mode != _MODE_ITERABLE_) {
-          // Last one was collection as well, don't reset state.
-          _mode =  _MODE_ITERABLE_;
+          _mode = _MODE_ITERABLE_;
           currentValue = new _CollectionChangeRecord();
         }
+        if (currentValue.isDirty) {
+          // We're dirty because the collection we tracked by reference mutated.
+          // In addition, our reference has now changed.  We should compare the
+          // previous reported value of that collection with the one from the
+          // new reference.
+          currentValue._revertToPreviousState();
+        }
       } else {
         _mode = _MODE_IDENTITY_;
       }
@@ -507,12 +520,14 @@ class _MapChangeRecord<K, V> implements MapChangeRecord<K, V> {
   final Map<dynamic, KeyValueRecord> _records = new Map<dynamic, KeyValueRecord>();
   Map _map;
   KeyValueRecord _mapHead;
+  KeyValueRecord _previousMapHead;
   KeyValueRecord _changesHead, _changesTail;
   KeyValueRecord _additionsHead, _additionsTail;
   KeyValueRecord _removalsHead, _removalsTail;
 
   Map get map => _map;
   KeyValue<K, V> get mapHead => _mapHead;
+  PreviousKeyValue<K, V> get previousMapHead => _previousMapHead;
   ChangedKeyValue<K, V> get changesHead => _changesHead;
   AddedKeyValue<K, V> get additionsHead => _additionsHead;
   RemovedKeyValue<K, V> get removalsHead => _removalsHead;
@@ -521,6 +536,24 @@ class _MapChangeRecord<K, V> implements MapChangeRecord<K, V> {
                  _changesHead != null ||
                  _removalsHead != null;
 
+  _revertToPreviousState() {
+    if (!isDirty) {
+      return;
+    }
+    KeyValueRecord record, prev;
+    int i = 0;
+    for (record = _mapHead = _previousMapHead;
+         record != null;
+         prev = record, record = record._previousNextKeyValue, ++i) {
+      record._currentValue = record._previousValue;
+      if (prev != null) {
+        prev._nextKeyValue = prev._previousNextKeyValue = record;
+      }
+    }
+    prev._nextKeyValue = null;
+    _undoDeltas();
+  }
+
   void forEachChange(void f(ChangedKeyValue<K, V> change)) {
     KeyValueRecord record = _changesHead;
     while (record != null) {
@@ -602,6 +635,18 @@ class _MapChangeRecord<K, V> implements MapChangeRecord<K, V> {
   }
 
   void _reset() {
+    if (isDirty) {
+      // Record the state of the mapping for a possible _revertToPreviousState()
+      for (KeyValueRecord record = _previousMapHead = _mapHead;
+           record != null;
+           record = record._nextKeyValue) {
+        record._previousNextKeyValue = record._nextKeyValue;
+      }
+      _undoDeltas();
+    }
+  }
+
+  void _undoDeltas() {
     var record = _changesHead;
     while (record != null) {
       record._previousValue = record._currentValue;
@@ -748,14 +793,42 @@ class _MapChangeRecord<K, V> implements MapChangeRecord<K, V> {
       _changesTail = record;
     }
   }
+
+  String toString() {
+    List itemsList = [], previousList = [], changesList = [], additionsList = [], removalsList = [];
+    KeyValueRecord record;
+    for (record = _mapHead; record != null; record = record._nextKeyValue) {
+      itemsList.add("$record");
+    }
+    for (record = _previousMapHead; record != null; record = record._previousNextKeyValue) {
+      previousList.add("$record");
+    }
+    for (record = _changesHead; record != null; record = record._nextChangedKeyValue) {
+      changesList.add("$record");
+    }
+    for (record = _additionsHead; record != null; record = record._nextAddedKeyValue) {
+      additionsList.add("$record");
+    }
+    for (record = _removalsHead; record != null; record = record._nextRemovedKeyValue) {
+      removalsList.add("$record");
+    }
+    return """
+map: ${itemsList.join(", ")}
+previous: ${previousList.join(", ")}
+changes: ${changesList.join(", ")}
+additions: ${additionsList.join(", ")}
+removals: ${removalsList.join(", ")}
+""";
+  }
 }
 
-class KeyValueRecord<K, V> implements KeyValue<K, V>, AddedKeyValue<K, V>,
-    RemovedKeyValue<K, V>, ChangedKeyValue<K, V> {
+class KeyValueRecord<K, V> implements KeyValue<K, V>, PreviousKeyValue<K, V>,
+      AddedKeyValue<K, V>, RemovedKeyValue<K, V>, ChangedKeyValue<K, V> {
   final K key;
   V _previousValue, _currentValue;
 
   KeyValueRecord<K, V> _nextKeyValue;
+  KeyValueRecord<K, V> _previousNextKeyValue;
   KeyValueRecord<K, V> _nextAddedKeyValue;
   KeyValueRecord<K, V> _nextRemovedKeyValue, _prevRemovedKeyValue;
   KeyValueRecord<K, V> _nextChangedKeyValue;
@@ -765,12 +838,13 @@ class KeyValueRecord<K, V> implements KeyValue<K, V>, AddedKeyValue<K, V>,
   V get previousValue => _previousValue;
   V get currentValue => _currentValue;
   KeyValue<K, V> get nextKeyValue => _nextKeyValue;
+  PreviousKeyValue<K, V> get previousNextKeyValue => _previousNextKeyValue;
   AddedKeyValue<K, V> get nextAddedKeyValue => _nextAddedKeyValue;
   RemovedKeyValue<K, V> get nextRemovedKeyValue => _nextRemovedKeyValue;
   ChangedKeyValue<K, V> get nextChangedKeyValue => _nextChangedKeyValue;
 
   String toString() => _previousValue == _currentValue
-        ? key
+        ? "$key"
         : '$key[$_previousValue -> $_currentValue]';
 }
 
@@ -785,16 +859,40 @@ class _CollectionChangeRecord<V> implements CollectionChangeRecord<V> {
   /** Used to keep track of removed items. */
   DuplicateMap _removedItems = new DuplicateMap();
 
+  ItemRecord<V> _previousCollectionHead;
   ItemRecord<V> _collectionHead, _collectionTail;
   ItemRecord<V> _additionsHead, _additionsTail;
   ItemRecord<V> _movesHead, _movesTail;
   ItemRecord<V> _removalsHead, _removalsTail;
 
+  CollectionChangeItem<V> get previousCollectionHead => _previousCollectionHead;
   CollectionChangeItem<V> get collectionHead => _collectionHead;
   CollectionChangeItem<V> get additionsHead => _additionsHead;
   CollectionChangeItem<V> get movesHead => _movesHead;
   CollectionChangeItem<V> get removalsHead => _removalsHead;
 
+  _revertToPreviousState() {
+    if (!isDirty) {
+      return;
+    }
+    _items.clear();
+    ItemRecord<V> record, prev;
+    int i = 0;
+    for (record = _collectionHead = _previousCollectionHead;
+         record != null;
+         prev = record, record = record._previousNextRec, ++i) {
+      record.currentIndex = record.previousIndex = i;
+      record._prevRec = prev;
+      if (prev != null) {
+        prev._nextRec = prev._previousNextRec = record;
+      }
+      _items.put(record);
+    }
+    prev._nextRec = null;
+    _collectionTail = prev;
+    _undoDeltas();
+  }
+
   void forEachAddition(void f(AddedItem<V> addition)){
     ItemRecord record = _additionsHead;
     while (record != null) {
@@ -824,14 +922,15 @@ class _CollectionChangeRecord<V> implements CollectionChangeRecord<V> {
 
   bool _check(Iterable collection) {
     _reset();
-    ItemRecord record = _collectionHead;
-    bool maybeDirty = false;
     if ((collection is UnmodifiableListView) &&
-    identical(_iterable, collection)) {
+        identical(_iterable, collection)) {
       // Short circuit and assume that the list has not been modified.
       return false;
     }
 
+    ItemRecord record = _collectionHead;
+    bool maybeDirty = false;
+
     if (collection is List) {
       List list = collection;
       _length = list.length;
@@ -873,6 +972,18 @@ class _CollectionChangeRecord<V> implements CollectionChangeRecord<V> {
    * removals).
    */
   void _reset() {
+    if (isDirty) {
+      // Record the state of the collection for a possible _revertToPreviousState()
+      for (ItemRecord record = _previousCollectionHead = _collectionHead;
+           record != null;
+           record = record._nextRec) {
+        record._previousNextRec = record._nextRec;
+      }
+      _undoDeltas();
+    }
+  }
+
+  void _undoDeltas() {
     ItemRecord record;
 
     record = _additionsHead;
@@ -1157,6 +1268,13 @@ class _CollectionChangeRecord<V> implements CollectionChangeRecord<V> {
       record = record._nextRec;
     }
 
+    var previous = [];
+    record = _previousCollectionHead;
+    while (record != null) {
+      previous.add(record);
+      record = record._previousNextRec;
+    }
+
     var additions = [];
     record = _additionsHead;
     while (record != null) {
@@ -1180,24 +1298,28 @@ class _CollectionChangeRecord<V> implements CollectionChangeRecord<V> {
 
     return """
 collection: ${list.join(", ")}
+previous: ${previous.join(", ")}
 additions: ${additions.join(", ")}
 moves: ${moves.join(", ")}
 removals: ${removals.join(", ")}
 """;
   }
 }
 
-class ItemRecord<V> implements CollectionItem<V>, AddedItem<V>, MovedItem<V>,
+class ItemRecord<V> implements PreviousCollectionItem<V>, CollectionItem<V>, AddedItem<V>, MovedItem<V>,
     RemovedItem<V> {
   int previousIndex = null;
   int currentIndex = null;
   V item = _INITIAL_;
 
+
+  ItemRecord<V> _previousNextRec;
   ItemRecord<V> _prevRec, _nextRec;
   ItemRecord<V> _prevDupRec, _nextDupRec;
   ItemRecord<V> _prevRemovedRec, _nextRemovedRec;
   ItemRecord<V> _nextAddedRec, _nextMovedRec;
 
+  PreviousCollectionItem<V> get previousNextItem => _previousNextRec;
   CollectionItem<V> get nextCollectionItem => _nextRec;
   RemovedItem<V> get nextRemovedItem => _nextRemovedRec;
   AddedItem<V> get nextAddedItem => _nextAddedRec;
@@ -1314,7 +1436,11 @@ class DuplicateMap {
     return record;
   }
 
+  bool get isEmpty => map.isEmpty;
+
   void clear() {
     map.clear();
   }
+
+  String toString() => "$runtimeType($map)";
 }