Bug 364323

Comment 1 Oliver Libutzki 2014-05-21 14:08:09 EDT

I second this. OSGi (Equinox) applications (e.g. Eclipse SamrtHome) which do not use the ui feature should be able to use the AbstractGuiceAwareExecutableExtensionFactory as well.

Comment 2 Stephan Seifermann 2020-02-07 05:06:42 EST

I would also like to see this proposal to be accepted.

In general, it would be nice to move stuff out of the UI bundle that is not related to UI. A good example is the resource factory registration, which should work without UI dependencies because it is just serialization/deserialization. Providing an ExecutableExtensionFactory without UI dependencies is one step towards this. Even providing a second factory besides the UI-related factory would be a big step forward.

Comment 3 Sebastian Zarnekow 2020-02-07 05:11:39 EST

Thank you for your input.

Unfortunately this is not easily possible for backwards compatibility reasons.

Fortunately it is possible for language implementors to implement their own executable extension factory and use that one.

Comment 4 Stephan Seifermann 2020-02-07 10:12:02 EST

Thanks for your fast reply.

I got your point and see why it might be difficult to change how the code generation works and libraries are structured without breaking existing projects. Especially, the backward compatibility of Xtext is one of various features I like.

To realize the UI independent factory, I just copied the code of AbstractGuiceAwareExecutableExtensionFactory and the generated Factory and removed the UI dependent modules. This works but introduces duplicated code, which I do not really like.

A solution to this could be moving and renaming the AbstractGuiceAwareExecutableExtensionFactory to org.eclipse.xtext (or another UI independend bundle). In place of the old class, a class with the same name but inheriting from the moved class could be created. This should not break existing code. In addition, an additional generation option could provide us with an additional extension factory in the runtime bundle.

Is this feasible?

Comment 5 Christian Dietrich 2020-03-06 05:07:29 EST

imho yes
@Stephan what do you think?

Comment 6 Stephan Seifermann 2020-03-06 07:14:33 EST

@Christian: I think you requested Sebastian's feedback rather than mine, right?

Comment 7 Christian Dietrich 2020-03-06 07:16:19 EST

yes. sry

Comment 8 Sebastian Zarnekow 2020-03-09 04:13:54 EDT

Just a brain-dump:

Constraint: 
We need exactly one injector per language in a running environment. Being it standalone, lsp or eclipse UI, having two injectors for a single language is prone to serious trouble.

Implications:
Moving the executable extension factory into the runtime bundle, would require the runtime bundle to access the UI module in eclipse to create a working single injector. Nevertheless, there is no way to introduce a dependency from the runtime bundle to the ui bundle. A way to solve this is via extension points. We'd need to introduce an extension point to register modules for a language. These modules would need to be configured in a specific order. So the extension point would need to be able to describe the ordering declaratively.
Extension points would allow to contribute arbitrary modules to arbitrary languages by default which is not what we want. This would need to be disallowed somehow.
Assuming we have that extension point: We'd need to have an activator in the runtime plugin that has a reference to the created injector to make sure it's only ever created once and the executable extension factory has a well-defined spot to find it.

These are all things that need be agreed upon, implemented and tested before we even start working on the code generator.

So making a step backwards:
Would you see any benefit in moving the AbstractGuiceAwareExecutableExtensionFactory to org.eclipse.xtext, let's say to org.eclipse.xtext.service.AbstractGuiceAwareExecutableExtensionFactory and let the existing class extend the extracted super class? Given that none of the other discussed changes in this ticket can be done (as far as I can tell) without implementing the approach outlined above.

Comment 9 Stephan Seifermann 2020-03-11 10:12:51 EDT

Having a base class to reuse for own factories would be beneficial. But I doubt that the base class can just be a copy of the existing factory because it has to be made ready for standalone usage. The existing factory does certainly not support this.

Until now, I have not thought about the implications of two injectors. The problematic point with two injectors are singletons that are no real singletons anymore when you have two injectors. To avoid this, the approach for ensuring one single injector should work.

However, such an approach is required anyway because it is also required if I implement a factory by myself. It would be even more beneficial to have this concept provided by the framework to avoid errors introduced by developers. In fact, I just copied and adjusted the factory code without considering the problems introduced by two injectors. I did not recognize errors but this does not mean a correct implementation.

I am not sure if the safe guard mechanisms to prevent contributions to the injector via extension points is really necessary. Developers can always break the generated code by writing new code that does not fulfill the assumptions of the framework. I rather see this as an improvement after the other measures have been realized. I think this would reduce the required effort for realizing the preparations you mentioned, right?