Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 367822

Summary:

[evaluator] Support for dynamic dispatch

Product:

[Modeling] OCL

Reporter:

Philipp W. Kutter <kutter>

Component:

Core

Assignee:

OCL Inbox <mdt-ocl-inbox>

Status:

CLOSED FIXED

QA Contact:

Severity:

major

Priority:

CC:

eclipse, ed, kovalsky

Version:

3.2.0

Flags:

ed: juno+

Target Milestone:

Hardware:

OS:

Windows 7

Whiteboard:

Legacy, Compliance

Bug Depends on:

366229

Bug Blocks:

Attachments:

Description	Flags
Screenshot of how far I brought the example with the OLD OCL implementation	none
Example Project how far I brought it with the OLD ocl implementation	none
WORKING projects with PIVOT Implemenation	none
ECore to reproduce with OLD implemenation as described	none

Description Philipp W. Kutter

2012-01-04 05:11:17 EST

Build Identifier:  20110916-0149

During testing and discussion of bug 339952, we found that the same problem does exist for overriding (not only overloading) at least for the old stable OCL implementation. It seems to be fixed for the new Pivot implemenation.

We again added the code to the standard tutorial with class Book, that can be run with newesst OCL.

Here is the example textually, using the tutorial syntax, slightly condensed:

 class Book    {
  operation opOcl() : String = ocl://'calling Book.opOcl()'
  operation opJava() : String = java://return "calling Book.opJava()";
 class BookSubtype extends Book    {
  operation opOcl() : String = ocl://'calling BookSubtype.opOcl()'
  operation opJava() : String = java://return "calling BookSubtype.opJava()";       
  operation opWithBookParCallingOpOnParOcl(par: Book): String = 
    ocl://par.opOcl()
  operation opWithBookParCallingOpOnParJava(par: Book): String =
    java://return par.opJava();
  attribute resultOfOpOclOfSubBook : String =
    ocl://self.opWithBookParCallingOpOnParOcl(self)
  attribute resultOfOpJaveOfSubBook : String = 
    java://return this.opWithBookParCallingOpOnParJava(this);


(please note that the Java code for the Java attribute needs to be added by
hand. The operation versions are generated from the GenModel annotation)

If you run it, you get different results for the two attributes. You see it in
the screenshot.

Some explanaitons to the example:
- We define opOcl and opJava both in Book and BookSubtype, no parameters.

All four methods simply return a string, telling in which class they are defined.
The returned strings are thus:
    "calling Book.opOcl()"
   "calling Book.opJava()"
    "calling BookSubtype.opOcl()"
    "calling BookSubtype.opJava()"

- We define opWithBookParCallingOpOnParOcl and opWithBookParCallingOpOnParJava only in BookSubtype, both of them have a parameter par of static type Book, and they simply call the parameterless opOcl, respectively opJava on that parmater. Thus their definitions are:
  par.opOcl()
  return par.opJava();

Then with the two attributes
    Result Of Op Ocl of SubBook
    Result Op Op Java of Subbook

we return the result of applying in the subtype BookSubtype opWithBookParCallingOpOnParOcl/opWithBookParCallingOpOnParJava to
self/this:
    self.opWithBookParCallingOpOnParOcl (self)
    this.opWithBookParCallingOpOnParJava (this)

We all know, that the expected behavior is that the same is returned for both
attributes. 

The logic is that based on the dynamic type of self/this we select the opOcl/opJava implementation of the most specialized class
and call it. The versions inherited from Book are implemented in the most specialized class, thus they SHOULD be called.

The Java code behaves like this.

The OCL for PIVOT behaves like this. 

The stable MDT OCL implementation was reported to NOT behave like this. In the attached example projects, I however have the problem that it returns nothing, not the wrong result.

Since this dynamic binding of overriding methods is a major feature for every OO language, and since I assume we all agree, that for ECore, that can be loaded from Java, and generates Java, this behavior has to be the same.

I consider this a major, breaking bug for any OCL+ECore usage in a larger
context.

I am attaching the screenshot and the ecore.


Reproducible: Always

Steps to Reproduce:
1. Generate standard ECore code from this 
2. Set the body of the resultOfOpJaveOfSubBook method to return
this.opWithBookParCallingOpOnParJava (this);
3. Launch an Eclips Application
4. Create an instance of the BookSubtype, and look at the attributes in the
property view

Comment 1 Philipp W. Kutter

2012-01-04 05:14:14 EST

Created attachment 208989 [details]
Screenshot of how far I brought the example with the OLD OCL implementation

Comment 2 Philipp W. Kutter

2012-01-04 05:14:58 EST

Created attachment 208990 [details]
Example Project how far I brought it with the OLD ocl implementation

Comment 3 Philipp W. Kutter

2012-01-04 05:15:46 EST

Created attachment 208991 [details]
WORKING projects with PIVOT Implemenation

Comment 4 Philipp W. Kutter

2012-01-04 05:16:49 EST

Created attachment 208992 [details]
ECore to reproduce with OLD implemenation as described

Comment 5 Axel Uhl

2012-01-04 12:11:28 EST

*** Bug 339952 has been marked as a duplicate of this bug. ***

Comment 6 Philipp W. Kutter

2012-02-01 02:05:59 EST

To fix this bug in the old ECore Binding,
AFAIK, the issue of polymorphic OperationCallExp is caused by the OCLInvocationDelegate where the body is fetched from a cache based on the static binding:

 

       public Object dynamicInvoke(InternalEObject target, EList<?> arguments)

                     throws InvocationTargetException {

              OCL ocl = delegateDomain.getOCL();

              if (body == null) {

                     body = InvocationBehavior.INSTANCE.getOperationBody(ocl, eOperation);

              }

                                ...

 

Instead of only passing eOperation to InvocationBehavior.INSTANCE.getOperationBody, the target or at least its dynamic class would need to get passed as well to find the "most derived" binding.

Ed Willink: can we contribute a fix of this bug? 

Both GMF-T and QVTO as well as other consumers of the old ECore binding would profit.

Comment 7 Axel Uhl

2012-02-01 03:19:45 EST

(In reply to comment #6)

I agree with Philipp's assessment regarding the additional argument required for the cache lookup and am looking forward to seeing a patch for this along the lines Philipp has outlined.

> Instead of only passing eOperation to
> InvocationBehavior.INSTANCE.getOperationBody, the target or at least its
> dynamic class would need to get passed as well to find the "most derived"
> binding.
> 
> Ed Willink: can we contribute a fix of this bug? 
> 
> Both GMF-T and QVTO as well as other consumers of the old ECore binding would
> profit.

I presume that in order to fulfill Ed's standards regarding backward compatibility the patch would need to introduce a setting to choose this behavior, although I'd personally really prefer making this the standard because the current behavior I consider a plain bug.

Best,
-- Axel

Comment 8 Ed Willink

2012-02-01 03:31:31 EST

InvocationDelegates are new code, so the issues of preserving traditional behaviour are much less constraining.

You seem to have almost spotted the bug; but it is not the cache adapter that is wrong. The cache correctly uses the passed eOperation. It is the call that neglects to look for a derived eOperation to locate in the cache.

This can and should be fixed. If you want to contribute that's helpful.

I suggest looking for and trying to reuse any dynamic resolution code that is already present; I'm sure that there is some for use during analysis. Overload detection should be based on exact OCL type match of all arguments. Beware that during analysis operation resolution selects a static operation with compatible arguments, but during execution resolution selects the derived operation with identical arguments to the static operation.

Testing should ensure that a.b(...) overloads are consistently resolved when
- b is an Ecore dynamic operation (an unregistered meta-model)
- b is an Ecore 'compiled' operation (a registered meta-model)
It would be nice to be consistent too for
- b is an additional Complete OCL operation

The DelegatesTest has registered/unregistered variants so can be extended for the first two. The location of existing Complete OCL tests needs a search.

Legacy built-in behaviour should be retained for library operations and additional Complete OCL operations.

The Ecore representation has no caches to assist in location of the derived operation, so you have to search at least all operations of all derived classes. (If the static operation is abstract - no body, I guess you have to search sibling and their superclasses too.)

You might be tempted to introduce caches for the derived lookup. I'd discourage this since the code generator already produces optimised dispatch tables making compiled Ecore models (much) faster. It should be possible to use the reflective mode to create the same dispatch tables dynamically. I see no reason why QVTo and QMF-T cannot benefit from using the code generated OCL.

Comment 9 Axel Uhl

2012-02-01 04:00:38 EST

A patch should ideally also contain an Impact Analyzer (IA) example / test case that uses polymorphic resolution and where the IA results depend on understanding the possibility of polymorphic resolution.

I'm happy assisting in producing a corresponding patch for the IA once we have a failing test case.

Comment 10 Axel Uhl

2012-02-01 04:13:39 EST

Quick analysis: EvaluationVisitorImpl.visitOperationCallExp currently calls getOperationBody(oper) for the statically-resolved EOperation. The method for the Ecore implementation is defined in o.e.o.ecore.EvaluationVisitorImpl.getOperationBody(EOperation).

I suggest to determine the operation that dynamically needs to be invoked already in EvaluationVisitorImpl.vitisOperationCallExp and then calling getOperationBody(dynamicOper) instead of getOperationBody(oper). This can leave the getOperationBody signature and caching mechanisms implemented through public APIs on InvocationBehavior unchanged and would probably minimize the set of changes required.

Comment 11 Ed Willink

2012-02-01 04:25:23 EST

(In reply to comment #8)
> You might be tempted to introduce caches for the derived lookup. I'd discourage
> this since the code generator already produces optimised dispatch tables making
> compiled Ecore models (much) faster. It should be possible to use the
> reflective mode to create the same dispatch tables dynamically. I see no reason
> why QVTo and QMF-T cannot benefit from using the code generated OCL.

Actually it looks so easy to put an EOperation to OclExpression cache in
DelegateEClassifierAdapter that it would be stupid not to.

(In reply to comment #10)
> Quick analysis: EvaluationVisitorImpl.visitOperationCallExp currently calls
> getOperationBody(oper) for the statically-resolved EOperation. The method for
> the Ecore implementation is defined in
> o.e.o.ecore.EvaluationVisitorImpl.getOperationBody(EOperation).
> 
> I suggest to determine the operation that dynamically needs to be invoked
> already in EvaluationVisitorImpl.vitisOperationCallExp and then calling
> getOperationBody(dynamicOper) instead of getOperationBody(oper). This can leave
> the getOperationBody signature and caching mechanisms implemented through
> public APIs on InvocationBehavior unchanged and would probably minimize the set
> of changes required.

That would be a better solution, but provides no cache and runs into the traditional behaviour nightmare. The facility to add CompleteOCL additional operations has been around for a long time; it is difficult to understand and use and every usage of it has surprised me through it's clumsiness. Both Acceleo and QVTo struggle with it for their extended libraries. I really do not want to change things in the legacy EvaluationVisitorImpl area.

Comment 12 Axel Uhl

2012-02-01 07:34:37 EST

Ed,

if you don't want the EvaluationVisitorImpl to change, yet achieve the fixed behavior, this IMHO would imply a change to the public InvocationBehavior APIs which is much more difficult from an API perspective.

If caching is the problem, the lookup results of the (source:Object, staticOp:EOperation) pair to the EOperation which is to be actually called may be stored in an additional cache.

Comment 13 Ed Willink

2012-02-01 14:10:51 EST

(In reply to comment #12)
> if you don't want the EvaluationVisitorImpl to change, yet achieve the fixed
> behavior, this IMHO would imply a change to the public InvocationBehavior APIs
> which is much more difficult from an API perspective.

This change is unlikely to be ready in time for SR2; we're already at SR2RC2. For Juno we can easily add APIs if necessary.

I don't actually see any need to change API in InvocationBehavior or OCLInvocationDelegate. It may be useful to add some DelegateEClassifierAdapter API to support the revised OCLInvocationDelegate code: from

body = InvocationBehavior.INSTANCE.getOperationBody(ocl, eOperation);

to something like

dynamicOperation = DelegateEClassifierAdapter.resolveDynamicOperation(target, eOperation);
body = InvocationBehavior.INSTANCE.getOperationBody(ocl, dynamicOperation);

> If caching is the problem, the lookup results of the (source:Object,
> staticOp:EOperation) pair to the EOperation which is to be actually called may
> be stored in an additional cache.

Certainly not a major problem; just more changes to a class that is known to have tightly-coupled user derivations.

Comment 14 Ed Willink

2012-02-01 14:33:53 EST

(In reply to comment #8)
> I suggest looking for and trying to reuse any dynamic resolution code that is
> already present; I'm sure that there is some for use during analysis. 

AbstractTypeChecker.findOperationMatching is for analysis; it's control flow identifies what needs to be considered. It needs simplifying for exact match.

Collection and Generic types need care and testing.

Comment 15 Ed Willink

2012-04-30 15:09:01 EDT

Revisiting. The foregoing analysis seems wrong. The problem is not primarily to do with the delegate evaluation; it is a longstanding feature of the main evaluator.

I see no code in EvaluationVisitorImpl.visitOperationImpl() that uses the type of source to refine the selection of operation. Static dispatch is therefore the legacy behaviour. The flattening of all operation lists in e.g. oclstdlib.ecore and oclstdlib.uml is in accord with this. Static dispatch is not inconsistent with the OCL 2.0, 2.2 or 2.3 specifications. It is just an offense against Object Oriented principles. (The pivot implementation has a well-defined dynamic dispatch.)

WONTFIX for the legacy implementations wrt OCL 2.3.

Raise a new bug if/when an Ecore binding is required to comply with OCL 2.5.

Comment 16 Ed Willink

2012-05-01 01:18:51 EDT

See http://wiki.eclipse.org/MDT/OCL_Limitations for detailed comparison of Ecore and Pivot functionality and OCL 2.5 considerations.

Comment 17 Philipp W. Kutter

2012-05-03 05:38:31 EDT

Here the same situation as 366229

I would propose to reconsider it as well.

While the overloading problem of 366229 can be considered as a subtle issue, where good modeling avoids overloading, for overriding this is not the case. Overriding is the essence of object oriented programming: attach the behavior to the object, not to static types.

This bug makes the OCL implementation unusable for our business modeling activities. It will drive us away from the current implementation, and as a consequence it will drive us away from the pivot implementation in the future.

Comment 18 Ed Willink

2012-05-07 13:06:22 EDT

An option to enable dynamic dispatch has been provided. It can be enabled programmatically or via a global preference.

Use of dynamic dispatch may incur a few perecent degradation; very dependent on how intensive the operation call part of an evaluation is.

Comment 19 Ed Willink

2013-05-20 11:37:16 EDT

CLOSED after a year in the RESOLVED state.

Comment 20 Svyatoslav Kovalsky

2013-05-22 03:45:10 EDT

Could you give the option name or a snippet to access it programmatically?

Comment 21 Ed Willink

2013-05-22 04:18:47 EDT

(In reply to comment #20)
> Could you give the option name or a snippet to access it programmatically?

See test_ambiguous_withDynamicDispatch in GIT\org.eclipse.ocl\tests\org.eclipse.ocl.ecore.tests\src\org\eclipse\ocl\ecore\tests\NamesTest.java.