[evaluator] OCL discards values from lists returned by eoperations

[eclipse-ocl-bot]

| --- | --- | | Bugzilla Link | 287052 | | Status | CLOSED FIXED | | Importance | P3 normal | | Reported | Aug 19, 2009 08:57 EDT | | Modified | Nov 21, 2014 11:23 EDT | | Version | 1.3.0 | | Blocks | 156363, 286784 | | Reporter | Laurent Goubet |

Description

An expression such as "e.oclAsType(EObject).eGet(attribute)->size()" will never return anything else than "1".

The EvaluationVisitorImpl delegates this call to its evaluation environment as it is an EOperation defined in the metamodel (here, ecore). Delegation is done at line 192 of EvaluationVisitorImpl#visitOperationCallExp() :\ ----------8<----------\ result = getEvaluationEnvironment().callOperation(oper, opCode, sourceVal, evalArgs);\ ---------->8----------

Be it the EcoreEvaluationEnvironment or the UMLEvaluationEnvironment, both use coerceValue() to determine the appropriate representation of the operation's return type. The offending code is in these implementations of coerceValue :

----------8<----------\ CollectionKind kind = getCollectionKind(element);

if (kind != null) {\ [...]\ } else {\ if (value instanceof Collection) {\ Collection<?> collection = (Collection<?>) value;\ return collection.isEmpty() ? null : collection.iterator().next();\ } else {\ return value;\ }\ }\ ---------->8----------

If we couldn't determine the collection kind ... we decide to only return the first value contained by the collection.

I am not familiar enough with the TypeChecker mechanism introduced by Christian in 1.3 to know if it could help in this case. Attached is a stupid patch using instanceofs to determine the needed collection kind, namely : a Bag if the "collection" value is a bag, a Sequence if it is an instanceof List (there only are ordered lists in Java AFAIU), an OrderedSet if it is an instanceof LinkedHashSet, a Set in all other cases.

Feel free to point me towards more intelligent potential implementations :).

[eclipse-ocl-bot]

By Laurent Goubet on Aug 19, 2009 09:03

Created attachment 144951 (attachment deleted)\ Second attempt at creating the attachment

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Aug 19, 2009 11:21

Hi Laurent,

I haven't revised the code or your patch. I'm just recalling to the OCL 2.0 specification. Take a look to the page 81. Rule [B] of OperationCallExpCS:

-- The source is either a collection or a single object used as a collection.\ [B] OperationCallExpCS.ast.arguments = argumentsCS.ast\ -- if the OclExpressionCS is a collectiontype, then the source is this OclExpressionCS.\ -- Otherwise, the source must be build up by defining a singleton set containing\ -- the OclExpressionCS. This is done though inserting a call to the standard\ -- operation "asSet()"

Since the return parameter of the EObject::eGet operation is a single Object, a set is implicitly created as the source of the operationCallExp, which will contain the returned Object. So I would always expect a 1 value, if you call ->size(). You will have always object regardless it's really one object or a list of objects.

I would previously call ->flatten() to obtain the number of elements in the case you are dealing with a multiple value structural feature.

If u agree, please simply resolve as invalid or something like that.

P.S: Please take part in Bug 242153. I need your and Alex votes to go on with the BUG.

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Laurent Goubet on Aug 19, 2009 11:28

Hi Adolfo,

The problem here doesn't lie in the call to "size" : I simply added it for the sake of having a number; thus the issue isn't with "eGet" being the source of the call to "size()".

the returned value of "eGet" truly is a single element being the first element of the "real" returned value (which was a collection). The additional values are discarded and cannot be accessed in any way.

P.S : I didn't join in Bug 242153 since I am totally unfamiliar with both IMP and LPG ... I'll try and take a look asap but I don't think my opinion is relevant here.

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Aug 19, 2009 12:21

Hi Laurent,

I have had a look to the code. I think that is not a problem of the coercingValue algorithm, which seems to be "right". I mean, the decision is good: If the feature expects to deal with multiple values, regardless you have a collection of values or a single value, you coerce the value to a collection. On the other hand, if the feature deals with a single value, regardless your have a single value or a collection of values, you corce the value to a single one.

I guess that the problem could one of the following:

