[null] Consider javax.annotation.Nullable/NonNull by default

[mickaelistria]

What it does

This adds support for javax.annotation.Nonnull and javax.annotation.Nullable, which are popular becaues of JSR-305 by default in JDT null analysis. So users of JSR-305 do not require specific configuration to leverage JDT null analysis

How to test

Enable null annotation and try

class A {
  @javax.annotation.Nonnull String s = null;
}

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[mickaelistria]

cc @rgrunber

[stephan-herrmann]

You might be aware that I have quite strong reservations against what you call JSR 305:

• Both the name "JSR 305" and the namespace "javax.annotation" pretend a legitimation that is utterly missing, you are probably aware of how badly that JSR failed to produce a consensus among experts.
• Those annotations don't have a canonical home, last time I looked I saw different forks, with no chance to determine "the correct" version, I even saw versions completely lacking the essential @Target meta annotation (see below).
• The threesome of @Nonnull, @Nullable and @CheckForNull is questioned/rejected by just about every expert in the field. Their intended semantics is incompatible with JDT's analysis, unless you know exactly what you're doing. In particular the mapping shown in the proposed change is essentially wrong.
• Last time I looked, these were still of the inferior kind of declaration annotations, which JDT should no longer promote (it's pre-Java-8 legacy), nor should we let people unknowingly get into the troubles(!) of inadvertently mixing declaration annotations and TYPE_USE annotations.

For decades it was furthermore the position of JDT not to endorse/favor/advertise any specific 3rd party library (with maybe the singular exception of junit, which is above any doubt, though :) ).

This is likely a wont fix, because it would likely create quite some confusion for those not aware of the issues I mentioned.

[mickaelistria]

To be honest I'm not familiar with the technical limitation of those particular null annotations. I'm just aware that they are relatively popular (much more than JDT ones if we look at all Maven consumers of this jsr-305 artifact), and that they do not work by default in Eclipse IDE while users would expect those to work.
I thought that this was an easy and not risking regressions and useful change for many users; but if you think I'm wrong, then please close.

[mickaelistria]

It was brought to my attention that javax.annotation.Nullable and javax.annotation.NonNull are actually shipped by the Eclipse Platform and used by e4. So IMO, it would be consistent to better support them by default.
Can you please elaborate about some incorrect things such a change could cause?

[iloveeclipse]

It was brought to my attention that javax.annotation.Nullable and javax.annotation.NonNull are actually shipped by the Eclipse Platform

Not that I know, not in the SDK itself. It is only in spotbugs and only in old versions of it.

and used by e4

git grep shows zero matches in platform UI. Where exactly it is used?

[mickaelistria]

OK, I verified and am actually wrong, the javax.annotations that are used are not the null-analysis ones, only some Inject and PostConstruct and so on.

[rgrunber]

The motivation for this is redhat-developer/vscode-java#1693 . It looks like it's something that a sizable amount of users are interested in. If this is a no-go because it creates more problems / is legacy / no agreed upon standard, then we may end up just carrying it in JDT-LS since the options are simple enough to override.

[mickaelistria]

795,000 hits for javax.annotation.NonNull on GitHub: https://github.com/search?l=Java&q=javax.annotation.NonNull&type=code

[iloveeclipse]

@mickaelistria : please consider to recommend everyone stop using JSR-305 annotations.
They were proposed by Bill Pugh (creator of FindBugs) but after (or before?) Bill lost interest to FindBugs and left the project without a notice, same happened to JSR-305.
So this JSR is dead since many years.
See spotbugs/spotbugs#130.

If sou want to do something in NP area, consider add support to https://github.com/jspecify/jspecify in JDT, because that is supposed to be next "common" NP annotations standard.

Note: I'm not sure if JDT implementation is compatible with jspecify, it just happened to me to know about that initiative.

[iloveeclipse]

See my comment regarding this dead JSR.

[stephan-herrmann]

If sou want to do something in NP area, consider add support to https://github.com/jspecify/jspecify in JDT, because that is supposed to be next "common" NP annotations standard.

So they are getting closer to a first release? That's probably a sound option moving forward (although I would add "quasi-" or similar in front of "standard", right? :) )

Note: I'm not sure if JDT implementation is compatible with jspecify, it just happened to me to know about that initiative.

Please let me know if you see some kind of test suite that we could use to test compatibility.

At a quick glance the annotations could be made to match with a little tweaking behind the scenes:

• @NonNull & @Nullable can be mapped directly (good to see TYPE_USE of course)
• @NullMarked is similar to our @NonNullByDefault, just ours can be fine-tuned, so one would need to figure which detail locations are covered by @NullMarked.
• @NullUnmarked looks like our @NonNullByDefault({}) (i.e., apply the default at no locations.). We don't currently have a configuration option for that.

This should go a long way for a quick integration, but still there will be fine points in the semantics. That's why I asked for tests.

[iloveeclipse]

Closing as two project committer have consensus that this PR is "no go" and there were no other committer votes for a merge.

[mickaelistria]

Their intended semantics is incompatible with JDT's analysis, unless you know exactly what you're doing. In particular the mapping shown in the proposed change is essentially wrong.

OK. Do you have some more insights about it (articles, former discussions...)? I'm curious as many projects happen to happily use those javax.annotation.NonNull, what can be not working in JDT?
Particularly as jdt-ls will enable support for these annotations by default, it would be good if we can anticipate the possible problems, their criticity and probabiliy.

[cpovirk]

I don't know how official a definition jsr305 produced, but my understanding is that jsr305's @Nullable means something more like "no information" than like "I declare that this can be null." That's different from what essentially everyone else in the world has decided that @Nullable annotations should mean. (This difference caused problems for Guava users, for example, when Guava annotated according to our understanding of jsr305 and various tools used the more common interpretation.)

• https://www.cs.umd.edu/~pugh/JSR-305.pdf#page=14
• https://youtrack.jetbrains.com/issue/IDEA-286129/Spotbugss-Nullable-annotation-causes-Cannot-annotate-with-both-Nullable-and-Nullable-warning#focus=Comments-27-5704200.0-0

[mickaelistria]

Ok thanks. And what about @nonnull? If semantics for this one match, JDT could support it by default even if @nullable is not.

…

-- Mickael Istria Eclipse IDE <https://www.eclipse.org/eclipseide> developer, for Red Hat Developers <https://developers.redhat.com/>

[stephan-herrmann]

Ok thanks. And what about @nonnull? If semantics for this one match, JDT
could support it by default even if @nullable is not.

I don't believe you'd gain much with supporting only one of the pair. If you want to omit one use @NonNullByDefault and @Nullable. Any other subset doesn't seem to make much sense.

The next issue is @Target:

• All "JSR-305" annotations I have seen are declaration annotations (in some variants even without any @Target).
• Nowadays any nullness annotation short of @Target=TYPE_USE should really be considered as legacy
• Mixing declaration and type use annotations in one compilation is a thorny issue, I can't claim that this situation is well tested in JDT. So supporting the legacy by default may make it harder to use the "modern" (as of 2014) style

As an exercise for the reader: is the following assignment safe?

@Nonnull String [] old = ...;
String @NonNull [] new = old;

Is it readable?

[szarnekow]

Particularly as jdt-ls will enable support for these annotations by default.

Please reconsider this. Promoting these annotations in any way is doing more harm than good to the Java ecosystem for all of the aforementioned reasons.