irmin.type: improve pp_ty

[craigfe]

Resolves #958.

This changes the pretty-printer for Irmin types to more closely match OCaml types (e.g. (unit * int) option rather than Option (Pair (Prim Unit, Prim Int))). It adds functionality for pretty-printing the components of algebraic types: record types are pretty-printed like OCaml object types and variants are pretty-printed similarly to OCaml polymorphic variants.

The tricky case here is recursive types, which were previously broken (would cause stack overflow when pretty-printed). Handling this properly requires preserving the unroll function passed to the mu combinator for later use. The type can then be pretty-printed by unrolling once with a type alias:

type my_recursive_record = { head : int; tail : my_recursive_record option } [@@deriving irmin]

(* gets pretty-printed as: *)
(< head : int; tail : my_recursive_record option > as my_recursive_record)

For recursive or mutually recursive types that are not algebraic, there is no sensible name for the type alias, so we pick them in some deterministic order.

This change also fixes an existing 'bug' in which the mu and mu2 fixpoints were unfolded once more than necessary (by returning the unfolded form). This was observable when pretty-printing the type.

[samoht]

Nice!

[icristescu]

This is neat! sorry for taking so long to have a look at it.
I'm wondering is there is a reason to have ([ Empty | Node of ((tree * int * tree) as 'a) ] as tree) instead of ([ Empty | Node of node] as tree)?

[pascutto]

Looks good, thanks.
Can you add a changelog entry for this?

[craigfe]

This is neat! sorry for taking so long to have a look at it.
I'm wondering is there is a reason to have ([ Empty | Node of ((tree * int * tree) as 'a) ] as tree) instead of ([ Empty | Node of node] as tree)?

Your suggestion is certainly cleaner. I tried it, but decided against it for two reasons:

• in general it's not easy to come up with nice semantic names like node for the recursive type. I tried tracking "name context" such as field/case names in recursive calls, which would allow inserting node here. Sadly it doesn't work for all cases (i.e. for non-algebraic data-types), so I picked the simpler / more consistent implementation.

• in other cases the pretty-printer displays the entire type structure it knows about. Printing node here rather than (tree * int * tree) would be inconsistent. Not particularly a problem, but at some point we might want to be able to deserialise type representations too.

The as 'a turns out not to be necessary here, which we could detect (but I'm not sure it's worthwhile).

[craigfe]

Can you add a changelog entry for this?

Done.

[liautaud]

Does embedding self_unroll into every recursive variant or record add any noticeable overhead? If I understand correctly, this new field is required to avoid stack overflows when pretty-printing, but does it also help outside of pretty-printing?

[craigfe]

Does embedding self_unroll into every recursive variant or record add any noticeable overhead?

I haven't measured, but I expect not. I imagine that the number of self values in most programs would be negligible.

If I understand correctly, this new field is required to avoid stack overflows when pretty-printing, but does it also help outside of pretty-printing?

It's currently just for pretty-printing.

If we ever have other cases where we recurse over the structure of representations without a corresponding value – such as if we ever support ∀ a. a Type.t -> a Type.t Type.t – self_unroll will be necessary to avoid infinite recursion.