search

eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

[evaluator] invalid not properly assigned to let-variables #681

Closed eclipse-ocl-bot closed 1 month ago

eclipse-ocl-bot commented 1 month ago

| --- | --- | | Bugzilla Link | 342644 | | Status | CLOSED FIXED | | Importance | P3 normal | | Reported | Apr 12, 2011 17:37 EDT | | Modified | May 27, 2011 03:13 EDT | | Version | 3.1.0 | | Depends on | 342561 | | Blocks | 344391 | | Reporter | Axel Uhl |

Description

In the expression

let a:Integer='123a'.toInteger() in a.oclIsInvalid()

invalid results instead of the expected true.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 12, 2011 17:54

Depends on the patch https://bugs.eclipse.org/bugs/attachment.cgi?id=193060 for bug 342561 which introduces the computeSourceTurningExceptionsIntoInvalid method in EvaluationVisitorImpl which should be re-used by the fix forthis bug.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 12, 2011 17:57

Created attachment 193101 (attachment deleted)\ Incremental patch containing bug fix

Apply https://bugs.eclipse.org/bugs/attachment.cgi?id=193060 first which introduces EvaluationVisitorImpl.computeSourceTurningExceptionsIntoInvalid used by this patch.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 12, 2011 18:19

Created attachment 193102 (attachment deleted)\ Cumulative patch

Added tests. Cumulative patch, including the changes for https://bugs.eclipse.org/bugs/show_bug.cgi?id=342561 as well because the new method in EvaluationVisitorImpl is required.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 13, 2011 05:06

Created attachment 193134 (attachment deleted)\ Cumulative patch including Impact Analyzer adjustments

This one includes the adjustments to the partial evaluator for forwarding the ValueNotFoundException properly

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 13, 2011 12:11

Created attachment 193169 (attachment deleted)\ Updated to also handle operation call arguments

Added test for passing an indirectly-obtained invalid to an operation and perform an oclIsInvalid() evaluation there. Same simple solution as for the other cases. Renamed method that does the exception encapsulation to indicate broader applicability.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 13, 2011 12:12

Bug 342561 has been marked as a duplicate of this bug.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 14, 2011 13:41

There are 39 xxx.accept(getVisitor()) calls in EvaluationVisitorImpl. It looks like they all deserve the same treatment.

A more compact name might be safeVisitExpression(...). I think it belongs in AbstractEvaluationVisitor to accompany the similar code for visitExpression.

I'm not clear why RuntimeException is trapped. Surely Exception is safer, even if nominally impossible?

Hitting all 39 accepts may make a significant inroad on 'invalid' inconsistency. It is probably a good idea for safeVisitExpression to not NPE on a Java-null input, and to convert all Java-null returns to invalid as well.

The similar code in visitExpression needs alignment. Should all suppressed exceptions be logged? Is visitExpression then identical to safeVisit? No they're deliberately different. It's only at the outer call that logging occurs, internally anything might be handled by oclIsInvalid or oclIsUndefined or 'false and' or 'true or' and so should not trouble the log file.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 14, 2011 13:55

RuntimeException currently needs to be caught, e.g. because the toInteger and toReal implementations throw them if presented with a string that doesn't properly convert.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 15, 2011 03:42

I'll add tests for the if/then/else and Boolean and/or special cases regarding "invalid" handling and check if more accept(getVisitor())-calls need treatment. According to a proposal by Ed, I'll also rename evalRutningExceptionsIntoInvalid to safeVisitExpression.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 15, 2011 03:46

In visitIfExp currently the code looks like this:

    OCLExpression<C> condition = ie.getCondition();

    // evaluate condition\
    Boolean condVal = (Boolean) condition.accept(getVisitor());\
    if (condVal == null) {\
        return null;\
    }

    if (condVal.booleanValue()) {\
        return ie.getThenExpression().accept(getVisitor());\
    }\
    return ie.getElseExpression().accept(getVisitor());

This is consistent with the spec regarding not propagating invalid from the unused branch. However, I'm puzzled by the "return null" in case the condVal evaluates to null. I read the spec such that if the condition evaluates to anything but true or false, the result of the IfExp is invalid. Comments?

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 15, 2011 05:17

