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.

Sign up for GitHub

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

Jump to bottom

Add @CheckReturnValue to package-info.java (and @CanIgnoreReturnValue as appropriate) #863

Closed
kluever opened this issue Jan 23, 2023 · 4 comments
Closed

Add @CheckReturnValue to package-info.java (and @CanIgnoreReturnValue as appropriate) #863

kluever opened this issue Jan 23, 2023 · 4 comments

Comments

@kluever
Copy link
Collaborator

kluever commented Jan 23, 2023

Hey Ben,

In our quest to stomp out accidentally ignored return value bugs, I was hoping you'd be able to add ErrorProne's @CheckReturnValue to a package-info.java (in each package that caffeine owns), as well as @CanIgnoreReturnValue to methods that are "ignorable".

Specifically, we noticed that refresh() went from being void to returning a Future --- and that change is going to cause problems with errorprone's FutureReturnValueIgnored check.

Does this sound reasonable?
@ben-manes
Copy link
Owner

ben-manes commented Jan 23, 2023 •
edited

Sure! I tried to keep those annotations in sync with Guava's usages, but I might have missed a couple cases and for these divergences. I guess that I missed your switch over and will emulate 67350a3 and bd5a7777. You are welcome to send a PR or commit the changes, else I will try to take care of it this evening. I meant to cut a release last week so we can get this out pretty quick for you.

@kluever
Copy link
Collaborator Author

kluever commented Jan 23, 2023

Awesome, thanks for handling this! A bit more background: internally, we've made "CRV the default", so previously-safe API changes like void to non-void are a bit more problematic for us.

Thanks again --- hope all is well.

@ben-manes
Copy link
Owner

ben-manes commented Jan 24, 2023 •
edited

How lenient would you like me to be regarding @CanIgnoreReturnValue? I see cases like Cache.getIfPresent(key) annotated with remarks to remove in the internal bug b/27479612. However, other cases like Cache.getAllPresent(keys) are not annotated so the result must be handled. There isn't a consistent rule, as LoadingCache.getAll(keys) is annotated to be unchecked.

I would guess that you want it to be strict but, due to the enormous code base, deferred some refactorings until later. Do you want Caffeine's apis to be lenient or strict? I think that I'd need some more assistance to make sure that I did not fubar this and cause headaches.

ben-manes added a commit that referenced this issue Feb 6, 2023
@ben-manes
Add @CheckReturnValue at the package level (fixes #863)
33ca473 
This annotation is used by ErrorProne and Spotbugs to warn users when
they ignore the results of a method call. This is now the default
behavior rather than a few opt in cases. In rare cases the result is
an ignorable signal, like List's add(e) or our LoadingCache.refresh(k),
so @CanIgnoreReturnValue is used to disable the check for those methods.
For all other methods then users can suppress the warning when desired.
@ben-manes
Copy link
Owner

Released in 3.1.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ben-manes @kluever