Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generator not working for clojurescript > 1.10.520 #22

Open
whacked opened this issue Aug 30, 2020 · 22 comments
Open

Generator not working for clojurescript > 1.10.520 #22

whacked opened this issue Aug 30, 2020 · 22 comments

Comments

@whacked
Copy link

whacked commented Aug 30, 2020

This is related to #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 [email protected], which declares clojurescript 1.10.520 as a dependency. [email protected] 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 #21 (comment)

> (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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

whacked commented Sep 5, 2020

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
Copy link
Contributor

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
Copy link
Author

whacked commented Sep 6, 2020

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
Copy link
Contributor

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
Copy link
Contributor

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: d30f069

@gfredericks
Copy link
Contributor

Probably related: https://clojure.atlassian.net/browse/CLJS-871

@w01fe
Copy link
Member

w01fe commented Sep 7, 2020

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

@gfredericks
Copy link
Contributor

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

@whacked
Copy link
Author

whacked commented Sep 15, 2020

Thanks, I'll look over it this coming weekend

@whacked
Copy link
Author

whacked commented Sep 20, 2020

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.

$ 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
Copy link
Contributor

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

@whacked
Copy link
Author

whacked commented Sep 20, 2020

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
Copy link
Contributor

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
Copy link
Member

w01fe commented Oct 11, 2020

@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
Copy link
Contributor

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

@williammizuta
Copy link
Contributor

@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
Copy link
Contributor

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

schema: plumatic/schema#438
schema-generators: #27

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

@w01fe
Copy link
Member

w01fe commented Apr 29, 2022

Merged the Schema change and released 1.2.1. Thanks!

@whacked
Copy link
Author

whacked commented Apr 29, 2022

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

@williammizuta
Copy link
Contributor

@w01fe thanks.
I've just updated #27 to use the new released 1.2.1 version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants