Multi-arity function instrumentation fails with :static-fns true

Description

The following code, in a cljc file, will pass all tests in Clojure, as well as ClojureScript with :optimizations :none, but will fail the second assertion when :optimizations :advanced is enabled.

Environment

GNU/Linux

Activity

Show:
Thomas Heller
November 13, 2017, 10:31 AM

I would suspect that is more likely related to :static-fns. Since the arity is known the compiler will directly dispatch to the appropriate fn instead of going though the dispatch fn which `fdef` might not cover?

Can you try with :none and :static-fns true?

Mike Fikes
November 13, 2017, 11:55 AM

Minimal complete repro:

Put the following in src/hello_world/core.cljs:

And this in node.clj:

With a copy of the shipping cljs.jar copied next to node.clj, compile with

and run with

By revising node.clj and compling/running, you can see that it is :static-fns rather than :advanced. In particular, you can set :static-fns to false to override its setting under :advanced and instrumentation of this multi-arity function will work under :advanced.

As Thomas points out, this makes complete sense given the way that instrument works: It wraps the top-level function pointed to by the Var, not the individual direct-arity dispatch variants. (Take a look at how instrument-1 and instrument-1* work to set! the value of the Var to be a wrapped version of the original.)

It seems like this could either be fixed by detecting this situation and instead wrapping each of the dispatch variants, or by perhaps simply detecting when instrument is called while :static-fns is enabled and emitting a diagnostic (essentially indicating that this mode is unsupported).

Mike Fikes
November 14, 2017, 4:10 PM

The attached CLJS-2397-1.patch is attached for discussion. It takes the approach of not fixing the issue by actually instrumenting each direct dispatch fn, but instead emitting a diagnostic and filtering for the problematic cases. It is odd in that it abuses things by triggering an analyzer warning, and it likewise doesn't attempt to change the return value of cljs.spec.test.alpha/instrumentable-vars, but it might be an approach with merit (especially, since instrumentation is often done during dev only).

Here is an illustration of how the patch behaves with :static-fns true:

Note that, in the above, the return value for st/instruement only reflects that f has been instrumented.

Here is the same without :static-fns true:

Mike Fikes
December 14, 2017, 8:51 PM

Attaching re-baselined CLJS-2397-2.patch

David Nolen
December 22, 2017, 8:10 PM
Completed

Assignee

Mike Fikes

Reporter

import

Labels

Approval

None

Patch

Code

Priority

Minor