Bugzilla – Full Text Bug Listing
| Summary: | Support JDT @NonNull and @Nullable annotations in Platform UI | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Brian de Alwis <bsd> | ||||
| Component: | UI | Assignee: | Platform-UI-Inbox <Platform-UI-Inbox> | ||||
| Status: | RESOLVED WONTFIX | QA Contact: | |||||
| Severity: | enhancement | ||||||
| Priority: | P3 | CC: | daniel_megert, david_williams, gautier.desaintmartinlacaze, jonah, Lars.Vogel, loskutov, malaperle, marcel.bruch, mistria, stephan.herrmann, tom.schindl | ||||
| Version: | 4.6 | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| See Also: |
https://bugs.eclipse.org/bugs/show_bug.cgi?id=478603 https://bugs.eclipse.org/bugs/show_bug.cgi?id=540433 |
||||||
| Whiteboard: | |||||||
| Bug Depends on: | 546725 | ||||||
| Bug Blocks: | 540433 | ||||||
| Attachments: |
|
||||||
|
Description
Brian de Alwis
+1000. Sooner is better. Note: since we are at Java 1.7 level, we can't use 2.x jdt annotations but must stay at 1.x, but this is not an issue. See for example changes in https://git.eclipse.org/r/50580/. Thanks for bringing this up :) (In reply to Brian de Alwis from comment #0) > The ideal would be to use JDT's org.eclipse.jdt.annotations bundle, but JDT > is downstream from the Platform. As with EMF, this shouldn't be impossible > to surmount? That annotation bundle is the simplest possible bundle with *no* dependencies (other than a JRE). This "shouldn't be" an obstacle. The main open question on this road: we'd normally want a compile-time-only dependency and most of our build technology doesn't have means to declare this *with proper version bounds*. Yet, when Platform uses these annotations it is crucial to explicitly request version [1.1.0,2.0.0) of the annotation bundle (unless the Platform moves to Java 8 :) ). OTOH, given the negligible size of the bundle, should we actually make it a regular dependency and thus avoid all the hassle with compile time dependencies? A regular dependency would also ensure that plugins consuming the annotated API don't have to worry where to get the annotation bundle, it will already be present when you touch the annotated API. +1 for moving platform ui to Java 8, if that makes this change easier. Current Jettty version (used in help) also requires Java 8 and JFace data binding is planned to move also to Java 8 to support the new Date Time API. (In reply to Lars Vogel from comment #3) > +1 for moving platform ui to Java 8, if that makes this change easier. > Current Jettty version (used in help) also requires Java 8 and JFace data > binding is planned to move also to Java 8 to support the new Date Time API. -1 for Java 8 for platform (it deserves at least an extra discussion). I doubt Java 8 adoption is high enough, see for example https://ianskerrett.wordpress.com/2014/06/23/eclipse-community-survey-2014-results/ or https://plumbr.eu/blog/java/java-version-statistics-2015-edition or https://www.typesafe.com/company/news/survey-of-more-than-3000-developers-reveals-java-8-adoption-ahead-of-previous-forecasts. We will have lot of work with Java 7 anotations already, you will see, it is like opening a pandora box. (In reply to Andrey Loskutov from comment #4) > (In reply to Lars Vogel from comment #3) > > +1 for moving platform ui to Java 8, if that makes this change easier. > > Current Jettty version (used in help) also requires Java 8 and JFace data > > binding is planned to move also to Java 8 to support the new Date Time API. > > -1 for Java 8 for platform (it deserves at least an extra discussion). I > doubt Java 8 adoption is high enough, see for example The current rule is that is a platform project needs a feature in a higher Java version it can update its BREE. As for Java 7, it is not officially maintained (for free) and Neon is still a year from here, adaption will increase over time. If Platform remains at 1.7, just try to avoid the primary situation that would cause pain during migration to 1.8 later (see also [1]): @NonNull java.lang.String x // annotations next to a qualified type name Additionally annotations next to array types will invariably need to be changed from @NonNull String[] x to String @NonNull[] x If Platform migrates to 1.8 nullness can be specified more precisely, but as Andrey says, this can be a bit overwhelming on a large existing code base... I guess the build issue - need to specify dependency as [1.1.0,2.0.0) - should not seriously count as a reason against 1.7. Apart from the two incompatibilities mentioned above and in [1] using declaration annotations now and migrating to type annotations (Java 8) later should be a "compatible" path. [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 And to emphasis this, AFAIK the officially maintained Jetty version (used in help) already requires Java 8. So if Neon want to ship with a maintained (with security fixes, etc.) help system, we would have to use Java 8 anyway. See Bug 470227. (In reply to Andrey Loskutov from comment #4) > (In reply to Lars Vogel from comment #3) > > +1 for moving platform ui to Java 8, if that makes this change easier. > > Current Jettty version (used in help) also requires Java 8 and JFace data > > binding is planned to move also to Java 8 to support the new Date Time API. > > -1 for Java 8 for platform (it deserves at least an extra discussion). I > doubt Java 8 adoption is high enough, see for example > https://ianskerrett.wordpress.com/2014/06/23/eclipse-community-survey-2014-results/ > or https://plumbr.eu/blog/java/java-version-statistics-2015-edition or > https://www.typesafe.com/company/news/survey-of-more-than-3000-developers-reveals-java-8-adoption-ahead-of-previous-forecasts. > > > We will have lot of work with Java 7 anotations already, you will see, it is > like opening a pandora box. I agree with this. FWIW: ~75% of all error reports in the past 3 weeks come from users using Java 8 or higher. We received ~125K error reports from something in between 10-20K users (don't have the exact numbers at hand). This is no call to go with Java 8 but I thought it this might be interesting numbers for the BREE part of this discussion. (In reply to Marcel Bruch from comment #9) > FWIW: ~75% of all error reports in the past 3 weeks come from users using > Java 8 or higher. We received ~125K error reports from something in between > 10-20K users (don't have the exact numbers at hand). > > This is no call to go with Java 8 but I thought it this might be interesting > numbers for the BREE part of this discussion. It could also mean that more issues happen when Java 8 is used ;-). (In reply to Stephan Herrmann from comment #2) > Thanks for bringing this up :) > > (In reply to Brian de Alwis from comment #0) > > The ideal would be to use JDT's org.eclipse.jdt.annotations bundle, but JDT > > is downstream from the Platform. As with EMF, this shouldn't be impossible > > to surmount? > > That annotation bundle is the simplest possible bundle with *no* > dependencies (other than a JRE). This "shouldn't be" an obstacle. > > The main open question on this road: we'd normally want a compile-time-only > dependency and most of our build technology doesn't have means to declare > this *with proper version bounds*. Yet, when Platform uses these annotations > it is crucial to explicitly request version [1.1.0,2.0.0) of the annotation > bundle (unless the Platform moves to Java 8 :) ). We have migrated (parts) of Eclipse platform to Java 8. Can you point to information how to use these annotations, now that the moved to Java 8? (In reply to Lars Vogel from comment #11) > We have migrated (parts) of Eclipse platform to Java 8. Can you point to > information how to use these annotations, now that the moved to Java 8? It would be cool if you just start with the official help: Basic concepts: - http://help.eclipse.org/mars/topic/org.eclipse.jdt.doc.user/tasks/task-using_null_annotations.htm?cp=1_3_9_0 Update for Java 8: - http://help.eclipse.org/mars/topic/org.eclipse.jdt.doc.user/tasks/task-using_null_type_annotations.htm?cp=1_3_9_1 And optionally for external annotations: - http://help.eclipse.org/mars/topic/org.eclipse.jdt.doc.user/tasks/task-using_external_null_annotations.htm?cp=1_3_9_2 I'm eager to learn what additional help / explanation / advice would be needed. Lars, in bug 478603 you plan to use null annotations for IWorkbench API. Shouldn't we first configure & turn on NPE analysis in all (or a subset of) platform projects (except tests)? Similar to what we did for jgit & Co (https://git.eclipse.org/r/#/c/50580/)? Only after you properly configure the projects you will be able to properly annotate interfaces & classes, otherwise you might not see the breaking effect of the *wrong* annotations at all. It is pretty easy to make the projects not compilable after adding annotations at wrong places, but without NPE analysis enabled on a project you will notice this only if it is already too late :-). (In reply to Andrey Loskutov from comment #13) > Lars, in bug 478603 you plan to use null annotations for IWorkbench API. > > Shouldn't we first configure & turn on NPE analysis in all (or a subset of) > platform projects (except tests)? Similar to what we did for jgit & Co > (https://git.eclipse.org/r/#/c/50580/)? Sounds good. +1 for the enablement. I assume without any annotations, this does not make a difference. I'm "stealing" this bug for Platform UI. David: do we need to do something special such as have org.eclipse.jdt.annotations be built as a -1 dependency like EMF Core? Created attachment 258776 [details] Example of how to specify a compile time dependency > David: do we need to do something special such as have > org.eclipse.jdt.annotations be built as a -1 dependency like EMF Core? I am not sure that EMF -1 is a great analogy, but I have just barely skim read all the details in this bug. Is the main question, still, "how to have a compile time only" dependency? If so, then the attached patch is an example. I say an example because I created it for "org.eclipse.ui" bundle, and I am not sure that is really a bundle that needs it? I am not sure you can add such a "build block" to your repository pom -- maybe, just not sure. Plus, is seems to me you do have some UI bundles at Java 8, already, just not all. In that case, I am not sure what the effects would be. But the patch shows the general approach to having a "compile time only" dependency, which is not "required" by the bundle. But is on the classpath at compile time. Sound close to what you need? (In reply to David Williams from comment #17) > Sound close to what you need? Not exactly, but yes. I think we're happy to use the using the recommended approach to use optional require-bundle on the annotations bundle [1]. But your comment is just what I was looking for: In a full build, Tycho will order things so that o.e.jdt.annotation is compiled first. In a Gerrit build, is it will be pulled from latest "eclipse repository" (currently 4.6-I-builds) Basically we don't need to do anything special as JDT is built as part of the Platform? [1] http://help.eclipse.org/mars/topic/org.eclipse.jdt.doc.user/tasks/task-using_null_annotations.htm?cp=1_3_9_0_2#buildpath_setup (In reply to Brian de Alwis from comment #18) > (In reply to David Williams from comment #17) > > Sound close to what you need? > > Not exactly, but yes. I think we're happy to use the using the recommended > approach to use optional require-bundle on the annotations bundle [1]. But > your comment is just what I was looking for: > > In a full build, Tycho will order things so that > o.e.jdt.annotation is compiled first. In a Gerrit build, > is it will be pulled from latest "eclipse repository" > (currently 4.6-I-builds) > > Basically we don't need to do anything special as JDT is built as part of > the Platform? > > [1] > http://help.eclipse.org/mars/topic/org.eclipse.jdt.doc.user/tasks/task- > using_null_annotations.htm?cp=1_3_9_0_2#buildpath_setup After reading that "help" section, I think I see that for this to work "in the workbench" you may have to do as it says, with the optional import. Since PDE does not use the "POM information". So, to answer your initial question another way, if you just wanted to use 'annotations' as a "prereq' instead of "part of the build", then I think you would want to do several things. a. If no other dependencies on "IDE code", then just as well be in a separate Git repository, and have a it's own build, just to build exactly those to bundles. b. I think they would have to go into their own p2 repository, which I would see as a "child" of our normal repositories (such as 4.6-I-buids) just for compatibility. c. Presumably the "annotations build" would build only "on demand" or when a change detected, since I do not think they change much. Perhaps the main (if not only) advantage to that sort of set up is that it does reduces the "cross-repository" dependencies between our components, which in the long run is necessary to make our builds less monolithic. The disadvantage is that it is more work. :) (but, not impossibly more). Mass move to M6 (In reply to Andrey Loskutov from comment #13) > Lars, in bug 478603 you plan to use null annotations for IWorkbench API. > > Shouldn't we first configure & turn on NPE analysis in all (or a subset of) > platform projects (except tests)? Similar to what we did for jgit & Co > (https://git.eclipse.org/r/#/c/50580/)? I suggest to start smaller, and a first "only" activate the the normal "Null pointer access" in platform UI via Bug 491888. I'd like to use those annotations in the generic editor bundle (part of Platform Text, new with Oxygen). Is this fine? Can I simply add the JDT annotations bundle as an optional dependency? (In reply to Mickael Istria from comment #22) > I'd like to use those annotations in the generic editor bundle (part of > Platform Text, new with Oxygen). Is this fine? Can I simply add the JDT > annotations bundle as an optional dependency? https://git.eclipse.org/r/#/c/62344/ does it, but Stephan, wasn't there some magic build.properties property? I ask because JDT depends on platform and not other way around, not that we will get some crazy cycles in the maven/tycho build. (In reply to Andrey Loskutov from comment #23) > (In reply to Mickael Istria from comment #22) > > I'd like to use those annotations in the generic editor bundle (part of > > Platform Text, new with Oxygen). Is this fine? Can I simply add the JDT > > annotations bundle as an optional dependency? > > https://git.eclipse.org/r/#/c/62344/ does it, but Stephan, wasn't there some > magic build.properties property? I ask because JDT depends on platform and > not other way around, not that we will get some crazy cycles in the > maven/tycho build. Like this? http://git.eclipse.org/c/tracecompass/org.eclipse.tracecompass.git/tree/tmf/org.eclipse.tracecompass.tmf.core/build.properties additional.bundles = org.eclipse.jdt.annotation jars.extra.classpath = platform:/plugin/org.eclipse.jdt.annotation The problem with build.properties is that you can't point it to a specific version which is important if you want to use type annotations (In reply to Thomas Schindl from comment #25) > The problem with build.properties is that you can't point it to a specific > version which is important if you want to use type annotations Exactly. An optional dependency is the best approximation of a versioned build time dependency. Also o.e.j.annotation doesn't depend on anything (besides JRE), so semantically, depending on this bundle cannot possibly create a cycle (unless you *are* JRE :) ). Such dependency only requires that you have access to some repo, that contains the annotation bundle from a previous build. repo.eclipse.org has it and meanwhile even maven central has it, so plenty of places to find it. (In reply to Thomas Schindl from comment #25) > The problem with build.properties is that you can't point it to a specific > version which is important if you want to use type annotations We control that with versioning it in the target. But there is also the fact that (maybe I'm wrong!) if you install the plugin (platform UI) and JDT Annotation is in the target, then it will get installed and take space in the installation even thought it's not required. Not a big problem but I thought I'd bring it up that possibility. (In reply to Marc-Andre Laperle from comment #27) > (In reply to Thomas Schindl from comment #25) > > The problem with build.properties is that you can't point it to a specific > > version which is important if you want to use type annotations > > We control that with versioning it in the target. Does that affect entries from build.properties as well? Still, the build would be more fragile, if versioning information is not directly captured in the project, don't you think? > But there is also the fact > that (maybe I'm wrong!) if you install the plugin (platform UI) and JDT > Annotation is in the target, then it will get installed and take space in > the installation even thought it's not required. Not a big problem but I > thought I'd bring it up that possibility. You're not worried about adding 25kbyte to an installation, are you? If so, you could file a ticket against JDT/Core because the size could actually be much reduced (jar contains an unneeded source file taking most of the space). (In reply to Stephan Herrmann from comment #28) > You're not worried about adding 25kbyte to an installation, are you? If so, > you could file a ticket against JDT/Core because the size could actually be > much reduced (jar contains an unneeded source file taking most of the space). Not really. I pointed it out in case someone does care. In Bug 540433 we started using the null annotations. Please open specific bugs for areas in which you would like to see use them. RESOLVED / FIXED, looks great, but without a Target Milestone nor commit, what exactly has been fixed? Or has any commit been made for this bug? Seeing Bug 540433 re-opened, shouldn't we re-open this bug to resolve the architectural question here? (In reply to Stephan Herrmann from comment #31) > RESOLVED / FIXED, looks great, but without a Target Milestone nor commit, > what exactly has been fixed? Or has any commit been made for this bug? > > Seeing Bug 540433 re-opened, shouldn't we re-open this bug to resolve the > architectural question here? Yes, please reopen (In reply to Lars Vogel from comment #32) > Yes, please reopen @Till, saw your JDT email that using non-null annotations would be useful. AFAIK it is still not possible to use non-null annotations in platform or JDT code, see this bug. If somebody could summarize why you are not able to create a dependency on org.eclipse.jdt.annotation[2.0.0,3.0.0) then we could advise, but I think all options are openly on the table. Perhaps we are blocked because people dislike defining a optional dependency where we want a compile time dependency. In that case, feel free to extend PDE & tycho to support a notion of versioned compile time plugin dependency. Until then optional dependency is it. Perhaps we are blocked because people are afraid of a cycle, but that cycle only exists on the levels of namespaces / repositories / builds. Using a previously built version of the annotation should be perfectly fine. No cycle at the plugin level. JUST DO IT :) (In reply to Stephan Herrmann from comment #35) > JUST DO IT :) Does JDT use the annotations JDT? If yes, we can look at the setup. (In reply to Lars Vogel from comment #36) > (In reply to Stephan Herrmann from comment #35) > > > JUST DO IT :) > > Does JDT use the annotations JDT? If yes, we can look at the setup. The small configuration burden had long been resolved in https://git.eclipse.org/r/#/c/62344 If you see problems, ask specific questions. (In reply to Stephan Herrmann from comment #37) > (In reply to Lars Vogel from comment #36) > > (In reply to Stephan Herrmann from comment #35) > > > > > JUST DO IT :) > > > > Does JDT use the annotations JDT? If yes, we can look at the setup. > > The small configuration burden had long been resolved in > https://git.eclipse.org/r/#/c/62344 > > If you see problems, ask specific questions. You avoid answering the question if JDT uses its own annotations. Does it? (In reply to Lars Vogel from comment #38) > You avoid answering the question if JDT uses its own annotations. Does it? That was on purpose because I think it's not relevant here :) But here it is anyway: no we don't yet use null annotations on JDT code, but everybody contributing to that functionality uses it on their code outside JDT. Thanks Stephan for the information. Once JDT starts consuming null annotations themself, we will also start this. Consumption should IMHO start in the area in which something is developed. |