(In reply to comment #8)

RuntimeException currently needs to be caught, e.g. because the toInteger and toReal implementations throw them if presented with a string that doesn't properly convert.

I'm not criticising the catch of RuntimeException, which is needed for e.g. NumberFormatException. I'm suggesting to broaden it to Exception in case any of these get through.

(In reply to comment #10)

I'm puzzled by the "return null" in case the condVal evaluates to null. I read the spec such that if the condition evaluates to anything but true or false, the result of the IfExp is invalid. Comments?

The code is consistent with Java-null and Java-exception and getInvalid() all being equivalent to invalid. Hence my suggestion that visitSafeExpression convert Java-null to getInvalid() too.

However there is code that tests for null, so I regard changing all 39 accepts and adjusting nulls as a bit of an experiment. It should make no difference, but I guesstimate that it will provoke about 10 test failures, of which we may decide that 7 should have a changed test verdict and 3 require if-null fixes.

If it is possible to do this, further evolution can proceed on the basis that Java-null should be replaced by getInvalid(). If not, the indeterminate return semantics will be a permananent API concern.

You may want to revisit Bug 287977 and the evolution of Laurent's tests to those in the Pivot test suite; e.g./org/eclipse/ocl/examples/pivot/tests/EvaluateBooleanOperationsTest.java

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 15, 2011 05:45

(In reply to comment #11)

(In reply to comment #10)

I'm puzzled by the "return null" in case the condVal evaluates to null. I read the spec such that if the condition evaluates to anything but true or false, the result of the IfExp is invalid. Comments?

The code is consistent with Java-null and Java-exception and getInvalid() all being equivalent to invalid. Hence my suggestion that visitSafeExpression convert Java-null to getInvalid() too.

isUndefined(val) checks for either null or invalid. The implementation of oclIsInvalid() currently does a ==getInvalid(). Therefore, null is not considered invalid which is consistent with my understanding of spec and impl. My interpretation of the spec is such that if the condition of an IfExp evaluates to anything but true or false, the IfExp has to evaluate to invalid. Therefore,

(if null then x else y endif).oclIsInvalid()

should return true which currently it doesn't. That's why I'm proposing to change the code such that a null condition value results in an invalid IfExp value and not null.

However there is code that tests for null, so I regard changing all 39 accepts and adjusting nulls as a bit of an experiment. It should make no difference, but I guesstimate that it will provoke about 10 test failures, of which we may decide that 7 should have a changed test verdict and 3 require if-null fixes.

I don't think we need to adjust all 39 accepts, although nothing seems to speak against it. I tried to systematically list the cases where conversion of exceptions to invalid is necessary. Passing on the exception as another way of signaling invalid is okay as long as no assignment to a variable/parameter/accumulator takes place and the value is not subject to an oclIsInvalid() or oclIsUndefined() call. That's why I think the places where I added the interception are sufficient. Let me know if you'd still like to wrap all other accept(...) calls as well. It'll just cause more effort for the review ;-)

If it is possible to do this, further evolution can proceed on the basis that Java-null should be replaced by getInvalid(). If not, the indeterminate return semantics will be a permananent API concern.

See above, I don't think this is correct. Java-null encodes undefined, not invalid. Therefore, it must not be replaced.

You may want to revisit Bug 287977 and the evolution of Laurent's tests to those in the Pivot test suite; e.g./org/eclipse/ocl/examples/pivot/tests/EvaluateBooleanOperationsTest.java

Do you know of any tests for the Ecore code base regarding the behavior of the Boolean operations in the face of invalid arguments?

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 15, 2011 05:54

(In reply to comment #12)

You may want to revisit Bug 287977 and the evolution of Laurent's tests to those in the Pivot test suite; e.g./org/eclipse/ocl/examples/pivot/tests/EvaluateBooleanOperationsTest.java

Where can I find those? I can't see an org.eclipse.ocl.examples.pivot.tests project in CVS under tests/.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 15, 2011 05:58

(In reply to comment #11)

You may want to revisit Bug 287977 and the evolution of Laurent's tests to those in the Pivot test suite; e.g./org/eclipse/ocl/examples/pivot/tests/EvaluateBooleanOperationsTest.java

Why, at the time, weren't Laurent's tests added to the o.e.o.e.tests bundle where applicable? It seems that even as a separate failing test suite at least they could serve as documentation of the flaws that still exist and could guide us in reasonable improvements that can be applied to the existing code base with reasonable cost/effort.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 15, 2011 06:02

Created attachment 193340 (attachment deleted)\ Adjusted invalid-behavior for IfExp as well

Moved saveVisitExpression operation up to abstract visitor; fixed IfExp behavior for null condition value and added corresponding tests.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 15, 2011 07:08

(In reply to comment #13)

(In reply to comment #12)

You may want to revisit Bug 287977 and the evolution of Laurent's tests to those in the Pivot test suite; e.g./org/eclipse/ocl/examples/pivot/tests/EvaluateBooleanOperationsTest.java

Where can I find those? I can't see an org.eclipse.ocl.examples.pivot.tests project in CVS under tests/.

There is currently only one Pivot+Xtext tests plugin - awaiting tidying up

Try: org.eclipse.mdt/org.eclipse.ocl/tests/org.eclipse.ocl.examples.xtext.tests

(In reply to comment #14)

(In reply to comment #11)

You may want to revisit Bug 287977 and the evolution of Laurent's tests to those in the Pivot test suite; e.g./org/eclipse/ocl/examples/pivot/tests/EvaluateBooleanOperationsTest.java

Why, at the time, weren't Laurent's tests added to the o.e.o.e.tests bundle where applicable? It seems that even as a separate failing test suite at least they could serve as documentation of the flaws that still exist and could guide us in reasonable improvements that can be applied to the existing code base with reasonable cost/effort.

IIRC perhaps 75% of the problems Laurent identified were not problems or were very arguable in the light of specification ambiguities. They only become bugs once the specification clarifies; until then traditional behaviour throws in a conflicting inertia.

Some of the problems were due to distributed irregularities. Each new problem revealed further issues. Hence the motivation for localization of behaviour in a library model.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 18, 2011 03:27

Learning from https://bugs.eclipse.org/bugs/show_bug.cgi?id=287977, I'm walking\ through all of Laurent's test cases, seeing how much of the failures can be\ fixed at reasonable cost in the Ecore implementation without a full re-write.\ His test suite is invaluable as it has indicated a large number of fairly easy\ to fix issues.

So far I'm down to 58 remaining failures in the Numeric operations Tests,\ OclAny operations Tests and String operations Tests. From the others there were\ two categories of issues that I haven't found ways to fix in the current\ "mature" implementation with reasonable cost:

1) Operations on collection types not resolved by analyzer due to collection\ type conformance detection problems. Example "Bag{4, 5, 'test'} <> Set{4,\ 'test', 5, 4}" reports an error because the <>(Set(OclAny)) is said not to be\ found on Bag(OclAny). I think this is a bug, maybe deserving a separate\ bugzilla.

2) Issues with numeric types due to inconsistent handling between Java and OCL\ semantics. Ed observed this several times before. The key problem is that the\ java.lang.Number subtypes have an equals(...) implementation that is\ inconsistent with ObjectUtil.equals that implements the OCL semantics for\ numeric types. The regular Java collections used to hold values of numeric\ types resort to the equals-implementation of the java.lang.Number subtypes.\ Wrapping those numeric values with objects whose equals implementation relies\ on the ObjectUtil.equal definition (and that provides a consistent hashCode as\ well) would be necessary. This is probably what the Pivot evaluator implements\ already. Again, this may deserve a separate bugzilla.

I'll work on resolving the remaining issues, providing a batched-up patch,\ hoping that's in-line with what Ed asked for, trying to minimize review effort.\ The two additional bugzillas IMHO should at least result in documentation that\ clearly describes these shortcomings / limitations / incompatibilities.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 18, 2011 04:00

I have a question regarding Laurent's tests and their claims regarding invalid in conjunction with the comparison operations "=" and "<>". In a number of test cases such as testCollectionEqualInvalid and testBooleanEqualInvalid, the tests try to assert that a comparison with invalid should be possible and should evaluate to a valid boolean result. For example, testBooleanEqualInvalid tests that "invalid=true" evaluates to false.

However, testNumberEqualInvalid asserts that "invalid=0" evaluate to invalid.

Here's my take: I have not found any part of the spec so far from which I could infer that a comparison with invalid on either side would evaluate to anything but invalid. I'll also shoot Laurent an e-mail trying to clarify. If I don't receive any objections I'll adjust these tests such that comparison with invalid will always yield invalid, no matter on which side of the comparison operator the invalid appears.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 18, 2011 04:05

(In reply to comment #17)

Learning from https://bugs.eclipse.org/bugs/show_bug.cgi?id=287977, I'm walking through all of Laurent's test cases, seeing how much of the failures can be fixed at reasonable cost in the Ecore implementation without a full re-write. His test suite is invaluable as it has indicated a large number of fairly easy to fix issues.

Yes they're good. But use the evolved Pivot version that is more readable, adds some more tests, and has 'consistent' expectations. To avoid clutter, please use a new Bugzilla to debate ambiguous expectations.

So far I'm down to 58 remaining failures in the Numeric operations Tests, OclAny operations Tests and String operations Tests. From the others there were two categories of issues that I haven't found ways to fix in the current "mature" implementation with reasonable cost:

1) Operations on collection types not resolved by analyzer due to collection type conformance detection problems. Example "Bag{4, 5, 'test'} <> Set{4, 'test', 5, 4}" reports an error because the <>(Set(OclAny)) is said not to be found on Bag(OclAny). I think this is a bug, maybe deserving a separate bugzilla.

You probably need to review the bugs I resolved for M5 and M6; fixed in the Pivot but outstanding in the old core. I think it best to reopen these and use an [old] prefix for bugs only in the old code base.

Fixing the evaluator should be much simpler than the analyzer, where the distributed library model is in many places. However the evaluator does 'oclAsSet' at run-time and has other run-time conformsTo checks that should be at compile-time.\ \

2) Issues with numeric types due to inconsistent handling between Java and OCL semantics. Ed observed this several times before. The key problem is that the java.lang.Number subtypes have an equals(...) implementation that is inconsistent with ObjectUtil.equals that implements the OCL semantics for numeric types. The regular Java collections used to hold values of numeric types resort to the equals-implementation of the java.lang.Number subtypes. Wrapping those numeric values with objects whose equals implementation relies on the ObjectUtil.equal definition (and that provides a consistent hashCode as well) would be necessary. This is probably what the Pivot evaluator implements already. Again, this may deserve a separate bugzilla.

See Section 4.2 of my 'Aligning OCL with UML' paper, which extols the necessity/virtues of Primitive, Collection and Object Value polymorphism. Eliminating ObjectUtil and CollectionUtil made things a lot simpler and will make code generation simpler too.

Currently the Pivot wraps BigInteger as IntegerValueImpl, giving a double wrapping of Java primitives. I plan to change this to BigIntegerValueImpl and add IntegerValueImpl as a direct wrapped for int bypassing Integer and so recovering speed. I'm still pondering on a fast extensible double dispatch that allows user-definition of e.g. MyShortValueImpl to drop in automatically without any need to modify existing classes.

I'll work on resolving the remaining issues, providing a batched-up patch, hoping that's in-line with what Ed asked for, trying to minimize review effort.

You may find that opportunities that Value polymorphism offers for regularity and optimisation justifies supporting the migration to at least the pivot evaluator. Compare the implementation of iterations in the two approaches.

The two additional bugzillas IMHO should at least result in documentation that clearly describes these shortcomings / limitations / incompatibilities.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 19, 2011 11:44

Created attachment 193597 (attachment deleted)\ Fix many more issues pointed out by Laurent's tests

In particular, behavior for invalid / null is fixed, as well as some issues of UnlimitedNatural comparison. The patch includes a fix to BagImpl which had issues with its iterator in case null values were contained which is permissible. The Set{null} special case should be considered properly now. null arguments to binary and ternary stdlib operations can safely be passed now. Fixed empty set comparison. Fixed Integer to Double coercion where necessary. Fixed null/OclVoid in tuples. Forward invalid from arg in case it's not the argument of a Boolean operation. Fixed String::substring and other String:: implementations which needs to result in invalid in certain cases according to the spec. Handle oclIsTypeOf/oclIsKindOf properly for UnlimitedNatural. Let - evaluate to invalid. Look up Integer operations if no matching operation is found on UnlimitedNatural. Fixed UnlimitedNatural comparison operator implementations. Adjusted UML tests to fixes according to OCL 2.3 spec.

Remaining open issues from Laurent's original test suite that I think are harder to fix than can be addressed by this bugzilla:

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 20, 2011 04:25

Created attachment 193663 (attachment deleted)\ Additionally fixes the collection operation lookup problem

In addition to attachment 193597 (attachment deleted), this one now also fixes the issues with collection operation lookup including = and <> and ensuring that for collection equality the collection kind is considered, and fixes the semantics of OrderedSet::flatten, making it return a Set always. I chose this in the face of OCL 2.3 (OMG 10-11-42) section A.2.5.8 leaving it unspecified how an OrderedSet is flattened and because flattening with duplicates would distort the original element ordering. With this I was able to uncomment all tests that so far failed because the respective collection operations were not found.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 20, 2011 08:59

Created attachment 193691 (attachment deleted)\ Additionally fixes upperBound/UnlimitedNatural issues

There was another issue with the implicit conversion taking place for EStructuralFeature.upperBound from EInt to OCL UnlimitedNatural. UnlimitedNatural didn't have an instance class name set in the oclstdlib.ecore model which I added (java.lang.Integer). This avoids trouble when an Integer object is checked for runtime compatibility with UnlimitedNatural which in turn caused comparisons of upperBound values to an IntegerLiteral to fail.

Afterwards, another issue surfaced regarding sortedBy implementation for UnlimitedNatural. It sorted the -1 value as lesser than 0 instead of greater than everything. Fixed, testcase added.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 20, 2011 09:09

Don't forget that there are four 'identical' versions of oclstdlib.

oclstdlib.ecore, oclstdlib.uml, and their hand coded OCLStandardLibrary initializations.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 20, 2011 09:17

(In reply to comment #23)

Don't forget that there are four 'identical' versions of oclstdlib.

oclstdlib.ecore, oclstdlib.uml, and their hand coded OCLStandardLibrary initializations.

Thanks for the hint. I did the OCLStandardLibrary initialization

    OCL_UNLIMITED_NATURAL.setInstanceClass(Integer.class);

in org.eclipse.ocl.ecore.internal.OCLStandardLibraryImpl. There doesn't seem to be a need for adjustments in the UML package. I can't find any reference being made to an instance class there.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 20, 2011 11:01

Created attachment 193709 (attachment deleted)\ OrderedSet flattens as OrderedSet

Compared to the obsoleted patch 193691, this one reverts the flattening of OrderedSet back to OrderedSet (instead of Set). It is unspecified in OCL 2.3 A.2.5.8 and we're free to choose. OrderedSet produces more predictable results and may cause less confusion about random element re-ordering as would be the case with a Set.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 20, 2011 19:09

Created attachment 193768 (attachment deleted)\ Added missing =, <> in oclstdlib.ecore

The plugin tests failed because there, the stdlib model is not produced programmatically but loaded from oclstdlib.ecore where the = and <> operations were not yet modeled correctly on Collection(T). In addition, parameter conformance checking for OclAny args didn't work in case distinct OclAny instances existed.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 27, 2011 03:57

First comments:

2 tests fail; testStringToLower, testStringToUpper. Both involving non-ASCII characters. The files provided by the patch are not UTF-8 encoded. See Bug 291310 for details on this nightmare. Since OCL 2.3 introduces \uxxxx and does not support multi-lingual quotes, you can probably follow Bug 291310 and remove the tests.

Please use the AbstractTestSuite so that tearDown cleans up the non-disposed ocl, and so that you can use checkUTFEncoding().

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 27, 2011 04:37

(In reply to comment #27)

First comments:

2 tests fail; testStringToLower, testStringToUpper. Both involving non-ASCII characters. The files provided by the patch are not UTF-8 encoded. See Bug 291310 for details on this nightmare. Since OCL 2.3 introduces \uxxxx and does not support multi-lingual quotes, you can probably follow Bug 1310 and remove the tests.

That's a nasty problem with patch creation. I need to find out how to male the utf characters survive. I'll re-create the patch and check it has the proper utf chars in it.

Please use the AbstractTestSuite so that tearDown cleans up the non-disposed ocl, and so that you can use checkUTFEncodi

Will take a look later today.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 27, 2011 11:02

(In reply to comment #28)

(In reply to comment #27)

First comments:

2 tests fail; testStringToLower, testStringToUpper. Both involving non-ASCII characters. The files provided by the patch are not UTF-8 encoded. See Bug 291310 for details on this nightmare. Since OCL 2.3 introduces \uxxxx and does not support multi-lingual quotes, you can probably follow Bug 1310 and remove the tests.

That's a nasty problem with patch creation. I need to find out how to male the utf characters survive. I'll re-create the patch and check it has the proper utf chars in it.

I've checked, and the patch contains the proper Unicode characters, as it seems. Browser and Emacs correctly display them. Only when I say "Apply Patch" in Eclipse will the Unicode characters get messed up. Seems to be a problem with the team provider implementation that is non Unicode-resistent. I wonder what happens upon commit / update. If that ain't a problem, we could handle this by copy/paste from the patch for the two locations affected for now, and I commit them in the proper Unicode format.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 27, 2011 11:06

(In reply to comment #29)\ I've tried committing and reverting a unicode change in Revalidator.java (r1.5, 1.6, 1.7). Works just fine. So it just seems to be an issue of "apply patch." Can you live with manually patching the four offending lines from the original patch?

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 27, 2011 11:22

(In reply to comment #27)

Please use the AbstractTestSuite so that tearDown cleans up the non-disposed ocl, and so that you can use checkUTFEncoding().

Which of the new tests do not inherit from AbstractTestSuite? The Evaluation*OperationTest suites each inherit from AbstractEvaluationTest which extends AbstractTestSuite, so their tearDown is executed. Any other tests that need to inherit from AbstractTestSuite and so far don't?

Using checkForUTF8Encoding can replace the initial check in testStringToLower and testStringToUpper. However, it doesn't help in replacing the final assertResult where a heavy Unicode-reliant string is provided. I really propose we handle it such that you replace the offending lines directly from the patch as displayed in the browser after using Eclipse to apply the patch in general, then ensure you're happy with it, and if you +1 the thing, I ensure that upon commit the Unicode characters end up in CVS.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 27, 2011 12:10

Created attachment 194178 (attachment deleted)\ Alternative GIT patch to avoid Unicode hassle

Apply by using

patch -p2 342644.patch

in your workspace directory. Same contents as 193768, but this way applying without the distorted Unicode characters may be easier.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 27, 2011 12:29

(In reply to comment #31)

Which of the new tests do not inherit from AbstractTestSuite? The Evaluation*OperationTest suites each inherit from AbstractEvaluationTest which extends AbstractTestSuite, so their tearDown is executed. Any other tests that need to inherit from AbstractTestSuite and so far don't?

Sorry. Got lost in the inheritance tree.\

Using checkForUTF8Encoding can replace the initial check in testStringToLower and testStringToUpper. However, it doesn't help in replacing the final assertResult where a heavy Unicode-reliant string is provided. I really propose we handle it such that you replace the offending lines directly from the patch as displayed in the browser after using Eclipse to apply the patch in general, then ensure you're happy with it, and if you +1 the thing, I ensure that upon commit the Unicode characters end up in CVS.

Doing the commit correctly by hand is not a problem. The problem is that Bug 291310 is still open, so that while use of UTF-8 for test projects fixed all interactive usage, Alex failed to persuade the Hudson compiler to use the UTF-8 encoding argument. We will not get test passes from Unicode tests. The original Unicode tests for inter-lingual quote matching were highly suspect in principle and without any foundation in the specification and so were removed. The new tests are very cosmetic and so we can comment them out at least until Bug 291310 is fixed.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 27, 2011 12:39

(In reply to comment #33)

Doing the commit correctly by hand is not a problem. The problem is that Bug 291310 is still open, so that while use of UTF-8 for test projects fixed all interactive usage, Alex failed to persuade the Hudson compiler to use the UTF-8 encoding argument. We will not get test passes from Unicode tests. The original Unicode tests for inter-lingual quote matching were highly suspect in principle and without any foundation in the specification and so were removed. The new tests are very cosmetic and so we can comment them out at least until Bug 291310 is fixed.

Ah, good point; hadn't thought about Hudson. Will comment out the two offending tests and produce a new patch.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 27, 2011 14:13

Created attachment 194189 (attachment deleted)\ Commented unicode-dependent tests (until Hudson copes)

Due to Eclipse/Linux/Windows encoding issues the unicode UTF-8 encoded characters may already be messed up in this patch.

Using checkForUTF8Encoding() now as proposed by Ed.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 28, 2011 15:38

I paid particular attention to the JUnit tests. Any existing test for wh`ich the verdict changes needs considerable care. There is a change for oclIsTypeOf(OclIsInvalid) that I do not consider correct that affects quite a few tests.\ ---------------------------\ testStringToUpper: 'SS' is still a Unicode problem and failure from the patch.\ ---------------------------\ uml EvaluationHaltedTest; I'm not clear what the purpose of the change is\ ---------------------------\ org.eclipse.ocl.uml.tests.RegressionTest.test_oclInvalid\ \ was inv: invalid.oclIsTypeOf(OclInvalid)\ now inv: invalid.oclIsTypeOf(OclInvalid).oclIsInvalid()\ \ No. inv: invalid.oclIsTypeOf(OclInvalid).oclIsInvalid() is true, so invalid.oclIsTypeOf(OclInvalid).oclIsInvalid() is false.\ \ ditto: org.eclipse.ocl.ecore.tests.RegressionTest.test_oclInvalid

ditto: org.eclipse.ocl.ecore.tests.BasicOCLTest.test_laxNullHandling_OclInvalid

ditto: org.eclipse.ocl.ecore.tests.BasicOCLTest.test_OclInvalid_typeConformance_191041

Remove the extra code in EvaluationVisitorImpl\ ---------------------------\ org.eclipse.ocl.tests.GenericIteratorsTest\ \ sole change is to introduce an unneeded @SuppressWarnings("unchecked")

---------------------------\ org.eclipse.ocl.ecore.tests.RegressionTest.test_bagIterationWithNullOccurrences()

This is difficult to read: contrast with org.eclipse.ocl.ecore.tests.EvaluationCollectionOperationTest.testCollectionExcludingNullValue which has more powerful helper functions.

This use of null surprised me, but it makes clear that the Mature Evaluator API should be documented as Java-null == OCL-null, Java-exception == OCL-invalid with optional real objects for LHS usage.

---------------------------\ org.eclipse.ocl.ecore.tests.CollectionsTest, org.eclipse.ocl.uml.tests.CollectionsTest

test_flatten_notNested, test_flatten_emptySource_195252

the sibling tests are testing that the expected result is obtained, so you should rt5est for the truth of an OrderedSet result, not the falsity of a Set result; that is a different test.

---------------------------\ oclstdlib.ecore

Addition of Collection::=, <> is suspect, in so far as the covariant overload has undefined semantics in OCL 2.3. It is likely to be eliminated in OCL 2.4, so since they're currently not in I'm inclined to leave them out. Espexcially suince oclstdlib.uml has no equivalents.

Addition of UnlimitedNatural instanceTypeName. Is this necessary. OclInvalid, OclVoid, Real still have no instanceTypeName. It's not even accurate, the instance is variously Integer, Long amd BigInteger.

---------------------------\ CollectionUtil.union

No. e.g. A union o SEquences is a Sequence. See the pivot's AbstractCollectionValue.union for more plausible logic.

Pivot derives OrderedSetImpl from LinkedHashSet in order to fix equals() for ordering.

---------------------------\ BagImpl

A variety of non-use of OCL-value semantics issues remain.

---------------------------\ AbstractTypeChecker ??? very hard to comment; probably better but ... this is one of the nightmare areas. I see very little improvement to the analyzer so I'm a bit concerned about making just these possibly useful changes.

---------------------------\ AbstractEvaluatiorVisitor.oclIsKindOf needs updating to match oclIsTypeOf

---------------------------\ AbstractEvaluatiorVisitor.saveVisitExpression; use safeVisitExpression.

---------------------------\ EvaluationVisitorImpl.UNLIMITED duplicates UnlimitedNaturalLiteralExp.UNLIMITED use the old value.

---------------------------\ EvaluationVisitorImpl.visitOperationCallExp:PredefinedType.OCL_IS_TYPE_OF\ new getInvalid() test is wrong.\ UnlimitedNatural has no subtypes comment is wrong.

---------------------------\ EvaluationVisitorImpl.visitOperationCallExp:PredefinedType.OCL_IS_KIND_OF\ new getInvalid() test is wrong. \ UnlimitedNatural has no subtypes comment and functionality is wrong; OclVoid, OclInvalid are subtypes

---------------------------\ EvaluationVisitorImpl.visitOperationCallExp:PredefinedType.OCL_AS_TYPE\ UNLIMITED is an invalid Integer or Real! there is no PLUS_INFINITY (yet; I would like to add PLUS_INFINITY so that we can lose UnlimitedNatural altogether).\ the value conformance check for Real to Integer and Integer to UnlimitedNatural is missing\ conversions for BigInteger and BigDecimal are missing.

---------------------------\ EvaluationVisitorImpl.visitOperationCallExp:isBooleanOperation(opCode)\ the second use seems counterproductive; another list of options and no check that source is boolean.

---------------------------\ EvaluationVisitorImpl.visitOperationCallExp:OR etc\ Better. But can still malfunction for an e.g. Integer argument.

---------------------------\ EvaluationVisitorImpl.visitOperationCallExp:binary String\ No. This now CCEs for a Boolean argument.

---------------------------\ EvaluationVisitorImpl.visitOperationCallExp:INDEX_OF\ Yes. [A pity API compatibility prevents fixing CollectionUtil where the bug is.]\ Worth a comment in CollectionUtil.indexOf that the null is indicating invalid.

---------------------------\ EvaluationVisitorImpl.visitOperationCallExp:ternary\ now fails for OCL-null arguments; restore undefined tests

---------------------------\ EvaluationVisitorImpl.visitIfExp\ will malfunction if accept returns getInvalid().

---------------------------\ EvaluationVisitorImpl.visitCollectionLiteralExp\ change for invalid probably ok; iff accept API is sound\ change for unit nulls is wrong. Set{null} is what it says. 11.2.3 and A.2.5.3 talk primarily about use of null in a collection context not use of null a collection.

---------------------------\ EvaluationVisitorImpl API\ there are three return nulls. Only that for visitNullIteralExp is API consistent.\ Each of the oclIsKindOf and oclIsTypeOf has a 'invalid' null return.\ oclIsKindOf is called 4 times, 3 times with the null propagating erroneously.\ oclIsTypeOf is called 2 times, 2 times with the null propagating erroneously.

This API is not documented.

---------------------------\ Copyrights: should be 2011 or 2007,2011. If editing, where there are multiple copyright holders, keep just the first with "and others" for the rest.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 29, 2011 02:59

Created attachment 194330 Evaluation tests migrated (unchanged) to Pivot model

I've remigrated the Evaluation tests to the pivot model as a quick check on evolved test expectations. Migration attached.

In the following, the first migrated sub-test that fails is quoted. I therefore consider the test to be wrong, and the code that causes the test to pass to be wrong as well.

Collections do not use OCL equals semantics

    assertQueryTrue(null, "Sequence{3, 4.0, 'test'}->excludes(3.0)");

    assertQueryTrue(null, "Sequence{3, 4.0, 'test'}->excludesAll(Sequence{3.0, 4, 'TEST'})");

    assertQueryTrue(null, "Set{null}->excludes(null)");

    assertQueryFalse(null, "Sequence{3, 4.0, 'test'}->includes(3.0)");

    assertQueryFalse(null, "Sequence{3, 4.0, 'test'}->includesAll(Sequence{3, 4, 'test'})");

    assertQueryFalse(null, "Set{null}->includes(null)");

    assertQueryTrue(null, "Set{null}->isEmpty()");

    assertQueryFalse(null, "Set{null}->notEmpty()");

Incorrect validity semantics

    assertQueryEquals(null, null, "Sequence{}->first()");

    assertQueryEquals(null, null, "Sequence{}->last()");

Inconsistent OrderedSet types

    assertQueryEquals(null, valueFactory.getEmptySetValue(),
        "OrderedSet{}->flatten()");

    assertQueryResults(null, "Set{'b'}", "OrderedSet{'a', 'b', 'c'} - Set{'c', 'a'}");

Numeric precision

The following tests are unsound; the pivot version has an epsilon tolerance.

    assertQueryEquals(null, Double.valueOf(1.11d / 1.12d), "1.11 / 1.12");

    assertQueryEquals(null, Double.valueOf(1.11d * 1.12d), "1.11 * 1.12");

Numeric conformance

    assertQueryInvalid(null, "(3.0).oclAsType(Integer)");   -- 3.0 is convertible without error to 3

    assertQueryTrue(null, "3.oclIsTypeOf(Integer)");        -- 3 is an UnlimitedNatural not an Integer

Unlimited conformance

    assertQueryInvalid(null, "1.max(*)");

    assertQueryInvalid(null, "1.min(*)");

    assertQueryEquals(null, UnlimitedNaturalLiteralExp.UNLIMITED,
        "*.oclAsType(Integer)");

Pivot discrepancy 1: tuples

The following

    assertQueryResults(null, "Set{Tuple{first = null, second = 3}, Tuple{first = 4, second = 3}}", "Sequence{null, 4}->product(Sequence{3})");

gives the correct result for the pivot but is reported as a failure. I suspect some anomally on Tuple hashes/types; one of a pivot evaluator or test harness bug.

Pivot discrepancy 2: Ecore visibility

Types such as EClass are not visible in the pivot model so these tests differ.

Pivot discrepancy 3: non-lax nulls

Pivot has no counterpart to non-lax null handling.

Pivot discrepancy 4: Debatable invalidity conformance

For the following tests the Pivot analyzer detects static failures. The exact meaning of conformance for 'invalid' and 'null' is unclear.

In a strict subtyping sense if invalid conforms to all types, the expression is valid. However operations on invalid are not valid. So arguably invalid conforms to all types in so far as an 'invalid' value may be assigned/returned to any type, but invalid does not conform to any type in so far as it does not usefully inherit features. THe obnly features of OclVoid and OclInvalid should be those that explicitly enumerated with correspondingly explicit semantics.

    assertQueryInvalid(null, "let s : Set(Integer) = invalid in Set{4}->intersection(s)");

    assertQueryInvalid(null, "let s : Set(Integer) = null in Set{4}->intersection(s)");

    assertQueryInvalid(null, "invalid > invalid");  -- many more variants

    assertQueryInvalid(null, "null >= null");   -- many more variants

The mature code is not wrong here; just identifying the compile-time/run-time difference.

:notepad_spiral: 342644.patch

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 29, 2011 04:28

(In reply to comment #37)\ Was it on purpose that you obsoleted the non-git patch for the ecore tests? If so, why?

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 29, 2011 05:40

(In reply to comment #38)

(In reply to comment #37) Was it on purpose that you obsoleted the non-git patch for the ecore tests? If so, why?

I didn't. You obsoleted it as part of comment #35.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 29, 2011 05:56

(In reply to comment #37)

Pivot discrepancy 1: tuples

The following

    assertQueryResults(null, "Set{Tuple{first = null, second = 3},

Tuple{first = 4, second = 3}}", "Sequence{null, 4}->product(Sequence{3})");

gives the correct result for the pivot but is reported as a failure. I suspect some anomally on Tuple hashes/types; one of a pivot evaluator or test harness bug.

The problem is in the test which should be written:

    assertQueryResults(null, "let n : UnlimitedNatural = null in Set{Tuple{first = n, second = 3}, Tuple{first = 4, second = 3}}", "Sequence{null, 4}->product(Sequence{3})");\
    assertQueryResults(null, "let n : UnlimitedNatural = null in Set{Tuple{first = n, second = 3}, Tuple{first = 4, second = 3}}", "Bag{null, 4}->product(Sequence{3})");\
    assertQueryResults(null, "let n : UnlimitedNatural = null in Set{Tuple{first = n, second = 3}, Tuple{first = 4, second = 3}}", "Set{null, 4}->product(Sequence{3})");\
    assertQueryResults(null, "let n : UnlimitedNatural = null in Set{Tuple{first = n, second = 3}, Tuple{first = 4, second = 3}}", "OrderedSet{null, 4}->product(Sequence{3})");

to ensure that the null in the expec6ed value has UnlimitedNatural rather than OclVoid type.

The mature test harness is perhaps not testing for equality precisely enough to distinguish the alternate results.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 29, 2011 07:34

Created attachment 194348 Migrated pivot tests

The attached hack enables the evolved tests from the pivot suite to be run on the Ecore code.

I'm afraid this identifies a number of new bugs and missing OCL 2.2 operations.

You need to decide what to implement and what to document as unimplemented.

[I deleted the oclType() tests since clearly the mature code will not emulate the pivot reflection.]

:notepad_spiral: Pivot342644.patch

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 29, 2011 11:42

(In reply to comment #36)

Thanks for the thorough review. Let me try to tackle one point after the other\ for those that I understand (please take a look at the questions I insert\ in-line):

I paid particular attention to the JUnit tests. Any existing test for wh`ich the verdict changes needs considerable care. There is a change for oclIsTypeOf(OclIsInvalid) that I do not consider correct that affects quite a few tests.

I agree that changed test verdicts need a very close look. I tried to insert\ comments where this happened to explain the expected results.

testStringToUpper: 'SS' is still a Unicode problem and failure from the patch.

Commented now.

uml EvaluationHaltedTest; I'm not clear what the purpose of the change is

The way testHaltedQuery evaluated the queryExp, it passed null as self context.\ This now would lead to a Sequence containing as its single element the null\ value which is then bound to the i iterator during the collect and therefore an\ attempt to invoke halt on null which now fails fast with an invalid, as I think\ the spec says it should. Furthermore, the self argument to halt is null,\ causing InterruptibleEvalEnv.callOperation to fail on its\ assertEquals(HALT_KIND_NONE, kind). Therefore, for the specific test I felt I\ had to change the query the way I did.

org.eclipse.ocl.uml.tests.RegressionTest.test_oclInvalid

was inv: invalid.oclIsTypeOf(OclInvalid) now inv: invalid.oclIsTypeOf(OclInvalid).oclIsInvalid()

I can't read from the specification that oclIsTypeOf is among the exceptions\ that for a part of the expression that is invalid should not evaluate to\ invalid. Those exceptions AFAIK are limited to oclIsUndefined, oclIsInvalid and\ the boolean operator shortcut evaluations. Therefore, now correctly as I would\ have said, invalid.oclIsTypeOf(OclInvalid) evaluates to invalid. However, there\ are excerpts of the spec, such as 8.2.1, which make use of\ oclIsTypeOf(OclInvalid). The bewildering part though is that those constraints\ offend another constraint that says that OCL collections cannot contain\ invalid, so iterating over a colection such as OclInvalid.allInstances() would\ yield an invalid collection itself.

Let me know what you suggest. Again, I read from the spec that\ invalid.oclIsTypeOf(OclInvalid) has to evaluate to invalid because any\ operation call on invalid except for oclIsInvalid and oclIsUndefined and the\ boolean shortcuts are to evaluate to invalid.

No. inv: invalid.oclIsTypeOf(OclInvalid).oclIsInvalid() is true, so invalid.oclIsTypeOf(OclInvalid).oclIsInvalid() is false.

Sorry, I don't understand this paragraph.

ditto: org.eclipse.ocl.ecore.tests.RegressionTest.test_oclInvalid

See above, same thing for Ecore.

ditto: org.eclipse.ocl.ecore.tests.BasicOCLTest.test_laxNullHandling_OclInvalid

ditto: org.eclipse.ocl.ecore.tests.BasicOCLTest.test_OclInvalid_typeConformance_191041

ditto, ditto ;-)

Remove the extra code in EvaluationVisitorImpl

Which extra code do you mean?

org.eclipse.ocl.tests.GenericIteratorsTest

sole change is to introduce an unneeded @SuppressWarnings("unchecked")

It's required because otherwise Indigo with default Java settings complains\ about "Type safety : A generic array of PK is created for a varargs parameter."

org.eclipse.ocl.ecore.tests.RegressionTest.test_bagIterationWithNullOccurrences()

This is difficult to read: contrast with org.eclipse.ocl.ecore.tests.EvaluationCollectionOperationTest.testCollectionExcludingNullValue which has more powerful helper functions.

Please note that test_bagIterationWithNullOccurrences is not about any OCL\ expression parsing and evaluation. It is testing the raw BagImpl class with raw\ Java values because there was a respective bug in the implementation. I can't\ see how, e.g., assertExpressionResults from testCollectionExcludingNullValue\ would help here. Can you maybe provide more concrete hints as to how you think\ readability can be improved?

This use of null surprised me, but it makes clear that the Mature Evaluator API should be documented as Java-null == OCL-null, Java-exception == OCL-invalid with optional real objects for LHS usage.

org.eclipse.ocl.ecore.tests.CollectionsTest, org.eclipse.ocl.uml.tests.CollectionsTest

test_flatten_notNested, test_flatten_emptySource_195252

the sibling tests are testing that the expected result is obtained, so you should rt5est for the truth of an OrderedSet result, not the falsity of a Set result; that is a different test.

I was trying to preserve Laurent's test semantics as far as possible. I suggest\ I add the positive tests you mention.

oclstdlib.ecore

Addition of Collection::=, <> is suspect, in so far as the covariant overload has undefined semantics in OCL 2.3. It is likely to be eliminated in OCL 2.4, so since they're currently not in I'm inclined to leave them out. Espexcially suince oclstdlib.uml has no equivalents.

But that would make many correct and useful comparisons fail to parse. I think\ this would not be a good solution for our users. What semantics could the\ covariant overload have in your opinion, other than the one implemented?\ Conversely, if you think there is something missing in oclstdlib.uml, then I\ suggest we add it there. How and where is oclstdlib.uml used? I've now added\ the two operations to Collection Type Collection(T) in oclstdlib.uml.

Addition of UnlimitedNatural instanceTypeName. Is this necessary. OclInvalid, OclVoid, Real still have no instanceTypeName. It's not even accurate, the instance is variously Integer, Long amd BigInteger.

I though it was accurate from what I could see regarding the mature Ecore\ implementation as I haven't come across a non-Integer representation of an\ UnlimitedNatural. Where / when is an UnlimitedNatural instance represented by a\ Long or BigInteger in the mature implementation?

If the instanceTypeName is missing, UnlimitedNatural objects are not\ comparable. The UMLReflectionImpl.isComparable(EClassifier) implementation\ looks like this:

public boolean isComparable(EClassifier type) {\
    Class<?> javaClass = type.getInstanceClass();

    return (javaClass != null) &&\

Comparable.class.isAssignableFrom(javaClass);\ }

If no instance class is defined, objects are not comparable. Alternatively, the\ isComparable operation may be tweaked, but I expect trouble in other areas too,\ perhaps when the sorting is to be carried out because there again reference may\ be made to the instance class. Therefore I really prefer the current proposal\ that sets the instance class to java.lang.Integer, particularly if there is no\ evidence that BigInteger or Long may be used in the mature implementation\ anyhow.

CollectionUtil.union

No. e.g. A union o SEquences is a Sequence. See the pivot's AbstractCollectionValue.union for more plausible logic.

I claim the implementation proposed by the patch handles this correctly. The\ change affects the otherwise failing case (as the comment I introduced tries to\ explain) of one argument being a Bag. In this case, even if it's empty, the\ result has to be a Bag, no matter whether the Bag is the self or the operand.\ If two sequences are to be united, none is a bag, so createNewSequence(self) is\ executed as before. You could make the above clearer by providing a failing\ test case.

Pivot derives OrderedSetImpl from LinkedHashSet in order to fix equals() for ordering.

The mature Ecore impl uses LinkedHashSet as OrderedSet implementation. What are\ you getting at?

BagImpl

A variety of non-use of OCL-value semantics issues remain.

What are you suggesting? I thought we agreed that in particular the numerical\ aspects (3.0 vs. 3 and so on) would be too hard to fix with the resources given\ and with a potential perspective of making the Pivot evaluator work with less\ effort for Ecore in post-Indigo releases. No?

Besides: what exactly are you referring to by "non-use of OCL value semantics"\ in the concrete context of BagImpl?

AbstractTypeChecker ??? very hard to comment; probably better but ... this is one of the nightmare areas. I see very little improvement to the analyzer so I'm a bit concerned about making just these possibly useful changes.

So let me try to explain the changes in detail.

The changes in getRelationship are owed to the occasional existence of two\ distinct AnyType instances, one being distinct from stdlib.getOclAny().\ Therefore, I relaxed the identity comparisons into "instanceof AnyType"\ comparisons. Besides, the possibility of both types to compare being OclAny was\ not handled. So I had to add the SAME_TYPE branch.

The changes around line 196 relate to the conformance of UnlimitedNatural to\ Integer which so far was not acknowledged properly by getRelationship. So far,\ only the conformance of UnlimitedNatural to Real and Integer to Real was\ considered. This caused some of the new tests to fail and clearly contradicts\ the specification.

The changes to findOperationMatching starting in line 1093, as the comment\ tries to explain, for one thing ensure again that the UnlimitedNatural\ conformance to Integer is also considered appropriately during operation lookup\ because otherwise *.div(1) doesn't parse. The other change there relates to the\ lookup of collection operations. There are many of the new tests which\ otherwise fail because operations available on Collection are not re-modeled on\ Collection's subclasses. If you will, this is a partial "polymorphic lookup"\ that at least makes the stdlib behave as the spec and Laurent's tests suggests\ it should.

AbstractEvaluatiorVisitor.oclIsKindOf needs updating to match oclIsTypeOf

Done, thanks. Affected was the case null.oclIsKindOf(OclInvalid). Also adjusted\ EvaluationOclAnyOperationTest.testoclIsKindOfNullLaxNullHandling accordingly,\ making reference in a comment to the OCL 2.3 specification.

Apart from that, an operation call on invalid (in particular calls to\ oclIsKindOf and oclIsTypeOf) should (see above) evaluate to invalid. Exceptions\ are, as discussed above, oclIsUndefined and oclIsInvalid as well as the Boolean\ shortcuts.

AbstractEvaluatiorVisitor.saveVisitExpression; use safeVisitExpression.

Thanks. Typo. Fixed.

EvaluationVisitorImpl.UNLIMITED duplicates UnlimitedNaturalLiteralExp.UNLIMITED use the old value.

Not really. UnlimitedNaturalLiteralExp.UNLIMITED is type int. When using it for\ auto-boxing, a new java.lang.Integer(-1) will be constructed each time. I\ introduced EvaluationVisitorImpl.UNLIMITED to avoid this repetitive auto-boxing\ effort.

EvaluationVisitorImpl.visitOperationCallExp:PredefinedType.OCL_IS_TYPE_OF new getInvalid() test is wrong.

I don't think so. See above on the exceptions of operation calls on invalid\ which don't include oclIsKindOf and oclIsTypeOf.

UnlimitedNatural has no subtypes comment is wrong.

EvaluationVisitorImpl.visitOperationCallExp:PredefinedType.OCL_IS_KIND_OF new getInvalid() test is wrong.

See above.

UnlimitedNatural has no subtypes comment and functionality is wrong; OclVoid, OclInvalid are subtypes

Good point, wrong part of comment removed.

EvaluationVisitorImpl.visitOperationCallExp:PredefinedType.OCL_AS_TYPE UNLIMITED is an invalid Integer or Real! there is no PLUS_INFINITY (yet; I would like to add PLUS_INFINITY so that we can lose UnlimitedNatural altogether). the value conformance check for Real to Integer and Integer to UnlimitedNatural is missing conversions for BigInteger and BigDecimal are missing.

I think this behavior was missing before the patch, too. The tests so far\ hadn't obviated the need for change. I agree that an according test should be\ added, requiring the behavior to become stricter here too.

I changed the behavior accordingly, now, and changed these assertions in\ EvaluationNumberOperationTest.testUnlimitedOclAsType:

    assertResultInvalid("*.oclAsType(Integer)");\
    assertResultInvalid("*.oclAsType(Real)");

which pass now.

EvaluationVisitorImpl.visitOperationCallExp:isBooleanOperation(opCode) the second use seems counterproductive; another list of options and no check that source is boolean.

Partly agreed, the check for opCode is not necessary as handled by the\ subsequent switch/case. However, checking for instanceof Boolean misses the\ invalid case which is important to catch for the shortcut evaluations.

EvaluationVisitorImpl.visitOperationCallExp:OR etc Better. But can still malfunction for an e.g. Integer argument.

But that would have been caught by the analyzer and shouldn't have compiled in\ the first place. Therefore, I don't think such a check is required here. null\ and invalid, though, do conform to Boolean and therefore need to be considered.\ I therefore now do a "shortcut" for sourceVal instanceof Boolean. If false, I\ still have to check for the Boolean operations because I don't want to enter\ the if-branch for each invalid value.

EvaluationVisitorImpl.visitOperationCallExp:binary String No. This now CCEs for a Boolean argument.

See above. Has been handled by analyzer. Boolean does not conform to String.

EvaluationVisitorImpl.visitOperationCallExp:INDEX_OF Yes. [A pity API compatibility prevents fixing CollectionUtil where the bug is.] Worth a comment in CollectionUtil.indexOf that the null is indicating invalid.

Changed the "undefined" into "invalid" in the comment in\ CollectionUtil.indexOf.

EvaluationVisitorImpl.visitOperationCallExp:ternary now fails for OCL-null arguments; restore undefined tests

Passing null to an operation of course does not generally need to evaluate to\ invalid. In particular, it's perfectly legal to add null to a collection, e.g.,\ using INSERT_AT. Letting null pass on fails a bit later, with an NPE which then\ gets turned into invalid. I've now added a more precise null / undefined check\ depending on the particular operation to fail fast/clean.

EvaluationVisitorImpl.visitIfExp will malfunction if accept returns getInvalid().

Sort of. It'll as always fails with a CCE which then gets turned into invalid\ in the exception handler. We generally still have this possibility unless we\ wrap all accept calls using safeVisit... which I don't think is necessary.\ Well... since there already was a check for null here I simply expanded it into\ an isUndefined check which also covers invalid cleanly.

EvaluationVisitorImpl.visitCollectionLiteralExp change for invalid probably ok; iff accept API is sound change for unit nulls is wrong. Set{null} is what it says. 11.2.3 and A.2.5.3 talk primarily about use of null in a collection context not use of null a collection.

Sorry, I don't parse that last sentence. I agree that the hint to A.2.5.3 is\ not helpful. Removed. The key issue here, however, is the combination of\ implicit conversion caused by -> into a Set{...} literal. Set{null} therefore\ needs to be considered empty, so the null value must not be added to the Set in\ this case. I tried to enhance the comment.

EvaluationVisitorImpl API there are three return nulls. Only that for visitNullIteralExp is API consistent.

I also think the one for OCL_AS_TYPE on null, casting to VoidType is\ consistent. To be more clear, I'll replace the return null by a return\ sourceVal. The one at the end of visitOperationCallExp I replaced by a return\ getInvalid() which I think makes more sense.

Each of the oclIsKindOf and oclIsTypeOf has a 'invalid' null return. oclIsKindOf is called 4 times, 3 times with the null propagating erroneously.

Fixed.

oclIsTypeOf is called 2 times, 2 times with the null propagating erroneously.

Fixed.

This API is not documented.

Copyrights: should be 2011 or 2007,2011. If editing, where there are multiple copyright holders, keep just the first with "and others" for the rest.

Done. New patch to be attached in a few minutes.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 29, 2011 11:47

Created attachment 194374 (attachment deleted)\ Fixes issues according to Ed's comment #36

See my comment #42.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 29, 2011 11:48

Created attachment 194375 (attachment deleted)\ Commented unicode-dependent tests (until Hudson copes)

Same contents as 194374, except that the commented Unicode stretches should be preserved better.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 29, 2011 12:20

(In reply to comment #37)

In the following, the first migrated sub-test that fails is quoted. I therefore consider the test to be wrong, and the code that causes the test to pass to be wrong as well.

Interesting, quick and self-confident implication :-)

Collections do not use OCL equals semantics

    assertQueryTrue(null, "Sequence{3, 4.0, 'test'}->excludes(3.0)");

    assertQueryTrue(null, "Sequence{3, 4.0,

'test'}->excludesAll(Sequence{3.0, 4, 'TEST'})");

Numerics stuff. WONTFIX for mature.

    assertQueryTrue(null, "Set{null}->excludes(null)");

Special case handling in mature to enable the implicit set conversion to work with ->isEmpty() as demanded by the spec.

    assertQueryFalse(null, "Sequence{3, 4.0, 'test'}->includes(3.0)");

    assertQueryFalse(null, "Sequence{3, 4.0,

'test'}->includesAll(Sequence{3, 4, 'test'})");

    assertQueryFalse(null, "Set{null}->includes(null)");

    assertQueryTrue(null, "Set{null}->isEmpty()");

    assertQueryFalse(null, "Set{null}->notEmpty()");

More of the above.

Incorrect validity semantics

    assertQueryEquals(null, null, "Sequence{}->first()");

    assertQueryEquals(null, null, "Sequence{}->last()");

Good catch. Fixed, preparing another update to the patch which will follow soon.

Inconsistent OrderedSet types

    assertQueryEquals(null, valueFactory.getEmptySetValue(),
        "OrderedSet{}->flatten()");

Good point. It demonstrated that there were two "instanceof Set<?>" which should explicitly have excluded the "instanceof LinkedHashSet<?>" case. Test case adjusted, implementation fixed.

    assertQueryResults(null, "Set{'b'}", "OrderedSet{'a', 'b', 'c'} -

Set{'c', 'a'}");

What's wrong with this one?

Numeric precision

The following tests are unsound; the pivot version has an epsilon tolerance.

    assertQueryEquals(null, Double.valueOf(1.11d / 1.12d), "1.11 / 1.12");

    assertQueryEquals(null, Double.valueOf(1.11d * 1.12d), "1.11 * 1.12");

Numerics. WONTFIX for mature.

Numeric conformance

    assertQueryInvalid(null, "(3.0).oclAsType(Integer)");    -- 3.0 is

convertible without error to 3

    assertQueryTrue(null, "3.oclIsTypeOf(Integer)");        -- 3 is an

UnlimitedNatural not an Integer

Numerics. WONTFIX for mature.

Unlimited conformance

    assertQueryInvalid(null, "1.max(*)");

    assertQueryInvalid(null, "1.min(*)");

    assertQueryEquals(null, UnlimitedNaturalLiteralExp.UNLIMITED,
        "*.oclAsType(Integer)");

UnlimitedNatural handling in mature can't easily handle all Integers as UnlimitedNaturals. WONTFIX in mature.

[Pivot discrepancies removed... Want to open a new bug for them?]

Patch upgrade to come in a minute.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 29, 2011 12:22

Created attachment 194380 (attachment deleted)\ Additional fixes according to Ed's comment #37

See my comment #45

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Apr 29, 2011 12:22

Created attachment 194381 (attachment deleted)\ Commented unicode-dependent tests (until Hudson copes)

Same as 194380, as git patch with Unicode preserved

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 29, 2011 12:33

(In reply to comment #45)\ An easy one to responmd to.

No problem with the WONTFIXes so long as we gather them together in the\ conformance statement.\

    assertQueryTrue(null, "Set{null}->excludes(null)");

Special case handling in mature to enable the implicit set conversion to work with ->isEmpty() as demanded by the spec.

Separate reply since this may be a long discussion.

Inconsistent OrderedSet types

    assertQueryResults(null, "Set{'b'}", "OrderedSet{'a', 'b', 'c'} -

Set{'c', 'a'}");

What's wrong with this one?

Return type should/could be OrderedSet; not specified though since operation is\ missing. \

Patch upgrade to come in a minute.

Please supply just one new patch at the end of your working day, or when you\ have responded to everything you think worth responding to. I will then look at\ it this evening/early tomorrow.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 29, 2011 13:13

(In reply to comment #45)\

    assertQueryTrue(null, "Set{null}->excludes(null)");

Special case handling in mature to enable the implicit set conversion to work with ->isEmpty() as demanded by the spec.

(In reply to comment #42)

EvaluationVisitorImpl.visitCollectionLiteralExp change for invalid probably ok; iff accept API is sound change for unit nulls is wrong. Set{null} is what it says. 11.2.3 and A.2.5.3 talk primarily about use of null in a collection context not use of null a collection.

Sorry, I don't parse that last sentence. I agree that the hint to A.2.5.3 is not helpful. Removed. The key issue here, however, is the combination of implicit conversion caused by -> into a Set{...} literal. Set{null} therefore needs to be considered empty, so the null value must not be added to the Set in this case. I tried to enhance the comment.

The specification is not 100% clear or consistent so it is probably possible to find a sentence somewhere to justify any favoured interpretation.

My interpretation of the intent is that:

'null' is an object-value that exists (is valid) but whose content-value is unspecified; the domain of possible content-values is empty. 'null' may therefore be usefully associated with variables and aggregated in collections. Usage of the content-value is severely limited by the empty domain of content-values.

'invalid' is an object-value that does not exist (is invalid) and so there can be no content-value. Since 'invalid' has no content-value, any attempt to create an aggregated content-value is impossible and so a collection containing 'invalid' is flattened to an 'invalid' non-collection.

Therefore Set{null} is an aggregated content just the same as Set{1}.

A non-collection may be promoted to a collection by use of the -> operator on a non-collection. This is underspecified and conflictingly specified and has no AST support and so must be performed at run-time. For the pivot evaluator, a new OclAny::oclAsSet() operation is introduced to reify this promotion explicitly in the AST in the same way as Collection::collect is used to reify implicit collect. I will use oclAsSet for clarity of exposition even though the mature code conversion is hard coded.

1->isEmpty() is a shortform for 1.oclAsSet()->isEmpty()

it exploits OclAny::oclAsSet() to convert 1.oclAsSet() to Set{1} which is not empty.

null->isEmpty() is a shortform for null.oclAsSet()->isEmpty()

it exploits the OclVoid::oclAsSet() overload to convert null.oclAsSet() to Set{} which is empty.

invalid->isEmpty() is a shortform for invalid.oclAsSet()->isEmpty()

it exploits the OclInvalid::oclAsSet() overload to convert invalid.oclAsSet() to invalid which is invalid.

By migrating the semantics to the oclAsSet operation, we model it, exploit dynamic dispatch to distinguish unit/empty CollectionLiteralExp content, enabling static analysis, we avoid any special interpretation of null collection content; the special behaviour is in the creation of the collection.

Clearly the mature code will not reify the oclAsSet operation, but it can achieve the same observable semantics; it is the creation of the collection from null that is special not the usage of a collection containing null.

[The pivot evaluator does not actually use dynamic dispatch for oclAsSet; the behaviour is all folded into OclAny::oclAsSet(), which one day will allow analysis to recognize OclAny::oclAsSet() as 'final' and allow the dispatch overhead to be reduced.]

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Apr 29, 2011 13:57

(In reply to comment #42)

org.eclipse.ocl.uml.tests.RegressionTest.test_oclInvalid

was inv: invalid.oclIsTypeOf(OclInvalid) now inv: invalid.oclIsTypeOf(OclInvalid).oclIsInvalid()

I can't read from the specification that oclIsTypeOf is among the exceptions that for a part of the expression that is invalid should not evaluate to invalid. Those exceptions AFAIK are limited to oclIsUndefined, oclIsInvalid and the boolean operator shortcut evaluations. Therefore, now correctly as I would have said, invalid.oclIsTypeOf(OclInvalid) evaluates to invalid. However, there are excerpts of the spec, such as 8.2.1, which make use of oclIsTypeOf(OclInvalid). The bewildering part though is that those constraints offend another constraint that says that OCL collections cannot contain invalid, so iterating over a colection such as OclInvalid.allInstances() would yield an invalid collection itself.

Let me know what you suggest. Again, I read from the spec that invalid.oclIsTypeOf(OclInvalid) has to evaluate to invalid because any operation call on invalid except for oclIsInvalid and oclIsUndefined and the boolean shortcuts are to evaluate to invalid.

No. inv: invalid.oclIsTypeOf(OclInvalid).oclIsInvalid() is true, so invalid.oclIsTypeOf(OclInvalid).oclIsInvalid() is false.

Sorry, I don't understand this paragraph.

The specification is very unhelpful when it comes to understanding operations on OclVoid and OclInvalid. It says nothing works except a short list, but then implies a variety of other exceptions, and practical examples reveal yet more.

It is certainly possible to argue that the behaviour you have implemented is consistent with selected sentences in the specification. However it is different to the MDT/OCL 3.0.0 behaviour and I feel that the 3.0.0 behaviour is better and closer to what I would like OCL 2.4 to endorse.

My current interpretation of the intent of 'conformance' is:

There is a value-conformance lattice whereby OclInvalid conforms to OclVoid which conforms to everything (other than OclInvalid) ... This allows the 'null' and 'invalid' values to be returned as the abnormal result of any operation or property call.

There is a rather similar type-conformance lattice whereby a conforming type 'inherits' the features of types to which it conforms. OclVoid and OclInvalid are not type-conformant to anything, therefore only operations explicitly declared for OclVoid and OclInvalid are available for these types. Note that this would imply that invalid.oclIsKindOf(Boolean) would be false (assuming OclAny::oclIsKindOf is re-used for OclInvalid). This also implies that null.doSomething() is a static error even if there is a MyType::doSomething() operation.

However the specification does not distinguish value-conformance and type-conformance and so we have a mess. Until the specification provides an unambiguous semantics, I think it is undesirable to change the behaviour, which is vaguely consistent with usage of value-conformance for all conformance semantics.

I consider that all oclXXX routines should be explicitly defined for OclVoid and OclInvalid, even if that definition is perhaps to return invalid. The statement that all operations on null and invalid are invalid then applies only to user-defined operations.

For oclIsKindOf, and oclIsTypeOf, I feel the result should always be two-valued Boolean, so the unspecified operations give for instance invalid.oclIsTypeOf(OclInvalid) is true. I feel that an invalid result is surprising, contrary to examples in the specification and contrary to MDT/OCL 3.0 behaviour (and pivot behaviour) so please re-instate the 3.0 behaviour.