-
Notifications
You must be signed in to change notification settings - Fork 98
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
base: main
Are you sure you want to change the base?
Conversation
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 |
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 |
From a quick look I think it's the other way around, our migrations are bugged in a way that will turn I'm looking into testing and fixing this! |
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 module M = struct
type t = A
end
type t = M.(t) When asked if Of course this was already the case with global opens too I suppose -- just thought I'd add that here. |
6871aac
to
f16b0d3
Compare
6fdbb76
to
d22f931
Compare
4430330
to
e9c9c83
Compare
Signed-off-by: Nathan Rebours <[email protected]>
Signed-off-by: Nathan Rebours <[email protected]>
With -pp-simple drivers can print a simplified AST. This is useful for debugging purposes. It is similar to the ppxlib-pp-ast tool packaged with ppx-tools, but it is not configurable. Signed-off-by: Patrick Ferris <[email protected]>
Co-authored-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]>
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]>
Signed-off-by: Patrick Ferris <[email protected]>
Signed-off-by: Patrick Ferris <[email protected]>
7f8a540
to
575d335
Compare
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 ?