From 65679cda1837e8a10fc32ca53f6381a6d3f416d3 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Thu, 1 May 2025 10:16:05 +0400 Subject: [PATCH 01/14] 144: Add a reproducer from the issue #144 --- .../src/contract/set/PersistentHashSetTest.kt | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index d1a5c777..3f746e06 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -6,6 +6,8 @@ package tests.contract.set import kotlinx.collections.immutable.persistentHashSetOf +import kotlinx.collections.immutable.minus +import kotlinx.collections.immutable.toPersistentHashSet import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertTrue @@ -27,4 +29,19 @@ class PersistentHashSetTest { assertEquals(set2, builder.build().toSet()) assertEquals(set2, builder.build()) } + + /** + * Test from issue: https://github.com/Kotlin/kotlinx.collections.immutable/issues/144 + */ + @Test + fun reproducer() { + val firstBatch = listOf(4554, 9380, 4260, 6602) + val secondBatch = listOf(1188, 14794) + val extraElement = 7450 + + val set = firstBatch.plus(secondBatch).plus(extraElement).toPersistentHashSet() + val result = set.minus(firstBatch.toPersistentHashSet()).minus(secondBatch) + assertEquals(1, result.size) + assertEquals(extraElement, result.first()) + } } \ No newline at end of file From 38690df6256473d9dd9996f9a0925fd875e10a4d Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 2 May 2025 10:20:03 +0400 Subject: [PATCH 02/14] 144: Convert numbers to binary representation in reproducer --- .../src/contract/set/PersistentHashSetTest.kt | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index 3f746e06..5e8e1418 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -35,9 +35,18 @@ class PersistentHashSetTest { */ @Test fun reproducer() { - val firstBatch = listOf(4554, 9380, 4260, 6602) - val secondBatch = listOf(1188, 14794) - val extraElement = 7450 + val firstBatch = listOf( + 0b0_00100_01110_01010, + 0b0_00110_01110_01010, + 0b0_01001_00101_00100, + 0b0_00100_00101_00100 + ) + val secondBatch = listOf( + 0b0_00001_00101_00100, + 0b0_01110_01110_01010 + ) + val extraElement = + 0b0_00111_01000_11010 val set = firstBatch.plus(secondBatch).plus(extraElement).toPersistentHashSet() val result = set.minus(firstBatch.toPersistentHashSet()).minus(secondBatch) From 443f5998ae05983a1d362bdfdcf34c9284f894c3 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 2 May 2025 10:28:43 +0400 Subject: [PATCH 03/14] 144: Add one more reproducer --- .../src/contract/set/PersistentHashSetTest.kt | 45 ++++++++++++++----- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index 5e8e1418..7931e52a 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -34,22 +34,45 @@ class PersistentHashSetTest { * Test from issue: https://github.com/Kotlin/kotlinx.collections.immutable/issues/144 */ @Test - fun reproducer() { - val firstBatch = listOf( - 0b0_00100_01110_01010, - 0b0_00110_01110_01010, - 0b0_01001_00101_00100, - 0b0_00100_00101_00100 + fun reproducer1() { + validate( + firstBatch = listOf( + 0b0_00100_01110_01010, + 0b0_00110_01110_01010, + 0b0_01001_00101_00100, + 0b0_00100_00101_00100 + ), + secondBatch = listOf( + 0b0_01110_01110_01010, + 0b0_00001_00101_00100 + ), + extraElement = + 0b0_00111_01000_11010 ) - val secondBatch = listOf( - 0b0_00001_00101_00100, - 0b0_01110_01110_01010 + } + + @Test + fun reproducer2() { + validate( + firstBatch = listOf( + 0b0_00000_00000_00000, + 0b0_00001_00000_00000, + 0b0_00000_00000_00001, + 0b0_00001_00000_00001, + ), + secondBatch = listOf( + 0b0_00010_00000_00000, + 0b0_00010_00000_00001 + ), + extraElement = + 0b0_00000_00000_00010 ) - val extraElement = - 0b0_00111_01000_11010 + } + private fun validate(firstBatch: List, secondBatch: List, extraElement: Int) { val set = firstBatch.plus(secondBatch).plus(extraElement).toPersistentHashSet() val result = set.minus(firstBatch.toPersistentHashSet()).minus(secondBatch) + assertEquals(1, result.size) assertEquals(extraElement, result.first()) } From e5e7339464157382067e359fcebae4c5ec280b2e Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 2 May 2025 10:35:07 +0400 Subject: [PATCH 04/14] 144: Add assertEquals(hashSet, set) to validate --- core/commonTest/src/contract/set/PersistentHashSetTest.kt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index 7931e52a..52d8d1bd 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -71,9 +71,12 @@ class PersistentHashSetTest { private fun validate(firstBatch: List, secondBatch: List, extraElement: Int) { val set = firstBatch.plus(secondBatch).plus(extraElement).toPersistentHashSet() + val hashSet = HashSet(firstBatch) + secondBatch + extraElement + assertEquals(hashSet, set) + val result = set.minus(firstBatch.toPersistentHashSet()).minus(secondBatch) assertEquals(1, result.size) - assertEquals(extraElement, result.first()) +// assertEquals(extraElement, result.first()) } } \ No newline at end of file From 1b0f813603ceb53aa002d85832ef7ae39df55d1e Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 2 May 2025 10:38:00 +0400 Subject: [PATCH 05/14] 144: Add check for firstBatchSet --- core/commonTest/src/contract/set/PersistentHashSetTest.kt | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index 52d8d1bd..0fe8e33f 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -74,9 +74,13 @@ class PersistentHashSetTest { val hashSet = HashSet(firstBatch) + secondBatch + extraElement assertEquals(hashSet, set) - val result = set.minus(firstBatch.toPersistentHashSet()).minus(secondBatch) + val firstBatchSet = firstBatch.toPersistentHashSet() + val firstBatchHashSet: Set = HashSet(firstBatch) + assertEquals(firstBatchHashSet, firstBatchSet) + + val result = set.minus(firstBatchSet).minus(secondBatch) assertEquals(1, result.size) -// assertEquals(extraElement, result.first()) + assertEquals(extraElement, result.first()) } } \ No newline at end of file From 1b266c7c8b1ca479c03c09c5e100dc9d4385c7c8 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Fri, 2 May 2025 11:07:57 +0400 Subject: [PATCH 06/14] 144: Simplify repro --- .../src/contract/set/PersistentHashSetTest.kt | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index 0fe8e33f..c9aa0ef2 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -5,6 +5,7 @@ package tests.contract.set +import kotlinx.collections.immutable.implementations.immutableSet.PersistentHashSet import kotlinx.collections.immutable.persistentHashSetOf import kotlinx.collections.immutable.minus import kotlinx.collections.immutable.toPersistentHashSet @@ -70,17 +71,15 @@ class PersistentHashSetTest { } private fun validate(firstBatch: List, secondBatch: List, extraElement: Int) { - val set = firstBatch.plus(secondBatch).plus(extraElement).toPersistentHashSet() - val hashSet = HashSet(firstBatch) + secondBatch + extraElement - assertEquals(hashSet, set) + val set: PersistentHashSet = + firstBatch.plus(secondBatch).plus(extraElement).toPersistentHashSet() + as PersistentHashSet val firstBatchSet = firstBatch.toPersistentHashSet() - val firstBatchHashSet: Set = HashSet(firstBatch) - assertEquals(firstBatchHashSet, firstBatchSet) - val result = set.minus(firstBatchSet).minus(secondBatch) + val actual = set.minus(firstBatchSet).minus(secondBatch) + val expected = persistentHashSetOf(extraElement) - assertEquals(1, result.size) - assertEquals(extraElement, result.first()) + assertEquals(expected, actual) } } \ No newline at end of file From cd80ec9fe978a58be000cdecb49a36c71acec48c Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 5 May 2025 13:38:33 +0400 Subject: [PATCH 07/14] 144: Add equalsTest --- .../src/contract/set/PersistentHashSetTest.kt | 37 +++++++++++++++++-- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index c9aa0ef2..f7a815d4 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -71,14 +71,43 @@ class PersistentHashSetTest { } private fun validate(firstBatch: List, secondBatch: List, extraElement: Int) { + val set = firstBatch.plus(secondBatch).plus(extraElement).toPersistentHashSet() + val result = set.minus(firstBatch.toPersistentHashSet()).minus(secondBatch) + + assertEquals(1, result.size) + assertEquals(extraElement, result.first()) + } + + @Test + fun equalsTest() { + val firstBatch = listOf( + 0b0_00000_00000_00000, + 0b0_00001_00000_00000, + 0b0_00000_00000_00001, + 0b0_00001_00000_00001, + ) + val secondBatch = listOf( + 0b0_00010_00000_00000, + 0b0_00010_00000_00001 + ) + val extraElement = + 0b0_00000_00000_00010 + val set: PersistentHashSet = - firstBatch.plus(secondBatch).plus(extraElement).toPersistentHashSet() + (firstBatch + secondBatch + extraElement).toPersistentHashSet() as PersistentHashSet - val firstBatchSet = firstBatch.toPersistentHashSet() + val firstBatchSet: PersistentHashSet = + firstBatch.toPersistentHashSet() + as PersistentHashSet - val actual = set.minus(firstBatchSet).minus(secondBatch) - val expected = persistentHashSetOf(extraElement) + val actual: PersistentHashSet = + set.minus(firstBatchSet) + as PersistentHashSet + + val expected: PersistentHashSet = + (secondBatch + extraElement).toPersistentHashSet() + as PersistentHashSet assertEquals(expected, actual) } From f0d397f0e7bf6797896e50530d503970874d2bbc Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 5 May 2025 13:39:27 +0400 Subject: [PATCH 08/14] 144: Simplify equalsTest --- core/commonTest/src/contract/set/PersistentHashSetTest.kt | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index f7a815d4..c9464823 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -90,11 +90,9 @@ class PersistentHashSetTest { 0b0_00010_00000_00000, 0b0_00010_00000_00001 ) - val extraElement = - 0b0_00000_00000_00010 val set: PersistentHashSet = - (firstBatch + secondBatch + extraElement).toPersistentHashSet() + (firstBatch + secondBatch).toPersistentHashSet() as PersistentHashSet val firstBatchSet: PersistentHashSet = @@ -106,7 +104,7 @@ class PersistentHashSetTest { as PersistentHashSet val expected: PersistentHashSet = - (secondBatch + extraElement).toPersistentHashSet() + secondBatch.toPersistentHashSet() as PersistentHashSet assertEquals(expected, actual) From 6e7bdcdbf5b92c83f492e36863154f5c4f781975 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 5 May 2025 13:43:08 +0400 Subject: [PATCH 09/14] 144: Simplify equalsTest --- core/commonTest/src/contract/set/PersistentHashSetTest.kt | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index c9464823..6155bd67 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -82,9 +82,7 @@ class PersistentHashSetTest { fun equalsTest() { val firstBatch = listOf( 0b0_00000_00000_00000, - 0b0_00001_00000_00000, - 0b0_00000_00000_00001, - 0b0_00001_00000_00001, + 0b0_00001_00000_00000 ) val secondBatch = listOf( 0b0_00010_00000_00000, From 381767795d5b709e550a8b4269f70577afb8207c Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 5 May 2025 13:45:20 +0400 Subject: [PATCH 10/14] 144: Refactor equalsTest --- .../src/contract/set/PersistentHashSetTest.kt | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index 6155bd67..3eb87b50 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -80,29 +80,27 @@ class PersistentHashSetTest { @Test fun equalsTest() { - val firstBatch = listOf( + val batch = listOf( 0b0_00000_00000_00000, 0b0_00001_00000_00000 ) - val secondBatch = listOf( - 0b0_00010_00000_00000, - 0b0_00010_00000_00001 - ) + val singleElement = + 0b0_00010_00000_00000 val set: PersistentHashSet = - (firstBatch + secondBatch).toPersistentHashSet() + (batch + singleElement).toPersistentHashSet() as PersistentHashSet - val firstBatchSet: PersistentHashSet = - firstBatch.toPersistentHashSet() + val batchSet: PersistentHashSet = + batch.toPersistentHashSet() as PersistentHashSet val actual: PersistentHashSet = - set.minus(firstBatchSet) + set.minus(batchSet) as PersistentHashSet val expected: PersistentHashSet = - secondBatch.toPersistentHashSet() + persistentHashSetOf(singleElement) as PersistentHashSet assertEquals(expected, actual) From ae52b396c59796f9c314e916238a58940f0c7e14 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Mon, 5 May 2025 14:26:06 +0400 Subject: [PATCH 11/14] 144: Refactor equalsTest --- core/commonTest/src/contract/set/PersistentHashSetTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index 3eb87b50..9bda0c29 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -96,7 +96,7 @@ class PersistentHashSetTest { as PersistentHashSet val actual: PersistentHashSet = - set.minus(batchSet) + (set - batchSet) as PersistentHashSet val expected: PersistentHashSet = From c3825739d82088ed1801d211752386c2f22f4fbb Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Wed, 7 May 2025 15:25:18 +0400 Subject: [PATCH 12/14] 144: Simplify equalsTest --- .../src/contract/set/PersistentHashSetTest.kt | 25 +++---------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index 9bda0c29..cb9c022f 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -80,28 +80,11 @@ class PersistentHashSetTest { @Test fun equalsTest() { - val batch = listOf( - 0b0_00000_00000_00000, - 0b0_00001_00000_00000 - ) - val singleElement = - 0b0_00010_00000_00000 - - val set: PersistentHashSet = - (batch + singleElement).toPersistentHashSet() - as PersistentHashSet - - val batchSet: PersistentHashSet = - batch.toPersistentHashSet() - as PersistentHashSet - - val actual: PersistentHashSet = - (set - batchSet) - as PersistentHashSet + val set1: PersistentHashSet = persistentHashSetOf(0, 32768, 65536) as PersistentHashSet + val set2: PersistentHashSet = persistentHashSetOf(0, 32768) as PersistentHashSet - val expected: PersistentHashSet = - persistentHashSetOf(singleElement) - as PersistentHashSet + val expected = persistentHashSetOf(65536) + val actual = set1 - set2 assertEquals(expected, actual) } From 945b1c708003645c5604e7446130d2694d4b97fa Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Wed, 7 May 2025 15:26:08 +0400 Subject: [PATCH 13/14] 144: Move single values are kept only on root level upper --- .../src/implementations/immutableSet/TrieNode.kt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/core/commonMain/src/implementations/immutableSet/TrieNode.kt b/core/commonMain/src/implementations/immutableSet/TrieNode.kt index 32ecca83..d126ab54 100644 --- a/core/commonMain/src/implementations/immutableSet/TrieNode.kt +++ b/core/commonMain/src/implementations/immutableSet/TrieNode.kt @@ -621,17 +621,17 @@ internal class TrieNode( val realSize = realBitMap.countOneBits() return when { realBitMap == 0 -> EMPTY + // single values are kept only on root level + realSize == 1 && shift != 0 -> when (val single = mutableNode.buffer[mutableNode.indexOfCellAt(realBitMap)]) { + is TrieNode<*> -> TrieNode(realBitMap, arrayOf(single), mutator.ownership) + else -> single + } realBitMap == bitmap -> { when { mutableNode.elementsIdentityEquals(this) -> this else -> mutableNode } } - // single values are kept only on root level - realSize == 1 && shift != 0 -> when (val single = mutableNode.buffer[mutableNode.indexOfCellAt(realBitMap)]) { - is TrieNode<*> -> TrieNode(realBitMap, arrayOf(single), mutator.ownership) - else -> single - } else -> { // clean up all the EMPTYs in the resulting buffer val realBuffer = arrayOfNulls(realSize) From 6646ec3f3c7e822811cb0624e7e0635aa84d0e19 Mon Sep 17 00:00:00 2001 From: Dmitry Nekrasov Date: Wed, 7 May 2025 15:34:17 +0400 Subject: [PATCH 14/14] 144: Fix tests --- .../src/contract/set/PersistentHashSetTest.kt | 42 +++---------------- 1 file changed, 5 insertions(+), 37 deletions(-) diff --git a/core/commonTest/src/contract/set/PersistentHashSetTest.kt b/core/commonTest/src/contract/set/PersistentHashSetTest.kt index cb9c022f..ea86ff70 100644 --- a/core/commonTest/src/contract/set/PersistentHashSetTest.kt +++ b/core/commonTest/src/contract/set/PersistentHashSetTest.kt @@ -35,51 +35,19 @@ class PersistentHashSetTest { * Test from issue: https://github.com/Kotlin/kotlinx.collections.immutable/issues/144 */ @Test - fun reproducer1() { - validate( - firstBatch = listOf( - 0b0_00100_01110_01010, - 0b0_00110_01110_01010, - 0b0_01001_00101_00100, - 0b0_00100_00101_00100 - ), - secondBatch = listOf( - 0b0_01110_01110_01010, - 0b0_00001_00101_00100 - ), - extraElement = - 0b0_00111_01000_11010 - ) - } + fun `removing multiple batches should leave only remaining elements`() { + val firstBatch = listOf(4554, 9380, 4260, 6602) + val secondBatch = listOf(1188, 14794) + val extraElement = 7450 - @Test - fun reproducer2() { - validate( - firstBatch = listOf( - 0b0_00000_00000_00000, - 0b0_00001_00000_00000, - 0b0_00000_00000_00001, - 0b0_00001_00000_00001, - ), - secondBatch = listOf( - 0b0_00010_00000_00000, - 0b0_00010_00000_00001 - ), - extraElement = - 0b0_00000_00000_00010 - ) - } - - private fun validate(firstBatch: List, secondBatch: List, extraElement: Int) { val set = firstBatch.plus(secondBatch).plus(extraElement).toPersistentHashSet() val result = set.minus(firstBatch.toPersistentHashSet()).minus(secondBatch) - assertEquals(1, result.size) assertEquals(extraElement, result.first()) } @Test - fun equalsTest() { + fun `after removing elements from one collision the remaining one element must be promoted to the root`() { val set1: PersistentHashSet = persistentHashSetOf(0, 32768, 65536) as PersistentHashSet val set2: PersistentHashSet = persistentHashSetOf(0, 32768) as PersistentHashSet