`QCheck2.Gen` design considerations

[jmid]

With QCheck2 still not yet released (and still being tested) we have a brief opportunity to discuss the design of its Gen combinators.

Optional parameters

We have had problems with optional parameters in the past (see #75). These may cause problems when they are the only parameters. For QCheck this could usually be solved with type annotations:

utop # QCheck.Gen.small_string;;
- : ?gen:char QCheck.Gen.t -> string QCheck.Gen.t = <fun>
utop # QCheck.Gen.([small_string; string]);;
- : (?gen:char QCheck.Gen.t -> string QCheck.Gen.t) list = [<fun>; <fun>]
utop # (QCheck.Gen.([small_string; string]) : string QCheck.Gen.t list);;
- : string QCheck.Gen.t list = [<fun>; <fun>]
utop # QCheck.make QCheck.Gen.small_string;;
- : string QCheck.arbitrary =
{QCheck.gen = <fun>; print = None; small = None; shrink = None; collect = None;
 stats = []}

For Check2 however because QCheck2.Gen.t is opaque, even with the fix in PR #161, the type annotation trick doesn't work anymore:

utop # QCheck2.Gen.small_string;;
- : ?gen:char Gen.t -> string Gen.t = <fun>
utop # QCheck2.Gen.([small_string; string]);;
Error: This expression has type string t but an expression was expected of type
         ?gen:char t -> string t
utop #  (QCheck2.Gen.([small_string; string]) : string QCheck2.Gen.t list);;
Error: This expression has type ?gen:char t -> string t
       but an expression was expected of type string t
utop # QCheck2.Test.make QCheck2.Gen.small_string;;
Error: This expression has type ?gen:char Gen.t -> string Gen.t
       but an expression was expected of type 'a Gen.t

Effectively, this means the (labeled) parameter is required, not optional. As such, we should seriously consider changing QCheck2.Gen.small_string to one of the following:

  val small_string : string t                   (* just use char *)
  val small_string : char t -> string t         (* make it a regular argument *) 
  val small_string : gen:char t -> string t     (* make it an actual labelled argument *)
  ...

Of the three above, I would prefer one of the first two. Based on our choice, we should consider whether to update string_size accordingly following the principle of least surprise:

 val string_size : ?gen:char t -> int t -> string t

Looking at the Gen module's signature I can see a similar problem with Gen.pint : ?origin:int -> int t which we also need to address:

utop # QCheck2.Test.make QCheck2.Gen.pint;;
Error: This expression has type ?origin:int -> int Gen.t
       but an expression was expected of type 'a Gen.t

Here, we probably should go with a regular or labelled argument. Again, from the principle of least surprise, we should consider updating the remaining origin-taking combinators to take the same kind of origin argument:

  val int_range : ?origin:int -> int -> int -> int t
  val float_bound_inclusive : ?origin:float -> float -> float t
  val float_bound_exclusive : ?origin:float -> float -> float t
  val float_range : ?origin:float -> float -> float -> float t
  val char_range : ?origin:char -> char -> char -> char t

Besides theGen.generate* bindings, only two bindings now stand a bit out:

  val opt : ?ratio:float -> 'a t -> 'a option t
  val make_primitive : gen:(Random.State.t -> 'a) -> shrink:('a -> 'a Seq.t) -> 'a t

I think for opt the use of an optional parameter is warranted. For make_primitive we may consider whether labelled arguments are needed.

Generator names:

Naming-wise, we should consider the consistency. Here are the string generators:

  val string_size : ?gen:char t -> int t -> string t
  val string : string t
  val string_of : char t -> string t
  val string_readable : string t
  val small_string : ?gen:char t -> string t

Wouldn't string_small be nice and consistent - and easier to find using tab-completion?

I think the same goes for the list generators (small_list -> list_small):

  val list : 'a t -> 'a list t
  val small_list : 'a t -> 'a list t
  val list_size : int t -> 'a t -> 'a list t
  val list_repeat : int -> 'a t -> 'a list t

and the array generators (small_array -> array_small):

  val array : 'a t -> 'a array t
  val array_size : int t -> 'a t -> 'a array t
  val small_array : 'a t -> 'a array t
  val array_repeat : int -> 'a t -> 'a array t

For int*, float, and char generators it is harder...

Use of underscore in name suffixes

I spotted an unfortunate variation in the use of _l, _a or l, a suffixes:

  val oneof : 'a t list -> 'a t
  val oneofl : 'a list -> 'a t
  val oneofa : 'a array -> 'a t
  val frequency : (int * 'a t) list -> 'a t
  val frequencyl : (int * 'a) list -> 'a t
  val frequencya : (int * 'a) array -> 'a t
  val shuffle_a : 'a array -> 'a array t
  val shuffle_l : 'a list -> 'a list t
  val shuffle_w_l : (int * 'a) list -> 'a list t
  ...
  val flatten_l : 'a t list -> 'a list t
  val flatten_a : 'a t array -> 'a array t
  val flatten_opt : 'a t option -> 'a option t
  val flatten_res : ('a t, 'e) result -> ('a, 'e) result t

I know a lot of this is legacy, but with a clean QCheck2 now is a good time to consider cleaning these things a bit up.

[jmid]

Ah, I forgot:

Additional char/string generators

Looking at another port (Hedgehog for F# I think) I spotted these signatures:

   val ascii   : char t
   val latin1  : char t

which would make for a nice and readable spec test:

  let t = Test.make Gen.(string_of ascii) (fun s -> ...)

From this code, neither its author or any readers should be in doubt of the kind of strings tested for!

[sir4ur0n]

Optional parameters

In my opinion, whenever it makes sense, we should provide a generator that does not take any additional input (make it dead-easy to get started on QCheck) so I'd go with val small_string : string t by default.

Then we should also provide another function that takes a mandatory - labeled or not, this depends if there are other arguments - generator.

I guess there's no choice but to provide this second one with a different name :shrug:

Generator names

Yes, yes, yes, please, the naming inconsistencies have driven me crazy many times in the past, and it remains a barrier for newcomers :pray:

• :100: for string_small rather than small_string
• Same for list_* and any other similar generator
• I would also do the same for primitive generators like int. E.g. prefix all int generators with int_. Maybe some names would become a bit longer, but at least everyone would know what to expect.

Use of underscore in name suffixes

I am all in on using the _l syntax rather than l (obviously, same thing for _a vs a).

Additional char/string generators

Sure, why not

[c-cube]

fwiw I'm all for new, more consistent names, especially in QCheck2, as long as old names are present in QCheck (possibly with a deprecation tag :slightly_smiling_face: )

[vch9]

I totally agree with you too (especially with the generator names). Also, should we remove the module Gen and access it directly with QCheck2.* just like we had with arbitrary? I remember we mentioned this once.

[favonia]

In addition to what has already been said, I propose the inclusion of small_string_of as follows:

val small_string : string t
val small_string_of : char t -> string t

(or with small and string swapped if that's desired).