[evaluator] The EvaluationVisitor implementation needs a refactoring

[eclipse-ocl-bot]

| --- | --- | | Bugzilla Link | 291623 | | Status | CLOSED FIXED | | Importance | P3 enhancement | | Reported | Oct 07, 2009 10:55 EDT | | Modified | May 29, 2012 13:24 EDT | | Version | 1.3.0 | | Blocks | 318368 | | Reporter | Laurent Goubet |

Description

Created attachment 149001\ proposed refactoring

The EvaluationVisitorImpl class as it stands now (specifically, its\ "visitOperationCallExp()" method) is barely maintainable, and it cannot be\ understood by ordinary humans. Not mentionning that some of the implementation\ choices made in there prevent fixing some of the bugs highlighted by bug 287977\ such as "'null + 3' returns 'null' instead of 'invalid'". I also believe\ "extensibility" bugs such as bug 291220 won't be resolvable without first\ properly separating the code for each standard operations from the rest.

I think we all agree in saying that this method needs a good refactoring so as\ to be maintainable. This bug is mostly opened for discussions as to "how" we\ could refactor it for the best. The attached patch is a first attempt I made\ which, even I don't find it satisfactory, dispatches the evalution of each\ standard operation in its own method (and thus each can be\ debugged/overriden/understood standalone) and drastically reduces the\ complexity of "visitOperationCallExp()" from 14 nested switches with a random\ number of "case"s for a single operation code to 4 non-nested switches with\ only one "case" for a given operation code (at the exception of the ambiguous\ "minus" operation).

Keep in mind that I did not format this patch so as to minimize the number of\ differences ... and that still makes a lot. Also, even if I did refrain from\ fixing any implementation bug with this patch, the very nature of this\ refactoring solves 20 or so failures from the test suite attached to bug 287977\ as operation calls on null and invalid are now properly resolved to the\ accurate operation ("null + 3" is now invalid instead of null).

Back to this bug's purpose : I explored some potential ways to refactor the\ visitor, and didn't managed anything more than option 1) I described on\ http://dev.eclipse.org/mhonarc/lists/mdt-ocl.dev/msg00218.html .

I stopped trying to strive towards 2) or 3) as I didn't want to instantiate\ sub-visitors each time we entered the evaluation of an operation, and some of\ these operations need an access to the visitor or its delegate to be evaluated\ (examples of such operations as the boolean "and", "or" and "implies" which are\ shortcut and don't always need their argument evaluated).

I also tried to create a factory that could create "VisitableOperations" all\ with their own "evaluate()" method so as to totally remove the switch on\ operation code from the EvaluationVisitor ... yet this ended up only moving the\ complexity (that corresponds roughly to approach #4 Alexander mentionned on the\ mailing list, yet I didn't go as far as linking the code to an EOperation).

All in all, I believe the proposed patch could be comitted as a starting point\ for the refactoring as it already starts improving the whole thing without\ hindering neither performance (in fact, it improves performance) nor evaluation\ (all tests still pass, along with tests attached to bug 287977 that passed\ beforehand). Once this separation of evaluation code from operation dispatching\ logic done, we can start thinking of how to enable dynamic operation\ dispatching within OCL and fix the evaluation failures we still have (as well\ as improve performance by stopping using exceptions and checking beforehand,\ look at the implementation of the substring operation for an example of this).

:notepad_spiral: OCLEvaluationVisitor_draft.patch

[eclipse-ocl-bot]

By Alexander Igdalov on Oct 07, 2009 13:44

It is easy to get lost in all the changes. I hope you know what you do;)\ But my +1 for committing an interim patch that simplifies the code like this.\ However, I don't understand you way of removing the lax null option. Namely, I don't understand the code:

