eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

[releng] OCL models cannot be regenmodeled under 3.6M4 #487

Closed eclipse-ocl-bot closed 1 month ago

eclipse-ocl-bot commented 1 month ago

| --- | --- | | Bugzilla Link | 298201 | | Status | CLOSED FIXED | | Importance | P3 normal | | Reported | Dec 18, 2009 12:05 EDT | | Modified | May 22, 2013 14:16 EDT | | Blocks | 290103 | | Reporter | Ed Willink |

Description

Models are genmodeled with custom dynamic templates.

These templates have not been updated to track UML2 and so they have an include path failure for CodeGenUtil.

Generating the CST model with standard templates results in a number of missing @since, @noreference, @noimplement of which only the former are errors for us.

These attributes could all be embedded within the

 * <!-- begin-user-doc -->
 * <!-- end-user-doc -->

comment region and so be preserved automatically.

Do we need custom templates just for this nicety?

Should the minor improvement be contributed back so that we don't get broken again?

Detailed investigation of the custom template deviations required.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Dec 18, 2009 12:32

Ed, I think that the bug Bug 290103 should track this an similar issues.

I believe that the current used templates (org.eclipse.ocl/templates) should be modified to accommodate any change in EMF and/or UML one.

Regards,\ Adolfo.

This bug has been marked as a duplicate of bug 290103

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 18, 2009 17:11

The existing templates indeed do no more than provide @noimplement and\ @noreference comments automatically.

Unfortunately they do this for o.e.ocl and o.e.ocl.uml but not o.e.ocl.ecore.\ This is inconsistent and wrong.

In the case of int values, we want to exclude ints from the API because they\ change with Class addition/re-ordering, so o.e.ocl.ecore should be @noreference\ too.

In the case of interfaces, we may wish to exclude o.e.ocl from the API with\ @noimplement, but there is no reason why o.e.ocl.uml should be @noimplement,\ particularly when o.e.ocl.ecore is not.

So the templates didn't even work as intended.

At best the templates are VERY slow (ten times slower) compared to the standard\ ones. Perhaps they have a leakage bug in them. At worst they don't work now\ anyway. Fixing the changed include path sometimes helps. I don't have time to\ debug this VERY slow and 'sometimes'.

Philosophically good templates can be an asset, but bad templates in a vintage\ language that have consistently failed to track the changes in UML and EMF\ templates means that we are getting much more pain than benefit.

The simple expedient of placing @noreference and/or @noimplement within the\ user documentation section means we do not need them at all and so don't need\ their bugs and don't need to maintain them. We have to use this technique for\ @since anyway.

I'll submit a patch to delete the templates and migrate the annotations.

If someone has time to work on the templates, the standard templates could be\ enhanced to at least @noreference the int values. @noimplement perhaps could be\ a genmodel option.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 19, 2009 09:50

I am totally unable to prepare a patch without getting clobbered by timeouts (Bug 293355). With a couple of hundred trivial annotation migrations, preparing a patch piecemeal and metging is also not practical.

Can we therefore review the principle?

a) Delete the custom templates and so revert to the standard ones\ b) Migrate the @nereference and @noimplement lines inside the user doicumentation section, so that regeneration preserves them\ c) Making the Ecore-binding int properties @noreference like the UML-binding\ d) Removing @noimplement from the UML-binding interfaces to match the Ecore-binding.

The above involve no code changes at all. The change in annotations appears to have no impact on QVTd or QVTo.

After migration of the annotations, the only problem with regeneration is the usual spurious import of xxx.* from xxx.util.XxxSwitch.java.

eclipse-ocl-bot commented 1 month ago

By Laurent Goubet on Dec 21, 2009 03:46

As far as I'm concerned, I am all for reverting to the standard JET template, especially so if the custom templates only provide annotations.

a) +1\ b) Doesn't this require changes in the models? Adding "documentation" annotation for the API tags doesn't bother me though. +1\ c) These are dangerous as API (any client referencing them will eventually see their code fail without real warning), +1\ d) +1

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 21, 2009 05:08

Thanks for the +1's Laurent. I'm not sure Afolfo is happy, so I'll wait a couple of days to give him (and Alex) a chance to comment.

b) Migrate the @nereference and @noimplement lines inside the user

doicumentation section, so that regeneration preserves them

b) Doesn't this require changes in the models? Adding "documentation"

annotation for the API tags doesn't bother me though. +1

A change within the comments only. Every interface file gets a 2 line comment migration. The Package interface also gets a comment migration/revision on every int returning feature method. Editing with regular expression replace matches is a great help here.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Dec 21, 2009 05:51

(In reply to comment #5)

Thanks for the +1's Laurent. I'm not sure Afolfo is happy, so I'll wait a couple of days to give him (and Alex) a chance to comment.

b) Migrate the @nereference and @noimplement lines inside the user doicumentation section, so that regeneration preserves them

b) Doesn't this require changes in the models? Adding "documentation" annotation for the API tags doesn't bother me though. +1

A change within the comments only. Every interface file gets a 2 line comment migration. The Package interface also gets a comment migration/revision on every int returning feature method. Editing with regular expression replace matches is a great help here.

Hi all,

I'm afraid I'm not sure about which is the best solution.

Using templates is good, since a decision taken when generating, will always be preserved every time we generate. I haven't ever annoyed when generating OCL or OCLCST code, I don't really care if the generation takes 10 secs isntead of 1, or a minute instead of six secs....as said, I haven't been worried about that. What I don't really like about using templates, is that we always have to be stearing at any change in EMF/UML.

So, I'm not unhappy with the proposed change, if just API tooling related are acomplished by the templates, and you preserve them in documentation section.

I also agree with the annotations-related conclusions.

Regards,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 21, 2009 06:00

Created attachment 154868 Possibly complete patch

Timeouts seem a bit better today.

Attached might demonstrate the changes.

:notepad_spiral: Bug298201c.patch

eclipse-ocl-bot commented 1 month ago

By Alexander Igdalov on Dec 21, 2009 07:22

Hi All,

(In reply to comment #3)

Can we therefore review the principle?

a) Delete the custom templates and so revert to the standard ones b) Migrate the @nereference and @noimplement lines inside the user doicumentation section, so that regeneration preserves them c) Making the Ecore-binding int properties @noreference like the UML-binding d) Removing @noimplement from the UML-binding interfaces to match the Ecore-binding.

My +1 to a, b, c, and d.

As regards the patch, I would omit @noextend and @noimplement annotations in CST interfaces. QVTO, for instance, makes use of some the OCL interfaces such as CSTNode and OCLExpressionCS. Since OCL grammar is extensible, why should not the CST interfaces be extensible too?\ In general, any CST interface might be extended (though I can hardly imagine a three-state boolean or heirs of InvalidLiteralExpCS). Thus, I would also get rid of @noextend and @noimplement in the CST model.

Regards,

eclipse-ocl-bot commented 1 month ago

By Alexander Igdalov on Dec 21, 2009 07:40

(In reply to comment #6)

Adolfo,

I am not sure that having to align the templates for the sake of API tooling only is reasonable. In this case I vote for the workaround.

However, we can consider raising a bugzilla against EMF and creating a corresponding patch for the EMF templates. In this patch we could provide JET extensions for optional .javajetinc files. If these files are included then the corresponding customizations take place (API tooling annotation insertions in our case).

If such patch is approved in EMF we could re-introduce the templates in OCL.

Cheers,

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Dec 21, 2009 09:18

(In reply to comment #9)

(In reply to comment #6)

Adolfo,

I am not sure that having to align the templates for the sake of API tooling only is reasonable. In this case I vote for the workaround.

Alex, let me disagree (partly). API tooling has been adopted as convention tool to facilitate the work and avoid bad practices to MDT-OCL developers and MDT-OCL clients. That said, if we want to follow the API tooling conventions, and take advantages from its facilities for our clients, we have two choices:

  1. Using some templates, so that when generating the code we can include some API tooling wonders for free and, preserving them along the time, and saving us of having to be worried about them when regenerating after any metamodel change.
  2. Doing that work by hand in every already generated class.

So, I would definitely do the point 1, regardless I had to align templates or I hadn't. Now, we have a third choice:

  1. Let's use some regexp facilities, so that all API tooling annotations, are included in the documentation section, so that the templates are not needed anymore.

This decision has advantages and disadvantages:\

That's the reason why I'm not completely pleased with the proposed change. Undoubtedly, if we don't need to worry about changes of the EMF/UML templates , the proposal has some added value (Although the proposal goes against some MDE and generation principles).

However, we can consider raising a bugzilla against EMF and creating a corresponding patch for the EMF templates. In this patch we could provide JET extensions for optional .javajetinc files. If these files are included then the corresponding customizations take place (API tooling annotation insertions in our case).

If such patch is approved in EMF we could re-introduce the templates in OCL.

I'm not sure what you refer. Are you suggesting to have a parameter in the genmodel, so that when it's enabled it automatically applies the changes (include the annotations) in the same fashion MDT-OCL is doing with the current templates ?. This sounds good, but we must convince Ed (Merks), that this a general purpose change, which seems to be sensible, in my opinion. Actually, Ecore itself shouldn't be extended, as Ed Merks usually says, so this annotations would be helpful for Ecore itself.

The idea of having a metamodel conceived to be extended, and not extended, and hence, annotated so that API tooling complains when extending. We could refine the idea, but it sounds good. I also guess the we would need a patch. I'm not a JET expert, and honestly I don't think this is prioritary. I think that we could at least throw an enhancement request.

What do you think ?

Regards,\ Adolfo.

Cheers,

  • Alex.
eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 21, 2009 10:06

Adolfo. I agree that it is error prone to have to add the annotations manually to new classes. Furtunately this is not a common occurrence. Unfortunately the existing templates had a number of errors that we are now correcting (manually).

Alex. I agree with removing the constraints on CST altogether. I think Christian added these to provide some API change flexibility when the 'internal' was removed from the package to support re-use by QVTd.

I am now unconvinced that these annotations are correct at all. I now think they should all be removed.

@noreference on int features is not really the correct annotation. The int values are provided by EMF to provide efficient run-time behaviours and provided these values are not persisted, there is no reason why clients should not use them, so while @noreference avoids maintaining API filters when values change, it imposes an over-strict constraint on clients. We need an @nopersist annotation to accurately document the API. This is perhaps why Ed Merks will very reasonably decline to update EMF to supply an @noreference on int features. Perhaps we should put a 'This value is provided to facilitate efficient client usage. Its value may change when Eclipse is restarted, therefore a URI should be used to persist this value.' warning instead.

@noimplement on the abstract-binding is also not accurate. Why should a client encounter numerous warnings if they attempt to provide, for instance, a CMOF-binding? Ecore and UML bindings should not be privileged.

The latter problem may vanish if my plans for a generic OCLruntime binding mature to support run-time reflection via OCLClassifier, OCLOperation etc. See bug 283052.

Therefore compared to:

a) Delete the custom templates and so revert to the standard ones\ b) Migrate the @nereference and @noimplement lines inside the user\ doicumentation section, so that regeneration preserves them\ c) Making the Ecore-binding int properties @noreference like the UML-binding\ d) Removing @noimplement from the UML-binding interfaces to match the\ Ecore-binding.

I vote

a) +1\ b) -1\ c) -1\ d) +1

I now propose:

a) Delete the custom templates and so revert to the standard ones\ e) Removing @noimplement and @noreference from all CST, OCL, OCL.Ecore, OCL.UML interfaces. (just requires a regeneration).\ f) Add a 'This value is provided to facilitate efficient client usage. The value may change when Eclipse is restarted, therefore a URI should be used to persist this value.' comment on all integer features.

We will need API filters whenever integer values change.

eclipse-ocl-bot commented 1 month ago

By Alexander Igdalov on Dec 21, 2009 10:11

