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

Investigate warnings from JSpecify mode on caffeine #1002

Open
msridhar opened this issue Jul 20, 2024 · 3 comments
Open

Investigate warnings from JSpecify mode on caffeine #1002

msridhar opened this issue Jul 20, 2024 · 3 comments

Comments

@msridhar
Copy link
Collaborator

          I noticed the `addSuppressWarningsFix` in the stack trace, so I tried disabling `suggestSuppressions` and the build was successful. I have not reviewed the warnings to determine if they make sense.
caffeine git:(master) gradle build -x test
executing gradlew instead of gradle
Configuration on demand is an incubating feature.
Calculating task graph as no cached configuration is available for tasks: build

> Task :caffeine:compileJava
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java:333: warning: [NullAway] passing @Nullable parameter 'this.executor' where @NonNull is required
    requireState(this.executor == null, "executor was already set to %s", this.executor);
                                                                              ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java:362: warning: [NullAway] passing @Nullable parameter 'this.scheduler' where @NonNull is required
    requireState(this.scheduler == null, "scheduler was already set to %s", this.scheduler);
                                                                                ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java:477: warning: [NullAway] passing @Nullable parameter 'this.weigher' where @NonNull is required
    requireState(this.weigher == null, "weigher was already set to %s", this.weigher);
                                                                            ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java:528: warning: [NullAway] passing @Nullable parameter 'keyStrength' where @NonNull is required
    requireState(keyStrength == null, "Key strength was already set to %s", keyStrength);
                                                                            ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java:558: warning: [NullAway] passing @Nullable parameter 'valueStrength' where @NonNull is required
    requireState(valueStrength == null, "Value strength was already set to %s", valueStrength);
                                                                                ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java:595: warning: [NullAway] passing @Nullable parameter 'valueStrength' where @NonNull is required
    requireState(valueStrength == null, "Value strength was already set to %s", valueStrength);
                                                                                ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java:753: warning: [NullAway] passing @Nullable parameter 'this.expiry' where @NonNull is required
    requireState(this.expiry == null, "Expiry was already set to %s", this.expiry);
                                                                          ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java:857: warning: [NullAway] passing @Nullable parameter 'this.ticker' where @NonNull is required
    requireState(this.ticker == null, "Ticker was already set to %s", this.ticker);
                                                                          ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java:908: warning: [NullAway] passing @Nullable parameter 'this.evictionListener' where @NonNull is required
        "eviction listener was already set to %s", this.evictionListener);
                                                       ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java:960: warning: [NullAway] passing @Nullable parameter 'this.removalListener' where @NonNull is required
        "removal listener was already set to %s", this.removalListener);
                                                      ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java:444: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        result[0] = (oldValue == null) ? null : remappingFunction.apply(k, oldValue);
                  ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java:816: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
          oldValue[0] = Async.getIfReady(oldValueFuture);
                      ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalLoadingCache.java:115: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        oldValue[0] = cache().getIfPresentQuietly(key);
                    ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalLoadingCache.java:183: warning: [NullAway] returning @Nullable expression from method with @NonNull return type
        return cacheLoader.load(key);
        ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:2106: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        value[0] = n.getValue();
                 ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:2446: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        oldValue[0] = n.getValue();
                    ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:2490: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        oldKey[0] = node.getKey();
                  ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:2491: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        oldValue[0] = node.getValue();
                    ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:2535: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        nodeKey[0] = n.getKey();
                   ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:2536: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        oldValue[0] = n.getValue();
                    ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:2540: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
          oldValue[0] = null;
                      ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:2593: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        nodeKey[0] = n.getKey();
                   ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:2594: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        prevValue[0] = n.getValue();
                     ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:2702: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        nodeKey[0] = n.getKey();
                   ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:2704: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        oldValue[0] = n.getValue();
                    ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:2875: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        nodeKey[0] = n.getKey();
                   ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:2876: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        oldValue[0] = n.getValue();
                    ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:3185: warning: [NullAway] passing @Nullable parameter 'node.getValue()' where @NonNull is required
    V value = transformer.apply(node.getValue());
                                             ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:3999: warning: [NullAway] passing @Nullable parameter 'cache.getIfPresentQuietly(key)' where @NonNull is required
      return transformer.apply(cache.getIfPresentQuietly(key));
                                                        ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:4523: warning: [NullAway] referenced method returns @Nullable, but functional interface method java.util.function.Function.apply(T) returns @NonNull
        Function<CompletableFuture<V>, V> transformer = Async::getIfReady;
                                                        ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java:4576: warning: [NullAway] referenced method returns @Nullable, but functional interface method java.util.function.Function.apply(T) returns @NonNull
        Function<CompletableFuture<V>, V> transformer = Async::getIfReady;
                                                        ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/CaffeineSpec.java:242: warning: [NullAway] passing @Nullable parameter 'valueStrength' where @NonNull is required
    requireArgument(valueStrength == null, "%s was already set to %s", key, valueStrength);
                                                                            ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncLoadingCache.java:257: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        oldValueFuture[0] = asyncCache.cache().getIfPresentQuietly(key);
                          ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/UnboundedLocalCache.java:260: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        notificationValue[0] = null;
                             ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/UnboundedLocalCache.java:261: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        notificationKey[0] = null;
                           ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/UnboundedLocalCache.java:1074: warning: [NullAway] passing @Nullable parameter 'cache.data.get(key)' where @NonNull is required
      return transformer.apply(cache.data.get(key));
                                             ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/UnboundedLocalCache.java:1077: warning: [NullAway] passing @Nullable parameter 'cache.data.get(key)' where @NonNull is required
      V value = transformer.apply(cache.data.get(key));
                                                ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/UnboundedLocalCache.java:1191: warning: [NullAway] referenced method returns @Nullable, but functional interface method java.util.function.Function.apply(T) returns @NonNull
      Function<CompletableFuture<V>, V> transformer = Async::getIfReady;
                                                      ^
    (see http://t.uber.com/nullaway )
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/UnboundedLocalCache.java:1245: warning: [NullAway] referenced method returns @Nullable, but functional interface method java.util.function.Function.apply(T) returns @NonNull
      Function<CompletableFuture<V>, V> transformer = Async::getIfReady;
                                                      ^
    (see http://t.uber.com/nullaway )
39 warnings

> Task :jcache:compileJava
/Users/ben/projects/caffeine/jcache/src/main/java/com/github/benmanes/caffeine/jcache/CacheFactory.java:209: warning: [NullAway] unbound instance method reference cannot be used, as first parameter of functional interface method java.util.function.Function.apply(T) is @Nullable
          Optional.ofNullable(config.getCacheLoaderFactory()).map(Factory::create);
                                                                  ^
    (see http://t.uber.com/nullaway )
1 warning

Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

For more on this, please refer to https://docs.gradle.org/8.9/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.

BUILD SUCCESSFUL in 1m 17s
54 actionable tasks: 25 executed, 29 up-to-date

Publishing build scan...
https://caffeine.gradle-enterprise.cloud/s/44bmxntmxwjle

Configuration cache entry discarded because incompatible tasks were found: ':caffeine:forbiddenApisCodeGen', ':guava:forbiddenApisTest', ':jcache:forbiddenApisTestResources', ':caffeine:forbiddenApisTest', ':jcache:forbiddenApisMain', ':guava:forbiddenApisMain', ':caffeine:forbiddenApisJmh', ':caffeine:forbiddenApisMain', ':jcache:forbiddenApisTest', ':simulator:forbiddenApisTest', ':simulator:forbiddenApisMain', ':caffeine:forbiddenApisJavaPoet'.

Originally posted by @ben-manes in #996 (comment)

@msridhar
Copy link
Collaborator Author

msridhar commented Jul 20, 2024

Couple of quick observations here.

/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/Caffeine.java:333: warning: [NullAway] passing @Nullable parameter 'this.executor' where @NonNull is required
    requireState(this.executor == null, "executor was already set to %s", this.executor);

This is a false positive due to #674. I should re-prioritize that one and try to fix it soon (though it's a bit messy).

Generally we've tried to specifically look for org.jspecify.annotations.Nullable annotations in certain places for now. It looks like Caffeine is using Checker Framework's @Nullable. @ben-manes any plan to transition to JSpecify annotations? If not we can look into broadening JSpecify mode to also recognize other type-use @Nullable annotations.

/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java:444: warning: [NullAway] Writing @Nullable expression into array with @NonNull contents.
        result[0] = (oldValue == null) ? null : remappingFunction.apply(k, oldValue);

To me this looks like a real issue. With JSpecify semantics, I would expect the result variable to be declared as @Nullable CompletableFuture<V>[] result, to allow for the array to contain null values. I'd also expect the enclosing method's return type to be @Nullable CompletableFuture<V>. @ben-manes does that sound right to you?

@ben-manes
Copy link

This is a false positive due to #674.

It is expected that this would show up only when JSpecify mode is enabled or it just dumb luck?

any plan to transition to JSpecify annotations?

I am planning on mirroring Guava and migrate when they have. That's mostly because the previous annotation discussions are a lot of bike shedding that I don't enjoy (maven dependency scope, stylistic semantics, etc). Pushing those who like to argue towards Guava helps me close the issues and say that I'll follow whatever best practices the broader community chooses. Hopefully they make the transition soon with 1.0 released (congrats!) so I'll do a fast follow.

To me this looks like a real issue.

That seems reasonable to me.

@msridhar
Copy link
Collaborator Author

This is a false positive due to #674.

It is expected that this would show up only when JSpecify mode is enabled or it just dumb luck?

I think our current hacks make things work outside JSpecify mode. I'm not exactly sure why they don't work in JSpecify mode; you're right that might be a separate issue from #674.

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

No branches or pull requests

2 participants