PoC: support OCaml 5.3's keywords entry in OCAMLPARAM

[dra27]

This is a little proof-of-concept and request for feedback following a check in ocaml/opam-repository#26831 that the mechanism added in 5.3.0~beta1 in ocaml/ocaml#13471 is working. The effect (pun intended) is mildly limited for 5.3, but it seems worth checking these packages now because keywords changes in future may potentially have more impact, so it feels to me to be better to be ready with a "simpler" release.

The mechanism added allows old code (i.e. old releases of packages) to be compiled even if that code uses identifiers which have become keywords since it was written. This is done either by explicitly passing -keywords 5.2 to the compiler invocations or by setting OCAMLPARAM to _,keywords=5.2. Certainly for the effect keyword, it's generally expected that repositories will choose to switch the identifier (e.g. to effect_) rather than use the -keywords flag, but the mechanism means that opam packages can be made to work without requiring a release, which obviously reduces adoption friction for new versions of the compiler.

This works well for the small number of packages in 5.3 which use effect as an identfier, but it doesn't work for Irmin (cf. mirage/irmin#2346). The OCAMLPARAM trick doesn't work because the ppx standalone driver doesn't inspect OCAMLPARAM, so it's creating the "wrong" AST. This PR is a possible idea to allow the standalone driver (which is invoking the compiler driver, albeit via compiler-libs) to parse the keywords part of OCAMLPARAM.

This Dockerfile fails with OCaml 5.3:

FROM ocaml/opam:debian-12-ocaml-5.3
RUN git clone https://github.com/mirage/irmin.git
WORKDIR irmin
RUN git checkout 3.9.0
RUN sudo ln -f /usr/bin/opam-2.2 /usr/bin/opam
RUN opam install ./irmin.opam --deps-only
RUN opam install repr ppx_irmin mtime bheap
ENV OCAMLPARAM=_,keywords=5.2
RUN opam exec -- dune build -p irmin

giving:

(cd _build/default && .ppx/851ddbf37b8928057d14b915d5ceebb6/ppx.exe --lib Type --cookie 'library-name="irmin"' -o src/irmin/node_intf.pp.ml --impl src/irmin/node_intf.ml -corrected-suffix .ppx-corrected -diff-cmd - -dump-ast)
File "src/irmin/node_intf.ml", line 127, characters 7-13:
127 |   type effect := expected_depth:int -> node_key -> t option
             ^^^^^^
Error: Syntax error

but if this branch is instead pinned then it builds. I'm not necessarily advocating this being the exact mechanism - the only aim is that it should be possible to "patch" an existing compiler simply by setting an environment variable (this is the general intention of OCAMLPARAM and the very obscure $(ocamlc -where)/ocaml_compiler_internal_params file)

• Incompatibly added at the moment, making ppxlib 5.3+ only!
• Probably worth including a -keywords option equivalent to the compiler's (In which case, the _ part of OCAMLPARAM should arguably be parsed)?
• The splitting logic duplicates internal code from Compenv which, if wanted, should probably be exposed in compiler-libs instead
• I've probably violated the way Clflags should actually be accessed, but I was being a bit lazy trying to work out all the ocaml-compiler-libs stuff...!

[NathanReb]

This looks like a good approach. Fixing the compat with older compilers shouldn't be too hard as we can make the function a no-op on older compilers.

Since it uses compiler-libs directly, it should be moved to astlib. You can use the (* IF_AT_LEAST 503 <ocaml-code> *) syntax, similarly to what's done here: https://github.com/ocaml-ppx/ppxlib/blob/main/astlib/location.ml#L89.

I'm of course happy to take over this if you'd prefer to focus on other things, I assume you're quite busy with the 5.3 release closing in!

[dra27]

Ah, thanks for the pointer - it at least brings what I've written to the same state of CI as the main branch! Happy to hand this off or continue working on it - what do you think about having a -keywords flag?

[NathanReb]

I'm happy to add the flag! Can't hurt and it will be documented in the driver's help making this compat feature more easily discovered by ppx users.

[dra27]

Sounds good - I'm more than happy if you want to take this branch from here 🙂.

Adding a CLI flag just means that the fold_settings function wants to return whether the _ was encountered in the list in OCAMLPARAM before the last keywords= item (which means OCAMLPARAM takes priority) or after it (which means --keywords takes priority), then an obvious dance between choosing whether to set Clflags.keyword_edition from OCAMLPARAM or from args.

There's actually not as much as internal stuff from compiler-libs being duplicated as I'd first feared, because the OCaml driver actually validates the final value of keywords_editions (from either OCAMLPARAM or the command line) just before using it - so ppxlib gets that error handling "for free".

[NathanReb]

Sounds good, I'll take it from here. Might ping you for a final review if that's okay!

[NathanReb]

I think I got it right, @dra27 let me know what you think!

[patricoferris]

Thanks @dra27 and @NathanReb :)

[patricoferris]

The problem is we can't make a dep on compilerlibs?

[NathanReb]

I think the exposed version of this is a bit too high level and is used to parse the CLI args of a driver so it wouldn't work well for us. Maybe @dra27 can confirm?

[patricoferris]

Right, it might be good to ask upstream to expose some of the internals if downstream libraries like ppxlib have to depend on that behaviour. Having just seen the issues caused in #540 by copying pieces of the compiler's internals into ppxlib, I think a long-term strategy of forcing the compiler to expose these things and help with long term stability would be nice?

Not a blocker for this PR because it seems the pieces we need are not necessarily exposed (but maybe should be? for the 5.3 release?).

[NathanReb]

Yeah that's a good point! We should then at least move the bits we'd like exposed in the compiler to Astlib.

@dra27 do you think we could reasonably expect something like read_OCAMLPARAM, returning a pair of args bindings lists, to be exposed at some point in the future?

[NathanReb]

Or maybe even apply_keyword_edition directly!

[NathanReb]

CC @dra27 any thoughts on that?

[dra27]

It's definitely too late to be making compiler-libs changes for 5.3, but that obviously preclude doing that for 5.4.

I agree with the general concern at the duplication (I was concerned about it when hacking up the first version, too). However, it's worth remembering that it's extremely hard for OCaml itself to change the interpretation of the variable, so the details are very unlikely to change - i.e. we're duplicating a little bit of code which deals with a very non-internal detail of the compiler here.

[NathanReb]

Yeah that's a good point!

Since we can't reasonably get this into 5.3, I'd say this PR is good to go! What do you think @patricoferris ?

[patricoferris]

Yep! Good to go :))

[dra27]

Thank you!