Deriver: share fuel between mutual generators

[vch9]

Fixes #188

Now mutual recursive generators share the size fuel (or indicator w/e).

open QCheck.Gen

let rec gen_tree_sized n =
  map
    (fun gen0 -> Node gen0)
	(map
	  (fun (gen0, gen1) -> (gen0, gen1))
	  (pair int (gen_forest (n / 2))))))]

and gen_forest_sized n = match n with
  | 0 -> pure Nil
  | n ->
    frequency
	  [(1, pure Nil);
          (1,
	        (map (fun gen0 -> Cons gen0)
                  (map (fun (gen0, gen1) -> (gen0, gen1))
                  (pair (gen_tree_sized (n / 2)) (gen_forest_sized (n / 2))))))]))

let gen_tree = sized @@ gen_tree

let gen_forest = sized @@ gen_forest

Are you ok with that fix @jmid ?

Also in ba96a95 I removed the use of frequency when the list contains one element only.

[jmid]

Great, thanks! 🙂 👍

Is it correctly understood that

• variant types derive "unsized" _gen generators based on oneof
• recursive variant types derive sized _gen generators prefixed with a Gen.sized @@
• mutually recursive variant types derive a pair of generators (one _sized and one _gen with the latter calling the former with Gen.sized @@)?

If so, would it make sense to unify the last two, so that any kind of recursive variant needing fuel (mutually recursive or not) always derives a pair of generators _sized and _gen?

• it would be more predictable
• this would mean one less special case to understand/consider/remember - both for developers and for end-users
• it would allow more flexibility if one wants to pass a different fuel generator with Gen.sized_size than the default one that Gen.sized uses

[vch9]

• recursive variant types derive sized _gen generators prefixed with a Gen.sized @@
• mutually recursive variant types derive a pair of generators (one _sized and one _gen with the latter calling the former with Gen.sized @@)?

yes

If so, would it make sense to unify the last two, so that any kind of recursive variant needing fuel (mutually recursive or not) always derives a pair of generators _sized and _gen?

Yes it is! I did not want to break to much code regarding this particular fix. However, if we agree on the fix I can do this as well :) I will ping you when it's done.

[vch9]

Done @jmid

---

The rule is now:

