Skip to content

Update code to prepare for nullness annotations in Guava. #3026

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 18, 2022
Merged

Update code to prepare for nullness annotations in Guava. #3026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion integration/kotlinx-coroutines-guava/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,6 @@ Integration with Guava [ListenableFuture](https://github.com/google/guava/wiki/L

<!--- INDEX com.google.common.util.concurrent -->

[com.google.common.util.concurrent.ListenableFuture]: https://kotlin.github.io/kotlinx.coroutines/https://google.github.io/guava/releases/28.0-jre/api/docs/com/google/common/util/concurrent/ListenableFuture.html
[com.google.common.util.concurrent.ListenableFuture]: https://kotlin.github.io/kotlinx.coroutines/https://google.github.io/guava/releases/31.0.1-jre/api/docs/com/google/common/util/concurrent/ListenableFuture.html

<!--- END -->
7 changes: 6 additions & 1 deletion integration/kotlinx-coroutines-guava/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,17 @@
* Copyright 2016-2021 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
*/

val guavaVersion = "28.0-jre"
val guavaVersion = "31.0.1-jre"

dependencies {
compile("com.google.guava:guava:$guavaVersion")
}

java {
targetCompatibility = JavaVersion.VERSION_1_8
sourceCompatibility = JavaVersion.VERSION_1_8
}

externalDocumentationLink(
url = "https://google.github.io/guava/releases/$guavaVersion/api/docs/"
)
10 changes: 4 additions & 6 deletions integration/kotlinx-coroutines-guava/src/ListenableFuture.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,8 @@ public fun <T> ListenableFuture<T>.asDeferred(): Deferred<T> {
// Finally, if this isn't done yet, attach a Listener that will complete the Deferred.
val deferred = CompletableDeferred<T>()
Futures.addCallback(this, object : FutureCallback<T> {
override fun onSuccess(result: T?) {
// Here we work with flexible types, so we unchecked cast to trick the type system
@Suppress("UNCHECKED_CAST")
runCatching { deferred.complete(result as T) }
override fun onSuccess(result: T) {
runCatching { deferred.complete(result) }
.onFailure { handleCoroutineException(EmptyCoroutineContext, it) }
}

Expand Down Expand Up @@ -351,7 +349,7 @@ private class JobListenableFuture<T>(private val jobToCancel: Job): ListenableFu
*
* To preserve Coroutine's [CancellationException], this future points to either `T` or [Cancelled].
*/
private val auxFuture = SettableFuture.create<Any>()
private val auxFuture = SettableFuture.create<Any?>()

/**
* `true` if [auxFuture.get][ListenableFuture.get] throws [ExecutionException].
Expand Down Expand Up @@ -436,7 +434,7 @@ private class JobListenableFuture<T>(private val jobToCancel: Job): ListenableFu
}

/** See [get()]. */
private fun getInternal(result: Any): T = if (result is Cancelled) {
private fun getInternal(result: Any?): T = if (result is Cancelled) {
throw CancellationException().initCause(result.exception)
} else {
// We know that `auxFuture` can contain either `T` or `Cancelled`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't the result here be null now? In which case, getInternal should also return a nullable value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auxFuture.set is called in 2 places:

(That appears to match this comment.)

It's true that T might be a nullable type in some cases, so the result might be null. But that's OK: The result can be null only if it came through complete(result: T). And that can happen only in the case that T is a nullable type.

Put another way, complete and getInternal combine to implement a system that accepts a T and produces a T. It just happens to store the T as an Any? in the meantime because it sometimes wants to store a Cancelled instead.

More broadly: This PR shouldn't change any runtime behavior. result: T could always be a null value. But once Guava is fully annotated for nullness (maybe in Guava 32.0?), the existing auxFuture.set(result) call will stop working: At that point, Kotlin will not only know that result: T could be a null value but also know that auxFuture.set should not be called with a null value because its parameter type is Any. This PR changes auxFuture so that auxFuture.set accepts an Any? and the code continues to compile. But it doesn't change the values that were already possible.

Expand Down