From 14351446d7561fe1eea4d0c48c4cd1177071c9fd Mon Sep 17 00:00:00 2001 From: Ilya Gorbunov Date: Tue, 21 Jan 2020 04:57:02 +0300 Subject: [PATCH] KT-15363 Fix failing test: missing capacity parameter check in JS Partially restore JVM map capacity implementation --- libraries/stdlib/js/src/kotlin/collections.kt | 14 ++++++++-- .../jvm/src/kotlin/collections/MapsJVM.kt | 28 ++++++++++++------- .../src/kotlin/collections/Collections.kt | 1 + .../stdlib/src/kotlin/collections/Maps.kt | 9 ++++++ .../stdlib/src/kotlin/collections/Sets.kt | 1 + .../test/collections/ContainerBuilderTest.kt | 3 ++ 6 files changed, 44 insertions(+), 12 deletions(-) diff --git a/libraries/stdlib/js/src/kotlin/collections.kt b/libraries/stdlib/js/src/kotlin/collections.kt index 7d453019d87..eb22233caf3 100644 --- a/libraries/stdlib/js/src/kotlin/collections.kt +++ b/libraries/stdlib/js/src/kotlin/collections.kt @@ -187,5 +187,15 @@ internal actual fun checkCountOverflow(count: Int): Int { * JS map and set implementations do not make use of capacities or load factors. */ @PublishedApi -@kotlin.internal.InlineOnly -internal actual inline fun mapCapacity(expectedSize: Int) = expectedSize +internal actual fun mapCapacity(expectedSize: Int) = expectedSize + +/** + * Checks a collection builder function capacity argument. + * In JS no validation is made in Map/Set constructor yet. + */ +@SinceKotlin("1.3") +@ExperimentalStdlibApi +@PublishedApi +internal actual fun checkBuilderCapacity(capacity: Int) { + require(capacity >= 0) { "capacity must be non-negative." } +} \ No newline at end of file diff --git a/libraries/stdlib/jvm/src/kotlin/collections/MapsJVM.kt b/libraries/stdlib/jvm/src/kotlin/collections/MapsJVM.kt index 366fc86f161..4227236653e 100644 --- a/libraries/stdlib/jvm/src/kotlin/collections/MapsJVM.kt +++ b/libraries/stdlib/jvm/src/kotlin/collections/MapsJVM.kt @@ -97,19 +97,27 @@ internal actual fun Map.toSingletonMap(): Map = /** * Calculate the initial capacity of a map, based on Guava's - * [com.google.common.collect.Maps.capacity](https://github.com/google/guava/blob/master/guava/src/com/google/common/collect/Maps.java) - * approach. This is similar to [java.util.HashMap.putMapEntries] (JDK8+) but provides further optimizations for small or large sizes. + * [com.google.common.collect.Maps.capacity](https://github.com/google/guava/blob/v28.2/guava/src/com/google/common/collect/Maps.java#L325) + * approach. */ @PublishedApi internal actual fun mapCapacity(expectedSize: Int): Int = when { - // We are not coercing the value to a valid one and not throwing an exception. It is up to the receiving class to - // properly handle negative values. Note that all built-in classes do exactly that and will throw. + // We are not coercing the value to a valid one and not throwing an exception. It is up to the caller to + // properly handle negative values. expectedSize < 0 -> expectedSize - expectedSize < MIN_CAPACITY -> expectedSize + 1 - expectedSize < MAX_CAPACITY -> ((expectedSize / 0.75F) + 1.0F).toInt() - else -> MAX_CAPACITY + expectedSize < 3 -> expectedSize + 1 + expectedSize < INT_MAX_POWER_OF_TWO -> ((expectedSize / 0.75F) + 1.0F).toInt() + // any large value + else -> Int.MAX_VALUE } +private const val INT_MAX_POWER_OF_TWO: Int = 1 shl (Int.SIZE_BITS - 2) -// java.util.HashMap.MAX_CAPACITY -private const val MAX_CAPACITY = 1 shl 30 -private const val MIN_CAPACITY = 3 +/** + * Checks a collection builder function capacity argument. + * Does nothing, capacity is validated in List/Set/Map constructor + */ +@SinceKotlin("1.3") +@ExperimentalStdlibApi +@PublishedApi +@kotlin.internal.InlineOnly +internal actual inline fun checkBuilderCapacity(capacity: Int) {} \ No newline at end of file diff --git a/libraries/stdlib/src/kotlin/collections/Collections.kt b/libraries/stdlib/src/kotlin/collections/Collections.kt index 8158128307a..a052d33e351 100644 --- a/libraries/stdlib/src/kotlin/collections/Collections.kt +++ b/libraries/stdlib/src/kotlin/collections/Collections.kt @@ -178,6 +178,7 @@ public inline fun buildList(@BuilderInference builderAction: MutableList. @kotlin.internal.InlineOnly public inline fun buildList(expectedSize: Int, @BuilderInference builderAction: MutableList.() -> Unit): List { contract { callsInPlace(builderAction, InvocationKind.EXACTLY_ONCE) } + checkBuilderCapacity(expectedSize) return ArrayList(expectedSize).apply(builderAction) } diff --git a/libraries/stdlib/src/kotlin/collections/Maps.kt b/libraries/stdlib/src/kotlin/collections/Maps.kt index 8b3484b975a..0757c82b6b2 100644 --- a/libraries/stdlib/src/kotlin/collections/Maps.kt +++ b/libraries/stdlib/src/kotlin/collections/Maps.kt @@ -148,6 +148,7 @@ public inline fun buildMap(@BuilderInference builderAction: MutableMap buildMap(expectedSize: Int, @BuilderInference builderAction: MutableMap.() -> Unit): Map { contract { callsInPlace(builderAction, InvocationKind.EXACTLY_ONCE) } + checkBuilderCapacity(expectedSize) return LinkedHashMap(mapCapacity(expectedSize)).apply(builderAction) } @@ -157,6 +158,14 @@ public inline fun buildMap(expectedSize: Int, @BuilderInference builderAc @PublishedApi internal expect fun mapCapacity(expectedSize: Int): Int +/** + * Checks a collection builder function capacity argument. + */ +@SinceKotlin("1.3") +@ExperimentalStdlibApi +@PublishedApi +internal expect fun checkBuilderCapacity(capacity: Int) + /** Returns `true` if this map is not empty. */ @kotlin.internal.InlineOnly public inline fun Map.isNotEmpty(): Boolean = !isEmpty() diff --git a/libraries/stdlib/src/kotlin/collections/Sets.kt b/libraries/stdlib/src/kotlin/collections/Sets.kt index c5af60f100b..1112d70d9fc 100644 --- a/libraries/stdlib/src/kotlin/collections/Sets.kt +++ b/libraries/stdlib/src/kotlin/collections/Sets.kt @@ -115,6 +115,7 @@ public inline fun buildSet(@BuilderInference builderAction: MutableSet.() @kotlin.internal.InlineOnly public inline fun buildSet(expectedSize: Int, @BuilderInference builderAction: MutableSet.() -> Unit): Set { contract { callsInPlace(builderAction, InvocationKind.EXACTLY_ONCE) } + checkBuilderCapacity(expectedSize) return LinkedHashSet(mapCapacity(expectedSize)).apply(builderAction) } diff --git a/libraries/stdlib/test/collections/ContainerBuilderTest.kt b/libraries/stdlib/test/collections/ContainerBuilderTest.kt index 3884f19e94b..f2c85ac6f69 100644 --- a/libraries/stdlib/test/collections/ContainerBuilderTest.kt +++ b/libraries/stdlib/test/collections/ContainerBuilderTest.kt @@ -18,6 +18,7 @@ class ContainerBuilderTest { assertEquals(listOf('a', 'b', 'c', 'd'), y) + assertEquals(listOf(1), buildList(0) { add(1) }) assertFailsWith { buildList(-1) { add(0) } } @@ -38,6 +39,7 @@ class ContainerBuilderTest { assertEquals(setOf('c', 'b', 'd'), y) + assertEquals(setOf(1), buildSet(0) { add(1) }) assertFailsWith { buildSet(-1) { add(0) } } @@ -59,6 +61,7 @@ class ContainerBuilderTest { assertEquals(mapOf('a' to 1, 'c' to 3, 'b' to 2, 'd' to 4), y) + assertEquals(mapOf("a" to 1), buildMap(0) { put("a", 1) }) assertFailsWith { buildMap(-1) { put("x", 1) } }