Add persistent Collection builder functions by Goooler · Pull Request #166 · Kotlin/kotlinx.collections.immutable

Goooler · 2023-12-29T03:47:18Z

Closes #137.

qurbonzoda

Documentation for the functions should be amended. Otherwise, LGTM.

qurbonzoda · 2024-01-05T09:42:57Z

+ * The list passed as a receiver to the [builderAction] is valid only inside that function.
+ * Using it outside the function produces an unspecified behavior.


Builders in kotlinx.collections.immutable are capable of constructing a new persistent collection multiple times. However, I'm struggling to identify a compelling use case for retaining a reference to the builder and utilizing it outside the builderAction. If we constraint the builder to be valid only within the function, could using a MutableList as the receiver be a more performance-efficient choice?

cc @ilya-g, WDYT?

It feels like if that were the case, mutate would already be optimizing this in the same way, no?

Goooler · 2024-02-27T03:31:30Z

+@OptIn(ExperimentalTypeInference::class, ExperimentalContracts::class)
+public inline fun <T> buildPersistentList(@BuilderInference builderAction: PersistentList.Builder<T>.() -> Unit): PersistentList<T> {
+        contract { callsInPlace(builderAction, InvocationKind.EXACTLY_ONCE) }
+        return persistentListOf<T>().builder().apply(builderAction).build()
+}


Annotate @BuilderInference for better generic type support.

Add InvocationKind.EXACTLY_ONCE contract to indicate the function parameter will be invoked exactly one time.

Ref https://github.com/JetBrains/kotlin/blob/3ff2bc403cb64cee273833d4dbbaf6d8c928edbc/libraries/stdlib/src/kotlin/collections/Collections.kt#L180-L187

AFAIK @BuilderInference is a no-op, and has been for a while now. My understanding is that builder inference is applied by default everywhere

I didn't remove it to keep the same behaviors as the builders in stdlib.

Removed now.

The opt-in for ExperimentalTypeInference::class is unnecessary now

kyay10 · 2025-05-24T23:20:32Z

+@OptIn(ExperimentalTypeInference::class, ExperimentalContracts::class)
+public inline fun <T> buildPersistentList(@BuilderInference builderAction: PersistentList.Builder<T>.() -> Unit): PersistentList<T> {
+        contract { callsInPlace(builderAction, InvocationKind.EXACTLY_ONCE) }
+        return persistentListOf<T>().builder().apply(builderAction).build()


Could use mutate instead. Same for the other builder functions

Good catch! Replaced.

kyay10 · 2025-05-25T02:04:14Z

It looks like you lost the InvocationKind.EXACTLY_ONCE contract while refactoring. Could you add it back in please?

Goooler · 2025-05-25T02:06:37Z

Leaked in-place lambda 'builderAction: MutableList.() -> Unit'.
Wrong invocation kind 'EXACTLY_ONCE' for 'builderAction: MutableList.() -> Unit' specified, the actual invocation kind is 'AT_MOST_ONCE'.

kyay10 · 2025-05-25T02:08:34Z

Oooo, looks like mutate might have an incorrect contract then! I think it'd be reasonable to add contracts to mutate while you're at it, since it's in the same file, and does very similar things to the builders

Goooler · 2025-05-25T02:15:59Z

kotlinx.collections.immutable/core/commonMain/src/extensions.kt

Line 41 in bd5ac10

    
           public inline fun <T> PersistentList<T>.mutate(mutator: (MutableList<T>) -> Unit): PersistentList<T> = builder().apply(mutator).build()

builder and apply are applied for mutate, and I see the contract is already on the apply:

@kotlin.internal.InlineOnly
public inline fun <T> T.apply(block: T.() -> Unit): T {
    contract {
        callsInPlace(block, InvocationKind.EXACTLY_ONCE)
    }
    block()
    return this
}

kyay10 · 2025-05-25T02:19:25Z

Yes, but contracts currently aren't inferred nor inherited automatically like this, so sadly you have to explicitly add the contract to mutate. If you will, contracts are part of the API surface.

Goooler · 2025-05-25T02:22:01Z

I believe we should add contracts in a separate PR.

This comment was marked as outdated.

Sign in to view

Goooler mentioned this pull request Dec 30, 2023

Add buildPersistentList { }, buildPersistentMap { }, buildPersistentSet { } to match the builders in kotlin stdlib #137

Open

Goooler force-pushed the add-build-extensions branch from fe58e7d to bc52615 Compare January 4, 2024 15:48

qurbonzoda self-requested a review January 4, 2024 17:49

qurbonzoda suggested changes Jan 4, 2024

View reviewed changes

Comment thread core/commonMain/src/extensions.kt Outdated

Comment thread core/commonMain/src/extensions.kt Outdated

Goooler force-pushed the add-build-extensions branch from bc52615 to 20ad7a1 Compare January 4, 2024 18:28

qurbonzoda requested a review from ilya-g January 5, 2024 09:14

qurbonzoda reviewed Jan 5, 2024

View reviewed changes

Goooler force-pushed the add-build-extensions branch 2 times, most recently from 62dfb51 to c90d590 Compare January 18, 2024 10:30

Goooler force-pushed the add-build-extensions branch from c90d590 to 1ac235e Compare February 27, 2024 03:27

Goooler commented Feb 27, 2024

View reviewed changes

Goooler force-pushed the add-build-extensions branch from 1ac235e to f4b5f15 Compare September 4, 2024 20:07

kyay10 reviewed May 24, 2025

View reviewed changes

Goooler force-pushed the add-build-extensions branch 2 times, most recently from c2ade34 to 505c5c2 Compare May 25, 2025 01:59

Goooler force-pushed the add-build-extensions branch from 505c5c2 to d0eac5a Compare May 25, 2025 02:13

Add persistent Collection builder functions

16df77e

Goooler force-pushed the add-build-extensions branch from d0eac5a to 16df77e Compare May 25, 2025 02:24

		* The list passed as a receiver to the [builderAction] is valid only inside that function.
		* Using it outside the function produces an unspecified behavior.

Conversation

Goooler commented Dec 29, 2023

Uh oh!

This comment was marked as outdated.

qurbonzoda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Goooler Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyay10 commented May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Goooler commented May 25, 2025

Uh oh!

kyay10 commented May 25, 2025

Uh oh!

Goooler commented May 25, 2025

Uh oh!

kyay10 commented May 25, 2025

Uh oh!

Goooler commented May 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Goooler Feb 27, 2024 •

edited

Loading

kyay10 commented May 25, 2025 •

edited

Loading