Clojure

clojure.walk/walk returns a PersistentVector when the input is of type IMapEntry

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Declined
  • Affects Version/s: Release 1.7
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code

Description

`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`

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

Activity

Hide
Alex Miller added a comment -

Thanks for filing this. I spent some time looking at it and I don't think there is anything that needs to be done at this time.

Re your description, "`walk` is supposed to build up a data structure of the same type as its input." Regarding IMapEntry.... prior to 1.8, IMapEntry concrete types were descendants of AMapEntry, in particular the most common concrete type was MapEntry, which were used in PersistentHashMap.

In 1.8, PHM now returns PersistentVectors, which now implement IMapEntry. So, walk now takes a map entry (which is a PV) and returns a PV, which is an IMapEntry. So I believe the contract is still satisfied.

The new map-entry? predicate can be used to catch only 2-element PVs (not other counts) as map entries, however it is important to still consider whether you will incorrectly snare 2 element vectors that aren't in a map in this particular case. The current clojure.walk code however will have the same effective result in either case so the clojure.walk code does not need to change (and in fact non-PV entries still exist in sorted maps).

Show
Alex Miller added a comment - Thanks for filing this. I spent some time looking at it and I don't think there is anything that needs to be done at this time. Re your description, "`walk` is supposed to build up a data structure of the same type as its input." Regarding IMapEntry.... prior to 1.8, IMapEntry concrete types were descendants of AMapEntry, in particular the most common concrete type was MapEntry, which were used in PersistentHashMap. In 1.8, PHM now returns PersistentVectors, which now implement IMapEntry. So, walk now takes a map entry (which is a PV) and returns a PV, which is an IMapEntry. So I believe the contract is still satisfied. The new map-entry? predicate can be used to catch only 2-element PVs (not other counts) as map entries, however it is important to still consider whether you will incorrectly snare 2 element vectors that aren't in a map in this particular case. The current clojure.walk code however will have the same effective result in either case so the clojure.walk code does not need to change (and in fact non-PV entries still exist in sorted maps).
Hide
David Bürgin added a comment -

Hello Alex Miller, I ran into this while trying to walk a Hiccup-like data structure.

The goal was to namespace-qualify all elements in the data structure.

(->> [:outer {:attr "x"}
      [:inner "text"]]
     (clojure.walk/prewalk (fn [x]
                             (if (and (sequential? x) (not (map-entry? x)))
                               (into [(->> x first name (keyword "q"))] (rest x))
                               x))))
;; => [:q/outer {:attr "x"}
;;     [:q/inner "text"]]

This works only with clojure.walk/prewalk, not with postwalk, because only during prewalk is the map entry type present. When using postwalk, the condition is false and the attribute gets namespace-qualified too (which we want to avoid).

The patch would fix this. Would you consider reopening this issue? Or is this still according to the contract?

Show
David Bürgin added a comment - Hello Alex Miller, I ran into this while trying to walk a Hiccup-like data structure. The goal was to namespace-qualify all elements in the data structure.
(->> [:outer {:attr "x"}
      [:inner "text"]]
     (clojure.walk/prewalk (fn [x]
                             (if (and (sequential? x) (not (map-entry? x)))
                               (into [(->> x first name (keyword "q"))] (rest x))
                               x))))
;; => [:q/outer {:attr "x"}
;;     [:q/inner "text"]]
This works only with clojure.walk/prewalk, not with postwalk, because only during prewalk is the map entry type present. When using postwalk, the condition is false and the attribute gets namespace-qualified too (which we want to avoid). The patch would fix this. Would you consider reopening this issue? Or is this still according to the contract?
Hide
Alex Miller added a comment -

I think this is the same as https://dev.clojure.org/jira/browse/CLJ-2031 which is just waiting for Rich’s review.

Show
Alex Miller added a comment - I think this is the same as https://dev.clojure.org/jira/browse/CLJ-2031 which is just waiting for Rich’s review.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: