Skip to content
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

Bump AST to OCaml 5.2.0 #514

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

patricoferris
Copy link
Collaborator

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
Copy link
Collaborator Author

patricoferris commented Jul 29, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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 and others added 8 commits September 14, 2024 10:29
Printing to source code and parsing with the compiler before
running the upward migration is expected to fail because on compilers
older than 5.2, `fun x y -> z` and `fun x -> fun y -> z` parse to
the same AST.

We therefore need to first migrate it to the compiler version before
printing it.

Signed-off-by: Nathan Rebours <[email protected]>
Signed-off-by: Patrick Ferris <[email protected]>
Signed-off-by: Patrick Ferris <[email protected]>
Signed-off-by: Patrick Ferris <[email protected]>
This also changes some internals of ppx_traverse to build
a maximum arity function rather than single arity.

Signed-off-by: Patrick Ferris <[email protected]>
Signed-off-by: Patrick Ferris <[email protected]>
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.

2 participants