Implement basic NonEmptySet

[ibado]

This is a super basic implementation for #2565. After review, if this is fine, I can add any other functionality may be required.

[serras]

Nice job! I think this just need a few more tests to ensure that the implementation is correct even if somebody else changes or tries to optimize it even further.

[serras]

@ibado I'm wondering whether the first + rest separation is really needed apart from the constructor. More concretely, I'm proposing to make NonEmptySet simply a wrapper for Set, but only expose the right constructor:

@JvmInline public value class NonEmptySet<out T> private constructor(
  val elements: Set<T>
): AbstractSet<T> {
  public constructor(first: T, varargs rest: T): this(setOf(first) + rest.toSet())
}

You can still overload the plus operator to return NonEmptySet, but you already know that's the case if one of the arguments is NonEmptySet.

  public operator fun plus(s: Set<@UnsafeVariance T>): NonEmptySet<T> =
    NonEmptySet(elements + s)

This has the added benefit of completely removing the wrapper at runtime, since it's a value class.

@roomscape what do you think?

[roomscape]

@roomscape what do you think?

I think you're right, the first/rest separation may not be needed outside of the constructor. An efficient implementation will always need to create a Set to implement the required operations. Including the first item alongside the others in that set makes the implementation simpler, and I can't actually think of any functionality that would benefit from keeping one item separate.

In that case, your suggestion of a simple wrapper class works very well. Interface delegation would be a good fit:

class NonEmptySet<out T> private constructor(elements: Set<T>): Set<T> by elements

I like the idea of making it an inline class too, but I'm cautious about the implications, and I think the benefits would only be slight. Value classes can use interface delegation, so that would work. But value classes do seem to have some quirks, and in particular I think generic parameters are still experimental. It might be safer to leave that for a future enhancement.

[ibado]

I uploaded a simplify version following the approach of @roomscape since I think is nice to delegate the "set" capabilities to the wrapped set. Not sure about the inline value class.. It's up to you.
Please take a look and LMK 🙂
P.S: the overrides for plus are necessary for returning back a NonEmptySet

[serras]

@ibado I've been working on a similar PR for NonEmptyList, and I've realized that you need to override equals if you want NonEmptySet to be comparable with Set. This commit shows how.

Once you do this, you can really change to a value class, which removes any wrapper at runtime:

@JvmInline public value class NonEmptySet<out T> private constructor(
  private val elements: Set<T>
) : Set<T> by elements {

[ibado]

@serras Done! the equals was already there, but I was not checking for NonEmptySet.
Still not sure how stable value classes are but what a hell 🤣

[ibado]

Now :arrow-core:compileTestKotlinJvm is failing 🙃 Looks that also your PR. Something about IR

[serras]

@ibado I think the commit I've just pushed should fix the problem (it's just a different way to write the generator)

[nomisRev]

Looks great @ibado! Thank you and congrats on your first contribution 🙌 🥳

Small styling nit.

I think this just need a few more tests to ensure that the implementation is correct

This would also be nice, you can run ./gradlew koverHtmlReport to check the coverage of NonEmptySet.

[serras]

@ibado congrats on your first contribution! Thanks for your work here, and for ensuring the best quality for this new implementation :)

[ibado]

Thanks! Happy to help 😄