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

Replace camlp5 with ppx_parser #11860

Merged
merged 6 commits into from
Dec 11, 2024
Merged

Conversation

tobil4sk
Copy link
Member

@tobil4sk tobil4sk commented Dec 7, 2024

We've had issues with camlp5 on windows as it doesn't get much maintenance anymore (and still relies on Unix configure/makefiles rather than dune).

This PR is an attempt to port our parser to ppx_parser, which is a reimplementation of camlp-stream parsers with ppxlib preprocessors which are much better maintained. It was missing a couple features but I've manged to upstream these:

We also need this patch to allow it to install with ocaml 4.08, so for now I've set it to build with my fork:

ppx_parser is based on camlp4 which, unlike camlp5, did not perform automatic left-factorization. We've avoided this problem with manual refactoring: #11859.

Another minor difference is that when clauses are not supported within rules, only outside of them, so the clause can only include variables bound in the first element of the rule. This is not a big issue after the refactoring that was already required.

Other differences are purely syntactic.

@Simn
Copy link
Member

Simn commented Dec 9, 2024

@yuxiaomao Could you check if this works with your codebases? And if it does, could you check the --times before/after to make sure we don't have a parsing performance regression?

@yuxiaomao
Copy link
Contributor

yuxiaomao commented Dec 9, 2024

All projects compile (except one but it's already failing with development), and I can't notice significant performance change compared to development 5174e89 .
(For some projects I think development has slightly worse performance in parsing compared to 7f3dd53 , but that's out of the scope of this PR)

@yuxiaomao
Copy link
Contributor

Note that when testing the compilation with one project (without compilation server), the first compilation is significant slower than usual (+50% total time, from 30s to 45s). Can't reproduce and difficult to say if it's related to this PR or not x)

@skial skial mentioned this pull request Dec 9, 2024
1 task
@Simn
Copy link
Member

Simn commented Dec 10, 2024

I'm trying to do my local setup for this and will log everything I come across. With ppx_parser 0.1.1 installed I'm currently running into this error:

File "src/syntax/grammar.ml", lines 47-49, characters 8-14:
47 | ........match%parser s with
48 |            | [ (sep2,_); [%let l = psep_trailing sep f] ] when sep2 = sep -> v :: l
49 |            | [ ] -> [v]
Error: function expected

Edit: Right, as Yuxiao had already told me I need to run opam pin ppx_parser https://github.com/tobil4sk/ppx_parser.git#relax-ocaml-constraint for the time being. With that being done compilation just works!

@Simn
Copy link
Member

Simn commented Dec 10, 2024

@kLabz Could you check if this works for you as well?

I'm pretty eager to merge this quickly, which may or may not be because I nuked my local camlp5 setup and now can't compile Haxe on any other branch anymore...

@Simn Simn marked this pull request as ready for review December 10, 2024 06:07
@kLabz
Copy link
Contributor

kLabz commented Dec 10, 2024

Can do that in 1~2 hours

@Simn
Copy link
Member

Simn commented Dec 10, 2024

I've removed most of the [%s s] because I don't like how this looks now. There are 23 occurrences left, maybe we should just go all the way and get rid of them. It's easy to do by changing the relevant functions from and name args = function%parser to and name args s = match%parser s with.

haxe.opam Show resolved Hide resolved
@Simn
Copy link
Member

Simn commented Dec 11, 2024

I'll go ahead and merge this now so that we can move forward. There might be some adjustments to make, but we can always do that later.

Thank you SO MUCH for looking into this, getting rid of camlp5 has been on my bucket list for a while now.

@Simn Simn merged commit a9d7988 into HaxeFoundation:development Dec 11, 2024
50 checks passed
@tobil4sk tobil4sk deleted the replace-camlp5 branch December 11, 2024 09:17
@RblSb
Copy link
Member

RblSb commented Dec 11, 2024

pin-depends should be updated to use base git repository (not a fork) instead of current removed PR branch

@kLabz
Copy link
Contributor

kLabz commented Dec 11, 2024

that has been done already

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.

5 participants