From d8fa6fe25522f45b7070278ad152abe4724cbdf2 Mon Sep 17 00:00:00 2001 From: "Lasse R.H. Nielsen" Date: Tue, 4 Jul 2023 11:00:50 +0000 Subject: [PATCH] Fix bug in VM `_CompactHashIterable`. The iterable is used for the `keys` and `values` of `LinkedHashMap`. The original code remembered the internal data list and used-count when the iterable was created, and if the iterable was modified between creating and iterating, the iterated values would not match the map. The solution is to not cache those values, and read them from the hash table when creting the `Iterator` instead. Fixes #48282 Tested: Added regression test to corelib/map_test Bug: https://dartbug.com/48282 Change-Id: I79310615e7090556e6f45b0d7f297755951ef046 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/312263 Commit-Queue: Lasse Nielsen Reviewed-by: Slava Egorov --- .../_internal/vm_shared/lib/compact_hash.dart | 15 +-- tests/corelib/map_test.dart | 123 ++++++++++++++++++ 2 files changed, 129 insertions(+), 9 deletions(-) diff --git a/sdk/lib/_internal/vm_shared/lib/compact_hash.dart b/sdk/lib/_internal/vm_shared/lib/compact_hash.dart index 4d30021b0d4..705915a9c47 100644 --- a/sdk/lib/_internal/vm_shared/lib/compact_hash.dart +++ b/sdk/lib/_internal/vm_shared/lib/compact_hash.dart @@ -629,8 +629,8 @@ mixin _LinkedHashMapMixin on _HashBase, _EqualsAndHashCode { } } - Iterable get keys => _CompactIterable(this, _data, _usedData, -2, 2); - Iterable get values => _CompactIterable(this, _data, _usedData, -1, 2); + Iterable get keys => _CompactIterable(this, -2, 2); + Iterable get values => _CompactIterable(this, -1, 2); } base class _CompactLinkedIdentityHashMap extends _HashFieldBase @@ -675,17 +675,13 @@ base class _CompactLinkedCustomHashMap extends _HashFieldBase // and checks for concurrent modification. class _CompactIterable extends Iterable { final _HashBase _table; - // dart:core#_List (sdk/lib/_internal/vm/lib/array.dart). - final List _data; - final int _len; final int _offset; final int _step; - _CompactIterable( - this._table, this._data, this._len, this._offset, this._step); + _CompactIterable(this._table, this._offset, this._step); - Iterator get iterator => - _CompactIterator(_table, _data, _len, _offset, _step); + Iterator get iterator => _CompactIterator( + _table, _table._data, _table._usedData, _offset, _step); int get length => _table.length; bool get isEmpty => length == 0; @@ -928,6 +924,7 @@ mixin _LinkedHashSetMixin on _HashBase, _EqualsAndHashCode { i = _HashBase._nextProbe(i, sizeMask); pair = _index[i]; } + return false; } diff --git a/tests/corelib/map_test.dart b/tests/corelib/map_test.dart index a5f749f9715..cedf51396cc 100644 --- a/tests/corelib/map_test.dart +++ b/tests/corelib/map_test.dart @@ -140,6 +140,8 @@ void main() { isValidKey: (a) => a is int)); testFrom(); + + testLazyKeysValueEntries(); } void test(Map map) { @@ -1048,3 +1050,124 @@ void testUnmodifiableMaps() { checkUnmodifiable(UnmodifiableMapView({1: 1})); checkUnmodifiable(const MapView({1: 1})); } + +void testLazyKeysValueEntries() { + // Regression test for https://dartbug.com/48282 + // + // Checks that the keys, values and entries iterables are lazy iterables + // backed by the map. + // + // Creates a fresh map, then fills in entries, and then removes them again. + // At each step, check that the `keys`, `values` and `entries` iterables + // contain the expected elements. (Not checking ordering, since HashMap + // doesn't guarantee one.) + const mapSize = 129; + void testWithKeyType>( + String keyTypeName, K Function(int) toKey) { + void testWithMap(String mapType, Map map) { + // This test does a lot of linear work per element, so too large + // mapSize makes it slow. + // It prints a diagnostic at the end if a test is particularly slow. + var sw = Stopwatch()..start(); + String testName = "$mapType<$keyTypeName, int>"; + var keys = map.keys; + var values = map.values; + var entries = map.entries; + Expect.equals(0, map.length, testName); + Expect.equals(0, keys.length, testName); + Expect.equals(0, values.length, testName); + Expect.equals(0, entries.length, testName); + Expect.isTrue(keys.toList().isEmpty, testName); + Expect.isTrue(values.toList().isEmpty, testName); + Expect.isTrue(entries.toList().isEmpty, testName); + + for (var i = 0; i < mapSize; i++) { + map[toKey(i)] = i; + Expect.equals(i + 1, map.length, testName); + Expect.equals(i + 1, keys.length, testName); + Expect.equals(i + 1, values.length, testName); + Expect.equals(i + 1, entries.length, testName); + Expect.listEquals( + [for (var j = 0; j <= i; j++) toKey(j)]..sort(), + keys.map((x) => x).toList()..sort(), + testName, + ); + Expect.listEquals( + [for (var j = 0; j <= i; j++) j], + values.map((x) => x).toList()..sort(), + testName, + ); + { + // No `operator==` on `MapEntry`. + var currentEntries = entries.map((x) => x).toList() + ..sort((e1, e2) => e1.value.compareTo(e2.value)); + for (var j = 0; j <= i; j++) { + var currentEntry = currentEntries[j]; + Expect.equals(toKey(j), currentEntry.key, testName); + Expect.equals(j, currentEntry.value, testName); + } + } + Expect.equals(map.keys.last, keys.last); + Expect.equals(map.values.last, values.last); + Expect.equals(map.keys.last, entries.last.key); + Expect.equals(map.values.last, entries.last.value); + } + for (var i = 0; i < mapSize; i++) { + Expect.equals(map.keys.first, keys.first); + Expect.equals(map.values.first, values.first); + Expect.equals(map.keys.first, entries.first.key); + Expect.equals(map.values.first, entries.first.value); + var removed = map.remove(toKey(i)); + Expect.equals(i, removed, testName); + Expect.equals(mapSize - 1 - i, map.length, testName); + Expect.equals(mapSize - 1 - i, keys.length, testName); + Expect.equals(mapSize - 1 - i, values.length, testName); + Expect.equals(mapSize - 1 - i, entries.length, testName); + Expect.listEquals( + [for (var j = i + 1; j < mapSize; j++) toKey(j)]..sort(), + keys.map((x) => x).toList()..sort(), + testName, + ); + Expect.listEquals( + [for (var j = i + 1; j < mapSize; j++) j], + values.map((x) => x).toList()..sort(), + testName, + ); + { + // No `operator==` on `MapEntry`. + var currentEntries = entries.map((x) => x).toList() + ..sort((e1, e2) => e1.value.compareTo(e2.value)); + for (var j = i + 1; j < mapSize; j++) { + var currentEntry = currentEntries[j - i - 1]; + Expect.equals(toKey(j), currentEntry.key, testName); + Expect.equals(j, currentEntry.value, testName); + } + } + } + Expect.isTrue(keys.isEmpty, testName); + Expect.isTrue(values.isEmpty, testName); + Expect.isTrue(entries.isEmpty, testName); + var elapsed = sw.elapsedMilliseconds; + if (elapsed > 200) { + print("$testName: $elapsed ms"); + } + } + + testWithMap("SplayTreeMap", SplayTreeMap()); + testWithMap("HashMap", HashMap()); + testWithMap("LinkedHashMap", LinkedHashMap()); + testWithMap("LinkedHashMap-literal", {}); + } + + testWithKeyType("int", (int x) => x); + testWithKeyType("String", (int x) => "$x"); + testWithKeyType("Key", Key.new); +} + +class Key implements Comparable { + final int id; + Key(this.id); + int get hashCode => id.hashCode ^ 1023; + bool operator ==(Object other) => other is Key && id == other.id; + int compareTo(Key other) => id.compareTo(other.id); +}