fix record equality with other maps, = includes type, .equals doesn't. see #418

5 files changed, 64 insertions, 38 deletions

diff --git a/src/clj/clojure/core_deftype.clj b/src/clj/clojure/core_deftype.clj
index c14f82f3..b9960c82 100644
--- a/src/clj/clojure/core_deftype.clj
+++ b/src/clj/clojure/core_deftype.clj
@@ -151,14 +151,8 @@
      [(eqhash [[i m]] 
         [i
          (conj m 
-               `(hashCode [this#] (-> ~tag hash ~@(map #(list `hash-combine %) (remove #{'__meta} fields))))
-               `(equals [this# ~gs] 
-                        (boolean 
-                         (or (identical? this# ~gs)
-                             (when (identical? (class this#) (class ~gs))
-                               (let [~gs ~(with-meta gs {:tag tagname})]
-                                 (and  ~@(map (fn [fld] `(= ~fld (. ~gs ~fld))) base-fields)
-                                       (= ~'__extmap (. ~gs ~'__extmap)))))))))])
+               `(hashCode [this#] (clojure.lang.APersistentMap/mapHash this#))
+               `(equals [this# ~gs] (clojure.lang.APersistentMap/mapEquals this# ~gs)))])
       (iobj [[i m]] 
             [(conj i 'clojure.lang.IObj)
              (conj m `(meta [this#] ~'__meta)
@@ -190,7 +184,13 @@
                    `(count [this#] (+ ~(count base-fields) (count ~'__extmap)))
                    `(empty [this#] (throw (UnsupportedOperationException. (str "Can't create empty: " ~(str classname)))))
                    `(cons [this# e#] ((var imap-cons) this# e#))
-                   `(equiv [this# o#] (.equals this# o#))
+                   `(equiv [this# ~gs] 
+                        (boolean 
+                         (or (identical? this# ~gs)
+                             (when (identical? (class this#) (class ~gs))
+                               (let [~gs ~(with-meta gs {:tag tagname})]
+                                 (and  ~@(map (fn [fld] `(= ~fld (. ~gs ~fld))) base-fields)
+                                       (= ~'__extmap (. ~gs ~'__extmap))))))))
                    `(containsKey [this# k#] (not (identical? this# (.valAt this# k# this#))))
                    `(entryAt [this# k#] (let [v# (.valAt this# k# this#)]
                                             (when-not (identical? this# v#)
diff --git a/src/jvm/clojure/lang/APersistentMap.java b/src/jvm/clojure/lang/APersistentMap.java
index 0fe08cae..50092ba6 100644
--- a/src/jvm/clojure/lang/APersistentMap.java
+++ b/src/jvm/clojure/lang/APersistentMap.java
@@ -13,7 +13,7 @@ package clojure.lang;
 import java.io.Serializable;

 import java.util.*;

 

-public abstract class APersistentMap extends AFn implements IPersistentMap, Map, Iterable, Serializable {

+public abstract class APersistentMap extends AFn implements IPersistentMap, Map, Iterable, Serializable, MapEquivalence {

 int _hash = -1;

 

 public String toString(){

@@ -45,15 +45,19 @@ public IPersistentCollection cons(Object o){
 }

 

 public boolean equals(Object obj){

-	if(this == obj) return true;

+	return mapEquals(this, obj);

+}

+

+static public boolean mapEquals(IPersistentMap m1, Object obj){

+	if(m1 == obj) return true;

 	if(!(obj instanceof Map))

 		return false;

 	Map m = (Map) obj;

 

-	if(m.size() != size() || m.hashCode() != hashCode())

+	if(m.size() != m1.count() || m.hashCode() != m1.hashCode())

 		return false;

 

-	for(ISeq s = seq(); s != null; s = s.next())

+	for(ISeq s = m1.seq(); s != null; s = s.next())

 		{

 		Map.Entry e = (Map.Entry) s.first();

 		boolean found = m.containsKey(e.getKey());

@@ -68,6 +72,9 @@ public boolean equals(Object obj){
 public boolean equiv(Object obj){

 	if(!(obj instanceof Map))

 		return false;

+	if(obj instanceof IPersistentMap && !(obj instanceof MapEquivalence))

+		return false;

+	

 	Map m = (Map) obj;

 

 	if(m.size() != size())

@@ -87,20 +94,22 @@ public boolean equiv(Object obj){
 public int hashCode(){

 	if(_hash == -1)

 		{

-		//int hash = count();

-		int hash = 0;

-		for(ISeq s = seq(); s != null; s = s.next())

-			{

-			Map.Entry e = (Map.Entry) s.first();

-			hash += (e.getKey() == null ? 0 : e.getKey().hashCode()) ^

-			        (e.getValue() == null ? 0 : e.getValue().hashCode());

-			//hash ^= Util.hashCombine(Util.hash(e.getKey()), Util.hash(e.getValue()));

-			}

-		this._hash = hash;

+		this._hash = mapHash(this);

 		}

 	return _hash;

 }

 

+static public int mapHash(IPersistentMap m){

+	int hash = 0;

+	for(ISeq s = m.seq(); s != null; s = s.next())

+		{

+		Map.Entry e = (Map.Entry) s.first();

+		hash += (e.getKey() == null ? 0 : e.getKey().hashCode()) ^

+				(e.getValue() == null ? 0 : e.getValue().hashCode());

+		}

+	return hash;

+}

+

 static public class KeySeq extends ASeq{

 	ISeq seq;

 

diff --git a/src/jvm/clojure/lang/MapEquivalence.java b/src/jvm/clojure/lang/MapEquivalence.java
new file mode 100644
index 00000000..40448425
--- /dev/null
+++ b/src/jvm/clojure/lang/MapEquivalence.java
@@ -0,0 +1,17 @@
+/**
+ *   Copyright (c) Rich Hickey. All rights reserved.
+ *   The use and distribution terms for this software are covered by the
+ *   Eclipse Public License 1.0 (http://opensource.org/licenses/eclipse-1.0.php)
+ *   which can be found in the file epl-v10.html at the root of this distribution.
+ *   By using this software in any fashion, you are agreeing to be bound by
+ * 	 the terms of this license.
+ *   You must not remove this notice, or any other, from this software.
+ **/
+
+/* rich Aug 4, 2010 */
+
+package clojure.lang;
+
+//marker interface
+public interface MapEquivalence{
+}
diff --git a/src/jvm/clojure/lang/Util.java b/src/jvm/clojure/lang/Util.java
index 8ef2c637..338c60a8 100644
--- a/src/jvm/clojure/lang/Util.java
+++ b/src/jvm/clojure/lang/Util.java
@@ -27,13 +27,19 @@ static public boolean equiv(Object k1, Object k2){
 		{
 		if(k1 instanceof Number && k2 instanceof Number)
 			return Numbers.equiv(k1, k2);
-		else if(k1 instanceof IPersistentCollection && k2 instanceof IPersistentCollection)
-			return ((IPersistentCollection)k1).equiv(k2);
+		else if(k1 instanceof IPersistentCollection || k2 instanceof IPersistentCollection)
+			return pcequiv(k1,k2);
 		return k1.equals(k2);
 		}
 	return false;
 }
 
+static public boolean pcequiv(Object k1, Object k2){
+	if(k1 instanceof IPersistentCollection)
+		return ((IPersistentCollection)k1).equiv(k2);
+	return ((IPersistentCollection)k2).equiv(k1);
+}
+
 static public boolean equals(Object k1, Object k2){
 	if(k1 == k2)
 		return true;
diff --git a/test/clojure/test_clojure/protocols.clj b/test/clojure/test_clojure/protocols.clj
index d8d2ac7f..ea9c5130 100644
--- a/test/clojure/test_clojure/protocols.clj
+++ b/test/clojure/test_clojure/protocols.clj
@@ -175,22 +175,16 @@
 (defrecord DefrecordObjectMethodsWidgetA [a])
 (defrecord DefrecordObjectMethodsWidgetB [a])
 (deftest defrecord-object-methods-test
-  (testing ".equals depends on fields and type"
-    (is (true? (.equals (DefrecordObjectMethodsWidgetA. 1) (DefrecordObjectMethodsWidgetA. 1))))
-    (is (false? (.equals (DefrecordObjectMethodsWidgetA. 1) (DefrecordObjectMethodsWidgetA. 2))))
-    (is (false? (.equals (DefrecordObjectMethodsWidgetA. 1) (DefrecordObjectMethodsWidgetB. 1)))))
-  (testing ".hashCode depends on fields and type"
-    (is (= (.hashCode (DefrecordObjectMethodsWidgetA. 1)) (.hashCode (DefrecordObjectMethodsWidgetA. 1))))
-    (is (= (.hashCode (DefrecordObjectMethodsWidgetA. 2)) (.hashCode (DefrecordObjectMethodsWidgetA. 2))))
-    (is (not= (.hashCode (DefrecordObjectMethodsWidgetA. 1)) (.hashCode (DefrecordObjectMethodsWidgetA. 2))))
-    (is (= (.hashCode (DefrecordObjectMethodsWidgetB. 1)) (.hashCode (DefrecordObjectMethodsWidgetB. 1))))
-    (is (not= (.hashCode (DefrecordObjectMethodsWidgetA. 1)) (.hashCode (DefrecordObjectMethodsWidgetB. 1))))))
+  (testing "= depends on fields and type"
+    (is (true? (= (DefrecordObjectMethodsWidgetA. 1) (DefrecordObjectMethodsWidgetA. 1))))
+    (is (false? (= (DefrecordObjectMethodsWidgetA. 1) (DefrecordObjectMethodsWidgetA. 2))))
+    (is (false? (= (DefrecordObjectMethodsWidgetA. 1) (DefrecordObjectMethodsWidgetB. 1))))))
 
 (deftest defrecord-acts-like-a-map
   (let [rec (r 1 2)]
-    (is (= (r 1 3 {} {:c 4}) (merge rec {:b 3 :c 4})))
-    (is (= {:foo 1 :b 2} (set/rename-keys rec {:a :foo})))
-    (is (= {:a 11 :b 2 :c 10} (merge-with + rec {:a 10 :c 10})))))
+    (is (.equals (r 1 3 {} {:c 4}) (merge rec {:b 3 :c 4})))
+    (is (.equals {:foo 1 :b 2} (set/rename-keys rec {:a :foo})))
+    (is (.equals {:a 11 :b 2 :c 10} (merge-with + rec {:a 10 :c 10})))))
 
 (deftest degenerate-defrecord-test
   (let [empty (EmptyRecord.)]