• we are erroneously trying to coerce the value.
• we erroneously coercing a collection value, when we have a feature which deals with a single value.
• we are erroneously considering that the feature deals with a single value, when it should be deal with a collection of values (getCollectionKind shouldn't returned a null kind).

I would exactly need the problematic oclExpression, and the Ecore or UML Environment you are using to go on this.

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Ed Willink on Aug 19, 2009 14:33

I don't understand the problem:

The code you're examining is what happens when you have a Collection for a non-collection element; a fairly daft scenario that might merit flattening to give the first value.

getCollectionKind should not return null for a greater then 1 upper bound.

[eclipse-ocl-bot]

By Laurent Goubet on Aug 20, 2009 03:21

The problem here comes from the fact "eGet" is an EOperation with its return type defined to "EJavaObject". Obviously the multiplicity here is 1 since we can be dealing with either multiple or single multiplicity features (I think we all know what "eGet" does).

In this case, the EOperation can return just about anything except for primitives. This includes every single subclass of "Object" ... and this includes Collections. The issue with the coercing algorithm is that it knows it is dealing with a Collection, but it forcibly deletes every value except for the very first.

I observed this with eGet, it can happen with any EOperation defined by clients on their DSLs. AFAIU, this will be reproduced with any EOperation with a "1" upper bound but a return type like "EJavaObject", "EList" or any custom data type representing a list.

Granted, these probably won't happen often. The bug hadn't even been detected even though I believe it is present since the first versions. I still believe we should try and fix this behavior : writing an exception which return value should be a collection, yet only getting the first value of this collection seems erroneous to me.

[eclipse-ocl-bot]

By Ed Willink on Aug 20, 2009 04:20

Can you attach the JUnit test case that demonstrates the problem?

Yes it needs fixing, but I'm not convinced that this is the correct diagnosis and consequently solution.

[eclipse-ocl-bot]

By Laurent Goubet on Aug 20, 2009 04:29

Ed,

I haven't created a JUnit test for this, I simply reused the sample of bug 286784.

I'll make one reproducing this bug and atach it asap

[eclipse-ocl-bot]

By Laurent Goubet on Oct 13, 2009 09:44

Created attachment 149436 (attachment deleted)\ Test case highlighting the problem

The attached test case highlights this problem. I put a lot of assertions in there, but all of them are needed as Java doesn't check for the ordering when calling "LinkedHashSet.equals')" ...

This test retrieves the superslass of an EClass (created for the occasion with two superclasses) by two differents means :

• the straightforward "clazz.eSuperTypes"
• the convoluted "clazz.eGet(clazz.eClass().getEStructuralFeature('eSuperTypes'))

I think we all aggree that the results of these two calls should be equal in both type (OrderedSet) and content (the two superclasses of "self"). Where this bug strikes is : the results are not equal. "eGet" is an EOperation defined with its return type set to "EJavaObject", thus OCL doesn't manage to find the actual collection type ... and decides to simply return the first value of this collection, ignoring the remainder.

[eclipse-ocl-bot]

By Ed Willink on Oct 16, 2009 09:23

Hm . I responded to this two days ago, but I think I remember what I wrote.

The test case helps. Using the debuggers toString hsows that the parsed AST contains Set{} for a CollectionLiteralExp.

The problem is that eGet does not return a collection so an implicit set is created for it, and since the implicitly created set contains the unflattened results, your resulting size is 1 not 2.

What you want cannot be done in OCL 2.0 or MDT/OCL 1.x since CollectionTyupe cannot be used in a cast.

Your test case should be used in a JUnit for MDT/OCL 3.0.

[eclipse-ocl-bot]

By Laurent Goubet on Oct 16, 2009 10:04

Ed,

The problem is not with "the resulting size" being "1", the problem is that the result we get is not even a Collection. With what we get as is, the second super type of the test EClass is not even accessible : it's been discarded before the result has been returned.

I must admit I don't know what the ast looks like for this example, but I would expect to get a Collection, even if its type is erroneous (Set instead of an OrderedSet). Feel free to affect the target milestone if you think it cannot be done before MDT/OCL 3.0.

[eclipse-ocl-bot]

By Ed Willink on Oct 16, 2009 10:55

(Sorry, my lost reply was longer and better.)

The problem is your

->asType(EClass)

The -> applied to an Object (not statically known to be a Collection) provokes creation of a Set to contain it. You then cast the Set to a non-Collection.

The only bug I see for OCL 2.0 is a failure to generate an error from this cast.

[eclipse-ocl-bot]

By Laurent Goubet on Apr 19, 2010 11:47

As it stands, this problem also arises when trying to retrieve an UML tagged value which contains multiple elements ; that is, all values except the very first are discarded.

so if you have an XMI looking like:

\ Lib 1\ Lib 2\ for a given stereotype application (obviously multi-valued) and try to retrieve its value through the standard UML operation "Element::getValue(Stereotype, String) : EJavaObject" you end up with only "Lib 1". The (convoluted) expression to use here would be :\ umlElement.getValue(umlElement.getAppliedStereotype('Setting'), 'libraries') I'll try to attach an UML model which present such a tagged value for you to try out :p. Note that because of the code I highlighted in comment \#0 there is absolutely no way around this issue ; no user will ever see his "Lib 2" value because of the coercion from "Collection" to "collection.iterator().next()".

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Jun 16, 2010 16:05

Laurent, Ed,

Coming again from the newsgroup ;P

I'm recalling (now, better spelled) the different causes of the problems we have here:

"I guess that the problem could be one of the following:\ a) we are erroneously trying to coerce the value.\ b) we are erroneously coercing a collection value, when we have a feature which\ deals with a single value.\ c) we are erroneously considering that the feature deals with a single value,\ when it should deal with a collection of values (UMLEcoreEnvironment::getCollectionKind function shouldn't\ return a null kind)."

This coud be better understood as follows:

On the one hand, we could think that this is not a problem from the OCL point of view. The feature, to be more precise, the Element::getValue(...) Operation, has been defined as mono-valued so that the coercion algorithm () is properly working: A collection value is being coerced to a single one, which it turns into the first value of the collection (This could also be debatable).

On the other hand, as noticed by Laurent, the user would have expected to obtain a collection from the operation above, and the OCL implementation is avoiding to obtain the desired result.

• It's not a problem of providing an incorrect OCL expression, but it's a problem of the coercing algorythm used.
• It's not a problem of the UML metamodel. In a similar way, in Java we usually have Object getAValue( ) methods, which in runtime may return a List

[eclipse-ocl-bot]

By Ed Willink on Jun 16, 2010 16:50

I really do not want to see this fixed in the current evaluator which has no considered support for "Collection conformsTo OclAny", so as I observed in comment 12 the only problem is a missing error message.

The model-driven evaluator will support "Collection conformsTo OclAny" allowing the non-collection return of eGet() to be cast to a collection and so allow direct use of collection methods. Hopefully in 3.1.0.

For Acceleo, perhaps the best workaround is for Acceleo to provide the requisite functionality in an OclAny::asCollection() built-in library operation.

In 4.0.0, the model-driven library should apply to the existing parser, so the missing error message should appear until .oclAsType(Collection) is used.

[eclipse-ocl-bot]

By Laurent Goubet on Jun 17, 2010 03:44

Adolfo, Ed,

This might either be seen as a coercion bug (the assumption that "0..1" multiplicity is a single value, and cannot be a single collection) or as Ed mentions, a specification issue (Collection doesn't conform to OclAny); which means we (OCL) think that "EObject", "OclAny", "EJavaObject"... and in fact "any DataType" are one and the same : OclAny.

If I defined operation "MyType::test() : MyDataType" with "MyDataType" representing a collection, this bug would be observed. And that isn't such a rare occurence.

I am inclined to tell this is a coercion bug, Adolfo's "a)".

Acceleo doesn't have any way to workaround this, as coerceValue is private.

However I look at it, this :

if (value instanceof Collection) {\ Collection<?> collection = (Collection<?>) value;\ return collection.isEmpty() ? null : collection.iterator().next();\ } else {\ return value;\ }

Is something I cannot see as "not a bug"; at the very least the condition of this if should be "value instanceof Collection && ((Collection)value).size() <= 1" and in the case where the collection contains more than one value, return a proper Bag, Sequence, OrderedSet or Set ...

I guess OCL simply isn't meant to be used to call "UML" operations ... as a good number of those return EJavaObjects.

[eclipse-ocl-bot]

By Ed Willink on Jun 17, 2010 04:59

(In reply to comment #16)

If I defined operation "MyType::test() : MyDataType" with "MyDataType" representing a collection, this bug would be observed. And that isn't such a rare occurence.

This not valid because we do not currently support Collection conforms to OclAny.

Acceleo doesn't have any way to workaround this, as coerceValue is private.

Would it help if we made coerceValue protected?

---

I feel that we have at least 10 bugs associated with Collection conforms to OclAny, quite apart from numerous obscure corner null/invalid cases.

In MDT/OCL 3.0.0, we have actually changed very little, so hopefully we will not get major complaints.

Establishing something close to exact conformance is possible with the current code but quite hard. It should be really easy once the model-driven library is in place; users can redefine their own conformance.

Therefore I do not want to:

a) spend time discussing the existing problems\ b) change the existing problems to perhaps just different problems

