Fork me on GitHub

hello, while implementing a custom map type in CLJS, I noticed that the `contains?` fn doesn't use the `-contains-key?` from `IAssociative`. I'm making the impl on both CLJ and CLJS, on CLJ it uses the `-contains-key?`, is this a bug on CLJS or is there are reason for it?


by looking at sources, the impl of `contains?` in CLJ is very different, the CLJS one just relies on `get`, while the CLJ has a bunch of type checks


The CLJ one was implemented first, CLJS significantly later. I have heard (but have not studied in detail myself) that CLJS tends to implement much more using Clojure's defprotocol, where in CLJ similar things were implemented via Java interfaces and methods.


yeah, the CLJS came already with protocols, so their DS were started with then already. what is not making sense to me here, is that CLJS does have the IAssociative protocol that contains the -contains-key? method, so why the CLJ contains? does check and use this protocol, but CLJS doesn't?


Check the git history, but I suspect the answer is just “no one noticed”. In early days cljs was written quickly to achieve parity with clj and corners were cut


A quick 'git blame' on the source file containing defn of contains? in ClojureScript shows it was last changed in 2013, and that was a one-line change on the line that uses get in the implementation of contains?. Seeing what it was before that is taking a bit longer, because the file was moved from src/cljs/core.cljs to src/main/cljs/core.cljs in 2015, after that change was made, and I'm still trying to figure out how to see history of a file that has been removed...


Looks like contains? called -lookup before this commit and changed to use get with that commit, along with many other changes. Not clear to me why, but I've spent almost no time looking at ClojureScript internals, compared to Clojure/JVM internals.


From looking further at the changes to get in that commit, it appears that get was generalized to work on all things implementing ILookup, but also to work on Google Closure arrays and JavaScript strings. contains? changing to use get at the same time enabled contains? to work on those types, too.


gotcha, the issue with the get approach is that it forces the data structure to realize the value, which a specialized -contains-key? can do without realizing the value. I guess the most compelling argument to change this is the parity, given Clojure contains? does work with -contains-key?


this is what CLJ contains? is:

public static Object contains(Object coll, Object key) {
        if (coll == null) {
            return F;
        } else if (coll instanceof Associative) {
            return ((Associative)coll).containsKey(key) ? T : F;
        } else if (coll instanceof IPersistentSet) {
            return ((IPersistentSet)coll).contains(key) ? T : F;
        } else if (coll instanceof Map) {
            Map m = (Map)coll;
            return m.containsKey(key) ? T : F;
        } else if (coll instanceof Set) {
            Set s = (Set)coll;
            return s.contains(key) ? T : F;
        } else if (!(key instanceof Number) || !(coll instanceof String) && !coll.getClass().isArray()) {
            if (coll instanceof ITransientSet) {
                return ((ITransientSet)coll).contains(key) ? T : F;
            } else if (coll instanceof ITransientAssociative2) {
                return ((ITransientAssociative2)coll).containsKey(key) ? T : F;
            } else {
                throw new IllegalArgumentException("contains? not supported on type: " + coll.getClass().getName());
        } else {
            int n = ((Number)key).intValue();
            return n >= 0 && n < count(coll);


so CLJS version could check for the protocols as clojure does, before going down to get


I do not understand what you mean when you say "Clojure does work with -contains-key?". There is no -contains-key? in Clojure.


@U0CMVHBL2 the equivalent is the containsKey method


well, Associative.containsKey and IPersistentSet.contains()


@U066U8JQJ I wouldn’t look for a deep reason here; I really think it’s either oversight or expedience


there were similar issues with find which I had no trouble getting a patch accepted for:


cool, thanks, I'll open a ticket