Clojure

clojure.walk should support records

Details

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

Description

Problem: clojure.walk throws exceptions if used on records.

user=> (defrecord Foo [x])
user.Foo
user=> (def f (Foo. :x))
#'user/f
user=> (use 'clojure.walk)
nil
user=> (postwalk identity {:foo f})
UnsupportedOperationException Can't create empty: user.Foo  user.Foo (NO_SOURCE_FILE:1)

Current Patch: 0001-CLJ-1105-Support-records-in-clojure.walk.patch adds a special case for records.

See also: CLJ-1239 "faster, more flexible dispatch for clojure.walk" which could supersede this ticket.

Screened by: Alex Miller - I think this will likely be superseded by CLJ-1239 in the future, but this is a reasonable short-term step.

Activity

Hide
Nicola Mometto added a comment -

maybe clojure should follow clojurescript's footsteps and move empty out of IPersistentCollection and create an
interface IEmptyableCollection extends IPersistentCollection { IEmptyableCollection empty(); }

Show
Nicola Mometto added a comment - maybe clojure should follow clojurescript's footsteps and move empty out of IPersistentCollection and create an interface IEmptyableCollection extends IPersistentCollection { IEmptyableCollection empty(); }
Hide
Stuart Halloway added a comment -

Can whoever claims this please consider walk's behavior in the face of all different collection types? I think it also fails with Java collections.

Also, the collection partitioning code in clojure.data may be of use.

Show
Stuart Halloway added a comment - Can whoever claims this please consider walk's behavior in the face of all different collection types? I think it also fails with Java collections. Also, the collection partitioning code in clojure.data may be of use.
Stuart Halloway made changes -
Field Original Value New Value
Approval Vetted [ 10003 ]
Fix Version/s Approved Backlog [ 10034 ]
Rich Hickey made changes -
Fix Version/s Approved Backlog [ 10034 ]
Fix Version/s Release 1.6 [ 10157 ]
Hide
Stuart Sierra added a comment -

clojure.walk can be made to support records directly, see https://github.com/stuartsierra/clojure.walk2

Show
Stuart Sierra added a comment - clojure.walk can be made to support records directly, see https://github.com/stuartsierra/clojure.walk2
Hide
Alex Miller added a comment -

Update title and adjust description slightly to focus this ticket on being an enhancement to make clojure.walk work with records.

Show
Alex Miller added a comment - Update title and adjust description slightly to focus this ticket on being an enhancement to make clojure.walk work with records.
Alex Miller made changes -
Description Using clojure.walk functions fails surprisingly for data containing records defined with defrecord:

user=> (defrecord Foo [x])
user.Foo
user=> (def f (Foo. :x))
#'user/f
user=> (use 'clojure.walk)
nil
user=> (postwalk identity {:foo f})
UnsupportedOperationException Can't create empty: user.Foo user.Foo (NO_SOURCE_FILE:1)

This seems to be because clojure.walk/walk guards a call to (empty form) with a (coll? form) check. The check succeeds because records implement IPersistentCollection, but (empty form) throws an exception. This looks to me like a bug in clojure.walk (it should check records separately and either treat them as atomic or implement a way of walking through them) but perhaps it is a sign of some unclarity in the contract of collections.
*Problem:* clojure.walk predates the existence of records and will cause exceptions if used with records.

For example:

user=> (defrecord Foo [x])
user.Foo
user=> (def f (Foo. :x))
#'user/f
user=> (use 'clojure.walk)
nil
user=> (postwalk identity {:foo f})
UnsupportedOperationException Can't create empty: user.Foo user.Foo (NO_SOURCE_FILE:1)

This seems to be because clojure.walk/walk guards a call to (empty form) with a (coll? form) check. The check succeeds because records implement IPersistentCollection, but (empty form) throws an exception. This looks to me like a bug in clojure.walk (it should check records separately and either treat them as atomic or implement a way of walking through them) but perhaps it is a sign of some unclarity in the contract of collections.
Issue Type Defect [ 1 ] Enhancement [ 4 ]
Summary defrecord classes implement IPersistentCollection but not .empty, clojure.walk assumes collections support empty clojure.walk should support records
Stuart Sierra made changes -
Assignee Stuart Sierra [ stuart.sierra ]
Hide
Stuart Sierra added a comment -

Attachment 0001-CLJ-1105-Support-records-in-clojure.walk.patch is a minimal patch adding support for records without altering the design of clojure.walk.

Show
Stuart Sierra added a comment - Attachment 0001-CLJ-1105-Support-records-in-clojure.walk.patch is a minimal patch adding support for records without altering the design of clojure.walk.
Stuart Sierra made changes -
Patch Code and Test [ 10002 ]
Attachment 0001-CLJ-1105-Support-records-in-clojure.walk.patch [ 12076 ]
Hide
Nicola Mometto added a comment -

clojure.walk is not required during bootstrapping, so I don't see why we couldn't just substitute clojure.walk with clojure.walk2 given that it only changes the internal implementation to a faster one

Show
Nicola Mometto added a comment - clojure.walk is not required during bootstrapping, so I don't see why we couldn't just substitute clojure.walk with clojure.walk2 given that it only changes the internal implementation to a faster one
Stuart Sierra made changes -
Description *Problem:* clojure.walk predates the existence of records and will cause exceptions if used with records.

For example:

user=> (defrecord Foo [x])
user.Foo
user=> (def f (Foo. :x))
#'user/f
user=> (use 'clojure.walk)
nil
user=> (postwalk identity {:foo f})
UnsupportedOperationException Can't create empty: user.Foo user.Foo (NO_SOURCE_FILE:1)

This seems to be because clojure.walk/walk guards a call to (empty form) with a (coll? form) check. The check succeeds because records implement IPersistentCollection, but (empty form) throws an exception. This looks to me like a bug in clojure.walk (it should check records separately and either treat them as atomic or implement a way of walking through them) but perhaps it is a sign of some unclarity in the contract of collections.
*Problem:* clojure.walk throws exceptions if used on records.

*Current Patch:* 0001-CLJ-1105-Support-records-in-clojure.walk.patch adds a special case for records.

*See also:* CLJ-1239 "faster, more flexible dispatch for clojure.walk" which could supersede this ticket.

*Demonstration:*

{code}
user=> (defrecord Foo [x])
user.Foo
user=> (def f (Foo. :x))
#'user/f
user=> (use 'clojure.walk)
nil
user=> (postwalk identity {:foo f})
UnsupportedOperationException Can't create empty: user.Foo user.Foo (NO_SOURCE_FILE:1)
{code}

This seems to be because clojure.walk/walk guards a call to (empty form) with a (coll? form) check. The check succeeds because records implement IPersistentCollection, but (empty form) throws an exception. This looks to me like a bug in clojure.walk (it should check records separately and either treat them as atomic or implement a way of walking through them) but perhaps it is a sign of some unclarity in the contract of collections.
Alex Miller made changes -
Labels walk
Alex Miller made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Description *Problem:* clojure.walk throws exceptions if used on records.

*Current Patch:* 0001-CLJ-1105-Support-records-in-clojure.walk.patch adds a special case for records.

*See also:* CLJ-1239 "faster, more flexible dispatch for clojure.walk" which could supersede this ticket.

*Demonstration:*

{code}
user=> (defrecord Foo [x])
user.Foo
user=> (def f (Foo. :x))
#'user/f
user=> (use 'clojure.walk)
nil
user=> (postwalk identity {:foo f})
UnsupportedOperationException Can't create empty: user.Foo user.Foo (NO_SOURCE_FILE:1)
{code}

This seems to be because clojure.walk/walk guards a call to (empty form) with a (coll? form) check. The check succeeds because records implement IPersistentCollection, but (empty form) throws an exception. This looks to me like a bug in clojure.walk (it should check records separately and either treat them as atomic or implement a way of walking through them) but perhaps it is a sign of some unclarity in the contract of collections.
*Problem:* clojure.walk throws exceptions if used on records.

{code}
user=> (defrecord Foo [x])
user.Foo
user=> (def f (Foo. :x))
#'user/f
user=> (use 'clojure.walk)
nil
user=> (postwalk identity {:foo f})
UnsupportedOperationException Can't create empty: user.Foo user.Foo (NO_SOURCE_FILE:1)
{code}

*Current Patch:* 0001-CLJ-1105-Support-records-in-clojure.walk.patch adds a special case for records.

*See also:* CLJ-1239 "faster, more flexible dispatch for clojure.walk" which could supersede this ticket.

*Screened by:* Alex Miller - I think this will likely be superseded by CLJ-1239 in the future, but this is a reasonable short-term step.
Alex Miller made changes -
Assignee Stuart Sierra [ stuart.sierra ]
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Closed [ 6 ]
Hide
Chouser added a comment -

The way this patch behaves can be surprising compared to regular maps:

(clojure.walk/prewalk-replace {[:a 1] nil} {:a 1, :b 2})
;=> {:b 2}

(defrecord Foo [a b])
(clojure.walk/prewalk-replace {[:a 1] nil} (map->Foo {:a 1, :b 2}))
;=> #user.Foo{:a 1, :b 2}

Note how the [:a 1] entry is removed from the map, but not from the record.

I believe CLJ-1239 also demonstrates this behavior.

Show
Chouser added a comment - The way this patch behaves can be surprising compared to regular maps: (clojure.walk/prewalk-replace {[:a 1] nil} {:a 1, :b 2}) ;=> {:b 2} (defrecord Foo [a b]) (clojure.walk/prewalk-replace {[:a 1] nil} (map->Foo {:a 1, :b 2})) ;=> #user.Foo{:a 1, :b 2} Note how the [:a 1] entry is removed from the map, but not from the record. I believe CLJ-1239 also demonstrates this behavior.
Hide
Andy Fingerhut added a comment -

This comment would probably get more visibility on CLJ-1239, given that this one is closed and CLJ-1239 is still open.

Show
Andy Fingerhut added a comment - This comment would probably get more visibility on CLJ-1239, given that this one is closed and CLJ-1239 is still open.

People

Vote (2)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved: