Skip to content

Fix recursive gen attempt 2 #677

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

Merged
merged 1 commit into from
Aug 27, 2022
Merged

Conversation

frenchy64
Copy link
Collaborator

@frenchy64 frenchy64 commented Mar 28, 2022

Close #452

Another attempt at #507

@frenchy64 frenchy64 marked this pull request as ready for review March 29, 2022 04:22
@frenchy64 frenchy64 changed the title WIP: fix recursive gen attempt 2 Fix recursive gen attempt 2 Mar 29, 2022
@frenchy64
Copy link
Collaborator Author

@ikitommi here's another shot at the recursive gen fix, all changes are in the generators ns. Still need a few more tests, but what do you think?

@ikitommi
Copy link
Member

I finally had time to read this through, looks really good as everything is resolved on the generator ns. Big thanks to your efforts on this, I really appreciate. Before merging, you think this is done, adds real support for recursive schemas with no new gotchas for using it? e.g. TODO test for :ref shadowing:, I would not know how to handle the shadowing if that would not worked (and if that ever worked before).

@ikitommi
Copy link
Member

oh, missed the Still need a few more tests part. I'll merge this when you are happy with it.

@frenchy64
Copy link
Collaborator Author

@ikitommi thanks for looking. Adding the test indeed revealed a bug, and I don't think my solution is general enough. Needs more thought.

@frenchy64
Copy link
Collaborator Author

I think I figured it out--very subtle! Explained the reasoning in comments.

I will try and improve the error message for infinite schemas, right now it's pretty confusing.

@frenchy64
Copy link
Collaborator Author

Still needs more work. I broke something: the generation never goes more than 1 recursion in the example in the original issue:

(mg/sample
  [:schema {:registry {::cons [:maybe [:tuple pos-int? [:ref ::cons]]]}}
   ::cons]
  {:size 30})
=> (nil [2 nil] [2 nil] [2 nil] [4 nil] [7 nil] [3 nil] nil [1 nil] nil [1 nil] [17 nil] [490 nil] [3444 nil] nil nil nil [20236 nil] nil nil [501 nil] nil [75 nil] [2703 nil] nil [14 nil] [237664 nil] nil [3733313 nil] [2 nil])

@frenchy64
Copy link
Collaborator Author

The problem was because caching does not take into account options. We need to disable caching while generating recursive generators. Added a test to show that recursive generators still cache.

I will sleep on it.

@ikitommi
Copy link
Member

Thanks for working on this.

@frenchy64 frenchy64 force-pushed the issue-452-rec-gen3 branch 9 times, most recently from b25966f to ddec795 Compare August 23, 2022 19:41
Also fixes problems with the :string and :vector generators,
where if you only provided a minimum size, the maximum
was set to (* 2 min). Using gen/sized to calculate the maximum
yields generators with better distributions.
@frenchy64
Copy link
Collaborator Author

@ikitommi thanks. If you're happy with it, please merge. Happy to address any feedback.

Before merging, you think this is done, adds real support for recursive schemas with no new gotchas for using it?

There's one optional tweak that users can make to their schemas to optimize their generators---I've added that to the README. Completely optional though.

The only people required to make changes is anyone that has created their own schemas that use -recur or -maybe-recur. They should trigger the println's I've added in those functions and hopefully they will be changed as a result.

@frenchy64 frenchy64 requested a review from ikitommi August 24, 2022 01:01
@ikitommi
Copy link
Member

This PR is superb in all ways I think of - it solves a hard problem in an elegant way with easy-to-follow explanation of what happens and why, lot of tests to verify it works correctly while keeping the code in the malli.generator ns. I learned a lot of implementation by following the hand-crafted generators in tests.

Big thanks @frenchy64!

@ikitommi ikitommi merged commit 888649b into metosin:master Aug 27, 2022
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

Successfully merging this pull request may close these issues.

On fixing Malli's recursive generation
2 participants