that is super weird

It seems like a global thing

you're saying that whether or not the generators namespace wants to include macros from clojure.core determines whether or not other namespaces can implicitly include macros from generators?

oh sorry, I was wrong there. I was looking for a test namespace that used gen/let in CLJS, but mistakingly pointed at the wrong line/namespace even. I’ll see if I can find a better example

this is the better example: https://github.com/clojure/test.check/blob/88eebf105fbb01db17ce12a3bff207cc395270b9/src/test/clojure/clojure/test/check/test.cljc#L18

sorry for the confusion

so you're saying removing that line would allow you to write what kind of test?

removing the conditional cljs portions of these require clauses in the test namespaces would enable you test if the macros work correctly in CLJS (provided they add a self-require)

you were asking for a way to test this behavior: this is a way to test it. another way would be to write a completely isolated test suite

okay I think I get it

I suppose that's probably good enough

I can imagine somebody removing the line from src that looks redundant and then seeing the test failure and fixing it by adding :include-macros true, but that's starting to sound rather less likely

the thing I was imagining was something like (ns clojure.test.check.require-test (:require [clojure.test.check.generators :as gen])) (deftest can-still-use-let-without-include-macros (is (gen/let [x gen/nat] 42)))

yes, but since it doesn’t seem a namespace local but a project-global inference thing, that doesn’t work if you combine it with the existing tests unchanged

so adapting the existing tests to remove the :include-macros option would already be the test

I don't understand that first statement

how does it not work?

ok. say you would add (ns clojure.test.check.require-test (:require [clojure.test.check.generators :as gen])) (deftest can-still-use-let-without-include-macros (is (gen/let [x gen/nat] 42))). this will already pass if clojure.test.check.generators :as gen has been required with :include-macros before.

I feel like I'm about to learn something terrible about clojurescript

in other words: that test already passes right now, without changing anything

you're saying that if namespace A requires X and namespace B requires X, then what's visible from X in B is affected by how A required it?

I’m not an expert in CLJS macro resolution but that’s my understanding of it. If A requires X with include-macros first, then B requires X without include-macros, it will still be able to use the macros.

I'm willing to bet fives of dollars that this is not true, because if it's true then I'll be so sad that I won't think to miss my fives of dollars

well, it’s easy to try out. just add this test namespace of yours and run the tests.

I’m not saying that I want this to be true, just that I saw this behavior when adding a similar test 🙂

On the other hand, I don’t see why this would be problematic. If you don’t add :include-macros true, the worst thing that will happen is that you will get a function instead of a macro resulting in an error (the original problem), so not getting the error as a side effect of some other namespace doing the require with include-macros, is a nice bonus.

@mfikes or @dnolen could maybe explain how this works in detail

okay, I'm running such a test now

(ns clojure.test.check.test2
  (:require [cljs.test :refer-macros [deftest is]]
            [clojure.test.check.generators :as gen]))

(deftest can-use-gen-let-without-include-macros
  (is (gen/let [x gen/nat] 42)))

at first I got a compiler warning about x not being defined, which I thought was a good sign, but haven't been getting that since

Have you cleared the target for?

Dir

(Damn spelling corrector)

yeah

and there's nothing else in the project that's requiring gen with :include-macros true

I just pushed this to the branch tmp-f5667927-abed-48d9-b5a8-7c7f0295b001 if you want to take a look

I’ll check it out, be back in a couple of hours

@gfredericks Change your test ns to something like:

(ns clojure.test.check.test2
  (:require [cljs.test :as t :refer-macros [deftest is]]
            [clojure.test.check.generators :as gen]))

(deftest can-use-gen-let-without-include-macros
  (is (gen/let [x gen/nat]
        (number? x))))

(t/run-tests)

$ clj -Sdeps '{:deps {org.clojure/clojurescript {:mvn/version "RELEASE"}}}' -m cljs.main -re node -i src/test/clojure/clojure/test/check/test2.cljs
WARNING: Use of undeclared Var clojure.test.check.test2/x at line 6 test2.cljs
WARNING: Use of undeclared Var clojure.test.check.test2/x at line 7 test2.cljs

Testing clojure.test.check.test2

ERROR in (can-use-gen-let-without-include-macros) (Error:NaN:NaN)
expected: (gen/let [x gen/nat] (number? x))
  actual: #object[Error Error: Assert failed: First arg to gen/let must be a vector of bindings.
(vector? bindings)]

Ran 1 tests containing 1 assertions.
0 failures, 1 errors.

We could just write a script for this to isolate it from the rest of the tests (if you still want this tested, I think it’s pretty rare that anyone does this in a CLJS lib)

@borkdude But is it true that the test then passes if you revert the changes to the other test namespaces?

not in this case, since I only load that one namespace here. I could test it by also loading those first

@gfredericks This branch: https://github.com/borkdude/test.check/tree/tmp-f5667927-abed-48d9-b5a8-7c7f0295b001

$ clj -Sdeps '{:deps {org.clojure/clojurescript {:mvn/version "RELEASE"}}}' -m cljs.main -re node -i src/test/clojure/clojure/test/check/test2.cljs
WARNING: Use of undeclared Var clojure.test.check.test2/x at line 6 test2.cljs
WARNING: Use of undeclared Var clojure.test.check.test2/x at line 7 test2.cljs

Testing clojure.test.check.test2

ERROR in (can-use-gen-let-without-include-macros) (Error:NaN:NaN)
expected: (gen/let [x gen/nat] (number? x))
  actual: #object[Error Error: Assert failed: First arg to gen/let must be a vector of bindings.
(vector? bindings)]

Ran 1 tests containing 1 assertions.
0 failures, 1 errors.

$ clj -Sdeps '{:deps {org.clojure/clojurescript {:mvn/version "RELEASE"}}}' -m cljs.main -re node -i src/test/clojure/clojure/test/check/test.cljc -i src/test/clojure/clojure/test/check/test2.cljs
WARNING: var: clojure.test.check.generators/gen-raw-long is not public at line 158 test.cljc
WARNING: var: clojure.test.check.generators/gen-raw-long is not public at line 158 test.cljc

Testing clojure.test.check.test2

Ran 1 tests containing 1 assertions.
0 failures, 0 errors.

I guess I’m pretty close to winning that 5 dollars…

It seems that once the CLJS compiler knows something is a macro, it cannot unknow it?

maybe that is what happens here (although that’s just me guessing): https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/analyzer.cljc#L629

so when you add a self-refer to a macro in a .cljc namespace, you effectively tell the CLJS compiler: I want to use this var as macro in CLJS. Any subsequent require to that namespace does not have to help CLJS with that anymore by adding include-macros true or refer-macros, it already knows.

I just asked in #cljs-dev about this

ok, so it’s confirmed that this behavior exists

what about making a standalone script that just does this:

clojure -Sdeps '{:deps {org.clojure/clojurescript {:mvn/version "RELEASE"}}}' -m cljs.main -re node -i src/test/clojure/clojure/test/check/test2.cljs

test2 would be called test_macros.cljs then

Blergh

How about we just remove all the include-macro clauses from all src/test nses, except for the self ones

yeah, that was my idea when I suggested that the tests already test this if they don’t add include-macros themselves

but we could not reproduce this with lein cljsbuild

maybe that build tool has some additional magic

but I’m fine with your proposal

I’m looking at the cljsbuild code, but it seems there is some magic around macros in there: https://github.com/emezeske/lein-cljsbuild/blob/master/support/src/cljsbuild/crossover.clj

I can see why the CLJS authors ask for repros not using these tools 🙂

yeah I think a global policy of "no other namespace should need to require-macros from a t.c namespace because they all do self-requiring" is good enough

so the additions to your patch would be A) deleting redundant :include-macros B) doing this trick for the prop namespace too

ok, will do

also in clojure.test.check.clojure-test for defspec I presume

👍 yep

thanks for tracking everything down

New patch uploaded

okay, I should get to it sometime this week

thanks 🙂