(In reply to comment #10)

(In reply to comment #9)

(In reply to comment #6)

Adolfo,

I am not sure that having to align the templates for the sake of API tooling only is reasonable. In this case I vote for the workaround.

Alex, let me disagree (partly). API tooling has been adopted as convention tool to facilitate the work and avoid bad practices to MDT-OCL developers and MDT-OCL clients. That said, if we want to follow the API tooling conventions, and take advantages from its facilities for our clients, we have two choices:

  1. Using some templates, so that when generating the code we can include some API tooling wonders for free and, preserving them along the time, and saving us of having to be worried about them when regenerating after any metamodel change.
  2. Doing that work by hand in every already generated class.

EMF Merge ensures that the user documentation is preserved. IOW, if we regenerate the model code over the exisiting sources the annotations do not vanish. I agree that it would be nice to have a chance to delete all generated files and make a "clean" regeneration. However, we do have custom code already (see @generated NOT tags in OCL AST), thus, such a clean regeneration won't work there for other reasons.

However, we can consider raising a bugzilla against EMF and creating a corresponding patch for the EMF templates. In this patch we could provide JET extensions for optional .javajetinc files. If these files are included then the corresponding customizations take place (API tooling annotation insertions in our case).

If such patch is approved in EMF we could re-introduce the templates in OCL.

I'm not sure what you refer. Are you suggesting to have a parameter in the genmodel, so that when it's enabled it automatically applies the changes (include the annotations) in the same fashion MDT-OCL is doing with the current templates ?.

I was thinking of a single-line modification to EMF JET templates. Something like:

<%@ include file="Class/someThoughtfulName.javajetinc" fail="silent" %>

This instruction tells the JET compiler to look for a custom template file called someThoughtfulName.javajetinc and if it exists include its content into the invoking JET template. Such a modification will not change the previous EMF behaviour - it will just provide yet another extensibility point (you can see there are many .javajetinc referred to in the templates). Thus, if we put someThoughtfulName.javajetinc into the Class directory of the templates folder we could add our custom functionality without having to keep a slightly modified copy of Class.javajet and others.

I'm not a JET expert, and honestly I don't think this is prioritary. I think that we could at least throw an enhancement request.

What do you think ?

I agree it's not a P1=)) Moreover, before we raise this request we should find out whether we actually need these annotations.

In addition to my comment #8:\ I have noticed that not only CST but AST interfaces are also generated with @noextend and @noimplement tags. QVTO extends AST interfaces too. So these tags should be removed as well.

Best,

eclipse-ocl-bot commented 1 month ago

By Alexander Igdalov on Dec 21, 2009 11:10

(In reply to comment #11)

Hi Ed,

Seems we are going in the same direction (I have had a mid-air collision with your comment :).

I have implicitly agreed with the most parts of your new proposal in my previous comment. I just don't understand

f) Add a 'This value is provided to facilitate efficient client usage. The value may change when Eclipse is restarted, therefore a URI should be used to persist this value.' comment on all integer features.

As I see it, these values are hardcoded. Thus, they shouldn't change when Eclipse is restarted. They can change iff an OCL model is modified and regenerated.

I agree with you that these constants could be made available to clients since they also provide reflective access to OCL models. Thus, my +1 to the new proposal with a small refinement to (f):

f) Add a 'This value may change when the model code is regenerated. It is subject to change without notice.' comment on all integer features.

Cheers,

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 21, 2009 11:32

Regenerating will lose the annotation that we want to lose; those that the templates used to preserve. All @generated NOT's get standard correct treatment.

I have no problem with someone (not me) suggesting an extension flexibility to the EMF templates, which we might consider using.

'This value may change when the model code is regenerated. It is

subject to change without notice.'

Your comment is shorter and more accurate. I was deliberately over-stating the hazard to allow a future version of a model implementation that allocated the integers dynamically at load-time following a package merge.

The 'It is subject to change without notice' covers dynamic allocation too.

eclipse-ocl-bot commented 1 month ago

By Alexander Igdalov on Dec 21, 2009 14:08

(In reply to comment #14)

Ed,

I have no problem with someone (not me) suggesting an extension flexibility to the EMF templates, which we might consider using.

I raised bug 298337 for the user-doc comment for int-fields. AFAIU, we do not need any other annotations yet.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 22, 2009 14:04

Committed to CVS HEAD.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 22, 2009 15:04

Verified in build #79.

[It turned out that the illegally implements were nothing to do with Xx extending Xx, but Ecore and UML extending @noimplement. So these filters all went. Ecore has no filters at all now.]

eclipse-ocl-bot commented 1 month ago

By Ed Willink on May 27, 2011 02:48

Closing after over 18 months in resolved state.