diff --git a/pkg/expect/lib/expect.dart b/pkg/expect/lib/expect.dart index 522ae045622..d0983065013 100644 --- a/pkg/expect/lib/expect.dart +++ b/pkg/expect/lib/expect.dart @@ -434,6 +434,10 @@ class Expect { _fail('Expect.mapEquals(unexpected key: <$key>$msg) fails'); } } + if (expectedKeys.length != actualKeys.length) { + _failNotEqual(expectedKeys.length, actualKeys.length, + "mapEquals", "map.length"); + } } /// Specialized equality test for strings. When the strings don't match, diff --git a/sdk/lib/_internal/js_dev_runtime/private/custom_hash_map.dart b/sdk/lib/_internal/js_dev_runtime/private/custom_hash_map.dart index e162d0da7f3..0ba28813a28 100644 --- a/sdk/lib/_internal/js_dev_runtime/private/custom_hash_map.dart +++ b/sdk/lib/_internal/js_dev_runtime/private/custom_hash_map.dart @@ -69,11 +69,11 @@ base class CustomHashMap extends InternalMap { @notNull bool containsKey(Object? key) { if (key is K) { - var buckets = JS('', '#.get(# & 0x3fffffff)', _keyMap, _hashCode(key)); - if (buckets != null) { + var bucket = JS('', '#.get(# & 0x3fffffff)', _keyMap, _hashCode(key)); + if (bucket != null) { var equals = _equals; - for (int i = 0, n = JS('!', '#.length', buckets); i < n; i++) { - K k = JS('', '#[#]', buckets, i); + for (int i = 0, n = JS('!', '#.length', bucket); i < n; i++) { + K k = JS('', '#[#]', bucket, i); if (equals(k, key)) return true; } } @@ -96,15 +96,20 @@ base class CustomHashMap extends InternalMap { V? operator [](Object? key) { if (key is K) { - var buckets = JS('', '#.get(# & 0x3fffffff)', _keyMap, _hashCode(key)); - if (buckets != null) { + var bucket = JS('', '#.get(# & 0x3fffffff)', _keyMap, _hashCode(key)); + var modifications = _modifications; + if (bucket != null) { var equals = _equals; - for (int i = 0, n = JS('!', '#.length', buckets); i < n; i++) { - K k = JS('', '#[#]', buckets, i); + for (int i = 0, n = JS('!', '#.length', bucket); i < n; i++) { + K k = JS('', '#[#]', bucket, i); if (equals(k, key)) { - V value = JS('', '#.get(#)', _map, k); - // coerce undefined to null. - return JS('!', '# === void 0', value) ? null : value; + if (modifications == _modifications) { + V value = JS('', '#.get(#)', _map, k); + // Coerce undefined to null. + return JS('!', '# === void 0', value) ? null : value; + } + // Calling equals changed the map. + throw ConcurrentModificationError(this); } } } @@ -113,71 +118,106 @@ base class CustomHashMap extends InternalMap { } void operator []=(K key, V value) { - var keyMap = _keyMap; int hash = JS('!', '# & 0x3fffffff', _hashCode(key)); - var buckets = JS('', '#.get(#)', keyMap, hash); - if (buckets == null) { - JS('', '#.set(#, [#])', keyMap, hash, key); - } else { - var equals = _equals; - for (int i = 0, n = JS('!', '#.length', buckets); ;) { - K k = JS('', '#[#]', buckets, i); - if (equals(k, key)) { - key = k; - break; - } - if (++i >= n) { - JS('', '#.push(#)', buckets, key); - break; + _set(key, hash, value); + } + + /// Sets a value, just like `[]=`, after having computed hash code. + /// + /// Used by `[]=` and `putIfAbsent` if `ifAbsent` modifies the map. + void _set(K key, int hash, V value) { + concurrentModification: + { + var bucket = JS('', '#.get(#)', _keyMap, hash); + if (bucket == null) { + JS('', '#.set(#, [#])', _keyMap, hash, key); + } else { + var modifications = _modifications; + var equals = _equals; + for (int i = 0, n = JS('!', '#.length', bucket); ;) { + K k = JS('', '#[#]', bucket, i); + if (equals(k, key)) { + if (modifications != _modifications) break concurrentModification; + key = k; + break; + } + if (++i >= n) { + // Check for modification before adding key to bucket. + if (modifications != _modifications) break concurrentModification; + JS('', '#.push(#)', bucket, key); + break; + } } } + JS('', '#.set(#, #)', _map, key, value); + _modifications = (_modifications + 1) & 0x3fffffff; + return; } - JS('', '#.set(#, #)', _map, key, value); - _modifications = (_modifications + 1) & 0x3fffffff; + // Break to here in case of modification. + throw ConcurrentModificationError(this); } V putIfAbsent(K key, V ifAbsent()) { var keyMap = _keyMap; + var modifications = _modifications; int hash = JS('!', '# & 0x3fffffff', _hashCode(key)); - var buckets = JS('', '#.get(#)', keyMap, hash); - if (buckets == null) { - JS('', '#.set(#, [#])', keyMap, hash, key); - } else { + var bucket = JS('', '#.get(#)', keyMap, hash); + if (bucket != null) { var equals = _equals; - for (int i = 0, n = JS('!', '#.length', buckets); i < n; i++) { - K k = JS('', '#[#]', buckets, i); - if (equals(k, key)) return JS('', '#.get(#)', _map, k); + for (int i = 0, n = JS('!', '#.length', bucket); i < n; i++) { + K k = JS('', '#[#]', bucket, i); + if (equals(k, key)) { + if (modifications == _modifications) { + return JS('', '#.get(#)', _map, k); + } + // Calling `equals` changed the map. + throw ConcurrentModificationError(this); + } } - JS('', '#.push(#)', buckets, key); } V value = ifAbsent(); if (value == null) JS('', '# = null', value); // coerce undefined to null. - JS('', '#.set(#, #)', _map, key, value); - _modifications = (_modifications + 1) & 0x3fffffff; + if (_modifications == modifications) { + if (bucket == null) { + JS('', '#.set(#, [#])', keyMap, hash, key); + } else { + JS('', '#.push(#)', bucket, key); + } + JS('', '#.set(#, #)', _map, key, value); + _modifications = (_modifications + 1) & 0x3fffffff; + } else { + // Start from scratch, an equal key might have been added. + _set(key, hash, value); + } return value; } V? remove(Object? key) { if (key is K) { int hash = JS('!', '# & 0x3fffffff', _hashCode(key)); + var modifications = _modifications; var keyMap = _keyMap; - var buckets = JS('', '#.get(#)', keyMap, hash); - if (buckets == null) return null; // not found + var bucket = JS('', '#.get(#)', keyMap, hash); + if (bucket == null) return null; // not found var equals = _equals; - for (int i = 0, n = JS('!', '#.length', buckets); i < n; i++) { - K k = JS('', '#[#]', buckets, i); + for (int i = 0, n = JS('!', '#.length', bucket); i < n; i++) { + K k = JS('', '#[#]', bucket, i); if (equals(k, key)) { - if (n == 1) { - JS('', '#.delete(#)', keyMap, hash); - } else { - JS('', '#.splice(#, 1)', buckets, i); + if (modifications == _modifications) { + if (n == 1) { + JS('', '#.delete(#)', keyMap, hash); + } else { + JS('', '#.splice(#, 1)', bucket, i); + } + var map = _map; + V value = JS('', '#.get(#)', map, k); + JS('', '#.delete(#)', map, k); + _modifications = (_modifications + 1) & 0x3fffffff; + // Coerce undefined to null. + return JS('!', '# === void 0', value) ? null : value; } - var map = _map; - V value = JS('', '#.get(#)', map, k); - JS('', '#.delete(#)', map, k); - _modifications = (_modifications + 1) & 0x3fffffff; - // coerce undefined to null. - return JS('!', '# === void 0', value) ? null : value; + // Calling equals changed the bucket, can't trust position of `k`. + throw ConcurrentModificationError(this); } } } diff --git a/sdk/lib/_internal/js_dev_runtime/private/linked_hash_map.dart b/sdk/lib/_internal/js_dev_runtime/private/linked_hash_map.dart index 79eb071fa72..324c85c498e 100644 --- a/sdk/lib/_internal/js_dev_runtime/private/linked_hash_map.dart +++ b/sdk/lib/_internal/js_dev_runtime/private/linked_hash_map.dart @@ -146,7 +146,7 @@ base class LinkedMap extends InternalMap { JS('', '#.set(#, #)', _map, key, value); }); if (length != JS('!', '#.size', map)) { - _modifications = (_modifications + 1) & 0x3fffffff; + _modifications = (_modifications + 1) & _smiMask; } } @@ -190,15 +190,41 @@ base class LinkedMap extends InternalMap { int length = JS('', '#.size', map); JS('', '#.set(#, #)', map, key, value); if (length != JS('!', '#.size', map)) { - _modifications = (_modifications + 1) & 0x3fffffff; + _modifications = (_modifications + 1) & _smiMask; } } + V _putIfAbsentKeyMap(K key, V ifAbsent(), Object map) { + // Key has non-trivial equality. + var keyMap = _keyMap; + var hash = JS('!', '# & 0x3fffffff', key.hashCode); + var buckets = JS('', '#.get(#)', keyMap, hash); + if (buckets != null) { + for (int i = 0, n = JS('!', '#.length', buckets); i < n; i++) { + var k = JS('', '#[#]', buckets, i); + if (k == key) return JS('', '#.get(#)', map, k); + } + } + var modifications = _modifications; + V value = ifAbsent(); + if (modifications != _modifications) { + key = putLinkedMapKey(key, keyMap); + } else if (buckets != null) { + JS('', '#.push(#)', buckets, key); + } else { + JS('', '#.set(#, [#])', keyMap, hash, key); + } + if (JS('!', '# === void 0', value)) value = JS('', '#', null); + JS('', '#.set(#, #)', map, key, value); + _modifications = (_modifications + 1) & _smiMask; + return value; + } + V putIfAbsent(K key, V ifAbsent()) { var map = _map; if (key == null) { - key = JS('', 'null'); if (JS('!', '#.has(null)', map)) return JS('', '#.get(null)', map); + key = JS('', 'null'); } else if (JS( '!', '#[#] !== #', @@ -206,28 +232,14 @@ base class LinkedMap extends InternalMap { dart.extensionSymbol('_equals'), dart.identityEquals, )) { - @notNull - K k = key; - var hash = JS('!', '# & 0x3fffffff', k.hashCode); - var buckets = JS('', '#.get(#)', _keyMap, hash); - if (buckets == null) { - JS('', '#.set(#, [#])', _keyMap, hash, key); - } else { - for (int i = 0, n = JS('!', '#.length', buckets); i < n; i++) { - k = JS('', '#[#]', buckets, i); - if (k == key) return JS('', '#.get(#)', map, k); - } - JS('', '#.push(#)', buckets, key); - } + return _putIfAbsentKeyMap(key, ifAbsent, map); } else if (JS('!', '#.has(#)', map, key)) { return JS('', '#.get(#)', map, key); } V value = ifAbsent(); - if (value == null) { - value = JS('', 'null'); - } + if (JS('!', '# === void 0', value)) value = JS('', '#', null); JS('', '#.set(#, #)', map, key, value); - _modifications = (_modifications + 1) & 0x3fffffff; + _modifications = (_modifications + 1) & _smiMask; return value; } @@ -262,7 +274,7 @@ base class LinkedMap extends InternalMap { var map = _map; V value = JS('', '#.get(#)', map, key); if (JS('!', '#.delete(#)', map, key)) { - _modifications = (_modifications + 1) & 0x3fffffff; + _modifications = (_modifications + 1) & _smiMask; } // coerce undefined to null. return JS('!', '# === void 0', value) ? null : value; @@ -273,7 +285,7 @@ base class LinkedMap extends InternalMap { if (JS('!', '#.size', map) > 0) { JS('', '#.clear()', map); JS('', '#.clear()', _keyMap); - _modifications = (_modifications + 1) & 0x3fffffff; + _modifications = (_modifications + 1) & _smiMask; } } } @@ -310,3 +322,5 @@ base class ImmutableMap extends LinkedMap { static Error _unsupported() => UnsupportedError("Cannot modify unmodifiable map"); } + +const _smiMask = 0x3fff_ffff; diff --git a/sdk/lib/collection/hash_map.dart b/sdk/lib/collection/hash_map.dart index 9c5ed8262fc..d266d60db16 100644 --- a/sdk/lib/collection/hash_map.dart +++ b/sdk/lib/collection/hash_map.dart @@ -129,16 +129,20 @@ abstract final class HashMap implements Map { /// If [equals] is provided, it is used to compare the keys in the map with /// new keys. If [equals] is omitted, the key's own [Object.==] is used /// instead. + /// The [equals] function *must not* change the map it's used as an equality + /// for. If it does, the resulting behavior is unspecified. /// /// Similarly, if [hashCode] is provided, it is used to produce a hash value /// for keys in order to place them in the map. If [hashCode] is omitted, /// the key's own [Object.hashCode] is used. + /// The [hashCode] function *must not* change the map it's used as a hash code + /// for. If it does, the resulting behavior is unspecified. /// /// The used `equals` and `hashCode` method should always be consistent, /// so that if `equals(a, b)`, then `hashCode(a) == hashCode(b)`. The hash /// of an object, or what it compares equal to, should not change while the - /// object is a key in the map. If it does change, the result is - /// unpredictable. + /// object is a key in the map. If the hash code or equality of an object does + /// change, the resulting behavior is unspecified. /// /// If you supply one of [equals] and [hashCode], /// you should generally also supply the other. @@ -164,7 +168,7 @@ abstract final class HashMap implements Map { /// /// If neither `equals`, `hashCode`, nor `isValidKey` is provided, /// the default `isValidKey` instead accepts all keys. - /// The default equality and hashcode operations are known to work on all + /// The default equality and hash code operations are known to work on all /// objects. /// /// Likewise, if `equals` is [identical], `hashCode` is [identityHashCode] diff --git a/sdk/lib/collection/linked_hash_map.dart b/sdk/lib/collection/linked_hash_map.dart index 8e421a484d1..d348963540d 100644 --- a/sdk/lib/collection/linked_hash_map.dart +++ b/sdk/lib/collection/linked_hash_map.dart @@ -116,15 +116,20 @@ abstract final class LinkedHashMap implements Map { /// If [equals] is provided, it is used to compare the keys in the table with /// new keys. If [equals] is omitted, the key's own [Object.==] is used /// instead. + /// The [equals] function *must not* change the map it's used as an equality + /// for. If it does, the resulting behavior is unspecified. /// /// Similarly, if [hashCode] is provided, it is used to produce a hash value /// for keys in order to place them in the hash table. If it is omitted, the /// key's own [Object.hashCode] is used. + /// The [hashCode] function *must not* change the map it's used as a hash code + /// for. If it does, the resulting behavior is unspecified. /// /// The used `equals` and `hashCode` methods should always be consistent, /// so that if `equals(a, b)` then `hashCode(a) == hashCode(b)`. The hash /// of an object, or what it compares equal to, should not change while the - /// object is in the table. If it does change, the result is unpredictable. + /// object is in the table. If the hash code or equality of an object does + /// change, the resulting behavior is unspecified. /// /// If you supply one of [equals] or [hashCode], /// you should generally also supply the other. diff --git a/sdk/lib/collection/splay_tree.dart b/sdk/lib/collection/splay_tree.dart index 3146063d121..40378cfb9ca 100644 --- a/sdk/lib/collection/splay_tree.dart +++ b/sdk/lib/collection/splay_tree.dart @@ -24,8 +24,7 @@ class _SplayTreeSetNode extends _SplayTreeNode> { /// A node in a splay tree based map. /// -/// A [_SplayTreeNode] that also contains a value, -/// and which implements [MapEntry]. +/// A [_SplayTreeNode] that also contains a value. class _SplayTreeMapNode extends _SplayTreeNode> { final V value; @@ -536,13 +535,15 @@ final class SplayTreeMap extends _SplayTree> int modificationCount = _modificationCount; int splayCount = _splayCount; V value = ifAbsent(); - if (modificationCount != _modificationCount) { - throw ConcurrentModificationError(this); - } - if (splayCount != _splayCount) { + if (modificationCount != _modificationCount || splayCount != _splayCount) { comp = _splay(key); - // Key is still not there, otherwise _modificationCount would be changed. - assert(comp != 0); + if (comp == 0) { + // Key was added. + _root = _root!._replaceValue(value); + _splayCount += 1; // Tree restructured. + return value; + } + // Key is still not there. } _addNewRoot(_SplayTreeMapNode(key, value), comp); return value; diff --git a/sdk/lib/core/map.dart b/sdk/lib/core/map.dart index 509ead82ce0..86654aaf3df 100644 --- a/sdk/lib/core/map.dart +++ b/sdk/lib/core/map.dart @@ -20,13 +20,17 @@ part of dart.core; /// /// It is generally not allowed to modify the map (add or remove keys) while /// an operation is being performed on the map, for example in functions called -/// during a [forEach] or [putIfAbsent] call. +/// during a [forEach] call. /// Modifying the map while iterating the keys or values /// may also break the iteration. /// /// It is generally not allowed to modify the equality of keys (and thus not /// their hashcode) while they are in the map. Some specialized subtypes may be /// more permissive, in which case they should document this behavior. +/// +/// Key equality must be an equality relation. If the key stored in a map +/// and the key used for lookup do not agree on whether the two are equal, +/// so equality is not *symmetric*, then lookup behavior is unspecified. abstract interface class Map { /// Creates an empty [LinkedHashMap]. /// @@ -302,8 +306,8 @@ abstract interface class Map { /// /// Returns the new value associated with the key. /// - /// If the key is present, invokes [update] with the current value and stores - /// the new value in the map. + /// If the key is present, invokes [update] with the current value + /// and stores the new value in the map. /// /// If the key is not present and [ifAbsent] is provided, calls [ifAbsent] /// and adds the key with the returned value to the map. @@ -348,6 +352,14 @@ abstract interface class Map { /// Returns the value associated to [key], if there is one. /// Otherwise calls [ifAbsent] to get a new value, associates [key] to /// that value, and then returns the new value. + /// + /// That is, if the key is currently in the map, + /// `map.putIfAbsent(key, ifAbsent)` is equivalent to `map[key]`. + /// If the key is not currently in the map, + /// it's instead equivalent to `map[key] = ifAbsent()` + /// (but without any guarantee that the `[]` and `[]=` operators are + /// actually called to achieve that effect). + /// /// ```dart /// final diameters = {1.0: 'Earth'}; /// final otherDiameters = {0.383: 'Mercury', 0.949: 'Venus'}; @@ -362,7 +374,8 @@ abstract interface class Map { /// print(result); // Mercury /// print(diameters); // {1.0: Earth, 0.383: Mercury, 0.949: Venus} /// ``` - /// Calling [ifAbsent] must not add or remove keys from the map. + /// The [ifAbsent] function is allowed to modify the map, + /// and if so, it behaves the same as the equivalent `map[key] = ifAbsent()`. V putIfAbsent(K key, V ifAbsent()); /// Adds all key/value pairs of [other] to this map. diff --git a/tests/corelib/map_putifabsent_test.dart b/tests/corelib/map_putifabsent_test.dart new file mode 100644 index 00000000000..4994062fb9e --- /dev/null +++ b/tests/corelib/map_putifabsent_test.dart @@ -0,0 +1,409 @@ +// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// Checks that `putIfAbsent(key, ifAbsent)` works like `[key]=ifAbsent()` +// if the key is not in the map, and as `[key]` if the key is in the map. + +import "package:expect/expect.dart"; +import 'dart:collection'; + +void main() { + testMaps(); + testMaps(); + testMaps(); + testMaps(); +} + +void testMaps() { + // Testing `putIfAbsent` of the platform maps. + test(() => HashMap()); + test(() => LinkedHashMap()); + test(() => SplayTreeMap(), ordered: true); + test(() => SplayTreeMap(compareComparable), ordered: true); + // The identity version. + test(() => Map.identity(), identity: true); + test(() => HashMap.identity(), identity: true); + test(() => LinkedHashMap.identity(), identity: true); + // And the configured versions, with equality. + test( + () => HashMap(equals: (x, y) => x == y, hashCode: (x) => x.hashCode), + ); + test( + () => LinkedHashMap( + equals: (x, y) => x == y, + hashCode: (x) => x.hashCode, + ), + ); + // With identity, recognizable as such, and not. + test( + () => HashMap(equals: identical, hashCode: identityHashCode), + identity: true, + ); + test( + () => LinkedHashMap(equals: identical, hashCode: identityHashCode), + identity: true, + ); + test( + () => HashMap( + equals: (x, y) => identical(x, y), + hashCode: (x) => identityHashCode(x), + ), + identity: true, + ); + test( + () => LinkedHashMap( + equals: (x, y) => identical(x, y), + hashCode: (x) => identityHashCode(x), + ), + identity: true, + ); + + // Test that [MapView] doesn't break anything. + test(() => MapView(HashMap())); + test(() => MapView(SplayTreeMap()), ordered: true); + + // Test the `putIfAbsent` implementation of `MapBase` and `MapMixin`. + test(() => MapBaseMap()); + test(() => MapMixinMap()); +} + +// Key and value types may be `Object?` or `Object`. +// If [identity] is true, the map uses identical as equality. +// If [ordered] is true, the map uses a `Comparable` as equality +// (and cannot accept `null` as key, even if the key type is `Object?`). +// Otherwise it uses `==` and maybe `hashCode`. +void test( + Map Function() map, { + bool identity = false, + bool ordered = false, +}) { + // Different kinds of keys. + // Every key type implements `Comparable`. + + // Custom key objects overriding `==`. + var key1 = Key(1) as K; + var key1B = Key(1) as K; // Equal, not identical to key1. + var key2 = Key(2) as K; // Different from, but comparable to, key1. + testKeys( + map(), + key1, + key1B, + key2, + identity: identity, + ordered: ordered, + ); + + // Custom key objects not overriding `==`. + // (Cannot have equal-but-not-identical value.) + var idKey1 = IdKey(1) as K; + var idKey2 = IdKey(2) as K; + testKeys( + map(), + idKey1, + idKey1, + idKey2, + identity: identity, + ordered: ordered, + ); + + // Built-in/"native" keys. + var num1 = 1 as K; + var num1B = 1.0 as K; // Equal, but sometimes not identical, to num1. + var num2 = 2 as K; // Different from, but comparable to, key1. + testKeys( + map(), + num1, + num1B, + num2, + identity: identity, + ordered: ordered, + ); + + // String keys (because web treats those specially). + var str1 = "abc" as K; + // Obfuscate that it's always "abc". Try to make it not canonicalized. + var str1B = String.fromCharCodes(("abcmore").codeUnits.take(3)) as K; + var str2 = "def" as K; + Expect.equals(str1, str1B, "Something's badly wrong with str1B?"); + testKeys( + map(), + str1, + str1B, + str2, + identity: identity, + ordered: ordered, + ); +} + +void testKeys( + Map map, + K k1, + K k1b, + K k2, { + required bool identity, + required bool ordered, +}) { + var ifAbsentCalled = 0; + + var v42 = 42 as V; + var v37 = 37 as V; + var v87 = 87 as V; + var vb = "BANANA" as V; + + V Function() add(K key, V value) => () { + expectNotIn(map, key); + ifAbsentCalled++; + return value; + }; + + V? result; + + Expect.mapEquals({}, map); + result = map.putIfAbsent(k1, add(k1, v42)); + Expect.mapEquals({k1: 42}, map); // Added k1:42 + Expect.equals(1, ifAbsentCalled); // Called add(42). + Expect.equals(42, result); // Returned 42. + + ifAbsentCalled = 0; + + // Using same key again doesn't change map or call `ifAbsent`. + result = map.putIfAbsent(k1, add(k1, v87)); + Expect.mapEquals({k1: 42}, map); // Did not change map. + Expect.equals(0, ifAbsentCalled); // Did not call add(87) + Expect.equals(42, result); // Returned existing value. + + // Same for equal, but not identical, key. + if (!identity) { + result = map.putIfAbsent(k1b, add(k1b, v87)); + Expect.mapEquals({k1: 42}, map); // Did not change map. + Expect.equals(0, ifAbsentCalled); // Did not call add(87) + Expect.equals(42, result); // Returned existing value. + } + + if (map is! Map) { + var vNull = null as V; + // Allows null value. Check that a null value isn't the same as no key. + Expect.mapEquals({k1: 42}, map); + result = map.putIfAbsent(k2, add(k2, vNull)); + Expect.mapEquals({k1: 42, k2: null}, map); // Added k2:null. + Expect.equals(1, ifAbsentCalled); // Called add(null). + Expect.equals(null, result); // Returned null. + ifAbsentCalled = 0; + + result = map.putIfAbsent(k2, add(k2, v87)); + Expect.mapEquals({k1: 42, k2: null}, map); // Did not change map. + Expect.equals(0, ifAbsentCalled); // Did not call add(87) + Expect.equals(null, result); // Returned existing value. + + map.remove(k2); + } + + if (!ordered && map is! Map) { + // Allows null key. + Expect.mapEquals({k1: 42}, map); + var kNull = null as K; + result = map.putIfAbsent(kNull, add(kNull, v37)); + Expect.mapEquals({k1: 42, null: 37}, map); // Added null:37 + Expect.equals(1, ifAbsentCalled); // Called add(37). + Expect.equals(37, result); // Returned 37. + ifAbsentCalled = 0; + + result = map.putIfAbsent(kNull, add(kNull, v87)); + Expect.mapEquals({k1: 42, null: 37}, map); // Did not change map. + Expect.equals(0, ifAbsentCalled); // Did not call add(87) + Expect.equals(37, result); // Returned existing value. + + map.remove(null); + } + Expect.mapEquals({k1: 42}, map); + + // Concurrent modification allowed. + // If `ifAbsent` modifies map, the returned value is still added. + + // Remove inside ifAbsent. + result = map.putIfAbsent(k2, () { + expectNotIn(map, k2); + ifAbsentCalled++; + return map.remove(k1) as V; // Remove inside putIfAbsent. + }); + Expect.mapEquals({k2: 42}, map); + Expect.equals(1, ifAbsentCalled); + Expect.equals(42, result); + ifAbsentCalled = 0; + + // Add other key inside ifAbsent. + map.clear(); + Expect.mapEquals({}, map); + result = map.putIfAbsent(k1, () { + expectNotIn(map, k1); + map[k2] = v87; // Add other key. + ifAbsentCalled++; + return v42; + }); + Expect.mapEquals({k1: 42, k2: 87}, map); + Expect.equals(1, ifAbsentCalled); + Expect.equals(42, result); + ifAbsentCalled = 0; + + map.remove(k1); + Expect.mapEquals({k2: 87}, map); + + // Add same key inside ifAbsent. + result = map.putIfAbsent(k1, () { + expectNotIn(map, k1); + map[k1] = vb; // Add value for same key. + Expect.mapEquals({k1: "BANANA", k2: 87}, map); + ifAbsentCalled++; + return v42; + }); + Expect.mapEquals({k1: 42, k2: 87}, map); // Value was overwritten. + Expect.equals(1, ifAbsentCalled); + Expect.equals(42, result); + ifAbsentCalled = 0; + + map.remove(k1); + Expect.mapEquals({k2: 87}, map); + + // Add *and* remove same key inside ifAbsent. + result = map.putIfAbsent(k1, () { + expectNotIn(map, k1); + map[k1] = vb; // Add value for same key. + Expect.mapEquals({k1: "BANANA", k2: 87}, map); + map.remove(k1); + Expect.mapEquals({k2: 87}, map); + ifAbsentCalled++; + return v42; + }); + Expect.mapEquals({k1: 42, k2: 87}, map); // Value was overwritten. + Expect.equals(1, ifAbsentCalled); + Expect.equals(42, result); + ifAbsentCalled = 0; + + // Add same key inside ifAbsent using `putIfAbsent`. + map.remove(k1); + Expect.mapEquals({k2: 87}, map); + result = map.putIfAbsent(k1, () { + expectNotIn(map, k1); + result = map.putIfAbsent(k1, add(k1, vb)); // Add value for same key. + Expect.equals(1, ifAbsentCalled); + Expect.mapEquals({k1: "BANANA", k2: 87}, map); + ifAbsentCalled++; + return v42; + }); + Expect.mapEquals({k1: 42, k2: 87}, map); // Value was overwritten. + Expect.equals(2, ifAbsentCalled); + Expect.equals(42, result); + ifAbsentCalled = 0; + + // Throw inside ifAbsent. + map.remove(k2); + Expect.mapEquals({k1: 42}, map); + try { + result = map.putIfAbsent(k2, () { + expectNotIn(map, k2); + ifAbsentCalled++; + throw "EXIT"; + }); + } on String catch (e) { + Expect.equals("EXIT", e); + } + expectNotIn(map, k2); + Expect.mapEquals({k1: 42}, map); + + // Throw inside ifAbsent after doing modification. Modification stays. + map.remove(k2); + Expect.mapEquals({k1: 42}, map); + try { + result = map.putIfAbsent(k2, () { + expectNotIn(map, k2); + result = map.putIfAbsent(k2, add(k2, v87)); + ifAbsentCalled++; + throw "EXIT"; + }); + } on String catch (e) { + Expect.equals("EXIT", e); + } + Expect.mapEquals({k1: 42, k2: 87}, map); +} + +// ------------------------------------------------------------------- +// Helper classes and functions. + +// Key class that does not override `operator==`. +class IdKey implements Comparable { + final int id; + const IdKey(this.id); + int compareTo(IdKey other) => id.compareTo(other.id); + String toString() => "IdKey($id)"; +} + +// Key class that does override `operator==`. +class Key extends IdKey { + const Key(super.id); + int get hashCode => id.hashCode ^ 0x3a5f731; + bool operator ==(Object other) => other is Key && id == other.id; + String toString() => "Key($id)"; +} + +int compareComparable(Object? v1, Object? v2) => + (v1 as Comparable).compareTo(v2); + +// Slow implementation of Map based on MapBase. +// Taken from `map_test.dart`. +mixin class MapBaseOperations { + final List _keys = []; + final List _values = []; + + V? operator [](Object? key) { + if (key is! K) return null; + int index = _keys.indexOf(key); + if (index < 0) return null; + return _values[index]; + } + + // Not testing this, so not caring if it recognizes concurrent modifications. + Iterable get keys => _keys.skip(0); + + void operator []=(K key, V value) { + int index = _keys.indexOf(key); + if (index >= 0) { + _values[index] = value; + } else { + _keys.add(key); + _values.add(value); + } + } + + V? remove(Object? key) { + if (key is! K) return null; + int index = _keys.indexOf(key); + if (index >= 0) { + var result = _values[index]; + key = _keys.removeLast(); + var value = _values.removeLast(); + if (index != _keys.length) { + _keys[index] = key; + _values[index] = value; + } + return result; + } + return null; + } + + void clear() { + // Clear cannot be based on remove, since remove won't remove keys that + // are not equal to themselves. + _keys.clear(); + _values.clear(); + } +} + +class MapBaseMap = MapBase with MapBaseOperations; +class MapMixinMap = MapBaseOperations with MapMixin; + +void expectNotIn(Map map, Object? key) { + Expect.isFalse(map.containsKey(key)); + Expect.isNull(map[key]); + Expect.isFalse(map.keys.contains(key)); +} diff --git a/tests/corelib/map_test.dart b/tests/corelib/map_test.dart index 0d9e4e099d3..2591e3084cb 100644 --- a/tests/corelib/map_test.dart +++ b/tests/corelib/map_test.dart @@ -183,6 +183,8 @@ void main() { testFrom(); testLazyKeysValueEntries(); + + testRegressions47852(); } void test(Map map) { @@ -1241,3 +1243,17 @@ class Key implements Comparable { bool operator ==(Object other) => other is Key && id == other.id; int compareTo(Key other) => id.compareTo(other.id); } + +void testRegressions47852() { + // Bug in dev-compiler's putIfAbsent. + // https://dartbug.com/47852 + + var map = {}; + var key = DateTime.now(); // Overrides Object.==/hashCode + var wasAbsent = false; + map.putIfAbsent(key, () { + wasAbsent = true; + Expect.isFalse(map.containsKey(key)); + }); + Expect.isTrue(wasAbsent); +} diff --git a/tests/dartdevc/debugger/debugger_test_golden.txt b/tests/dartdevc/debugger/debugger_test_golden.txt index 119e1679037..1fd9e763ece 100644 --- a/tests/dartdevc/debugger/debugger_test_golden.txt +++ b/tests/dartdevc/debugger/debugger_test_golden.txt @@ -4665,6 +4665,32 @@ Value: ] ] ], + [ + "li", + { + "style": "padding-left: 13px;" + }, + [ + "span", + { + "style": "background-color: thistle; color: rgb(136, 19, 145); margin-right: -13px" + }, + "_putIfAbsentKeyMap: " + ], + [ + "span", + { + "style": "margin-left: 13px" + }, + [ + "object", + { + "object": "", + "config": {} + } + ] + ] + ], [ "li", {