Generator not working for clojurescript > 1.10.520

whacked commented 4 years ago

This is related to https://github.com/plumatic/schema-generators/issues/21, which helped me trace to clojurescript 1.10.520.

ISSUE

when using clojurescript > 1.10.520, generating for any non-primitive schema seems to fail with

> (g/sample 10 {:a s/Str})
The result object failed to print. It is available via *1 if you want to interact with it.
The exception was: 
#error {:message "Couldn't satisfy such-that predicate after 10 tries.", :data {:pred #object[G__31076], :gen #clojure.test.check.generators.Generator{:gen #object[Function]}, :max-tries 10}}

generating for primitives works as expected

cljs.user=> (g/sample 10 s/Str)
("" "" "" "(" "|" "s" "BdB" "or@nPC" "3nFU" "yTVJ}a")

REPRO

https://github.com/whacked/sg-issue-repro

note that for clojurescript 1.10.773, the error is printed to the REPL
for 1.10.597, it is printed to the host's console and not the REPL

workaround

based on #21 I have verified that this issue disappears when building with clojurescript 1.10.520. This is achieved by pulling npm install shadow-cljs@2.8.71, which declares clojurescript 1.10.520 as a dependency. shadow-cljs@2.8.72 declares 1.10.597 which begins exhibiting this issue.

other notes

I did a very quick check directly against clojure.test.check.random based on your comment in https://github.com/plumatic/schema-generators/issues/21#issuecomment-611113765

> (require '[clojure.test.check.generators :as gen])                                                                                                            
> (gen/sample gen/large-integer)
(0 -1 -1 -1 0 -13 4 12 5 1)
> (gen/sample gen/double)
(-0.5 2 1 0.5625 -0.5 -1.75 -3 0.76171875 -2.5 0.25)

I didn't dig much deeper than this, but when using the 10-sample default for test.check generators, there's very high bias for 0 and -1, for example,

(->> (repeatedly (partial gen/sample gen/large-integer))
     (take 100)  #  generate 100 rounds of 10 large-integers per sampling
     (apply concat)
     (frequencies)
     (map (comp vec reverse))
     (sort)
     (take-last 10)
     (map (comp vec reverse))
     (cljs.pprint/pprint))
([6 11]
 [-5 12] 
 [2 23] 
 [-4 29] 
 [-3 33] 
 [3 38] 
 [-2 72] 
 [1 82] 
 [-1 244] 
 [0 261])

So this could be related to the repeated random values that you point out in the comment, but the test.check dependency doesn't change across shadow 2.8.72 and 2.8.72, so perhaps the root cause is elsewhere.

gfredericks commented 4 years ago

Is it possible that this is the same root cause as https://clojure.atlassian.net/browse/TCHECK-157 and can be fixed the same way?

I'm wondering if shadow-cljs does too much pre-compilation.

Though honestly now that I've said it out loud I can't imagine a way that this would result in the reported symptom, but I'm going to post this comment anyhow.

gfredericks commented 4 years ago

I should clarify that the bias you're observing in some of the generators is there by design, and for standard uses of such-that should not be an issue.

The repetition I was referring to in the other issue would be from the random-number-generator directly (i.e., using the clojure.test.check.random namespace, not using "generators" in the test.check sense at all).

whacked commented 4 years ago

Thanks for the response. A little bit more digging shows that the predicate failure is thrown within a call to gen-fmap in https://github.com/clojure/test.check/blob/master/src/main/clojure/clojure/test/check/generators.cljc#L53

1.10.520 makes it past (h rnd size) whereas > 1.10.520 does not

when that call happens, there's a stack trace like this:

SHADOW import error /tmp/qs-b-fail/.shadow-cljs/builds/tester/dev/out/cljs-runtime/shadow.module.main.append.js

/tmp/qs-b-fail/.shadow-cljs/builds/tester/dev/out/cljs-runtime/cljs/core.cljs:11332
(defn ^{:jsdoc ["@constructor"]}
^
Error: Couldn't satisfy such-that predicate after 10 tries.
    at new cljs$core$ExceptionInfo (/tmp/qs-b-fail/.shadow-cljs/builds/tester/dev/out/cljs-runtime/cljs/core.cljs:11332:1)
    at Function.cljs$core$IFn$_invoke$arity$3 (/tmp/qs-b-fail/.shadow-cljs/builds/tester/dev/out/cljs-runtime/cljs/core.cljs:11361:1)
    at Function.cljs$core$IFn$_invoke$arity$2 (/tmp/qs-b-fail/.shadow-cljs/builds/tester/dev/out/cljs-runtime/cljs/core.cljs:11361:1)
    at /tmp/qs-b-fail/.shadow-cljs/builds/tester/dev/out/cljs-runtime/clojure/test/check/generators.cljc:434:11
    at /tmp/qs-b-fail/.shadow-cljs/builds/tester/dev/out/cljs-runtime/clojure/test/check/generators.cljc:425:14
    at Object.clojure$test$check$generators$such_that_helper [as such_that_helper] (/tmp/qs-b-fail/.shadow-cljs/builds/tester/dev/out/cljs-runtime/clojure.test.check.generators.js:729:3)
    at /tmp/qs-b-fail/.shadow-cljs/builds/tester/dev/out/cljs-runtime/clojure/test/check/generators.cljc:477:7
    at /tmp/qs-b-fail/.shadow-cljs/builds/tester/dev/out/cljs-runtime/mytest/core.cljs:139:25
    at /tmp/qs-b-fail/.shadow-cljs/builds/tester/dev/out/cljs-runtime/mytest/core.cljs:148:13
    at /tmp/qs-b-fail/.shadow-cljs/builds/tester/dev/out/cljs-runtime/cljs/core.cljs:4741:15

I didn't dig beyond this but maybe this helps orient the problem better. I'm just running a :node-script output target in the terminal Do you have a quick way of checking whether the predicate error is actually a result of the shadow.module.main.append.js import error?

gfredericks commented 4 years ago

I'm not very well versed in the modern cljs ecosystem -- I've never heard of shadow-cljs and I don't know what it does. Reducing the number of tools involved in reproducing a problem is always valuable.

whacked commented 4 years ago

good point.

I've updated the master branch in https://github.com/whacked/sg-issue-repro to use only leiningen + 1 source file. The result looks identical. The latest commit uses clojurescript 1.0.773, which exhibits the predicate exception, commenting out 1.0.520, which does not exhibit the exception.

$ lein cljsbuild auto

$ node out/index.js

gfredericks commented 4 years ago

I narrowed this down to the following behavior, which works correctly in 1.10.520 but is broken in 1.10.597:

(defrecord Rec [constructor])

(let [rec-instance (->Rec #(+ 4 %))]
  (case ((:constructor rec-instance) 19)
    23 :good
    nil :bad
    :unexpected))

I.e., this is specifically about records with a field named constructor -- for other field names, this does not happen.

It's also broken when accessing the field with (.-constructor some-rec-instance)

The specific instance of this in spec is here. This implies that a workaround is to rename that key. I don't know off the top of my head if that key is meant to be user-facing (I suspect not), or if there's any other reason to avoid renaming it. I'd like to check if @w01fe has an opinion before recommending that change.

gfredericks commented 4 years ago

I checked that the test suite fails with this error, and it does, but strangely not for cljs version 1.10.597, only for later versions. I'm not sure why that would be.

Anyhow, details here: https://github.com/plumatic/schema-generators/commit/d30f06996dc354c34d3d03efc19b989d6a11e3f2

gfredericks commented 4 years ago

w01fe commented 4 years ago

Thanks for tracking this down. I think it's fine to change the parameter name.

gfredericks commented 3 years ago

@whacked if you make a PR to the schema repo to change that name I can help get it released

whacked commented 3 years ago

Thanks, I'll look over it this coming weekend

whacked commented 3 years ago

No breakthroughs. Either I'm not going about this correctly or this is trickier than I thought. A few notes:

as you noticed, the breakage in this lib happens between cljs version 1.10.597 (works) and 1.10.741 (breaks)
messing with the naming of constructor in https://github.com/plumatic/schema/blob/ddb54c87dea6926c6d73542e517b53685cc82a37/src/cljx/schema/spec/collection.cljx#L70 in a variety of manners does not seem to affect the failing tests for 741.
- initial attempt is to rename constructor to konstructor
for any manner of minor changes to this name, the tests always pass in the prismatic/schema repo, regardless of cljs version
tests for 597 similarly pass for those minor changes in prismatic/schema-generators
I have also applied corresponding name changes to other possible consumers of the record within prismatic/schema, i.e. in schema.core, schema.generators.experimental, schema.spec.collection. Once you're in 741 it looks like the number generator starts giving NaNs, e.g.
```
Testing schema-generators.generators-test
```

ERROR in (sample-test) (Error:NaN:NaN) expected: (= (count res) 20) actual: #error {:message "Couldn't satisfy such-that predicate after 10 tries.", :data {}}


interesting warnings appear at the top from tests for 741 and not for 597:

WARNING: Use of undeclared Var goog.math.Long/fromString at line 41 out/clojure/test/check/random/longs.cljs WARNING: Use of undeclared Var goog.math.Long/fromNumber at line 45 out/clojure/test/check/random/longs.cljs WARNING: Use of undeclared Var goog.math.Long/fromNumber at line 51 out/clojure/test/check/random/longs.cljs WARNING: Use of undeclared Var goog.math.Long/getOne at line 56 out/clojure/test/check/random/longs.cljs WARNING: Use of undeclared Var goog.math.Long/fromString at line 41 out/clojure/test/check/random/longs.cljs WARNING: Use of undeclared Var goog.math.Long/fromNumber at line 45 out/clojure/test/check/random/longs.cljs WARNING: Use of undeclared Var goog.math.Long/fromNumber at line 51 out/clojure/test/check/random/longs.cljs WARNING: Use of undeclared Var goog.math.Long/getOne at line 56 out/clojure/test/check/random/longs.cljs WARNING: cljs.core/<=, all arguments must be numbers, got [#{nil js/Number} number] instead at line 999 out/clojure/test/check/generators.cljc WARNING: cljs.core/<=, all arguments must be numbers, got [#{nil js/Number} number] instead at line 999 out/clojure/test/check/generators.cljc


for posterity, this is how I'm setting up the testing. suggestions for improved methods welcome.

```sh
$ pwd
/tmp
$ ls
schema-generators
schema
$ ls -l schema-generators/checkouts/
schema -> /tmp/schema

comment out the [prismatic/schema "1.1.11"] dependency in schema-generators/project.clj

make changes to e.g. schema/src/cljx/schema/spec/collection.cljx, then

$ cd /tmp/schema
$ lein clean; lein cljx
$ cd /tmp/schema-generators
$ lein test

gfredericks commented 3 years ago

Is there any chance that you aren't successfully using the modified version of schema when running the tests for schema-generators?

whacked commented 3 years ago

I'm sure there's a chance, but the way I go about it, rather primitively, is that I make breaking changes in the schema checkout dep, which then causes schema-generators to break; revert, lein clean; lein cljx and it runs again. That's how I determine it's picking up the correct version and ended up with the workflow described. I have also done lein clean; lein cljx; lein install from schema and used [prismatic/schema "1.1.13-SNAPSHOT"] which then refers to the local build in the local maven cache; this behaves similarly.

If you have a better method, I'm more than happy to adopt it.

gfredericks commented 3 years ago

I haven't tried to sort through the issues you described because it's a big headache, but I did try to fix this on my own and came up with this:

the konstructor branch in the schema repo seems to work inasmuch as the tests still pass -- this is what I spent the most time on because I did a naive search-and-replace of constructor and it took a while to notice that there is one (.-constructor foo) call that is actually depending on the magic .-constructor property that caused this whole mess in the first place
the issue-22-fix branch in the schema-generators repo has a single line change to access the new key; I also set the schema dependency to a snapshot, which will only work if you lein install locally from the aforementioned schema branch; note that this schema-generators branch does not update the cljs version, so you'll want to play with that to convince yourself it works

w.r.t. releasing these things the big question is how much version compatibility to try to maintain; in particular

A) should the new version of schema work with older versions of schema-generators? (as-is it won't, since the old versions of schema-generators are looking for a :constructor key which won't be there anymore) B) should the new version of schema-generators work with older versions of schema? (as-is it won't, since the new version of schema-generators is looking for a :konstructor key which isn't there yet)

I defer to @w01fe on whether it's worth complicating the code to achieve either of these.

w01fe commented 3 years ago

@gfredericks Thanks for looking into this! I guess schema-generators still claims it's alpha, so there's no claimed guarantee of compatibility. I'd be inclined to say go for it, it should be easy enough for folks to bump one when they bump the other. But up to you.

gfredericks commented 3 years ago

Okay, I'll release the changes above once @whacked confirms that they solve the original problem.

williammizuta commented 2 years ago

@whacked do you have any updates about this topic? I am getting the same error when trying to schema-generators.generators/sample inside a deftest

williammizuta commented 2 years ago

@gfredericks I updated the branches you created applying the same change and it worked for me.

schema: https://github.com/plumatic/schema/pull/438 schema-generators: https://github.com/plumatic/schema-generators/pull/27

It only needs to change the plumatic/schema version used in schema-generators after plumatic/schema is published

w01fe commented 2 years ago

Merged the Schema change and released 1.2.1. Thanks!

whacked commented 2 years ago

@williammizuta sorry, since my last comment I didn't invest more time into this diagnosis; I have to defer.

williammizuta commented 2 years ago

@w01fe thanks. I've just updated https://github.com/plumatic/schema-generators/pull/27 to use the new released 1.2.1 version

plumatic / schema-generators