Allow Nullable (et al) from other packages than javax.annotation (JSR 305) e.g. org.eclipse.jdt.annotation.Nullable

[vorburger]

What version of Error Prone are you using?

2.1.3

Does this issue reproduce with the latest release?

yes (as 2.1.3 seems to be the latest release)

What did you do?

Following #723, I enabled the ReturnMissingNullable check on an existing code base (OpenDaylight) on which I had experimented with the Eclipse JDT based null analysis (see also http://www.lastnpe.org), and have hit this:

[ERROR] Some.java:[30,9] [ReturnMissingNullable] Returning null literal
[ERROR]     (see http://errorprone.info/bugpattern/ReturnMissingNullable)
[ERROR]   Did you mean '@Nullable @Override'?

The method was annotated with org.eclipse.jdt.annotation.Nullable instead of javax.annotation.Nullable. (If required, I could provide a recipe or a complete small program for reproducing the error, but it should hopefully be obvious what I mean.)

Would you guys consider accepting other annotations for your null analysis? Either by configuration of all accepted FQNs, or perhaps even through some pragmatic solution like "just look only at the name not the FQN with package, and accept anyhing named Nullable goes, unrelated to the package". (AFAIK some other thing does this as well - I can't remember if its SpotBugs (ex FindBugs) or IntelliJ or what; FYI e.g. in Eclipse there is a Preference to set a list of FQNs.)

Obviously this isn't only about Nullable but all typically 3 null related annotations (Nullable, NonNull and NonNullByDefault).

I may be motivated to contributed a PR for this later if I hit this problem more often, if there is interest for doing something about this - just filing this to gauge the maintainers initial reaction and position about this.

PS: Note the problem is of course not really directly related to lastnpe.org Eclipse JDT based null analysis - if one were to prefer using say e.g. the org.checkerframework.checker.nullness.qual.Nullable from the Checker Framework, then one would most likely hit exactly the same problem. Some people use their own internal null related annotations instead of any public one, and then customizes tools to use those. Long story short: IMHO a tool such as error-prone, in an ideal world, should be open to accepting different types of null annotations.

PPS: Please note that JSR 305 null analysis related annotations in the javax.annotation package are by no means "standard" or the only ones - there are, clearly, other such annotations out there - including in Google's own Android AFAIK. Also the old JSR 305 are not Java 8 Type annotations yet, but e.g. Eclipse' and Checker Framework's are.

[eaftan]

I'd be happy to accept a PR that adds support to ReturnMissingNullable for any annotation with a matching simple name, regardless of fully-qualified name.

[kevin1e100]

ReturnMissingNullable does accept any conventional, JSR-305-style, Nullable annotation IIRC. Eclipse's annotation, however, is a type annotation it seems [1] and I don't believe we support those ATM. I understand that it syntactically looks identical but under the hood a type annotation is very different.

[1] https://checkerframework.org/releases/2.1.14/api/org/eclipse/jdt/annotation/Nullable.html

[vorburger]

@kevin1e100 yeah you are right it is a Type annotation (so can be placed on more things than only return type). If ReturnMissingNullable already accept any annotations matching the simple name Nullable (so even if not from the package javax.annotation), just not Type annotations, then all I'm proposing here is to relax that constraint - which @eaftan seems to be OK with.

I'll see when I can propose a PR for this - probably when it bites me next time!

[cushon]

ReturnMissingNullable has been turned down