Fork me on GitHub
#cljs-dev
<
2018-01-25
>
mfikes00:01:35

cljs.analyzer/numeric-type? is still a bit relaxed here https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/analyzer.cljc#L2995 and I wonder if the correct test might be (contains? '#{js js/Number} t) This change would at least catch (< (js/Date.) (js/Date.)), with a

WARNING: cljs.core/<, all arguments must be numbers, got [js/Date js/Date] instead
diagnostic, but I wonder if it is too restrictive. The js is needed for forms like (< (js/parseInt "3") (js/parseInt "4")).

mfikes16:01:52

@dnolen High level summary of all the map entry tickets: With the latest patch in each (which is currently CLJS-2456.patch, CLJS-2477-3.patch, CLJS-2482.patch, CLJS-2460-2.patch, and CLJS-2478-2.patch), all tests pass, including the 30,977 tests / 121,688 assertions currently in Coal Mine. A potentially consequential change in CLJS-2478-2.patch is that vectors are no longer map entries. (Code that does something like (key [:a 1]) was always incorrect, but with the change it would actually fail.)

rauh17:01:40

@mfikes Does (into {} [[[:a 2] [:b 3]]]) also pass? It looks like it might fail because the loop still uses key/val

rauh17:01:20

Also, what's perf hit on this? The satisfies? is more expensive than implements and when -conj!ing vectors it will always be false for the first map-entry? check.

mfikes17:01:28

@rauh I haven't done any perf testing on the set of changes. Here is the result of your into question:

cljs.user=> (into {} [[[:a 2] [:b 3]]])
{[:a 2] [:b 3]}

rauh17:01:15

@mfikes Oh sorry, I mean to do: (into {} (list [:a 2] [:b 3]))

rauh17:01:58

This happens to work in CLJS currently, since it fails the vector & map-entry? check, and then creates a {:a 2, :b 3} map. Though it doesn't work in CLJ. So...

bronsa17:01:50

that also works in clj

rauh17:01:14

@bronsa Right. 3rd try is the charm. What I'm trying to say: This currently happens to work in CLJS (but not in CLJ):

(-> (transient {})
    (conj! (seq [[:a 2] [:b 3]]))
    (persistent!))

bronsa17:01:10

minimal repro is (conj {} '([1 2]))

mfikes18:01:10

@rauh

(-> (transient {})
    (conj! (seq [[:a 2] [:b 3]]))
    (persistent!))
fails with the changes with
Error: No protocol method IMapEntry.-key defined for type cljs.core/PersistentVector: [:a 2]

mfikes22:01:13

Thanks to the recent -Sdeps enhancement to the clojure tool, you can run Coal Mine against a ClojureScript master commit with a one-liner, without manually cloning anything:

clojure -Sdeps '{:deps {coal-mine {:git/url "" :sha "8486879d5381aae2eaa81f1404e152ff53afaab6"} org.clojure/clojurescript {:git/url "" :sha "9ddd356d344aa1ebf9bd9443dd36a1911c92d32f"} com.google.javascript/closure-compiler-unshaded {:mvn/version "v20170910"}}}' -J-Xmx3G -m coal-mine.script test
This ends up looking like
Cloning: 
Checking out:  at 8486879d5381aae2eaa81f1404e152ff53afaab6
Cloning: 
Checking out:  at 9ddd356d344aa1ebf9bd9443dd36a1911c92d32f

Building coal-mine.test-runner-1 ...
Running coal-mine.test-runner-1 in Node ...

Testing coal-mine.problem-1

Testing coal-mine.problem-2
...

mfikes22:01:54

Of course, you can also run it against a local checkout of the ClojureScript tree by using :local/root instead. I’m liking this flexibility 🙂

Alex Miller (Clojure team)22:01:50

good to see all the pieces coming together