{:as x} destructuring fails for an empty list in 1.5.0.RC1

129 views
Skip to first unread message

Toby Crawley

unread,
Dec 30, 2012, 2:42:53 PM12/30/12
to cloju...@googlegroups.com

user=> (clojure-version)
"1.4.0"
user=> (let [{:as x} '()] x)
{}

user=> (clojure-version)
"1.5.0-RC1"
user=> (let [{:as x} '()] x)
IllegalArgumentException No value supplied for key: null clojure.lang.PersistentHashMap.create (PersistentHashMap.java:77)

The bug was introduced by a change[1] to support duplicate keys in map
destructuring. Using PersistentHashMap/create here introduces the above
bug, since it does not properly handle empty lists.

I've created CLJ-1140[2] with an attached patch to fix the issue.

[1]:https://github.com/clojure/clojure/commit/93c795fe10ee5c92a36b6ec6373b3c80a31135c4
[2]:http://dev.clojure.org/jira/browse/CLJ-1140
--
Toby Crawley
http://immutant.org | http://torquebox.org

Ambrose Bonnaire-Sergeant

unread,
Dec 30, 2012, 10:52:19 PM12/30/12
to cloju...@googlegroups.com
Where is this useful? IIRC rest arguments are either nil or a non-empty list.

Ambrose

--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
To post to this group, send email to cloju...@googlegroups.com.
To unsubscribe from this group, send email to clojure-dev...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/clojure-dev?hl=en.


Toby Crawley

unread,
Dec 30, 2012, 11:10:37 PM12/30/12
to cloju...@googlegroups.com

Ambrose Bonnaire-Sergeant writes:

> Where is this useful? IIRC rest arguments are either nil or a non-empty
> list.

This doesn't just apply to rest arguments, since we can destructure in a
let with values from anywhere.

I have code similar to the following that works under 1.4, but fails
under 1.5.0-RC1, since drop-while can return '() in the case of no kwarg
options:

(defn foo [& args]
(let [fns (take-while fn? args)
{:keys [bar fuzz] :as opts} (drop-while fn? args)]
...))

(foo f1 f2 :bar "baz")
(foo f1 f2) ;; throws under 1.5.0-RC1

I realize there are other ways to do the same thing, but since this
works under 1.4.0, I consider it a regression.

Ambrose Bonnaire-Sergeant

unread,
Dec 30, 2012, 11:20:22 PM12/30/12
to cloju...@googlegroups.com
On Mon, Dec 31, 2012 at 12:10 PM, Toby Crawley <to...@tcrawley.org> wrote:

Ambrose Bonnaire-Sergeant writes:

> Where is this useful? IIRC rest arguments are either nil or a non-empty
> list.

This doesn't just apply to rest arguments, since we can destructure in a
let with values from anywhere.

 
Sure, but I'm sceptical whether this is a good idea/intended.
I'm pretty sure this is undocumented behaviour.

Interesting to see whether this patch will be accepted.

Thanks,
Ambrose

Toby Crawley

unread,
Jan 2, 2013, 11:59:24 AM1/2/13
to cloju...@googlegroups.com

Ambrose Bonnaire-Sergeant writes:
> Sure, but I'm sceptical whether this is a good idea/intended.
> I'm pretty sure this is undocumented behaviour.

Ah, good point. I was working off of a few assumptions, some of which
were incorrect:

1) destructuring kwargs as a map was documented, but
http://clojure.org/special_forms#Special%20Forms--Binding%20Forms%20(Destructuring)[1]
currently makes no mention of it. Is it documented anywhere?

2) destructuring worked consistently in let bindings and fn args - any
destructuring that worked in one would work in the other. I believe
this was true in 1.4.0, but is no longer true, which led to my
patch.

3) destructuring worked consistently across collection types - vector
binding works with anything nthable, map binding works with
anything associative. I assumed that when kwarg map binding was
added, it supported anything seqable, but that is not the case - it
only supports ISeqs.

So the bigger questions are: should destructuring be consistent in
both usage and collection support? Should kwarg map binding be
documented?

If the status quo on those questions stands, then my patch should
probably not be applied. If it is decided that destructuring should be
consistent (in both ways above), then my patch should probably be
replaced with a patch that makes map binding work on anything seqable.
Reply all
Reply to author
Forward
0 new messages