Clojure

equality bug on records created with nested calls to map->record

Details

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

Description

Depending on the type of the map passed to a record map constructor, records will not correctly compare for equality:

user> (defrecord a []) 
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2)  ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2)  ;; expected => {:a 1}
#user.a{:a 1}

Cause: The type of the map passed into the map constructor leaks into the __extmap, affecting equality comparison of the record. This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

Approach: Clean the extmap before putting it into the record constructor.

Patch: CLJ-1388-record-equality-and-map-record-factory.patch

Screened by: Alex Miller

  1. 0001-FIX-CLJ-1388.patch
    18/Mar/14 8:44 PM
    2 kB
    Nicola Mometto
  2. CLJ-1388.patch
    29/Apr/14 1:52 PM
    6 kB
    Nicola Mometto
  3. CLJ-1388-record-equality-and-map-record-factory.patch
    29/Apr/14 10:17 AM
    5 kB
    Steve Miner
  4. CLJ-1388v2.patch
    30/Apr/14 3:57 PM
    5 kB
    Nicola Mometto

Activity

Nicola Mometto made changes -
Field Original Value New Value
Attachment 0001-FIX-CLJ-1388.patch [ 12886 ]
Nicola Mometto made changes -
Patch Code and Test [ 10002 ]
Affects Version/s Release 1.6 [ 10157 ]
Nicola Mometto made changes -
Attachment 0001-FIX-CLJ-1388.patch [ 12886 ]
Nicola Mometto made changes -
Attachment 0001-FIX-CLJ-1388.patch [ 12887 ]
Alex Miller made changes -
Approval Triaged [ 10120 ]
Description This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

{code}
user> (defrecord a [])
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2) ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2)
#user.a{:a 1}
{code}

This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

*Approach:* Clean the extmap before putting it into the record constructor.

*Patch:* 0001-FIX-CLJ-1388.patch

*Screened by:*
Labels defrecord
Rich Hickey made changes -
Fix Version/s Release 1.7 [ 10250 ]
Approval Triaged [ 10120 ] Vetted [ 10003 ]
Alex Miller made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Description {code}
user> (defrecord a [])
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2) ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2)
#user.a{:a 1}
{code}

This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

*Approach:* Clean the extmap before putting it into the record constructor.

*Patch:* 0001-FIX-CLJ-1388.patch

*Screened by:*
{code}
user> (defrecord a [])
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2) ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2) ;; expected => {:a 1}
#user.a{:a 1}
{code}

This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

*Approach:* Clean the extmap before putting it into the record constructor.

*Patch:* 0001-FIX-CLJ-1388.patch

*Screened by:* Alex Miller
Hide
Steve Miner added a comment - - edited

The proposed patch 0001-FIX-CLJ-1388.patch makes every map->Record method pay to copy the argument map every time. However, according to my tests, the problem only occurs with records without any fields. So it should be sufficient to generate the (into {} m#) case only when `fields` is empty. [Update: this is wrong, explained below.]

Show
Steve Miner added a comment - - edited The proposed patch 0001-FIX-CLJ-1388.patch makes every map->Record method pay to copy the argument map every time. However, according to my tests, the problem only occurs with records without any fields. So it should be sufficient to generate the (into {} m#) case only when `fields` is empty. [Update: this is wrong, explained below.]
Steve Miner made changes -
Attachment CLJ-1388-equality-bug-with-degenerate-records.patch [ 12964 ]
Hide
Steve Miner added a comment -

It would be better to fix the problem in the Java Record/create method, but I couldn't figure out how that worked. On the other hand, this bug seems like a fairly rare edge case so I think my patch is acceptable.

Show
Steve Miner added a comment - It would be better to fix the problem in the Java Record/create method, but I couldn't figure out how that worked. On the other hand, this bug seems like a fairly rare edge case so I think my patch is acceptable.
Hide
Alex Miller added a comment -

Moving out of Screened due to new patch

Show
Alex Miller added a comment - Moving out of Screened due to new patch
Alex Miller made changes -
Approval Screened [ 10004 ] Vetted [ 10003 ]
Hide
Nicola Mometto added a comment -

Steve, the problem doesn't occur with records without any fields, your testing was reporting that only because you are only using one record type.

Here's an example that returns true with my patch, but still returns false with yours.

user=> (defrecord a [a])
user.a
user=> (defrecord b [b])
user.b
user=> (def x1 (map->a {:a 1 :b 2}))
#'user/x1
user=> (def x2 (map->a (map->b {:a 1 :b 2})))
#'user/x2
user=> x1
#user.a{:a 1, :b 2}
user=> x2
#user.a{:a 1, :b 2}
user=> (= x1 x2)
false
user=> (.__extmap x2)
#user.b{:b 2}
Show
Nicola Mometto added a comment - Steve, the problem doesn't occur with records without any fields, your testing was reporting that only because you are only using one record type. Here's an example that returns true with my patch, but still returns false with yours.
user=> (defrecord a [a])
user.a
user=> (defrecord b [b])
user.b
user=> (def x1 (map->a {:a 1 :b 2}))
#'user/x1
user=> (def x2 (map->a (map->b {:a 1 :b 2})))
#'user/x2
user=> x1
#user.a{:a 1, :b 2}
user=> x2
#user.a{:a 1, :b 2}
user=> (= x1 x2)
false
user=> (.__extmap x2)
#user.b{:b 2}
Hide
Nicola Mometto added a comment -

It should also be noted that the overhead of copying the record map is probably insignificant.

Show
Nicola Mometto added a comment - It should also be noted that the overhead of copying the record map is probably insignificant.
Hide
Nicola Mometto added a comment -

I also thought at first to fix the problem either on the /create method or on the 3-arity ctor but given that:

  • a fix there would involve messing with the bytecode emitted and thus would be harder to implement than this simple 1-line patch
  • neither the /create method nor the 3-arity ctor is documented and thus should be considered implementation details

I think patching the map->record function is the best way to go.

Show
Nicola Mometto added a comment - I also thought at first to fix the problem either on the /create method or on the 3-arity ctor but given that:
  • a fix there would involve messing with the bytecode emitted and thus would be harder to implement than this simple 1-line patch
  • neither the /create method nor the 3-arity ctor is documented and thus should be considered implementation details
I think patching the map->record function is the best way to go.
Steve Miner made changes -
Attachment CLJ-1388-equality-bug-with-degenerate-records.patch [ 12964 ]
Steve Miner made changes -
Comment [ Attached a new patch that applies fix only to the problem case. Added tests that show the actual problem and confirm the fix. ]
Hide
Steve Miner added a comment -

Nicola, thanks for the correction. I missed the case with multiple records. I withdrew my patch. I'd still like to find a more finely tuned patch, but first I'll have to improve my tests as you demonstrated.

Show
Steve Miner added a comment - Nicola, thanks for the correction. I missed the case with multiple records. I withdrew my patch. I'd still like to find a more finely tuned patch, but first I'll have to improve my tests as you demonstrated.
Hide
Steve Miner added a comment - - edited

Attached CLJ-1388-record-equality-and-map-record-factory.patch that checks arg to map->Record for MapEquivalence, uses (into {} m#) when necessary. This makes equiv test work correctly with records as the argument (and other map-like values). Added tests with variety of args to map->Record.

Show
Steve Miner added a comment - - edited Attached CLJ-1388-record-equality-and-map-record-factory.patch that checks arg to map->Record for MapEquivalence, uses (into {} m#) when necessary. This makes equiv test work correctly with records as the argument (and other map-like values). Added tests with variety of args to map->Record.
Steve Miner made changes -
Hide
Steve Miner added a comment -

A few comments about the new patch... I think the basic issue is a bad interaction between = for records and the generated Record/create method. Everything works when the interal __extmap is a regular map (MapEquivalence), but it fails if __extmap is another record. I think that's because of equiv calling = on the __extmap's.

The user expects to create a new record using the value of another record because it's just like a map. However, = on records respects the record type so it's not = to a map.

The general work-around is to use (into {} x) on the argument to the map->Record. To meet the user's expectation, that `into` call can be incorporated into the map->Record. But I didn't like the defensive copy as most of the time it's unnecessary – the argument is typically a regular map. The `into` work-around is only necessary if the arg is not a MapEquivalence.

There might be a better way to fix the Record/create method but I couldn't figure it out.

Show
Steve Miner added a comment - A few comments about the new patch... I think the basic issue is a bad interaction between = for records and the generated Record/create method. Everything works when the interal __extmap is a regular map (MapEquivalence), but it fails if __extmap is another record. I think that's because of equiv calling = on the __extmap's. The user expects to create a new record using the value of another record because it's just like a map. However, = on records respects the record type so it's not = to a map. The general work-around is to use (into {} x) on the argument to the map->Record. To meet the user's expectation, that `into` call can be incorporated into the map->Record. But I didn't like the defensive copy as most of the time it's unnecessary – the argument is typically a regular map. The `into` work-around is only necessary if the arg is not a MapEquivalence. There might be a better way to fix the Record/create method but I couldn't figure it out.
Hide
Nicola Mometto added a comment -

Steve's last comment made me realize that the root of the problem is on the record .equiv method, where the extmaps are compared via `=`

This new patch (CLJ-1388.patch) addresses this issue by comparing the extmaps with Utils/equiv rather than `=`, which compares maps in a type-indipendent way.

There's still a case where we need recreate the given map, that is when the given map is not an IPersistentMap but simply a java.util.Map.

Steve, my new patch incorporates my fix and your tests, I modified your patch to include only the tests (that were really comprehensive) since I figured it's fair to keep your authorship on those, let me know if that's a problem with you.

Show
Nicola Mometto added a comment - Steve's last comment made me realize that the root of the problem is on the record .equiv method, where the extmaps are compared via `=` This new patch (CLJ-1388.patch) addresses this issue by comparing the extmaps with Utils/equiv rather than `=`, which compares maps in a type-indipendent way. There's still a case where we need recreate the given map, that is when the given map is not an IPersistentMap but simply a java.util.Map. Steve, my new patch incorporates my fix and your tests, I modified your patch to include only the tests (that were really comprehensive) since I figured it's fair to keep your authorship on those, let me know if that's a problem with you.
Nicola Mometto made changes -
Attachment CLJ-1388.patch [ 12967 ]
Hide
Steve Miner added a comment -

Whatever works for you regarding the tests is fine by me.

Show
Steve Miner added a comment - Whatever works for you regarding the tests is fine by me.
Alex Miller made changes -
Description {code}
user> (defrecord a [])
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2) ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2) ;; expected => {:a 1}
#user.a{:a 1}
{code}

This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

*Approach:* Clean the extmap before putting it into the record constructor.

*Patch:* 0001-FIX-CLJ-1388.patch

*Screened by:* Alex Miller
{code}
user> (defrecord a [])
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2) ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2) ;; expected => {:a 1}
#user.a{:a 1}
{code}

This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

*Approach:* Clean the extmap before putting it into the record constructor.

*Patch:* CLJ-1388.patch

*Screened by:*
Hide
Alex Miller added a comment -

It seems weird to me that a record should ever contain another record as its extmap. We should be considering the performance aspect but I'm concerned that not locking down extmap more just invites other weirder problems later.

In CLJ-1388.patch, you mention Utils/equiv in your comment but the patch calls Utils/equals - which did you mean?

Also, that patch currently checks if m# is an IPersistentMap - I can't imagine what case we would want to allow where a valid m# is NOT an IPersistentMap?

Show
Alex Miller added a comment - It seems weird to me that a record should ever contain another record as its extmap. We should be considering the performance aspect but I'm concerned that not locking down extmap more just invites other weirder problems later. In CLJ-1388.patch, you mention Utils/equiv in your comment but the patch calls Utils/equals - which did you mean? Also, that patch currently checks if m# is an IPersistentMap - I can't imagine what case we would want to allow where a valid m# is NOT an IPersistentMap?
Alex Miller made changes -
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Hide
Nicola Mometto added a comment -

Alex, the Utils/equiv in my comment is wrong (it's easy to confuse between equiv/equals, sorry), Utils/equals in the patch is the right method to use since it compares in a type agnostic way.

Since __extmap is an implementation detail and is only used internally by defrecord for its methods, I don't think it's going to be a problem whether it's a record or a regular clojure map. (Clojure only requires it to be an IPersistentMap)

Regarding the check for m# being an IPersistentMap, Steve in his tests had a case where the map->record ctor was invoked with a java.util.Map, I went to look into the docs for defrecord and it only mentions that the argument to map->record has to be a "map", it doesn't specify that it has to be a clojure map/IPersistentMap, so it seemed right to allow for java maps too and wrap them in IPersistentMaps internally.

Show
Nicola Mometto added a comment - Alex, the Utils/equiv in my comment is wrong (it's easy to confuse between equiv/equals, sorry), Utils/equals in the patch is the right method to use since it compares in a type agnostic way. Since __extmap is an implementation detail and is only used internally by defrecord for its methods, I don't think it's going to be a problem whether it's a record or a regular clojure map. (Clojure only requires it to be an IPersistentMap) Regarding the check for m# being an IPersistentMap, Steve in his tests had a case where the map->record ctor was invoked with a java.util.Map, I went to look into the docs for defrecord and it only mentions that the argument to map->record has to be a "map", it doesn't specify that it has to be a clojure map/IPersistentMap, so it seemed right to allow for java maps too and wrap them in IPersistentMaps internally.
Hide
Steve Miner added a comment -

My test with java.util.Map was an extension of the idea that anything map-like could be used to initialize a record. That might be a bridge too far, but my patch was testing for MapEquivalence to handle records so it made sense to allow j.u.Map, etc. With Nicola's latest patch, it's probably unnecessary to support non-IPersistentMaps so map->Record doesn't actually need to change.

Show
Steve Miner added a comment - My test with java.util.Map was an extension of the idea that anything map-like could be used to initialize a record. That might be a bridge too far, but my patch was testing for MapEquivalence to handle records so it made sense to allow j.u.Map, etc. With Nicola's latest patch, it's probably unnecessary to support non-IPersistentMaps so map->Record doesn't actually need to change.
Hide
Nicola Mometto added a comment - - edited

CLJ-1388v2.patch is like CLJ-1388.patch except it doesn't copy non IPersistentMaps in a clojure map.

To summarize, here's the status of the different patches for this ticket:

  • 0001-FIX-CLJ-1388.patch copies the argument of map->record in a clojure map via `(into {} m#)`, be it already a clojure map, a record, or a java.util.Map
  • CLJ-1388-record-equality-and-map-record-factory.patch adopts the same approach except it only copies the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.MapEquivalence
  • CLJ-1388.patch fixes the issue by changing the function that compares __extmaps from `=` (type aware) to `clojure.lang.Utils/equals` (type agnostic), this patch also copies the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.IPersistentMap
  • CLJ-1388v2.patch is the same as CLJ-1388.patch except it doesn't copy the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.IPersitentMap, thus map->record will not work with bare java.util.Maps (which is the behaviour it has already)
Show
Nicola Mometto added a comment - - edited CLJ-1388v2.patch is like CLJ-1388.patch except it doesn't copy non IPersistentMaps in a clojure map. To summarize, here's the status of the different patches for this ticket:
  • 0001-FIX-CLJ-1388.patch copies the argument of map->record in a clojure map via `(into {} m#)`, be it already a clojure map, a record, or a java.util.Map
  • CLJ-1388-record-equality-and-map-record-factory.patch adopts the same approach except it only copies the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.MapEquivalence
  • CLJ-1388.patch fixes the issue by changing the function that compares __extmaps from `=` (type aware) to `clojure.lang.Utils/equals` (type agnostic), this patch also copies the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.IPersistentMap
  • CLJ-1388v2.patch is the same as CLJ-1388.patch except it doesn't copy the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.IPersitentMap, thus map->record will not work with bare java.util.Maps (which is the behaviour it has already)
Nicola Mometto made changes -
Attachment CLJ-1388v2.patch [ 12971 ]
Hide
Alex Miller added a comment -

Are these patches all still in play? Having 4 active patches does not help move a ticket forward.

Can someone re-summarize at this point what questions exist?

Show
Alex Miller added a comment - Are these patches all still in play? Having 4 active patches does not help move a ticket forward. Can someone re-summarize at this point what questions exist?
Hide
Nicola Mometto added a comment -

0001-FIX-CLJ-1388.patch should be superseded by the other 3 patches since they solve the same problem in a more performant way.

To pick between the other patches, we need to chose which approach to go with.
Patches CLJ-1388.patch and CLJ-1388v2.patch fix the issue in the equiv method of the defrecord, patch CLJ-1388-record-equality-and-map-record-factory.patch fixes the issue in the map->record ctor by converting maps that don't implement MapEquivalence to a clojure map.

I'd go with either CLJ-1388.patch or CLJ-1388v2.patch since they both avoid copying alltoghether in the cases where map->record currently works, while CLJ-1388-record-equality-and-map-record-factory.patch needs to copy the arg into a map if the arg is a custom IPersistentMap or a record.

To pick between CLJ-1388.patch or CLJ-1388v2.patch we need to decide whether or not the current behaviour of map->record to require strictly an IPersistentMap is the way to go: if we decide that it's ok to pass non IPersitentMap maps like java.util.Map to map->record then pick CLJ-1388.patch otherwise CLJ-1388v2.patch

Show
Nicola Mometto added a comment - 0001-FIX-CLJ-1388.patch should be superseded by the other 3 patches since they solve the same problem in a more performant way. To pick between the other patches, we need to chose which approach to go with. Patches CLJ-1388.patch and CLJ-1388v2.patch fix the issue in the equiv method of the defrecord, patch CLJ-1388-record-equality-and-map-record-factory.patch fixes the issue in the map->record ctor by converting maps that don't implement MapEquivalence to a clojure map. I'd go with either CLJ-1388.patch or CLJ-1388v2.patch since they both avoid copying alltoghether in the cases where map->record currently works, while CLJ-1388-record-equality-and-map-record-factory.patch needs to copy the arg into a map if the arg is a custom IPersistentMap or a record. To pick between CLJ-1388.patch or CLJ-1388v2.patch we need to decide whether or not the current behaviour of map->record to require strictly an IPersistentMap is the way to go: if we decide that it's ok to pass non IPersitentMap maps like java.util.Map to map->record then pick CLJ-1388.patch otherwise CLJ-1388v2.patch
Alex Miller made changes -
Description {code}
user> (defrecord a [])
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2) ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2) ;; expected => {:a 1}
#user.a{:a 1}
{code}

This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

*Approach:* Clean the extmap before putting it into the record constructor.

*Patch:* CLJ-1388.patch

*Screened by:*
Depending on the type of the map passed to a record map constructor, records will not correctly compare for equality:

{code}
user> (defrecord a [])
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2) ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2) ;; expected => {:a 1}
#user.a{:a 1}
{code}

*Cause:* The type of the map passed into the map constructor leaks into the __extmap, affecting equality comparison of the record. This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

*Approach:* Clean the extmap before putting it into the record constructor.

*Patch:*

*Screened by:*
Hide
Alex Miller added a comment -

From brief conversation with Rich, we should not allow arbitrary map types in __extmap so would prefer to force a clean map and rely on standard equality checking. I think CLJ-1388-record-equality-and-map-record-factory.patch is the preferred path based on that, which still seems like it should avoid copying in nearly all common cases.

Show
Alex Miller added a comment - From brief conversation with Rich, we should not allow arbitrary map types in __extmap so would prefer to force a clean map and rely on standard equality checking. I think CLJ-1388-record-equality-and-map-record-factory.patch is the preferred path based on that, which still seems like it should avoid copying in nearly all common cases.
Alex Miller made changes -
Description Depending on the type of the map passed to a record map constructor, records will not correctly compare for equality:

{code}
user> (defrecord a [])
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2) ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2) ;; expected => {:a 1}
#user.a{:a 1}
{code}

*Cause:* The type of the map passed into the map constructor leaks into the __extmap, affecting equality comparison of the record. This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

*Approach:* Clean the extmap before putting it into the record constructor.

*Patch:*

*Screened by:*
Depending on the type of the map passed to a record map constructor, records will not correctly compare for equality:

{code}
user> (defrecord a [])
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2) ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2) ;; expected => {:a 1}
#user.a{:a 1}
{code}

*Cause:* The type of the map passed into the map constructor leaks into the __extmap, affecting equality comparison of the record. This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

*Approach:* Clean the extmap before putting it into the record constructor.

*Patch:* CLJ-1388-record-equality-and-map-record-factory.patch

*Screened by:*
Hide
Alex Miller added a comment -

Screened specifically CLJ-1388-record-equality-and-map-record-factory.patch - use map as is if it supports MapEquivalence (and can thus be compared under a map) and otherwise dump into a clojure map.

Show
Alex Miller added a comment - Screened specifically CLJ-1388-record-equality-and-map-record-factory.patch - use map as is if it supports MapEquivalence (and can thus be compared under a map) and otherwise dump into a clojure map.
Alex Miller made changes -
Approval Incomplete [ 10006 ] Screened [ 10004 ]
Description Depending on the type of the map passed to a record map constructor, records will not correctly compare for equality:

{code}
user> (defrecord a [])
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2) ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2) ;; expected => {:a 1}
#user.a{:a 1}
{code}

*Cause:* The type of the map passed into the map constructor leaks into the __extmap, affecting equality comparison of the record. This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

*Approach:* Clean the extmap before putting it into the record constructor.

*Patch:* CLJ-1388-record-equality-and-map-record-factory.patch

*Screened by:*
Depending on the type of the map passed to a record map constructor, records will not correctly compare for equality:

{code}
user> (defrecord a [])
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2) ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2) ;; expected => {:a 1}
#user.a{:a 1}
{code}

*Cause:* The type of the map passed into the map constructor leaks into the __extmap, affecting equality comparison of the record. This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

*Approach:* Clean the extmap before putting it into the record constructor.

*Patch:* CLJ-1388-record-equality-and-map-record-factory.patch

*Screened by:* Alex Miller
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Closed [ 6 ]

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: