Bug 526011 - PDE should provide an abstraction above annotation-configuration in .classpath
Summary: PDE should provide an abstraction above annotation-configuration in .classpath
Status: RESOLVED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.7   Edit
Hardware: PC Windows NT
: P3 normal (vote)
Target Milestone: 4.24 M1   Edit
Assignee: Stephan Herrmann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-13 11:52 EDT by Ed Willink CLA
Modified: 2022-04-12 17:49 EDT (History)
5 users (show)

See Also:


Attachments
toy example (7.73 KB, application/zip)
2021-01-28 16:03 EST, Stephan Herrmann CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ed Willink CLA 2017-10-13 11:52:07 EDT
Further to https://dev.eclipse.org/mhonarc/lists/tycho-user/msg07793.html in which custom annotations failed under Tycho.

".classpath is an IDE-specific and derived file so it's not taken into account by the headless Tycho build (as opposed to e.g. build.properties)."

Therefore .classpath should not be the primary location for the annotationpath; it should be somewhere where both IDE and Tycho can access it.
Comment 1 Ed Willink CLA 2017-10-13 12:15:28 EDT
This underlying problem is discussed in Bug 486868.

If build.properties is used as suggested in comment 3, it would be natural to support a comma-separated list and so solve the derived project re-use problem.
Comment 2 Stephan Herrmann CLA 2017-10-13 15:39:17 EDT
There are plenty of projects that don't have build.properites but have .classpath, hmm?
Comment 3 Ed Willink CLA 2017-10-14 03:49:41 EDT
If the primary definition is contained in a Manifest editor input file, just like e.g. the BREE, then the automated/manual update .classpath generation can update the classpath with <attributes> in the same way as the <classpathentry>. The Manifest editor can also provide a more conventional UI.

With hindsight this could have been done as the correct way to handle Bug 490496.

Now we need a manifest editor warning/quick fix to help rescue the downstream attributes.
Comment 4 Stephan Herrmann CLA 2017-10-14 07:34:07 EDT
I begin to feel that this is not a request for JDT, but for PDE.

Let me explain: JDT knows nothing about build.properties, and has very limited knowledge of MANIFEST.MF (just wrt standard jar file headers).

Ergo, for JDT .classpath is the only possible location to describe additional properties concerning dependencies. For plain Java projects, .classpath is *the* single source, not a derived file as discussed in the linked post on tycho-users.

PDE, however, can create another level of abstraction on top of this.

(A) PDE can invent any additional editor and file format where the user specifies these things in the most suitable UI

(B) PDE has at least two options for passing that information down to JDT:
(B.1) Automatically generate corresponding attributes into .classpath, or
(B.2) Let resolving of the Plug-in Dependencies classpath container insert the necessary attributes at this API level (i.e., without modifying .classpath).

At this point PDE is also free to negotiate with tycho, which location and format would be most suitable for consumption in tycho builds.


BTW: I am actually sceptical regarding any approach like (B.1) that generates stuff into .classpath. The reason: it is far too easy for one tool to generate stuff that will break another, independent tool. Every project that stores models in src/main/resources is broken by m2e generating exclusion filters "**" into .classpath. This file should better be hidden rather than each tool messing up others, IMHO.
Comment 5 Ed Willink CLA 2017-10-14 08:17:03 EDT
(In reply to Stephan Herrmann from comment #4)
> I begin to feel that this is not a request for JDT, but for PDE.

Yes. From the JDT developer's perspective this makes sense. But from an Eclipse Plugin Developer's point of view it is internal squabbling. It is unfortunate that the new JDT functionality was not fully considered at the plugin level earlier.

I recall that I did point out in Bug 486868 that e.g. if EMF has custom annotations, then any consumer of EMF must be able inherit those annotations. I failed to make headway with this observation, probably because the PDE-level problem was not relevant at the JDT-level.

Once the MANIFEST.MF/build.properties/... has the annotations definitions, my concerns that the e.g. EMF annotations are correctly inherited whether EMF is installed as plugins or as checked out projects should be soluble.
Comment 6 Stephan Herrmann CLA 2017-10-14 08:33:19 EDT
(In reply to Ed Willink from comment #5)
> (In reply to Stephan Herrmann from comment #4)
> > I begin to feel that this is not a request for JDT, but for PDE.
> 
> Yes. From the JDT developer's perspective this makes sense.

The bug was filed for JDT developers, so a JDT developer responds, surprised? :)

> But from an Eclipse Plugin Developer's point of view it is internal squabbling.

As always you are invited to participate in this internal - discussion.

So let's hear some proposals for:

- which file *and format* should be used in PDE land (and tycho) for specifying external annotation locations?

- where in the UI should this be surfaced, and how?
Comment 7 Ed Willink CLA 2017-10-14 10:20:17 EDT
OK. I'll take a shot.

Uniformity.

Currently in OCL, I have to specify that each of my plugins gets custom annotations from a selected annotation provider plugin. In QVTd, I have to do the same all over again, but I cannot reuse those from the OCL annotations provider. IMHO in the same way that a class defined in one package is useable everywhere, a custom annotation should be too. We just have the extra complexity that since custom annotations are a fixup, multiple annotations sources may have different possibly conflicting fixup definitions. Suggest, the strongest fixup is used, and the conflicts are INFOed. I suspect that this lack of uniformity accounts for some of the funnies that I see. e.g. if OCL has a fixup up for java.util.Map.get, but EMF does not. When I stop at a use of Map.get in the debugger, or editor, is the reference an OCL or an EMF  reference. Depending on the choice, JDT may or may not use the fixup. This flakiness is no doubt aggravated by the almost inevitable imperfections in the incremental builders.

Definition.

A plugin needs to declare that it is a simple annotations provider. Suggest in the MANIFEST.MF, "Eclipse-Annotations: path-to-annotations". By default the annotations are only used within the host bundle.

Export.

It may/may not be desired to publish the annotations for others. Suggest: "Eclipse-ExportAnnotations: this-bundle,that-bundle". This defines the host bundle as a composite annotations provider. By default the annotations are only used within the host bundle.

Import.

A bundle may be an annotation consumer. Explicit imports are a PITA, but if a re-used project suddenly adds/changes its annotations content this may be a very unpleasant surprise. Suggest import is disabled by default. Suggest "Eclipse-ImportAnnotations: bundle1,bundle2,bundle3" enables the annotations from annotation providers bundle1,bundle2,bundle3 to be used within this bundle.

Therefore in my-annotations-provider-plugin:

Eclipse-Annotations: path-to-my-annotations
Eclipse-ExportAnnotations: my-annotations-provider-plugin,
   your-annotations-provider-plugin,
   platform-annotations-provider-plugin

and in my other plugins

Eclipse-ImportAnnotations: my-annotations-provider-plugin

IDE.

Users can just hack the manifest, but better to add support to edit the Eclipse-Annotations.

Legacy fix-up.

If the .classpath writer detects an undeclared <classpathtry><attributes> for a location in the host plugin, a legacy rescue could be activated. ? an editor validation warning ? a writer pop-up dialog.
Comment 8 Ed Willink CLA 2017-10-14 10:32:15 EDT
NB. The 'Eclipse way' might be for each annotations provider to use an extension point to publish its annotations. However this would mean that when I install the Buggy application it may infect my builds with buggy annotations.
Comment 9 Ed Willink CLA 2017-10-16 06:04:59 EDT
(In reply to Ed Willink from comment #7)
> Suggest, the strongest fixup is used, and the conflicts are INFOed.

"stronger" is subjective. Suppose

Project Plausible defines @NonNull String Object::toString().
Project Paranoid defines @Nullable String Object::toString().

Which definition is stronger when project PlausiblyParanoid imports both?

The annotation provider imports should be a prioritized list. The earliest definition dominates. A validating UI may alert to the conflict hazards. The first definition is 'writeable'; i.e. where the JDT quick fixes are written.

Thus project PlausiblyParanoid may define a prioritized list of:

PlausiblyParanoid annotations
PlausiblyParanoidFixup annotations
Plausible annotations
Paranoid annotations

PlausiblyParanoidFixup can resolve the conflicts between Plausible and Paranoid in a hopefully very small set of definitions that can be regularly reviewed in conjunction with the Plausible and Paranoid developers.
Comment 10 Eclipse Genie CLA 2019-10-08 17:00:16 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.
Comment 11 Ed Willink CLA 2019-10-09 00:39:29 EDT
This is absolutely not stale. The absence of PDE support for JDT's @NonNull annotations undermines the utility of the facility in a multi-project usage.
Comment 12 Ed Willink CLA 2020-06-06 03:03:33 EDT
Bug 414237#c28 provides the very sad news that Stephan Herrmann is leaving JDT. The underlying JDT variants of this bug have therefore been WONTFIXed. consequently the chances of this bug being fixed are rather limited.

Bug 564014 discusses derisking the ongoing use of @NonNull / @Nullable annotations for the OCL (and QVTd) projects.
Comment 13 Ed Willink CLA 2020-12-08 08:44:59 EST
Bug 569379 reports that @NonNull can give strange failures with Tycho 2.1.0 but not 1.6.0.
Comment 14 Stephan Herrmann CLA 2021-01-28 14:41:32 EST
I have been playing a bit with this issue, and it seems the simplest thing that I can offer is the following:

* recommend to declare the external annotation folder as a source folder
  => build will include its content at the root of the jar, 
     no annotation path inside that jar necessary.

* invent a simple MANIFEST header like
     Eclipse-ExportExternalAnnotations: true

Then when PDE resolves a plugin dependency, and when it finds this header, it can internally set an annotationPath equal to the plugin location, which JDT can consume with no code changes needed.

This has the extra charm, that PDE doesn't need to care about the number of folders for external annotations, if all are source folders associated to the default output location, then all .eea are automatically picked up (make sure you have no classpath exclusions, or explicitly include *.eea).

Of course, later we can think of a warning if header and annotation path and source folders are out of sync.

@Ed, I'm pretty sure this would elegantly resolve this bug and bug 486868. Would it suit your needs? 

@Vikas, I believe, Eclipse is free to define new manifest headers, prefixed "Eclipse-". Can we just decide about the above, or is some form of approval necessary?
Comment 15 Stephan Herrmann CLA 2021-01-28 16:03:33 EST
Created attachment 285411 [details]
toy example

import the attached project into a workspace with the change installed to see it working already:

1. P1/src-gen/p1/CGen.java has a method annotated via .eea (thanks to bug 509397).

2. P1/src/p1/C.java violates the contract of that annotation, just to show an error

3. P1 sets the new header in MANIFEST.MF

4. P2 depends on P1 and its CGen, P1/src/pt/Test.java shows the same error

5. a second way of testing: hover of calls to m(), signature shows "@NonNull String s"

6. export P1 as deployable plug-in and add that plugin to the target platform

7. now closing project P1 does not affect P2, the error will still be raised,
   the same if you re-open P1.
Comment 16 Ed Willink CLA 2021-01-29 04:41:53 EST
(In reply to Stephan Herrmann from comment #14)
> @Ed, I'm pretty sure this would elegantly resolve this bug and bug 486868.
> Would it suit your needs? 

I don't quite follow the solution and had half expected build.properties to be involved since that has some wacky path stuff, but MANIFEST.MF can be good too.

I am really encouraged that you are looking at this PDE level issue.

My main query is ensuring that the 'correct' EEA for e.g. Map.get() is used when three different projects each provide an applicable EEA. And of course while single stepping the platform, all EEAs should be ignored. After stepping into a function with a Map parameter, does the parameter have the EEA of the caller or the callee? probably a merge from a debug perspective, but callee from a compiler modularity perspective..
Comment 17 Stephan Herrmann CLA 2021-01-29 10:48:39 EST
(In reply to Ed Willink from comment #16)
> (In reply to Stephan Herrmann from comment #14)
> > @Ed, I'm pretty sure this would elegantly resolve this bug and bug 486868.
> > Would it suit your needs? 
> 
> I don't quite follow the solution 

How could I help? comment 14 has two simple bullet items, which describe all that users need to know. Anything unclear about these items?

> and had half expected build.properties to
> be involved since that has some wacky path stuff, but MANIFEST.MF can be
> good too.

*some* addition in MANIFEST.MF seems unavoidable, since after packaging build.properties is no longer present. But, yes, when you declare your annotation folder as a source folder, then PDE will add an entry to build.properties, too (which is needed for packaging).
 
> I am really encouraged that you are looking at this PDE level issue.

Luckily I'm interested in having this feature myself :)


> My main query is ensuring that the 'correct' EEA for e.g. Map.get() is used
> when three different projects each provide an applicable EEA. 

This may require one more step after this one. Currently, the eea exported from plug-in A are applied only to classes from that plugin (since it's an attribute of that particular classpath attribute). This highlights the connection to bug 509397. When, where and how eeas are applied to other classpath entries (in your case JRE) still needs some more work. 

I would like to serarate two things, though: 

(a) What eeas does a client plugin need to apply in order to correctly view the API of plug-in A? Here I focus on annotations directly or indirectly affecting the API of that plug-in A.

(b) Can eeas be "shared" between projects / plug-ins? (Didn't lastnpe.org have a soluation for that?)

Interestingly, (a) has a sub-issue, which too needs more thought:

* plug-in A requires plug-in B (which is not annotated)
** a class CA from A extends a class CB from B, applying eea to B
* client plug-in needs to see A with B.eea applied for signatures of inherited methods

This implies, we need to propagate annotations that A defines for B to the classpath entry for B.

> And of course
> while single stepping the platform, all EEAs should be ignored. After
> stepping into a function with a Map parameter, does the parameter have the
> EEA of the caller or the callee? probably a merge from a debug perspective,
> but callee from a compiler modularity perspective..

I don't see the relevance of eea (a feature for static analysis) with regard to step debugging (a runtime concept).


It this point I'm ready to admit, that associating annotation paths to individual classpath entries, not to the entire build path is cumbersome.

Unfortunately, that was the best place in JDT's abstraction of a build path which we could find back when we introduced eeas. JDT is not prepared for attaching any attributes to the build path as such.
Comment 18 Stephan Herrmann CLA 2021-01-29 11:05:39 EST
Pragmatically, even if the current change doesn't solve all problems, it could well be all that is needed in PDE. With this the information is handed over to JDT.

We could than open a new bug against JDT to discuss cross-applying the eeas from one classpath entry also on other entries.
Comment 19 Stephan Herrmann CLA 2021-01-29 11:08:12 EST
I'm still a bit weary about performance (if looking for an eea requires opening many jars). In fact this leads me to a variant worth discussing:

Instead of exporting a single flag via MANIFEST.MF, we could add smth like

Eclipse-ExportAnnotationPackage: java.util,
  org.eclipse.ocl.foo,
  org.eclipse.ocl.bar

Theoretically this would be very handy for JDT: when resolving a class in package org.eclipse.ocl.foo always look here for attached eea.

This would entail to work items:
* validation & quickfix for updating this header from actual .eea in the project
* a new classpath attribute to communicate the list of packages to JDT

To decide about relevance of this performance issue I should probably experiment with a tweaked JDT that indeed looks for .eaa far and wide (not respecting boundaries between classpath entries). Yes, that's what I'll do next.
Comment 20 Ed Willink CLA 2021-01-29 12:46:52 EST
(In reply to Stephan Herrmann from comment #17)
> > I don't quite follow the solution 
> 
> How could I help? 

Probably I just didn't take time to study the detail. I look forward to a prototype.

> I don't see the relevance of eea (a feature for static analysis) with regard
> to step debugging (a runtime concept).

Considering what is visible in the debugger is perhaps overthinking it or considering a debug aid for inconsistent caller/callee declarations - probably statically possible anyway...

At any analysis context, the code currently belongs to a given class. Presumably it is the EEAs of this class's bundle and all its superclasses' bundles that 'merge'/'occlude' to give the effective EEAs at the analysis context. Conflicts between inherited EEAs are therefore statically determinate at the analysis point. There is therefore no ambiguity as to which Map.get() EEA to use.

Static analysis of an operation call can contrast the effective EEA seen by the caller with the effective EEA in force in the callee.

(In reply to Stephan Herrmann from comment #19)
> I'm still a bit weary about performance (if looking for an eea requires
> opening many jars).

I would expect that you have already read all the 'interesting' MANIFEST.MF, so if the architecture is like OCL and QVTd; all annotations in org.eclipse.ocl.pivot or org.eclipse.qvtd.pivot.qvtbase (without an ability to import / share). Each plugin's .classpath references its common EEA plugin. Perhaps Eclipse-ImportAnnotationBundle: org.eclipse.ocl.pivot throughout the secondary bundlesand Eclipse-ExportAnnotationPackage: ... in the cmmon/EEA bundle. 

So potentially the MANIFEST.MF in a plugin you have already loaded redirects to the common EEA plugin that you load if not already loaded, so in the worst case for the whole of OCL and QVTd you would only load two plugins on demand.
Comment 21 Stephan Herrmann CLA 2021-02-09 08:16:43 EST
FYI, I'm taking a little detour back through JDT/Core business: bug 571055.

This is relevant for the topic at hand because all the magic we are doing in the IDE must be mappable to CI builds, too. My central idea there is: in CI builds we could get away with this simple story:
* put all .eea onto the classpath
* provide "-annotationPath CLASSPATH"

Done.

Once that is settled, it would tilt the balance towards a more generic solution: rather than individual tuning for each and every classpath entry, also the PDE case could search for .eea on the entire classpath (of a given bundle, i.e.). We'd only need a compiler preference akin to "-annotationPath CLASSPATH". If that proves useful, we might raise an info if:
* project is a Plug-in project
* project has null annotations and eea enabled
* project still uses per-classpath-entry configuration of .eea (then discouraged)

We'd still have some options re the cooperation JDT-PDE.

This is a significant change in strategy (see also last para in comment 17), but (pending performance analysis) it looks the simplest to me.
Comment 22 Stephan Herrmann CLA 2021-07-01 12:38:54 EDT
(In reply to Stephan Herrmann from comment #21)
> FYI, I'm taking a little detour back through JDT/Core business: bug 571055.
> 
> This is relevant for the topic at hand because all the magic we are doing in
> the IDE must be mappable to CI builds, too. My central idea there is: in CI
> builds we could get away with this simple story:
> * put all .eea onto the classpath
> * provide "-annotationPath CLASSPATH"
> 
> Done.
> 
> Once that is settled, it would tilt the balance towards a more generic
> solution: rather than individual tuning for each and every classpath entry,
> also the PDE case could search for .eea on the entire classpath (of a given
> bundle, i.e.). We'd only need a compiler preference akin to "-annotationPath
> CLASSPATH". 

Over to bug 574603
Comment 23 Stephan Herrmann CLA 2022-03-31 14:23:21 EDT
Thanks Vikas, Andrey for privately nudging me towards a decision :)

I rebased the change and smoke-tested that it still does as advertised.

If you don't mind, here's my plan:

1. merge the gerrit now

2. use the next I-build for experiments in real world projects (yes, meanwhile I have
   such projects set up for these experiments).

3. decide before M3 if the approach carries its own weight / needs more sophistication ...
3.a. polish, or:
3.b. remove

WDYT?

I had been hoping that bug 574603 provides a one-size-fits-all solution, but as we already had reports about performance penalties from that feature I'm inclined to continue the search for more precise specification of eea locations.


For further testing, do you have a hint what exactly triggers PDE to re-compute its internal models? Project clean / close / open doesn't seem to do it (for external plug-ins, which is a focus here). Modifying the target platform does, but that's quite an expensive operation ...
Comment 25 Andrey Loskutov CLA 2022-04-07 03:39:03 EDT
(In reply to Eclipse Genie from comment #24)
> Gerrit change https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/175466 was
> merged to [master].
> Commit:
> http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/
> ?id=3f9f33643122a8109460e88362593d67fde42ed5

Stephan, can this bug be now closed, or is there anything to add?
Do we need some N&N entry?
Comment 26 Vikas Chandra CLA 2022-04-07 08:51:20 EDT
The N&N repo is https://github.com/eclipse-platform/www.eclipse.org-eclipse-news
in case an entry is required.
Comment 27 Stephan Herrmann CLA 2022-04-07 18:01:18 EDT
Thanks for the reminders. I don't feel quite ready for a N&N entry, yet, as I suspect that JDT may need some more work for this to smoothly work for end users. I'll make a note to write N&N when it's ready.
Comment 28 Stephan Herrmann CLA 2022-04-09 09:13:45 EDT
I have a first follow-up issue against JDT: https://github.com/eclipse-jdt/eclipse.jdt.core/issues/5

(Not sure how the linking across bug trackers is supposed to work ...)
Comment 29 Ed Willink CLA 2022-04-11 13:31:10 EDT
Common 14 had two bullet items but it is not clear to me how these relate to the 'final' solution.

Is there an example project that demonstrates the new facilities?

Is there something I can do to exercise the changes?
Comment 30 Stephan Herrmann CLA 2022-04-12 17:49:09 EDT
(In reply to Ed Willink from comment #29)
> Common 14 had two bullet items but it is not clear to me how these relate to
> the 'final' solution.

Let me try again:

> * recommend to declare the external annotation folder as a source folder
>   => build will include its content at the root of the jar, 
>      no annotation path inside that jar necessary.

The purpose here is to declare annotations within a plug-in A in such a way that those can superimpose generated classes from A¸ and to do so in a way that can be consumed in all relevant scenarii:

- while compiling Plug-in A
- while consuming Plug-in A as a Project
- while consuming Plug-in A in jar-shape

To do so simply go to project properties Java Build Path > Source > Add Folder and select the annotations folder. No new technology involved. Now the annotations are available for the 3 mentioned scenarii, but we still need to tell downstream plug-ins where to look.
 
> * invent a simple MANIFEST header like
>      Eclipse-ExportExternalAnnotations: true

Assume a plug-in B the requires A. If plug-in A contains this new header in its MANIFEST, then plug-in B (like all consumers of A) will know that classes in A may be eea-annotated by files in the same plug-in. The nice thing is that B doesn't have to configure anything for seeing A classes properly annotated.

So "A" will be your typical model project, with generated classes that need eea-annotations.

There's one CAVEAT, though: if plug-in B want's to eea-annotate classes from any of its required plug-ins, then you will need https://github.com/eclipse-jdt/eclipse.jdt.core/issues/5 for annotation locations to be properly merged. I.e., for now B should not define an annotation path for the "Required Plugins" container.

 
> Is there an example project that demonstrates the new facilities?

Not yet, unfortunately. I'll package a small example soonish.