eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

Migrate CS2AS design to a Guice-based one #1217

Open eclipse-ocl-bot opened 1 month ago

eclipse-ocl-bot commented 1 month ago

| --- | --- | | Bugzilla Link | 419462 | | Status | NEW | | Importance | P3 normal | | Reported | Oct 15, 2013 10:16 EDT | | Modified | Oct 16, 2013 07:26 EDT | | Reporter | Adolfo Sanchez-Barbudo Herrera |

Description

Currently OCL CS2AS framework is heavily based on the factory method pattern, in order to provide a way of customization to derived languages. Although the pattern is useful/enough, sometime it's tedious when we want to create an specific customization for a class, which transitively is created by other factory methods, so that, in the end, if we want to customize one class, we have to create, at best, two new classes.

Due to the fact that CS2AS framework relies on Xtext-generated framework which currently relies on Guice, it would be ideal if we replace and improve the current CS2AS framework design to also exploit Guice benefits. In this way, if an specific class needs to be customized/replaced with a new one, just an specific binding in the corresponding language RuntimeModule needs to be adjusted.

Other benefits:

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 15, 2013 11:03

branch asbh/419462 contains initial design where:

There is still work to do:\ a) Port/accommodate current CS2AS design to use a Guice based framework.\ b) In order to easily/quickly accommodate current CS2AS design, we need to use assisted injection[1], which requires including a new jar into Orbit. Implementing current object construction should be easily done using the current XXXXXCS2ASFactory class, which would be injected wherever a new object needs to be constructed/instantiated.\ c) StandaloneSetup and RuntimeModule look a little bit messy across different OCL Xtext implementations. I'll work around it to improve this area as well.\ b) Some performance checks ?

In principle, I'd firstly go for a) to see if I find any other problem regarding Guic-ifying current CS2AS framework

Any feedback about any design issue or room for improvement you may oversee, please let me know. The only arguably flaw I could highlight, is having just one factory interface for the whole CS2AS framework. I guess that modularizing into several meaningful smaller factories, could be an improvement. I'll think about it to see if it may be improved in this way or any other one.

[1] https://code.google.com/p/google-guice/wiki/AssistedInject

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 15, 2013 12:54

Guice with generics, gets even more complicated[1]

[1] http://stackoverflow.com/questions/5514115/problem-with-generic-return-type-in-guice-assisted-inject-factory

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 15, 2013 14:06

This is looking increasingly risky. It certainly is not easy.

If you migrate now you will have to make Guice/non-Guice code co-exist for new/old CS2AS code. This will be even harder. Development of this change is likely to conflict with concurrent progress giving GIT merge nightmares.

Much better to do it later once the old code has gone. Then a change to the auto-generator and a regenerate and we're done. This change might go in as quick although large GIT change.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 16, 2013 05:20

(In reply to Ed Willink from comment #3)

This is looking increasingly risky. It certainly is not easy.

Agree.

Working with generics looks like a pain with guice. Providing that Guice also needs information at runtime about the generics, it seems that we will have some performance impact.

Likewise, the AbstractGenericModule would require to be enhanced with a derived class capable of dealing with assitedInject factories (a nice enhancement, but more work to do). In essence, and similarly what AbstractGenericModule provides to do the bindings:

install(new FactoryModuleBuilder() .implement(Interface1.class, Class1.class) .implement(Interface2.class, Class2.class) ... .build(MyFactory.class))

Needs to be reflectively done so that every "implement(...)" can be declaratively specified with some java method. For instance:

Class<? extends Interface1> factoryImplementInterface1(Class forFactory) {\ return Class1.class;\ }

Allowing us to do derived language customizations via overriding the specific method which does an specific "implement" for the assitedInject factory. Otherwise, to just change one class, we would have to rewrite the whole factory installation:

install(new FactoryModuleBuilder() .implement(Interface1.class, MyCustomClass1.class) .implement(Interface2.class, Class2.class) ... .build(MyFactory.class))

Another pain: if the same factory to do assitedInjection create the same type, annotations need to be used:

interface ICS2ASFactory { ...\ @NonNull @Named("PreOrderVisitor") BaseCSVisitor<Continuation<?>> createPreOrderVisitor(CS2PivotConversion conversion);\ @NonNull @Named("PostOrderVisitor") BaseCSVisitor<Continuation<?>> createPostOrderVisitor(CS2PivotConversion conversion);\ }

The other option to workaround these two issues is having one factory per class to be constructed. Untenable.

The benefit of Guice of customization for derived classes, accidentally creates a lot of complexity in the design.

If you migrate now you will have to make Guice/non-Guice code co-exist for new/old CS2AS code. This will be even harder. Development of this change is likely to conflict with concurrent progress giving GIT merge nightmares.

Much better to do it later once the old code has gone. Then a change to the auto-generator and a regenerate and we're done. This change might go in as quick although large GIT change.

The idea was, trying to Guicify current (old) CS2AS OCL framework, so that for QVTo I can use the (new) autogenerated classes via Guice injection. Don't forget that currently, for QVTo editor and test cases I need to mix new autogenerated CS2AS classes for QVTo with old CS2AS framework classes for OCL.

I think that Guici-fying the old framework now is good and useful. However, given the increasing pain I'm experiencing, specially with generics, without knowing the performance impact, I'm not sure the value of doing that. Providing you are not convinced about doing this, I'm fine if you prefer to put this off.

Thoughts ?

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 16, 2013 06:05

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #4)

Thoughts ?

Simplistically there are two jobs to do

A) The real work (the editors, CS2AS) \ B) The support work (Guice)

If doing the support work first aids the real work, then it can be beneficial to side track from the true goal.

This now looks very doubtful. Potentially a massive amount of sideways progress with many new problems and largely aesthetic benefits.

Let's get on with the real work.

Doing A then B now looks much faster to reach the real goal and possibly faster to the dual goal.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 16, 2013 06:14

Ok.

I've committed and pushed the branch my current (incomplete experimentation).

Move to archives ?

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 16, 2013 07:26

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #6)

Move to archives ?

Yes