Instrumentation of fns with primitive type hints fails

Question

`
(require '[clojure.spec :as s] '[clojure.spec.test :as st])
(defn foo [^double val] val)
(s/fdef foo :args (s/cat :val double?))
(st/instrument `foo)
(foo 5.2)

user=> (foo 5.2)
ClassCastException clojure.spec.test$spec_checking_fn$fn__13069 cannot be cast to clojure.lang.IFn$DO

   	user/eval6 (NO_SOURCE_FILE:5)
   	user/eval6 (NO_SOURCE_FILE:5)
   	clojure.lang.Compiler.eval (Compiler.java:6951)
   	clojure.lang.Compiler.eval (Compiler.java:6914)
   	clojure.core/eval (core.clj:3187)
   	clojure.core/eval (core.clj:3183)
   	clojure.main/repl/read-eval-print--9704/fn--9707 (main.clj:241)
   	clojure.main/repl/read-eval-print--9704 (main.clj:241)
   	clojure.main/repl/fn--9713 (main.clj:259)
   	clojure.main/repl (main.clj:259)
   	clojure.main/repl-opt (main.clj:323)
   	clojure.main/main (main.clj:422)

`

Cause: spec replaces var values with instrumented functions that will not work with primitive function interfaces

Approach: Take primitive interfaces into account and make them work, or document/fail that instrumentation will not work with these.

Answer 1

Comment made by: hiredman

spec replaces var values with instrumented functions, which works for the default linking case, var deref cast to ifn, invoke, but in the other cases (primitive functions, direct linking, others?) this won't work

Answer 2

Comment made by: kennyjwilli

Hmm. Well this should be at least be documented. So, spec cannot be used on functions with a type hinted arg?

Answer 3

Comment made by: seancorfield

Spec cannot be used on functions with primitive typed hinted arguments or returns -- non-primitive type hints seem to be fine.

But documentation isn't enough here: instrumenting a namespace and then discovering it broke a function (that happened to have a primitive type hint) isn't acceptable. If the instrumentation isn't going to work, the function should be skipped (and a warning produced, hopefully).

Answer 4

Comment made by: hiredman

yeah, I was giving the root cause of the issue, not excusing the issue.

Understanding the root cause predicts other places where there will be issues: where ever some non-default function linking strategy is used.

One such place is direct linked functions, but I suspect for direct linked functions, Clojure/Core will just say you should only instrument code for testing, and you should only turn on direct liking for production.

Another case, which I am sort of surprised we haven't heard more about yet is protocol functions.

Answer 5

Comment made by: seancorfield

Your comment about direct linking made me wonder about the validity of spec'ing and instrumenting {{clojure.core}} functions. The examples show {{clojure.core/symbol}}, but Clojure's core library is shipped as direct linked, as of 1.8.0 isn't it?

Answer 6

Comment made by: hiredman

what alters the calling convention isn't the function being compiled with direct linking on, but a caller of that function being compiled with direct linking on.

This code will throw a non-conforming error for the bogus symbol spec with direct linking off, and return the symbol foo with direct linking on

`
(require '[clojure.spec :as s])

(s/fdef symbol
:args string?
:ret symbol?)

(defn foo
[]
(symbol 'foo))

(s/instrument-all)

(foo)
`

Answer 7

Comment made by: hiredman

This code returns true because m is a protocol function, if you replace it with a regular function it throws a non-conforming error

`
(require '[clojure.spec :as s])

(defprotocol P
(m [_]))

(deftype T []
P
(m [_] true))

(s/fdef m
:args (s/cat :p (constantly false))
:ret string?)

(defn foo
[]
(m (T.)))

(s/instrument-all)

(foo)
`

Answer 8

Comment made by: alexmiller

@Sean instrumenting core functions will work for calls from your code into core (which are presumably not direct-linked), but will not affect calls from one core function to another as they are direct-linked and do not go through the var. One thing we've considered for a long while is building a dev version of core that would not be direct-linked and could potentially turn on instrumentation or other helpful dev-time tooling.

Answer 9

Comment made by: seancorfield

Thanks for that answer @alexmiller -- We have dev set to non-direct-linking but QA / production set to direct linking, so I'm only concerned about possible issues in dev with {{(s/instrumental-all)}} and wanting to be sure "code won't break". If instrumentation won't affect existing (direct-linked) calls within core, that's good enough for me. I am concerned about primitive hinting and protocols (and whatever crawls out of the woodwork here) since you don't want to be forced to read the source of every library you include, just to see whether {{(s/instrument-all)}} is safe or whether it will bite you in some weird way while you're developing.

Answer 10

Comment made by: borkdude

Running into this issue while spec'ing clojure.core/partition-all.

Comments

As this bug seems to be here to stay, maybe it's something Kondo should be made aware of?

Answer 11

Reference: https://clojure.atlassian.net/browse/CLJ-1941 (reported by alex+import)