ClojureScript

#{x y} can produce set with duplicates

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

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.

Activity

Hide
Michał Marczyk added a comment -

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

Show
Michał Marczyk added a comment - Patch applicable on top of new patch for CLJS-516 (which applies to current master).
Hide
Michał Marczyk added a comment -

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.)

Show
Michał Marczyk added a comment - 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.)
Hide
David Nolen added a comment - - edited
Show
David Nolen added a comment - - edited See CLJS-516
Hide
Michał Marczyk added a comment -

Proper commit message. For real this time.

Show
Michał Marczyk added a comment - Proper commit message. For real this time.
Hide
Michał Marczyk added a comment -

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

Show
Michał Marczyk added a comment - Wait, actually this didn't seem to work... Let's try again.
Hide
Michał Marczyk added a comment -

Proper commit message.

Show
Michał Marczyk added a comment - Proper commit message.
Hide
Michał Marczyk added a comment -

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.

Show
Michał Marczyk added a comment - 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.
Hide
Michał Marczyk added a comment -

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.

Show
Michał Marczyk added a comment - 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.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: