Dead code in Java2QVTTypeResolver

[eclipse-qvt-oml-bot]

| --- | --- | | Bugzilla Link | 425070 | | Status | ASSIGNED | | Importance | P3 normal | | Reported | Jan 08, 2014 04:14 EDT | | Modified | Apr 04, 2014 05:17 EDT | | Version | 3.4 | | Reporter | Christopher Gerking |

Description

Class Java2QVTTypeResolver contains the following code. Can it be true that the else clause is dead code, because even if rawType == List.class, the if clause would be executed?

if(List.class.isAssignableFrom(rawClass)) { ...\ }\ else if(rawType == List.class) { ... \ }

By the way, in the affected method, I do not understand why there is sometimes a check via isAssignableFrom (like in the above if clause) and sometimes only plain class comparison (like in the above else clause). Shouldn't every case be handled by checking via isAssignableFrom()?

[eclipse-qvt-oml-bot]

By Christopher Gerking on Apr 03, 2014 20:44

The concerning code is really problematic. A Java declaration such as

@Operation(contextual = true, kind = Kind.HELPER)\ public static SortedSet foo(SortedSet self)

resolves to

helper Set(EClass) :: foo() : Set(EClass).

However, when calling foo(), the actual parameter is most likely not a SortedSet, because OCL uses another Set implementation internally (namely HashSet). Thus, the blackbox method invocation fails with an IllegalArgumentException: argument type mismatch.

The same problem occurs when a Sequence parameter gets mapped to some List subtype other than ArrayList (such as e.g. Stack).

I suggest to get rid of all calls to 'isAssignableFrom' in Java2QvtTypeResolver. This would allow substitution of an OCL/QVT type only in case of exact type match with the class used internally (HashSet, ArrayList). Of course we could further refine this to allow more general parameter and more specific return types, but the required distinction seems too much of an effort. Legacy user code is a problem, though.

[eclipse-qvt-oml-bot]

By Ed Willink on Apr 04, 2014 01:57

The original comments

Yes. Surely rawType == List.class is dead, since rawType == rawClass? Put a debug bomb in the dead code and run the test cases.

Yes isAssignableFrom should be used more generally. The final == Collection.class is surely wrong since Collection is abstract.

The Bag/LinkedHashSet == tests mirror the inflexibility of OCL but\ == Bag.class is wrong since the inflexibility is BagImpl.class.

---

IMHO tight coupling between OCL/QVTo types and Java types should be avoided so that the underlying Java code has considerable freedom in its use of Collection types.

@Operation(contextual = true, kind = Kind.HELPER)\ public static SortedSet foo(SortedSet self)

causes a tight coupling, so I think that the arguments should be converted to the Java type if they are not already assignable to the required type.

In the reverse direction a conversion may also be necessary, particularly to LinkedHashSet since CollectionUtil is quite fussy. (The Pivot variant of OCL will do this cobnversion itself when it creates a boxed version to comply with OCL equality semantics.)

In so far as possible detection of types from values should be avoided since Java values do not carry unambiguous OCL type information. OCL/QVTo type information should be propagated and values converted where necessary to comply with the required types.

[eclipse-qvt-oml-bot]

By Christopher Gerking on Apr 04, 2014 05:08

(In reply to Ed Willink from comment #2)

Yes isAssignableFrom should be used more generally.

I don't think so anymore, because this leads to the above problem of OCL types being represented by unintended subclasses like e.g. SortedSet, which causes an IllegalArgumentException when calling the concerning blackbox methods.

Due to the fact that OCL internally uses HashSet/ArrayList as concrete types, the usage of isAssignableFrom enables specifying these types directly inside the declaration of a Java blackbox method:

public static HashSet foo(HashSet self)

works just as well as

public static Set foo(Set self)

I see no reason to allow this, but now that we have this functionality, we probably have to ensure legacy support. And as long as users do not use unspecified subclasses like SortedSet/Stack here, everything should work out.

The final == Collection.class is surely wrong since Collection is abstract.

Yes but this doesn't prevent it from being used inside a blackbox signature:

public static Collection foo(Collection self)

represents

helper Collection(EClass) :: foo() : Collection(EClass)

Thus, I think the code is correct.

== Bag.class is wrong since the inflexibility is BagImpl.class.

Same argumentation as above.

[eclipse-qvt-oml-bot]

By Christopher Gerking on Apr 04, 2014 05:17

Created attachment 241603 Java2QVTTypeResolver patch

The patch only gets rid of dead code.

The usage of isAssignableFrom is left for the Java Set/List classes. As stated above, it enables legacy support for declarations that directly use the more concrete HashSet/ArrayList classes. Of course this doesn't prevent problems with other unanticipated classes like SortedSet, Stack etc. However, these problems only apply to users that do not stick to the specified Java<->QVT type mapping rules, so IMO not a bug.

:notepad_spiral: Java2QVTTypeResolver.patch