Clojure

= on sorted collections with different key types incorrectly throws

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.5, Release 1.6
  • Fix Version/s: Release 1.9
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

Comparing a sorted-set with numbers to a set with keywords is not symmetric:

user=> (= #{:a} (sorted-set 1))
false
user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword  clojure.lang.Keyword.compareTo (Keyword.java:109)

The latter case should return false instead of throwing.

Cause: APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check.

Proposed: It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.

See also: CLJ-1983 (downstream example with clojure.data/diff)

Patch: 0001-CLJ-1242-equals-doesn-t-throw-on-sorted-collections.patch

Screened by: Alex Miller

Activity

Hide
OHTA Shogo added a comment -

PersistentVector also has the same problem.

user=> (compare [1] [:a])
java.lang.ClassCastException: clojure.lang.Keyword cannot be cast to java.lang.Number

The cause of this problem is that Util.compare() casts the second argument
to Number without checking its type when the first argument is a Number.

Show
OHTA Shogo added a comment - PersistentVector also has the same problem. user=> (compare [1] [:a]) java.lang.ClassCastException: clojure.lang.Keyword cannot be cast to java.lang.Number The cause of this problem is that Util.compare() casts the second argument to Number without checking its type when the first argument is a Number.
Hide
OHTA Shogo added a comment -

Umm, my brain was not working right.
Util.compare() should raise an Exception when the arguments' type are different.

Show
OHTA Shogo added a comment - Umm, my brain was not working right. Util.compare() should raise an Exception when the arguments' type are different.
Hide
Fran├žois Rey added a comment -

Upvoting.
Here's a instance of this bug in codox:
https://github.com/weavejester/codox/issues/91

Show
Fran├žois Rey added a comment - Upvoting. Here's a instance of this bug in codox: https://github.com/weavejester/codox/issues/91
Alex Miller made changes -
Field Original Value New Value
Priority Minor [ 4 ] Major [ 3 ]
Approval Triaged [ 10120 ]
Hide
Stuart Halloway added a comment -

The behavior of get is consistent with Java collections, so I think changing that expectation should be considered a feature request and not a bug.

The fix for equals should be informed by the approach taken in the JDK, where the implementation of equals (not get) has exception catchers.

Show
Stuart Halloway added a comment - The behavior of get is consistent with Java collections, so I think changing that expectation should be considered a feature request and not a bug. The fix for equals should be informed by the approach taken in the JDK, where the implementation of equals (not get) has exception catchers.
Stuart Halloway made changes -
Priority Major [ 3 ] Minor [ 4 ]
Hide
Alex Miller added a comment -

I re-focused this ticket on just the equality aspect. The other request regarding `get` with a value of a different type is consistent with Java behavior and should be considered "as designed" - a separate enhancement ticket could be considered for that one.

user=> (def s (java.util.TreeSet.))
#'user/s
user=> (.add s 1)
true
user=> (.contains s "a")
ClassCastException java.lang.Long cannot be cast to java.lang.String  java.lang.String.compareTo (String.java:108)
Show
Alex Miller added a comment - I re-focused this ticket on just the equality aspect. The other request regarding `get` with a value of a different type is consistent with Java behavior and should be considered "as designed" - a separate enhancement ticket could be considered for that one.
user=> (def s (java.util.TreeSet.))
#'user/s
user=> (.add s 1)
true
user=> (.contains s "a")
ClassCastException java.lang.Long cannot be cast to java.lang.String  java.lang.String.compareTo (String.java:108)
Alex Miller made changes -
Summary get/= on sorted collections when types don't match result in a ClassCastException = on sorted collections with different key types incorrectly throws
Labels collections
Description user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword clojure.lang.Keyword.compareTo (Keyword.java:109)

but

user=> (= (sorted-set 1) :a)
false

also

user=> (get (sorted-set 1) :a 2)
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword clojure.lang.Keyword.compareTo (Keyword.java:109)
Comparing a sorted-set with numbers to a set with keywords is not symmetric:

{code}
user=> (= #{:a} (sorted-set 1))
false
user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword clojure.lang.Keyword.compareTo (Keyword.java:109)
{code}

The latter case should return false instead of throwing.

*Cause:* APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check. It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.
Alex Miller made changes -
Description Comparing a sorted-set with numbers to a set with keywords is not symmetric:

{code}
user=> (= #{:a} (sorted-set 1))
false
user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword clojure.lang.Keyword.compareTo (Keyword.java:109)
{code}

The latter case should return false instead of throwing.

*Cause:* APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check. It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.
Comparing a sorted-set with numbers to a set with keywords is not symmetric:

{code}
user=> (= #{:a} (sorted-set 1))
false
user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword clojure.lang.Keyword.compareTo (Keyword.java:109)
{code}

The latter case should return false instead of throwing.

*Cause:* APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check. It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.

*See also:* CLJ-1983 (downstream example with clojure.data/diff)
Alex Miller made changes -
Priority Minor [ 4 ] Major [ 3 ]
Alex Miller made changes -
Status Open [ 1 ] Closed [ 6 ]
Resolution Duplicate [ 3 ]
Alex Miller made changes -
Resolution Duplicate [ 3 ]
Status Closed [ 6 ] Reopened [ 4 ]
Hide
Alex Miller added a comment -

oops, sorry for the close/reopen.

Show
Alex Miller added a comment - oops, sorry for the close/reopen.
Hide
Rich Hickey added a comment -

Stu - link for "The fix for equals should be informed by the approach taken in the JDK, where the implementation of equals (not get) has exception catchers." ?

Show
Rich Hickey added a comment - Stu - link for "The fix for equals should be informed by the approach taken in the JDK, where the implementation of equals (not get) has exception catchers." ?
Rich Hickey made changes -
Approval Triaged [ 10120 ] Vetted [ 10003 ]
Alex Miller made changes -
Fix Version/s Release 1.9 [ 10750 ]
Hide
Alex Miller added a comment -

I don't think this change is good in its current location - should reconsider alternative impl based on the proposed suggestion.

Show
Alex Miller added a comment - I don't think this change is good in its current location - should reconsider alternative impl based on the proposed suggestion.
Alex Miller made changes -
Description Comparing a sorted-set with numbers to a set with keywords is not symmetric:

{code}
user=> (= #{:a} (sorted-set 1))
false
user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword clojure.lang.Keyword.compareTo (Keyword.java:109)
{code}

The latter case should return false instead of throwing.

*Cause:* APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check. It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.

*See also:* CLJ-1983 (downstream example with clojure.data/diff)
Comparing a sorted-set with numbers to a set with keywords is not symmetric:

{code}
user=> (= #{:a} (sorted-set 1))
false
user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword clojure.lang.Keyword.compareTo (Keyword.java:109)
{code}

The latter case should return false instead of throwing.

*Cause:* APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check.

*Proposed:* It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.

*See also:* CLJ-1983 (downstream example with clojure.data/diff)
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Nicola Mometto made changes -
Attachment 0001-CLJ-1242-equals-doesn-t-throw-on-sorted-collections.patch [ 16016 ]
Description Comparing a sorted-set with numbers to a set with keywords is not symmetric:

{code}
user=> (= #{:a} (sorted-set 1))
false
user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword clojure.lang.Keyword.compareTo (Keyword.java:109)
{code}

The latter case should return false instead of throwing.

*Cause:* APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check.

*Proposed:* It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.

*See also:* CLJ-1983 (downstream example with clojure.data/diff)
Comparing a sorted-set with numbers to a set with keywords is not symmetric:

{code}
user=> (= #{:a} (sorted-set 1))
false
user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword clojure.lang.Keyword.compareTo (Keyword.java:109)
{code}

The latter case should return false instead of throwing.

*Cause:* APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check.

*Proposed:* It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.

*See also:* CLJ-1983 (downstream example with clojure.data/diff)

*Patch:* 0001-CLJ-1242-equals-doesn-t-throw-on-sorted-collections.patch
Hide
Nicola Mometto added a comment -

Updated patch to only handle equals/equiv

Show
Nicola Mometto added a comment - Updated patch to only handle equals/equiv
Alex Miller made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]
Hide
Alex Miller added a comment -

Rich: TreeSet extends AbstractSet and uses its equals() implementation - that impl checks for ClassCastException and returns null. http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/AbstractSet.java?av=f#96

Show
Alex Miller added a comment - Rich: TreeSet extends AbstractSet and uses its equals() implementation - that impl checks for ClassCastException and returns null. http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/AbstractSet.java?av=f#96
Alex Miller made changes -
Description Comparing a sorted-set with numbers to a set with keywords is not symmetric:

{code}
user=> (= #{:a} (sorted-set 1))
false
user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword clojure.lang.Keyword.compareTo (Keyword.java:109)
{code}

The latter case should return false instead of throwing.

*Cause:* APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check.

*Proposed:* It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.

*See also:* CLJ-1983 (downstream example with clojure.data/diff)

*Patch:* 0001-CLJ-1242-equals-doesn-t-throw-on-sorted-collections.patch
Comparing a sorted-set with numbers to a set with keywords is not symmetric:

{code}
user=> (= #{:a} (sorted-set 1))
false
user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword clojure.lang.Keyword.compareTo (Keyword.java:109)
{code}

The latter case should return false instead of throwing.

*Cause:* APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check.

*Proposed:* It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.

*See also:* CLJ-1983 (downstream example with clojure.data/diff)

*Patch:* 0001-CLJ-1242-equals-doesn-t-throw-on-sorted-collections.patch

*Screened by:* Alex Miller
Approval Vetted [ 10003 ] Screened [ 10004 ]
Nicola Mometto made changes -
Attachment 0001-CLJ-1242-equals-doesn-t-throw-on-sorted-collections.patch [ 16016 ]
Hide
Nicola Mometto added a comment -

updated patch to fix indentation

Show
Nicola Mometto added a comment - updated patch to fix indentation
Nicola Mometto made changes -
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Status Reopened [ 4 ] Closed [ 6 ]
Resolution Completed [ 1 ]

People

Vote (7)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: