ClojureScript

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

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Environment:
    GNU/Linux
  • Patch:
    Code

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.

(ns cljs-arity-spec-issue.core-test
  (:require #?@(:clj [[clojure.test :refer :all]
                      [clojure.spec.alpha :as s]
                      [clojure.spec.test.alpha :as st]]

              :cljs [[cljs.test
                      :refer-macros [deftest testing is use-fixtures]]
                     [cljs.spec.alpha :as s]
                     [cljs.spec.test.alpha :as st]])))

(defn arities
  ([a]
   (inc a))
  ([a b]
   (+ a b))
  ([a b c]
   0))
(s/fdef arities
        :args (s/or :arity-1 (s/cat :a number?)
                    :arity-2 (s/cat :a number? :b number?)
                    :arity-3 (s/cat :a string? :b boolean? :c map?))
        :ret number?)

(st/instrument)

(deftest arities'
  (testing "Arity-1 Positive"
    (is (arities 1)))
  (testing "Arity-1 Negative"
    (is (thrown? #?(:clj RuntimeException :cljs :default)
                 (arities "bad"))))) ; This test fails with :advanced enabled

Activity

Hide
Jeaye Wilkerson added a comment -

It may be helpful to note that I've found this is likely not related to the spec, but instead the defn itself. My reasoning is that, if the defn is changed to single-arity, while the spec is left the same, the issue goes away.

(ns cljs-arity-spec-issue.core-test
  (:require #?@(:clj [[clojure.test :refer :all]
                      [clojure.spec.alpha :as s]
                      [clojure.spec.test.alpha :as st]]

              :cljs [[cljs.test
                      :refer-macros [deftest testing is use-fixtures]]
                     [cljs.spec.alpha :as s]
                     [cljs.spec.test.alpha :as st]])))

(defn arities [a]
  (inc a))
(s/fdef arities ; Same spec, but only a single arity function.
        :args (s/or :arity-1 (s/cat :a number?)
                    :arity-2 (s/cat :a number? :b number?)
                    :arity-3 (s/cat :a string? :b boolean? :c map?))
        :ret number?)

(st/instrument)

(deftest arities' ; All tests will pass now.
  (testing "Arity-1 Positive"
    (is (arities 1)))
  (testing "Arity-1 Negative"
    (is (thrown? #?(:clj RuntimeException :cljs :default)
                 (arities "bad")))))
Show
Jeaye Wilkerson added a comment - It may be helpful to note that I've found this is likely not related to the spec, but instead the defn itself. My reasoning is that, if the defn is changed to single-arity, while the spec is left the same, the issue goes away.
(ns cljs-arity-spec-issue.core-test
  (:require #?@(:clj [[clojure.test :refer :all]
                      [clojure.spec.alpha :as s]
                      [clojure.spec.test.alpha :as st]]

              :cljs [[cljs.test
                      :refer-macros [deftest testing is use-fixtures]]
                     [cljs.spec.alpha :as s]
                     [cljs.spec.test.alpha :as st]])))

(defn arities [a]
  (inc a))
(s/fdef arities ; Same spec, but only a single arity function.
        :args (s/or :arity-1 (s/cat :a number?)
                    :arity-2 (s/cat :a number? :b number?)
                    :arity-3 (s/cat :a string? :b boolean? :c map?))
        :ret number?)

(st/instrument)

(deftest arities' ; All tests will pass now.
  (testing "Arity-1 Positive"
    (is (arities 1)))
  (testing "Arity-1 Negative"
    (is (thrown? #?(:clj RuntimeException :cljs :default)
                 (arities "bad")))))
Hide
Thomas Heller added a comment -

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?

Show
Thomas Heller added a comment - 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?
Hide
Mike Fikes added a comment - - edited

Minimal complete repro:

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

(ns hello-world.core
  (:require [cljs.nodejs :as nodejs]
            [clojure.spec.alpha :as s]
            [clojure.spec.test.alpha :as st]))

(nodejs/enable-util-print!)

(defn arities
  ([a]
   (inc a))
  ([a b]
   (+ a b))
  ([a b c]
   0))

(s/fdef arities
        :args (s/or :arity-1 (s/cat :a number?)
                    :arity-2 (s/cat :a number? :b number?)
                    :arity-3 (s/cat :a string? :b boolean? :c map?))
        :ret number?)

(st/instrument)

(defn -main [& args]
  (try
    (prn (arities "bad"))
    (catch :default ex
      (prn ex))))

(set! *main-cli-fn* -main)

And this in node.clj:

(require 'cljs.build.api)

(cljs.build.api/build "src"
  {:main 'hello-world.core
   :output-to "main.js"
   :optimizations :none
   :static-fns true
   :target :nodejs})

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

java -cp cljs.jar:src clojure.main node.clj

and run with

node main.js

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).

