Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

fix(change_detector): Fix for detecting NanN moves in collections #1149

Closed
wants to merge 1 commit into from
Closed

fix(change_detector): Fix for detecting NanN moves in collections #1149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 50 additions & 44 deletions lib/change_detection/dirty_checking_change_detector.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -1343,7 +1317,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;
Expand All @@ -1370,10 +1345,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;
}

Expand Down Expand Up @@ -1414,10 +1389,12 @@ 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]) {
map.putIfAbsent(record.item, () => new _DuplicateItemRecordList()).add(record, insertBefore);
var key = _getKey(record.item);
map.putIfAbsent(key, () => new _DuplicateItemRecordList()).add(record, insertBefore);
}

/**
Expand All @@ -1427,9 +1404,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);
}

/**
Expand All @@ -1438,10 +1416,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;
}

Expand All @@ -1451,5 +1430,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;
}
15 changes: 15 additions & 0 deletions test/change_detection/dirty_checking_change_detector_spec.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down