Bug 342561

Comment 1 Axel Uhl 2011-04-12 09:33:37 EDT

(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"...

Comment 2 Axel Uhl 2011-04-12 10:17:01 EDT

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.

Comment 3 Axel Uhl 2011-04-12 10:26:18 EDT

(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?

Comment 4 Axel Uhl 2011-04-12 11:43:31 EDT

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.

Comment 5 Axel Uhl 2011-04-12 11:54:19 EDT

Created attachment 193060 [details]
Added missing @since

Comment 6 Ed Willink 2011-04-12 12:00:26 EDT

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?

Comment 7 Axel Uhl 2011-04-12 17:36:13 EDT

(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?

Comment 8 Axel Uhl 2011-04-13 05:08:03 EDT

See https://bugs.eclipse.org/bugs/attachment.cgi?id=193134 which is a cumulative patch also fixing this problem.

Comment 9 Axel Uhl 2011-04-13 05:23:23 EDT

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?

Comment 10 Axel Uhl 2011-04-13 12:12:35 EDT

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 ***

Comment 11 Ed Willink 2021-08-08 09:02:09 EDT

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.