Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 347960 - "something = oclUndefined" should not be evaluated to oclUndefined
Summary: "something = oclUndefined" should not be evaluated to oclUndefined
Status: CLOSED INVALID
Alias: None
Product: OCL
Classification: Modeling
Component: Core (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: OCL Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 346952
  Show dependency tree
 
Reported: 2011-06-01 11:19 EDT by Michael Golubev CLA
Modified: 2012-05-29 13:22 EDT (History)
3 users (show)

See Also:


Attachments
TestCase that fails (2.80 KB, text/plain)
2011-06-01 13:33 EDT, Michael Golubev CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Golubev CLA 2011-06-01 11:19:42 EDT
Recently OCL had applied a change, ensuring that (quoting comment from the org.eclipse.ocl.EvaluationVisitorImpl, the subject change comes as revision 1.8 at May, 1)

// Note: Generally the result of an operation invocation on the
// undefined
// object or with an undefined argument is undefined except ...

in particular, now, if x.oclIsUndefined() then all of the following expressions will be evaluated to getInvalid():

(1)     x = 42
(2)     x <> 42
(3)     someCollection->select(m | m = x)

The (3) is especially weird, because now we instead of just writing something like 

let parent = ... in
childReferences->select(r | parent.visualID = r.parent.visualID)->asSequence()

we must write the following monster: 

let parent = ... in
childReferences->select(r | 
    not parent.visualId.oclIsUndefined() 
    and not r.parent.visualID.oclIsUndefined()
    and parent.visualID = r.parent.visualID
)->asSequence()

Now in the GMF tooling we have a HUNDREDs of the select's written when the first form of the select was just valuated to empty sequence, and ALL of them now have to be transformed to the latter form with checks. 

I understand that this change comes from specification, but I would like to as you to at least consider the rollback of that behavior at least in the equality checks.
Comment 1 Ed Willink CLA 2011-06-01 12:24:05 EDT
Your description is very difficult to understand because you keep referring to the non-existent concept of undefined.

There is null and invalid.

oclIsUndefined() returns true for null and invalid.

The unit tests pass on:

		assertResultTrue("null = null");

so I can only assume you have some invalid values as part of your problem and if you have an invalid partial result, you never could expect a useable result without using oclIsUndefined().

Since we are now past RC3, you will need to provide a very clear example of the malfunction for us to even look at an alternate behaviour for RC4, and then we will have to be strongly motivated to push for an RC4 approval.

At the moment this looks like a Resolved INVALID.
Comment 2 Michael Golubev CLA 2011-06-01 13:32:19 EDT
Ok, sorry, I am definitely bad in formulating :). 
So, attached please find the test case that hopefully better describes the issue.

The method testSelectThatWorkedBeforeMay1ButNotToday() actually fails with 
junit.framework.AssertionFailedError: Result is: org.eclipse.emf.ecore.impl.DynamicEObjectImpl@394576 (eClass: org.eclipse.emf.ecore.impl.EClassImpl@1ff5160 (name: OclInvalid_Class) (instanceClassName: null) (abstract: false, interface: false))

You may ignore the other 3 passing test methods, they are here just to ensure that something actually works. 

You probably may agree that looking at the ocl expression 
     "eAttributes->select( a | a.eType.name = 'B')->size()"

its difficult to predict that it will result to Invalid in case if, among all of the eAttributes, there is one without a type. 

Regards, 
Michael
Comment 3 Michael Golubev CLA 2011-06-01 13:33:09 EDT
Created attachment 197119 [details]
TestCase that fails
Comment 4 Ed Willink CLA 2011-06-01 13:42:17 EDT
Mid-air collision but I think it is relevant to your later posts.

(In reply to comment #0)
I've tried to reproduce this.

> The (3) is especially weird, because now we instead of just writing something
> like 
> 
> let parent = ... in
> childReferences->select(r | parent.visualID = r.parent.visualID)->asSequence()

The problem seems to be in r.parent.visualID when r.parent is null. Access to a feature of null is generating the invalid that 'breaks' the equality.

> we must write the following monster: 
> 
> let parent = ... in
> childReferences->select(r | 
>     not parent.visualId.oclIsUndefined() 
>     and not r.parent.visualID.oclIsUndefined()
>     and parent.visualID = r.parent.visualID
> )->asSequence()

I cannot workout which permutations of hierarchical nulls you want to be equal, so I cannot work out how to rewrite your code. Your rewrite looks suspect if parent is null.

This seems to be a case where some rather suspect code has been identified by enhanced tooling.

Since GMF Tooling is off-train and presumably on-train projects using GMF tooling are happy with what they have, there is no threat to Indigo. GMT toling can update so that it's Indigo-aligned release is OCL compliant.

Do we need to post a cross-project-dev warning to not-regenerate GMF editors using Helios GMF Tooling and Indigo OCL?
Comment 5 Ed Willink CLA 2011-06-01 14:12:40 EDT
Thanks for the excellent test case.

The OCL 2.3 specification is very clear:

In 7.4.11 (non-normative) "In general, an expression where one of the parts is null or invalid will itself be invalid."

In 11.2.3 "Any property call applied on null results in invalid, except
for the oclIsUndefined(), oclIsInvalid(), =(OclAny) and <>(OclAny) operations."

In A.2.1.1 (non-normative) "The interpretation of operations is considered strict unless there is an explicit statement in the following. Hence, an invalid or
null argument value causes an invalid operation result. This ensures the propagation of error conditions."

The above are from OCL 2.3 where the vagueness of undefined has been resolved, but you will find the same principle in OCL 2.0.

You are navigating from a null object and so get invalid.

The bug was inadequate distinction between OCL-null/invalid and Java-null in the EvaluatorVisitorImpl. This has been much improved.

I'm afraid I see no prospect of deliberately re-introducing a clear bug at RC4 for an off-train project.

Your test case can be fixed by changing

eAttributes->select( a | a.eType.name = 'B')->size()

to

eAttributes->select( a | a.eType->notEmpty() and a.eType.name = 'B')->size() 

cf. the need for (a.getType() != null) && ... in Java to avoid an NPE.
Comment 6 Ed Willink CLA 2011-06-04 06:27:56 EDT
Michael: I've been thinking about your use case, and there is an argument that what you want might be appropriate in a revised OCL specification.

Since OclVoid conforms to all types, perhaps null has null instances of all properties and so transitive navigation of null-valued properties yields null rather than invalid.

On the one hand OCL has very strict null rejection so the above is not appropriate.

On the other hand there is a long list of exceptions to this strictness.

If we consider '.' as a map operator, then without knowing which terms are collections 'a.b.c.d' creates an isomorphic tree of results for 'd' for each 'a.b.c'. If a null anywhere in b or c crashes the calculation, the mapping is useless.

I guess this can be considered when the problems of establishing a stricter type than Collection<OclAny> for heterogeneous collections is addressed. 

Are your actual use cases solved by simple 'NPE' guards or do they exhibit horrendous depth?
Comment 7 Michael Golubev CLA 2011-06-04 06:54:00 EDT
Well, in our context the most of the direct calls that forms a long chains (like a.b.c.d) were already guarded against null's, because we are using it to generate something and the old-way result (null) in most cases is useless for generation. So we naturally had those checks in most cases. 

My primary concern was an iteration expressions (like the ->select from my test case), because the result was still usefull under the old implementation even if some parts of the iteration failed with "kinda-NPE". 

But I have realized that most of these problems may be solved by introducing the qvto helper 

helper undefinedAsFalse(param : Boolean) : Boolean {
	return if param.oclIsUndefined() then false else param endif 
}

and changing all the 
           ->select( a | a.eType.name = 'B')
into 
           ->select( a | undefinedAsFalse(a.eType.name = 'B'))

This may be done semi-automatically by text searching for iteration constructs. 

In theory we even may try to adjust our own evaluator to traverse ASTs and inject this guard calls (try-catches in the Java analogy) to every iteration before delegating to OCL evaluation (also I haven't tried this yet).
Comment 8 Ed Willink CLA 2011-06-04 13:47:31 EDT
I'm glad you have a solution that's not too horrible.

[The new OCL using a pivot model and modeled standard library would allow you as a user to define your own full speed selectIfValid iteration. No idea when QVTo can be aligned with this.]

You didn't answer my question about a cross-project-issues-dev posting. Is this an irritation for the developers of GMF Tooling for which you have a satisfactory workaround, or is it a problem for users of GMF Tooling that needs dissemination?
Comment 9 Michael Golubev CLA 2011-06-06 03:49:45 EDT
(In reply to comment #4)

> Do we need to post a cross-project-dev warning to not-regenerate GMF editors
> using Helios GMF Tooling and Indigo OCL?

I don't think so, after applying the fixes, all of the GMF generation tests are passing and I have successfully regenerated the sample projects, so I have closed #346952 and don't think there is a critical exposure to others. 

Also GMF allows to override templates, so if the new problem will be found in user project, it should always be possible to apply the similar fixes as actually collected in xpt::OclMigrationProbelms.qvto
Comment 10 Ed Willink CLA 2012-05-29 13:22:56 EDT
Closing all bugs resolved in Indigo.