Broken equality for sets?

[Justin Kramer]

Steve Losh uncovered what looks like a bug:

https://gist.github.com/4043824

This affects Clojure 1.4 and 1.5 master. The offending line seems to be:

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/APersistentSet.java#L58

If I understand correctly, equals should not be checking hashCode. That check was removed from APersistentMap in the following commit:

https://github.com/clojure/clojure/commit/b5f5ba2e15dc2f20e14e05141f7de7c6a3d91179

I removed the hashCode check in a local branch, which appears to fix the issue (and all tests pass). I can submit a patch if this is truly a bug.

Justin

[Herwig Hochleitner]

Sure looks like a bug to me. I think opening a ticket is a safe bet here ;-)

2012/11/9 Justin Kramer <jkkr...@gmail.com>

Justin

--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
To view this discussion on the web visit https://groups.google.com/d/msg/clojure-dev/-/lQXUC9hPvuMJ.
To post to this group, send email to cloju...@googlegroups.com.
To unsubscribe from this group, send email to clojure-dev...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/clojure-dev?hl=en.

[Philip Potter]

That looks like a bug to me.

The = function in clojure delegates to Util.equiv(), which delegates
to the .equiv() method if present. The problem with sets is that
equiv() simply delegates to equals() when it should have a distinct
implementation (see vector for an example)

Clojure has two different equality relations: equals() & hashCode(),
which consider (Long. -1) and (Integer.-1) to be distinct, and equiv()
& hashEq(), which consider (Integer. -1) and (Long. -1) to be equal.
Clojure's persistent collections also implement these two different
equalities, so that [(Long. -1)] is equiv() but not equals() to
[(Integer. -1)].

user=> (def v [(Long. -1)])
#'user/v
user=> (def v* [(Integer. -1)])
#'user/v*
user=> (= v v*)
true
user=> (.equals v v*)
false

The same behaviour should occur for sets, but it doesn't, because
equiv() delegates to equals():

user=> (def s #{(Long. -1)})
#'user/s
user=> (def s* #{(Integer. -1)})
#'user/s*
user=> (= s s*)
false ;;; should be true here, this is the bug
user=> (.equals s s*)
false ;;; this is correct

so definitely file a bug. I hope this helps you file a patch, too.

Phil

[Justin Kramer]

Ticket created:

http://dev.clojure.org/jira/browse/CLJ-1106

On Friday, November 9, 2012 2:41:20 AM UTC-5, Philip Potter wrote:

The same behaviour should occur for sets, but it doesn't, because
equiv() delegates to equals():

user=> (def s #{(Long. -1)})
#'user/s
user=> (def s* #{(Integer. -1)})
#'user/s*
user=> (= s s*)
false    ;;; should be true here, this is the bug
user=> (.equals s s*)
false    ;;; this is correct

so definitely file a bug. I hope this helps you file a patch, too.

With a patch that makes APersistentSet consistent with APersistentMap, both = and .equals return true. This is also the case for maps (with and without the patch):

user=> (def m {(Long. -1) :foo})

#'user/m

user=> (def m* {(Integer. -1) :foo})

#'user/m*

user=> (= m m*)

true

user=> (.equals m m*)

true

I'm not sure if this should be considered a separate issue from the set quality one.

Justin

[Christophe Grand]

The behaviour is more subtle:

Currently maps are:
 .equals when their keys are .equiv and their values .equals.
 .equiv when their keys are .equiv and their values .equiv.

So sets delegating .equals to .equiv would be coherent with the current behaviour of maps.

Christophe

Justin

--

You received this message because you are subscribed to the Google Groups "Clojure Dev" group.

To view this discussion on the web visit https://groups.google.com/d/msg/clojure-dev/-/tQWIeFrpL2cJ.

To post to this group, send email to cloju...@googlegroups.com.
To unsubscribe from this group, send email to clojure-dev...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/clojure-dev?hl=en.

--
Professional: http://cgrand.net/ (fr)
On Clojure: http://clj-me.cgrand.net/ (en)

[Andy Fingerhut]

It seems that there have been around half a dozen tickets on issues with = and hasheq in the last year or so.

Is it a good time to have some more extensive unit tests in this area that try to hit all of the concrete data structures several times each in this area?

Andy

[Philip Potter]

Hmm. This behaviour breaks .equals() symmetry & transitivity, and
.equiv() transitivity, for operations crossing j.u.Map and
IPersistentMap:

user=> (def a (doto (java.util.HashMap.) (.put (Long. -1) -1)))
#'user/a
user=> (def b {(Long. -1) -1})
#'user/b
user=> (def c {(Integer. -1) -1})
#'user/c
user=> (= a b)
true
user=> (= b c)
true
user=> (= a c)
false

user=> (.equals a c)
true
user=> (.equals c a)
false
user=> (.equals c b)
true
user=> (.equals b a)
true

...though that said, I can see fairly solid-looking design reasons why
this behaviour is pragmatically useful. Should I file a bug (or two)?

[Christophe Grand]

On Assembla, the design page on numerics reads:

hash maps and sets use = for keys

• this will make maps and sets unify 42 and 42N, since =
• will use stricter .equals when calling through java.util interfaces
  • not there yet
  • this will require renaming Associative.containsKey, since semantics will now differ

So it looks like no one volunteered to fully execute the plan — or this page is not the final word on numerics.

Christophe

[Philip Potter]

The same is still on confluence:

http://dev.clojure.org/display/doc/Enhanced+Primitive+Support

If matching java.util interfaces closely is the desired design, does
.equals() count as a java.util interface method? Should it check the
keys are .equals rather than .equiv?

[Rich Hickey]

It's more subtle than that, as we still have yet to break out containsKey and have it conform to the contract of j.u.Map, at which point .equals will be .equals for key and value.

In any case, all thinking on this needs to consider java.util conformance as well.

[Christophe Grand]

Thanks, good to know that the design notes on numerics I quoted are still current.
It follows that, for example, .contains on sets, seqs and vectors should be fixed to conform to j.u.Collection. Right?

[Rich Hickey]

Yes. We might have to audit our use and see what needs to change to 'containsEquiv', if anything.

[Christophe Grand]

Thianking about it, isn't the j.u.Map contract too strong in its wording?
TreeMaps are Maps but neither get, put or containsKey obey j.u.Map because the comparator defines its own equivalence relation. (btw TreeMaps for equality, use their equivalence relationon keys and equality on values, like APersistentMap does)
Ditto for contains (in TreeSet for example).

So is it important to follow java.util.Map to the letter? Where/when does it matter?

Christophe

On Clojure http://clj-me.cgrand.net/
Clojure Programming http://clojurebook.com
Training, Consulting & Contracting http://lambdanext.eu/

[Christophe Grand]

Please forget what I just said: in Comparator's doc it's recommended that comparators used with sorted collections are consistent with equals (define teh same equivalence relation) to not violate the general contract of maps/sets.

It follows that even .get and .valAt for example should diverge.