Support jakarta.annotation version 3.0 in E4 Injector

[HannesWell]

Fixes #1565

[HannesWell]

@merks is there a reason to not update to jakarta.annotation version 3 respectively to provide both to allow easy migration?

https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/e912d230f7dc8715d5616232236aa60c5bab7898/eclipse.platform.releng.prereqs.sdk/eclipse-sdk-prereqs.target#L805-L810

Of course an application ideally only uses one version and I don't know what happens if two versions are available respectivly if the E4 injector really works then.

[merks]

Supporting a wider range is good. I don’t see why both 2.x and 3.x need to be in the tp. Some other 3rd party dependency might need the 2.x. I’ll have to check with the aggregation analyzer.

[github-actions]

Test Results

 1 758 files  ±0   1 758 suites  ±0   1h 31m 55s ⏱️ - 8m 56s
 4 170 tests ±0   4 147 ✅ ±0   22 💤 ±0  0 ❌ ±0  1 🔥 ±0 
13 107 runs  ±0  12 940 ✅ ±0  164 💤 ±0  0 ❌ ±0  3 🔥 ±0 

For more details on these errors, see this check.

Results for commit 306174f. ± Comparison against base commit 1524ff1.

♻️ This comment has been updated with latest results.

[laeubi]

Of course an application ideally only uses one version and I don't know what happens if two versions are available respectivly if the E4 injector really works then.

Having multiple versions is supported by OSGi and the OSGi resolver should take care of it but the injector will only be able to handle "compatible" bundles then so all have to use 3.x or 2.x you can't mix them (in the current form). So if currently a "client" uses a narrow version range but the injector is bound to a higher range it wont work.

Eclipse Sisu semm to use a quite interesting aproach where they just look at the annotation class name and accept everything that ends with Inject e.g. com.google.Inject, javax.annotation.Inject and jakarta.annotation.Inject can be used together in any version.

[merks]

There are a lot of upper bounds that exclude 3.0.0 which is to be expected because goodness knows what 3.0.0 breaks, which is nothing for this use case, but one doesn't know that before there is a 3.0.0.

So I expect the downstream ecosystem will be in a similar state. Certainly better if only the package name and the package version specified for it via its exporting bundle mattered, regardless of the bundle symbolic name and bundle version.

[jukzi]

This does not fix DependencyInjectionViewTest for me locally. Instead it crashes with:

MESSAGE bundle org.eclipse.pde.ds.tck:1.0.0.qualifier (397) Component descriptor entry 'OSGI-INF/testServiceSingleton.xml' not found
WARNING: Annotation classes from the 'javax.inject' or 'javax.annotation' package found.
It is recommended to migrate to the corresponding replacements in the jakarta namespace.
The Eclipse E4 Platform will remove support for those javax-annotations in a future release.
To suppress this warning, set the VM property: -Declipse.e4.inject.javax.warning=false
To disable processing of 'javax' annotations entirely, set the VM property: -Declipse.e4.inject.javax.disabled=true


!ENTRY org.eclipse.e4.core.di 2 0 2024-09-24 09:19:11.740
!MESSAGE Possbible annotation mismatch: method "void org.eclipse.e4.ui.internal.workbench.ResourceHandler.init()" annotated with "jakarta.annotation-api:2.1.1:jakarta.annotation.PostConstruct" but was looking for "jakarta.annotation-api:3.0.0:jakarta.annotation.PostConstruct
 or jakarta.annotation-api:1.3.5:javax.annotation.PostConstruct"

!ENTRY org.eclipse.e4.ui.workbench 4 0 2024-09-24 09:19:11.744
!MESSAGE Unable to load resource platform:/plugin/org.eclipse.platform/LegacyIDE.e4xmi
!STACK 0
java.lang.NullPointerException: Cannot invoke "org.eclipse.emf.ecore.resource.ResourceSet.getResource(org.eclipse.emf.common.util.URI, boolean)" because "this.resourceSet" is null
	at org.eclipse.e4.ui.internal.workbench.ResourceHandler.getResource(ResourceHandler.java:287)
	at org.eclipse.e4.ui.internal.workbench.ResourceHandler.loadResource(ResourceHandler.java:263)
	at org.eclipse.e4.ui.internal.workbench.ResourceHandler.loadMostRecentModel(ResourceHandler.java:178)
	at org.eclipse.e4.ui.internal.workbench.swt.E4Application.loadApplicationModel(E4Application.java:368)
	at org.eclipse.e4.ui.internal.workbench.swt.E4Application.createE4Workbench(E4Application.java:244)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:568)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:546)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152)
	at org.eclipse.pde.internal.junit.runtime.NonUIThreadTestApplication.start(NonUIThreadTestApplication.java:58)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:208)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:143)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:109)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:439)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:271)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:668)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:605)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1481)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1454)

instead of having the Annotations as dependency for using java.lang.reflect.AnnotatedElement.isAnnotationPresent(Class<? extends Annotation>)
we could simply check the name like:
Arrays.stream(element.getDeclaredAnnotations()).anyMatch(a -> a.annotationType().getName().equals(annotationClass.getName()))
in org.eclipse.e4.core.internal.di.AnnotationLookup.AnnotationProxy.isPresent(AnnotatedElement) - which solves the test for me

[HannesWell]

This does not fix DependencyInjectionViewTest for me locally. Instead it crashes with:

Looks like what I have suspected in #1565 (comment).

instead of having the Annotations as dependency for using java.lang.reflect.AnnotatedElement.isAnnotationPresent(Class<? extends Annotation>)
we could simply check the name like:
Arrays.stream(element.getDeclaredAnnotations()).anyMatch(a -> a.annotationType().getName().equals(annotationClass.getName()))
in org.eclipse.e4.core.internal.di.AnnotationLookup.AnnotationProxy.isPresent(AnnotatedElement) - which solves the test for me

Yes, almost. We just have to use getAnnotations() instead of getDeclaredAnnotations(). The method AnnotatedElement.isAnnotationPresent(Class<? extends Annotation>) also operates on the former.

I just pushed an update.

Eclipse Sisu semm to use a quite interesting aproach where they just look at the annotation class name and accept everything that ends with Inject e.g. com.google.Inject, javax.annotation.Inject and jakarta.annotation.Inject can be used together in any version.

I have to say that I wouldn't be comfortable with that since then even more false positives are possible. This might be only hypothetical, but on the other hand it wouldn't be too difficult to support more annotations if really desired.

There are a lot of upper bounds that exclude 3.0.0 which is to be expected because goodness knows what 3.0.0 breaks, which is nothing for this use case, but one doesn't know that before there is a 3.0.0.

Thanks for the analysis. That's what I have expected and I'm actually happy that proper version-ranges are used, even if this makes migration in this case a bit harder than it would have been without.

[laeubi]

I have to say that I wouldn't be comfortable with that since then even more false positives are possible. This might be only hypothetical, but on the other hand it wouldn't be too difficult to support more annotations if really desired.

What means "even more"? Currently we have not nay false positive at all (only false negatives).

[jukzi]

Do we need any import at all? Should be possible to use class names only as String.

[HannesWell]

Initially I wanted to keep this change minimal, but if this is started it should be done completely.
So in theory yes, but while implementing it I noticed that obtaining the values of jakarta/javax.inject.Named is not trivial because I think we want a more performant solution than using reflection all the time.
I think I'll try it with cached MethodHandles or alike.

[laeubi]

I think we want a more performant solution than using reflection all the time

Have you measured the performance impact? Injection frameworks always require reflection and the JVM should be quite good at that so I'm curious if it is really a performance impact (in practical application).

[merks]

My suspicion would be that in the overall scheme of things the performance here is not relevant (especially base I've debugged all the injection stuff that happens a framework like Xtext that's using guice).

[eclipse-platform-bot]

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

runtime/bundles/org.eclipse.e4.core.di/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From 4b8c95c8d50d16b0767d4e6ced4972aa35d860cb Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <platform-bot@eclipse.org>
Date: Fri, 4 Oct 2024 21:59:56 +0000
Subject: [PATCH] Version bump(s) for 4.34 stream


diff --git a/runtime/bundles/org.eclipse.e4.core.di/META-INF/MANIFEST.MF b/runtime/bundles/org.eclipse.e4.core.di/META-INF/MANIFEST.MF
index a084435dfd..29b2880e42 100644
--- a/runtime/bundles/org.eclipse.e4.core.di/META-INF/MANIFEST.MF
+++ b/runtime/bundles/org.eclipse.e4.core.di/META-INF/MANIFEST.MF
@@ -1,7 +1,7 @@
 Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-SymbolicName: org.eclipse.e4.core.di
-Bundle-Version: 1.9.500.qualifier
+Bundle-Version: 1.9.600.qualifier
 Bundle-Name: %pluginName
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
-- 
2.46.1

Further information are available in Common Build Issues - Missing version increments.

stale

[jukzi]

Has to be public to make org.eclipse.e4.core.internal.services.MessageFactoryServiceImpl.processPostConstruct(Object, Class<?>) compile.

• i would like to see this PR merged soon. Even if it may not be the final solution it still helps to solve the current situation

[HannesWell]

Yes.

Even if it may not be the final solution it still helps to solve the current situation

The problem with the first approach was that it only worked in multiple versions for some kind of annotations and not for all kind of annotations processed by the injector. And while partial progress is certainly better than no progress inconsistent behavior can be really bad.

But I think I have found a good approach now that works with all kind of annotation and I hope to complete it within the next days.

[jukzi]

remove TODO

[jukzi]

why is it needed to check if javax is loadable?

[HannesWell]

This methods checks if the javax-classes are even available in the application in any version and if it's therefore worth to check for them. And, maybe even more important, it emits the warning about javax annotations being 'deprecated'.

But in the past it was asked to improve the implementation because the message is to coarse. So this can definitively improved. OTOH, if support for javax annotation does not require their classes in the build-system we can probably support it much easier and longer and it could be consider to un-deprecate their support.

[merks]

If it can be supported by reflection for very little ongoing maintenance overhead, that seems like a good thing for the consumers.

[HannesWell]

The most work would be testing it. Currently we have the test-suite duplicated once for javax and once for jakarta.
But if we just use the class-names permanently to support e.g. multiple versions of a class eventually, the injector would even be simpler without all the logic for the deprecation warnings etc.

The main question is probably for how long we want to explicitly test javax annotations.
At the same time it might not even be necessary to explicitly test with all supported class names, as long as we make sure that the injector just cares about class-names and can handle multiple Class-objects with the same name.