Bump AST to OCaml 5.2.0

[patricoferris]

This is a draft PR to bump the AST of ppxlib to OCaml's 5.2 AST. The changes to the internal representation of functions are the main challenge, regardless of what we do we will need to patch quite a few ppxes that directly pattern-match on the constructors of the AST for functions.

There are a lot of changes to the pretty-printing too which I copied mostly from upstream -- it is unclear to me if this file is supposed to be copied across or updated with ppxlib specific changes ? @NathanReb or @pitag-ha maybe you could shed some light here. This is currently breaking this PR on older compiler versions but I'm not 100% sure what is causing that or where the check is ?

[patricoferris]

Some code is not round-tripping here and I think it might be caused by (although I could be wrong) function arity issues. See the 4.12.1 build in the CI (there are other issues too I know). But it seems something like

type t = { x : int; y : int }
let f = fun z -> fun { x; y } -> x + y + z

and this roundtrip to

let f = fun z { x; y } -> x + y + z

Though I thought the latter is just syntactic sugar for the former, but then I haven't read the new arity change PR in full yet!

The actual code causing this is:

method include_infos :
  'a . ('a -> 'a) -> 'a include_infos -> 'a include_infos=
  fun _a ->
    fun { pincl_mod; pincl_loc; pincl_attributes } ->
    let pincl_mod = _a pincl_mod in
    let pincl_loc = self#location pincl_loc in
    let pincl_attributes = self#attributes pincl_attributes in
    { pincl_mod; pincl_loc; pincl_attributes }

Although it is unclear given the diff presented. Would it make more sense to write a structural equality checker ourselves that could output the problematic AST nodes when they don't compare correctly rather than using Poly.( <> ) + s-expression diff?

[NathanReb]

Having a proper comparison function for this and a proper printer would be great yes. I guess the printer is a bit more important in the short term as it would help us solve this particular issue.

We can probably work out a decent enough printer using Ast_traverse.lift.

[NathanReb]

From a quick look I think it's the other way around, our migrations are bugged in a way that will turn fun x y -> x + y into fun x -> fun y -> x + y.

I'm looking into testing and fixing this!

[patricoferris]

Another tricky corner case that has appeared during the 5.2 AST bump is that local opens in types make it very hard (perhaps impossible) to know if a type is recursive or not using really_recursive/type_is_recursive.

module M = struct
  type t = A
end

type t = M.(t)

When asked if type t = M.(t) is recursive or not are tests say yes. Of course, it could be the case that it actually is recursive. We just don't know without going to look at the module and working out if there's actually a t in there if it is or not.

Of course this was already the case with global opens too I suppose -- just thought I'd add that here.

[NathanReb]

This looks great!

I think I need a bit of context on a few changes to better understand them but overall this looks solid.

[NathanReb]

What happens when we're generating patterns to match over anonymous functions?

E.g. what will this generate:

match expr with
| [%expr fun x y -> z] -> ...

If I understood the expression counterpart correctly, we generate Ppxlib.Ast_builder.Default.coalesce_arity node instead of node right?

[patricoferris]

See the comment here: #514 (comment)

[NathanReb]

I believe this works because this triggers multiple calls to coalesce_arity, one for each [%expr ...].

Is there a way we could add a test for a single metaquot invocation? E.g. would [%expr fun x -> fun y -> fun z -> ...] raise the same result?

[patricoferris]

Hmm yeah we're going to run into trouble here. There are two distinct cases:

OCaml 5.1 or less

AST migration creates max arity functions for us regardless of how we build the function. Because we don't migrate downwards, the function remains max arity. There's not much we can do about this behaviour as we have no way to distinguish between [%expr fun x y -> x + y] and [%expr fun x -> fun y -> x + y]. So, no matter what, in this case we will get a maximum arity function.

OCaml 5.2 and above

It is possible for us to create functions that are not max arity and the AST migrations will not take care of this. Something like [%expr fun x -> fun y -> x + y] will not be max arity on it's own. In the 5.2+ parsetree this will be two single argument functions. In the case where this is used as an expression, metaquot will insert calls to coalesce_arity between each function. This is because the lift object in metaquot applies coalesce to every expression.

For expressions in patterns, we do not apply the function allowing users to distinguish between the two representations:

let f v = match v with
  | [%expr fun x -> fun y -> x + y] -> ()
  | [%expr fun x y -> x + y] -> ()
  | _ -> ()

However, I'm realising that on OCaml 5.1 or less, the migration will make these two cases identical and perhaps we should coalesce in this case to make the behaviour the same on all compilers? If someone wishes to distinguish between the two representations then they will have to use constructors directly?

[patricoferris]

See this test: 908a820

[NathanReb]

Yes, I agree it's a bit of tough case here but I'm slightly leaning towards having a unified behaviour here as it can otherwise produce semantically different code depending on the version of the compiler.

I think if one were to pattern-match over arity on functions with metaquot, they could still do something like this:

match expr with
| [%expr fun [%p? arg] -> [%e? body]] -> ...

in a recursive manner.

[NathanReb]

What do you think?