ClojureScript

defrecord does not emit IKVReduce protocol

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Critical Critical
  • Resolution: Unresolved
  • Affects Version/s: 1.7.145
  • Fix Version/s: 1.9.671
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test

Description

Records are maps and in Clojure they support reduce-kv (IKVReduce protocol).
This is not true in ClojureScript:

(defrecord Foobar [x y])
 (reduce-kv assoc {} (Foobar. 1 2))

Fails wit Error: No protocol method IKVReduce.-kv-reduce defined for type : [object Object]

Activity

Hide
David Nolen added a comment -

Just seems like an oversight. Patch welcome, this one is a relatively easy one.

Show
David Nolen added a comment - Just seems like an oversight. Patch welcome, this one is a relatively easy one.
Hide
Daniel Skarda added a comment -

OK

I checked Clojure implementation. Records do not implement any reduce protocol on their own. For IKVReduce records use default implementation using reduce and destructuring. Is this approach OK?

Recently Alex Miller implemented many optimizations of reduce protocols in Clojure. Eg range returns an object which implements IReduce protocol so reduce (and transducers in general) can take advantage of it. Any plans for such optimizations in ClojureScript?

;;clojure/src/clj/clojure/core.clj:6523
;;slow path default
clojure.lang.IPersistentMap
(kv-reduce 
  [amap f init]
  (reduce (fn [ret [k v]] (f ret k v)) init amap))
Show
Daniel Skarda added a comment - OK I checked Clojure implementation. Records do not implement any reduce protocol on their own. For IKVReduce records use default implementation using reduce and destructuring. Is this approach OK? Recently Alex Miller implemented many optimizations of reduce protocols in Clojure. Eg range returns an object which implements IReduce protocol so reduce (and transducers in general) can take advantage of it. Any plans for such optimizations in ClojureScript?
;;clojure/src/clj/clojure/core.clj:6523
;;slow path default
clojure.lang.IPersistentMap
(kv-reduce 
  [amap f init]
  (reduce (fn [ret [k v]] (f ret k v)) init amap))
Hide
David Nolen added a comment -

Going with the Clojure implementation is fine. Yes all of the optimizations in 1.7.0 are on the table for ClojureScript but these are separate issues from this one.

Show
David Nolen added a comment - Going with the Clojure implementation is fine. Yes all of the optimizations in 1.7.0 are on the table for ClojureScript but these are separate issues from this one.
Hide
Samuel Miller added a comment -

Mind if I take this as my first cljs bug? Poking around quickly I think I know what needs to happen.

Show
Samuel Miller added a comment - Mind if I take this as my first cljs bug? Poking around quickly I think I know what needs to happen.
Hide
David Nolen added a comment -

Sure! Have you submitted your CA yet?

Show
David Nolen added a comment - Sure! Have you submitted your CA yet?
Hide
Samuel Miller added a comment -

Yes, I did yesterday.

Show
Samuel Miller added a comment - Yes, I did yesterday.
Hide
Samuel Miller added a comment -

Here is a potential patch. I implemented a basic IKVreduce based on Daniel Skarda's comment. Note: I am a little fuzzy on macros still so please look over what I have. There is probably a better way. Also added a test for reduce-kv on records.

I ran the test on Linux on V8 and SpiderMonkey. I plan to get JSC and Nashorn working and tested this week but if someone wants to test them out before that would be great.

Show
Samuel Miller added a comment - Here is a potential patch. I implemented a basic IKVreduce based on Daniel Skarda's comment. Note: I am a little fuzzy on macros still so please look over what I have. There is probably a better way. Also added a test for reduce-kv on records. I ran the test on Linux on V8 and SpiderMonkey. I plan to get JSC and Nashorn working and tested this week but if someone wants to test them out before that would be great.
Hide
Sebastian Bensusan added a comment -

Experience report:

I just tested the patch in the Node Repl and it seems to work:

cljs.user=> (defrecord A [a b])
cljs.user/A
cljs.user=> (reduce-kv (fn [m k v] (assoc m k (inc v))) {} (A. 1 2))
{:a 2, :b 3}

and the provided tests passed in Spidermonkey, V8, and Nashorn (I don't have JSC installed).

For completeness: before applying the patch the same code fails with:

Error: No protocol method IKVReduce.-kv-reduce defined for type : [object Object]
Show
Sebastian Bensusan added a comment - Experience report: I just tested the patch in the Node Repl and it seems to work:
cljs.user=> (defrecord A [a b])
cljs.user/A
cljs.user=> (reduce-kv (fn [m k v] (assoc m k (inc v))) {} (A. 1 2))
{:a 2, :b 3}
and the provided tests passed in Spidermonkey, V8, and Nashorn (I don't have JSC installed). For completeness: before applying the patch the same code fails with:
Error: No protocol method IKVReduce.-kv-reduce defined for type : [object Object]
Hide
David Nolen added a comment -

Is this the same approach taken by Clojure?

Show
David Nolen added a comment - Is this the same approach taken by Clojure?
Hide
Samuel Miller added a comment - - edited

You can see the relevant current Clojure code here...
https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L6526
I think it is the same. I literally just tried to translate it over into CLJS. I might of understood something wrong though.

Show
Samuel Miller added a comment - - edited You can see the relevant current Clojure code here... https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L6526 I think it is the same. I literally just tried to translate it over into CLJS. I might of understood something wrong though.
Hide
David Nolen added a comment - - edited

Yes that's the slow path. Please use the implementation used by defrecord instead. If defrecord doesn't have one then this patch is OK.

Show
David Nolen added a comment - - edited Yes that's the slow path. Please use the implementation used by defrecord instead. If defrecord doesn't have one then this patch is OK.
Hide
Samuel Miller added a comment -

As far as I can tell there is no implementation on defrecord itself however there are separate implementations on the the java classes PersistentVector, PersistentArrayMap, PersistentTreeMap, and PersistenHashMap in pure java. I am not sure if you would want to do something similar for Clojurescript.

I can also spend some time trying to make a more performant version.

Show
Samuel Miller added a comment - As far as I can tell there is no implementation on defrecord itself however there are separate implementations on the the java classes PersistentVector, PersistentArrayMap, PersistentTreeMap, and PersistenHashMap in pure java. I am not sure if you would want to do something similar for Clojurescript. I can also spend some time trying to make a more performant version.
Hide
António Nuno Monteiro added a comment -

Confirmed that Clojure uses the slow path via the IPersistentMap implementation in defrecord
https://github.com/clojure/clojure/blob/d920ad/src/clj/clojure/core.clj#L6712

Patch still applies and can also confirm it works for me.

Show
António Nuno Monteiro added a comment - Confirmed that Clojure uses the slow path via the IPersistentMap implementation in defrecord https://github.com/clojure/clojure/blob/d920ad/src/clj/clojure/core.clj#L6712 Patch still applies and can also confirm it works for me.
Hide
Mike Fikes added a comment -

CLJS-1297-19-July-2015.patch no longer applies.

Show
Mike Fikes added a comment - CLJS-1297-19-July-2015.patch no longer applies.
Hide
Tommi Reiman added a comment - - edited

Hi. What's the current status of this? Spec-tools fails to generate the JSON Schemas out of clojure.spec + records on cljs and there is a local fix proposed on that side: https://github.com/metosin/spec-tools/pull/130/files

Show
Tommi Reiman added a comment - - edited Hi. What's the current status of this? Spec-tools fails to generate the JSON Schemas out of clojure.spec + records on cljs and there is a local fix proposed on that side: https://github.com/metosin/spec-tools/pull/130/files
Hide
Mike Fikes added a comment -

Hey Samuel, can you re-baseline your attached patch? (It no longer applies on master.)

Show
Mike Fikes added a comment - Hey Samuel, can you re-baseline your attached patch? (It no longer applies on master.)
Hide
Erik Assum added a comment -

I've created a new patch based on Samuels and kept the attribution.

Show
Erik Assum added a comment - I've created a new patch based on Samuels and kept the attribution.
Hide
Mike Fikes added a comment -

0001-CLJS-1297-defrecord-does-not-emit-IKVReduce-protocol.patch applies cleanly, passes CI and Canary.

Show
Mike Fikes added a comment - 0001-CLJS-1297-defrecord-does-not-emit-IKVReduce-protocol.patch applies cleanly, passes CI and Canary.

People

Vote (2)
Watch (8)

Dates

  • Created:
    Updated: