== operator does not work anymore if Comparable is implemented!

I added some code text to make the issue more readable.

Then about the issue itself... we have here a case where the left side is implementing Comparable, but the right side is not... that also means that left and right implement different types, unrelated ones on top of that. Currently we have defined, that if the left side is a Comparable and the right side is not, that those two can never be equal. It is kind of strange to go to compareTo, but then to fall back to equals. On top of that a usual implementation of equals returns false if the types are not equal enough... comparing itself with an Integer in equals will lead in any normal java class to false.

If anything then I would mark this report as improvement, because you want to "advance" the current logic. But frankly I would like to know more about your use case, because you are doing things here, that are a bit "out of spec".

Also I wonder why the issue got this many votes, when I hear the first time about that part of the logic being a problem. So I guess they would have some reasons for having such a change and I would like to hear these

The use case is a DSL with "relaxed" type handling. Our DSL user is a power user, but does not have programming skills. Therefore I don't want him to care about type convertions, e.g. "x > 50" should work if x is a String that smells like a Number. My first approach was to have some kind of "AutoType" class that wraps around another type and provides the conversions on the fly by overriding various operators. Currently I can either implement Comparable or override equals - but not both ...

By the way - The Java documentation does not require Comparable.compareTo to correspond with equals - here is a snippet from the Comparable.compareTo JavaDoc:

"It is strongly recommended, but not strictly required that (x.compareTo==0) == (x.equals). Generally speaking, any class that implements the Comparable interface and violates this condition should clearly indicate this fact. The recommended language is "Note: this class has a natural ordering that is inconsistent with equals."

first of "x > 50" is not "x == 50" and even if that works, what about "50 < x"? Once the order is reversed you will fail with your DSL... and not only with compareTo, but with equals too.

The operator logic is still a open problem I will try to access in 2.0, but till then it might be more easy if you do your own thing here. You could hook into the compiler and reaplace every BinaryExpression for <=>,<,<=,==,>=,> and != with your own method and so bypass all the stuff Groovy is doing.

let's ignore my use case for the moment - I still stick to my opinion that this issue is a bug and not an improvement. Java defines equality through equals. It is only recommended that Comparable provides the same equality logic, but not required. Therefore == should use equals and not compareTo. Maybe this issue should be set as duplicate to #GROOVY-2334 since the resolution of #GROOVY-2334 would resolve this issue too.

If I mark this as duplicate of GROOVY-2334 then this issue won't be resolved before Groovy 2.0 and we won't try to find an alternative solution anymore.

You wrote: "Java defines equality through equals. It is only recommended that Comparable provides the same equality logic, but not required. Therefore == should use equals and not compareTo."

Wouldn't you find it strange that <,<=,=>,>,<=> all use compareTo, but == does not? Normally you assume that x<=y & x>=y implies x==y. But if equals and compareTo differ for (x,y), then this won't be the case.

I absolutely agree with you and I understand your catch-22. If I were to decide, I'd stick with Java and would use equals() for ==. The Java developer's mindset for equality is equals(). If you change this contract than Java objects behave differently in Groovy, and even within Groovy there are some other strange side effects - in this example, the second assertion will fail (because BigDecimal has a natural ordering that's inconsistent with equals):

def v1 = 1.25
def v2 = 1.250
def values = [v1]
assert v1 == v2
assert values.contains(v2)

Inconsistent natural ordering is a rare case - the only really prominent example I know is BigDecimal (see javadoc of Comparable and BigDecimal) - and since Groovy makes heavy use of BigDecimals this has some potential to break existing code!

For the meantime I'd apply the following patch because neither Object.equals() nor Comparable.compareTo() require the object to be compared to be of a special type or to be within the type's hierarchy (see JavaDoc). Here is a snippet of DefaultTypeTransform.compareToWithEqualityCheck:

        ...
            if (!equalityCheckOnly || left.getClass().isAssignableFrom(right.getClass())
                    || right.getClass().isAssignableFrom(left.getClass())
                    || (left instanceof GString && right instanceof String)) {
                Comparable comparable = (Comparable) left;
                return comparable.compareTo(right);
            }
        }

        if (equalityCheckOnly) {
            return -1; // anything other than 0
        }
        throw new GroovyRuntimeException("Cannot compare " + left.getClass().getName() + " with value '" +
                left + "' and " + right.getClass().getName() + " with value '" + right + "'");

Currently the various compare-operators do not catch the behavior of all Java classes (especially for Java classes that want to be comparable to other types). There are cases (such as mine) where I really want this behavior (e.g. think of some kind of relaxed type comparison adapter). A quick patch could look like this:

