Fork me on GitHub
ambrosebs18:08:28 1.4.0 Adds for documenting and instrumenting protocol methods.

(s/defprotocol MyProtocol
    :extend-via-metadata true
    (^:always-validate method1 :- s/Int
      [this a :- s/Bool]
      [this a :- s/Any, b :- s/Str]
      "Method doc2")
    (^:never-validate method2 :- s/Int
      "Method doc2"))

🚀 15
👍 5
🙌 3

so cool that this library is still going and still providing its same good value!

👆 1
👍 1

Very cool! Curious, is it a zero-cost abstraction? That is, when checking is off, ideally this would be identical to vanilla protocol usage. With clojure.spec hacks normally there's a backing defn, which adds indirection and a cost. I've been meaning to try again for a while.


@U45T93RA6 mostly no, there is a cost. there are essentially 3 different modes: instrumentation on, instrumentation off, and leave as-is (see the docstring for the 3rd). In the first two, inlining is disabled, so direct calls will pay an extra indirection + cache lookup in those cases. In normal wrapped calls, there is an extra indirection that also passes the cache via set! to the "actual" protocol function.


perhaps it's a good idea to use the 3rd mode in prod, then it would be zero-cost (at least, that's what I intended).


you can move between mode 1 and 2, but mode 3 is permanent. so yeah, it's probably a good candidate for prod.


Perhaps the docstring could be a little clearer since one might not immediately know/remember what inlinling is, etc. Also if this is problematic for your environment is bit of a strange wording, because disabling checking in production is something I'd do out of principles, not because of 'problems' Anyway this does look like a great defprotocol. It looks like one could do this at macroexpansion-time?

(if *assert*
With other solutions I've seen (and written!) one can't, because the method names for declarations will be different vs. the method names for usages.

👍 1

Agreed, actually the wording assumes you'd only want this if Clojure's implementation details change and breaks instrumentation. I didn't really consider you could use it for perf reasons until this conversation.


> With other solutions I've seen (and written!) one can't, because the method names for declarations will be different vs. the method names for usages. Yes, I studied your impl in speced.def while designing this, and s/defprotocol does not rename methods. the same name is used to extend and invoke.


Not sure if there was anything worth studying :) Is the gist of your solution the following? It works, would love to port it to speced.def. Please LMK if there's some missing piece

(clojure.core/defprotocol P
  (do-it [this]))

(def sync! (fn [^clojure.lang.AFunction outer-mth
                ^clojure.lang.AFunction inner-mth]
             (when-not (identical? (.__methodImplCache outer-mth)
                                   (.__methodImplCache inner-mth))
               ;; lock to prevent outdated outer caches from overwriting newer inner caches
               (locking inner-mth
                 (set! (.__methodImplCache inner-mth)
                       ;; vv WARNING: must be calculated within protected area
                       (.__methodImplCache outer-mth)
                       ;; ^^ WARNING: must be calculated within protected area

(let [inner-method do-it
      instrument! (fn [f] ;; the value of @#'do-it
                    (fn [x] ;; a fn with the same sig as do-it
                      (assert (pos? x)) ;; an example assertion, as it would be provided by users
                      ;; invoke whatever `do-it` does:
                      (f x)))
      outer-method (instrument! inner-method)]
  (alter-var-root #'P assoc-in [:method-builders #'do-it] (fn [cache]
                                                            (set! (.__methodImplCache outer-method) cache)
                                                            (sync! outer-method inner-method)
  (alter-meta! #'do-it assoc :inline (fn [& args]
                                       `((do ~(symbol #'do-it))
  (alter-var-root #'do-it (constantly outer-method)))

(extend-protocol P
  (do-it [this]
(do-it 1)
(do-it -1) ;; the assertion is triggered

(extend-protocol P
  (do-it [this]
(do-it 1.0)
(do-it -1.0) ;; the assertion is triggered


(also, does that do in the alter-meta! add something?)


@U45T93RA6 yep, looks like it covers all the bases for Clojure JVM. the do is the dirty hack to stop the compiler inlining the method call: ((do do-it) ...) doesn't inline, but (do-it ...) can. these tests should fail without the do

👀 1

I wanted to make a shared library with these ideas but I couldn't decide which functions to expose. Once we have a few impls, maybe it'll be more obvious.


there was also interest in adding this to malli.


Thanks much :) will see if I can create a tiny ns with the core ideas

👏 1

Besides from sync! , I think I'd only need this defn:

(defn instrument! [protocol-var
                   fns #_{#'foo ;; a var from the protocol, to be instrumented
                          (fn [f] ;; always receives a single function `f`, representing the original value of @#'foo
                            (fn [this x y z] ;; consumers must build this argv the right arities, for each method (and possibly for each arity of the method)
                              (f this x y z)))} ;; consumers must build the right calls to `f`
  (doseq [[var-ref wrapper] fns
          :let [inner-method @var-ref
                outer-method (wrapper inner-method)]]
    (alter-var-root protocol-var assoc-in [:method-builders var-ref] (fn [cache]
                                                                       (set! (.__methodImplCache outer-method) cache)
                                                                       (sync! outer-method inner-method)
    (alter-meta! var-ref assoc :inline (fn [& args]
                                         `((do ;; do: for preempting inlining
                                             ~(symbol var-ref)) ;; n.b. `symbol` shortcut not available in older clojures
    (alter-var-root var-ref (constantly outer-method))))
individual libraries (e.g. speced.def, malli-whatever) would be responsible for passing the right fns map. Each library likely does its own parsing anyway so it's not much of a requirement This API would make sense for me for speced.def. I just tried it now with some speced/fn s and it works - would fit for a defprotocol reimpl. Would be thankful if you release this as an isolated library. I actually have to be pedantic about copyright :)


(Also since I use malli at work, I'd give two separate usages to it!)


Thanks @U45T93RA6! I'll spin up a lib.

🙂 1

Hey there :) was there luck with the lib?