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

Prepare for 5.2 AST in Ppxlib #160

Merged
merged 4 commits into from
Mar 21, 2025
Merged

Conversation

patricoferris
Copy link
Contributor

This PR is a patch for an upcoming release of ppxlib where the internal AST is bumped from 4.14 to 5.2. Until that is merged and released, there is no reason to merge this PR.

The most notable change is the representation of functions in the AST.

Functions


Currently

In the parsetree currently, functions like:

fun x y z -> ...

Are represented roughly as

Pexp_fun(x, Pexp_fun (y, Pexp_fun(z, ...)))

Functions like:

function A -> ... | B -> ...

Are represented roughly as

Pexp_function ([ case A; case B ])

Since 5.2

All of these functions now map to a single AST node Pexp_function (note, this is the same name as the old cases function). The first argument is a list of parameters meaning:

fun x y z -> ...

Now looks like:

Pexp_function([x; y; z], _constraint, body)

And the body is where we can either have more expressions (Pfunction_body _) or cases (Pfunction_cases _). That means:

function A -> ... | B -> ...

Has an empty list of parameters:

Pexp_function([], _, Pfunction_cases ([case A; case B]))

Local Module Opens for Types


Another feature added in 5.2 was the ability to locally open modules in type definitions.

module M = struct
  type t = A | B | C
end

type t = Local_open_coming of M.(t)

This has a Ptyp_open (module_identifier, core_type) AST node. Just like normal module opens this does create some syntactic ambiguity about where things come from inside the parentheses. The approach of these patches is to locally open the same module in any code that is generated.

This PR assumes ppxlib.0.35.0 will be the ppxlib that contains the 5.2 AST bump. ppxlib.0.34.0 will likely be the stable release of ppxlib with 5.3 support. This definitely might change, and the PR should be updated accordingly.

@NathanReb
Copy link
Collaborator

Thanks! I'll merge and release as soon as the deps problem is fixed. Seems we need a few ppxlib.0.36 compatible packages before this one's able to build!

@NathanReb
Copy link
Collaborator

Could you add a pin-depends on ppx_deriving.6.1.0 ? That way we can make the CI happy and merge this, I'll update the opam file and release once ocaml/opam-repository#27621 is merged!

@NathanReb
Copy link
Collaborator

It just got merged so that won't be necessary!

@NathanReb
Copy link
Collaborator

Just added a changelog entry, this should also trigger a new build which will hopefully pick ppx_deriving.6.1.0.

@NathanReb
Copy link
Collaborator

It seems we still break the Ptyp_poly invariant somewhere!

@patricoferris
Copy link
Contributor Author

Darn it, it seems our own ast_helper function is biting us here. Maybe we can change it to:

  let poly ?loc ?attrs a b = 
    if a = [] then b
    else mk ?loc ?attrs (Ptyp_poly (a, b))

I know we're also floating the idea of getting rid of ast_helper altogether though and perhaps this is a good opportunity to start doing that!

@NathanReb
Copy link
Collaborator

I'm fixing it as we speak, I'm replacing Typ.poly with Ast_builder.Default.ptyp_poly

@NathanReb
Copy link
Collaborator

Yes, we should remove Ast_helper entirely at some point! Let's start deprecating it.

I just realized it even has the following function:

  let force_poly t =
    match t.ptyp_desc with Ptyp_poly _ -> t | _ -> poly ~loc:t.ptyp_loc [] t

and that's exposed...

@NathanReb
Copy link
Collaborator

We both needed a rebase AND to fix a few extra ptyp_poly ([], ...) that I somehow missed in #162 !

@patricoferris
Copy link
Contributor Author

I see there are a few more calls to Typ.poly from Ast_helper, and whilst these are not as bad as the force_poly function, perhaps we should also be changing these ?

@NathanReb
Copy link
Collaborator

Are you sure? I can't find anymore of those with git grep.

@NathanReb NathanReb merged commit 0a25740 into ocaml-ppx:master Mar 21, 2025
1 check passed
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