protected Object visitOclIsKindOf(Object sourceVal, Object argVal) { ...\ // OclVoid and Invalid conform to all classifiers; we know we're in case of lax\ // null handling, return TRUE.\ if (isUndefined(sourceVal)) {\ return Boolean.TRUE;\ } ...\ }

We should return invalid here, or am I missing something?\ There is a similar place in visitOclIsTypeOf().

It seems you want to get rid of the lax null evaluation option. If so then its usages should be removed from EvaluationOptions and other places.

[eclipse-ocl-bot]

By Ed Willink on Oct 07, 2009 15:12

I think that's a big improvement and we can do better still. Eliminate the\ switch completely. (+1 if you find this interim step helpful).

If OCLStandardLibrary provides a map of Operation -> IOperationVisitor

where

interface IOperationVisitor {

public Object visit(EvaluationVisitor<PK, C, O, P, EL, PM, S, COA, SSA, CT,\ CLS, E> evalVisitor, OperationCallExp<C, O> oc);

}

then we get the very simple

public Object visitOperationCallExp(OperationCallExp<C, O> oc) {\ O oper = oc.getReferredOperation();\ IOperationVisitor opVisitor =\ getOCLStandardLibrary().getOperationVisitor(oper);\ if (opVisitor != null)\ return opVisitor.visit(this, oc);\ // error handling\ }

A variety of layers of AbstractOperationVisitor and NumericOperationVisitor can\ provide shared functionality, argument checking, argument conversion, ...

This allows extended standard libraries to just more referredOperation\ handlers, and eliminate/replace those they dislike, and extend those they want\ to refine.

The slightly tedious bit is the initialisation of the OCLStandardLibrary with\ each referredOperation handler. After that initialisation, it might be possible\ to do a consistency check between the stdlib operation declarations and the\ dynamically installed operation visitors. Perhaps using

IOperationVisitor.validate(O operation)

where the derived implementation's argument count and type definitions are\ checked against the meta-model.

A TypeSpecificOperationVisitor might support a further dispatch based on the\ type.

I recommend moving this code to o.e.o.evaluator with concrete operations in\ e.g. o.e.o.evaluator.operations.ToString.java where a stateless\ ToString.INSTANCE avoids creating more than one instance of each handler.

[eclipse-ocl-bot]

By Ed Willink on Oct 07, 2009 15:36

Replying to #1.

The lax null handling seems much more sensible; probably need an OCL issue. Java allows (MyClass)null so why doesn't OCL? We need to handle it. The comment in EvaluationOptions needs updating.

All functionality in the EvaluationVisitor is complicated by the lack of a distinct OCL null from a Java null, which may arise through poor implementation code.

[eclipse-ocl-bot]

By Laurent Goubet on Oct 08, 2009 04:49

(In reply to comment #1)

protected Object visitOclIsKindOf(Object sourceVal, Object argVal) { ... // OclVoid and Invalid conform to all classifiers; we know we're in case of lax // null handling, return TRUE. if (isUndefined(sourceVal)) { return Boolean.TRUE; } ... }

We should return invalid here, or am I missing something? There is a similar place in visitOclIsTypeOf().

It seems you want to get rid of the lax null evaluation option. If so then its usages should be removed from EvaluationOptions and other places.

Nope, I did my best to reproduce the previous behavior. Here is what was done previously :

line 498 of the EvaluationVisitorImpl :\ if (isUndefined(sourceVal)) {\ switch (opCode) {\ case PredefinedType.OCL_IS_TYPE_OF:\ case PredefinedType.OCL_IS_KIND_OF:\ case PredefinedType.OCL_AS_TYPE:\ if (isLaxNullHandling()) {\ break;\ } else {\ return getInvalid();\ }\ }\ }

And later on :

line 483 of the AbstractEvaluationVisitor :\ if (isUndefined(value)) {\ return isLaxNullHandling() ? Boolean.TRUE : null;\ }

You'll notice the redudant check of isLaxNullHandling(), but the behavior is there : if the source is undefined and we're not in lax null handling, return invalid, else, return true.

My code reproduces the same behavior as I also check beforehand and shortcut the evaluation if we're in lax null handling mode (that's the only shortcut logic left within the switch with my patch).

(In reply to comment #3)

The lax null handling seems much more sensible; probably need an OCL issue. Java allows (MyClass)null so why doesn't OCL? We need to handle it. The comment in EvaluationOptions needs updating.

OCL also allows it : null.oclAsType(MyClass) returns "null" when lax null handling is on, and it is on by default.

Back to this patch; I indeed feel it would be better to commit this interim patch before trying to tackle the bug fixing and the further improvement of the visitor; what Ed proposed in comment #2 is what I tried to achieve ... but couldn't because of the importance of the refactoring : as I mentionned, I was simply moving the complexity instead of improving the design.

When commited, this interim patch will also ease the reviewing process of further improvements. As Alex said, there is already too many changes in this "simple" patch to properly review it (and that is further demonstrated by the confusion with my handling of the "LAX_NULL_HANDLING" option). I won't be able to come back on commiting it before the end of november, If there is no "-1" until then, I'll commit it and start fixing the current bugs before striving towards Ed's proposition (comment #2).

[eclipse-ocl-bot]

By Alexander Igdalov on Oct 08, 2009 05:18

(In reply to comment #3)

Replying to #1.

The lax null handling seems much more sensible; probably need an OCL issue. Java allows (MyClass)null so why doesn't OCL? We need to handle it. The comment in EvaluationOptions needs updating.

Sounds reasonable. My +1 to deviating from the spec here.\ IOW:

1. oclAsType(), oclIsKindOf(), oclIsTypeOf() invoked on invalid must return invalid regardless of the operation argument (even if it is OclInvalid). This is done in accordance with the spec.

2. oclIsKindOf(), oclIsTypeOf() invoked on null must return invalid regardless of the operation argument (even if it is OclVoid or OclInvalid). This is done in accordance with the spec.

3.\ null.oclAsType(OclInvalid) -- returns invalid\ null.oclAsType(SomeTypeNotOclInvalid) -- returns null // the only deviation

---

Concerning (2), we might think of oclIsKindOf() as a synonym of Java's instanceof operator. However, in OCL null.oclIsKindOf(...) returns invalid. In Java null instanceof ... returns false. Here I am not inclined to deviate from the OCL specification in order to be aligned with Java. What do you think?

[eclipse-ocl-bot]

By Laurent Goubet on Oct 08, 2009 05:24

Alexander, Ed,

I'd rather move the discussion about the behavior of oclAsType, oclIsKindOf and oclIsTypeOf with null/invalid source/argument to another bugzilla : this bug's purpose is already large enough as it stands.

[eclipse-ocl-bot]

By Ed Willink on Oct 08, 2009 06:26

Language discussion moved to bug 291721.

[eclipse-ocl-bot]

By Ed Willink on Mar 14, 2011 16:45

Pivot Evaluator is one class per feature.

[eclipse-ocl-bot]

By Ed Willink on May 27, 2011 06:41

Resolved for Indigo is 3.1.0 not 3.2.0.

[eclipse-ocl-bot]

By Ed Willink on May 29, 2012 13:24

Closing all bugs resolved in Indigo.