Skip to content

Flow.firstOrNull should allow nullable types #2229

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

Closed
ansman opened this issue Sep 4, 2020 · 6 comments
Closed

Flow.firstOrNull should allow nullable types #2229

ansman opened this issue Sep 4, 2020 · 6 comments

Comments

@ansman
Copy link
Contributor

ansman commented Sep 4, 2020

Flow.firstOrNull does not allow nullable types, presumably so that you can differentiate between the flow being empty or not. However, every other firstOrNull function such as List.firstOrNull does allow nullable types and I see no technical reason why the flow version should not allow nullable types.

This change would be binary compatible since the type argument's nullability isn't even reflected in the bytecode. It would also be source compatible since it only loosens the restrictions.

@qwwdfsad
Copy link
Collaborator

It is indeed not allowed to properly differentiate between empty flow and flow with null.
We discussed it at some point and decided that we cannot change the constraint in stdlib, but it would be nice to try to do better in Flow.

We are open to revisiting this decision, but at least requires a compelling use-case apart from technical reasons

@ansman
Copy link
Contributor Author

ansman commented Sep 10, 2020

I feel like being able to differentiate between an empty flow and a null value is a weaker use case then simply wanting the first element of a flow with a nullable type.

My use case is that I often have flows with a nullable type where I want to await the first element. For my purposes I don’t care if the item is null or the flow is empty.

You’ll still be able to differentiate between empty flows and null when using non nullable types and for nullable types you’ll simply have to use first() with a try catch.

@elizarov
Copy link
Contributor

@ansman Can you give a more specific example of the kind of flows of nullable items that you work with, please? What are those? What is the purpose of nulls in your case?

@ansman
Copy link
Contributor Author

ansman commented Sep 10, 2020

One such use case is reading a value from disk. I have a "settings" class that stores keys and values. Observing a key returns a flow with a nullable type since the key might not exist. If there is catastrophic error while reading the settings I simply close the flow rather than throw an exception since for consumers it doesn't matter why, they simply care that there is no key.

I could of course just emit null before closing it but I don't want to overwrite an existing value that might have been sent. Also, if I use send null before closing and use first() someone might one day switch to closing without sending and it would cause consumers to crash in rare cases that would be hard to test.

I feel like wanting the first or null from a flow is a real use case while differentiating between null and empty is a hypothetical use case. Using flows with non null types has the nice side effect of being able to use the return value to check but there is already a better mechanism for this which is calling first() and catching the exception which should be the canonical way to differentiate between null and empty.

@zach-klippenstein
Copy link
Contributor

I strongly disagree that exceptions should be used to detect the empty case. In general, exceptions should not be used for control flow at all, they are expensive and should be used to represent only "exceptional" cases, as the name suggests (e.g. incorrect usage of an API).

I also disagree that using null to detect empty is purely hypothetical. I have also written lots of code (albeit with lists, not flows) that uses firstOrNull (or singleOrNull) in chains of calls with elvis operators where null at any point in the chain is used to represent something like "not found" and terminate the chain.

Your example has some pretty specific concerns, eg that emitting a null at the wrong time could overwrite a previous value. That seems very specific to the use case of a data repository type of API. Also, your reason for not taking the simple approach of emitting null before closing is definitely hypothetical (somebody could one day change the code), and I don't think it's a good justification for not taking that approach: of course someone could change any code some day and break anything - code isn't written in stone. One of the jobs of unit tests is to verify important, load-bearing behavior like that.

I think it makes sense for the stdlib to provide functions that err on the side of caution and safety and don't expose ambiguities. Given the current stdlib function, it might not cover every possible use case but its returning null has a single meaning and it means the same thing in every codebase. You can easily write your own extension function that does what you want and keep the ambiguity scoped to your codebase, where you're willing to accept the potential risk that comes with the ambiguity. However if the stdlib were to include the function you're requesting, that ambiguity would be introduced into every kotlin codebase, and it would be much harder to just look at unfamiliar code and figure out if the null value is being used for both semantics, or was intended to only mean one thing and there's a hidden bug. This is one of the major issues with Java APIs that Kotlin solves, it would be a real shame to go backwards.

@ansman
Copy link
Contributor Author

ansman commented Sep 10, 2020

Firstly I doubt an exception will make any impact on performance in this case. Secondly firstOrNull already uses this mechanism to abort the flow.

You can still write code like that, if you are using non null items. Same as today. My point is that if you need to check if the flow was empty when using nullable types you may use a try-catch to do so.

I think the difference between list's and flow's version is strange and currently there is no way to get the first element for a flow with a nullable type. Making an extension is always possible but naming will be difficult and it will take more cognitive load for the reader to understand why firstOrNull cannot be used.

Making this change will not affect any existing code or new code that uses flows with non null types.

qwwdfsad pushed a commit that referenced this issue Sep 24, 2020
qwwdfsad pushed a commit that referenced this issue Oct 5, 2020
qwwdfsad pushed a commit that referenced this issue Oct 7, 2020
recheej pushed a commit to recheej/kotlinx.coroutines that referenced this issue Dec 28, 2020
* Allow nullable types in Flow.firstOrNull
* Allow nullable types in Flow.singleOrNull
* Align Flow.single and Flow.singleOrNull with Kotlin standard library

Fixes Kotlin#2229
Fixes Kotlin#2289

Co-authored-by: Nicklas Ansman Giertz <[email protected]>
recheej pushed a commit to recheej/kotlinx.coroutines that referenced this issue Dec 28, 2020
* Allow nullable types in Flow.firstOrNull
* Allow nullable types in Flow.singleOrNull
* Align Flow.single and Flow.singleOrNull with Kotlin standard library

Fixes Kotlin#2229
Fixes Kotlin#2289

Co-authored-by: Nicklas Ansman Giertz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants