Avoid giving the result of loads a non-null type. #1805
Conversation
ben-manes@2542ca8 changed types like the return type of `Cache.get` to be non-null instead of unspecified. As discussed in ben-manes#594, the types really "should" be nullable, but that is inconvenient for most users. When Caffeine was using Checker Framework annotations, it could use `@PolyNull` to give Checker Framework users the best of both worlds and give users of other tools (notably Kotlin) a convenient (if not fully strict) experience, since those tools viewed the types as having unspecified nullness (in Kotlin terms, "platform types"). Caffeine could still use `@PolyNull` today if you want to restore the Checker Framework dependency. (And maybe someday JSpecify will include its own version, as discussed in jspecify/jspecify#79.) But the important thing is that Caffeine _not_ affirmatively make the type be non-null. And that's what `@NullMarked` does. That would cause trouble for a number of Kotlin users in Google's codebase. So, to undo that, we can use `@NullUnmarked` (recognized by Kotlin as of 2.0.20) on the methods where we want to keep types unspecified. Then we can optionally use `@NonNull` annotations on the _other_ types in the API in order to still make those types non-null. Kotlin actually _still_ doesn't quite get the types right, thanks to https://youtrack.jetbrains.com/issue/KT-73658. But this PR at least annotates correctly (I hope :)), which may help other tools even today and which should help Kotlin eventually. If enough people run into the Kotlin bug in practice, then we could consider backing out even more nullness information: If we were to remove `@NullMarked` from `Cache`+`AsyncCache` and then potentially sprinkle in some `@NonNull` annotations to compensate, then we could still preserve at least some of the new information in those classes. And of course other classes can remain `@NullMarked`.
|
|
caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java
Show resolved
Hide resolved
caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java
Outdated
Show resolved
Hide resolved
caffeine/src/main/java/com/github/benmanes/caffeine/cache/AsyncCache.java
Show resolved
Hide resolved
|
Feel welcome to merge when you are comfortable with the changes. |
|
Thanks for the review, and thanks for giving the JSpecify annotations a spin in the first place! It looks like I may end up wanting to touch |
|
I have no preference, and appreciate the thoughtfulness in the contribution. |
|
Thanks. I will be waiting to hear back from our open-source-licensing people in any case. Hopefully the process will be relatively quick, since (I think you've said) your CLA is basically our CLA :) |
|
It is just the Apache Foundation's, which merely reiterates the license as a notice for an optional explicit agreement instead of an implicit one through the act of contributing. Its entirely optional if not signing is preferable.
https://www.apache.org/licenses/cla-faq.html#cclas-not-required |
This makes them match what `CacheLoader` already allows for synchronous loading and reloading via `@Nullable V load(K key)` and `@Nullable V reload(K key, V oldValue)`.
|
Thanks. If the licensing people take a while to get back to me, then I can merge without signing. I've added the changes to the loader classes. I'll probably make sure that I have all our depot's tests passing before I merge anything here. (I could believe that there are other types in Caffeine that could still benefit from annotations, especially more of those found in type arguments. I'm not currently planning to look proactively, but we'll see if any more come to my attention or if I go looking for a distraction :)) |
|
I found a few more, not through build/test failures but just through further examination. And I was doing the further examination because I am trying to get together an alternative approach: We could allow the various cache classes to have a nullable value type. That wouldn't mean that the caches could "contain" In the meantime, I'm testing the latest version of this PR :) |
|
oh that sounds pretty amazing if it can work that way! If helpful, the examples section acts as integration tests using separate builds. This way I could avoid complexity in the main build, like Graal AOT set up, by isolating those tooling scenarios. Then I write it up under the guise of a tutorial, somewhat to help me remember how it works, and I have regression test that can run against the locally built jar in CI. That approach might be attractive to you if nullability is currently best verified in Kotlin so that I don't mangle that in the future. |
|
Great! My new attempt at annotating seems to be working well: It requires fewer updates to users (not that I needed to make too many changes for your original change plus this PR, but less is nice), and it ensures that anyone who passes a null-returning loader will have to handle null during cache operations. It's passing all our tests. Meanwhile, I did get this PR passing all our tests, as well. My plan is to merge this one (to at least immediately eliminate the non-null types that were my main worry) and then create another PR that shows my new attempt as a diff against this one. (I even got approval for the CLA :)) |
|
Sorry, I just noticed that the default is "Merge pull request" but that the Caffeine history looks linear, at least recently. It might have been better for me to either squash or rebase. |
This does not mean that the cache will "contain" `null`, since `null` is never cached. However, it does allow users to declare whether they want "a cache whose loads might return `null`" (which thus can return `null` from cache operations) or "a cache whose loads will never return `null`" (which thus doesn't require users to handle `null` when they request that the cache return or load a value). This PR follows up on ben-manes#1805. Ideally I will come back to write some Kotlin tests for this. For the moment, I can only say that the results look good in my testing inside Google.
|
No problem. I disabled the other options going forward so squash is required. There is no default, only the last option selected by the user. |
…1806) This does not mean that the cache will "contain" `null`, since `null` is never cached. However, it does allow users to declare whether they want "a cache whose loads might return `null`" (which thus can return `null` from cache operations) or "a cache whose loads will never return `null`" (which thus doesn't require users to handle `null` when they request that the cache return or load a value). This PR follows up on #1805.
2542ca8
changed types like the return type of
Cache.getto be non-null insteadof unspecified.
As discussed in #594, the
types really "should" be nullable, but that is inconvenient for most
users.
When Caffeine was using Checker Framework annotations, it could use
@PolyNullto give Checker Framework users the best of both worlds andgive users of other tools (notably Kotlin) a convenient (if not fully
strict) experience, since those tools viewed the types as having
unspecified nullness (in Kotlin terms, "platform types").
Caffeine could still use
@PolyNulltoday if you want to restore theChecker Framework dependency. (And maybe someday JSpecify will include
its own version, as discussed in
jspecify/jspecify#79.)
But the important thing is that Caffeine not affirmatively make the
type be non-null. And that's what
@NullMarkeddoes. That would causetrouble for a number of Kotlin users in Google's codebase.
So, to undo that, we can use
@NullUnmarked(recognized by Kotlin as of2.0.20) on the methods where we want to keep types unspecified. Then we
can optionally use
@NonNullannotations on the other types in theAPI in order to still make those types non-null.
Kotlin actually still doesn't quite get the types right, thanks to
https://youtrack.jetbrains.com/issue/KT-73658. But this PR at least
annotates correctly (I hope :)), which may help other tools even today
and which should help Kotlin eventually.
If enough people run into the Kotlin bug in practice, then we could
consider backing out even more nullness information: If we were to
remove
@NullMarkedfromCache+AsyncCacheand then potentiallysprinkle in some
@NonNullannotations to compensate, then we couldstill preserve at least some of the new information in those classes.
And of course other classes can remain
@NullMarked.