Fork me on GitHub
#cljs-dev
<
2020-10-01
>
wilkerlucio22:10:17

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?

wilkerlucio22:10:45

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

andy.fingerhut02:10:06

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.

wilkerlucio04:10:20

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?

favila09:10:14

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

andy.fingerhut14:10:56

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

andy.fingerhut14:10:05

Looks like contains? called -lookup before this commit https://github.com/clojure/clojurescript/commit/7e7f3a76eefd621165259c0f24d6507318dca029 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.

andy.fingerhut14:10:23

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.

wilkerlucio14:10:14

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?

wilkerlucio14:10:23

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);
        }
    }

wilkerlucio14:10:13

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

andy.fingerhut14:10:33

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

favila14:10:20

@U0CMVHBL2 the equivalent is the containsKey method

favila14:10:46

well, Associative.containsKey and IPersistentSet.contains()

favila14:10:36

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

favila14:10:48

there were similar issues with find which I had no trouble getting a patch accepted for: https://clojure.atlassian.net/browse/CLJS-2136

wilkerlucio15:10:44

cool, thanks, I'll open a ticket