I think the GString check can be moved up as you suggest to improve
the readability of the code without breaking existing behavior, so
I've done that.
Whether to go with your other suggestions is less clear to me.
If we assume the check in EqualsTest#testParentChildrenEquals remains
valid, i.e. we want symmetry of equals for java.util.Date and
java.sql.Time which compareTo gives us so long as we have both the
left<<right and right<<left isAssignable checks, then the current code
in master seems reasonable. And once we have the right<<left case, we
need the Object check as well for GROOVY-4046.
But having said that, it is unclear to me whether there are many other
parts of the Java libraries (or user code for that matter) which have
similar behavior or if this just a broken aspect of the Date family of
classes which are now mostly superseded by Java 8's Date-Time package
(java.time). We know for instance that java.sql.Timestamp remains
broken even when attempting our isAssignable checks etc. and indeed
used to throw CCE in pre Java 1.6 versions.
I guess the underlying assumption in the existing code is that for
Comparable classes, the compareTo method is more likely to provide a
"business/human" friendly equality check than equals (since the
contracts around equals behavior have more specific strict
requirements), as illustrated by e.g. BigDecimal. The question is
then, do we want to continue honoring this assumption or provide that
only for the specific number widening that we do anyway.
So, if we want to change the code to something like:
if (equalityCheckOnly) {
return left.equals(right) ? 0 : -1;
} else {
return ((Comparable) left).compareTo(right);
}
then something like the following could no longer be made to work:
class NumberHolder implements Comparable<NumberHolder> {
Number n
int compareTo(NumberHolder other) {
n <=> other.n
}
}
assert new NumberHolder(n: 10L) == new NumberHolder(n: 10.0G)
For me, such a breaking change sounds more like a 3.0 thing.
Cheers, Paul.
On Fri, Jul 22, 2016 at 11:18 PM, Jochen Theodorou <blackd...@gmx.org> wrote:
>
>
> 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
>
>
>
