Community
Participate
Working Groups
In the expression let a:Integer='123a'.toInteger() in a.oclIsInvalid() invalid results instead of the expected true.
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.
Created attachment 193101 [details] Incremental patch containing bug fix Apply https://bugs.eclipse.org/bugs/attachment.cgi?id=193060 first which introduces EvaluationVisitorImpl.computeSourceTurningExceptionsIntoInvalid used by this patch.
Created attachment 193102 [details] 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.
Created attachment 193134 [details] Cumulative patch including Impact Analyzer adjustments This one includes the adjustments to the partial evaluator for forwarding the ValueNotFoundException properly
Created attachment 193169 [details] 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.
*** Bug 342561 has been marked as a duplicate of this bug. ***
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.
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'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.
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?
(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
(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?
(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/.
(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.
Created attachment 193340 [details] 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.
(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.
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.
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.
(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.
Created attachment 193597 [details] 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: - numeric type inconsistencies, e.g., when inserting 3.0 and 3 into a java.util.Set which then is handled as an OCL Set(Real) - issues with resolving collection operations in analyzer due to problems with collection conformance implementation - remaining issues due to the representation of UnlimitedNatural * as -1
Created attachment 193663 [details] Additionally fixes the collection operation lookup problem In addition to attachment 193597 [details], 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.
Created attachment 193691 [details] 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.
Don't forget that there are four 'identical' versions of oclstdlib. oclstdlib.ecore, oclstdlib.uml, and their hand coded OCLStandardLibrary initializations.
(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.
Created attachment 193709 [details] 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.
Created attachment 193768 [details] 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.
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().
(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.
(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.
(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?
(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.
Created attachment 194178 [details] 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.
(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.
(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.
Created attachment 194189 [details] 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.
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.
Created attachment 194330 [details] 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.
(In reply to comment #37) Was it on purpose that you obsoleted the non-git patch for the ecore tests? If so, why?
(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.
(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.
Created attachment 194348 [details] 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.]
(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.
Created attachment 194374 [details] Fixes issues according to Ed's comment #36 See my comment #42.
Created attachment 194375 [details] Commented unicode-dependent tests (until Hudson copes) Same contents as 194374, except that the commented Unicode stretches should be preserved better.
(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.
Created attachment 194380 [details] Additional fixes according to Ed's comment #37 See my comment #45
Created attachment 194381 [details] Commented unicode-dependent tests (until Hudson copes) Same as 194380, as git patch with Unicode preserved
(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.
(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.]
(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.
(In reply to comment #42) > > 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. Perhaps this will be unnecessary when 'oclAsSet' semantics is realised as in comment #49. I would certainly prefer not to change this test. > > > --------------------------- > > 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." Ok. Probably a Java 5/6 issue. > > > --------------------------- > > 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? Laurent introduced the nice usage of assertResults which allowed an OCL expression to define the results avoiding the complexity of Java collection poking; it also makes the tests more portable. > > 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. Yes. Though the assertFalse sub-test might belong in 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. > > 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. Where does this change help? I'm puzzled that it's been wrong for so long. Simple Java overload is that OclAny::"="(OclAny) can be overloaded by Collection::"="(OclAny) requiring tedious run-time type tests and conversions. As I finished writing my 'Modeling the OCL Standard Library' paper, I realized that covariant overload could be supported by the definition OclAny::"="<OclSelf>(OclSelf) in which OclSelf is a special template parameter denoting the statically determined derived 'self' type. i.e. 4=4.0 would therefore look first for UnlimitedNatural<OclSelf>::"="(OclSelf) fail statically on the argument and so try Integer<OclSelf>::"="(OclSelf) and again fail on the argument and so eventually settle on Real<OclSelf>::"="(OclSelf) as the statically 'referredOperation', assuming such a definition exists else OclAny<OclSelf>::"="(OclSelf). This is all done at compile time. At run-time Real<OclSelf>::"="(OclSelf) is guaranteed to have conformant-to-Real self and argument. > > > 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? BigInteger and BigDecimal may be used explicitly in Ecore models. They are sometimes used when extended precision is attempted. > > 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. You may be right, but I'm very worried about the ripple with respect to current code. If this also fixes bugs for Real then it may be good; if not I suspect that the current code is not fully understood. > > > --------------------------- > > 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. I misread the code. The debatable cases such as Sequence{}->union(Set{1}) are not semantically valid, so how CollectionUtil behaves is irrelevant. > > > 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? LinkedHashSet treats {1,2,3} and {3,2,1} as equal!!!! It is a set that maintains elements in a linked list. Ordering is not part of the semantics. > > > --------------------------- > > 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? No Problem. WONTFIX. (Currently 3.0 and 3 would be in different 'bins'); > > > --------------------------- > > 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. I don't doubt that your change is locally better. The problem is that I do not understand the code, but have observed that it is distributed in too many places. I'm therefore worried that a 'fix' in one place causes bugs in other places. > > > --------------------------- > > 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. Ok. Perhaps makes a noticeable difference; just re-use the -1 then. > > > --------------------------- > > 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. > Use of BigXXX is very patchy and associated with some CCEs. Improvements would be good but perhaps too hard. > > > --------------------------- > > 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. Probably. It is best not to rely too much on the analyzer if possible. [End of current batch of comments]
(In reply to comment #49) > 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. Explanation of pivot work on oclAsSet appreciated. My understanding of the mature code is that the implicit set conversion for -> on an expression with upper multiplicity 1 happens by a conversion to a Set{...} collection literal at compile time, not at run time. A quick test with a breakpoint in OCL.evaluate and entering self.name->isEmpty() in the console on an Ecore element gives "Set{self.name}->isEmpty()" as the expression. So fixing this would require deep changes in the parser which would have to introduce a new element into the AST which is a much deeper, less compatible change, I'm afraid. > [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.] What do you suggest for the mature impl?
(In reply to comment #50) Well... I'd call it a draw, and I don't care so much. Next week I'll look into it and try to ensure that oclIsKindOf and oclIsTypeOf don't respond with invalid when called on null or invalid for the cases discussed.
(In reply to comment #51) > (In reply to comment #42) > > > 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. > > Perhaps this will be unnecessary when 'oclAsSet' semantics is realised as in > comment #49. I would certainly prefer not to change this test. As mentioned in my other comment: please make a suggestion. Personally, I think starting to mess with the parser/analyzer and probably also with the stdlib to introduce something like oclAsSet is too hard for now. Please also note that prior to the change suggested by my patch it was entirely impossible to have null in a CollectionLiteralExp which is even worse. So far, all null values were ignored which is clearly against the spec. I relaxed this such that it's largely possible to have null in CollectionLiterals except for the special case of Set{null} which is a workaround for the missing oclAsSet. I'm very willing to document this special case in the conformance documentation and think that the patch by far improves the situation, not aggravates it. > > > --------------------------- > > > 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? > > Laurent introduced the nice usage of assertResults which allowed an OCL > expression to define the results avoiding the complexity of Java collection > poking; it also makes the tests more portable. Again: this is not about OCL, it's about a bug in an implementation class that I want to unit-test in isolation, independent of any OCL magic. If you disagree with the test location, please suggest a better one. > > > 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. > > Yes. Though the assertFalse sub-test might belong in a different test. Suggestions? > > > --------------------------- > > > 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. > > Where does this change help? I'm puzzled that it's been wrong for so long. Well, you complained above about it not having equivalents in oclstdlib.uml. Now it does and you're unhappy again. It probably didn't have any equivalent because oclstdlib.uml is not used anywhere. I'm ok with adding it for documentation. However, it would make for another good bugzilla to ask what oclstdlib.uml is good for and whether we may dare to simply remove it if we don't understand it anymore. > > 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? > > BigInteger and BigDecimal may be used explicitly in Ecore models. They are > sometimes used when extended precision is attempted. How would such values have UnlimitedNatural as their static type? Most certainly BigInteger and BigDecimal cannot be used for integral types in the mature implementation because all integral values are coerced to long before being operated on. What would BigInteger and BigDecimal map to in mature OCL land anyway? Certainly not UnlimitedNatural. So I currently don't see the point. > > 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. > > You may be right, but I'm very worried about the ripple with respect to current > code. If this also fixes bugs for Real then it may be good; if not I suspect > that the current code is not fully understood. Easy thing: there were bugs. The change proposed fixed them. No other noticeable harm done, in particular all other tests green. You say you don't understand the details. I suggest you try to provide failing tests, then we have a case. I spent quite some time over this code and only see positive effects of specifying Integer as the instance class, no negative ones. > > > 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? > > LinkedHashSet treats {1,2,3} and {3,2,1} as equal!!!! It is a set that > maintains elements in a linked list. Ordering is not part of the semantics. Good point. I'll have to check on Monday. I suppose that so far OrderedSet::=(OrderedSet) misbehaves in the mature implementation, also pre-342644. Separate bugzilla, maybe? > > > --------------------------- > > > 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. > > I don't doubt that your change is locally better. The problem is that I do not > understand the code, but have observed that it is distributed in too many > places. I'm therefore worried that a 'fix' in one place causes bugs in other > places. Same as above. I find it little convincing to put up a vague doubt against a bug fixed that includes tests. Again, I spent quite some time analyzing what happens with collection operation resolution. If you don't feel you want to get into the details, one way may just be to accept that if I provide a fix that doesn't hurt other tests then the changes may just be assumed okay until a reasonable test proves otherwise. > > > --------------------------- > > > 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. > > Ok. Perhaps makes a noticeable difference; just re-use the -1 then. You mean the other int UNLIMITED = -1 constant? Ok.
> LinkedHashSet treats {1,2,3} and {3,2,1} as equal!!!! It is a set that > maintains elements in a linked list. Ordering is not part of the semantics. The problem here will be similar to that of numeric equivalence. Since the mature implementation throughout chose to implement OrderedSet as LinkedHashSet, equality is usually mapped to that of LinkedHashSet which ignores ordering as you pointed out. This will particularly be a problem for OrderedSets nested in other collections as long as their Java equals is used. Therefore, fixing it in one place will create another inconsistency in the other place. Suggestion: WONTFIX, document in conformance doc. Ok?
(In reply to comment #55) > > LinkedHashSet treats {1,2,3} and {3,2,1} as equal!!!! It is a set that > > maintains elements in a linked list. Ordering is not part of the semantics. > > The problem here will be similar to that of numeric equivalence. Since the > mature implementation throughout chose to implement OrderedSet as > LinkedHashSet, equality is usually mapped to that of LinkedHashSet which > ignores ordering as you pointed out. This will particularly be a problem for > OrderedSets nested in other collections as long as their Java equals is used. > > Therefore, fixing it in one place will create another inconsistency in the > other place. Suggestion: WONTFIX, document in conformance doc. Ok? I checked ObjectUtil. It's possible to at least support the non-nested case. Only nested case remains an issue to be documented in a conformance statement. Ok?
Created attachment 194415 [details] invalid/null.oclIs[Kind|Type]Of fix, OrderedSet comparison with ordering See my previous comments
Created attachment 194416 [details] Commented unicode-dependent tests (until Hudson copes) Same a 194415, as git patch, helping to preserve commented Unicode tests
(In reply to comment #52) > A quick test with a breakpoint in > OCL.evaluate and entering self.name->isEmpty() in the console on an Ecore > element gives "Set{self.name}->isEmpty()" as the expression. So fixing this > would require deep changes in the parser which would have to introduce a new > element into the AST which is a much deeper, less compatible change, I'm > afraid. > > What do you suggest for the mature impl? If the mature evaluator cannot distinguish whether an AST looking like Set{null}->isEmpty() originated from Set{null}->isEmpty() or null->isEmpty(), you're in trouble. You need backward traceability, or some magic 'implicit' flag somewhere. This was all going to be WONTFIX anyway, so no-change is probably better than partial-change.
(In reply to comment #56) > (In reply to comment #55) > > > LinkedHashSet treats {1,2,3} and {3,2,1} as equal!!!! It is a set that > > > maintains elements in a linked list. Ordering is not part of the semantics. > > I checked ObjectUtil. It's possible to at least support the non-nested case. > Only nested case remains an issue to be documented in a conformance statement. > Ok? Yes. There's probably just a general conformance statement about nested collections ignoring OCL equality, set ordering, ????.
(In reply to comment #54) > (In reply to comment #51) > > (In reply to comment #42) > > > > 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. > > > > Perhaps this will be unnecessary when 'oclAsSet' semantics is realised as in > > comment #49. I would certainly prefer not to change this test. > > As mentioned in my other comment: please make a suggestion. Personally, I think > starting to mess with the parser/analyzer and probably also with the stdlib to > introduce something like oclAsSet is too hard for now. > > Please also note that prior to the change suggested by my patch it was entirely > impossible to have null in a CollectionLiteralExp which is even worse. So far, > all null values were ignored which is clearly against the spec. I relaxed this > such that it's largely possible to have null in CollectionLiterals except for > the special case of Set{null} which is a workaround for the missing oclAsSet. > I'm very willing to document this special case in the conformance documentation > and think that the patch by far improves the situation, not aggravates it. I see. But the problem is that this is application code that is broken by 'correct' OCL. Rather than change the query wouldn't it be better to either call evaluator(Object) to provide a context, or change the halt() implementation to handle a null argument. (In reply to comment #59) > (In reply to comment #52) > > A quick test with a breakpoint in > > OCL.evaluate and entering self.name->isEmpty() in the console on an Ecore > > element gives "Set{self.name}->isEmpty()" as the expression. So fixing this > > would require deep changes in the parser which would have to introduce a new > > element into the AST which is a much deeper, less compatible change, I'm > > afraid. > > > > What do you suggest for the mature impl? > > If the mature evaluator cannot distinguish whether an AST looking like > Set{null}->isEmpty() originated from Set{null}->isEmpty() or null->isEmpty(), > you're in trouble. You need backward traceability, or some magic 'implicit' > flag somewhere. > > This was all going to be WONTFIX anyway, so no-change is probably better than > partial-change. The above makes me uncomfortable for arbitrary users. In 3.0.0, the bug was: 'null' cannot exist in collection literals, and is inconsistent if included in other ways and is handled inconsistently if a null is in a collection. null->... 'works' The current patch: null can work in collections, except that Set{null} is converted to Set{} null->... 'works' This seems unequivocably better but irregular. ----- Suggestion: Change the analyzer to encode Collection{xxx} rather than Set{xxx} for xxx-> so that the evaluator can recognize Collection{xxx} as oclAsSet. Collection{xxx} is not a legal concrete syntax, so there is no ambiguity for the evaluator. Old ASTs will malfunction as before with the revised evaluator. New ASTs should not be processed by an old evaluator. If you're happy with this idea we'd better double check with Obeo.
(In reply to comment #61) > I see. But the problem is that this is application code that is broken by > 'correct' OCL. Rather than change the query wouldn't it be better to either > call evaluator(Object) to provide a context, or change the halt() > implementation to handle a null argument. Ok, if the duplicated/modified query is the problem, I can change the test to use evaluate(Object) to avoid null being passed in. It then should be possible to use the existing query. On the Set{null} issue: > Suggestion: > > Change the analyzer to encode Collection{xxx} rather than Set{xxx} for xxx-> so > that the evaluator can recognize Collection{xxx} as oclAsSet. > > Collection{xxx} is not a legal concrete syntax, so there is no ambiguity for > the evaluator. > > Old ASTs will malfunction as before with the revised evaluator. > > New ASTs should not be processed by an old evaluator. > > If you're happy with this idea we'd better double check with Obeo. I don't think this is such a good idea. 10-11-42, Section 7.5.3: "Because the multiplicity of the role manager is one, self.manager is an object of type Person. Such a single object can be used as a Set as well. It then behaves as if it is a Set containing the single object. The usage as a set is done through the arrow followed by a property of Set." This makes it very explicit that a Set needs to be produced, not a Collection. Let me again summarize my conviction on this topic: Instead of failing for all null values in CollectionLiterals we should only fail for a single null in a Set literal which reduces the chances for errors. Collections were already correctly able to hold null values, so ->including or ->insertAt already handled things correctly. The number of deviations from the spec goes down. The remaining non-conformance is necessary because of the way the implementation handles the implicit Set conversion (putting it into the abstract syntax as a Set literal). The spec and pragmatics demand that a null value converted to a Set this way results in an empty Set. One workaround I may think of but that also has its quirks: the parser / analyzer could add an EAnnotation to the Set{...} literal resulting from the implicit -> conversion, and the evaluator could check for such an annotation. If such an annotation exists and the literal contains only one item and this single item evaluates to null, an empty Set results. Otherwise, the null value is inserted into the Set. The problem then would only be that a non-standard annotation controls the behavior of a Set literal. What do you think?
(In reply to comment #54) > > > 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. > > > > You may be right, but I'm very worried about the ripple with respect to current > > code. If this also fixes bugs for Real then it may be good; if not I suspect > > that the current code is not fully understood. > > Easy thing: there were bugs. The change proposed fixed them. No other > noticeable harm done, in particular all other tests green. You say you don't > understand the details. I suggest you try to provide failing tests, then we > have a case. I spent quite some time over this code and only see positive > effects of specifying Integer as the instance class, no negative ones. You suggest that the only interesting thing about the instanceTypeName is that it is Comparable. However a find references on getInstanceClass reveals another critical usage; isKindOf and isTypeOf. Their code makes me very uncomfortable. It is extremely simplistic, failing totally for Long, BigInteger and BigDecimal, and possibly exploiting the null instanceTypeName for Double. Since UnlimitedNatural barely exists in the current code. I don't like this change; it is just too risky. You need to start with some white box tests that pass and fail on the current code for at least Long/Integer and Double (and ideally BigInteger, Short, Float and BigDecimal too). This can then demonstrate that the revision is clearly better. I currently see new bugs in isKindOf and isTypeOf. But: surely the only UnlimitedNatural objects are '*'? So this fix is solely to make '*' comparison work. Too much risk for a cosmetic gain. WONTFIX unless you can provide a thorough revision of Long/Integer etc.
(In reply to comment #62) > (In reply to comment #61) > > I see. But the problem is that this is application code that is broken by > > 'correct' OCL. Rather than change the query wouldn't it be better to either > > call evaluator(Object) to provide a context, or change the halt() > > implementation to handle a null argument. > > Ok, if the duplicated/modified query is the problem, I can change the test to > use evaluate(Object) to avoid null being passed in. It then should be possible > to use the existing query. > > On the Set{null} issue: > > > Suggestion: > > > > Change the analyzer to encode Collection{xxx} rather than Set{xxx} for xxx-> so > > that the evaluator can recognize Collection{xxx} as oclAsSet. > > > > Collection{xxx} is not a legal concrete syntax, so there is no ambiguity for > > the evaluator. > > > > Old ASTs will malfunction as before with the revised evaluator. > > > > New ASTs should not be processed by an old evaluator. > > > > If you're happy with this idea we'd better double check with Obeo. > > I don't think this is such a good idea. 10-11-42, Section 7.5.3: "Because the > multiplicity of the role manager is one, self.manager is an object of type > Person. Such a single object can be > used as a Set as well. It then behaves as if it is a Set containing the single > object. The usage as a set is done through the > arrow followed by a property of Set." This makes it very explicit that a Set > needs to be produced, not a Collection. > > Let me again summarize my conviction on this topic: Instead of failing for all > null values in CollectionLiterals we should only fail for a single null in a > Set literal which reduces the chances for errors. Collections were already > correctly able to hold null values, so ->including or ->insertAt already > handled things correctly. The number of deviations from the spec goes down. The > remaining non-conformance is necessary because of the way the implementation > handles the implicit Set conversion (putting it into the abstract syntax as a > Set literal). The spec and pragmatics demand that a null value converted to a > Set this way results in an empty Set. > > One workaround I may think of but that also has its quirks: the parser / > analyzer could add an EAnnotation to the Set{...} literal resulting from the > implicit -> conversion, and the evaluator could check for such an annotation. > If such an annotation exists and the literal contains only one item and this > single item evaluates to null, an empty Set results. Otherwise, the null value > is inserted into the Set. > > The problem then would only be that a non-standard annotation controls the > behavior of a Set literal. What do you think? I was thinking of an EAnnotation too. It works but is klunky. I also though of an new 'implicit' property for CollectionLiteral. You miss the point of my suggestion. With the improved code, there is an ambiguity for Set{null}. I propose to change one of the ambiguities to Collection{null}, but only in the AST, not in the observable behaviour. Yes the spec says a Set is produced. (It elsewhere says that a Bag is produced). I'm suggesting that visitCollectionLiteralExp, which formerly threw away all nulls, and is improved to throw away only singleton set nulls, instead recognises collection something as oclAsSet and consequently creates an empty set or a singleton set according to the null-ness of the argument. The Collection kind is solely in the AST as a flag avoiding extra objects. Both approaches will be incorrectly handled by a different evluator that doesn't understand the new protocol. An EAnnotation will probably be silently ignored, but might get lost in a careless copy. A Collection kind will not get lost, but might fail a surprisingly strict validation, and might fail to evaluate since Collection is abstract. In both cases this is a semantic API change for a proprietary representation. I don't think we support alternative evaluators. I prefer the Collection approach since it is more compact.
(In reply to comment #64) > I also though of an new 'implicit' property for CollectionLiteral. > > You miss the point of my suggestion. Maybe you just have to make it more clear... Along the same lines it seems I haven't made my point sufficiently clear. Using Collection{...} in the AST is not good because operation calls will be resolved based on its type. Collection has only a subset of the operations that Set has. This may cause lots of code to fail which so far works based on Set's operations. > Both approaches will be incorrectly handled by a different evluator that > doesn't understand the new protocol. An EAnnotation will probably be silently How likely is this? We have to judge the overall qualities of the holistic set-up. It will always have flaws, and we have to make trade offs. Either we can't use any null in collection literals (current code), or we have a special case for Set{null}, or we have an annotation that the evaluator that comes with the parser handles correctly, and other evaluators would evaluate erroneously to a single-element Set containing a single null value instead of being empty. Other evaluators based on the current analyzer will fail in the same way, so an annotation doesn't make things worse. > In both cases this is a semantic API change for a proprietary representation. > I don't think we support alternative evaluators. I prefer the Collection approach > since it is more compact. ...but flawed because of the operations offered on Set and not on Collection. I'll provide a patch with the annotation that you may choose to review.
(In reply to comment #63) > But: surely the only UnlimitedNatural objects are '*'? So this fix is solely to > make '*' comparison work. Too much risk for a cosmetic gain. WONTFIX unless you > can provide a thorough revision of Long/Integer etc. Not cosmetic at all. Any multiplicity comparison on any Ecore element will fail because at another place in the code there is this magic conversion of the upper bounds to static type UnlimitedNatural. Therefore I argue this shall not be WONTFIX. What exactly is the problem with Long/Integer?
(In reply to comment #65) > Using Collection{...} in the AST is > not good because operation calls will be resolved based on its type. Collection > has only a subset of the operations that Set has. This may cause lots of code > to fail which so far works based on Set's operations. You're right. There may be very unpleasant downstream analysis ripples if oclAsSet is encoded as a Collection kind. Too hard to follow and fix. ---- So three choices: WONTFIX for the x->/Set{x}-> ambiguity for null x. An EAnnotation; extra AST objects for all oclAsSet usages. A new 'implicit' property. Oops; an API change, requiring rather enlightened interpretation of internal model APIs to introduce after M6. ---- So the only difficult thing is the EAnnotation URI. Looking at the existing ecore models perhaps: "http://www.eclipse.org/ocl/1.1.0/OCL/Annotations" with an "oclAsSet" detail of "true". I think 1.1.0 rather than 3.1.0 since this is an upward compatible change in the style of emf/2002's many evolutions.
(In reply to comment #67) > WONTFIX for the x->/Set{x}-> ambiguity for null x. > > An EAnnotation; extra AST objects for all oclAsSet usages. > > A new 'implicit' property. Oops; an API change, requiring rather enlightened > interpretation of internal model APIs to introduce after M6. > ---- > So the only difficult thing is the EAnnotation URI. Looking at the existing > ecore models perhaps: > > "http://www.eclipse.org/ocl/1.1.0/OCL/Annotations" with an "oclAsSet" detail of > "true". > > I think 1.1.0 rather than 3.1.0 since this is an upward compatible change in > the style of emf/2002's many evolutions. Annotation solution implemented. Works like a charm. Tests that so far had trouble with Set{null} adjusted. Will attach patch in a few minutes. Remaining open issue as far as I see it is the UnlimitedNatural instance class issue. Would be great if you could detail the cases that concern you.
(In reply to comment #66) > (In reply to comment #63) > > But: surely the only UnlimitedNatural objects are '*'? So this fix is solely to > > make '*' comparison work. Too much risk for a cosmetic gain. WONTFIX unless you > > can provide a thorough revision of Long/Integer etc. > > Not cosmetic at all. Any multiplicity comparison on any Ecore element will fail > because at another place in the code there is this magic conversion of the > upper bounds to static type UnlimitedNatural. Therefore I argue this shall not > be WONTFIX. Looking again. I got confused between the classes and co-classes in oclstdlib.ecore. The extra instanceTypeName matches the static code. Ok. Dangerous but see next Long/Integer sub-thread.
(In reply to comment #66) > What exactly is the problem with Long/Integer? I think we can postpone this to RC1, so at to get something out for M7. See Bug 344368.
Created attachment 194420 [details] Solves the Set{null} problem using a CollectionLiteralExp annotation See last comment, fixes the Set{null} problem by adding an annotation to a CollectionLiteralExp that the analyzer creates only for implicit set conversion. The evaluator then doesn't add a null value to the collection if it finds this annotation.
Created attachment 194421 [details] Same as 194420 but as git patch, preserving Unicode characters in tests Same as 194420
Getting close: AbstractOCLAnalyzer.getCollectionSourceExpression wrong URI in getEAnnotation(Environment.OCL_NAMESPACE_URI) EcoreEvaluationEnvironment.createTuple // do numeric coercion if necessary: if (value instanceof Number && property.getEType().getInstanceClass() != null && !(value instanceof Double) && Double.class.isAssignableFrom(property.getEType().getInstanceClass())) { value = ((Number) value).doubleValue(); } should be shared functionality and probably handle Long correctly too. ? Bug 344368 if (value != null || !(property.getEType() instanceof VoidType)) { // don't try to set null on a VoidType property; it's already null; trying to // set it will cause an exception in the EMF setting delegate infrastructure // because VoidType does not conform to EClass. tuple.eSet(property, value); } Ugh! comment just screams bug to hide bug. Deleting the || !(property.getEType() instanceof VoidType) doesn't fail any test. Is the comment and code obsolete? Same issue in TupleFactory, where, if needed, a single derivation of eGet seems called for. EcoreEvaluationEnvironment.isKindOf } else if (classifier instanceof AnyType) { return Boolean.TRUE; What does this do? It isn't an OclAny conformance test and it isn't needed to make any test pass? EvaluationVisitorImpl..OCL_AS_TYPE - sourceVal != getInvalid() && is redundant EvaluationVisitorImpl.visitCollectionLiteralExp - | parts.size() > 1 | is redundant
(In reply to comment #73) > Getting close: > > AbstractOCLAnalyzer.getCollectionSourceExpression > wrong URI in getEAnnotation(Environment.OCL_NAMESPACE_URI) Oh, thanks, good catch. > EcoreEvaluationEnvironment.createTuple > // do numeric coercion if necessary: > if (value instanceof Number && property.getEType().getInstanceClass() != > null && > !(value instanceof Double) && > > Double.class.isAssignableFrom(property.getEType().getInstanceClass())) { > value = ((Number) value).doubleValue(); > } > should be shared functionality and probably handle Long correctly too. ? Bug > 344368 As you suggest, 344368 should handle this. > if (value != null || !(property.getEType() instanceof VoidType)) { > // don't try to set null on a VoidType property; it's already null; > trying to > // set it will cause an exception in the EMF setting delegate > infrastructure > // because VoidType does not conform to EClass. > tuple.eSet(property, value); > } > Ugh! comment just screams bug to hide bug. Deleting the || > !(property.getEType() instanceof VoidType) doesn't fail any test. > Is the comment and code obsolete? > Same issue in TupleFactory, where, if needed, a single derivation of eGet seems > called for. Absolutely required. I ran into a ClassCastException where a VoidTypeImpl cannot be cast to an EClass. It happened during playing with the OCL console in the context of Ecore navigation and queries for EReferences with UnlimitedNaturals as upper bounds where I constructed tuples with the references' names and upper bounds. In some combination that I'm yet to reproduce the analyzer inferred OclVoid as a tuple component's type, leading up to the NPE. I'm yet to integrate this into the OCL test suite in TuplesTest. Adding these tests showed other problems related to OclVoid in Tuples which I'm providing a fix for now. > EcoreEvaluationEnvironment.isKindOf > } else if (classifier instanceof AnyType) { > return Boolean.TRUE; > What does this do? It isn't an OclAny conformance test and it isn't needed to > make any test pass? If I comment it, seven tests fail. Example: EvaluationStringOperationTest.testStringOclIsKindOf, with 'test'.oclIsKindOf(OclAny). > EvaluationVisitorImpl..OCL_AS_TYPE > - sourceVal != getInvalid() && is redundant Correct. Removed. > EvaluationVisitorImpl.visitCollectionLiteralExp > - | parts.size() > 1 | is redundant Redundant, but more efficient. The isImplicitSetConversion test operation needs to fiddle with the annotations which requires several hash lookup operations which are expensive compared to a size() (usually a simple getter on a cached field) and int comparison. That's why I argue it should stay.
Created attachment 194430 [details] Solves null/OclVoid in Tuples and adds according tests See my last comment
Created attachment 194431 [details] Same as 194430 but as git patch, preserving Unicode characters in tests Same as 194430 but as git patch, preserving Unicode characters in tests
(In reply to comment #75) > Created attachment 194430 [details] o.e.o is missing EcoreEvaluationenvironment is missing the git patch doesn't work for me; no git
(In reply to comment #74) Although I can't review 194430, there were only trivial changes required to 194420. so I can +1 194420 with the trivial changes and careful commit of the Unicode characters. > > > if (value != null || !(property.getEType() instanceof VoidType)) { > > // don't try to set null on a VoidType property; it's already null; > > trying to > > // set it will cause an exception in the EMF setting delegate > > infrastructure > > // because VoidType does not conform to EClass. > > tuple.eSet(property, value); > > } > > Ugh! comment just screams bug to hide bug. Deleting the || > > !(property.getEType() instanceof VoidType) doesn't fail any test. > > Is the comment and code obsolete? > > Same issue in TupleFactory, where, if needed, a single derivation of eGet seems > > called for. > > Absolutely required. I ran into a ClassCastException where a VoidTypeImpl > cannot be cast to an EClass. It happened during playing with the OCL console in > the context of Ecore navigation and queries for EReferences with > UnlimitedNaturals as upper bounds where I constructed tuples with the > references' names and upper bounds. In some combination that I'm yet to > reproduce the analyzer inferred OclVoid as a tuple component's type, leading up > to the NPE. I'm yet to integrate this into the OCL test suite in TuplesTest. > > Adding these tests showed other problems related to OclVoid in Tuples which I'm > providing a fix for now. > Raise a seprate Bugzilla for these tests. The extra code looks redundant and smelly =ratyher than wrong, so we can run with it. > > EcoreEvaluationEnvironment.isKindOf > > } else if (classifier instanceof AnyType) { > > return Boolean.TRUE; > > What does this do? It isn't an OclAny conformance test and it isn't needed to > > make any test pass? > > If I comment it, seven tests fail. Example: > EvaluationStringOperationTest.testStringOclIsKindOf, with > 'test'.oclIsKindOf(OclAny). > Yes. I now get 4 failure on Ecore standalone. I msread of test of souyrce not test of target. > > EvaluationVisitorImpl.visitCollectionLiteralExp > > - | parts.size() > 1 | is redundant > > Redundant, but more efficient. The isImplicitSetConversion test operation needs > to fiddle with the annotations which requires several hash lookup operations > which are expensive compared to a size() (usually a simple getter on a cached > field) and int comparison. That's why I argue it should stay. Yes, a marginal improvement.
(In reply to comment #78) > > > > > if (value != null || !(property.getEType() instanceof VoidType)) { > > > // don't try to set null on a VoidType property; it's already null; > > > trying to > > > // set it will cause an exception in the EMF setting delegate > > > infrastructure > > > // because VoidType does not conform to EClass. > > > tuple.eSet(property, value); > > > } > > > Ugh! comment just screams bug to hide bug. Deleting the || > > > !(property.getEType() instanceof VoidType) doesn't fail any test. > > > Is the comment and code obsolete? > > > Same issue in TupleFactory, where, if needed, a single derivation of eGet seems > > > called for. > > > > Absolutely required. I ran into a ClassCastException where a VoidTypeImpl > > cannot be cast to an EClass. It happened during playing with the OCL console in > > the context of Ecore navigation and queries for EReferences with > > UnlimitedNaturals as upper bounds where I constructed tuples with the > > references' names and upper bounds. In some combination that I'm yet to > > reproduce the analyzer inferred OclVoid as a tuple component's type, leading up > > to the NPE. I'm yet to integrate this into the OCL test suite in TuplesTest. > > I guess the problem comes from the following revised test public void test_tuplePartAccess_238049() { OCLExpression<EClassifier> expr = parse( "package ocltest context Fruit " + "inv: Tuple{first : OclVoid = 'Roger', last : OclVoid = null}.first " + "endpackage"); assertEquals("Roger", evaluate(expr)); } but it generates a static error. Behind the scenes it is clearly a propblem to assign to OclVoid. The comment is very misleading. Any assignmment to OclVoid is liable to encounter conformance trouble and quite rightly so. Change comment to "Don't try to set an OclVoid property", except that why do we protect OclVoid. Setting "1_2_3_4" on an Integer will cause EMF to barf too. The problem is upstream. > > Adding these tests showed other problems related to OclVoid in Tuples which I'm > > providing a fix for now. I had considerable difficulties getting the correct homogeneous tuple type for the pivot model so you may have trouble here too.
(In reply to comment #77) > (In reply to comment #75) > > Created attachment 194430 [details] [details] > o.e.o is missing > EcoreEvaluationenvironment is missing > > the git patch doesn't work for me; no git Should work if you set the "Apply patch" dialog to ignore the first two path elements. That's how I get the git-exported patch applied to my CVS workspace.
(In reply to comment #80) > (In reply to comment #77) > > (In reply to comment #75) > > > Created attachment 194430 [details] [details] [details] > > o.e.o is missing > > EcoreEvaluationenvironment is missing > > > > the git patch doesn't work for me; no git > > Should work if you set the "Apply patch" dialog to ignore the first two path > elements. That's how I get the git-exported patch applied to my CVS workspace. No. Apply Patch knwos nothing abour a/plugins... ; nothing exists. Did you miss the +1 for 194420 + trivia in #78?
(In reply to comment #78) > (In reply to comment #74) > Although I can't review 194430, there were only trivial changes required to > 194420. so I can +1 194420 with the trivial changes and careful commit of the > Unicode characters. But 194420 still has the oclIsTypeOf bugs that 194430 fixes. If you insist to keep the Tuple issue separate, I'll raise a separate bugzilla for it and provide the fix there. I'll quickly argue below why I think it's essential, contrary to your reasoning. I will prepare one more patch that contains the oclIsTypeOf-related fixes that 194430 contained, reverting the fix for the Tuple bug, hoping to get your +1 for that, too. > > > if (value != null || !(property.getEType() instanceof VoidType)) { > > > // don't try to set null on a VoidType property; it's already null; > > > trying to > > > // set it will cause an exception in the EMF setting delegate > > > infrastructure > > > // because VoidType does not conform to EClass. > > > tuple.eSet(property, value); > > > } > > > Ugh! comment just screams bug to hide bug. Deleting the || > > > !(property.getEType() instanceof VoidType) doesn't fail any test. > > > Is the comment and code obsolete? > > > Same issue in TupleFactory, where, if needed, a single derivation of eGet seems > > > called for. > > > > Absolutely required. I ran into a ClassCastException where a VoidTypeImpl > > cannot be cast to an EClass. It happened during playing with the OCL console in > > the context of Ecore navigation and queries for EReferences with > > UnlimitedNaturals as upper bounds where I constructed tuples with the > > references' names and upper bounds. In some combination that I'm yet to > > reproduce the analyzer inferred OclVoid as a tuple component's type, leading up > > to the NPE. I'm yet to integrate this into the OCL test suite in TuplesTest. > > > > Adding these tests showed other problems related to OclVoid in Tuples which I'm > > providing a fix for now. > > > Raise a seprate Bugzilla for these tests. The extra code looks redundant and > smelly =ratyher than wrong, so we can run with it. OclVoid is the type the analyzer assigns to a single "null" value. So Tuple{a=null, b="abc"} will be inferred to Tuple(a:OclVoid, b:String). The evaluator doesn't execute this properly and fails with the aforementioned CCE. This is a bug, and it's easily fixed by the patch I provided. I'm okay with extracting this into a separate bugzilla.
(In reply to comment #81) > (In reply to comment #80) > > (In reply to comment #77) > > > (In reply to comment #75) > > > > Created attachment 194430 [details] [details] [details] [details] > > > o.e.o is missing > > > EcoreEvaluationenvironment is missing > > > > > > the git patch doesn't work for me; no git > > > > Should work if you set the "Apply patch" dialog to ignore the first two path > > elements. That's how I get the git-exported patch applied to my CVS workspace. > > No. Apply Patch knwos nothing abour a/plugins... ; nothing exists. > > Did you miss the +1 for 194420 + trivia in #78? My apply patch dialog has a drop-down that lets me choose how many leading path segments to ignore in the patch. My CVS workspace has the bundles immediately checked-out into the workspace dir. So if I tell apply-patch to ignore two leading segments, a/plugins and a/tests gets ignored, and the remaining path matches my workspace. Maybe you have a different workspace setup?
(In reply to comment #79) I also have to revise Laurent's tests then and split off one test for 344391 because without the Tuple-null fix it fails: EvaluationCollectionOperationTest.testCollectionProductNullValue fails with exceptions without the fix.
Created attachment 194437 [details] Reverts the Tuple-null fix, fixes oclIsTypeOf for null/invalid Tuple null fix moved to bug 344391. Had to comment one more of Laurent's tests that depend on null in Tuples which then depends on 344391. In addition to 194420 this patch fixes the oclIsTypeOf behavior for null/invalid where so far the results were wrong, considering null and invalid as oclIsTypeOf anything although they should only be oclIsTypeOf OclVoid and OclInvalid, respectively.
Created attachment 194438 [details] Reverts the Tuple-null fix, fixes oclIsTypeOf for null/invalid Sorry, 194437 forgot to revert one part of the Tuple-null fix. This one should be ok now.
(In reply to comment #82) > (In reply to comment #78) > > (In reply to comment #74) > > Although I can't review 194430, there were only trivial changes required to > > 194420. so I can +1 194420 with the trivial changes and careful commit of the > > Unicode characters. > > But 194420 still has the oclIsTypeOf bugs that 194430 fixes. If you insist to > keep the Tuple issue separate, I'll raise a separate bugzilla for it and > provide the fix there. I'll quickly argue below why I think it's essential, > contrary to your reasoning. I'm losing track of where we are. I thought that I raised 7 queries against 194420 on which we quickly agreed about 6. Only the tuples are oustanding. So commit 194420 plus the trivial changes for the 6 agreements. I suspect the tuple change is correct, but I'm not sure what I'm checking. It's just that this bug has many issues resolved and I/we want it committed for M7 tomorrow. If we have to bounce a tuple discussion back and forth a couple of times the commit gets to be very late. The tuples were broken in M6 so waiting till RC1 for the fix should be no hardship. Please commit this as soon as possible so that we have a new CVS reference for other patches. (Patches on patches are a nightmare.)
(In reply to comment #87) > (In reply to comment #82) > > (In reply to comment #78) > > > (In reply to comment #74) > > > Although I can't review 194430, there were only trivial changes required to > > > 194420. so I can +1 194420 with the trivial changes and careful commit of the > > > Unicode characters. > > > > But 194420 still has the oclIsTypeOf bugs that 194430 fixes. If you insist to > > keep the Tuple issue separate, I'll raise a separate bugzilla for it and > > provide the fix there. I'll quickly argue below why I think it's essential, > > contrary to your reasoning. > > I'm losing track of where we are. I thought that I raised 7 queries against > 194420 on which we quickly agreed about 6. Only the tuples are oustanding. So > commit 194420 plus the trivial changes for the 6 agreements. > > I suspect the tuple change is correct, but I'm not sure what I'm checking. It's > just that this bug has many issues resolved and I/we want it committed for M7 > tomorrow. > > If we have to bounce a tuple discussion back and forth a couple of times the > commit gets to be very late. The tuples were broken in M6 so waiting till RC1 > for the fix should be no hardship. > > Please commit this as soon as possible so that we have a new CVS reference for > other patches. (Patches on patches are a nightmare.) Good, I take this as a +1 for 194438 which I'll commit now, with the Unicode stuff manually patched to get it into CVS. Based on this I'll then produce a CVS patch for 344391. Maybe we get 344391 into CVS too today, for M7. That would be great.
Committed to CVS HEAD.
Closing
