ocaml-ppx / ppx_tools

Tools for authors of ppx rewriters
MIT License
134 stars 39 forks source link

Compatibility module for Parsetree.pattern_desc. #54

Closed aantron closed 6 years ago

aantron commented 7 years ago

OCaml 4.04 adds the Ppat_open constructor. This makes code that pattern matches on pattern_desc not portable between 4.04 and 4.03.

This is the 4.04 implementation of the compatibility module.

Closes #53.

See https://github.com/aantron/bisect_ppx/pull/113, https://github.com/aantron/bisect_ppx/issues/112 cc @rleonid, @gasche

alainfrisch commented 7 years ago

I think it's an interesting direction to expose a copy of the current Parsetree definition (corresponding to the most recent version of OCaml) in ppx_tools, and provide to/from mappings with old definitions in branches of ppx_tools corresponding to old versions of OCaml (and the identity for the current version). The mapping from old to new is guarantee to be total, while the other direction can fail (if an actual parsetree uses new features).

I'd rather provide such a compatibility layer in a new module, such as Ast_compat, rather than in Ast_convenience. And document it as a general compatibility layer instead of "just" as something done to support the introduction of Ppat_open.

It will also be required, at some point, to cover the whole Parsetree (and not just patterns and constants); but this can come later. One could also consider providing a version of Ast_mapper working on the Ast_compat types, so that client code (e.g. ppx rewriters) can simply do as if they were working with the most recent version of OCaml's Parsetree.

aantron commented 7 years ago

I agree with that. I've thought about the same thing, especially since I am working on some big code now that actually uses the whole of Parsetree and will need to be compatible between versions. Do you want to wait on/abandon this PR then?

alainfrisch commented 7 years ago

Do you think you might want to work on this whole Parsetree compatibility layer?

aantron commented 7 years ago

Yes, sent an email actually.

I thought about it for the last few hours. Perhaps it makes sense to make it a separate library?

ppx_tools could then either depend on it, or point users to it. Thoughts?

alainfrisch commented 7 years ago

ppx_tools is not really restricted to ppx; any program that produce ASTs can use its "metaquot" and "convenience" facility. If the compatibility layer is implemented in a separate library, it would be incompatible with ppx_tools (metaquot and convenience will work on the "Parsetre from compiler-libs", not the "Parsetree from the compatibility library").

So, without giving it much more thoughts, I think it would be best to avoid adding a separate library.

No problem to give you push access to the repo. (FWIW, I don't use ppx_tools myself and see it completely as a community project -- the more maintainers we have, the better.)

Drup commented 7 years ago

I agree. "ppx_tools" is really "ast_tools" at this point (and that's fine).

If we are moving toward this direction, it would be nice to have ppx_core-style autogenerated class mapper/folder/iterator/foldmapper (for each version of the ast).

@alainfrisch All this might go against your initial wish of keeping ppx_tools simple and free of build complexity ....

alainfrisch commented 7 years ago

Keeping things simple is always nice, but my real concern is to avoid external dependencies.

aantron commented 7 years ago

I will PR some initial version to ppx_tools in weeks. Will interleave work on it with the project that needs it.

alainfrisch commented 7 years ago

Relevant blog post from @diml : https://blogs.janestreet.com/an-solution-to-the-ppx-versioning-problem/#comment-66525

aantron commented 7 years ago

@alainfrisch the project I was working on that needed the parse tree layer did the same thing as codept does. Since it is now redundant, I don't have plans to work on the parse tree in the near future anymore. Perhaps @Octachron has some interest in this, however :)

alainfrisch commented 7 years ago

@aantron I've started to work to semi-automated mapping between versions of Parsetree. See https://github.com/alainfrisch/ppx_tools/tree/copy_parsetree

aantron commented 6 years ago

Now that ocaml-migrate-parsetree is out and in routine use, I think we can close this and #55. Please let me know if you object.

alainfrisch commented 6 years ago

Indeed omp is the way to go.