GString's equals and compareTo behavior is not equivalent

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Blocker Blocker
  • Resolution: Won't Fix
  • Affects Version/s: 1.1-rc-3
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

def g = 'g'
def string = 'groovy'
def gString = "${g}roovy"
println gString.equals(string) // false
println gString == string // true

For classes implementing the Comparable interface, the result of == is based on the value of the compareTo method, not on the result of the equals method.

Beside the fact that gString.equals(string) not being true for the example above is unintuitive and leads to obvious problems with collections, the fact the compareTo and thus gString == string behaves differently is a BIG problem. It does not follow the strong recommendation of the Comparable interface (http://java.sun.com/j2se/1.5.0/docs/api/) which can lead to big problems.

Here is the code from the GString class.

//   
    public boolean equals(Object that) {
        if (that instanceof GString) {
            return equals((GString) that);
        }
        return false;
    }

    public boolean equals(GString that) {
        return toString().equals(that.toString());
    }

    public int compareTo(Object that) {
        return toString().compareTo(that.toString());
    }

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Hans Dockter added a comment - 
def g = 'g'
def string = 'groovy'
def gString = "${g}roovy"

SortedSet set = new TreeSet()
println set.add(string) // true
println set.add(gString) // false
println set.size() // 1
println set // ["groovy"]

In my groovy application I uses TreeSet and TreeMap a lot. Obviously this is nothing exotic.

Show
Hans Dockter added a comment - 
def g = 'g'
def string = 'groovy'
def gString = "${g}roovy"

SortedSet set = new TreeSet()
println set.add(string) // true
println set.add(gString) // false
println set.size() // 1
println set // ["groovy"]
In my groovy application I uses TreeSet and TreeMap a lot. Obviously this is nothing exotic.
Hide
Permalink
blackdrag blackdrag added a comment -

Hans, change your program to this

def g = 'g'
def string = 'groovy'
String gString = "${g}roovy"

SortedSet set = new TreeSet()
println set.add(string) // true
println set.add(gString) // false
println set.size() // 1
println set // ["groovy"]

and it will work

Show
blackdrag blackdrag added a comment - Hans, change your program to this 
def g = 'g'
def string = 'groovy'
String gString = "${g}roovy"

SortedSet set = new TreeSet()
println set.add(string) // true
println set.add(gString) // false
println set.size() // 1
println set // ["groovy"]
and it will work
Hide
Permalink
Paul King added a comment -

We should fix up the CCE for 1.1:

def g = 'g'
def string = 'groovy'
def gString = "${g}roovy"

def set = new TreeSet()
println set.add(gString) // true
println set.add(string)  // throws CCE
Show
Paul King added a comment - We should fix up the CCE for 1.1: 
def g = 'g'
def string = 'groovy'
def gString = "${g}roovy"

def set = new TreeSet()
println set.add(gString) // true
println set.add(string)  // throws CCE
Hide
Permalink
blackdrag blackdrag added a comment -

but Paul, look at the first lines of the trace:

at java.lang.String.compareTo(String.java:109)
	at java.util.TreeMap.put(TreeMap.java:562)
	at java.util.TreeSet.add(TreeSet.java:255)

The method throwing the exception is String#compareTo. We have no influence on this, unless we don't put a GString in the Set. And I don't think that is something for a 1.1, because that would be a major change.

Show
blackdrag blackdrag added a comment - but Paul, look at the first lines of the trace: 
at java.lang.String.compareTo(String.java:109)
	at java.util.TreeMap.put(TreeMap.java:562)
	at java.util.TreeSet.add(TreeSet.java:255)
The method throwing the exception is String#compareTo. We have no influence on this, unless we don't put a GString in the Set. And I don't think that is something for a 1.1, because that would be a major change.
Hide
Permalink
Paul King added a comment -

Of course, I should have looked more closely. This is the same error the Java gives, so is what we expect for 1.1. It might be nice to have a safeAdd() method (either by default or with a specially named method) at some point but not in scope for 1.1.

Show
Paul King added a comment - Of course, I should have looked more closely. This is the same error the Java gives, so is what we expect for 1.1. It might be nice to have a safeAdd() method (either by default or with a specially named method) at some point but not in scope for 1.1.
Hide
Permalink
David Smiley added a comment -

This is set as "won't fix" yet the "fix-for" is 1.6 (which hasn't been released). Judging from the comment thread, it seems this should be in an unresolved state (i.e. it should be fixed).

Show
David Smiley added a comment - This is set as "won't fix" yet the "fix-for" is 1.6 (which hasn't been released). Judging from the comment thread, it seems this should be in an unresolved state (i.e. it should be fixed).
Hide
Permalink
Paul King added a comment -

Guillaume, I reopened this as I wasn't sure what our intention was. If it is meant to be closed, we probably should at least state why we aren't going to fix it.

Show
Paul King added a comment - Guillaume, I reopened this as I wasn't sure what our intention was. If it is meant to be closed, we probably should at least state why we aren't going to fix it.
Hide
Permalink
blackdrag blackdrag added a comment -

I am still for won't fix. That "fix.for" was set to 1.6 is probably a mistake. The remaining task was to fix the CCE, but as long as we can not replace String#compareTo, there is no way of "fixing" this issue. Here are other bugs targeting 2.0 for changing the dependency on compareTo, I don't think we need this one as well.

Show
blackdrag blackdrag added a comment - I am still for won't fix. That "fix.for" was set to 1.6 is probably a mistake. The remaining task was to fix the CCE, but as long as we can not replace String#compareTo, there is no way of "fixing" this issue. Here are other bugs targeting 2.0 for changing the dependency on compareTo, I don't think we need this one as well.
Hide
Permalink
Paul King added a comment -

I have no problems with leaving as won't fix either, just wanted the reasons recorded so people realise that further queries down the track belong on other issues.

Show
Paul King added a comment - I have no problems with leaving as won't fix either, just wanted the reasons recorded so people realise that further queries down the track belong on other issues.

People

  • Assignee:
    Unassigned
    Reporter:
    Hans Dockter
Vote (0)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved: