Add `@CanIgnoreReturnValue` annotations #868

Stephan202 · 2023-02-13T06:29:36Z

Suggested commit message:

Add `@CanIgnoreReturnValue` annotations

This cancels out the package-level `@CheckReturnValue` annotations
introduced in ff1238567ede4001270ac6ed4682989b6b508c7e for selected
methods.

While there, add `@CheckReturnValue` to all remaining packages.

Reason for filing this PR, is that when upgrading to Caffeine 3.1.3 our build fails with

[WARNING] /path/to/CacheFactory.java:[365,59] [CheckReturnValue] The result of `recordStats(...)` must be used
  If you really don't want to use the result, then assign it to a variable: `var unused = ...`.

  If callers of `recordStats(...)` shouldn't be required to use its result, then annotate it with `@CanIgnoreReturnValue`.

    (see https://errorprone.info/bugpattern/CheckReturnValue
 Did you mean 'statsCounter.ifPresent(counter -> caffeine = caffeine.recordStats(() -> counter));'?

The above error is triggered by this code:

statsCounter.ifPresent(counter -> caffeine.recordStats(() -> counter));

(Our build has nearly all Error Prone checks enabled.)

This cancels out the package-level `@CheckReturnValue` annotations introduced in ff12385 for selected builder-style methods.

CLAassistant · 2023-02-13T06:29:41Z

All committers have signed the CLA.

ben-manes · 2023-02-13T06:32:32Z

caffeine/build.gradle

@@ -122,6 +122,7 @@ tasks.named('compileJava').configure {
  dependsOn generateLocalCaches, generateNodes
  finalizedBy compileCodeGenJava
  options.errorprone.disable('CheckReturnValue')
+  options.errorprone.error('CanIgnoreReturnValueSuggester')


This might go into codeQuality.gradle?

caffeine/gradle/codeQuality.gradle

Lines 154 to 155 in d084f2d

options.errorprone {

def enabledChecks = [

Ah, I missed that. Lemme try!

This way it also flags some test code. That's fine; will add the annotations there too.

ben-manes · 2023-02-13T06:35:25Z

@kluever please review. I guess that I missed that this was added to all the build methods. I just want to confirm this is everyone's intent.

ben-manes · 2023-02-13T06:42:23Z

I suppose we can also remove some of the suppressions that are now irrelevant. I think that would be CaffeinePolicy, CacheBuilderTest, and CacheBuilderFactory.

Stephan202 · 2023-02-13T06:47:38Z

I suppose we can also remove some of the suppressions that are now irrelevant. I think that would be CaffeinePolicy, CacheBuilderTest, and CacheBuilderFactory.

Thanks for flagging. That actually uncovers that additional @CanIgnoreReturnValue annotations are necessary, because the return self cases (where self is casted) aren't flagged by CanIgnoreReturnValueSuggester. Doing another pass...

Stephan202

Resolved most @SuppressWarnings; there remaining ones call a method whose return value should generally not be ignored. One more commit coming up.

Stephan202 · 2023-02-13T07:10:26Z

jcache/src/main/java/com/github/benmanes/caffeine/jcache/CacheProxy.java

  @Override
+  @CanIgnoreReturnValue
  public boolean remove(K key, V oldValue) {


The annotation also applies to some other methods in this class; they're not flagged likely because there's no test calling them. I'll add a separate commit for this.

Thanks. Mostly it is called by the jsr107 tck where our tests are to increase the code coverage due to possible gaps. The tck is a dependency that is unzipped during the build to run its tests, so a bit indirect and wouldn't be flagged.

Stephan202 · 2023-02-13T07:32:27Z

Updated the PR title and suggested commit message, since the PR now also covers some test code.

ben-manes

I'll merge and release once @kluever gives his blessing.

ben-manes · 2023-02-13T20:17:10Z

hey @kluever, sorry to bother you but can you please take a quick skim?

kluever

LGTM

Do you want to add @CheckReturnValue to the package-info.java (or alternatively, on each top-level type?)

kluever · 2023-02-14T18:53:26Z

guava/src/main/java/com/github/benmanes/caffeine/guava/CaffeinatedGuavaLoadingCache.java

@@ -123,7 +123,7 @@ public V apply(K key) {
  }

  @Override
-  @SuppressWarnings({"CheckReturnValue", "FutureReturnValueIgnored"})
+  @SuppressWarnings("FutureReturnValueIgnored")


We generally have been recommending just capturing into a var unused = ... variable instead of using @SuppressWarnings(...). Up to you tho.

I see 36 usages of @SuppressWarnings("FutureReturnValueIgnored"). Happy to make this change, but perhaps this is better done in a separate PR. Will await @ben-manes' verdict :)

I'd prefer @SuppressWarnings as other linters will flag unused variables (e.g. Eclipse). It's unfortunate as it does expand the suppression to the method instead of statement, but does a better job of avoiding warning spam. I'm open to revisiting to a better scheme separately so as to narrow the scope.

Stephan202 · 2023-02-14T19:14:39Z

Do you want to add @CheckReturnValue to the package-info.java (or alternatively, on each top-level type?)

This was already done in ff12385 :)

Assuming each package has such a file, all were modified:

$ git grep -L CheckReturnValue -- '*package-info.java' | wc -l
0

Stephan202 · 2023-02-14T19:19:11Z

Some packages lack a package-info.java, but none seem to really need it (that said, happy to add :) )

$ git ls-files '*.java' -- ':!:*module-info.java' | xargs dirname | sort -u | while read d; do test -f "${d}/package-info.java" || echo "${d}"; done
caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache
caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/local
caffeine/src/javaPoet/java/com/github/benmanes/caffeine/cache/node
caffeine/src/jmh/java/com/github/benmanes/caffeine
caffeine/src/jmh/java/com/github/benmanes/caffeine/cache
caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/impl
caffeine/src/jmh/java/com/github/benmanes/caffeine/cache/sketch
caffeine/src/jmh/java/com/github/benmanes/caffeine/profiler
caffeine/src/test/java/com/github/benmanes/caffeine
caffeine/src/test/java/com/github/benmanes/caffeine/apache
caffeine/src/test/java/com/github/benmanes/caffeine/cache
caffeine/src/test/java/com/github/benmanes/caffeine/cache/buffer
caffeine/src/test/java/com/github/benmanes/caffeine/cache/issues
caffeine/src/test/java/com/github/benmanes/caffeine/cache/stats
caffeine/src/test/java/com/github/benmanes/caffeine/cache/testing
caffeine/src/test/java/com/github/benmanes/caffeine/eclipse
caffeine/src/test/java/com/github/benmanes/caffeine/eclipse/acceptance
caffeine/src/test/java/com/github/benmanes/caffeine/eclipse/mutable
caffeine/src/test/java/com/github/benmanes/caffeine/google
caffeine/src/test/java/com/github/benmanes/caffeine/jsr166
caffeine/src/test/java/com/github/benmanes/caffeine/lincheck
caffeine/src/test/java/com/github/benmanes/caffeine/testing
examples/coalescing-bulkloader/src/main/java/com/github/benmanes/caffeine/examples/coalescing/bulkloader
examples/coalescing-bulkloader/src/test/java/com/github/benmanes/caffeine/examples/coalescing/bulkloader
examples/write-behind-rxjava/src/main/java/com/github/benmanes/caffeine/examples/writebehind/rxjava
examples/write-behind-rxjava/src/test/java/com/github/benmanes/caffeine/examples/writebehind/rxjava
guava/src/test/java/com/github/benmanes/caffeine/guava
guava/src/test/java/com/github/benmanes/caffeine/guava/compatibility
jcache/src/test/java/com/github/benmanes/caffeine/jcache
jcache/src/test/java/com/github/benmanes/caffeine/jcache/configuration
jcache/src/test/java/com/github/benmanes/caffeine/jcache/copy
jcache/src/test/java/com/github/benmanes/caffeine/jcache/event
jcache/src/test/java/com/github/benmanes/caffeine/jcache/expiry
jcache/src/test/java/com/github/benmanes/caffeine/jcache/integration
jcache/src/test/java/com/github/benmanes/caffeine/jcache/management
jcache/src/test/java/com/github/benmanes/caffeine/jcache/processor
jcache/src/test/java/com/github/benmanes/caffeine/jcache/size
jcache/src/test/java/com/github/benmanes/caffeine/jcache/spi
simulator/src/main/java/com/github/benmanes/caffeine/cache/simulator/parser/address/penalties
simulator/src/main/java/com/github/benmanes/caffeine/cache/simulator/policy/sketch/climbing/gradient
simulator/src/main/java/com/github/benmanes/caffeine/cache/simulator/policy/sketch/climbing/hill
simulator/src/main/java/com/github/benmanes/caffeine/cache/simulator/policy/sketch/climbing/inference
simulator/src/main/java/com/github/benmanes/caffeine/cache/simulator/policy/sketch/climbing/sim
simulator/src/test/java/com/github/benmanes/caffeine/cache/simulator/admission/bloom

ben-manes · 2023-02-14T20:11:52Z

Some packages lack a package-info.java, but none seem to really need it (that said, happy to add :) )

If you'd like to add to those other packages that would be fine, I think only the /main/ was my intent with a few missed. The javaPoet would be good as I forgot about it (codegen) and I didn't think tests / benchmarks / examples needed it but wouldn't care either way if added. Happy to do what you guys think is best 🙂

Stephan202 · 2023-02-15T15:25:58Z

I'm always up for enabling more static analysis ;) My agenda for the rest of the week is pretty tight, though, so it might make sense to defer that work to a separate PR; I can't say right now when I'll get around to a new pass.

Stephan202 · 2023-02-18T10:12:54Z

Added another commit; as far as I can tell all packages now declare @CheckReturnValue.

I noticed that a subset of the existing packages also declare nullability annotations (as well as a copyright header, in some cases); I left that out for now. If I should e.g. add those things to just the packages in the caffeine module, lemme know.

ben-manes · 2023-02-18T10:17:17Z

lgtm. We’re not using checker so it’s only for users, so probably unnecessary. You can merge once the ci passes 🙂

Stephan202 · 2023-02-18T10:22:11Z

👍 Thanks for the quick review! Updated the suggested commit message to include this latest change.

ben-manes · 2023-02-20T01:49:43Z

Released in 3.1.4

Add @CanIgnoreReturnValue annotations to non-test code

69e1154

This cancels out the package-level `@CheckReturnValue` annotations introduced in ff12385 for selected builder-style methods.

ben-manes reviewed Feb 13, 2023

View reviewed changes

ben-manes requested a review from kluever February 13, 2023 06:36

Move the Error Prone config

eccde54

Resolve suppressed warnings

c523245

Stephan202 commented Feb 13, 2023

View reviewed changes

Suggestion

1c75ee6

Stephan202 changed the title ~~Add @CanIgnoreReturnValue annotations to non-test code~~ Add @CanIgnoreReturnValue annotations Feb 13, 2023

ben-manes approved these changes Feb 13, 2023

View reviewed changes

ben-manes requested a review from cpovirk February 14, 2023 04:23

kluever reviewed Feb 14, 2023

View reviewed changes

Add more package-info.java files

fd088dd

Stephan202 merged commit 1803278 into ben-manes:master Feb 18, 2023

Stephan202 deleted the improvement/add-CanIgnoreReturnValue-annotations branch February 18, 2023 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `@CanIgnoreReturnValue` annotations #868

Add `@CanIgnoreReturnValue` annotations #868

Stephan202 commented Feb 13, 2023 •

edited

Loading

CLAassistant commented Feb 13, 2023 •

edited

Loading

ben-manes Feb 13, 2023

Stephan202 Feb 13, 2023

Stephan202 Feb 13, 2023

ben-manes commented Feb 13, 2023

ben-manes commented Feb 13, 2023

Stephan202 commented Feb 13, 2023

Stephan202 left a comment

Stephan202 Feb 13, 2023

ben-manes Feb 13, 2023

Stephan202 commented Feb 13, 2023 •

edited

Loading

ben-manes left a comment

ben-manes commented Feb 13, 2023

kluever left a comment

kluever Feb 14, 2023

Stephan202 Feb 14, 2023

ben-manes Feb 14, 2023

Stephan202 commented Feb 14, 2023

Stephan202 commented Feb 14, 2023

ben-manes commented Feb 14, 2023

Stephan202 commented Feb 15, 2023

Stephan202 commented Feb 18, 2023

ben-manes commented Feb 18, 2023

Stephan202 commented Feb 18, 2023

ben-manes commented Feb 20, 2023

Add @CanIgnoreReturnValue annotations #868

Add @CanIgnoreReturnValue annotations #868

Conversation

Stephan202 commented Feb 13, 2023 • edited Loading

CLAassistant commented Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-manes commented Feb 13, 2023

ben-manes commented Feb 13, 2023

Stephan202 commented Feb 13, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 commented Feb 13, 2023 • edited Loading

ben-manes left a comment

Choose a reason for hiding this comment

ben-manes commented Feb 13, 2023

kluever left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 commented Feb 14, 2023

Stephan202 commented Feb 14, 2023

ben-manes commented Feb 14, 2023

Stephan202 commented Feb 15, 2023

Stephan202 commented Feb 18, 2023

ben-manes commented Feb 18, 2023

Stephan202 commented Feb 18, 2023

ben-manes commented Feb 20, 2023

Add `@CanIgnoreReturnValue` annotations #868

Add `@CanIgnoreReturnValue` annotations #868

Stephan202 commented Feb 13, 2023 •

edited

Loading

CLAassistant commented Feb 13, 2023 •

edited

Loading

Stephan202 commented Feb 13, 2023 •

edited

Loading