-
Notifications
You must be signed in to change notification settings - Fork 100
Support OCaml 5.4.0 #570
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
base: main
Are you sure you want to change the base?
Support OCaml 5.4.0 #570
Conversation
There are a few missing promotions it seems! |
I think we're just waiting for the magic numbers to be bumped in OCaml. |
I thought they were bumped on the development branch now. It also seems the dune version we're using for the trunk CI build does not build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for taking the time to work on this.
From the top of your head, do you know which functions in the migration need to be reviewed?
astlib/longident.ml
Outdated
type t = (*IF_AT_LEAST 504 Ocaml_common.Longident.t = *) | ||
Lident of string | ||
| Ldot of t loc * string loc | ||
| Lapply of t loc * t loc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a Longident
module in ast_504
similar to what we have for Asttypes
. That way if the type is further modified, it'll be treated as a regular AST change.
I think we should still have a Longident
module in astlib
but it should only provide the utility functions on Longidents from the compiler, same as the Parse
, or Pprintast
modules do for other moving Ast types.
Since Astlib is supposed to provide the stable layer on top of the compiler-libs that's necessary for ppxlib, we should maybe make it provide the exact set of functions we need and nothing more.
What would you think about the following:
- Make Legacy_longident define only the type
- Make
Longident
provide the functions we use, conditionnally compiled to work with the current compiler - Have a
Longident
module inPpxlib
that expose the functions corresponding to our internal AST.
As I write this I wonder if, in fact, we don't need any compiler functions on longidents but simply need the ones for our internal AST. Making a compat layer for functions unnecessary as we can simply have a local copy of the correct version.
the magic numbers have been bumped |
Thanks for the review @NathanReb -- I think I followed what you were suggesting and applied the changes. To reiterate:
And everything just works. Presumably when we bump the internal AST to 504 we will have to almost invert this in order to migrate from below to 504. |
Also, we're waiting on a new version of dune that support 5.4.0 too. |
the dune PR has been merged in opam-repository |
This also updates the easier parts of ppxlib to be compatible with 5.4.0. We will need to version the Location module. Signed-off-by: Patrick Ferris <[email protected]>
Adds support for the OCaml 5.4, mainly versioning the Longident module and handling migrations of tuples with and without labels. Signed-off-by: Patrick Ferris <[email protected]>
Signed-off-by: Patrick Ferris <[email protected]>
Signed-off-by: Patrick Ferris <[email protected]>
Signed-off-by: Patrick Ferris <[email protected]>
We now have a dune 3.18.0 with 5.4.0 support (afaict). See the conversation in #566