Fork me on GitHub
#cljs-dev
<
2018-11-08
>
borkdude09:11:16

I think this is a bug, but I’d like to verify. This works on clj but not on cljs. The output of instrumentable-syms should be valid as input for instrument.

(require '[clojure.spec.alpha :as s])
(require '[clojure.spec.test.alpha :as stest])
(defn foo [x])
(s/fdef foo :args (s/cat :x int?))
(defn bar [x])
(s/fdef bar :args (s/cat :x int?))
(def syms (stest/instrumentable-syms))
(println syms)
(stest/instrument (disj syms `bar))
(try (foo "string")
     (catch #?(:clj Exception :cljs :default) _
       (println "foo throws")))
(bar "string") ;; bar doesn't throw

borkdude09:11:44

this gets tedious fast if you can only pass a literal expression. e.g. in one function I had to spell out all possible instrumentable syms but one.

thheller09:11:33

IIRC instrument is based on a macro and might have issues with dynamic values

thheller09:11:53

its not as flexible as CLJ in that regard

mfikes12:11:07

That smells like an actual bug because stest/instrument involves eval

mfikes12:11:34

Ahh, when I try this in a REPL, it has heartburn trying to eval the form (disj ...), being unable to resolve the symbol.

dnolen17:11:13

@borkdude instrument is macro, if you need to do fancier stuff you also need to write a macro

dnolen17:11:35

this is exactly the same as the testing macros

dnolen17:11:45

there's no vars to magically do this stuff

dnolen17:11:01

2966 is not a bug

borkdude17:11:03

@dnolen thanks for confirming

borkdude17:11:14

it works in some circumstances by luck then I guess

dnolen17:11:32

right, nothing you can depend on

dnolen17:11:54

we should probably write a pre-condition or something that errors on incorrect usage

borkdude17:11:08

makes sense, since it’s a macro there’s no performance penalty

borkdude17:11:11

or write a spec for it, but since it’s part of spec, I’m not sure that works - maybe it would

mfikes17:11:44

Yeah, I was mistaken about the use of eval in the macro. (It is really there just to help massage compile time data that might be using quoting.)

borkdude18:11:18

I’m not sure what the macro should look like when you want to instrument all but one sym:

(defmacro my-macro [exclude-sym]
  (let [isms (stest/instrumentable-syms)
        wo-foo (disj isms exclude-sym)]
    `(stest/instrument ~wo-foo)))
Here I get: Caused by: java.lang.AssertionError: Assert failed: (symbol? sym)

borkdude18:11:59

Splicing in with ~@wo-foo gets me: Caused by: java.lang.IllegalArgumentException: Don't know how to create ISeq from: repro$bar where the call is

(my-macro `foo)

dnolen19:11:10

not enough information

dnolen19:11:26

the assertion tells you where it's going wrong

dnolen19:11:50

the stacktrace

borkdude19:11:04

yes, resolve.

dnolen19:11:15

so fix it? 🙂

borkdude19:11:35

yeah, I wasn’t sure if my macro sucked 😉

dnolen19:11:24

but I think you should be able to sort this out

borkdude19:11:07

Got it. By adding this clause to form->sym-or-syms:

(cond
    (::no-eval (meta sym-or-syms))
    (second sym-or-syms)
    (every? symbol? sym-or-syms)
    sym-or-syms
    :else
    (eval sym-or-syms))
something like
(stest/instrument [cljs.user/foo cljs.user/bar])
becomes possible. The rest of the cljs tests all pass. And this fixed my macro problem.

dnolen19:11:34

but why do we need to do that

dnolen20:11:10

anyways your macro needs to deal with syntax quote

dnolen20:11:20

we’re not going to change anything here

borkdude20:11:25

If we allow both

(stest/instrument [`foo `bar])
and (stest/instrument [cljs.user/foo cljs.user/bar]) then this kind of macro becomes possible:
(defmacro instrument-without [exclude-syms]
   (let [isms (stest/instrumentable-syms)
         wo-sym (apply disj isms exclude-syms)]
     `(stest/instrument ~wo-sym)))
I’m not sure it’s possible without that change.

dnolen20:11:45

I would keep trying 🙂

borkdude20:11:40

Got it:

(defmacro instrument-without [exclude-syms]
  (let [exclude-syms (map second exclude-syms)
        isms (stest/instrumentable-syms)
        filtered (apply disj isms exclude-syms)
        filtered (mapv (fn [sym]
                         [sym]
                         (list 'quote sym))
                       filtered)]
    `(stest/instrument ~filtered)))

(instrument-without [`foo])

mfikes20:11:57

@borkdude instrumentable-sysms is a runtime function, right? How does your macro call it?

mfikes20:11:34

Ahh, I see, the macro above can work in self-hosted. To make one that works in JVM as well, I would just riff on the shipping instrument:

(defmacro instrument-without [exclude-syms]
  `(stest/instrument '[~@(apply disj
                        (#?(:clj  s/speced-vars
                            :cljs cljs.spec.alpha$macros/speced-vars))
                        (eval exclude-syms))]))

borkdude20:11:25

This code worked for me on the JVM:

(ns repro.core
  (:require [clojure.spec.alpha :as s]
            [clojure.spec.test.alpha :as stest])
  #?(:cljs (:require-macros [repro.core :refer [instrument-without]])))

(defn foo [i])
(s/fdef foo :args (s/cat :i number?))

(defn bar [i])
(s/fdef bar :args (s/cat :i number?))

(defn baz [i])
(s/fdef baz :args (s/cat :i number?))

(defmacro instrument-without [exclude-syms]
  (let [exclude-syms (map second exclude-syms)
        isms (stest/instrumentable-syms)
        filtered (apply disj isms exclude-syms)
        filtered (mapv (fn [sym]
                         [sym]
                         (list 'quote sym))
                       filtered)]
    `(stest/instrument ~filtered)))

(defn -main [& args]
  (println "hello" (instrument-without [`foo])))

#?(:cljs (set! *main-cli-fn* -main))

borkdude20:11:11

The output is:

hello [repro.core/bar repro.core/baz]

mfikes20:11:18

Oh, by "JVM," I meant JVM-ClojureScript.

mfikes20:11:06

OK, trying to understand what you did with a macro sitting in the same namespace with -main 🙂

borkdude20:11:06

compiled code does the same as called with -m:

clj -R:cljs/dev -m cljs.main -re node -c repro.core

borkdude20:11:40

it’s cljc

mfikes20:11:00

OK. I see.

borkdude20:11:28

I only got it to work in a multi-segmented namespace, I think because there is some code looking for "." in the symbols

mfikes20:11:52

I'm wondering if you are actually picking up the vars defined in Clojure... hrm.

borkdude20:11:22

if I compile this to .js and then run node out/main.js there is no clojure anymore?

mfikes21:11:11

I'm referring to foo, etc. So, if you instead

#?(:cljs (defn foo [i]))
and put all of the other stuff like that in conditional blocks so that they are not defined in Clojure, then you will see a difference in output.

mfikes21:11:07

In other words, I suspect you are getting it to print things that look correct only because it is actually Clojure's spec implementation doing it, not ClojureScript's.

borkdude21:11:20

oops, you’re right

borkdude21:11:39

So that’s the kind of macro we end up with… not pretty 🙂

mfikes21:11:42

The above macro wouldn't work in Clojure, so a truly 3-way portable one would look uglier.

borkdude21:11:08

well, the challenge was to make it work in cljs, clojure is trivial

borkdude21:11:08

what’s s/speced-vars? macro doesn’t work here yet

mfikes21:11:42

The macro is currently assuming that the runtime spec and spec.test namespaces are already loaded. Here is a reference to the full thing showing it working: https://gist.github.com/mfikes/c1b6ce9b7a3a80559774bd89accaea50

mfikes21:11:41

The main bit you may be missing is the need to require cljs.spec.alpha, not clojure.spec.alpha in the macro namespace. (Otherwise you are getting Clojure's implementation, which doesn't have s/speced-vars)

borkdude21:11:48

that’s indeed tricky when you refer to clojure.spec.alpha in a .cljc file, I didn’t realize that. Is that also something that could go wrong in speculative.core.cljc right now?

mfikes21:11:11

Not a problem in Speculative. The issue is not really whether clojure.* is being required from a .cljc file, but whether the file is being compiled as Clojure or ClojureScript. When it is Clojure, you get the actual namespace being required, and when it is ClojureScript, clojure.* -> cljs.* aliasing kicks in.

borkdude21:11:58

I still don’t fully get it. When I run this:

clj -R:cljs/dev -m cljs.main -re node -m repro.core
I’m compiling the file as ClojureScript, no?

mfikes21:11:50

But, it then requires macros on itself, which in JVM ClojureScript causes it to be compiled as Clojure as well, for the macros namespace compilation.

borkdude21:11:13

ah right. when there’s a .clj file, it looks there for macros

borkdude21:11:59

and when you’re making a macro in a cljc file and you self-refer it, it also works like that

mfikes21:11:08

A simple way to understand this macro: It is the 0-arity version of stest/instrument, with a slight modification to (apply disj ... (eval excluded-syms))

borkdude21:11:33

how is that the 0-arity version, isn’t it just the 1-arity version you’re calling?

borkdude21:11:37

oh man… this is tricky 🙂

borkdude21:11:41

so no need to add the ::no-eval metadata in your version of the macro?

mfikes22:11:00

Right. That’s, strictly speaking, an internal optimization that comes into play when dealing with a crap ton of specs

borkdude22:11:51

I’m wondering if I’m running into a similar issue here: https://github.com/borkdude/speculative/blob/instrument-all/test/speculative/core_test.cljc#L16 The specs aren’t instrumented, 0 of them, in JVM cljs.

borkdude22:11:05

after calling (stest/instrument) that is

mfikes22:11:54

Truth be told, you probably want ::no-eval, but it’s unfortunately not part of the public contract ¯\(ツ)

mfikes22:11:07

See CLJS-2883 for more insight into the ::no-eval thing

borkdude22:11:29

I think I might be running into a problem with the 0-arity in the link above. At compile time nothing is instrumented yet, so the test doesn’t pass. Does that make sense?

mfikes22:11:22

On the surface, no. But, AFK.

borkdude22:11:43

although it first requires speculative.core so that doesn’t make sense to me either.

borkdude23:11:16

This is weird. Commenting out the require on clojure.spec.alpha makes the instrument-all test pass… https://github.com/borkdude/speculative/commit/559caea91f7beb5cbf6b98b2a068d96f472185aa#diff-942e8a56b45a1acf66fc6a611eb6aa1dR3

borkdude09:11:53

found it. was a reloading issue