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 <lrn@google.com>
Reviewed-by: Slava Egorov <vegorov@google.com>

sdk/lib/_internal/vm_shared/lib/compact_hash.dart

@@ -629,8 +629,8 @@ mixin _LinkedHashMapMixin<K, V> on _HashBase, _EqualsAndHashCode {
     }
   }
 
-  Iterable<K> get keys => _CompactIterable<K>(this, _data, _usedData, -2, 2);
-  Iterable<V> get values => _CompactIterable<V>(this, _data, _usedData, -1, 2);
+  Iterable<K> get keys => _CompactIterable<K>(this, -2, 2);
+  Iterable<V> get values => _CompactIterable<V>(this, -1, 2);
 }
 
 base class _CompactLinkedIdentityHashMap<K, V> extends _HashFieldBase
@@ -675,17 +675,13 @@ base class _CompactLinkedCustomHashMap<K, V> extends _HashFieldBase
 // and checks for concurrent modification.
 class _CompactIterable<E> extends Iterable<E> {
   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<E> get iterator =>
-      _CompactIterator<E>(_table, _data, _len, _offset, _step);
+  Iterator<E> get iterator => _CompactIterator<E>(
+      _table, _table._data, _table._usedData, _offset, _step);
 
   int get length => _table.length;
   bool get isEmpty => length == 0;
@@ -928,6 +924,7 @@ mixin _LinkedHashSetMixin<E> on _HashBase, _EqualsAndHashCode {
       i = _HashBase._nextProbe(i, sizeMask);
       pair = _index[i];
     }
+
     return false;
   }
 

tests/corelib/map_test.dart

@@ -140,6 +140,8 @@ void main() {
       isValidKey: (a) => a is int));
 
   testFrom();
+
+  testLazyKeysValueEntries();
 }
 
 void test<K, V>(Map<K, V> 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<K extends Comparable<Object>>(
+      String keyTypeName, K Function(int) toKey) {
+    void testWithMap(String mapType, Map<K, int> 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<K, int>());
+    testWithMap("HashMap", HashMap<K, int>());
+    testWithMap("LinkedHashMap", LinkedHashMap<K, int>());
+    testWithMap("LinkedHashMap-literal", <K, int>{});
+  }
+
+  testWithKeyType<int>("int", (int x) => x);
+  testWithKeyType<String>("String", (int x) => "$x");
+  testWithKeyType<Key>("Key", Key.new);
+}
+
+class Key implements Comparable<Key> {
+  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);
+}