so unless you are prepared to commit two man-weeks of detailed analysis and fix time to the current code, please be patient.

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Jun 17, 2010 05:11

Ed, Laurent

I'm Ok if we wait until the model-driven library is in place.

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Laurent Goubet on Jun 17, 2010 05:25

I also am okay with waiting for the model-driven library, I only wanted to give my 2 cents :).

Having coerceValue protected would help, as we (Acceleo ... and other adopters maybe? I don't know if others have such issues) could override it and avoid the value discarding in our "non-standard" build mode.

[eclipse-ocl-bot]

By Ed Willink on Jun 17, 2010 06:57

Hi. I couldn't resist thinking a bit more.

-1 to the patch...

coerceValue has four paths to match available value to required typed-element.\ a) Collection -> Collection --- conditional copy\ b) !Collection -> Collection --- creates collection\ c) Collection -> !Collection --- null or first element\ d) !Collection -> !Collection --- transparent

a) should do nothing; a copy should not be needed since OCL only does queries\ b) should not occur since static type analysis should have scheduled a conversion\ c) should not occur since static type analysis should have scheduled a conversion\ d) correctly does nothing

The c) path establishes a traditional MDT/OCL behaviour

• an empty collection is null
• a non-empty collection is the first element

Changing c) to return a new Collection for an object expectation is definitely a different behaviour and may upset users who expect the null.

I think coerceValue should do nothing on all paths, if we were fixing the current code.

For the benefit of Acceleo, we can make coerceValue protected and deprecated. I recommend you do nothing on path c) so that your original problem can then be coded as:

"e.oclAsType(EObject).eGet(attribute).asCollection()->size()"

where OclAny::asCollection() : Collection does nothing, but satisfies the OCL type checking.

In due course, your original example should be coded as

"e.oclAsType(EObject).eGet(attribute).oclAsType(Collection)->size()"

"e.oclAsType(EObject).eGet(attribute)->size()" will always be semantically valid and return 1 whenever eGet returns a Collection (even an empty one).

We can highlight this semantic change in 'coerceValue' as part of the change to support Collection conformsTo OclAny.

[eclipse-ocl-bot]

By Ed Willink on Jun 18, 2010 06:02

Since Adrian's problem persists on the newsgroup, I've investigated my theory that coerceValue should do nothing.

Our JUnit tests never use the convert Object to Collection path; so collection conversions have been correctly synthesized by the type system.

The UML JUnit tests use the convert Collection to its first element when resolving non-navigable opposites and stereotypes and unnamed associations; not an Ecore capability. The search for a non-navigable opposite creates a collection of solutions, which coerceValue cleans up. (This problem goes away with the pivot model, since the pivot is a copy of the user-meta-model, the missing opposite property can be inserted into the copy without corrupting the original; all opposites are navigable.)

I think we could move the cleanup to the back of the non-navigable opposites search, so that coerceValue indeed did nothing, and so did not corrupt the Collection masquerading as non-Collection path.

[eclipse-ocl-bot]

By Laurent Goubet on Jun 18, 2010 06:55

Ed,

you went way farther than I did when first observing the issue; and I believe your solution to be way better than my quick & dirty proposal.

[eclipse-ocl-bot]

By Ed Willink on Jun 18, 2010 07:54

Created attachment 172208 Avoid redundant coercions

Attached patch makes

self.eGet(self.eClass().getEStructuralFeature('eSuperTypes'))->flatten()->asSet()

work in a JUnit test on Ecore, and potentially makes UML work too, except that I don't know how to make reflection work for UML; there is no getMetaClass() and specifying EObject as implicit root is not supported. The pivot model's oclType() method will solve this.

NB. It is necessary to use flatten() to workaround parsing failures in oclAsType(Set) and/or analysis failures in oclAsType(Set(EClass)).

I'm inclined to just leave this as a patch for enthusiasts, but maybe someone will review it and decide that they're happy with it's safety.

:notepad_spiral: Bug287052.patch

[eclipse-ocl-bot]

By Ed Willink on Jun 18, 2010 16:27

(In reply to comment #23)

NB. It is necessary to use flatten() to workaround parsing failures in oclAsType(Set) and/or analysis failures in oclAsType(Set(EClass)).

Bug 297538.

[eclipse-ocl-bot]

By Ed Willink on Feb 01, 2011 03:24

The new evaluator uses the statically determined types to propagate values rather than deducing type information from dynamic values for which as in the case of eGet retuirn Object, the type dynamic information is unsafe.

[eclipse-ocl-bot]

By Ed Willink on May 27, 2011 06:40

Resolved for Indigo is 3.1.0 not 3.2.0.

[eclipse-ocl-bot]

By Ed Willink on May 29, 2012 13:22

Closing all bugs resolved in Indigo.

[eclipse-ocl-bot]

By Yves BERNARD on Nov 20, 2014 07:45

Created attachment 248789 Test model (Papyrus/Luna)

:compression: Test.zip

[eclipse-ocl-bot]

By Yves BERNARD on Nov 20, 2014 07:54

I get a similar issue with OCL 5.0.2 on Luna (SR1).\ On the test model above, using the Intercative OCL console, I tried the query:

let o : ecore::EObject = self.owner.oclAsType(ecore::EObject) in\ let cf : ecore::EStructuralFeature = o.oclAsType(ecore::EObject).eClass().getEStructuralFeature('value') in\ o.eGet(cf)->flatten()->asSequence()

with the context set to any of the 2 values defined for the slot of the instance specification.

In either case, only the first value ("A1") is returned.

Did I do something wrong?

Yves

[eclipse-ocl-bot]

By Ed Willink on Nov 20, 2014 13:37

(In reply to Yves BERNARD from comment #29)

Did I do something wrong?

Yes. You added a new bug to a closed bug.

You are using type-unsafe non-OCL functionality in the classic Eclipse OCL, for which eGet() dopes not know what to do. It is a known limitation.

The equivalent Pivot functionality in the Xtext OCL Console should allow you to use oclContainer() and .value.

[eclipse-ocl-bot]

By Yves BERNARD on Nov 21, 2014 03:01

Yes. You added a new bug to a closed bug.

Sorry if it was not the correct way to deal with it. My topic was exactly the same and then it was logical to me doing this way (i.e. reopening the topic) since it is a common policy. Anyway, if the procedure is different here , I can create a new one, just let me know.

The equivalent Pivot functionality in the Xtext OCL Console should allow you to use oclContainer() and .value.

My understanding is that your advice is to avoid using eGet(). Did I get it right?

Thanks,

Yves

[eclipse-ocl-bot]

By Ed Willink on Nov 21, 2014 04:09

(In reply to Yves BERNARD from comment #31)

Yes. You added a new bug to a closed bug. Sorry if it was not the correct way to deal with it. My topic was exactly the same and then it was logical to me doing this way (i.e. reopening the topic) since it is a common policy. Anyway, if the procedure is different here , I can create a new one, just let me know.

It's slightly a matter of time. One week after it's resolved, it may not be resolved after all. Three years later, it's a new problem, that may refer to the old problem for reference.

The equivalent Pivot functionality in the Xtext OCL Console should allow you to use oclContainer() and .value. My understanding is that your advice is to avoid using eGet(). Did I get it right?

I'm endeavouring to support the OMG specifications, where you will find no reference to EObject, eClass(), eGet() etc, so yes, avoid eGet() wherever possible a) because it's non-OMG, b) because when used in Eclipse OCL its polymorphic Object/List return is broken.

[eclipse-ocl-bot]

By Yves BERNARD on Nov 21, 2014 11:23

It's slightly a matter of time. One week after it's resolved, it may not be resolved after all. Three years later, it's a new problem, that may refer to the old problem for reference.

Ok. So let's open a new one.