Dune / OMP support

[ejgallego]

Dear ppx_import devs, as noted in ocaml/dune#193 (comment) , Dune can support ppx_import style rewriters now.

However, some action would be needed on the ppx_import part. In particular, migrating it to use ocaml-migrate-parsetree, and maybe, even porting the build system to Dune (which IMVHO seems like a good idea).

Do you think any of these goals are desirable? Would you welcome a patch?

[whitequark]

Yes and yes.

[ejgallego]

Great, thanks @whitequark ; IIUC, using OMP requires to choose a version to start with, I understand that Ast_407 is the preferred choice?

[whitequark]

Sure.

[ejgallego]

I opted to use Ast_406 as the versioned ppx_tools.metaquot does not yet support 4.07.

The main roadblock I'm facing for now is the lack of Types support in OMP. Example:

  | Tarrow (label, lhs, rhs, _) ->
    Typ.arrow label (core_type_of_type_expr ~subst lhs)
                    (core_type_of_type_expr ~subst rhs)

Here, label comes from the current OCaml compiler, however Typ.arrow is in OMP so types do not match.

What strategy would you recommend folks ? Thanks!

p.s: The tree I'm working on is at https://github.com/ejgallego/ppx_import/tree/dune%2Bomp compilation with dune works up to the point of seeing the above error.

[whitequark]

Right. I'm not actually sure this is easily solvable.

[ejgallego]

There's not a lot of stuff to "migrate"; I could file a request to OMP so they support Types, or build a compat module in ppx_import itself.

However I have 0 experience with ppx so I'd appreciate guidance from the far more experienced people around.

[whitequark]

The problem is that ppx_import depends on Untypeast...

[ejgallego]

Untypeast

Sorry if my questions are naïve @whitequark , where is this dependency inserted? I can't seem to locate it.

[ejgallego]

Another naïve question I have is whether we should target ppxlib or ppx_tools_versioned.

[whitequark]

Hm, maybe not anymore. Really, I haven't worked with any of this in ages, @gasche would probably be in a much better position to answer.

[gasche]

I think it should be possible to separate (conceptually) the aspects of ppx_import that rely on the parsetree, and those that rely on the typedtree. Then the parsetree stuff would be versioned, and the typedtree stuff I guess would remain version-pinned (or you motivate people to implement ocaml-migrate-typedtree indeed).

The case you highlight is special in that a typedtree-world value, label, is sent to the parsetree-world. This doesn't happen very often. In the non-versioned master it works because they share the same Asttypes definition, but in the versioned work the asttypes definitions are distinct.

I would just consider that there are two Asttypes modules, and write conversion functions from one to the other. So write identity-looking functions from "the typedtree Asttypes" to "the parsetree Astty[pes". It's not a very big module, so it wouldn't be a lot of work.

[ejgallego]

Well, the only place I can see is Ast_mapper.register, in fact, I am not sure how this should be replaced in the new code.

If we forget about the typedtree migration [which can be achieved by compiling the plugin in 4.06.1], the only bits to replace in order to get the rewrite to compiler are:

• Ast_mapper.tool_name: I'm unsure what's the equivalence now.
• Ast_mapper.register: ditto.

[ejgallego]

I would just consider that there are two Asttypes module, and write conversion functions from one to the other. So write identity-looking functions from "the typedtree Asttypes" to "the parsetree Astty[pes". It's not a very big module, so it wouldn't be a lot of work.

Yup, that looks like a good plan. However before we do this I'd like to get the rewriter working with 4.06.1 + Ast_406 [which seems to work]. Then we can go back and support a few compilers more with a mini-migrate typedtree.

[ghost]

@ejgallego Ast_mapper.register should be replaced by Migrate_parsetree.Driver.register. The callback gets a config record containing a tool_name field.

[ejgallego]

@ejgallego Ast_mapper.register should be replaced by Migrate_parsetree.Driver.register. The callback gets a config record containing a tool_name field.

Thanks @diml !

[gasche]

@diml while you are here (thanks!), how do Ppxlib.Deriving plugins deal with the typedtree versioning issue? Do they never observe typed declarations or use Env?

[ghost]

They don't... In general, it's not clear how two ppx rewriters using the typer can be used together, so ppxlib doesn't try to support these. ppx_import is a particular case as it doesn't need to type the input. However, if you consider ppx rewriters such as ppx_show, it is not possible to use two of these on the same file.

It seems to me that the current ppx model is not very friendly to rewriters using the typer. I thought about how it could be changed to make it easier to write ppx rewriters using the typer and especially compose them. It seems to me that given that most ppx rewriters only generate code locally, we could simply do the expansion in the typer: i.e. whenever the typer encounters a foreign syntax, such as an extension point or a type declaration with a [@@deriving] attribute, then it would call a callback that would expand it and recurse on the result. The typer could pass typing informations to the callback. I believe that this would be a better model, although it's a non trivial amount of work.