...
try {
  Comparable comparable = (Comparable) left;
  return comparable.compareTo(right);
} catch(ClassCastException ex) {
  if(equalityCheckOnly)
    return ((Boolean) InvokerHelper.invokeMethod(left, "equals", right)).booleanValue() ? 0 : -1;
  throw new GroovyRuntimeException("Cannot compare " + left.getClass().getName() + " with value '" +
    left + "' and " + right.getClass().getName() + " with value '" + right + "'");
}

This would keep your current Groovy contract to use compareTo() if the object is Comparable and use equals() otherwise. This will however, not solve the BigDecimal example shown above and still is inconsistent with Java equality. If currently affected Groovy code should be brought to a minium (I'd guess 99% unaffected), then you have to replace the InvokeHelper call with a return of -1.

I think we both agree, that Comparable and equals are not a perfect fit to express all the operations we want to do with them. One problem here is if the given types differ. now javadoc for Comparable says

This interface imposes a total ordering on the objects of each class that implements it. This ordering is referred to as the class's natural ordering, and the class's compareTo method is referred to as its natural comparison method.

This natural order implies that the objects compare are of the same class in some sort of way or that the operation will cause an exception. Throwing an exception means the operation is not defined, so we are talking here about a partial function when you look at all possible values.
the goal is to get a function where the signum of x.compareTo(y) is the revers version of y.compareTo(x). So if x.compareTo(y) throws an exception, y.compareTo(x) should do the same or you break symmetry big times. That includes for example that x.compareto(null) in java has to throw a NPE, because null.compareTo(x) would do that. This also implies that if x and y are of different types and if x does not know the type of y in any way, that x.compareTo(y) must throw an exception, since both types are not comparable. Which then again implies that y.compareTo(x) must throw an exception too, even if y would know x. And that is a difference from equals, which can always return false then. And the common way to ensure this is to throw a ClassCastException.
So even if the javadoc does not say you have to use the same type, if you want to fulfill their implications, you have to do that. In the jdk source you will most probably not find an example that allows you to compare two instances of different unrelated classes. I mean they had their reason to define Comparable as "interface Comparable<T>"

Now or your patch... true, compareTo might throw a ClassCastException, but who ensures, that is not due to a programming error in a method called from there somewhere deep down the stack? In fact we don't know if the exception is thrown as part of the contract for Comparable, or because of a different reason. So the danger is that we hide a problem, leading to endless debugging sessions and angry users later.

Also your assumption may be correct, that most code won't be affected by this change, but only if they don't depend on the exception being thrown. So your patch builds on what you expect others not to do?

You are right - Catching the ClassCastException is not a good idea because there may be code that relies on the exception. BTW: I've written the patch in a way with the intention of altering existing behavior as less as possible! Note that if there is code relying on a ClassCastException to be thrown directly by compareTo (and not down the stack) - the chances that this ClassCastException is redirected to the client is nearly 0% (due to the isAssignableFrom() checks that are done before) - In this case the client would receive a GroovyRuntimeException.

At the moment I don't see a problem with all the stuff you've written above. Sure - In Java I'd have the problem that I cannot assure that x.compareTo(y) is the revers version of y.compareTo(x) if I do not have control of both classes. However, it's Groovys strength to do such things ... I can add the required methods as I like. And I can implement Comparable<Object> if I want to.

If you want the original exception to be delivered, then the current implementation needs to be changed (the isAssignableFrom() checks need to be eliminated and a possible ClassCastException is delivered from the compareTo method itself). You could remove the equalityCheckOnly stuff and just rely on the compareTo method. This has the side-effect that the "==" operator may throw a ClassCastException, however, since your current contract is to use Comparable if implemented, this is ok. This would be a better approach than my patch is - however - it changes behavior.

are you sure about the GroovyRuntimeException? These are usually carriers of real exceptions and they get unwrapped once you enter Groovy code again. Before 1.0 Groovy had a very bad tradition of swallowing exceptions and it took really a huge effort to remove all those black holes. So you may understand, that I am not eager to add a new hole.

It is very well possible that due to a programming error a ClassCastException will occur. For a programming language it is very important to not to swallow such exceptions. Because that will mean the user will not see the place of error, instead the application will just react strangely. That is really a hell to debug.

Because we check with isAssignableFrom it is even more a high probability, that we have a programming error if this exceptions happens.

what's your proposal to resolve the issue ?

I don't think this is something we can solve before 2.0, so I am setting a new fix version