trunk-support supports trunk

[panglesd]

This PR is unfinished (locations are wrong, migration is untested, migrations don't roundtrip...), but should hopefully allow users to compile ppxlib against trunk.
Note that you will need to pin ocaml-compiler-libs to [email protected]:art-w/ocaml-compiler-libs.git#ocaml-5.2-trunk since the dependency is not compatible with 5.02.

---

Ocaml pretty recently added two parsetree changes in its parsetree:

• Local open in types ocaml/ocaml#12044
• Syntactic function arity ocaml/ocaml#12236

Local open types

I migrated local open in types as we migrate local open in patterns (4.04 -> 4.03): by raising an error. This means that any code using local open in types anywhere will be unable to use ppxlib, until ppxlib bumps its AST to 5.02. This will require a lot of work, since I suspect many PPXes directly pattern-match/create function nodes.

Syntactic arities

For syntactic arities, it seems that there are some problems. Let's collect any issue found when implementing the migrations in this thread (I'll update the list and add more information as we make progress).

For 5.02 -> 5.01:

• some locations (for "subfunctions") are missing. We can use the "whole location" maybe. Similarly forpexp_loc_stack.
• We need to check to which node to attach the attributes to.

For 5.01 -> 5.02:

• I have only done the migration "naively", without using the new feature where a function has syntactic arity greater than 1.
• Some location information might also be missing? (Need investigation)
• Doing it properly requires some focus to do it right!

[hhugo]

I was able to use this with trunk and run most of the jsoo test-suite.
The naive 5.01 -> 5.02 migration is responsible for one breakage in the test-suite.

[pitag-ha]

Thanks, @panglesd, for this PR and all your work on trunk support!

Would it sound good to you to separate the two changes? I'd merge the support for local open types as is: It's a new syntax feature represented by a new node and your migrations do what expected.

The syntactic arity support seems a lot more complex. We'll have trouble, both getting the migrations semantically as right as possible and updating all the PPXs it'll break once we bump the AST. First things first: To get the migrations right, we should add tests. Btw, @hhugo could you point us to the jsoo test that breaks? Once the migrations are isolated and with tests, let's ask the author of the compiler PR @ncik-roberts if they'd be willing to give us a hand, similarly to how Octachron helped with the support of value binding type constraints. By the end of the day, the original author understands the change the best.

[ncik-roberts]

Yes, when you are in that state please let me know. I hope I can be helpful, both with how to proceed with the OCaml 5.2 release and for a good long-term API for ppxlib here.

[panglesd]

I opened this PR quickly, to allow for compilation on trunk of (hopefully most) programs that depend on ppxlib. I did not add test or anything because I just did it quickly and it was not finished.

We have made some progress on migrations with @Julow! But the changes are not pushed yet, since it's WIP.

About local open types: I think we can do better. This naive migration will get us unable to use ppxlib in conjunction with local open types. We can use an attribute with a carefully crafted payload to remember the local open type, and put it back in the upward migration.

About syntactic arity, we are also using attributes to distinguish between fun x -> fun y -> 1 and fun x y -> 1.

I will:

• finish the migrations with @Julow
• write the tests
• separate in two PRs

Hopefuly soon!

[pitag-ha]

Re local open types: IMO, it's not necessary to spend time improving those migrations. With your current migrations, the lack of local open types support will only be there before we bump the AST to 5.2. That will be at the end of the 5.2 RC phase / beginning of 5.2 stable release. Before getting into those release phases, people tend to only test and benchmark trunk on existing code. So before those release phases, it's important that we make Ppxlib compatible with 5.2 for existing code, but we don't need to support its new syntax features. Once the new compiler is stably released, we bump the AST and people will be able to use the new compiler features -including local open types- in conjunction with Ppxlib. Let me know if you have a different opinion, @panglesd.

[hhugo]

Btw, @hhugo could you point us to the jsoo test that breaks?

The broken tests all live in https://github.com/ocsigen/js_of_ocaml/blob/master/lib/tests/test_fun_call.ml
It checks the arity of functions at runtime in javascript code generated by jsoo.

Here is the failure reported by the CI https://github.com/ocsigen/js_of_ocaml/actions/runs/6326376239/job/17179814876?pr=1487

[panglesd]

@hhugo could you try again with this branch? I just pushed the work we did with @Julow.

It needs testing (we haven't added tests yet) but if everything goes well, arity should be better preserved than in the previous naive version.

[hhugo]

@hhugo could you try again with this branch? I just pushed the work we did with @Julow.

It needs testing (we haven't added tests yet) but if everything goes well, arity should be better preserved than in the previous naive version.

The latest version of this PR fixes the issue. Thanks

[hhugo]

ocaml/ocaml#12639 broke support for trunk again

[pitag-ha]

See #449 (comment).

[panglesd]

I cherry picked a commit from @hhugo, and then made the trunk CI be run again (by pinning ocaml-compiler-libs).

I planned to merge but the tests failed with some cryptic:

$ cat binary_ast | ../identity_driver.exe -impl -
-  let b = 2
+  File "_none_", line 1:
+  Error: Internal error: invalid [@@ocaml.ppx.context { load_path }] pair syntax
+  [1]

I wanted to investigate the issue but again, got busy on other things. So in the light of #449 (comment), either I have time to investigate today, or I'll merge this evening and let users of the branch fix errors they might encounter (or fix it myself later, but at least this PR is merged).

[hhugo]

After a very quick look:

• The test failure was caused by Add a -H flag, second attempt ocaml/ocaml#12246
• updating test/driver/stdin_input/test.t/binary_ast using a trunk should fix the issue.
• I think we can't expect ppxlib to be able to read old marshalled ast.

[panglesd]

If I remember correctly, there is some alternative toolchain (related to reason) that requires ppxlib to read an AST built with a different version than what ppxlib was built with, so we might want to do something here.

I think @pitag-ha explains that here: #409 (comment)

In the meantime, let's merge this.

[panglesd]

The test failure was caused by ocaml/ocaml#12246

Thanks for the link!