Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Iterable<A>.toNonEmptyListOrNull() is broken for nullable A #3123

Closed
mjadczak opened this issue Sep 1, 2023 · 2 comments
Closed

Iterable<A>.toNonEmptyListOrNull() is broken for nullable A #3123

mjadczak opened this issue Sep 1, 2023 · 2 comments

Comments

@mjadczak
Copy link
Contributor

mjadczak commented Sep 1, 2023

The current implementation of this function is as follows:

public fun <A> Iterable<A>.toNonEmptyListOrNull(): NonEmptyList<A>? =
  firstOrNull()?.let { NonEmptyList(it, drop(1)) }

but when A is a nullable type, and the first element of the list is null, the function incorrectly returns null!

It's also probably not best practice to iterate the iterable twice, just in case it cannot be iterated multiple times, or it is costly to do so.

Here is a proposed replacement, which is admittedly less pretty, but doesn't have the same issue and only iterates the iterable once:

public fun <A> Iterable<A>.toNonEmptyListOrNull(): NonEmptyList<A>? {
    val iter = iterator()
    if (!iter.hasNext()) return null
    return NonEmptyList(iter.next(), Iterable { iter }.toList())
}
@nomisRev
Copy link
Member

nomisRev commented Sep 4, 2023

Hey @mjadczak,

Thank you for this bug report! 🙌 Ouch, nullability problems 🙈 So easy to sneak through when not using Option in generic context.

If you're interested in contributing this could be a great contribution!

mjadczak pushed a commit to mjadczak/arrow that referenced this issue Sep 7, 2023
This also ensures that we iterate the iterable only once.

This also removes the Set<A> receiver versions of .toNonEmptySetOrNull()
and .toNonEmptySetOrNone()
@mjadczak
Copy link
Contributor Author

mjadczak commented Sep 7, 2023

@nomisRev submitted as #3127 - my first PR so please let me know if there's some extra bits I need to run (not sure if the various format and ABI checking runs as part of the normal CI)

serras added a commit that referenced this issue Oct 26, 2023
* Add failing tests

For case when .toNonEmptyListOrNull and .toNonEmptySetOrNull are called
on an iterable starting with null

* Fix .toNonEmptyXxxOrNull for nullable types (#3123)

This also ensures that we iterate the iterable only once.

This also removes the Set<A> receiver versions of .toNonEmptySetOrNull()
and .toNonEmptySetOrNone()

---------

Co-authored-by: Matt Jadczak <m.jadczak@mwam.com>
Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
Co-authored-by: Alejandro Serrano <trupill@gmail.com>
@serras serras closed this as completed Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants