Skip to:

  1. Skip to Jira Navigation
  2. Skip to Side Navigation
  3. Skip to Main Content

Enable destructuring of sequency maps

Description

At present, Clojure's destructuring implementation will create a hash-map from any encountered value satisfying clojure.core/seq? This has the I argue undesirable side effect of making it impossible to employ destructuring on a custom Associative type which is also a Seq. This came up when trying to destructure instances of a tagged value class which for the purpose of pattern matching behave as [k v] seqs, but since the v is known to be a map, are also associative on the map part so as to avoid the syntactic overhead of updates preserving the tag.

;; A sketch of such a type (deftype ATaggedVal [t v] clojure.lang.Indexed (nth [self i] (nth self i nil)) (nth [self i o] (case i (0) t (1) v o)) clojure.lang.Sequential clojure.lang.ISeq (next [this] (seq this)) (first [this] t) (more [this] (.more (seq this))) (count [this] 2) (equiv [this obj] (= (seq this) obj)) (seq [self] (cons t (cons v nil))) clojure.lang.Associative (entryAt [self key] (.entryAt v key)) (assoc [_ sk sv] (ATaggedVal. t (.assoc v sk sv))) clojure.lang.ILookup (valAt [self k] (.valAt v k)) (valAt [self k o] (.valAt v k o)) clojure.lang.IPersistentMap (assocEx [_ sk sv] (ATaggedVal. t (.assocEx v sk sv))) (without [_ sk] (ATaggedVal. t (.without v sk))))

So using such a thing,

(let [{:keys [x]} (ATaggedVal. :foo {:x 3 :y 4})] x) ;; expect 3 => nil

Since for any type T such that clojure.core/get will behave, T should satisfy clojure.core/map? it should be correct simply to change the behavior of destructure to only build a hash-map if map? isn't already satisfied.

The attached patch makes this change.

Environment

None

Attachments

1
  • 22 Aug 2015, 04:47 PM

Activity

Show:

Reid D McKenzie August 26, 2015 at 7:15 PM

I contend that the behavior broken is, at best, undefined behavior consequent from the implementation and that the failure to cast exception is at least clearer than the silent nil behavior of the original implementation.

I would personally prefer to extend the destructuring checks logically to (cond map? x seq? (hash-map (seq x)) :else (throw "Failed to destructure non-map type") but I think that change is sufficiently large that it would meaningfully decrease the chances of this patch being accepted.

Ragnar Dahlén August 24, 2015 at 7:20 AM

The current patch for https://clojure.atlassian.net/browse/CLJ-1778#icft=CLJ-1778 does not address this issue.

The idea seems sound to me, if we're map destructuring a value that's
already a map (as determined by map?), we don't need to create a
new one by calling seq and HashMap/create, unless there's a
really good reason it should be exactly that map implementation (I
don't see one).

I don't think the current patch is OK though as it makes an
(unneccessary) breaking change to the behaviour of map destructuring.
Previously, destructuring a non-seqable value returned nil, but with
patch, seq is always called on value and for non-seqble types this
will instead throw an exception. It should be trivial to change the
patch to retain the original behaviour.

1.8.0-master:

user=> (let [{:keys [x]} (java.util.Date.)] x) nil

with 0001-Enable-destructuring-of-seq-map-types.patch:

user=> (let [{:keys [x]} (java.util.Date.)] x) IllegalArgumentException Don't know how to create ISeq from: java.util.Date clojure.lang.RT.seqFrom (RT.java:528)

Alex Miller August 22, 2015 at 7:21 PM

Probably worth watching https://clojure.atlassian.net/browse/CLJ-1778#icft=CLJ-1778 too which might cause this not to apply anymore.

Could you add an example of what doesn't work to the description?

Details

Assignee

Reporter

Labels

Approval

Triaged

Patch

Code

Priority

Created August 22, 2015 at 4:47 PM
Updated August 26, 2015 at 7:15 PM

Flag notifications