Update code to prepare for nullness annotations in rxjava3. #3007
Conversation
There was a problem hiding this comment.
Hi! Thanks for the PR. Do I understand correctly that the changes to the RxJava3 module are completely disjoint from the changes to the Guava integration? If so, it would probably be better if these were separated into different pull requests so we could merge them independently.
In any case, please change the base of this PR to the develop branch (see https://github.com/Kotlin/kotlinx.coroutines/blob/master/CONTRIBUTING.md#submitting-prs). Also, if you agree with the reasoning that the fix to the RxJava3 module is premature, you could only rebase the changes to the Guava module.
| @@ -42,7 +42,7 @@ public suspend fun CompletableSource.await(): Unit = suspendCancellableCoroutine | |||
| * function immediately resumes with [CancellationException] and disposes of its subscription. | |||
| */ | |||
| @Suppress("UNCHECKED_CAST") | |||
| public suspend fun <T> MaybeSource<T>.awaitSingleOrNull(): T? = suspendCancellableCoroutine { cont -> | |||
| public suspend fun <T : Any> MaybeSource<T>.awaitSingleOrNull(): T? = suspendCancellableCoroutine { cont -> | |||
There was a problem hiding this comment.
tl;dr: we'll probably just make this compile somehow, and only make the proposed change after 1.7.
Adding T : Any boundaries is a good change. In fact, it's so good that it's the third time it's suggested! The reason our earlier attempts failed is that this would break a lot of code generic in T that depends on this library: see #2630. It looks like the change to the compiler will force our hand, but the general concern still stands: we don't want to introduce breaking changes that aren't that useful.
Due to this, I think the proper thing to do here is to make the code compile without introducing any incompatibility. When people migrate their code to 1.7, they will in any case have to adapt their code a lot, including the generic functions that we don't want to break. After they fix this on their side, we'll be able to remove the hacks that we'll need for the code to compile, and without breaking any code.
What do you think? I didn't understand from your description whether you actually need this change already for some reason.
There was a problem hiding this comment.
By the way, there is a way to workaround that: #2954
It's not currently merged due to IDE issues
There was a problem hiding this comment.
Essentially, my goal is to break a bunch of Google's code that depends on this library :)
Of course, I don't really want to break our code. But as you say, I expect for us to have to break it eventually. So I would rather break (well, fix) a small amount of code now than I would break (fix) a large amount of code later.
To be fair, this is a particularly good time for a breaking change for us: We don't have a ton of RxJava3 code yet, but we're expecting to start seeing a lot more soon. So it was pretty easy to update our existing users of these APIs a couple weeks ago, but already I'm seeing backsliding.
I understand that, in the wider community, these APIs already have far more users. Still, I think the principle stands: Updating users of the APIs is only going to get harder over time.
It would be great if we could avoid the breaking change entirely with #2954. However, I suspect that that's not going to work in the long term: Even an API declaration like public suspend fun <T> SingleSource<T>.await() is going to become an error eventually, since SingleSource requires a non-null type argument. So I think that even that API has to change to public suspend fun <T> SingleSource<T & Any>.await(). And I think that's as much a breaking change as this PR is.
I may be able to mostly accomplish my goal another way: If I can set the @io.reactivex.rxjava3.annotations:strict flag for the rest of our codebase, then I may be able to prevent callers from ever using these APIs with a nullable type argument. As it turns out, though, that's tricky because, if I set that flag for our whole codebase, we can no longer compile this project :) And our build setup / kotlinc doesn't appear to provide a good way to turn a flag "off" if it's on by default, so I can't easily turn it off just for this project. I can continue exploring ways to make that work, too; I'm just trying as many different approaches as I can.
There was a problem hiding this comment.
Two small updates on the breaking-changes front:
- First, if you ultimately find this PR to be too disruptive, it looks like we'll probably maintain it as a small patch for Google's copy. That unblocks my goal of flipping [email protected]:strict on for our depot. (I very much appreciate your catching my earlier errors so that I didn't break something, even if it would only have been in Google's code and not for all rxjava3 users :))
- We've found that another future Kotlin flag flip, -Xenhance-type-parameter-types-to-def-not-null=true, forces our rxjava3 users to make some (but not all) of the same changes as this PR does. I don't know if that flag and the rxjava3.annotations flag will end up getting flipped by default in the same release of Kotlin or different releases. Nor do I know whether users would prefer to take all their rxjava3-related breakages at once or have multiple smaller breaking releases. But I wanted to pass along that that's another Kotlin future flag flip that may break some kotlinx-coroutines users.
|
Thanks for the quick response. I should have thought to look for history, and sorry for getting distracted by some build issues and not reading the documentation about the branch to work on, etc. :( I will rebase, split the Guava change out, read some more docs, see if I can find some more background on #2630, and get back to you -- hopefully later this week, but I have overcommitted myself a bit. |
|
Hey, no worries. The history for this is not that easy to find publically, I was simply describing the current state of things. Also, where I'm from, there are public holidays at the end of this week, so I wouldn't be able to respond until Monday anyway. |
(In addition to keeping the code compiling in the future, this change should make some runtime nullness errors impossible.) rxjava3 nullness annotations don't yet trigger Kotlin compile errors, but that will be changing in Kotlin 1.7: https://github.com/JetBrains/kotlin/blob/05822c59b516b6d252bd6d27e9032e660e15b625/core/compiler.common.jvm/src/org/jetbrains/kotlin/load/java/JavaNullabilityAnnotationSettings.kt#L42-L46 We can preview the behavior by passing [email protected]:strict: https://kotlinlang.org/docs/java-interop.html#nullability-annotations We additionally set -Xtype-enhancement-improvements-strict-mode so that the Kotlin compiler looks at type-use annotations on type arguments, type parameters, etc.: https://kotlinlang.org/docs/java-interop.html#annotating-type-arguments-and-type-parameters Usually, the required update is to restrict a type parameter to non-nullable types, since most rxjava types do not support null type arguments. In a few cases, the update is to change an unnecessarily nullable type to be non-nullable. Finally, I removed a `value == null` check in `RxMaybeCoroutine.onCompleted` that the compiler now identifies as unnecessary. (I can keep it if you'd prefer.) I should warn you that I have very little understanding of coroutines and of this library. I'm here because we're seeing compile errors inside Google as we work to improve how we handle Kotlin-Java interoperability, and these changes looked like they might be the right fixes. Sorry for any mistakes.
cpovirk
changed the title
|
OK, 6 months later, I finally made some time to try to understand things instead of blindly changing Bookkeeping:
That leaves only(?) the whole question of whether to make breaking changes now at all, as discussed in the other thread. |
|
I realized that I never hit "Re-request review." Again, though, I know you'd already indicated that you probably wouldn't merge this PR anytime soon (possibly never, in fact :)), so I'm hitting the button just in case you were waiting on me to do that. I think I've addressed the smaller comments, leaving only the big "Should we do this at all?" question. And as I said above, I expect to be able to patch Google's local copy of kotlinx.coroutines if you don't want to merge this. |
|
Hi! We are not yet decided what to do. As soon as we merge #3324 (which is blocked by 1.7.0 regression, so we are waiting for 1.7.10) where definitely-non-nullable types are available, we'll start investigating the compatibility story, that mostly lies within Java interop and flexible types. We'll figure the final shape out before 1.8.0 where Rx3 annotations are strict. |
|
Superseded by #3393 |
(In addition to keeping the code compiling in the future, this change should make some runtime nullness errors impossible.)
rxjava3 nullness annotations don't yet trigger Kotlin compile errors, but that is scheduled to change in Kotlin
1.71.8.We can preview the behavior by passing
[email protected]:strict.We additionally set
-Xtype-enhancement-improvements-strict-modeso that the Kotlin compiler looks at type-use annotations on type arguments, type parameters, etc.Usually, the required update is to restrict a type parameter to non-nullable types, since most rxjava types do not support null type arguments. In a few cases, other changes are required.
I should warn you that I have very little understanding of coroutines and of this library. I'm here because we're seeing compile errors inside Google as we work to improve how we handle Kotlin-Java interoperability, and these changes looked like they might be the right fixes. Sorry for any mistakes.