bug in clojure.walk/walk.clj for clojure.lang.IMapEntry ?

[Yehonathan Sharvit]

`walk` is supposed to build up a data structure of the same type as its input.

With `clojure.lang.IMapEntry`, it doesn't work as expected.

The output is of type: `clojure.lang.PersisentVector`

(class (walk/walk identity identity  (first {:a 1})) ); clojure.lang.PersisentVector

This is the code for `walk`:

(defn walk
	"Traverses form, an arbitrary data structure. inner and outer are
	functions. Applies inner to each element of form, building up a
	data structure of the same type, then applies outer to the result.
	Recognizes all Clojure data structures. Consumes seqs as with doall."
	{:added "1.1"}
	[inner outer form]
	(cond
	(list? form) (outer (apply list (map inner form)))
	(instance? clojure.lang.IMapEntry form) (outer (vec (map inner form)))
	(seq? form) (outer (doall (map inner form)))
	(instance? clojure.lang.IRecord form)
	(outer (reduce (fn [r x] (conj r (inner x))) form form))
	(coll? form) (outer (into (empty form) (map inner form)))
	:else (outer form)))

In the code, we see clearly that for  `clojure.lang.IMapEntry`, the return value is of type: `clojure.lang.PersisentVector`.

I'm suggesting the folllowing fix for `clojure.lang.IMapEntry`

   (instance? clojure.lang.IMapEntry form) (outer (first {(inner (key form)) (inner (val form))}))

What do you think?

[Alan Thompson]

You have been bitten the sequence abstraction.   Try this in the repl:

user=>  (first {:a 1})

;=> [:a 1]

any sequence operation converts maps into a sequence of MapEntry objects, which are length-2 vectors.  If you leave out first, it works:

user=>  (use 'clojure.walk)

user=>  (walk identity identity  {:a 1})

{:a 1}

Alan

--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to clojure-dev...@googlegroups.com.
To post to this group, send email to cloju...@googlegroups.com.
Visit this group at http://groups.google.com/group/clojure-dev.
For more options, visit https://groups.google.com/d/optout.

[Yehonathan Sharvit]

I think you are missing something:

(class (first {:a 1})); clojure.lang.MapEntry

--
You received this message because you are subscribed to a topic in the Google Groups "Clojure Dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/clojure-dev/IQti2AdHf7M/unsubscribe.
To unsubscribe from this group and all its topics, send an email to clojure-dev...@googlegroups.com.

To post to this group, send email to cloju...@googlegroups.com.
Visit this group at http://groups.google.com/group/clojure-dev.
For more options, visit https://groups.google.com/d/optout.

--

"Are we what we become or do we become what we are?" - Prof. Beno Gross

[Alan Thompson]

I do not understand.  Try this in the repl:

user=> (def the-first (first {:a 1}) )

#'user/the-first

user=> (class the-first)

clojure.lang.PersistentVector

user=> (doseq [it (supers (class the-first))] (println it))

clojure.lang.Indexed

clojure.lang.IHashEq

java.util.Collection

clojure.lang.IPersistentStack

clojure.lang.IReduceInit

clojure.lang.Reversible

clojure.lang.APersistentVector

java.lang.Comparable

java.lang.Runnable

clojure.lang.IPersistentCollection

clojure.lang.AFn

clojure.lang.Associative

clojure.lang.IFn

clojure.lang.IPersistentVector

java.util.List

clojure.lang.Sequential

clojure.lang.IEditableCollection

java.util.Map$Entry

java.util.RandomAccess

clojure.lang.IReduce

clojure.lang.IMeta

java.util.concurrent.Callable

clojure.lang.IMapEntry

clojure.lang.Seqable

clojure.lang.IObj

java.lang.Iterable

clojure.lang.IKVReduce

clojure.lang.ILookup

java.io.Serializable

clojure.lang.Counted

java.lang.Object

The point is that, whether you view it as a clojure.lang.PersistentVector, clojure.lang.IPersistentVector, java.util.Map$Entry, clojure.lang.IMapEntry, or whatever, calling (first {:a 1}) transforms the map {:a 1} into something that is not a map.  That is why walk fails to do what you would expect.

Alan

[Alex Miller]

I suspect the real problem here (if there is one) is not with map entries, but with PersistentVectors, which will (as of 1.8) get caught in the IMapEntry branch there. There is a new map-entry? that can be used to get a better outcome, but I suspect you still may want something different.

I would appreciate a jira on this as I would like to consider it more before a final 1.8 release. 

[Yehonathan Sharvit]

Here is the jira ticket: http://dev.clojure.org/jira/browse/CLJ-1847

[Yehonathan Sharvit]

Alan,

In my REPL (clojure 1.7.0), I have:

user=> (class (first {:a 1}))

clojure.lang.MapEntry

What clojure version are you using? what REPL?

--
You received this message because you are subscribed to a topic in the Google Groups "Clojure Dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/clojure-dev/IQti2AdHf7M/unsubscribe.
To unsubscribe from this group and all its topics, send an email to clojure-dev...@googlegroups.com.
To post to this group, send email to cloju...@googlegroups.com.
Visit this group at http://groups.google.com/group/clojure-dev.
For more options, visit https://groups.google.com/d/optout.

[Nicola Mometto]

I don't think there's any bug here.

clojure.walk/walk guarantees "building up a data structure of the same type", which I woulnd't interpret as "of the same class".

Note that for example, for every IPersistentList, it will return a PersistentList, for every ISeq it will return a LazySeq etc.

Given that in 1.8 PersistentVector is an IMapEntry, I don't see where's the problem with returning a PV when handed an IME.

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

To unsubscribe from this group and stop receiving emails from it, send an email to clojure-dev...@googlegroups.com.

[Nicola Mometto]

If anything, I think that IMapEntry instance check should simply go as it's now covered by the coll? check 

[Alan Thompson]

REPL-y 0.3.7, nREPL 0.2.10

Clojure 1.8.0-RC1

Java HotSpot(TM) 64-Bit Server VM 1.8.0_66-b17

[Yehonathan Sharvit]

Alan, now I understand it behaves differently in 1.7.0 and 1.8.0!

--
You received this message because you are subscribed to a topic in the Google Groups "Clojure Dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/clojure-dev/IQti2AdHf7M/unsubscribe.
To unsubscribe from this group and all its topics, send an email to clojure-dev...@googlegroups.com.
To post to this group, send email to cloju...@googlegroups.com.
Visit this group at http://groups.google.com/group/clojure-dev.
For more options, visit https://groups.google.com/d/optout.

[Yehonathan Sharvit]

Something is very confusing with the behaviour of walk for MapEntries

Let's say I want to implement `keywordize-keys`:

(defn map-keyword [x]

  (if (instance? clojure.lang.IMapEntry x)

    (first {(keyword (first x)) (second x)})

    x))

(walk/prewalk map-keyword {"a" "AAA" "b" ["c" "d"]})

(walk/postwalk map-keyword {"a" "AAA" "b" ["c" "d"]})

In Clojure 1.7.0, prewalk is fine but postwalk doesn't work.

(walk/prewalk map-keyword {"a" "AAA" "b" ["c" "d"]}); {:a "AAA", :b ["c" "d"]}

(walk/postwalk map-keyword {"a" "AAA" "b" ["c" "d"]}); {"a" "AAA", :b ["c" "d"]}

The reason is because `walk` returns a vector for map entry.

In Clojure 1.8.0-RC1, both prewalk and postwalk don't work:

(defn map-keyword [x]

  (if (map-entry? x)

    (first {(keyword (first x)) (second x)})

    x))

(walk/prewalk map-keyword {"a" "AAA" "b" ["c" "d"]}); {:a "AAA", :b [:c "d"]}

(walk/postwalk map-keyword {"a" "AAA" "b" ["c" "d"]}){:a "AAA", :b [:c "d"]}

The reason is because a vector of vector of length 2 is considered as a map entry

(map-entry? [1 2]); true

To me, it seems very confusing.

[Sean Corfield]

Something is very confusing with the behaviour of walk for MapEntries

Let's say I want to implement `keywordize-keys`:

I guess I’m confused about what you’re trying to do… clojure.walk/keywordize-keys already exists and is defined as:

(defn keywordize-keys

  "Recursively transforms all map keys from strings to keywords."

  {:added "1.1"}

  [m]

  (let [f (fn [[k v]] (if (string? k) [(keyword k) v] [k v]))]

    ;; only apply to maps

    (postwalk (fn [x] (if (map? x) (into {} (map f x)) x)) m)))

This does the right thing in 1.8.0-RC1 (and earlier).

Sean

[Yehonathan Sharvit]

I was mentioning `keywordize-keys` as an example.

You could think about trying to implement something like `keywordize-values` where the string values are converted to keyword.

Anyway, my implementation of the `map-keyword`, where I check if the form is a MapEntry,  seems natural. I know I could do it like the clojure implentation, by checking if the form is a `map`. 

But I don't understand why my implementation is failing. I mentioned it as an example of the consequences of the "bug" in `walk`.

--
You received this message because you are subscribed to a topic in the Google Groups "Clojure Dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/clojure-dev/IQti2AdHf7M/unsubscribe.
To unsubscribe from this group and all its topics, send an email to clojure-dev...@googlegroups.com.
To post to this group, send email to cloju...@googlegroups.com.
Visit this group at http://groups.google.com/group/clojure-dev.
For more options, visit https://groups.google.com/d/optout.

[Alex Miller]

Before Clojure 1.8, map entries were all subclasses of AMapEntry (most importantly MapEntry in PersistentHashMap). As of 1.8, PHM now returns PersistentVectors instead. There are some tradeoffs in this approach. One of those is that PersistentVector now implements IMapEntry. 

We added the map-entry? predicate to contain the logic to exclude PVs that are not size 2. However, PVs returned from a PHM are indistinguishable from any other PV and thus map-entry? (in your example) snares a 2-element vector value as well. One way to resolve this (in this example) is to use more context, specifically to do the work in the context of the map, instead of the map entry, which is effectively what keywordize-keys in core is already doing. 

Perhaps the broader comment here is that a bit more care is needed in any case-based analysis that involves map entries (because map entries are persistent vectors but persistent vectors are not necessarily map entries). The transformation function here is not respecting the latter possibility and the only useful way to resolve that in this case is to only do this transformation from the map context, not from the entry context.

On Saturday, November 14, 2015 at 10:08:25 PM UTC-6, Yehonathan Sharvit wrote:

I was mentioning `keywordize-keys` as an example.

You could think about trying to implement something like `keywordize-values` where the string values are converted to keyword.

Anyway, my implementation of the `map-keyword`, where I check if the form is a MapEntry,  seems natural. I know I could do it like the clojure implentation, by checking if the form is a `map`. 

But I don't understand why my implementation is failing. I mentioned it as an example of the consequences of the "bug" in `walk`.

On Sun, Nov 15, 2015 at 4:48 AM, Sean Corfield <se...@corfield.org> wrote:

Something is very confusing with the behaviour of walk for MapEntries

Let's say I want to implement `keywordize-keys`:

I guess I’m confused about what you’re trying to do… clojure.walk/keywordize-keys already exists and is defined as:

(defn keywordize-keys

  "Recursively transforms all map keys from strings to keywords."

  {:added "1.1"}

  [m]

  (let [f (fn [[k v]] (if (string? k) [(keyword k) v] [k v]))]

    ;; only apply to maps

    (postwalk (fn [x] (if (map? x) (into {} (map f x)) x)) m)))

This does the right thing in 1.8.0-RC1 (and earlier).

Sean

--
You received this message because you are subscribed to a topic in the Google Groups "Clojure Dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/clojure-dev/IQti2AdHf7M/unsubscribe.

To unsubscribe from this group and all its topics, send an email to clojure-dev+unsubscribe@googlegroups.com.

To post to this group, send email to cloju...@googlegroups.com.
Visit this group at http://groups.google.com/group/clojure-dev.
For more options, visit https://groups.google.com/d/optout.

[Colin Fleming]

Alex, could you describe the motivation for this change? What is the advantage to returning PVs instead of MapEntries?

To unsubscribe from this group and all its topics, send an email to clojure-dev...@googlegroups.com.

To post to this group, send email to cloju...@googlegroups.com.
Visit this group at http://groups.google.com/group/clojure-dev.
For more options, visit https://groups.google.com/d/optout.

--

"Are we what we become or do we become what we are?" - Prof. Beno Gross

--

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

To unsubscribe from this group and stop receiving emails from it, send an email to clojure-dev...@googlegroups.com.