Bug 369079 - [null] Allow multiple null annotations
Summary: [null] Allow multiple null annotations
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.8   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 4.6 M4   Edit
Assignee: Stephan Herrmann CLA
QA Contact:
URL:
Whiteboard: To be verified for 4.6M5
Keywords:
: 415755 482220 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-01-19 09:09 EST by Sven Efftinge CLA
Modified: 2016-01-28 01:46 EST (History)
10 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sven Efftinge CLA 2012-01-19 09:09:44 EST
(I can imagine that this has been discuss before but I couldn't find the bug/comment.)

Currently it seems as if one can only configure one qualified name for Nullable and for NonNull annotations. Given the many different existing annotations out there with the very same semantic meaning, it seems important to allow multiple annotations per case to support code which uses two or more different libraries which are each annotated with different annotations.

It would be even better if the compiler would allow wildcards and do name-based matching. Having the "Nullable" and "NonNull" as default.

That wouldn't force everybody to have the currently provided runtime jar on the classpath and solve most integration issues without any further ado on the user side.
Comment 1 Stephan Herrmann CLA 2012-01-19 09:35:32 EST
(In reply to comment #0)
> (I can imagine that this has been discuss before but I couldn't find the
> bug/comment.)

Indeed, this has been discussed back and forth :)
For a summary of that discussion see bug 365387 comment 53.
The conclusion is: the current limitation is deliberate but not final.

We can use this bug as the "future RFE" mentioned in that comment.

> Currently it seems as if one can only configure one qualified name for Nullable
> and for NonNull annotations. Given the many different existing annotations out
> there with the very same semantic meaning, it seems important to allow multiple
> annotations per case to support code which uses two or more different libraries
> which are each annotated with different annotations.

This is right. As long as you only use one 3rd party library with null annotations you can tell the compiler to use those. Beyond that a single set of annotation types simply won't suffice.
 
> It would be even better if the compiler would allow wildcards and do name-based
> matching. Having the "Nullable" and "NonNull" as default.

If one thing negative we learned from AspectJ, then it's that using name based wildcards for describing semantic issues is brittle/dangerous. Plus I don't even see which wildcards we would suggest to match ("Nullable", "CheckForNull" but not "Nullable" (from FindBugs)).
 
> That wouldn't force everybody to have the currently provided runtime jar on the
> classpath and solve most integration issues without any further ado on the user
> side.

We don't have a runtime jar, only a compile time jar, and that is necessary no matter how annotations are configured.

So, yes, specifying annotation names as a list is an interesting option and will be implemented time permitting. Beyond that I don't see any need for action.
Comment 2 Ayushman Jain CLA 2012-01-19 09:46:54 EST
(In reply to comment #1)
> will be implemented time permitting.

Yup. No promises for 3.8! :)
Comment 3 Sven Efftinge CLA 2012-01-19 10:34:48 EST
(In reply to comment #1)
> > It would be even better if the compiler would allow wildcards and do name-based
> > matching. Having the "Nullable" and "NonNull" as default.
> 
> If one thing negative we learned from AspectJ, then it's that using name based
> wildcards for describing semantic issues is brittle/dangerous.

I don't know to what similar feature in AspectJ you are referring to.

> Plus I don't
> even see which wildcards we would suggest to match ("Nullable", "CheckForNull"
> but not "Nullable" (from FindBugs)).

Is "Nullable" from find bugs semantically different?
I proposed something like '*.Nullable' and '*.NonNull*' which seems to align nicely with 308 and your current choice. What could be the real world problems caused by something like that?

> > That wouldn't force everybody to have the currently provided runtime jar on the
> > classpath and solve most integration issues without any further ado on the user
> > side.
> 
> We don't have a runtime jar, only a compile time jar, and that is necessary no
> matter how annotations are configured.

Which still is needed at compile time. That is you'll have to get it from somewhere depending on what build system you use, etc. Btw. is it already available in a public maven repository?
Having the two annotations somewhere in your project seems much simpler, as long as there is not something which comes with the JDK.
Comment 4 Stephan Herrmann CLA 2012-01-19 12:23:40 EST
(In reply to comment #3)
> (In reply to comment #1)
> > If one thing negative we learned from AspectJ, then it's that using name based
> > wildcards for describing semantic issues is brittle/dangerous.
> 
> I don't know to what similar feature in AspectJ you are referring to.

Really? :) I was referring to name based wildcards in pointcut expressions,
which lots of folks agree as being the main problem with AspectJ.

> Is "Nullable" from find bugs semantically different?

Unfortunately, yes. FindBugs' "Nullable" lacks a precise definition but comes
closest to the legacy Java semantics: nothing known but don't warn me if I do
dangerous things. This is *not* the semantics the JDT compiler associates
with @Nullable.

> I proposed something like '*.Nullable' and '*.NonNull*' which seems to align
> nicely with 308 and your current choice. What could be the real world problems
> caused by something like that?

It would capture FindBugs' Nullable annotation which is inappropriate in this
list. You would also capture NonNullByDefault and NonNullAnnotationTests
and lots of things we can't foresee. It would be unwise to automatically
assign semantics to this unknown set of types.

It's also unclear how similar annotations in persistence frameworks
relate to the issue at hand.

> > We don't have a runtime jar, only a compile time jar, and that is necessary no
> > matter how annotations are configured.
> 
> Which still is needed at compile time.

I had experiments with emulating these annotations (i.e., interpret them
even if they cannot be resolved in your classpath), which was abandoned
because that would make our compiler violate the JLS.

I guess the intention of annotations is that you can define additional
"keywords", but you must first *define* them before use. I.e., the compiler
must find the annotation type on the classpath, that's how Java annotations
work, right?

> That is you'll have to get it from
> somewhere depending on what build system you use, etc. Btw. is it already
> available in a public maven repository?

please see bug 366298

> Having the two annotations somewhere in your project seems much simpler, as
> long as there is not something which comes with the JDK.

you can do that, but using an existing set of annotations would make it 
a bit easier to agree on the same annotations, no? :)
By including our version in the Eclipse SDK it should be easily accessible
for everybody, more magic to make it still easier is in the making.
Comment 5 Markus Keller CLA 2012-01-24 13:04:34 EST
I don't think we should allow multiple sets of annotation names per buildpath entry. Users should decide on a single set of annotations for the source in one project.

For required libraries, there's bug 331651. There, we should also allow to configure separate names per library, even if no external annotations are necessary.
Comment 6 Dani Megert CLA 2012-01-25 07:36:45 EST
(In reply to comment #5)
> I don't think we should allow multiple sets of annotation names per buildpath
> entry. Users should decide on a single set of annotations for the source in one
> project.

I agree.


> For required libraries, there's bug 331651. There, we should also allow to
> configure separate names per library, even if no external annotations are
> necessary.
Comment 7 Stephan Herrmann CLA 2013-08-23 10:18:41 EDT
*** Bug 415755 has been marked as a duplicate of this bug. ***
Comment 8 Stephan Herrmann CLA 2015-11-16 19:41:45 EST
*** Bug 482220 has been marked as a duplicate of this bug. ***
Comment 9 Andrey Loskutov CLA 2015-11-17 04:41:19 EST
(In reply to Stephan Herrmann from comment #8)
> *** Bug 482220 has been marked as a duplicate of this bug. ***

I'm coming from bug 482220 here.

From the end-user point of view, we don't want to configure and use multiple variants of Nullable in EGit *source* code, we just want that this works out of the box :-)

In EGit we want to consume NP annotated API from libraries we depend on *independently* which variant of Nullable they decided to use (see [1]). After I've made a mistake in commit [2] and switched EGit to follow JGit with the JGit own annotations, I've realized that because of this bug this decision will also have consequences for any of the downstream consumers of EGit and that we have to make new JGit NP annotations an API and export it to all clients and to change our and clients code to use JGit own annotations in the source too. 

So now I'm facing a big issue to setup JGit/EGit NP analysis *right* way and before I start making breaking changes again I would like to clarify the state of this bug.

Assume following story:

Project A uses and configures JDT to use a.Nullable. 
JDT happy with NP analysis in A.

Project B uses and configures JDT to use b.Nullable.
JDT happy with NP analysis in B.

Project C uses and configures JDT to use c.Nullable and depends on API from project A and B. Because JDT only "understands" c.Nullable in the context of C, JDT does not "see" annotations on A and B API and so does not flag any NP violations in the C code using the API from A and B.

In this case no one from A,B and C projects wants to wire or use *multiple configurations* of Nullable, because from the user point of view they are nothing else as "aliases" to the same thing, and appear only with one variant in the project *source*.

In the ideal word JDT would just build an "aliases" table per project and consider this aliases during the analysis. However we are not living in the ideal world. I think JDT can automatically "understands" right NP annotations for projects available within the workspace, but for sure this will not work on command line compilation or if the 3rd party library is used.

Not nice from the user point of view, but practical solution is the possibility to define for a project a *list* of annotations known to be used as Nullable:

org.eclipse.jdt.core.compiler.annotation.nullable=org.eclipse.jgit.annotations.Nullable,org.eclipse.jdt.annotation.Nullable,javax.annotation.Nullable

etc.

Without this possibility the current JDT NP analysis has a major issue with project dependency chains using different variants of the nullness annotations.

Sorry to say, but IMHO this is not an "enhancement" but a major concept issue.

Right now this forces me to switch EGit source to use JGit annotations or undo the JGit change [1] which introduced custom annotations and force both projects to use JDT annotations. But any of this decisions will force all downstream consumers from JGit/EGit API also to use the NP annotations library chosen by JGit (or ignore NP analysis on JGit/EGit code), and so forth. As soon as one in the consumers will use another library annotated with *another* NP annotations, the NP analysis will fail for the consumer of this library => kaboom.

@Stephan: what is your assessment, how complicated is to allow JDT to consume "aliases" via org.eclipse.jdt.core.compiler.annotation.nullable=org.eclipse.jgit.annotations.Nullable,org.eclipse.jdt.annotation.Nullable,javax.annotation.Nullable

Does this require a major refactoring or is this just a matter of "if(aliases.contains(annotationName))" switches here and there?

I'm asking it now because I want to consume annotated JGit API in EGit and must make a decision which way to go. If you say this is a bigger task for JDT, I will just align EGit on JGit annotations and don't care anymore. If the answer is "a simple patch is possible" I could wait or even contribute the JDT patch by myself.

@Terry: you wrote in [1] "Commit 847b3d1 enabled annotation-based NPE analysis to JGit. In so doing, it introduced a new dependency on the org.eclipse.jdt that is undesirable".

Not sure what was the reason to say that the dependency is undesirable - the dependency is not on the entire jdt but only on the org.eclipse.jdt.annotation bundle which is available on maven and has no other dependencies. 

Is there something within org.eclipse.jdt.annotation library that is not acceptable for JGit, or just the fact to have external annotation library is undesirable?

[1] http://git.eclipse.org/c/jgit/jgit.git/commit/?id=3a3bb2ce292fcfb8ad34408d2ac7106d05ba642f
[2] http://git.eclipse.org/c/egit/egit.git/commit/?id=b3596711ae6533669dcd948a24cad252728103d9
Comment 10 Stephan Herrmann CLA 2015-11-17 07:11:47 EST
(In reply to Andrey Loskutov from comment #9)
> From the end-user point of view, we don't want to configure and use multiple
> variants of Nullable in EGit *source* code, we just want that this works out
> of the box :-)

sure, and we all scream for ice cream :)


> In the ideal word JDT would just build an "aliases" table per project and
> consider this aliases during the analysis. However we are not living in the
> ideal world. I think JDT can automatically "understands" right NP
> annotations for projects available within the workspace, but for sure this
> will not work on command line compilation or if the 3rd party library is
> used.

yep, there's the rub. This configuration inside .settings/org.eclipse.jdt.core.prefs is not considered API of its project, hence we cannot assume that any project can see the configuration of any other project. In the general case the prefs will not even be present during compilation of downstream projects.

> Not nice from the user point of view, but practical solution is the
> possibility to define for a project a *list* of annotations known to be used
> as Nullable:
> 
> org.eclipse.jdt.core.compiler.annotation.nullable=org.eclipse.jgit.annotations.Nullable,org.eclipse.jdt.annotation.Nullable,javax.annotation.Nullable
> 
> 
> etc.

From my current understanding, too, this is the most likely solution.

 
> @Stephan: what is your assessment, how complicated is to allow JDT to
> consume "aliases" via
> org.eclipse.jdt.core.compiler.annotation.nullable=org.eclipse.jgit.annotations.Nullable,org.eclipse.jdt.annotation.Nullable,javax.annotation.Nullable
> 
> 
> Does this require a major refactoring or is this just a matter of
> "if(aliases.contains(annotationName))" switches here and there?

Not a trivial task, but in order to give a somewhat more useful answer I'll do a time boxed experiment in this direction right now. I'll be back ...
Comment 11 Stephan Herrmann CLA 2015-11-17 07:17:07 EST
(In reply to Markus Keller from comment #5)
> For required libraries, there's bug 331651. There, we should also allow to
> configure separate names per library, even if no external annotations are
> necessary.


I think we all agree that required libraries is exactly the issue we need to resolve.

As to the connection to bug 331651: for the encoding of external annotations we ended up with a "neutral" encoding, which does not contain any qualified names of annotations, but tokens '0' and '1'. This way, we will never get into trouble interpreting external annotations from different sources. Good. OTOH, that solution does not help for normal (internal) annotations.
Comment 12 Eclipse Genie CLA 2015-11-17 10:28:06 EST
New Gerrit change created: https://git.eclipse.org/r/60624
Comment 13 Stephan Herrmann CLA 2015-11-17 10:44:07 EST
(In reply to Eclipse Genie from comment #12)
> New Gerrit change created: https://git.eclipse.org/r/60624

This is WIP with one known failure. The main message: AFAICS there are only two methods left needing an update for this feature (BTB.scanFieldForNullAnnotation() and BTB.scanMethodForNullAnnotation()).

I opted for creating three new compiler options, rather than changing the interpretation of the existing options. One reason for doing so: I want to be very clear, that one set of null annotations is the primary set for the current project, for two reasons actually:
- the compiler synthesizes null annotations (e.g., for rendering error messages) and there should be no doubt which annotation names are used here
- later we may want to implement a warning if the current project actively uses a secondary null annotation (which was configured just to accommodate its use in some library).

This means: the existing options are still interpreted as single qualified names, only the new options are interpreted as comma-separated lists of qualified names.

***************************************************************************
*JDT leads* please let me know if you see any issues with adding these options and whether you have a strong preference for or against modifying interpretation of the old options vs. introducing new options.
***************************************************************************


Once that general question is settled I will add the corresponding API in JavaCore plus the corresponding CLI arguments.
Comment 14 Terry Parker CLA 2015-11-17 12:10:53 EST
(In reply to Andrey Loskutov from comment #9)

> @Terry: you wrote in [1] "Commit 847b3d1 enabled annotation-based NPE
> analysis to JGit. In so doing, it introduced a new dependency on the
> org.eclipse.jdt that is undesirable".
> 
> Not sure what was the reason to say that the dependency is undesirable - the
> dependency is not on the entire jdt but only on the
> org.eclipse.jdt.annotation bundle which is available on maven and has no
> other dependencies. 
> 
JGit is a low-level library used in many contexts. Keeping dependencies to a minimum is highly desirable because it decreases the friction of adopting the library and the maintenance costs of using it.

For example, at Google we support the Android and Chrome open source project by building JGit and Gerrit from source using javac and our internal build system. The JDT annotation library is not readily available in our environment. 

Since the null annotations annotate public APIs they are likely to propagate. For example it probably makes sense for the Gerrit project to retire its locally-defined annotations and adopt the JGit ones, which means adding whatever dependencies the JGit annotations require. There are currently no JDT dependencies in Gerrit.

Any non-Eclipse project is much more likely to define its own annotations (e.g., org.gradle.api.Nullable) or use the javax annotations rather than the JDT ones (e.g., the Guava libraries). The usefulness of Eclipse's null analysis engine is undercut in any project that attempts to integrate these libraries. This is a feature gap with IntelliJ (sorry to point it out): https://blog.jetbrains.com/idea/2011/03/more-flexible-and-configurable-nullublenotnull-annotations/

So Stephan, great to see your work here!
Comment 15 Stephan Herrmann CLA 2015-11-17 13:10:23 EST
Hi Terry,

(In reply to Terry Parker from comment #14)
> For example, at Google we support the Android and Chrome open source project
> by building JGit and Gerrit from source using javac and our internal build
> system. The JDT annotation library is not readily available in our
> environment. 

That really surprises me. What reason could there be for not having access to this annotation library? 17kB available via p2 and maven and with no transitive dependencies. And it's a compile time-only dependency. Are you saying you are building without any external dependencies? Tell me.


> Since the null annotations annotate public APIs they are likely to
> propagate. For example it probably makes sense for the Gerrit project to
> retire its locally-defined annotations and adopt the JGit ones, which means
> adding whatever dependencies the JGit annotations require. There are
> currently no JDT dependencies in Gerrit.

I wouldn't consider o.e.j.annotation a "JDT dependency" :)

 
> Any non-Eclipse project is much more likely to define its own annotations
> (e.g., org.gradle.api.Nullable) or use the javax annotations

please don't get me started on "the javax annotations".


I'm willing to resolve this issue very soon, but I also see a danger in doing so, because we are further *encouraging* fragmentation (I'm aware that we cannot *avoid* it, though).


Let me point out two issues that imply we don't just speak about names (I couldn't care less about those) but also about semantics:

(1) Most of the existing null annotations are stuck at the level of declaration annotations. The real power of null analysis only comes with Java 8 type annotations. At that level I'm not aware of many annotations other than JDT's and those from the Checkers Framework. Am I missing some?

(2) There are additional semantic differences in how @NonNullByDefault is defined in different libraries. When using the JDT version of this annotation, ECJ can interpret @NNBD as well as @NNBD(false) (Java 7) or fine tuning by way of specifying a set of DefaultLocations (Java 8) [1]. This capability is lost when using s.o. else's annotations.

... back to work ...

[1] http://help.eclipse.org/mars/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Ftasks%2Ftask-using_null_type_annotations.htm&cp=1_3_9_1_3&anchor=compatibility
Comment 16 Terry Parker CLA 2015-11-17 15:49:30 EST
(In reply to Stephan Herrmann from comment #15)
> Hi Terry,
> 
> (In reply to Terry Parker from comment #14)
> > For example, at Google we support the Android and Chrome open source project
> > by building JGit and Gerrit from source using javac and our internal build
> > system. The JDT annotation library is not readily available in our
> > environment. 
> 
> That really surprises me. What reason could there be for not having access
> to this annotation library? 17kB available via p2 and maven and with no
> transitive dependencies. And it's a compile time-only dependency. Are you
> saying you are building without any external dependencies? Tell me.
>
Our internal build system has no access to p2 or Maven. We need to do work to import any external dependency we use into our build system. Currently JGit only has a handful of dependencies (javaewah, junit, slf4j) so this is manageable. To use the JDT annotations I would need to create a new Java library within our build system to consume it.

Yes that is a uniquely painful situation at Google, but I still stand on the principle that dependencies on low-level libraries should be carefully managed. Dependency bloat causes lots of pain.
> 
> > Since the null annotations annotate public APIs they are likely to
> > propagate. For example it probably makes sense for the Gerrit project to
> > retire its locally-defined annotations and adopt the JGit ones, which means
> > adding whatever dependencies the JGit annotations require. There are
> > currently no JDT dependencies in Gerrit.
> 
> I wouldn't consider o.e.j.annotation a "JDT dependency" :)
> 
>  
> > Any non-Eclipse project is much more likely to define its own annotations
> > (e.g., org.gradle.api.Nullable) or use the javax annotations
> 
> please don't get me started on "the javax annotations".
> 
> 
> I'm willing to resolve this issue very soon, but I also see a danger in
> doing so, because we are further *encouraging* fragmentation (I'm aware that
> we cannot *avoid* it, though).

I think that ship has sailed. I don't know the history of the javax annotations (and am not trying to get you started!) but from what I can tell its failure pretty much guarantees fragmentation. :(

> 
> Let me point out two issues that imply we don't just speak about names (I
> couldn't care less about those) but also about semantics:
> 
> (1) Most of the existing null annotations are stuck at the level of
> declaration annotations. The real power of null analysis only comes with
> Java 8 type annotations. At that level I'm not aware of many annotations
> other than JDT's and those from the Checkers Framework. Am I missing some?
> 
> (2) There are additional semantic differences in how @NonNullByDefault is
> defined in different libraries. When using the JDT version of this
> annotation, ECJ can interpret @NNBD as well as @NNBD(false) (Java 7) or fine
> tuning by way of specifying a set of DefaultLocations (Java 8) [1]. This
> capability is lost when using s.o. else's annotations.

You are right, the improved semantics of Java 8 type annotations are highly desirable. I imagine active projects will be migrating to them.

The fragmentation here is similar to other new language features, where your code may use a library that hasn't been updated to use the new feature. The old and the new need to be able to work together. Ok, I imagine it is actually much worse than that because the lack of standardization creates a list of semantic differences between the different annotations. That is frustrating.

I am ignorant of how the null analysis works, so here are a couple of questions: 1) When performing null analysis on a compilation unit, can you mix processing of declaration annotations and type annotations? 2) If not, is it reasonable to add a check that the "alias" annotations are type annotations/support the JDT semantics? I think it is completely reasonable to have a "power" mode where all annotations present provide equivalent semantics as the JDT annotations, and the "declaration annotations/Java 7" mode otherwise.

> 
> ... back to work ...
> 
> [1]
> http://help.eclipse.org/mars/index.jsp?topic=%2Forg.eclipse.jdt.doc.
> user%2Ftasks%2Ftask-using_null_type_annotations.
> htm&cp=1_3_9_1_3&anchor=compatibility
Comment 17 Stephan Herrmann CLA 2015-11-17 18:58:58 EST
The latest patch set in gerrit starts to show what is possible when mixing different stuff. In particular, a project using JDT's type annotations (1.8) can successfully read also declaration annotations (1.5) from class files (assumed to be libraries). I tweaked the rendering of error messages but I expect that some situations will still produces slightly confusing error messages like saying 
  required 'String' but the provided value is inferred as @Nullable
where in a pure 1.8 setting we would see
  required '@NonNull String' but the provided value is inferred as @Nullable

These will have to be fixed as they occur.

Whether this mix creates bad semantic effects still needs to be investigated. Is it usefull to include - say - a generic type with 1.5 null annotations into full 1.8-based null type checking?
Comment 18 Andrey Loskutov CLA 2015-11-18 03:03:58 EST
(In reply to Stephan Herrmann from comment #17)
> The latest patch set in gerrit starts to show what is possible when mixing
> different stuff. 

Thanks Stephan!

> Whether this mix creates bad semantic effects still needs to be
> investigated. Is it usefull to include - say - a generic type with 1.5 null
> annotations into full 1.8-based null type checking?

You mean something like:
@Nullable <T> T getT();

And why this should be not included in 1.8 based analysis?
I would expect that 1.5 @Nullable return value acts for the NP analysis in the similar way as 1.8 nullable type, or do I miss something? But I'm clearly not an expert here, just a stupid user expectation.
Comment 19 Stephan Herrmann CLA 2015-11-19 11:03:37 EST
(In reply to Andrey Loskutov from comment #18)
> You mean something like:
> @Nullable <T> T getT();
> 
> And why this should be not included in 1.8 based analysis?
> I would expect that 1.5 @Nullable return value acts for the NP analysis in
> the similar way as 1.8 nullable type, or do I miss something? But I'm
> clearly not an expert here, just a stupid user expectation.

I hope so, too. In effect the 1.5 annotations should be re-interpreted as a subset of what 1.8 annotations can do. I just haven't seen any two features in Java being *really* orthogonal, so I'm still on the watch for s.t. where this game will create unexpected results. If we assume that the 1.5 annotations only appear in .class files, never in source (given the current project is at 1.8) this will actually save us from some hassle (like, e.g., fiddling with array types, qualified type names ...).
Comment 20 Stephan Herrmann CLA 2015-11-28 08:08:06 EST
(In reply to Stephan Herrmann from comment #13)
> ***************************************************************************
> *JDT leads* please let me know if you see any issues with adding these
> options and whether you have a strong preference for or against modifying
> interpretation of the old options vs. introducing new options.
> ***************************************************************************
> 
> 
> Once that general question is settled I will add the corresponding API in
> JavaCore plus the corresponding CLI arguments.

ping
Comment 21 Jay Arthanareeswaran CLA 2015-11-30 04:33:37 EST
(In reply to Stephan Herrmann from comment #13)
> ***************************************************************************
> *JDT leads* please let me know if you see any issues with adding these
> options and whether you have a strong preference for or against modifying
> interpretation of the old options vs. introducing new options.
> ***************************************************************************

I am fine with new options, if that lets us keeps some options open in future.

BTW, sorry if the generification of the CompilerOptions is causing you trouble for the String[] :)
Comment 22 Markus Keller CLA 2015-11-30 06:34:29 EST
(In reply to Stephan Herrmann from comment #13)
We definitely need a primary set of annotations, also so that Quick Fixes know which annotation to add.

We could also change the existing options into a list and then declare the first option to be the primary annotation.

(In reply to Stephan Herrmann from comment #15)
> (2) There are additional semantic differences in how @NonNullByDefault is
> defined in different libraries. [..]

We will need to make users aware of this restriction in the UI.

=> No strong preference from my side, but I guess separate options will cause less work for adopters.
Comment 23 Stephan Herrmann CLA 2015-12-03 08:33:02 EST
Thanks Jay and Markus.

Gerrit patch set 5 contains the new API constants in JavaCore, so people can comment.
Not yet included: new options for the batch compiler.
Comment 24 Stephan Herrmann CLA 2015-12-03 08:52:03 EST
Technical note:
Most of the change deals with the necessary switch in how we mark configured annotation types: with an unbounded list we can obviously no longer use predefined, well-known type-IDs. Instead I'm now using ReferenceBinding.typeBits, which perfectly matches the task at hand. With a little help from hasNullBit(int) this even simplifies the code in a few locations.
The conversion String<->String[] only happens inside CompilerOptions, so no penalty incurred by generification :)

In two places I included small improvements for consuming declaration annotations in a project whose primary null annotations are TYPE_USE: render appropriately for error messages to make sense; translate default of @NNBD into the corresponding set of DefaultLocation;s.
Comment 26 Stephan Herrmann CLA 2015-12-03 11:31:23 EST
To avoid getting into a rush against M4 deadlines I pushed the main changes.

I'm open to comments regarding the API javadoc.

The bug remains open until also corresponding batch compiler options have been added (M4).

Beyond that, bug 483566 will bring the necessary configuration UI (M5).

Plus I'm considering a new warning if a secondary annotation is used in source code.


With the current status adopters can start consuming the new preferences (by manually editing .settings/org.eclipse.jdt.core.prefs). However, I plan to announce this in the N&N only after the UI has been implemented, too.
FYI, the new preference keys are:
org.eclipse.jdt.core.compiler.annotation.nullable.secondary
org.eclipse.jdt.core.compiler.annotation.nonnull.secondary
org.eclipse.jdt.core.compiler.annotation.nonnullbydefault.secondary
See JavaCore.java ...
Comment 27 Eclipse Genie CLA 2015-12-06 08:44:58 EST
New Gerrit change created: https://git.eclipse.org/r/62067
Comment 29 Stephan Herrmann CLA 2015-12-06 12:40:09 EST
(In reply to Eclipse Genie from comment #28)
> Gerrit change https://git.eclipse.org/r/62067 was merged to [master].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=ce651c0ac69efcf9b95474a4774590a4a9111231
> 

This change adds support for the batch compiler.

For the batch compiler annotation names are grouped as (nullable|nonnull|nonnullbydefault). Supporting lists in each position between '|' didn't look compelling to me, so I decided to simply support specifying more than one "-warn:+nullAnnot(...)". Here the first set is taken as the primary annotations, avoiding the addition of a new option. 

If anybody argues that this slight inconsistency to the JavaCore options should be avoided we can simply add a new option also here, but I felt that this isn't necessary, since the option structure is already different (and distinction primary/secondary is less relevant for the batch compiler).

We have two follow-up tasks: bug 483566 (UI) and bug 483743 (Doc).
Comment 30 Manoj N Palat CLA 2016-01-25 04:43:17 EST
(In reply to Eclipse Genie from comment #25)
> Gerrit change https://git.eclipse.org/r/60624 was merged to [master].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/
> ?id=6dbe2813dd7277bd2252f3441ba09237b6b43790

Stephan: A minor correction found during verification in the test - I guess in the second test of NullAnnotationTest.testMultipleAnnotations() you meant to use options2 (instead of options1)?
Comment 31 Stephan Herrmann CLA 2016-01-25 14:22:12 EST
(In reply to Manoj Palat from comment #30)
> (In reply to Eclipse Genie from comment #25)
> > Gerrit change https://git.eclipse.org/r/60624 was merged to [master].
> > Commit:
> > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/
> > ?id=6dbe2813dd7277bd2252f3441ba09237b6b43790
> 
> Stephan: A minor correction found during verification in the test - I guess
> in the second test of NullAnnotationTest.testMultipleAnnotations() you meant
> to use options2 (instead of options1)?

Good catch, thanks.

As you probably saw, the key part of the test is the third compilation, but you're right, the intention was to compile each class with the appropriate configuration.

I tightened the test so that with option1 we would *not* see an expected warning, and then corrected this. Pushed as https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=155645260264d56d6f297342315412ebb0bad147
Comment 32 Manoj N Palat CLA 2016-01-28 01:46:02 EST
Verified for Eclipse Neon 4.6M5 Build id: Build id: I20160127-2000