groovy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jochen Theodorou <blackd...@gmx.org>
Subject Re: groovy git commit: GROOVY-7876: ClassCastException when calling DefaultTypeTransformation#compareEqual (closes #368)
Date Fri, 22 Jul 2016 13:18:01 GMT


On 22.07.2016 13:17, Paul King wrote:
[...]
> Which part could throw a ClassCastException above?

the compareTo itself.

actually, let me think out loud here... We have two cases to consider, 
x==y (equalityCheckOnly==true) and x<=>y (equalityCheckOnly=false). In 
the first case we do not want an exception, in the later we my want to 
have whatever compareTo will give us. Actually the first thing I would 
do now is move the case (left instanceof GString && right instanceof 
String) up to the else-if cascade instead of having it in this 
condition. I would also reconsider the

right.getClass() != Object.class && 
right.getClass().isAssignableFrom(left.getClass())) //GROOVY-4046

part... because even if right is assignable from left... what does it give?

That leaves equalityCheckOnly and

left.getClass().isAssignableFrom(right.getClass())

Let me use left<<right for this to make it shorter in this mail. So in 
case of x==y we try to avoid the exception, thus:

if (equalityCheckOnly && left<<right) return left.compareTo(right)
return -1

In case of x<=>y, we may want the exception, but then it makes no sense 
to use the lefty<<right check, or does it?

if (!equalityCheckOnly) return left.compareTo(right)

So we could either do:

if (equalityCheckOnly) {
   if (left<<right) return left.compareTo(right)
   return -1
} else {
   return left.compareTo(right)
}

or..

if (!equalityCheckOnly || left<<right) {
   return left.compareTo(right)
}
return -1

which is now quite near the old code...only that we actually do not need 
that GroovyRuntimeException at all.

But this also means there should not have been a need for your change... 
which let's me focus a bit on this strange condition added for 
Groovy-4046...

> assertFalse(new Object() == 1) // this is ok
> assertFalse(1 == new Object()) // this throws a ClassCastException, because Groovy redirects
the call to java.lang.Integer.compareTo(Integer i)

if left is an Object and right is Integer, then we never even get to the 
condition we are talking about, because left is no Comparable. And they 
will be seen as not equal... so this is correct. If left is an Integer 
and right is an Object, then equalityCheckOnly is true, 
left.getClass().isAssignableFrom(right.getClass()) should be false, 
(left instanceof GString && right instanceof String)  should be false,
(right.getClass() != Object.class && 
right.getClass().isAssignableFrom(left.getClass())) would have been 
true, if right would be something else... Thought the if would have 
worked without that condition part as well.

> enum MyEnum
> { A, B, C }
> assertFalse(new Object() == MyEnum.A) // this is ok
> assertFalse(MyEnum.A == new Object()) // this throws a ClassCastException, because Groovy
redirects the call to java.lang.Enum.compareTo(E e) where E extends java.lang.Enum<E

Here again left is not Comperable so the condition is skipped. Enum is 
comparable, but the exact same reasoning as above applies.

So imho for Groovy-4046 we no longer need that condition part. And if I 
remember right, that is related to me adding the 
left.getClass().isAssignableFrom(right.getClass()) later on. Didn't 
check the history, so I might be wrong about this and actually messed-up 
when I added that part.

Anyway... let us look at groovy 7876

>     enum E1 {A, B, C}
>     enum E2 {D, E, F}
>
>     def "test groovy oddness"() {
>         when:
>         def test = DefaultTypeTransformation.compareEqual(
>             Tuples.pair(E1.A, 1),
>             Tuples.pair(E2.D, 1))
>
>         then:
>         assert test == false
>     }

The issue here is not about E1 or E2. it is about the class returned by 
Tuples.pair. If I read 
https://www.eclipse.org/collections/javadoc/7.1.0/org/eclipse/collections/api/tuple/Pair.html

right, then I must say I think the generics for this class are simply 
wrong. T2 must extend T1, or compareTo is not valid.

This also means, the exception is not because of us doing something, it 
is because that class doing something. And I am frankly very much 
against hiding a ClassCastException, that is not our fault. If I had 
seen the issue earlier I would have closed it as won't fix actually. We 
can never know why the ClassCastException is thrown. It might be because 
of an programming error for example.

I think equals would have worked here.... which goes back to that old 
discussion about usage of equals vs. compareTo for ==

And I am actually thinking we should resolve that... in the following 
manner:

we still do our widening checks for numbers, and the special codes for 
String, GString and Character, but otherwise we should call equals.

That could for example make that code place into:

> if (equalityCheckOnly) {
>   return left.equals(right) // static or dynamic?
> } else {
>   return left.compareTo(right)
> }

(plus the GString part in the else-if-cascade of course) which would 
solve many surprises and simplifies the code a lot as well. Of course 
that would be breaking behavior to some extend, so surely not for 2.4.x. 
But in general we should discuss about using that

bye Jochen




Mime
View raw message