Turn on instrumentation for mx/defn with :malli/always meta

[opqdonut]

Fixes #823

[opqdonut]

Need to think about how ^:malli/always-check interacts with malli.dev/start!. Currently malli.dev/start! reinstruments the function and disables the throwing!

[opqdonut]

With my current approach, the malli.dev instrumentation goes "on top" of the always-check instrumentation. So after running malli.dev/start!, you get both the nice printed error and the exception:

user=> (require '[malli.experimental :as mx])
nil
user=> (mx/defn ^:malli/always-check f :- :int [x :- :int] x)
#'user/f
user=> (f 1)
1
user=> (f "1")
Execution error (ExceptionInfo) at malli.core/-exception (core.cljc:138).
:malli.core/invalid-input
user=> (require 'malli.dev)
nil
user=> (malli.dev/start!)
..instrumented #'user/f
started instrumentation
nil
user=> (f 1)
1
user=> (f "1")
-- Schema Error ---------------------------------------------------------------- NO_SOURCE_FILE:1 --

Invalid function arguments:

  ["1"]

Function Var:

  user/f

Input Schema:

  [:cat :int]

Errors:

  {:in [0], :message "should be an integer", :path [0], :schema :int, :value "1"}

More information:

  https://cljdoc.org/d/metosin/malli/CURRENT/doc/function-schemas

----------------------------------------------------------------------------------------------------
Execution error (ExceptionInfo) at malli.core/-exception (core.cljc:138).
:malli.core/invalid-input

[dvingo]

There is some prior work in this PR that is designed with cljs support in mind: https://github.com/metosin/malli/pull/702

I think emitting a defn that delegates to the instrumented version is much simpler to deal with due to how clojurescript compilation works (multiple js functions can be emitted per one defn form). I also don't really know of another way because alter-var-root isn't available in cljs.

That PR also has some cljs tests you can copy over to ensure the var meta is still preserved and multi-arity forms work as expected.

It would be awesome to have support for this syntax in cljs too :)

[opqdonut]

Huh, thanks! And sorry for not noticing your PR. That looks like a nice implementation to me. The only downside is that there's a duplicated defn impl. I'll update this PR with a similar implementation and some cljs tests.

[opqdonut]

I ran into some problems trying to run all of malli.experimental-test in cljs mode, so I had to disable some tests. I'll try to get them enabled next.

[dvingo]

No worries! That PR isn't directly related and is now quite old, but thanks for updating this PR!

I think the cljs test failures may be from attempting to get meta from a var at runtime - there are not real Vars in cljs. I haven't run the code, but looking at what is there, I think this line: (is (= v (k (meta var))) Is problematic in cljs, you can only use symbol literals - no indirection allowed because this call has to evaluate at compile time in a macro.

So to achieve this you may be able to put :var-meta (meta #'f1) etc in the expectations data and then use that in the assertion. I think that is the issue, but I can try it out later tonight to check.

[opqdonut]

Right, that's a good catch. I wonder if the instrumentation failures are because expectations refers to the vars, so they get stuck with the first definitions of the functions.

[dvingo]

@opqdonut I played around with some cljs fixes and have the tests passing. https://github.com/dvingo/malli/commit/2c8d99a906820fd81b466f4b6bfc4b44b0eac479

Some of the issues were that JS is just more lenient so the :call checks don't throw on a lot of the bad input when instrumentation is off. The other piece was that in cljs var metadata is not evaluated for almost all circumstances, which is the quote issue you ran into.

Everything is looking good aside from f6 which is the multi-arity with varags function. The instrumented function arities don't seem to be present. There are already existing tests for this in malli.instrument-test.cljs so I think this is due to the complexities of how vars and multi-arity functions are compiled and would take a while to debug. I don't think users will be doing this sort of dynamic instrumentation with cljs so I'm fine with this for now.

And lastly, the tests won't work under advanced compilation that instrument dynamically so I skipped those with the ^:simple metadata.

[opqdonut]

Thanks! I think I also had pretty much everything passing except f6. I wonder if the best solution here would be to just create a new malli.experimental.cljc-test namespace with some simple cljc cases that pass, and leave the current defn-test out of it.

[opqdonut]

I went over this with @ikitommi and we agreed to just run my new tests for cljs. No use in making the existing defn-test cljs-compatible.