Fix .toNonEmptyXxxOrNull for nullable types

[mjadczak]

The current implementation of Iterable<A>.toNonEmptyListOrNull() and Iterable<A>.toNonEmptySetOrNull() do not work correctly when A is a nullable type (if the first element of the iterable is null, the function returns null). The implementation also gets the iterator of the iterable more than once. See #3123 .

This PR changes the implementation of both to correctly handle the nullable case, as well as ensuring the iterable is only iterated once. The overload of the latter function with Set<A> as the receiver is also removed, as there is no difference in behaviour or functionality between it and the Iterable overload (and of course a set is iterable).

[nomisRev]

Thank you for the PR, and your first contribution @mjadczak! 🙌

Just one small issue with the binary breaking change, I proposed two potential solutions. Let me know what you think 👍

[nomisRev]

These either need to be annotated with @Deprecated("Same as Iterable operator", DeprecationLevel.HIDDEN) or we can keep them and perhaps get a slightly faster implementation. Otherwise it's going to affect the binary, which is currently failing the PR.

if(it.isNotEmpty()) first().let { NonEmptySet(it, minus(it)) } else null

[mjadczak]

Ah yes, I missed that this would cause a break.

Unfortunately this would not be faster - below is the implementation of minus for Set (I too miss Scala's structural sharing)

public operator fun <T> Set<T>.minus(element: T): Set<T> {
    val result = LinkedHashSet<T>(mapCapacity(size))
    var removed = false
    return this.filterTo(result) { if (!removed && it == element) { removed = true; false } else true }
}

however, given that NonEmptySet does not store its first element separately, unlike NonEmptyList, we could have a fast path for this by adding an internal fun fromSetUnsafe to the companion object and using that here in the case that the set is not empty?

[mjadczak]

Hey @nomisRev, wondering if there's anything needed here from my end before it's good to merge?

[mjadczak]

Ah, I see that Set methods have changed from fun to synthetic fun in the binary API - I'm guessing that's because they now just redirect to another method? I've had a look around but could find no information on this - do you know if there is an attribute or similar to force the compiler to keep the explicit method?

[nomisRev]

Ah, I see that Set methods have changed from fun to synthetic fun in the binary API

I think that is because of the HIDDEN, which shouldn't be a problem 🤔 @serras any idea? I cannot find anything in the documentation https://github.com/Kotlin/binary-compatibility-validator.

To merge we should get a second review, if we didn't get one this week I think we can merge. I am going to ask around for the synthetic on Slack.

[serras]

My understanding is that synthetic does not affect ABI compatibility, the JVM doesn’t see them differently. The difference is that Java (the language) doesn’t “see” the elements marked as synthetic, but that shouldn’t be a problem here either.

[serras]

@mjadczak could you please run ./gradlew apiDump on this (updated) branch and commit the results? The automic API updater doesn't work on forks.

[i-walker]

🙌🏾

[mjadczak]

@mjadczak could you please run ./gradlew apiDump on this (updated) branch and commit the results? The automic API updater doesn't work on forks.

Sorry, I had missed your message - looks like you did this, thank you very much!