Clojure

instrumented fdef with fspec unnecessarily invokes fspec generator

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Declined
  • Affects Version/s: Release 1.9
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Approval:
    Triaged

Description

With test.check is on the classpath, an instrumented fdef with fspec will invoke the generator for the fspec when invoked:

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

(defn foo [fnn] (fnn 42))
(s/fdef foo :args (s/cat :f (s/fspec :args (s/cat :i integer?)
                                     :ret integer?)))

(foo #(do (println %) (when (even? %) 42)))
42
42

(st/instrument `foo)

(foo #(do (println %) (when (even? %) 42)))
-1
0
-1
0
0
-1
0
-1
ExceptionInfo Call to #'user/foo did not conform to spec:
In: [0] val: nil fails at: [:args :f :ret] predicate: integer?
:clojure.spec/args  (#object[user$eval12$fn__13 0x515c6049 "user$eval12$fn__13@515c6049"])
:clojure.spec/failure  :instrument
:clojure.spec.test/caller  {:file "NO_SOURCE_FILE", :line 8, :var-scope user/eval12}
  clojure.core/ex-info (core.clj:4725)

Without test.check, this fails:

user=> (foo #(do (println %) (when (even? %) 42)))
FileNotFoundException Could not locate clojure/test/check/generators__init.class or clojure/test/check/generators.clj on classpath.  clojure.lang.RT.load (RT.java:458)

Activity

Hide
Zach Oakes added a comment -

I think it would make sense to add something like core.typed's ^:no-check for this. For example:

(s/fdef ^:no-check foo :args (s/cat :f (s/fspec :args (s/cat :i integer?) :ret integer?)))

As a stopgap measure, I made a boot task that has a copy of clojure.spec.test/run-all-tests and modifies it to ignore vars with that metadata. That means I have to add it to the metadata in defn rather than fdef but it still seems to work.

Show
Zach Oakes added a comment - I think it would make sense to add something like core.typed's ^:no-check for this. For example:
(s/fdef ^:no-check foo :args (s/cat :f (s/fspec :args (s/cat :i integer?) :ret integer?)))
As a stopgap measure, I made a boot task that has a copy of clojure.spec.test/run-all-tests and modifies it to ignore vars with that metadata. That means I have to add it to the metadata in defn rather than fdef but it still seems to work.
Hide
Sean Corfield added a comment -

Yes, there are definitely situations where I would want argument / return spec checking on calls during dev/test but absolutely need the function excluded from generative testing.

Show
Sean Corfield added a comment - Yes, there are definitely situations where I would want argument / return spec checking on calls during dev/test but absolutely need the function excluded from generative testing.
Hide
Sean Corfield added a comment -

If you don't have test.check on your classpath, the call to s/instrument succeeds but then attempting to call foo fails with:

boot.user=> (defn foo [fnn] (fnn 42))
#'boot.user/foo
boot.user=> (s/fdef foo :args (s/cat :f (s/fspec :args (s/cat :i integer?)
       #_=>                                      :ret integer?)))
boot.user/foo
boot.user=> 

boot.user=> (foo #(do (println %) (when (even? %) 42)))
42
42
boot.user=> (s/instrument 'foo)
#'boot.user/foo
boot.user=> (foo #(do (println %) (when (even? %) 42)))

java.io.FileNotFoundException: Could not locate clojure/test/check/generators__init.class or clojure/test/check/generators.clj on classpath.

That is certainly unexpected and not very friendly!

Show
Sean Corfield added a comment - If you don't have test.check on your classpath, the call to s/instrument succeeds but then attempting to call foo fails with:
boot.user=> (defn foo [fnn] (fnn 42))
#'boot.user/foo
boot.user=> (s/fdef foo :args (s/cat :f (s/fspec :args (s/cat :i integer?)
       #_=>                                      :ret integer?)))
boot.user/foo
boot.user=> 

boot.user=> (foo #(do (println %) (when (even? %) 42)))
42
42
boot.user=> (s/instrument 'foo)
#'boot.user/foo
boot.user=> (foo #(do (println %) (when (even? %) 42)))

java.io.FileNotFoundException: Could not locate clojure/test/check/generators__init.class or clojure/test/check/generators.clj on classpath.
That is certainly unexpected and not very friendly!
Hide
Alex Miller added a comment -

There are new options in instrument as of 1.9.0-alpha8 that allow you to stub/mock functions. Those are one potential answer to this and maybe the recommended one, although I haven't used them enough to say that for sure.

Show
Alex Miller added a comment - There are new options in instrument as of 1.9.0-alpha8 that allow you to stub/mock functions. Those are one potential answer to this and maybe the recommended one, although I haven't used them enough to say that for sure.
Hide
Alex Miller added a comment -

See also CLJ-1976.

Show
Alex Miller added a comment - See also CLJ-1976.
Hide
Sean Corfield added a comment -

Given that recent Alpha builds no longer check :ret or :fn with instrumentation, this issue seems to be resolved Alex Miller?

Show
Sean Corfield added a comment - Given that recent Alpha builds no longer check :ret or :fn with instrumentation, this issue seems to be resolved Alex Miller?
Hide
Allen Rohner added a comment -

fspec still requires generative testing.

Show
Allen Rohner added a comment - fspec still requires generative testing.
Hide
Sean Corfield added a comment -

Ah, OK, I thought that had also rolled back to just :args testing at this point (I hadn't retested this since we have test.check as dev/test dependency now anyway).

Show
Sean Corfield added a comment - Ah, OK, I thought that had also rolled back to just :args testing at this point (I hadn't retested this since we have test.check as dev/test dependency now anyway).
Hide
Alex Miller added a comment -

If an argument takes a function and you pass it a function, instrument cannot validate the fspec other than by generating args for the fspec and trying it out. So, this is the intended behavior. So, this is working as intended.

Show
Alex Miller added a comment - If an argument takes a function and you pass it a function, instrument cannot validate the fspec other than by generating args for the fspec and trying it out. So, this is the intended behavior. So, this is working as intended.
Hide
Allen Rohner added a comment -

Instrument could wrap the function in an fn that checks arguments the same way instrument does. fspec being tied to generators makes it significantly less useful for functions with side effects.

Show
Allen Rohner added a comment - Instrument could wrap the function in an fn that checks arguments the same way instrument does. fspec being tied to generators makes it significantly less useful for functions with side effects.
Hide
Alex Miller added a comment -

That's an interesting idea, not sure if it satisfies though. Also note that instrument provides a number of options for specifying simpler instrumented replacement specs/stubs.

Show
Alex Miller added a comment - That's an interesting idea, not sure if it satisfies though. Also note that instrument provides a number of options for specifying simpler instrumented replacement specs/stubs.
Hide
Brandon Bloom added a comment -

Following up from a discussion occurring now in Slack #clojure-spec. Lots more discussion there, check timestamp.

I found this very surprising. I've been using instrument during dev/repl-time (as recommended? distinguishing form test-time), but just learned that fspec isn't suitable for functions with side-effects due to the use of generators. I expected instrument to only instrument calls to vars, never to use the generators, and either 1) ignore fspec's details (easier) or 2) proxy the fn (many challenges here).

instrument has been very useful for dev (again, vs test), but not as useful as it could be if :ret and :fn were checked, and generators were never used.

Show
Brandon Bloom added a comment - Following up from a discussion occurring now in Slack #clojure-spec. Lots more discussion there, check timestamp. I found this very surprising. I've been using instrument during dev/repl-time (as recommended? distinguishing form test-time), but just learned that fspec isn't suitable for functions with side-effects due to the use of generators. I expected instrument to only instrument calls to vars, never to use the generators, and either 1) ignore fspec's details (easier) or 2) proxy the fn (many challenges here). instrument has been very useful for dev (again, vs test), but not as useful as it could be if :ret and :fn were checked, and generators were never used.
Hide
Brandon Bloom added a comment -

Whoops submitted a moment too soon. I wanted to add: During dev, I'd rather more things be checked a little bit (ie ret and fn) than fewer things be tested thoroughly (ie higher order functions subjected to generated inputs).

I think a distinction needs to be drawn between what can be conformed with 100% confidence and what cannot. For pure data w/o functions, we can expect conform to make some promises. With fspec etc, we can't rely on valid? => true as a security measure. I'm fine with that, but different contexts/times require different levels of confidence.

Show
Brandon Bloom added a comment - Whoops submitted a moment too soon. I wanted to add: During dev, I'd rather more things be checked a little bit (ie ret and fn) than fewer things be tested thoroughly (ie higher order functions subjected to generated inputs). I think a distinction needs to be drawn between what can be conformed with 100% confidence and what cannot. For pure data w/o functions, we can expect conform to make some promises. With fspec etc, we can't rely on valid? => true as a security measure. I'm fine with that, but different contexts/times require different levels of confidence.

People

Vote (2)
Watch (6)

Dates

  • Created:
    Updated:
    Resolved: