search

uber / NullAway

A tool to help eliminate NullPointerExceptions (NPEs) in your Java code with low build-time overhead
MIT License
3.62k stars 290 forks source link

Investigate warnings from JSpecify mode on caffeine #1002

Open msridhar opened 1 month ago

msridhar commented 1 month ago 
          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.
```console ➜ 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, 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, 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, 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, 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 https://github.com/uber/NullAway/issues/996#issuecomment-2227422915_
msridhar commented 1 month ago

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 commented 1 month ago

This is a false positive due to https://github.com/uber/NullAway/issues/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 commented 1 month ago

This is a false positive due to https://github.com/uber/NullAway/issues/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.