Re: groovy git commit: GROOVY-7876: ClassCastException when calling DefaultTypeTransformation#compareEqual (closes #368)

[Jochen Theodorou]

On 22.07.2016 01:30, pa...@apache.org wrote:
Repository: groovy
Updated Branches:
   refs/heads/master 858a6fd2e -> 9159673d6


GROOVY-7876: ClassCastException when calling 
DefaultTypeTransformation#compareEqual (closes #368)
[...]

http://git-wip-us.apache.org/repos/asf/groovy/blob/9159673d/src/main/org/codehaus/groovy/runtime/typehandling/DefaultTypeTransformation.java
----------------------------------------------------------------------
diff --git 
a/src/main/org/codehaus/groovy/runtime/typehandling/DefaultTypeTransformation.java
 
b/src/main/org/codehaus/groovy/runtime/typehandling/DefaultTypeTransformation.java
index a0d53a2..41484d8 100644
--- 
a/src/main/org/codehaus/groovy/runtime/typehandling/DefaultTypeTransformation.java
+++ 
b/src/main/org/codehaus/groovy/runtime/typehandling/DefaultTypeTransformation.java
@@ -582,7 +582,14 @@ public class DefaultTypeTransformation {
                      || (right.getClass() != Object.class && 
right.getClass().isAssignableFrom(left.getClass())) //GROOVY-4046
                      || (left instanceof GString && right instanceof String)) {
                  Comparable comparable = (Comparable) left;
-                return comparable.compareTo(right);
+                // GROOVY-7876: when comparing for equality we try to only 
call compareTo when an assignable
+                // relationship holds but with a container/holder class and 
because of erasure, we might still end
+                // up with the prospect of a ClassCastException which we want 
to ignore but only if testing equality
+                try {
+                    return comparable.compareTo(right);
+                } catch (ClassCastException cce) {
+                    if (!equalityCheckOnly) throw cce;
+                }
              }
          }


I think we have to change the change. We cannot rely on the exception, 
because the exception might have been thrown from inside.

            if (!equalityCheckOnly || 
left.getClass().isAssignableFrom(right.getClass())
                                   || (right.getClass() != Object.class && 
right.getClass().isAssignableFrom(left.getClass())) //GROOVY-4046
                                   || (left instanceof GString && right 
instanceof String)) {

Next I think there is a bug in this condition guarding the compareTo 
call. We do not want to call compareTo equalityCheckOnly is true. All 
the other conditions are further requirements, that are correctly 
chained by or, but  !equalityCheckOnly really needs to be false at this 
time already.

Then of course  if (!equalityCheckOnly) throw cce; makes no sense either

left.getClass().isAssignableFrom(right.getClass()) is supposed to cover 
the case this is about already. So I think the condition should be:

!equalityCheckOnly && ((left.getClass().isAssignableFrom(right.getClass())) ||
                       (right.getClass() != Object.class && 
right.getClass().isAssignableFrom(left.getClass()))  || //GROOVY-4046
                       (left instanceof GString && right instanceof String))


What do you guys think?


bye Jochen