From 28168bf230a434c79fb96faa4cee8dd4930f47bd Mon Sep 17 00:00:00 2001 From: Ilya Gorbunov Date: Thu, 12 Nov 2020 05:10:29 +0300 Subject: [PATCH] Correctly implement specialized MutableEntrySet.contains KT-41278 This is a workaround for the problem KT-43321. Introduce an intermediate abstract set specialized for Map.Entry elements and implement 'contains(Map.Entry)' method there. Then inherit that intermediate set in entrysets of JS HashMap, JS LinkedHashMap, JVM MapBuilder, that are specialized for MutableMap.MutableEntry elements, so that no override of 'contains' is required anymore. This allows to avoid incorrect special 'contains' bridge being generated that otherwise rejects all arguments except ones of MutableEntry type. --- .../kotlin/collections/AbstractMutableMap.kt | 6 +++ .../js/src/kotlin/collections/HashMap.kt | 9 +++-- .../src/kotlin/collections/LinkedHashMap.kt | 9 +++-- .../kotlin/collections/builders/MapBuilder.kt | 10 ++++- libraries/stdlib/test/collections/MapTest.kt | 37 +++++++++++++++++++ 5 files changed, 61 insertions(+), 10 deletions(-) diff --git a/libraries/stdlib/js/src/kotlin/collections/AbstractMutableMap.kt b/libraries/stdlib/js/src/kotlin/collections/AbstractMutableMap.kt index 5fca74d9397..f705221f119 100644 --- a/libraries/stdlib/js/src/kotlin/collections/AbstractMutableMap.kt +++ b/libraries/stdlib/js/src/kotlin/collections/AbstractMutableMap.kt @@ -46,6 +46,12 @@ public actual abstract class AbstractMutableMap protected actual construct } + // intermediate abstract class to workaround KT-43321 + internal abstract class AbstractEntrySet, K, V> : AbstractMutableSet() { + final override fun contains(element: E): Boolean = containsEntry(element) + abstract fun containsEntry(element: Map.Entry): Boolean + } + actual override fun clear() { entries.clear() } diff --git a/libraries/stdlib/js/src/kotlin/collections/HashMap.kt b/libraries/stdlib/js/src/kotlin/collections/HashMap.kt index 8cbdd4d17e9..c372f056592 100644 --- a/libraries/stdlib/js/src/kotlin/collections/HashMap.kt +++ b/libraries/stdlib/js/src/kotlin/collections/HashMap.kt @@ -1,7 +1,8 @@ /* - * Copyright 2010-2018 JetBrains s.r.o. and Kotlin Programming Language contributors. + * Copyright 2010-2020 JetBrains s.r.o. and Kotlin Programming Language contributors. * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file. */ + /* * Based on GWT AbstractHashMap * Copyright 2008 Google Inc. @@ -20,14 +21,14 @@ import kotlin.collections.MutableMap.MutableEntry // have to make sure mutating methods check `checkIsMutable`. public actual open class HashMap : AbstractMutableMap, MutableMap { - private inner class EntrySet : AbstractMutableSet>() { + private inner class EntrySet : AbstractEntrySet, K, V>() { override fun add(element: MutableEntry): Boolean = throw UnsupportedOperationException("Add is not supported on entries") override fun clear() { this@HashMap.clear() } - override operator fun contains(element: MutableEntry): Boolean = containsEntry(element) + override fun containsEntry(element: Map.Entry): Boolean = this@HashMap.containsEntry(element) override operator fun iterator(): MutableIterator> = internalMap.iterator() @@ -121,4 +122,4 @@ public actual open class HashMap : AbstractMutableMap, MutableMap stringMapOf(vararg pairs: Pair): HashMap { return HashMap(InternalStringMap(EqualityComparator.HashCode)).apply { putAll(pairs) } -} \ No newline at end of file +} diff --git a/libraries/stdlib/js/src/kotlin/collections/LinkedHashMap.kt b/libraries/stdlib/js/src/kotlin/collections/LinkedHashMap.kt index 67057019bb6..a626e23b37d 100644 --- a/libraries/stdlib/js/src/kotlin/collections/LinkedHashMap.kt +++ b/libraries/stdlib/js/src/kotlin/collections/LinkedHashMap.kt @@ -1,7 +1,8 @@ /* - * Copyright 2010-2018 JetBrains s.r.o. and Kotlin Programming Language contributors. + * Copyright 2010-2020 JetBrains s.r.o. and Kotlin Programming Language contributors. * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file. */ + /* * Based on GWT LinkedHashMap * Copyright 2008 Google Inc. @@ -40,7 +41,7 @@ public actual open class LinkedHashMap : HashMap, MutableMap { } } - private inner class EntrySet : AbstractMutableSet>() { + private inner class EntrySet : AbstractEntrySet, K, V>() { private inner class EntryIterator : MutableIterator> { // The last entry that was returned from this iterator. @@ -85,7 +86,7 @@ public actual open class LinkedHashMap : HashMap, MutableMap { this@LinkedHashMap.clear() } - override operator fun contains(element: MutableEntry): Boolean = containsEntry(element) + override fun containsEntry(element: Map.Entry): Boolean = this@LinkedHashMap.containsEntry(element) override operator fun iterator(): MutableIterator> = EntryIterator() @@ -275,4 +276,4 @@ public actual open class LinkedHashMap : HashMap, MutableMap { */ public fun linkedStringMapOf(vararg pairs: Pair): LinkedHashMap { return LinkedHashMap(stringMapOf()).apply { putAll(pairs) } -} \ No newline at end of file +} diff --git a/libraries/stdlib/jvm/src/kotlin/collections/builders/MapBuilder.kt b/libraries/stdlib/jvm/src/kotlin/collections/builders/MapBuilder.kt index 01c04cb6c30..16f5ffff796 100644 --- a/libraries/stdlib/jvm/src/kotlin/collections/builders/MapBuilder.kt +++ b/libraries/stdlib/jvm/src/kotlin/collections/builders/MapBuilder.kt @@ -595,13 +595,19 @@ internal class MapBuilderValues internal constructor( } } +// intermediate abstract class to workaround KT-43321 +internal abstract class AbstractMapBuilderEntrySet, K, V> : AbstractMutableSet() { + final override fun contains(element: E): Boolean = containsEntry(element) + abstract fun containsEntry(element: Map.Entry): Boolean +} + internal class MapBuilderEntries internal constructor( val backing: MapBuilder -) : MutableSet>, AbstractMutableSet>() { +) : AbstractMapBuilderEntrySet, K, V>() { override val size: Int get() = backing.size override fun isEmpty(): Boolean = backing.isEmpty() - override fun contains(element: MutableMap.MutableEntry): Boolean = backing.containsEntry(element) + override fun containsEntry(element: Map.Entry): Boolean = backing.containsEntry(element) override fun clear() = backing.clear() override fun add(element: MutableMap.MutableEntry): Boolean = throw UnsupportedOperationException() override fun addAll(elements: Collection>): Boolean = throw UnsupportedOperationException() diff --git a/libraries/stdlib/test/collections/MapTest.kt b/libraries/stdlib/test/collections/MapTest.kt index 7aa0347205e..e9c614c1fd8 100644 --- a/libraries/stdlib/test/collections/MapTest.kt +++ b/libraries/stdlib/test/collections/MapTest.kt @@ -400,6 +400,43 @@ class MapTest { assertEquals(3, filteredByValue["b"]) } + @Test + fun entriesCovariantContains() { + // Based on https://youtrack.jetbrains.com/issue/KT-42428. + fun doTest(implName: String, map: Map, key: String, value: Int) { + class SimpleEntry(override val key: K, override val value: V) : Map.Entry { + override fun toString(): String = "$key=$value" + override fun hashCode(): Int = key.hashCode() xor value.hashCode() + override fun equals(other: Any?): Boolean = + other is Map.Entry<*, *> && key == other.key && value == other.value + } + + val mapDescription = "$implName: ${map::class}" + + assertTrue(map.keys.contains(key), mapDescription) + assertEquals(value, map[key], mapDescription) + // This one requires special efforts to make it work this way. + // map.entries can in fact be `MutableSet`, + // which [contains] method takes [MutableEntry], so the compiler may generate special bridge + // returning false for values that aren't [MutableEntry] (including [SimpleEntry]). + assertTrue(map.entries.contains(SimpleEntry(key, value)), mapDescription) + assertTrue(map.entries.toSet().contains(SimpleEntry(key, value)), mapDescription) + } + + val mapLetterToIndex = ('a'..'z').mapIndexed { i, c -> "$c" to i }.toMap() + doTest("default read-only", mapLetterToIndex, "h", 7) + doTest("default mutable", mapLetterToIndex.toMutableMap(), "b", 1) + doTest("HashMap", mapLetterToIndex.toMap(HashMap()), "c", 2) + doTest("LinkedHashMap", mapLetterToIndex.toMap(LinkedHashMap()), "d", 3) + + val builtMap = buildMap { + putAll(mapLetterToIndex) + doTest("MapBuilder", this, "z", 25) + } + doTest("built Map", builtMap, "y", 24) + } + + fun testPlusAssign(doPlusAssign: (MutableMap) -> Unit) { val map = hashMapOf("a" to 1, "b" to 2) doPlusAssign(map)