<< Back to previous view

[CLJS-583] #{x y} can produce set with duplicates Created: 04/Sep/13  Updated: 17/Nov/13  Resolved: 17/Nov/13

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0002-CLJS-583-throw-on-duplicates-in-set-literals.patch    

 Description   
(def x 1)
(def y 1)
#{x y}
;= #{1 1}

This is because cljs.core.PersistentHashSet/fromArray does not check for duplicates. Clojure performs the check and throws an exception in the presence of duplicates; I'll replicate this behaviour in the forthcoming patch.



 Comments   
Comment by Michał Marczyk [ 04/Sep/13 2:15 AM ]

Fix with test.

NB. if all the members are themselves literals, then we already throw at read time. This patch fixes cases like the one in the ticket description.

As a minor optimization, the runtime check for duplicates is skipped if all the member expressions of the set literal are constants.

Also, I notice that we have a similar problem with maps. I'll fix that in a separate patch (and ticket) building on this one.

Comment by Michał Marczyk [ 04/Sep/13 2:28 AM ]

Actually, wait, I need to put that description in the commit message too. Will attach a patch with a better commit message in a second.

Comment by Michał Marczyk [ 04/Sep/13 2:34 AM ]

Proper commit message.

Comment by Michał Marczyk [ 04/Sep/13 2:36 AM ]

Wait, actually this didn't seem to work... Let's try again.

Comment by Michał Marczyk [ 04/Sep/13 2:47 AM ]

Proper commit message. For real this time.

Comment by David Nolen [ 04/Sep/13 11:08 PM ]

See CLJS-516

Comment by Michał Marczyk [ 05/Sep/13 12:07 AM ]

I'll tackle CLJS-516, thanks for the pointer!

NB. the desired behaviour is different: literal sets and maps are supposed to throw on duplicates, array-map is supposed to work "as if by assoc". So, I don't think there'll be much in the way of code sharing between that and this patch or CLJS-584. (Clojure uses separate PersistentArrayMap methods in the two cases.)

Comment by Michał Marczyk [ 20/Sep/13 8:50 PM ]

Patch applicable on top of new patch for CLJS-516 (which applies to current master).

Comment by David Nolen [ 17/Nov/13 1:46 PM ]

fixed, https://github.com/clojure/clojurescript/commit/fe9b5577cd9b9784d0c0c75edccc4441c99ee924

Generated at Sat Dec 20 11:17:56 CST 2014 using JIRA 4.4#649-r158309.