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 <blackd...@gmx.org> wrote: > > > On 22.07.2016 01:30, pa...@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.
