groovy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul King <pa...@asert.com.au>
Subject Re: groovy git commit: GROOVY-7876: ClassCastException when calling DefaultTypeTransformation#compareEqual (closes #368)
Date Fri, 22 Jul 2016 11:17:23 GMT
Happy to change if there is indeed a problem but I still can't see an
issue myself.
Perhaps I need examples. More comments below. But I do agree the code
could be made
easier to read - the double negatives don't help.


On Fri, Jul 22, 2016 at 8:29 PM, Jochen Theodorou <blackdrag@gmx.org> wrote:
>
>
> On 22.07.2016 01:30, paulk@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)) {

Which part could throw a ClassCastException above?

> 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.

This is how Groovy has always done equality for comparables, so the or
as it stands is correct - presumably this is to handle for instance BigDecimals
compared with equal valued non-BigDecimals or two BigDecimals with the
same value but two different scales. So we can change this but we'd need a
separate branch of code to cover that case. Agree though that the double
negative naming for the boolean is confusing.

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

Agree if your above statement was true but see my comments above.

> 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))

No, I don't think this represents the current behavior.

When we are doing a legitimate compareTo (equalityCheckOnly = false), there will
be existing code that expects the CCE from DTT.compareToWithEqualityCheck, e.g.
even in the Groovy code base ObjectRange#L198 calls SBA.compareGreaterThan
which calls down to DTT.compareToWithEqualityCheck. Indeed the issue examples
highlights that Java does typically return CCE when compareTo is used with an
incorrect class (two different enums in the example).

When we are just using compareTo for equality (equalityCheckOnly = true) that's
when we don't want the CCE to leak out - they just can't be equal in that case.
The isAssignable checks etc. were just trying to avoid the CCE ahead of time
but we can't do that in all cases, e.g. Holder/Container classes -
which won't be
possible to check in many cases anyway due to erasure.

> What do you guys think?

What is there seems the best we can do - though it could no doubt be
made much clearer if given some tlc. But happy for further comments -
perhaps I am missing something.

> bye Jochen

Cheers, Paul.

Mime
View raw message