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

Support @CheckForNull annotation #99

Closed
ben-manes opened this issue Dec 31, 2017 · 8 comments · Fixed by #397
Closed

Support @CheckForNull annotation #99

ben-manes opened this issue Dec 31, 2017 · 8 comments · Fixed by #397
Assignees

Comments

@ben-manes
Copy link

error: [NullAway] method returns @Nullable, but superclass method com.github.benmanes.caffeine.cache.AsyncLoadingCache.getIfPresent(java.lang.Object) returns @NonNull
  public @Nullable CompletableFuture<V> getIfPresent(@Nonnull Object key) {

However, this method is annotated with @CheckForNull which should implicitly mean @Nullable.

@msridhar
Copy link
Collaborator

Which library provides the @CheckForNull annotation?

@ben-manes
Copy link
Author

javax.annotation (jsr305 / findbugs)

@msridhar msridhar self-assigned this Dec 31, 2017
@msridhar
Copy link
Collaborator

Cool, this should be easy to support

@chengniansun
Copy link

Just for your information, Guava is migrating off from jsr305.

google/guava#2960

@uhafner
Copy link
Contributor

uhafner commented Jun 8, 2020

Are you ok with a pull request that adds support for a @CheckForNull annotation? From the code it looks like that I just need to update

public static boolean isNullableAnnotation(String annotName, Config config) {
and provide a test.

Or is there something conceptional wrong with this issue? Supporting CheckForNull would help projects a lot that use NullAway and SpotBugs

@msridhar
Copy link
Collaborator

msridhar commented Jun 8, 2020

I would be open to a PR that just adds @CheckForNull support. @lazaroclapp please comment if you see an issue with that.

@lazaroclapp
Copy link
Collaborator

I am fine with it. The idea is just to make CheckForNull another supported nullable annotation, right? As an exact alias for @Nullable, correct? Reading the previous discussion, we were a bit concerned about overhead of equality checking so many annotations if we just supported a custom list. But given that there seems to be interest for this particular annotation, I'd say go ahead.

We can open a separate task to optimize isNullableAnnotation and profile later.

@uhafner
Copy link
Contributor

uhafner commented Jun 9, 2020

I am fine with it. The idea is just to make @CheckForNull another supported nullable annotation, right? As an exact alias for @Nullable, correct?

Yes, just an alias for @Nullable. (The reason is that SpotBugs treats @Nullable differently: such references might be null but should not be flagged by a tool. In Spotbugs the semantics of @CheckForNull is the same as your @Nullable annotation). Then teams can use SpotBugs and NullAway to check the code...

lazaroclapp pushed a commit that referenced this issue Jun 10, 2020
The `@ChecksForNull` annotation is used by [SpotBugs](https://spotbugs.github.io) (and FindBugs) and has the same semantic as `@Nullable` for NullAway. The `@Nullable` annotation is ignored by Spotbugs so projects can't use SpotBugs *and* NullAway together. For details refer to the [SpotBugs documentation](https://spotbugs.readthedocs.io/en/stable/annotations.html).

This PR fixes #99.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants