Fork me on GitHub
#malli
<
2022-09-29
>
ingesol19:09:38

I tried to instrument a CLJS multimethod (the dispatch function), but it does not seem to work. I can call it with wrong input type, and it still works:

(defn my-dispatch
  {:malli/schema [:=> [:cat :string] :keyword]}
  [s]
  (keyword s)

(defmulti a-multi-method
  my-dispatch)

(defmethod a-multi-method :default [_]
  :the-default)

;; The below does not crash like it should
(a-multi-method 4)

ambrosebs20:09:51

Just a hunch, but have you tried:

(defmulti a-multi-method
  #(my-dispatch %))

ingesol06:09:10

Thanks, that’s an interesting idea. The assumption is that the original function will be cached inside the multimethod storage? Anyway, it did unfortunately not work.

ingesol09:09:33

Also, to clarify, calling the my-dispatch function normally from somewhere else with wrong args triggers validation

dvingo13:09:40

It looks like this is due to how multimethods are implemented. https://github.com/clojure/clojurescript/blob/961807166c8cf4d45a225d63416f06464fb27eaf/src/main/cljs/cljs/core.cljs#L11343 the dispatch-fn is passed into a JS object (deftype) and that captures the original function value. Just tried this in JS:

var afn = function afn(){ console.log('FIRST')}
y = {x: afn}
afn = function (){ console.log('SECOND')}
y.x()
// => FIRST
so I think this is what the problem is

👍 1
dvingo13:09:39

we may be able to mutate the MultiFn's dispatch-fn property when we instrument, but then the defmulti would have to have the instrumentation applied, not the function

ambrosebs14:09:00

> Thanks, that’s an interesting idea. The assumption is that the original function will be cached inside the multimethod storage? Anyway, it did unfortunately not work. Yes, I was also going to suggest #'my-dispatch but I forgot how vars work in CLJS.

ingesol16:09:33

Taking one step back, the most valuable thing for our codebase would be to be able to declare the contract for the multimethod in general. Instrumenting the dispatch function is more like a hack to get at least some of the value. I know that instrumenting defmulti is probably a large and maybe not attractive subject, just mentioning it.

ambrosebs16:09:57

AFAIK it is not possible in CLJ.

ambrosebs16:09:29

probably easier in cljs

ingesol17:09:12

To illustrate what I mean

(defmulti say-hi
  {:malli/schema [:=> [:cat [:map [:nationality :keyword]]] :string]}
  (fn [person]
    (:nationality person)))

(defmethod say-hi :german
  "Guten tag")

ingesol17:09:26

I think I remember some earlier discussion indicating that the above is simplistic and probably not a good idea, but did not find it. And @U055XFK8V, just to clarify, this is the thing you are saying is not possible in CLJ?

ambrosebs17:09:33

Yes, I've looked at clojure.lang.MultiFn and there's no obvious way to achieve this.

ambrosebs17:09:52

actually, MultiFn isn't a final class so perhaps a wrapper can be created. I might look into that.

ingesol17:09:48

But that would require an alternative defmulti macro, right?

ambrosebs17:09:05

I'm thinking we leave everything as-is, but an instrument! function when called on a multimethod would set the multimethod var to a thin wrapper around MultiFn.

ambrosebs17:09:46

The tricky part is making sure that wrapper works with existing defmethod calls. And if we subclass MultiFn, it might work.

ingesol17:09:48

This is all about the mechanics of doing the instrumentation, I also seem to remember there are issues with the semantics. Will try to look it up

ingesol18:09:21

Maybe forget that last thing. So at least the way we use multimethods, it makes sense to enforce a shared input/output contract for all defmethods. The only thing that is not clear is how/where on the defmulti to define the schema.

ingesol18:09:42

Pinging @U055NJ5CC in case he has opinions on this

ambrosebs18:09:19

Thinking something like

public MyMultiFn extends MultiFn {
  public MyMultiFn(IFn wrapper, MultiFn inner) { }
  public Object invoke(args ...) {
    wrapper.invoke(inner, args...);
  }
....
}
(defn instrument-multi! [var checking-wrapper]
  (alter-var-root var (fn [multi] (MyMultiFn. checking-wrapper multi)))

ambrosebs00:10:57

I managed to prototype defmulti instrumentation successfully for Prismatic Schema, so I take back my assertion that this was impossible.

🙂 1
ikitommi07:10:40

Would be great if multimethods could be schematized. No opinions on the impl.

ambrosebs18:10:21

Ok I'll prep a proof of concept

🍻 1
Bart Kleijngeld14:02:57

Any progress on this? Or an issue that I can monitor. Would love support for instrumenting multimethods. Thanks!

ambrosebs19:02:24

I managed to do this for schema with some success, but it wasn't accepted. I lost steam on the malli version https://github.com/plumatic/schema/pull/451

Bart Kleijngeld19:02:59

Thanks for sharing