Kotlin / kotlinx.collections.immutable

Immutable persistent collections for Kotlin
Apache License 2.0
1.16k stars 59 forks source link

immutable*Of(…) functions shouldn't be deprecated, and should return Immutable* instead of Persistent* #62

Open rgoldberg opened 4 years ago

rgoldberg commented 4 years ago

immutable*Of(…) functions shouldn't be deprecated.

They should return Immutable* instead of Persistent*.

Currently, e.g.:

val x = immutableSetOf(1, 2, 3)
val y: ImmutableSet<Int> = immutableSetOf(1, 2, 3)

// x is PersistentSet<Int>
// y is ImmutableSet<Int>

x should be an ImmutableSet<Int>, but is a PersistentSet<Int>.

If I want an Immutable*, why force me to write an explicit type?

It's not like immutable*Of(…) functions are difficult to create or maintain…

tir38 commented 4 years ago

I feel like this is only half the solution. You can still cast y back to PersistentSet:

val x = immutableSetOf(1, 2, 3)
val xAdd = x.add(4)
assertThat(xAdd.size).isEqualTo(4)

val y: ImmutableSet<Int> = immutableSetOf(1, 2, 3)
val yAdd: ImmutableSet<Int> = (y as PersistentSet).add(4)
assertThat(yAdd.size).isEqualTo(4)

I feel that what's really needed is truly-immutable implementations of ImmutableSet etc.

ilya-g commented 4 years ago

You can still cast y back to PersistentSet

That's true but what's the problem with that?

rgoldberg commented 4 years ago

I feel that what's really needed is truly-immutable implementations of ImmutableSet etc.

PersistentSets are immutable, thus there already is a truly-immutable implementation.

tir38 commented 4 years ago

I'm coming back to this and now I can't see what I was arguing for 🤷 . Neither x nor y were mutated by .add in my code sample. So forget what I said.

But I agree with OP. If I want an immutable set (i.e. no .add() method) then I should get one by calling immutableSetOf w/out having to cast.

It seems weird that I have to take an extra step (i.e. cast) to get what I asked for. I would expect that I'd have to take an extra step (cast) to get something extra: an add() method

Sporking commented 1 year ago

I agree with the OP also. I recently had a project that needed immutable, but not persistent collections. Specifically, I needed to make defensive copies of some data structures to "freeze" them so I could be sure they weren't modified further, either by the code that received them or by the code that provided them. These frozen instances wouldn't be updated further after that point, only accessed and iterated over, so persistent update features and efficiently creating "modified copies of" the data was not required. Retrieval speed, however, was important.

I considered using Guava (which I have usually used in the past for this sort of thing) but was displeased at how different its Java-inspired syntax was from from standard Kotlin syntax (e.g. ImmutableList.of(...) instead of immutableListOf(...)). I was pleased to see that the kotlinx.collections library supported both immutable data structures (which I need) and persistent structures (which I don't, at least for the time being) and seemed to understand the difference between those. So I downloaded the latest version of the kotlinx.collections library.

It overall seemed to be looking good (missing some useful data structures like ImmutableSortedSet, etc. though), but then I noticed that my uses of "immutableListOf" were underlined with deprecation warnings in Idea, with suggestions that I should call persistentListOf instead. This is annoying. I want an immutable list: an exact copy of a mutable list (with identical performance guarantees if possible, e.g. O(1) get/set!) like Guava would get me, but with Kotlin syntax. The natural way to get that would be with a function like "immutableListOf", similar to "mutableListOf" that is widely used in Kotlin.

If your library happens to implement immutability using a persistent list under the covers for the time being, that's fine (for now) as long as it works, but I don't want to write my code to assume that it's implemented that way. If I did, I would be constraining your library in its ability to come up with a better (more performant, more memory efficient, etc.) implementation of immutable lists or sets or whatever in a future release where these structures may not actually share code with your persistent data structures anymore. I don't want to potentially sacrifice performance such as O(1) get/set on list elements just to get persistent update features which I won't ever need to use!

Please un-deprecate immutableListOf (and similar functions), and have their return types be ImmutableList (and similar classes). I would like to be able to use your immutable data structures without in any way creating explicit dependencies in my code on your persistent data structures. This separation of interface and implementation will free your library developers to come up with better implementations later without affecting users' existing code.

Also, in general, please avoid having users pay performance penalties to get persistence in situations where only immutability (rather than persistence) is actually needed and requested. If you can make a data structure that is ideally performant and is both immutable and persistent, that's great, but if you can't (and very likely you won't always be able to), if a user only requests immutability, then that user should get the fastest/most-memory-efficient immutable data structure, optimized for reads (not writes or modified copies) regardless of whether it supports persistent updates optimally or not.

spacemase commented 1 year ago

I desire only immutability. so 100 👍 's to this.

Sporking commented 9 months ago

Any updates on this? This issue seems very easy to fix, with no obvious downsides (at least none that have been explained here so far), and is four years old. There have been at least two releases since this was filed. Is there some reason why this hasn't been fixed yet?

valeriyo commented 1 week ago

Yes, please un-deprecate immutable methods! AND return Immutable types!