Community
Participate
Working Groups
I suppose this should result in true instead. Reproduce by opening the classic OCL console in an Indigo M6 and type ('123a'.toInteger()).oclIsInvalid() Result displayed: Evaluating: ('123a'.toInteger()).oclIsInvalid() Results: OclInvalid
(In reply to comment #0) The problem is the way exceptions bubble up in EvaluationVisitorImpl. In particular, handling the toInteger() OperationCallExp causes the Integer.valueOf((String) sourceVal) to throw an exception which is not handled until the call stack was unwound up to AbstractEvaluationVisitor.visitExpression where the "catch (RuntimeException e)" is located. I think that in case an OperationCallExp is an oclIsInvalid() call, an try/catch has to surround the evaluation of the source expression and use invalid as source value in case an exception is caught. I'll try to prepare a patch, unless someone stops me by saying this is not a bug but a "compatibility feature"...
Created attachment 193048 [details] Fixed by catching exception during OperationCallExp source eval oclIsInvalid() can only work reasonably if a source expression evaluating to invalid is detected during evaluating the oclIsInvalid() operation call expression. Therefore, this patch puts a try/catch around the source evaluation. EvaluationHaltedException is forwarded. All other RuntimeExceptions are caught and transformed into getInvalid(). I added corresponding test cases for both, UML and Ecore.
(In reply to comment #2) What's the expected outcome of let a:Integer='123a'.toInteger() in a.oclIsInvalid() according to the specification we're trying to implement?
Created attachment 193059 [details] Extract exception handling into method so subclasses may redefine For example, the Impact Analyzer's partial evaluator needs to rely on a ValueNotFoundException being passed all the way up. Using the protected method computeSourceTurningExceptionsIntoInvalid it can do exactly this.
Created attachment 193060 [details] Added missing @since
The specification is not always 100% clear, so sometimes 'correct' is biased by my interpretation of 'correct'. 'invalid' is a permissible value so let a:Integer='123a'.toInteger() in a.oclIsInvalid() caches invalid in the let and then uses it. The Pivot evaluator gets this right IMHO. For a long time the Pivot evaluator propagated invalid as a true return value, with Java-null and exceptions as lazy invalids. In order to make the exception environment available I changed to use exceptions for returns. This faioled for your let case, so there is a turn around to convert exceptions as inputs to invalid. The problem in the old code is that it was never defined whether Java-null and Java-exception represent OCL-null or OCL-invalid and Java-null is used extensively. This wasn't a problem for OCL 1.x that only has undefined, but it is one of many causes of uncertainty in the old code. In the light of the above, are you confident that your patch is an unequivocal improvement for existing users?
(In reply to comment #6) > The specification is not always 100% clear, so sometimes 'correct' is biased by > my interpretation of 'correct'. > > 'invalid' is a permissible value so > > let a:Integer='123a'.toInteger() in a.oclIsInvalid() > > caches invalid in the let and then uses it. > > The Pivot evaluator gets this right IMHO. Ok, so I propose to take it one step at a time. > In the light of the above, are you confident that your patch is an unequivocal > improvement for existing users? The original bug described here is the one that deals with invalid resulting from the source particularly of an oclIsInvalid() call. Those are handled properly by the patch as shown by the tests. No other regressions seen so far with the given tests. So, yes, I think this is an improvement for existing users, particularly as there is no other way that I know of to detect invalid when it results from some nontrivial expression. The assignment to a let variable should trigger another bug. I'll open one right now and may find some time looking into it tomorrow. What other cases do we know where invalid isn't handled correctly in the standard evaluator?
See https://bugs.eclipse.org/bugs/attachment.cgi?id=193134 which is a cumulative patch also fixing this problem.
Ed Willink wrote in a separate e-mail: "It very clear from the attachment trail from bug 342561 that API compatibility is really hard. You found at least one problem. I have not studied the fix in detail, but at a glance it looks like partial API evolution that needs considerable study. You introduce a new method computeSourceTurningExceptionsIntoInvalid that is invoked from just one place. I am pretty sure that sources are computed in many places, so much more is needed. A simple search for "source.accept" reveals one more. What about converting Java-null to invalid too? I will review the finished piece of work." My comment on this: during patch development it turned out that it is necessary to allow for certain types of exceptions to be passed on instead of treating it as an invalid value. As this may need to be extended as the impact analyzer case has shown, I chose to introduce a protected method for this which extenders of the evaluation visitor may choose to override accordingly. The updated patch now shows its usage in PartialEvaluationVisitorImpl. Sources computed in other places may leave exceptions unhandled. I see two important cases where exceptions need to be turned into an invalid value: - source of oclIsInvalid() (or, more generally, an OperationCallExp) - assignment to a variable The latter in particular was the let-case described separately in bug [342644]. Other variable assignments that I'm aware of: - operation parameter passing - iterator binding More? For iterator binding I assume that a single invalid value can't appear in a source collection, so the entire IteratorExp would turn invalid. What about invalid accumulator values in an ->iterate(...)? Is something like x->iterate(i; acc=... | if acc.oclIsInvalid() then ... else ... endif) possible?
The root problem of this bug is the same as for 342644, with the same solution. Closing this as a duplicate of the other where the latest patch has been attached just now. *** This bug has been marked as a duplicate of bug 342644 ***
The symbolic analysis for bug 520440 motivates avoiding crashes (invalid) for operations that canreasonably be used as queries or whicg would require unwarranted programmer effort to guard againstthe crash. The failure return from toInteger() is therefore migrating to null in the Pivot-based OCL.
