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 10:29:47 GMT


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

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

Mime
View raw message