Fork me on GitHub
#cljs-dev
<
2018-10-22
>
domkm15:10:52

For anyone familiar with the CLJS implementation of clojure.spec, do you have any insight into the difficulty of fixing this bug? https://dev.clojure.org/jira/browse/CLJS-2940

dnolen17:10:03

but I thought recursive specs worked fine, but maybe there was a regression

dnolen17:10:06

should get a test for that if true

mfikes17:10:51

I bet CLJS-2940 can be further constrained to things involving s/nilable (in other words, I think the ticket title incorrectly implies a broader problem than that exhibited in the description). I’ll update it.

domkm18:10:34

My mistake. I tested with s/nilable and assumed that applied more broadly than it did.

mikerod18:10:43

were recursive specs ever documented anywhere?

mikerod18:10:31

I was unsure about when I could get away with them or not before I remember

mikerod18:10:39

I think mutually recursive was particularly interesting

domkm18:10:48

I'm not sure about explicit documentation but there is s/*recursion-limit* so they are meant to be supported

mikerod18:10:57

ah, true there is that

mikerod18:10:06

yeah, I just always assumed they were supposed to be fine

mikerod18:10:14

mutually recursive was odd to write I belive in some cases

mikerod18:10:24

I’m going off on a tangent that is better suited for #clojure-spec channel now though, so nvm here.

mfikes18:10:31

I wonder if we could update cljs.core/merge-with to match clojure.core/merge-with, now that maps produce map entries. (Specifically changing first to key and second to val in the implementation.)

dnolen19:10:39

I think the previous change already forced people to stop relying on the old semantics

mfikes19:10:25

I’ll put together a ticket for it, with perf numbers. Then we can let it sit for a while for consideration / soaking.

mfikes19:10:19

Cool. Speedups in the ballpark of 1.3. Details in ticket.

mfikes20:10:55

Hah, here is an example of code this patch would break, which doesn't rely on the older use of 2-vectors as ersatz map entries, but simply on the fact that of first / second are being used in the merge-with implementation

(->> [:a 1 :b 2] (partition 2) (merge-with vector {}))
In the wild here https://github.com/gf3/secretary/blob/1f2036a694e49f58a97c9401878602148f9d6310/src/secretary/core.cljs#L252 TIL there is a name for this concept: Hyrum's Law http://www.hyrumslaw.com

mfikes20:10:00

(Note that (partition 2) isn't producing vectors, but lazy seqs of length 2. 🙂 )

mfikes20:10:19

Captured this in the ticket

Alex Miller (Clojure team)20:10:18

eww (re using sequences and first/second for entries)

mfikes20:10:03

I guess, since Clojure and ClojureScript are "popular", Hyrum's Law states that someone will have discovered that first and second are hiding inside merge-with

Alex Miller (Clojure team)20:10:39

map entries should always be indexed / positional

mfikes20:10:05

Hyrum's Law also seems to imply you can't change anything in core without breaking someone.

mfikes20:10:41

This merge-with use also seems to rely on an undocumented aspect of conj that happens to work (I can't recall why, but it feels like it is an accidental internal implementation detail):

(conj {:c 3} (seq {:a 1, :b 2}))

Alex Miller (Clojure team)20:10:51

conj on maps expects something that seqs to map entries (which can be maps, but can also be just seqs of map entries)

Alex Miller (Clojure team)20:10:27

keys and vals have the same relaxed expectation

Alex Miller (Clojure team)20:10:33

user=> (keys (seq {:a 1, :b 2}))
(:a :b)

Alex Miller (Clojure team)20:10:22

I am increasingly running into cases where it would be useful to be able to state that something “seqs to map entries” (often in spec-related stuff)

Alex Miller (Clojure team)20:10:06

Rich and I have talked about it a couple times, maybe that will emerge as an abstraction at some point, dunno

andy.fingerhut20:10:16

Sounds like some of the subtleties you have run across while spec'ing core, I guess?

Alex Miller (Clojure team)20:10:39

yep, although I think I ran into it in the spec implementation, not in a spec

artur11:10:27

I applied shadow-cljs to myu project and it seems to work just fine, except for hot reload. It does watch my changes and recompiles but in my browser the changes are not visible and refresh also doesn't bring changes.

artur13:10:18

Figured it out 🙂

Alex Miller (Clojure team)20:10:22

“seqs to map entries” is weaker than map? but still sufficiently interesting to be useful

dnolen20:10:35

@mfikes yeah I think people should fix stuff like that, seems gratuitous?

andy.fingerhut20:10:04

If those use cases you mention are by design, then "seq to map entries" definitely sounds like it could use a name in spec.

mfikes20:10:06

This is a completely new concept to me. TIL that seq? of map-entry? has meaning.

mfikes20:10:31

@dnolen Yeah, for that particular case I'll probably file a ticket against the downstream repo.

Alex Miller (Clojure team)20:10:44

@andy.fingerhut the problem is that there is no easy way to write that predicate

Alex Miller (Clojure team)20:10:53

without actually forcing the seq

andy.fingerhut20:10:40

Do you consider it a requirement, or just a really really good idea, for a spec not to force a seq?

mfikes20:10:25

I gotta say, this is totally badass. I never knew:

(keys (filter (comp pos? val) {:a 0, :b 1}))

borkdude20:10:36

more interestingly, the example from secretary uses this: (->> [:a 1 :a 2 :b 3] (partition 2) (merge-with vector {})) ;;=> {:a [1 2], :b 3}, so you cannot simply replace (partition 2) with (apply hash-map) to fix it. But converting the 2-colls to map-entries will make it work with the patch.

mfikes21:10:43

> conj on maps expects something that seqs to map entries (which can be maps, but can also be just seqs of map entries) Perhaps instead of "seqs to map entries" it is "is a seq of map entries"? (Compare (conj {:c 3} (vec (seq {:a 1, :b 2}))) and (conj {:c 3} (seq (vec (seq {:a 1, :b 2})))))

bronsa21:10:46

the minimal case is just (vec {..}) vs (seq {..})/`{}` FWIW

bronsa21:10:30

but yeah, the "problem" here is that vectors take the path of a map entry, rather than that of a collection of map entries

👍 4
bronsa21:10:46

the polymorphism of conj on maps has always been a weird edge-case IMO

👍 4
mfikes21:10:46

Yeah, and it is specific to conj because keys and vals are OK with it.

bronsa21:10:02

the spec according to the impl would be: conj on maps takes either a map entry or a vector of size 2 representing a map entry, or something that seqs to map entries which is not a vector

👍 4
mfikes21:10:22

Indeed. I agree.

mfikes21:10:26

Regardless of spec, an interesting question is whether the docstring for conj should essentially say what you just said. 🙂

mfikes21:10:45

Maybe even: "conj on maps takes either a map, a map entry or a vector of size 2 representing a map entry, or something that seqs to map entries which is not a vector"

bronsa21:10:00

>> conj expects another (possibly single entry) map as the item, and returns a new map which is the old map plus the entries from the new, which may overwrite entries of the old. conj also accepts a MapEntry or a vector of two items (key and value)

bronsa21:10:31

it never mentions acceptiong anything that seqs to map entries

mfikes21:10:40

Oh, shit. I didn't think to look there. Thanks.

bronsa21:10:03

and I suspect that the implementation accepts it just because that was the easiest way to implement map+map merging

bronsa21:10:14

hard to say

mfikes21:10:04

That's what I thought. It was an internal accident of implementation based on ease of implementation. But Alex's comment is that it is instead intended to be supported, not as an accident.

bronsa21:10:21

¯\(ツ)

bronsa21:10:40

maybe it simply graduated from accidental to not accidental as the rationale got lost in time, but the edge-cases never got ironed out?

mfikes21:10:52

I'd buy that.

mfikes23:10:01

ClojureScript master's Graal.JS support is still good with the recent GraalVM 1.0.0-rc8 release.