Fix JS Map.keys.remove return value #KT-65525

libraries/stdlib/js/src/kotlin/collections/HashMapEntry.kt

@@ -20,7 +20,7 @@ internal class HashMapKeys<E> internal constructor(
     override fun clear() = backing.clear()
     override fun add(element: E): Boolean = throw UnsupportedOperationException()
     override fun addAll(elements: Collection<E>): Boolean = throw UnsupportedOperationException()
-    override fun remove(element: E): Boolean = backing.remove(element) != null
+    override fun remove(element: E): Boolean = backing.removeKey(element)
     override fun iterator(): MutableIterator<E> = backing.keysIterator()
 
     override fun checkIsMutable() = backing.checkIsMutable()

libraries/stdlib/js/src/kotlin/collections/InternalHashMap.kt

@@ -348,6 +348,14 @@ internal class InternalHashMap<K, V> private constructor(
         }
     }
 
+    override fun removeKey(key: K): Boolean {
+        checkIsMutable()
+        val index = findKey(key)
+        if (index < 0) return false
+        removeEntryAt(index)
+        return true
+    }
+
     private fun removeEntryAt(index: Int) {
         keysArray.resetAt(index)
         valuesArray?.resetAt(index)

libraries/stdlib/js/src/kotlin/collections/InternalMap.kt

@@ -22,6 +22,7 @@ internal interface InternalMap<K, V> {
     fun containsOtherEntry(entry: Map.Entry<*, *>): Boolean
 
     fun remove(key: K): V?
+    fun removeKey(key: K): Boolean
     fun removeValue(value: V): Boolean
     fun removeEntry(entry: Map.Entry<K, V>): Boolean
 

libraries/stdlib/js/src/kotlin/collections/InternalStringMap.kt

@@ -173,6 +173,12 @@ internal open class InternalStringMap<K, V> : InternalMap<K, V> {
         return removingValue
     }
 
+    override fun removeKey(key: K): Boolean {
+        val index = findKeyIndex(key) ?: return false
+        removeKeyIndex(key, index)
+        return true
+    }
+
     internal open fun removeKeyIndex(key: K, removingIndex: Int) {
         jsDeleteProperty(backingMap.unsafeCast<Any>(), key as Any)
 

libraries/stdlib/test/collections/MapTest.kt

@@ -403,17 +403,17 @@ class MapTest {
         assertEquals(3, filteredByValue["b"])
     }
 
+    class SimpleEntry<out K, out V>(override val key: K, override val value: V) : Map.Entry<K, V> {
+        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
+    }
+
     @Test
     fun entriesCovariantContains() {
         // Based on https://youtrack.jetbrains.com/issue/KT-42428.
         fun doTest(implName: String, map: Map<String, Int>, key: String, value: Int) {
-            class SimpleEntry<out K, out V>(override val key: K, override val value: V) : Map.Entry<K, V> {
-                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)
@@ -448,13 +448,6 @@ class MapTest {
     @Test
     fun entriesCovariantRemove() {
         fun doTest(implName: String, map: MutableMap<String, Int>, key: String, value: Int) {
-            class SimpleEntry<out K, out V>(override val key: K, override val value: V) : Map.Entry<K, V> {
-                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.entries.toMutableSet().remove(SimpleEntry(key, value) as Map.Entry<*, *>), "$mapDescription: reference")
@@ -478,6 +471,57 @@ class MapTest {
         doTest("linkedStringMapOf", mapLetterToIndex.toMap(linkedStringMapOf()), "w", 22)
     }
 
+    @Test
+    fun nullKeyAndValue() {
+        fun doTest(implName: String, map: MutableMap<String?, Int?>, key: String?, value: Int?) {
+            val mapDescription = "$implName: ${map::class}, key=$key, value=$value"
+
+            map[key] = value
+
+            assertTrue(map.contains(key), "$mapDescription: contains")
+            assertTrue(map.containsKey(key), "$mapDescription: containsKey")
+            assertTrue(map.containsValue(value), "$mapDescription: containsValue")
+            assertTrue(map.keys.contains(key), "$mapDescription: keys.contains")
+            assertTrue(map.values.contains(value), "$mapDescription: values.contains")
+            assertTrue(map.entries.contains(SimpleEntry(key, value) as Map.Entry<*, *>), "$mapDescription: entries.contains")
+
+            assertEquals(value, map.remove(key), "$mapDescription: remove")
+
+            map[key] = value
+            assertTrue(map.keys.remove(key), "$mapDescription: keys.remove")
+
+            map[key] = value
+            assertTrue(map.values.remove(value), "$mapDescription: values.remove")
+
+            map[key] = value
+            assertTrue(map.entries.remove(SimpleEntry(key, value) as Map.Entry<*, *>), "$mapDescription: entries.remove")
+        }
+
+        val mapLetterToIndex = ('a'..'z').mapIndexed { i, c -> "$c" to i }.toMap()
+        val combinations = listOf(
+            "A" to null,
+            null to 100,
+            null to null,
+        )
+
+        for ((key, value) in combinations) {
+            doTest("default mutable", mapLetterToIndex.toMutableMap(), key, value)
+            doTest("HashMap", mapLetterToIndex.toMap(HashMap()), key, value)
+            doTest("LinkedHashMap", mapLetterToIndex.toMap(LinkedHashMap()), key, value)
+
+            buildMap {
+                putAll(mapLetterToIndex)
+                doTest("MapBuilder", this, key, value)
+            }
+
+            @Suppress("UNCHECKED_CAST")
+            if (key != null) {
+                doTest("stringMapOf", mapLetterToIndex.toMap(stringMapOf()) as MutableMap<String?, Int?>, key, value)
+                doTest("linkedStringMapOf", mapLetterToIndex.toMap(linkedStringMapOf()) as MutableMap<String?, Int?>, key, value)
+            }
+        }
+    }
+
     @Test
     fun firstNotNullOf() {
         val map = mapOf("Alice" to 20, "Tom" to 13, "Bob" to 18)