Bugzilla – Full Text Bug Listing
| Summary: | Investigate support for dynamic EOperation call API | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Christian Damus <give.a.damus> | ||||||||||||||||||
| Component: | Core | Assignee: | Kenn Hussey <Kenn.Hussey> | ||||||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||
| Priority: | P3 | CC: | achim.demelt, agfitzp, ahunter.eclipse, alexander.igdalov, bruck.james, clay, davidms, dvorak.radek, Ed.Merks, ed, i.am.joel, Kenn.Hussey, knut.wannheden, kutter, ldamus, sebastian.zarnekow | ||||||||||||||||||
| Version: | 2.4.0 | Keywords: | plan | ||||||||||||||||||
| Target Milestone: | M4 | Flags: | Kenn.Hussey:
helios+
|
||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||
| Bug Blocks: | 286329, 291365 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Christian Damus
Created attachment 118008 [details] Prototype of dynamic operations with eCall Attached a patch that prototypes an EObject::eCall API with support for EOperations in dynamic models. It extends the Ecore portion of Ed's patch for bug 216701 (derived feature delegates) with fixes for a couple of issues that I reported on that bug, and to demonstrate the similar approach taken for operations. Included in the patch are: - update to Ecore.ecore, adding the eCall operation to EObject and an EInvocationTargetException datatype that it needs - *.ecorediag updates to match - Ecore.genmodel is not updated because I don't know how to update the Rose model (I thought Ecore was now its own master?) - regeneration of Ecore package - addition of EOperation.Internal interface to define the CallDelegate API - implementation of BasicEObjectImpl::eCall to provide the hook into the EOperation.Internal's call-delegate - BasicCallDelegate to implement fall-back behaviour applicable to all EObjects - example implementation of a CallDelegate using OCL (which includes and refactors the protoype for property delegates from bug 216701) - updates to the org.eclipse.ocl.ecore.tests to test the call delegate - added a reportsTo(Employee) operation into the dynamic Ecore model - reworked existing attributes in the model to use the reportsTo operation in their derivation expressions - added JUnit test case What is not included, most notably, is any support for eCall delegation to statically-generated operations. I would imagine that a simple solution to that would look much like the code in the BasicCallDelegate, which is very like generated eGet/eSet methods. Also important to eCall in a static context is the statically generated EOperation IDs and literals. As suggested by the comments in BasicCallDelegate, perhaps a good approach to avoiding name clashes with structural features (which could be common) is to use three underscores to separate EClass name from EOperation and to include parameter type names to distinguish overloaded signatures. Obviously, without support for statically-generated models, this is just a start. And, I didn't even mention compatibility. One would expect eCall to work on models generated with EMF 2.2 ... Any feed-back will be most welcome, and probably necessary before I attempt anything further along this line. Created attachment 118009 [details]
The real eCall prototype patch
That was cool. The last attempt to post this patch resulted in a teensy 7-byte attachment. The hazard of using Mylyn's clipboard patches, I guess.
Created attachment 148791 [details] updated patch, based on HEAD Here's an updated version of the patch, based on HEAD. Patches for OCL will be tracked under bug 291365. Created attachment 149767 [details]
streamlined patch
This patch contains just the contributions from Christian Damus (Zeligsoft) that pertain to operation invocation delegates, for the purposes of IP due diligence. The other changes (related to setting delegates and OCL implementations) will be tracked under their respective bugs (namely 216701, 291361, and 291365).
Created attachment 150408 [details] complete patch Here's a first cut at a complete patch, including changes for bug 216701 (since I'm working on both at the same time). Created attachment 150693 [details]
updated patch
Here's an updated version of the patch which takes overridden operations into account. In this version, reflective invocation of (overridden) operations is handled symmetrically for dynamic and static models.
Created attachment 151088 [details]
updated patch
This version ensures that the most derived behavior of an operation is always invoked (even if/when a base operation is passed as an argument to the reflective API), for both dynamic and generated models.
Support for obtaining the base operation identifier has been removed because it's just unnecessary bloat (it isn't being used anywhere).
This looks really great. So many tricky details. I didn't get to spend as long looking at it as I would have like, but I did notice a few little things: Should EInvocationException really be serializable? Since it seems it's only been added to be used as an exception thrown by EObject.eInvoke(), I don't think see a reason. This is super nit-picky, but I notice that all the non-serializable data types are brown in EcoreDataTypes.ecorediag (but some serializable ones are too, so I'm not actually sure about the colour distinction). EStructuralFeature.SettingDelegate.Factory is marked @since 2.5, not 2.6. There seems to be an unnecessary Object cast being generated for Object-typed parameters in eInvoke(). For example, see the case for isInstance() in EClassifierImpl.eInvoke(). I believe we similarly suppress an Object cast in eSet(), for instance. I'm also seeing unnecessary EOperation casts being generated the operation getter methods in package implementations. For example, see EcorePackageImpl.getEClass__IsSuperTypeOf_EClass() (and all the rest). The cast was needed for structural features, but since there are no EOperation subclasses, I can't see any reason to ever do it here. The Javadoc for BasicInvocationDelegate refers to dynamicCall() instead of dynamicInvoke(). Finally, a question: should EObjectImpl have a generated-style eInvoke() override? Wouldn't that be a little nicer than falling back to the non-generated BasicEObjectImpl version? This gets me thinking: these changes add a pretty important distinction between classes that are modeled explicitly as extending EObject and those that aren't. Classes that don't extend EObject won't "inherit" the EObject operation IDs, so those opereations won't be reflectively invokable. Does that sound right? It seems reasonable to me, but it's worth noting if true. If Ed is happy with these changes, I'm happy with them going in early in M4. (In reply to comment #8) Thanks for the careful review, Dave! I'll be sure to make the suggested changes before committing the code, likely on Monday. I'll discuss your question with Ed and make additional changes if necessary. The (updated) changes have been committed. Why has the inheritance of EModelElement and EGenericType from EObject been removed? This would be a major API change, that surely requires a migration to EMF 3.0.0. The change specifically breaks one MDT/OCL JUnit test that attempts to use eContents() in an OCL expression. Generally this change will cause major trouble to anyone using Ecore.ecore rather than the manually generated Java model. (In reply to comment #11) > Why has the inheritance of EModelElement and EGenericType from EObject been > removed? This would be a major API change, that surely requires a migration to > EMF 3.0.0. > > The change specifically breaks one MDT/OCL JUnit test that attempts to use > eContents() in an OCL expression. Generally this change will cause major > trouble to anyone using Ecore.ecore rather than the manually generated Java > model. This is not an API change - the Java class/interface has not be affected. This was a change at the metadata level to minimize the number of reflective constants that were generated for metamodels rooted by EModelElement. There's no point arguing about whether the change is in the P or the I of API. There clearly is a change to Ecore.ecore that affects meta-model users. Surely the genModel tooling should be (further) upgraded to behave as required and so avoid the meta-model change? After all there is very little about EObject that is accurately generated now that it has at least two different levels and variants. These redundant literals perhaps could be manually not-generated. Yes, it's a change and it affects a small number of users. One additional change was needed in EMF's generator in a piece of code that was hard coded to deal with the problem caused by EModelElement's explicit EObject inheritance. I don't understand this comment "After all there is very little about EObject that is accurately generated now that it has at least two different levels and variants." The generator is working well... It seems best to change your test case and to ensure that reflect code assumes implicit extension of EObject even when it's not explicit. It's simply of no value to be able to eInvoke EObject's EOperations; one can always cast and call those methods.... Changing the MDT/OCL JUnit test is easy, but the need to change it points out a major problem for OCL users. Any MDT/OCL user who has followed regular newsgroup advice on how to make EObject the base of their meta-model hierarchy and so facilitate use of eContainer() within an OCL expression, will find that their OCL expressions will fail to parse in Helios, since the eContainer() method is no longer inherited in their meta-model. This is liable to break a large proportion of existing advanced OCL usage. Since OCL 'casting' should be model-driven, I doubt that OCL code can be rewritten to cast to a type that the meta-model claims to be incompatible. Ecore.ecore is the meta-model for Ecore. If it is inappropriate for code generation, then perhaps the Ecore.ecore puiblished as the meta-model should be slightly different to the Ecore.ecore used by Ecore.genmodel. (In reply to comment #15) > Any MDT/OCL user who has followed regular newsgroup advice on how to make > EObject the base of their meta-model hierarchy and so facilitate use of > eContainer() within an OCL expression, will find that their OCL expressions > will fail to parse in Helios, since the eContainer() method is no longer > inherited in their meta-model. This is liable to break a large proportion of > existing advanced OCL usage. Since OCL 'casting' should be model-driven, I > doubt that OCL code can be rewritten to cast to a type that the meta-model > claims to be incompatible. I'm not sure I follow... EObject wasn't changed here, only EModelElement and EGenericType (in the Ecore metamodel) were. So, I fail to see how model hierarchies rooted by EObject (versus types in the Ecore metamodel itself, which is explicitly discouraged) could be affected... On further investigation this gets more complicated. a) The usage of eContents() within an OCL expression is an MDT/OCL extension that perhaps deserves to be broken. Oddly since UML2 is Ecore at heart, my assumption that container() would work with the UML2-binding was erroneous. Both UML2 and Ecore bindings require the non-standard eContainer(). See Bug 283052 for further discussion. b) OCL is not properly model-driven in its eContents() lookup. It is the Java model not Ecore.ecore that is used and which has changed. Previously getEAllOperations() for an EPackage returned 16 operations including eContents(). Now getEAllOperations() for an EPackage returns just 2; getEAnnotation() and getEClassifier(). Surely this is a significant change that was not intended? Personally I think the advice that was given was less than ideal. All instances support EObject implicitly so it would seem better (in my opinion) to support all the EObject API regardless of explicit inheritance. Of course folks who follow the advice for their own model will find it keeps working. It was intentional that none of the Ecore classes explicitly include EObject's operations to avoid the need to generated eInvoke cases and associated constants for all generated classes... Given the API breaking changes in OCL 3.0 itself, now would be a good time to change it so that EObject's operations are always supported. We have been hit with the change as well. so anyone extending EModelElement must regenerate when adopting EMF 2.6. This breaks 2.5 compatibility. Can you confirm? (In reply to comment #19) > so anyone extending EModelElement must regenerate when adopting EMF 2.6. This > breaks 2.5 compatibility. Can you confirm? Yes, models extending EModelElement (which is explicitly discouraged) must regenerate so that operation indexes get properly adjusted. This happens any time a feature or operation is added to a base (extended) model, unless your model is using feature/operation offsets. Note that models need not necessarily enable operation reflection when regenerating - that is optional. Kenn, But previous to this, there were no generated operation indexes. So it's not immediately obvious to me that regenerating would be required this time. Would it be? (In reply to comment #21) > Kenn, > > But previous to this, there were no generated operation indexes. So it's not > immediately obvious to me that regenerating would be required this time. Would > it be? Actually, good point. It depends on what kind of code has been generated (e.g. whether any custom templates are being used which reference/generate operation indexes), and with which generator (e.g. the UML2 generator, which extends the EMF one, generates code with operation indexes [now literals] in it). OCL has not regenerated but seems fine albeit in a hybrid state, only the reflectivde eAllOperations() 'fails'. I tried regenerating and hit a major disaster area (largely empty files), which I put down to the use of custom templates. Since both Ecore and UML2 bindings ultimately extend Ecore, is this likely to be problem? I'm reluctant to inflict any more OCL-related pain on projects at present, so an OCL regeneration for M4 is not planned. We found the following change in EcorePackageImp that breaks us. I will cut and paste from an email:
the following line was removed from EcorePackageImpl#initializePackageContents()
eModelElementEClass.getESuperTypes().add(this.getEObject());
EMF has changed an element (EClass) supertype list. Our GMF edit helper framework is based on the EcorePackage and now it can no longer recognize an EModelElement as an EObject due to the above line being removed. The element types are registered against EClass; therefore, calls to eClass#getSuperTypes() no longer includes EObjects. Other code remains unaffected since it does instanceof checks.
(In reply to comment #24) > We found the following change in EcorePackageImp that breaks us. I will cut and > paste from an email: > > the following line was removed from > EcorePackageImpl#initializePackageContents() > eModelElementEClass.getESuperTypes().add(this.getEObject()); > > EMF has changed an element (EClass) supertype list. Our GMF edit helper > framework is based on the EcorePackage and now it can no longer recognize an > EModelElement as an EObject due to the above line being removed. The element > types are registered against EClass; therefore, calls to eClass#getSuperTypes() > no longer includes EObjects. Other code remains unaffected since it does > instanceof checks. This doesn't sound good. I'm not sure I fully understand the issue, but wouldn't this only be a problem for models that directly extend EModelElement, i.e., ones that have EModelElement in their class hierarchy? Note that a given model has long had the flexibility of choosing whether or not to directly extend EObject, so it's not such a good idea to assume that every model does. Perhaps the edit helper could be updated based on the fact that all models implicitly extend EObject (whether they do explicitly or not)... (In reply to comment #22) > > Actually, good point. It depends on what kind of code has been generated (e.g. > whether any custom templates are being used which reference/generate operation > indexes), and with which generator (e.g. the UML2 generator, which extends the > EMF one, generates code with operation indexes [now literals] in it). Did the generated operation indexes in UML2 use the same naming convention as the new literals, i.e., would they conflict? If not, it still seems possible that regeneration is not strictly necessary. (In reply to comment #26) > (In reply to comment #22) > > > > Actually, good point. It depends on what kind of code has been generated (e.g. > > whether any custom templates are being used which reference/generate operation > > indexes), and with which generator (e.g. the UML2 generator, which extends the > > EMF one, generates code with operation indexes [now literals] in it). > > Did the generated operation indexes in UML2 use the same naming convention as > the new literals, i.e., would they conflict? If not, it still seems possible > that regeneration is not strictly necessary. Actually, they used to be hardcoded... so for any model that extends EModelElement, those numbers would now be wrong... So then UML definitely requires updating. But, in general, it seems we can say we wouldn't expect even models that extend EModelElement to require regeneration. Sound right? (In reply to comment #28) > So then UML definitely requires updating. But, in general, it seems we can say > we wouldn't expect even models that extend EModelElement to require > regeneration. Sound right? Generally, yes. But there could be cases where someone generated code for a model that extends EModelElement using the UML2 code generator or template extensions which depend on operation indexes. So it's not a guarantee. ;) (In reply to comment 25) Yes, you're right - this is only a problem for models that directly extend EModelElement. Briefly, in GMF, advice that contributes commands to an editing gesture is registered against an EClass. It can be marked as "inherited" by sub-types of that EClass. If it is correct to assume that all models implicitly extend EObject, then we can easily fix this in GMF by automatically including any advice bound to EObject, whenever EObject isn't already in the supertypes. (In reply to comment #30) > If it is correct to assume that all models implicitly extend EObject, then we > can easily fix this in GMF by automatically including any advice bound to > EObject, whenever EObject isn't already in the supertypes. Sounds like a plan. Created attachment 155103 [details]
Allows reflective EOperation invocation in dynamic models
Kenn, I'm having problems invoking EOperations in a purely dynamic model (i.e. without generated code). The operation indexes for my (base) class start at 0 (zero). Reflectively invoking a method on my dynamic EObject uses EObjectImpl.eInvoke(), which first runs through the switch for its own operations (also zero-based). So I end up calling e.g. EObject.getClass() instead of my own operation.
Maybe I missed something, but I could only successfully invoke EOperations in a dynamic model with the attached patch. This overrides the eInvoke() methods in DynamicEObjectImpl. Sadly, it is a copy of the methods from BasicEObjectImpl. The alternative to copying the code would be to directly delegate to eDynamicInvoke() from eInvoke(), but this lacks the checks for the validity of the given EOperation.
Achim, I imagine the second method isn't needed given that it's not been overridden to be different from that... > I imagine the second method isn't needed given that it's not been overridden to
> be different from that...
Yeah, you're right. I just chatted with Kenn and he had the same insight. Only the method with the operation index must be overriden.
(In reply to comment #34) > > I imagine the second method isn't needed given that it's not been overridden to > > be different from that... > > Yeah, you're right. I just chatted with Kenn and he had the same insight. Only > the method with the operation index must be overriden. That's right, only the one override is needed. The fix has been committed to CVS. Fix available in HEAD: 2.6.0.I200911261507. To clarify, the original feature was made available 2.6.0.I200911261507. The DynamicEObjectImpl fix is available in 2.6.0.I201001101746. |