Show
Mike Fikes added a comment - - edited Minimal complete repro: Put the following in src/hello_world/core.cljs:
(ns hello-world.core
  (:require [cljs.nodejs :as nodejs]
            [clojure.spec.alpha :as s]
            [clojure.spec.test.alpha :as st]))

(nodejs/enable-util-print!)

(defn arities
  ([a]
   (inc a))
  ([a b]
   (+ a b))
  ([a b c]
   0))

(s/fdef arities
        :args (s/or :arity-1 (s/cat :a number?)
                    :arity-2 (s/cat :a number? :b number?)
                    :arity-3 (s/cat :a string? :b boolean? :c map?))
        :ret number?)

(st/instrument)

(defn -main [& args]
  (try
    (prn (arities "bad"))
    (catch :default ex
      (prn ex))))

(set! *main-cli-fn* -main)
And this in node.clj:
(require 'cljs.build.api)

(cljs.build.api/build "src"
  {:main 'hello-world.core
   :output-to "main.js"
   :optimizations :none
   :static-fns true
   :target :nodejs})
With a copy of the shipping cljs.jar copied next to node.clj, compile with
java -cp cljs.jar:src clojure.main node.clj
and run with
node main.js
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).
Hide
Mike Fikes added a comment -

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:

ClojureScript Node.js REPL server listening on 50413
To quit, type: :cljs/quit
cljs.user=> (require '[clojure.spec.alpha :as s] '[clojure.spec.test.alpha :as st])
nil
cljs.user=> (defn f [x] 1)
#'cljs.user/f
cljs.user=> (s/fdef f :args any?)
cljs.user/f
cljs.user=> (defn g ([x] 1) ([x y] 2))
#'cljs.user/g
cljs.user=> (s/fdef g :args any?)
cljs.user/g
cljs.user=> (defn h ([x] 1) ([x & ys] 2))
#'cljs.user/h
cljs.user=> (s/fdef h :args any?)
cljs.user/h
cljs.user=> (st/instrument)
WARNING: cljs.user/h cannot be instrumented (it is variadic and :static-fns is enabled) in file <cljs repl>
WARNING: cljs.user/g cannot be instrumented (it is multi-arity and :static-fns is enabled) in file <cljs repl>
[cljs.user/f]

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:

ClojureScript Node.js REPL server listening on 51318
To quit, type: :cljs/quit
cljs.user=> (require '[clojure.spec.alpha :as s] '[clojure.spec.test.alpha :as st])
nil
cljs.user=> (defn f [x] 1)
#'cljs.user/f
cljs.user=> (s/fdef f :args any?)
cljs.user/f
cljs.user=> (defn g ([x] 1) ([x y] 2))
#'cljs.user/g
cljs.user=> (s/fdef g :args any?)
cljs.user/g
cljs.user=> (defn h ([x] 1) ([x & ys] 2))
#'cljs.user/h
cljs.user=> (s/fdef h :args any?)
cljs.user/h
cljs.user=> (st/instrument)
[cljs.user/h cljs.user/f cljs.user/g]
Show
Mike Fikes added a comment - 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:
ClojureScript Node.js REPL server listening on 50413
To quit, type: :cljs/quit
cljs.user=> (require '[clojure.spec.alpha :as s] '[clojure.spec.test.alpha :as st])
nil
cljs.user=> (defn f [x] 1)
#'cljs.user/f
cljs.user=> (s/fdef f :args any?)
cljs.user/f
cljs.user=> (defn g ([x] 1) ([x y] 2))
#'cljs.user/g
cljs.user=> (s/fdef g :args any?)
cljs.user/g
cljs.user=> (defn h ([x] 1) ([x & ys] 2))
#'cljs.user/h
cljs.user=> (s/fdef h :args any?)
cljs.user/h
cljs.user=> (st/instrument)
WARNING: cljs.user/h cannot be instrumented (it is variadic and :static-fns is enabled) in file <cljs repl>
WARNING: cljs.user/g cannot be instrumented (it is multi-arity and :static-fns is enabled) in file <cljs repl>
[cljs.user/f]
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:
ClojureScript Node.js REPL server listening on 51318
To quit, type: :cljs/quit
cljs.user=> (require '[clojure.spec.alpha :as s] '[clojure.spec.test.alpha :as st])
nil
cljs.user=> (defn f [x] 1)
#'cljs.user/f
cljs.user=> (s/fdef f :args any?)
cljs.user/f
cljs.user=> (defn g ([x] 1) ([x y] 2))
#'cljs.user/g
cljs.user=> (s/fdef g :args any?)
cljs.user/g
cljs.user=> (defn h ([x] 1) ([x & ys] 2))
#'cljs.user/h
cljs.user=> (s/fdef h :args any?)
cljs.user/h
cljs.user=> (st/instrument)
[cljs.user/h cljs.user/f cljs.user/g]
Hide
Mike Fikes added a comment -

Attaching re-baselined CLJS-2397-2.patch

Show
Mike Fikes added a comment - Attaching re-baselined CLJS-2397-2.patch

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: