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
