Fork me on GitHub
#cljs-dev
<
2018-01-26
>
mfikes00:01:19

FWIW, I found that completely avoiding shell scripts for "build logic" and instead just writing it in Clojure made the above much easier and cleaner to pull off.

Alex Miller (Clojure team)01:01:26

for sure (I’ve started doing some of the same around the clojure api docs)

mfikes15:01:07

@dnolen If you end up doing "ClojureScript Friday" work, I'd vote to get the map entry stuff in, but only if there is not an immediate release also planned. (The map entry stuff seems solid, but every now and then we still find a new corner case to address.)

dnolen18:01:18

@mfikes applying and testing these now

dnolen18:01:22

@mfikes for 2478 let’s a get patch that uses implements? instead of satisfies?

mfikes19:01:17

Sorry, @dnolen I’m trying that simple change… but it is failing. Attempting to figure out why.

TypeError: this.mseq.cljs$core$ISeq$_first$arity$1(...).cljs$core$IMapEntry$_key$arity$1 is not a function
    at cljs.core.KeySeq.cljs$core$ISeq$_first$arity$1 (builds/out-simp/core-simple-test.js:1676:411)
    at Object.cljs.core.first (builds/out-simp/core-simple-test.js:534:165)
    at Object.cljs.core.zipmap (builds/out-simp/core-simple-test.js:1756:213)
    at Object.cljs.pprint.map_params (builds/out-simp/core-simple-test.js:2840:144)
    at Object.cljs.pprint.compile_directive (builds/out-simp/core-simple-test.js:2842:284)
    at Function.<anonymous> (builds/out-simp/core-simple-test.js:2870:205)
    at Function.cljs.core.apply_to_simple.cljs$core$IFn$_invoke$arity$3 (builds/out-simp/core-simple-test.js:897:157)
    at Function.cljs.core.apply_to_simple.cljs$core$IFn$_invoke$arity$2 (builds/out-simp/core-simple-test.js:896:188)
    at Function.cljs.core.apply.cljs$core$IFn$_invoke$arity$2 (builds/out-simp/core-simple-test.js:909:244)
    at Object.cljs.pprint.consume (builds/out-simp/core-simple-test.js:2209:107)

dnolen19:01:48

@mfikes hrm that code gen seems a bit strange no?

dnolen19:01:05

two protocol methods chained together like that?

mfikes19:01:07

A direct minimal repro is (zipmap [:a] [1])

mfikes19:01:25

This causes:

Error: No protocol method IMapEntry.-key defined for type cljs.core/PersistentVector: [:a 1]
    at Object.cljs$core$missing_protocol [as missing_protocol] (/Users/mfikes/Projects/clojurescript/out/.cljs_node_repl/cljs/core.js:326:9)
    at Object.cljs$core$_key [as _key] (/Users/mfikes/Projects/clojurescript/out/.cljs_node_repl/cljs/core.js:2000:17)
    at Object.cljs$core$key [as key] (/Users/mfikes/Projects/clojurescript/out/.cljs_node_repl/cljs/core.js:29149:18)
    at /Users/mfikes/Projects/clojurescript/out/.cljs_node_repl/cljs/core.js:33326:32
    at Object.cljs$core$pr_sequential_writer [as pr_sequential_writer] (/Users/mfikes/Projects/clojurescript/out/.cljs_node_repl/cljs/core.js:32530:142)
    at Object.cljs$core$print_prefix_map [as print_prefix_map] (/Users/mfikes/Projects/clojurescript/out/.cljs_node_repl/cljs/core.js:33325:18)
    at Object.cljs$core$print_map [as print_map] (/Users/mfikes/Projects/clojurescript/out/.cljs_node_repl/cljs/core.js:33346:18)
    at cljs.core.PersistentArrayMap.cljs$core$IPrintWithWriter$_pr_writer$arity$3 (/Users/mfikes/Projects/clojurescript/out/.cljs_node_repl/cljs/core.js:33573:18)
    at Object.cljs$core$pr_writer_impl [as pr_writer_impl] (/Users/mfikes/Projects/clojurescript/out/.cljs_node_repl/cljs/core.js:32709:12)
    at Object.cljs$core$pr_writer [as pr_writer] (/Users/mfikes/Projects/clojurescript/out/.cljs_node_repl/cljs/core.js:32830:18)

dnolen19:01:47

@mfikes did you drop IMapEntry impl from vectors?

dnolen19:01:23

if so I don’t think that’s necessary

mfikes19:01:53

Yes… that is part of the change in CLJS-2478. So, I took patch 2 from that ticket and replaced satisfies? with implements? in the map-entry? predicate.

dnolen19:01:43

let’s revert in the 2478 patch and mention it

dnolen19:01:58

It doesn’t matter that vectors are IMapEntry

dnolen19:01:52

let’s pause

dnolen19:01:57

something is wrong here

mfikes19:01:33

OK… yeah. I would have thought implements? would have had only a perf effect, not derailing functinoality.

dnolen19:01:53

but I think it’s symptom of something else

dnolen19:01:15

yeah I don’t really understand that stacktrace

dnolen19:01:50

need to understand how printing is trigger a call to -key

dnolen19:01:01

zipmap makes a transient

dnolen19:01:17

TransientArrayMap needs to support vectors

dnolen19:01:28

so dropping IMapEntry from vectors is not a good idea

dnolen19:01:03

there might be another place

dnolen19:01:25

searching for current places where we call (satisfies? IMapEntry ...) should be helpful

dnolen19:01:40

(side note, I think all those should be implements? anyway)

mfikes19:01:04

If we allow vectors to remain as map entries (and they satisfy the map-entry? predicate), there are two places that, in hindsight, I think I was assuming they wouldn’t be, and now I’m re-questioning correctness: https://github.com/clojure/clojurescript/blob/d1dc3c53c778772b176fc2163bf090f56e2a75e6/src/main/cljs/clojure/walk.cljs#L47 https://github.com/clojure/clojurescript/blob/d1dc3c53c778772b176fc2163bf090f56e2a75e6/src/main/cljs/cljs/core.cljs#L10542-L10543

mfikes19:01:25

(In other words, those two places would incorrectly handle vectors.)

dnolen19:01:24

@mfikes ah ok then the right thing to do is align with Clojure

dnolen19:01:41

where Clojure support vectors we need to support them too

dnolen19:01:18

so stick with vectors are not IMapEntry, and fix those places where Clojure takes them to also take them (not just check IMapEntry)

dnolen19:01:01

to prevent breakage we can actually extend vector to IMapEntry (not inline, in order to ensure implements? IMapEntry fails for vectors)

dnolen19:01:20

but internally we going to separate the checks into implements? IMapEntry + vector? check

mfikes19:01:44

Cool. I want to understand the aspect about TransientArrayMap needing to support vectors and how just using implements? causes things to fail. I'm assuming this reflects the logic you have in mind above.

mfikes19:01:00

I'll read the code more thoroughly.

dnolen19:01:46

well it worked right?

dnolen19:01:04

-conj! could take vector in TAM case

dnolen19:01:27

but my suggestion showed that switching to implements? means that would stop working

dnolen19:01:01

so we need implements? case and vector? case

dnolen19:01:50

inside of TAM -conj! impl

mfikes19:01:07

Cool. I'll get there in a second. 🙂 I think your understanding is a step or two ahead of mine right now . 😜

dnolen19:01:35

well let’s break it down again

dnolen19:01:44

1. Vectors are no longer IMapEntry

dnolen19:01:00

2. For performance we should use implements? for IMapEntry checks

dnolen19:01:25

3. Any previous place where vectors were treated as IMapEntry must now handle vectors explicitly

dnolen19:01:11

4. To prevent breakage, we should extend-type vectors to IMapEntry (`satisfies? will work implements?` won’t)

dnolen19:01:37

actually #4 may not be true

dnolen19:01:47

so let’s consider that last

dnolen19:01:56

and see how much breakage this might cause

mfikes19:01:30

Cool. There is evidently still a place that falls under (3) that is not being addressed in the patch. (The fallout of how zipmap behaves.)

dnolen19:01:47

well zipmap will make a TAM

dnolen19:01:06

(transient {}) is in zipmap

mfikes20:01:22

OK @dnolen I sorted out what was going on. It was a CM error on my part. CLJS-2478-2.patch is designed to be applied after CLJS-2456.patch. I mistakenly went into the local repo where I had produced CLJS-2478-2.patch, changed satisfies? to implements? and simply ran the tests. The tests failed because that repo did not have CLJS-2456.patch applied. The failure essentially took the form of where a seq on a map would return vectors, not map entries, and the print code (correctly) expects map entries and was applying key. Sorry for the long explanation, but this CM screw up partly explains why I was at a complete loss as to understand what was going on. TLDR: Changing satisfies? to implements? results in the patch working, as you would expect. Will attach it to the ticket now.

anmonteiro22:01:30

cljs.user=> 100/3
clojure.lang.ExceptionInfo: failed compiling constant: 100/3; class clojure.lang.Ratio is not a valid ClojureScript constant. {:constant 100/3, :type clojure.
lang.Ratio}

anmonteiro22:01:38

should we support emitting these?

mfikes22:01:21

(This is one of the few differences with self-hosted, which reads 100/3 as 33.333333333333336.)