Clojurians Log v2

so direct linking does take care of fdefs not being checked when clojure calls core internally

I wonder if there’s a way to have fdefs, but having them faster than right now, or only print a warning instead of raising an exception

hmm, (=) seems to work in cljs, it returns true, but you’ll also get a warning

maybe we could leave out/disable specs that only check for any?, (s/* any?) or (s/+ any?) for performance sake. they check nothing, but they make things slower. e.g. = is important to be fast I think?

WIP for apply: https://github.com/borkdude/speculative/commit/23c85e675127e3793f620315682543e8138929d8 Tests pass on clj but not in cljs/planck.

plk:

ERROR in (apply-test) (d@file:23:179)
Uncaught exception, not in assertion.
expected: nil
  actual: #object[RangeError RangeError: Maximum call stack size exceeded.]

oh wow…

$ plk -A:test
ClojureScript 1.10.339
cljs.user=> (require '[speculative.core])
nil
cljs.user=> (require '[clojure.spec.test.alpha :as stest])
nil
cljs.user=> (stest/instrument `apply)
Maximum call stack size exceeded.
Maximum call stack size exceeded.

Same error as here: https://clojurians.slack.com/archives/CDJGJ3QVA/p1540142591000100

Added to JIRA: https://dev.clojure.org/jira/browse/CLJS-2942?focusedCommentId=50353#comment-50353

Well, at least speculative is proving some worth 🙂

Nice :)

bumped into this one on my work project: https://github.com/slipset/speculative/pull/31

rule of thumb: accept ifn?, return fn? ?

Not sure what’s up with github today. I get 404s on objects I created just a minute ago.

Github is in trouble today

I commented on your pr. An example where ifn? is valid for reduce would be appreciated 🙂

Also for my understanding.

WRT GitHub, I pushed a revision to Planck last night and it wouldn’t show up in the GitHub UI. It finally resolved itself this morning.

We have this:

(deftype SqlReduce [conn query params query-timeout fetch-size ptf]
  clojure.lang.IReduce
  (reduce [this f]
    (.reduce ^clojure.lang.IReduceInit this f (f)))
  clojure.lang.IReduceInit
  (reduce [this f init]
    (trace "DB: reducing sql query" query params)
    (jdbc/with-db-transaction [tx conn]
      (let [rfn (^:once fn* [coll]
                 (let [coll (if ptf (pmap ptf coll) coll)]
                   (reduce f init coll)))
            opts {:fetch-size fetch-size
                  :timeout query-timeout
                  :result-type :forward-only
                  :concurrency :read-only}
            prepared (jdbc/prepare-statement
                      (:connection tx) query opts)
            sql-params (cons prepared params)]
        (jdbc/query tx sql-params {:result-set-fn rfn})))))

I can look into more details

maybe it’s something transducer specific

merged

Whoah, I didn’t realize that this doesn’t work in Clojure: ([1] 7 :nf)

(Related to the ifn? / reduce question)

cljs.user=> (reduce [7] 0 [11])
7
cljs.user=> (reduce [7] 1 [11])
11

I think I’ve always assumed that things like (#{2} 1 7) work. 😞

thanks for the example, I’ll stop digging 😉

But, interestingly, neither of those work in Clojure. Perhaps there is a JIRA in the Clojure stack somewhere for that.

I think I found it, when you call vec on an eduction, the reducing function will be some kind of object

https://github.com/clojure/clojure/blob/4ef4b1ed7a2e8bb0aaaacfb0942729252c2c3091/src/jvm/clojure/lang/PersistentVector.java#L66

https://github.com/clojure/clojure/blob/4ef4b1ed7a2e8bb0aaaacfb0942729252c2c3091/src/jvm/clojure/lang/PersistentVector.java#L51

so deftype methods are also checked by the s/fdef. nice. I guess…

so example: (vec (eduction (map inc) [1 2 3]))

but this won’t be caught by our spec because of direct linking

> borkdude [9:34 AM] > Is it possible to have clojure.spec not throw exceptions on fdef violations, but only print “fdef error, line so and so” and then continue as always? > alexmiller [3:08 PM] > no

Hmm. That could be a nice feature. That’s what we ended up doing with :checked-arrays with :warn vs. :error

FWIW, the “not-found” arity for map lookup works in Clojure: ({0 2} 1 :nf)

yeah, we could run a fork of spec in the tests if that detects failures faster on the coalmine thing

do we stall work on JIRA patches for clojurescript, or maybe wrap in a #?(:clj ...) for now?

I made a PR with a conditional for the tests, until the JIRA ticket is fixed

Merged. I’ll do the same with my thing for merge/merge-with

detected a violation of the merge-with spec in secretary (client side routing):

(route-matches [_ route]
       (when-let [[_ & ms] (re-matches* re route)]
         (->> (interleave params (map decode ms))
              (partition 2)
              (merge-with vector {})))))

Expound output:

Function arguments↵
↵
  (... ... ())↵
           ^^↵
↵
should satisfy↵
↵
  map?↵
↵
or↵
↵
  nil?↵
↵

I’m also getting an error on merge:

core.cljs:2045 Uncaught Error: Invalid arity: 24
    at Function.G__10206 [as call] (core.cljs:2045)
    at Function.re_com.box.box_base.cljs$core$IFn$_invoke$arity$variadic (box.cljs:119)

core.cljs:2045. I’m not sure what this has to do with spec

maybe it’s the call to apply

ok, when I (stest/unstrument [merge-with merge]) my client side app also works

maybe we should also put conditionals around those specs proper, until they work properly in cljs…

hmm, client side gets reeeeallly slow.

It seems like using associative? is a reasonable relaxation

But does this case involve an empty list?

hmm, I was wrong. vector is the function here.

I take back what I said 😉

what secretary is doing is probably something like (merge-with vector {} {:a 1} {:a 2})

I got an error on merge-with, but now I’m not sure what triggered it. sorry for the noise

I think it works like this: (merge-with vector {} '([:a 1] [:b 2])). works in cljs, but not in clj.

so it’s not even an associative? but it works 😉

(merge-with vector {} '([:a 1] [:b 2] [:a 2])) ;;=> {:a [1 2], :b 2}

https://github.com/gf3/secretary/blob/1f2036a694e49f58a97c9401878602148f9d6310/src/secretary/core.cljs#L252

Right, @borkdude this might help understand:

(merge {} (seq {:a 1, :b 2}))

Actually, this is closer to the facet that is important:

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

yeah get it. probably we should allow a seq of MapEntry-s and on cljs a seq of 2-vectors

Even ClojureScript only allows non-`MapEntry`’s in order to avoid breaking code that got used to MapEntry being represened as 2-vectors

I vaguely recall that (conj {} (seq {:a 1, :b 2})) only works due to an internal implementation detail (for some other internal use case). I don’t think it is really meant to be used by application code.

At the end of the day, though, it gets hard to spec code that accidentally relies on implementation details. For example (+ :what-the-fuck)

MapEntry was only introduced later in ClojureScript I believe?

https://dev.clojure.org/jira/browse/CLJS-2013

Right, so for a long time, 2-vectors were used as a substitute. But, that didn’t stop code from then using them as vectors.

And, there was not much that could be done about the consequence that (empty (first {:a 1})) used to return an empty vector

so now users of our lib can’t use secretary. what to do? change the spec or PR secretary? something’s gotta give

I guess partition 2 could have been written like (apply hash-map) here: https://github.com/gf3/secretary/blob/1f2036a694e49f58a97c9401878602148f9d6310/src/secretary/core.cljs#L251

and how do you explain such a PR? eh yeah, we wrote a spec your code is offending. this seems the other way around 😉

Right, IMHO, this will have to almost be a philosophical stance that would need to be taken by the library. Should it spec the intent, or should it bend over backwards to characterize the current implementation, or is there some reasonable balance.

For example, my opinion is that using associative? instead of map? in the merge spec might be viewed as a reasonable relaxation.

First thought is that we should spec the intent.

It’s a bit like with Alex debacle when spec’ing require.

Exactly

But, I also hold the view that if you don’t try to spec the intent, then you are really saying that the spec is the entire transitive closure of the implementation of any given core fn. 😞

I also think a pr against secretary would in place. Especially it their use is somewhat strange.

Maybe speculative can afford to spec the intent, while core can’t. But that still leaves the problem that certain users would be unable to use speculative because of any one offending function anywhere in their code.

And there is this thing called Hyrums law, which I’m struggling a bit with ATM with Eastwood.

Right. At the same time users may be convinced that their idioms should follow the intent of the functions.

Oh, cool, I’m glad there is a name for the concept 🙂

Learnt it from @andy.fingerhut :)

luckily users can always unstrument functions, but it would be even nicer if you could say: I only want to see errors triggered by me, not by libraries

An interesting concept is on the flip side: If you have clients exhibiting Hyrum’s law, then it makes it really difficult to revise the implementations of core functions. ClojureScript’s aget is probably the most famous example.

@borkdude that would be nice (if possible)

@slipset that would also solve the performance problem I’m now seeing on client and server

@mfikes especially if you value backwards compatibility.

You certainly can’t have it half way in ClojureScript: Instumentation is implemented by replacing the function being instrumented with a new version.

that’s why I opted for a warning instead of a “stop the system” error

“yeah, I know secretary has this weird thing, let’s move on”

I bet core will end up in a position that it is impossible to spec core because of Hyrum’s law.

So we’re saving them tons of work by figuring that out for them :)

I guess you could say in the exception: on which ns/file was this triggered, if this in a whitelist, it’s a real exception, else a warning

that would be awesome for speculative probably

and that’s something that would be possible in cljs I think

Perhaps there is a chance that, if speculative (or something like it) became viewed as useful, then library owners would feel compelled to fix their code.

It seems to involve a Catch 22 though 🙂

why isn’t spec a library in cljs?

Because when spec was ported to ClojureScript, it was not yet a library.

Perhaps I have that wrong…

ok… if it was a library it would be easier to patch 🙂

I would still argue that we’d be doing both core and the community a favor by opening prs against projects which kind’a violates the intents of the functions.

Although by that, not saying that having specs warn instead of error is a bad idea.

Well, yeah, even from the human standpoint of trying to understand code, you pretty much expect merge to return a map

(merge ()) works, but, WTF.

because it’s an empty seq of map-entries/2-tuples 😉

(merge 1) also works

lol

ok, let’s do the PR route then

You can merge anything if it is a single argument 🙂

we’re not going to relax specs unless there is a compelling reason, not to support edge cases in the wild

seems like a plan?

because we’re hoping people will make PRs to make the code more readable

I do feel a bit odd asking a library to change working code. Perhaps I can get over that feeling 🙂

we can try 😉

hi, speculative police here.

But, certainly, speculative can easily take the approach of leaning towards specing intent

I was thinking that, in Replete (which I view as a learning tool, really), it would be interesting for it to ship with speculative, perhaps even enabled, so that newbies get better feedback when they call (merge 1)

yeah, makes sense

Ahh here is the real problem. If speculative catches an error somewhere in core itself, or in core.async, I bet there is no chance of getting that code revised

Having said that, this patch was accepted https://github.com/clojure/core.async/commit/cbf8778bd7b24a3102056d21eab1f1cc2860dd95

But, arguably, that was in support of the :checked-arrays feature actually in ClojureScript proper

It would be somewhat tongue in cheek to have certain specs relaxed in speculative b/c quirks in core with comments explaining exactly why the relaxation was in place.

I’m starting to see why secretary did this.

(->> '(:* cat :* bat) (apply hash-map)) ;;=> {:* bat}

That code seems fine

(->> '(:* cat :* bat)  (partition 2)) ;;=> ((:* cat) (:* bat))

no, it loses the cat value

so I think this usage is actually legit

Losing the cat value is what is supposed to happen, though

(Per the docstring for hash-map)

no it’s not (in secretary):

FAIL in (route-matches-test) (:)
splats
expected: (= (s/route-matches "*.*" "cat.bat") {:* ["cat" "bat"]})
  actual: (not (= {:* "bat"} {:* ["cat" "bat"]}))

there may be another way, but I see how he ended up here

Yeah, perhaps in seeking a solution, he discovered something that works (accidentally), and went with it

Maybe the lib should’a been called well-actually?

That Hyrum’s Law explanation is pretty good. One gem is “the interface has evaporated: the implementation has become the interface”

Given that, yeah, using aget to access JavaScript object properties makes complete sense

That clarifies my thinking: speculative really has to document some level of intent, otherwise speculative ends up being core itself

(Meaning speculative would end up specifying the core implementation, and thus might as well be core—it’s the problem where, if you want to know if a function is being called correctly, one way to find out is to call it and see how it behaves for your inputs)

to make it fit our spec, secretary could do:

(route-matches [_ route]
        (when-let [[_ & ms] (re-matches* re route)]
          (->> (interleave params (map decode ms))
               (partition 2)
               (reduce
                (fn [acc [k v]]
                  (update acc k (fn [o]
                                  (cond (not o) v
                                        (not (vector? o)) [o v]
                                        :else (conj o v)))))
                {}))))

(and to make the tests of secretary pass)

If you take the stance that speculative specs the intent, Hyrum’s Law implies it can never really be used broadly. Unless spec had some way of letting you only apply it to the code you are writing in your project.

Sorry, I’m just coming around to the point that Michiel made

the clojurescript merge-with doesn’t speak about map-entries at all. it just takes first and second from a seq

I think speculative should have a clear rationale

first I thought the rationale was: catching errors at dev time

later: we could use this to test core

now: creating PRs for libraries to make them more compliant

so yeah, let’s settle on something so people know why they might want to use this

Ahh, that’s a great example of how it make it difficult to optimize ClojureScript. In merge-with, that should actually be key and val not, first and second

My first perspective was better error-messages at dev-time.

Secondary, it turns out to be some sort of lint tool for libs.

Yeah, that’s what I really want. I want it to catch silly arguments I might happen to be passing. Not because I don’t understand core, but because I simply made a mistake.

It says, Hey, don’t do that.

right. so if we settle on that rationale, we should only or primarily be focussing on the :args part of the specs, that’s something we could agree on

And if the perspective is better error-msgs, then it would be mostly aimed at beginners trying out Clojure, so the compatibility with every lib out there is not such a big point?

also we could agree that using any? only specs are not a part of speculative, as they only slow things down and catch nothing

Agreed.

@slipset don’t agree there. I want to use it myself during dev.

Disagreement is fine.

It’s a vehicle for a better solution.

you may convince me not to use it of course 😉

I would suggest though, that a :ret is a nice thing if it doesn’t cause problems.

:ret only works if you’re generatively testing

Since it could be used for testing core.

And there is a gist out there somewhere which turns on :ret checking outside of gem-test.

yes, some libs do it

but in normal spec usage, it doesn’t do anything

:ret does at least show up in doc output

Anyways I need to get of the phone and work out a bit.

have a nice workout

The main unsolved problem I see is that, if I agree with the speculative rationale, and I want to use it, but too many of my favorite libs trigger errors, preventing me from using it. Perhaps in that case, I as a user of speculative would be compelled to work with my favorite libs to get them fixed.

perhaps, but the initial experience is important :thinking_face:

for a closed environment like mariacloud or replete it’s cool

Yes. Especially if speculative takes on a didactic flavor, it should definitely end up in Replete.

(And enabled by default.)

Cool. I’m now satisfied that we can do something that core really can’t do, but still benefit the community.

ok, that could be a rationale… focusing on intent and learning of core spec functions?

Yeah, probably not a great analogy but like training wheels Or perhaps a better analogy, a safety net

I was hoping to use this as a more general dev tool though

Me too

I would have it enabled in my day-to-day work.

Performance isn’t a problem in learning tools. So we can adapt the rationale to not deal with those problems 😜

Another problem unsolved as of yet: If core itself triggers an error. Hopefully it wont.

it doesn’t because of direct linking. I’m not sure about cljs

does cljs support direct linking?

oh the function replacement right?

No… in the sense that, when you call a function you are always looking it up via something like cljs.core.inc

I guess :advanced could get wonky… I haven’t thought about that very much

It will be interesting if the patch in https://dev.clojure.org/jira/browse/CLJS-2943 can break any code out there that is passing vectors in.

you can test today by trying the secretary example

it could also be part of the rationale of speculative to bring clojure and clojurescript more together… and to bring code written in libraries more to the level of multi-platform

so our specs only support code that runs in both (as much as possible)

(thinking out loud)

@mfikes have you tested the secretary thing? if you can paste your merge-with here I can give it a try 🙂

I think it will fail anyway: (key [:a 1]) ;;=> No protocol method IMapEntry.-key defined for type cljs.core/PersistentVector: [:a 1]

Here

(defn merge-with
  "Returns a map that consists of the rest of the maps conj-ed onto
  the first.  If a key occurs in more than one map, the mapping(s)
  from the latter (left-to-right) will be combined with the mapping in
  the result by calling (f val-in-result val-in-latter)."
  [f & maps]
  (when (some identity maps)
    (let [merge-entry (fn [m e]
                        (let [k (key e) v (val e)]
                          (if (contains? m k)
                            (assoc m k (f (get m k) v))
                            (assoc m k v))))
          merge2 (fn [m1 m2]
                   (reduce merge-entry (or m1 {}) (seq m2)))]
      (reduce merge2 maps))))

Yeah, that is what I thought might happen. Fucking Hyrum.

I guess you can run this change on your coal-mine

Yeah, I have that queued. Travis is backlogged right now.

In cases like these, I will often submit a ticket to the affected project. It is this bit of code here, right? https://github.com/gf3/secretary/blob/1f2036a694e49f58a97c9401878602148f9d6310/src/secretary/core.cljs#L252

true

Maybe we should rename this project to hyrum 😅

Hah!

I just learned something completely new about Clojure from Alex:

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

<Mind blown>

Only in certain APIs, like keys, or conj ....

The fact that you two got involved in this project is such an unexpected, positive thing!

Also, the discussions, tickets and knowledge sharing that happens because of this is so cool!

does that have to do with keys destructuring on seqs? (let [{:keys [:a :b]} '(:a 1 :b 2)] [a b])

I don't know. I'm still recovering from brain explosion.

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

@mfikes I think we may want to use a more relaxed spec for map? in functions like merge then. The only thing that secretary then has to do is to convert those 2-colls to proper map-entries.

There’s a discussion about this in #cljs-dev right now.

Yeah, go lurk if not already in that channel 🙂

It’s quite interesting. I feel like a kid listening in on the grownups having serious discussions :)

Me too :)

heissenspec

(I’m watching Breaking Bad)

@slipset yeah, this project is fun.

@mfikes

cljs.user=> (MapEntry. 1 2)
            ^
WARNING: Wrong number of args (2) passed to MapEntry at line 1
[1 2]

what’s this?

(merge-with vector {} (sequence (comp (partition-all 2) (map (fn [[k v]] (MapEntry. k v)))) [:a 1 :a 2 :b 3]))

just shoving (map (fn [[k v]] (MapEntry. k v))) into the ->> also works of course

@borkdude MapEntry has a hash value as well

You can unit it with nil

why’s the contructor different from Clojure?

Does Clojure define it or Java (I’m afk and can’t recall). In ClojureScript it is a deftype

Java

ClojureScript has no choice, given it is a deftype

it does seem a little bit inconsistent and inconvenient that Clojure allows you to use 2-vecs instead of MapEntry in some places and in others it doesn’t. Also given that MapEntries are probably not intended to be created by end users

Yeah. ClojureScript has just been trying to follow Clojure

makes sense

Just tried to write a spec for assoc 😕

Function signature:

(assoc map key val)(assoc map key val & kvs)

From the docstring

When applied to a vector, returns a new vector that
  contains val at index. Note - index must be <= (count vector).

map = associative?

And there we have a diff between clojurescript and clojure

23:24 $ clj
Clojure 1.10.0-RC1
user=> (assoc nil 'lol)
Evaluation error (ArityException) at clojure.lang.AFn.throwArity (AFn.java:429).
Wrong number of args (2) passed to: clojure.core/assoc--5316
user=> (assoc nil 'lol 'lol)
{lol lol}
user=> (assoc nil 'lol 'lol 'lol)
Evaluation error (IllegalArgumentException) at clojure.core/assoc--5316 (core.clj:195).
assoc expects even number of arguments after map/vector, found odd number
user=>

whereas Planck says

23:25 $ plk
ClojureScript 1.10.339
cljs.user=> (assoc nil 'lol)
            ^
WARNING: Wrong number of args (2) passed to cljs.core/assoc at line 1
{lol nil, nil nil}
cljs.user=> (assoc nil 'lol 'lol)
{lol lol}
cljs.user=> (assoc nil 'lol 'lol 'lol)
{lol nil}
cljs.user=>

And with that, I’ll hit the sack!

I would say this is a bug in cljs: (assoc nil 'lol 'lol 'lol)

goodnight @slipset

FWIW, here is the actual genesis of the intent vs. implementation and specs thing: https://youtu.be/6ftW8UwwP_4?t=2359

(At least recently. :)