• If the type t is recursive, you have:
  • a sized generator: gen_sized : n -> t Gen.t
  • a generator (using Gen.sized): gen : t Gen.t`
• If the the type t is mutually recursive, for each t' in t, you have:
  • a sized generator (even if the t' is not recursive)
  • a generator (using Gen.sized) for t'

The idea behind the second point is: share the size indicator in each sub-type.

---

There was also a fix in these commits:

type 'a tree = Leaf of 'a | Node of 'a tree * 'a tree
type int_tree = int tree

(* generator for int_tree before the fix: *)
let gen_int_tree = gen_tree

(* generator for int_tree after the fix: *)
let gen_int_tree gen_tree QCheck.Gen.int

The parameters were ignored.

[jmid]

Could I ask you to rebase? (it would be nice to have a green light from CI before merging, now that it's working again) 🙏‍

[vch9]

If the CI pass and you agree on the generated code, I think we can merge this @jmid.
However, I'm not fully confident with the code I wrote, I will soon write unit tests (and/or PBTs) to test generated generators.

[vch9]

If the CI pass and you agree on the generated code, I think we can merge this @jmid. However, I'm not fully confident with the code I wrote, I will soon write unit tests (and/or PBTs) to test generated generators.

I quickly found bugs writing these tests. They are not yet related to this specific PR I let you decide whether I should add those tests in this PR, or if we merge and I come up with a follow-up PR.

(I first wrote the deriver for QCheck.arbitrary, I then switched to QCheck.Gen.t, some generators are named differently and I did not pay attention to this)

[jmid]

I quickly found bugs writing these tests. They are not yet related to this specific PR I let you decide whether I should add those tests in this PR, or if we merge and I come up with a follow-up PR.

(I first wrote the deriver for QCheck.arbitrary, I then switched to QCheck.Gen.t, some generators are named differently and I did not pay attention to this)

OK, makes sense. It probably makes most sense to submit those tests (and fixes) as a separate PR.

First: Sorry for leaving this hanging for some time... 😬

I took a quick look and found that there's now two copies of (what looks like) identical copies of is_type_rec:

qcheck/src/ppx_deriving_qcheck/ppx_deriving_qcheck.ml

Lines 43 to 47 in d0c0b1e

	let rec is_rec_typ typ_name = function
	| { ptyp_desc = Ptyp_constr ({ txt = x; _ }, _); _ } ->
	longident_to_str x = typ_name
	| { ptyp_desc = Ptyp_tuple xs; _ } -> List.exists (is_rec_typ typ_name) xs
	| _ -> false

qcheck/src/ppx_deriving_qcheck/ppx_deriving_qcheck.ml

Lines 461 to 465 in d0c0b1e

	let rec is_rec_typ typ_name = function
	| { ptyp_desc = Ptyp_constr ({ txt = x; _ }, _); _ } ->
	longident_to_str x = typ_name
	| { ptyp_desc = Ptyp_tuple xs; _ } -> List.exists (is_rec_typ typ_name) xs
	| _ -> false

It indicates that it would be worth it to take an extra clean-up pass over the code.

Unrelated: The comments with n / 1 look like typos to me:
https://github.com/vch9/qcheck/blob/d0c0b1eea65cd68acde284928d47597aab2d85a4/src/ppx_deriving_qcheck/ppx_deriving_qcheck.ml#L25

I was a bit confused by the order of the code: First some basic helpers, some sized_gen, some tuple+record, ... then a group of gen_from_ where sized_gen is used almost at the end. It could be nice to restructure it a bit to help the reader and add a few comments (e.g., about how you break all tuples down to 2-3-4 combinations).

There's already a tests for recursive types (tree, expr), for two-level (expr-value), and truly mutually recursive types (tree-forest), so you've done a great effort there already. If you feel like putting the plugin to the test, I just remembered this one
https://github.com/yomimono/ocaml-test-omp/blob/d086037027537ba4e23ce027766187979c85aa3d/test/parsetree_405.ml
which I think could make a solid stress test of the mutual recursion code! 😬 😉

Finally: I'm super happy with this deriving plugin (I started using it myself to avoid writing this boilerplate) - so thanks for your effort! 🙏‍

[c-cube]

I haven't looked, but thank you @vch9 for your hard work, and @jmid for the reviewing!

question: does this use ppxlib's mechanism in such a way that [@@deriving_inline ] works? That'd also be super cool :)

[vch9]

I took a quick look and found that there's now two copies of (what looks like) identical copies of is_type_rec:

It does not surprise me, my rebase was suspicious.

It indicates that it would be worth it to take an extra clean-up pass over the code.

and, yes!

I was a bit confused by the order of the code: First some basic helpers, some sized_gen, some tuple+record, ... then a group of gen_from_ where sized_gen is used almost at the end. It could be nice to restructure it a bit to help the reader and add a few comments (e.g., about how you break all tuples down to 2-3-4 combinations).

Yes it is not ideal, I will restructure the code before this MR's changes, with more documentation obviously :). Even though, the commits in this MR are not really great: I introduced a signifcant change in d1edc6f, which I generalize in a7cb9b6 and lead to several removals from previous commit. I think with the testing suite we have for the deriver, it could be a good thing to restart from scratch to produce a more sustainable code (I had trouble to read my own code during the rebase).

If you feel like putting the plugin to the test, I just remembered this one
https://github.com/yomimono/ocaml-test-omp/blob/d086037027537ba4e23ce027766187979c85aa3d/test/parsetree_405.ml
which I think could make a solid stress test of the mutual recursion code! grimacing wink

I'll have a look, sounds like a solid test

[vch9]

question: does this use ppxlib's mechanism in such a way that [@@deriving_inline ] works? That'd also be super cool :)

I have not used [@@deriving_inline] yet in my life, but this is something I would drastically need, I'll have a look.

[vch9]

@jmid This should be ready for a second round.

I introduced several commits to move/comments code which improve I believe the readability (I'm open to other suggestions as well). In particular src/ppx_deriving_qcheck/qcheck_generators.ml should facilitate #190.

[vch9]

The imperative can easily be removed I believe, I thought using side effects of our loved language would simplify the code. This can be changed in this PR, or whenever this cause an issue. The environment is calculated at the beginning and can be passed as a parameters in every gen_* functions rather than using an imperative reference.

I promised myself not to insist on this - merely only point it out sweat_smile The curr_type () relies on the underlying ref option not being None though (which is how every Null-pointer bug in C/Java/... starts). @c-cube What do you think?

what's the context? if it's global mutable state then it's kind of bad, indeed. Is it for mutually recursive functions?

It was a global mutable state to remember recursive functions and facilitate at several locations whether the type was recursive or not. However, I removed the global state for a variable moved around in f6d55c6.

[vch9]

Remove the (kind ppx_rewriter) and added ppx_deriving as a dependency.

[jmid]

curr_type = "" surprised me a bit at first.

In a sense it moves the "absence of curr_type representation from None to "".

However, we no longer have a ref - and here the absence is only local to derive_gen.
Any reader can thus convince him/her/themself that curr_type will be updated to be
non-empty just a few lines below (l.465) before being used.

I therefore think it is fine 👍‍ (and definitely better than the ref!)

[jmid]

@jmid you should be happy with f6d55c6! I removed references, this lead to the removal of several functions that could have been inlined :p

I know this is intended as a tongue-in-cheek remark 😅 ...but just to be clear:
I am all for helper functions ... when they help readability of the code.
I suppose I also prefer that they match entities in the domain (here: longindent, env, basic types, type constructors, ...)

[jmid]

Just to repeat: I think the outcome of these iterations is clearer, simpler, and shorter.
Thanks again for a great effort! 🙏‍

[vch9]

Just to repeat: I think the outcome of these iterations is clearer, simpler, and shorter.

I also believe that, do not worry :)

[jmid]

Great. I think this shaped out very nicely - thanks for your effort! 😀

I'll let the CI go green before merging (which also gives @c-cube a brief window to comment/object/celebrate...)

[vch9]

Rebased the history - thanks a lot for the review!

[vch9]

If the CI pass and you agree on the generated code, I think we can merge this @jmid. However, I'm not fully confident with the code I wrote, I will soon write unit tests (and/or PBTs) to test generated generators.

I quickly found bugs writing these tests. They are not yet related to this specific PR I let you decide whether I should add those tests in this PR, or if we merge and I come up with a follow-up PR.

(I first wrote the deriver for QCheck.arbitrary, I then switched to QCheck.Gen.t, some generators are named differently and I did not pay attention to this)

Follow-up PR with those tests: #208