Regarding the data types themselves and compatibility with multiple compiler versions, it seems to me that the most straightforward solution is to provide construction functions + view patterns. These can be versioned independently of the underlying data types.

[ejgallego]

Ok, I got the rewriter compiling now, however tests fail with:

$ dune build
    ocamldep src_test/.test_ppx_import.eobjs/test_ppx_import.ml.d (exit 2)
(cd _build/default && /home/egallego/.opam/4.06.1/bin/ocamldep.opt -modules -ppx .ppx/ppx_import/ppx.exe -impl src_test/test_ppx_import.ml) > _build/default/src_test/.test_ppx_import.eobjs/test_ppx_import.ml.d
File "/tmp/camlppx7c32d4", line 1:
Error: I can't decide whether /tmp/camlppx7c32d4 is an implementation or interface file
File "src_test/test_ppx_import.ml", line 1:
Error: Error while running external preprocessor
Command line: .ppx/ppx_import/ppx.exe '/tmp/camlppx7c32d4' '/tmp/camlppx7b156a'

will investigate more later.

[ghost]

Oops, that's a bug in dune. This PR should fix the issue: ocaml/dune#1218

[ejgallego]

Thanks again @diml !

[ejgallego]

Ok, so with the dune fix things seem to be working now with ppx_import in isolation :)

However, combining with ppx_deriving.show seems to be a problem:

dune build
    ocamldep src_test/.test_ppx_import.eobjs/test_ppx_import.ml.d (exit 2)
(cd _build/default && /home/egallego/.opam/4.06.1/bin/ocamldep.opt -modules -ppx '.ppx/ppx_deriving.show+ppx_import/ppx.exe --as-ppx' -impl src_test/test_ppx_import.ml) > _build/default/src_test/.test_ppx_import.eobjs/test_ppx_import.ml.d
File "src_test/test_ppx_import.ml", line 26, characters 10-28:
Error: show cannot be derived for [%import :Stuff.a]
Makefile:2: recipe for target 'build' failed
make: *** [build] Error 1

I do understand that this will be trickier to solve, right?

[ghost]

Yh, it's an ordering problem: ppx_import is not applied soon enough. I'm preparing a patch for omp to allow specifying that a rewriter must be applied before other ones.

[ghost]

I made the PR: ocaml-ppx/ocaml-migrate-parsetree#51

[ejgallego]

Thanks @diml . Another weird problem I'm hitting is that (libraries ...) don't seem to be properly set when calling ocamlfind. For example, I have:

(library
 (public_name coq-serapi.serlib)
 (name serlib)
 (synopsis "Serialization Library for Coq")
 (preprocess (staged_pps ppx_import ppx_sexp_conv))
 (libraries coq.stm sexplib))

But I do get:

ocamldep serlib/.serlib.objs/ser_equality.ml.d (exit 2)
(cd _build/default && /home/egallego/.opam/4.06.1/bin/ocamldep.opt -modules -ppx '.ppx/ppx_import+ppx_sexp_conv/ppx.exe --as-ppx --cookie '\''library-name="serlib"'\''' -impl serlib/ser_equality.ml) > _build/default/serlib/.serlib.objs/ser_equality.ml.d
File "serlib/ser_equality.ml", line 4, characters 12-26:
Error: [%import]: cannot locate module Equality

[ejgallego]

Does that mean that coq.stm should become a runtime dependency of the ppx?

[ghost]

Hmm, normally when tool_name = "ocamldep" ppx_import shouldn't lookup modules in the environment. Could you try printing tool_name?

[ejgallego]

File "serlib/ser_vernacexpr.ml", line 46, characters 31-55:
Error: [%import]: cannot locate module Vernacexpr
    ocamldep serlib/.serlib.objs/ser_xml_datatype.ml.d (exit 2)
(cd _build/default && /home/egallego/.opam/4.06.1/bin/ocamldep.opt -modules -ppx '.ppx/ppx_import+ppx_sexp_conv/ppx.exe --as-ppx --cookie '\''library-name="serlib"'\''' -impl serlib/ser_xml_datatype.ml) > _build/default/serlib/.serlib.objs/ser_xml_datatype.ml.d
tool_name: ppxlib_driver

[ghost]

Ah, I see the issue. We need a patch in ppxlib as well to not override the tool name. I'm looking at this.

[ghost]

This PR for ppxlib should fix the issue: ocaml-ppx/ppxlib#41

[ejgallego]

I confirm that with the above PR ppx_import + ppx_sexp_conv do work ! 🎉 🎉 🎉

The problem with ppx_deriving.std persists tho, strange as I'm using [@@deriving sexp] without trouble in the other repo.

[ejgallego]

@gasche a small note, I think that ppx_import doesn't rely on typedtree per-se, but in typed.

[ejgallego]

Note that #27 , fixing this issue is basically ready for review, and maybe merge.