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

Allow caches to have nullable type arguments for their value types. #1806

Merged
merged 5 commits into from
Dec 10, 2024

Conversation

cpovirk
Copy link
Collaborator

@cpovirk cpovirk commented Dec 5, 2024

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.

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.

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.
See
https://github.com/ben-manes/caffeine/actions/runs/12181931315/job/33979716621?pr=1806

```
Warning: /home/runner/work/caffeine/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/CacheLoader.java:233: warning: [NullAway] method returns @nullable, but superclass method com.github.benmanes.caffeine.cache.CacheLoader.load(K) returns @nonnull
      @OverRide public @nullable V load(K key) {
                                   ^
    (see http://t.uber.com/nullaway )
  Did you mean '@SuppressWarnings("NullAway") @OverRide public @nullable V load(K key) {'?
error: warnings found and -Werror specified
```
Copy link
Owner

@ben-manes ben-manes left a comment

Choose a reason for hiding this comment

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

Nice!

When adopting jspecify, I had forgotten that tests disabled NullAway. It probably makes sense for me to revisit that as a safety net for reviewing nullability changes.

nullaway {
if (name.contains("Test") || name.contains("Jmh")) {
disable()
}

@cpovirk
Copy link
Collaborator Author

cpovirk commented Dec 5, 2024

I'll come back to have a look at the JCache error. I'm hoping that I'll just need to twiddle some annotations, but it may be that we are running up against the frontier of what NullAway can do for generics so far.

@ben-manes
Copy link
Owner

I'm unsure why it only failed on one compile and not elsewhere, where it somehow decided it was eligible to retrieve it from the build cache. The build uses the common env variable CI=true to enable -Werror so that local dev builds can observe all issues rather than fail eagerly at the first. You can use --rerun or --no-build-cache if that interferes at all, and swap JDK versions using JAVA_VERSION=? where it defaults to 11. Locally it failed right away.

@cpovirk
Copy link
Collaborator Author

cpovirk commented Dec 10, 2024

Thanks. I thought I had a problem last week, and then it passed a local build today, and so I pushed it to start the rest of the testing, since I figured it couldn't be that easy :) In parallel, I kicked off a build with CI=true, and that was enough to get it to fail locally again. So the latest commit may or may not be of value. I'll see what I can figure out....

@cpovirk
Copy link
Collaborator Author

cpovirk commented Dec 10, 2024

All right, I propagated the annotations up to the point at which the errors I was seeing all looked like cases in which NullAway's implementation isn't yet complete, and then I suppressed. It continues to be possible that there are more places that could benefit from annotations, but I feel good about what I've got. I'll merge this, and I may try to come back with a PR that expands some Javadoc to explain how to navigate null values and type arguments.

@cpovirk cpovirk merged commit e512b50 into ben-manes:master Dec 10, 2024
142 checks passed
@cpovirk cpovirk deleted the remark branch December 10, 2024 17:59
@ben-manes
Copy link
Owner

ben-manes commented Dec 10, 2024

SonarQube also has false positives on JSpecify, but those don't break the build so I am suppressing them remotely. It thinks that Cache.asMap() is nullable which is not the case.

Screenshot 2024-12-10 at 1 05 24 PM

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

Successfully merging this pull request may close these issues.

2 participants