Fix #203 Port to ppxlib - continuation of #206

[XVilka]

cppo is also required for OCaml 4.10 support, so cannot remove yet: https://github.com/ocaml-ppx/ppx_deriving/blob/master/src/api/ppx_deriving.cppo.ml#L626

[XVilka]

@rgrinberg @gasche please review this one.

[XVilka]

Any objections on this PR?

[rgrinberg]

and one macro I left for the convenience, because it reduces the code a lot - Attribute_patt(loc_, txt_, payload) used quite often in the matching, would be a lot of needless copy-pasting, imho.

I don't think it's the case and even if it was, it's still not a reason to use text substitution as a form of abstraction in OCaml.

[XVilka]

@rgrinberg @gasche please take a look, lets start the ppxlib (and ppx in the future) porting begin.

[mseri]

What is it that is blocking this PR at the moment? It would be very nice to have a ppxlib based ppx_deriving.

[gasche]

What is delaying the move to ppxlib is mostly maintainer availability. A couple months ago I gave maintainer rights to @thierry-martinez in the hope that he may be able to work on a release, but I didn't let him know explicitly enough (I only notified him before) and he is now probably overworked by the covid19 lockdown.

[thierry-martinez]

@gasche Sorry for the delay: I just noticed that the invitation you sent me has expired!

[gasche]

No apologies needed (and sorry for dumping this responsibility on you). I resent an invitation.

[thierry-martinez]

Thanks @XVilka and sorry again for the very long delay. I have only suggestions for minor changes, but I am ready to approve.

[thierry-martinez]

Why should we reject OCaml 4.04? The current code looks actually to work fine on 4.04 (even on 4.04.0). Anyway, if we want to constrain users to use OCaml 4.05, I think we should update the synopsis as well.

[thierry-martinez]

If we want to get rid of cppo, we could instead add the following binding at the beginning of the module:

let stdlib_compare = compare

[mseri]

For this one could also depend on stdlib-shims which is quite lightweight

[XVilka]

Agree on moving to stdlib-shims. It will solve this particular #if, but there are some others, unrelated to Stdlib. For example, see this for 4.10 support: https://github.com/ocaml-ppx/ppx_deriving/blob/master/src/api/ppx_deriving.cppo.ml#L626

[thierry-martinez]

I think we could maintain compatibility with OCaml 4.04 (see below).

[XVilka]

@thierry-martinez we restricted to 4.05 to reduce the cppo reliance:
I removed unnecessary ifdefs to reduce the churn like the following, most distributions have 4.05 now anyway:

open Parsetree
open Ppxlib

#if OCAML_VERSION >= (4, 05, 0)
type tyvar = string Location.loc
type tyvar = string Location.loc
#else

type tyvar = string
#endif

I would appreciate if we merge it as is, then we can improve it in subsequent pull requests.

[thierry-martinez]

Merged. Thanks, @XVilka!

[kit-ty-kate]

Using ppx_deriving master (now including this PR), many of the packages using ppx_deriving now fail with error messages similar to this one:

- + ocamlfind ocamldep -package zarith -package ppx_sexp_conv -package bytes -package sexplib -package cstruct -modules src/nocrypto.mli > src/nocrypto.mli.depends
- Location.Error(_)
- File "src/nocrypto.mli", line 1:
- Error: Error while running external preprocessor
- Command line: /home/opam/.opam/4.11.0+trunk/lib/ppx_deriving/./ppx_deriving package:ppx_sexp_conv '/tmp/camlppx3023b9' '/tmp/camlppx0159b2'

Is this expected?

[gasche]

You are politely phrasing it as a question but it sounds like a bug indeed. I don't know whether we should look for a bug within ppx_deriving itself, or at the interaction between ppx_deriving and ppxlib, or between ppxlib and the "driver" logic. (One reason to suspect the driver would be if the ocamlfind packages woud fail, and the dune packages would not, for example.)

[ghost]

It's a bit odd that the error is reported like this. It should be passed from the ppx rewriter to the compiler via an ocaml.error extension point, and properly reported by the compiler.

You can try wrapping the entry point in ppx_deriving_main.cppo.ml as follow:

  try
    Ast_mapper.register "ppx_deriving" mapper
  with exn ->
    Location.report_exception Format.err_formatter exn;
    exit 1

that should help understand what is going on.

[kit-ty-kate]

One reason to suspect the driver would be if the ocamlfind packages woud fail, and the dune packages would not, for example.

It seems to be the case from the results I have. The only package I can see for now using dune that still does fail is jupyter:

#=== ERROR while compiling jupyter.2.7.1 ======================================#
# context              2.1.0~alpha | linux/x86_64 | ocaml-base-compiler.4.10.0 | file:///home/opam/opam-repository
# path                 ~/.opam/4.10.0/.opam-switch/build/jupyter.2.7.1
# command              ~/.opam/4.10.0/bin/dune build -p jupyter -j 71
# exit-code            1
# env-file             ~/.opam/log/jupyter-8-5bdbb6.env
# output-file          ~/.opam/log/jupyter-8-5bdbb6.out
### output ###
# [...]
# Segmentation fault (core dumped)
# File "jupyter/src/kernel/message_channel.mli", line 1:
# Error: Error while running external preprocessor
# Command line: /home/opam/.opam/4.10.0/lib/ppx_deriving/./ppx_deriving package:ppx_deriving_yojson '/tmp/camlppx443db1' '/tmp/camlppx387ffb'
# 
#       ocamlc jupyter/src/kernel/.jupyter_kernel.objs/byte/jupyter_kernel__Client.{cmi,cmti} (exit 2)
# (cd _build/default && /home/opam/.opam/4.10.0/bin/ocamlc.opt -ppx '/home/opam/.opam/4.10.0/lib/lwt_ppx/./ppx.exe --as-ppx' -ppx '/home/opam/.opam/4.10.0/lib/ppx_deriving/./ppx_deriving package:ppx_deriving_yojson' -short-paths -safe-string -strict-sequence -strict-formats -w A-4-31-33-34-39-41-42-43-44-45-48-49-50-58 -g -bin-annot -I jupyter/src/kernel/.jupyter_kernel.objs/byte -I /home/opam/[...]
# Segmentation fault (core dumped)
# File "jupyter/src/kernel/client.mli", line 1:
# Error: Error while running external preprocessor
# Command line: /home/opam/.opam/4.10.0/lib/ppx_deriving/./ppx_deriving package:ppx_deriving_yojson '/tmp/camlppx675f5a' '/tmp/camlppx54c3ed'

However it uses a patched version of ppx_deriving: ocaml-ppx/ppx_deriving_yojson#118. I will try again with @thierry-martinez's new and improved PR

[kit-ty-kate]

You can try wrapping the entry point in ppx_deriving_main.cppo.ml as follow:

  try
    Ast_mapper.register "ppx_deriving" mapper
  with exn ->
    Location.report_exception Format.err_formatter exn;
    exit 1

that should help understand what is going on.

I tried and the catch-block wasn't hit. The error seems to have been caught earlier and the program exited before the Ast_mapper.register was able to return anything (a prerr_endline displays something if placed before the call, but not after)

[thierry-martinez]

I propose a fix here: #221

I don't know where exactly the segmentation fault occurs, but it appears that it occurs while executing the mapper obtained by conversion through Migrate_parsetree. Instead of converting the whole mapper, I suggest to convert only the parts of the AST that have to be deconstructed by pattern-matching: we keep the compatibility between the different AST versions, it appears to fix the segmentation fault, and it should be more efficient.

[gasche]

I find it worrying that we would have a segmentation fault that we can't explain and don't know if we actually fixed. I can try to have a look in the next few days; I assume that the issue is easy to reproduce by trying to build jupyter?

[kit-ty-kate]

@gasche yes it is. Here is a Dockerfile to reproduce the segfault:

FROM ocurrent/opam:debian-10
RUN opam pin add -n ppx_deriving git://github.com/ocaml-ppx/ppx_deriving.git#cb1ad02d14d4757221c8dfa2aea2df616ca9500c
RUN opam pin add -n ppx_deriving_yojson git://github.com/thierry-martinez/ppx_deriving_yojson.git#5a2680829a9d1cb30821d296b3ec66cb56fc7cc1
RUN git -C ~/opam-repository pull origin master && opam update
RUN opam depext jupyter.2.7.1
RUN opam install --deps-only jupyter.2.7.1
RUN opam install jupyter.2.7.1

[thierry-martinez]

I think I understood the segmentation fault, but I don't see what would be the good solution to fix it. Ppx_deriving_main calls Ast_mapper.register where Ast_mapper is defined in Ast_408. Ast_408.Ast_mapper.register calls (through the reference register_function) the function Ast_408.Ast_mapper.run_main, which in turn calls the function Ast_408.Ast_mapper.apply_lazy. The problem occurs in this function.

The problem is that the source file in read by using input_value (which is known to be unsafe), and the result is supposed to be of type Ast_408.Parsetree.signature or Ast_408.Parsetree.structure, which is correct only if the current OCaml is 4.08.

I observe the segmentation fault quite soon afterwards, when Migrate_parsetree_408_409_migrate.copy_module_type is called on the signature just read, which is expected to fail since Parsetree,module_type has changed between 4.08 and my current 4.10.

I don't see a good solution for this problem apart from not using Ast_xxx.Ast_mapper.register. I believe that these functions cannot check that the version of the given AST is correct since I believe that AST are not versioned. What they could do is to suppose that the given AST is from the current version of the compiler instead of the migrated one, but that would suppose that they convert the AST before applying the mapper, which would involve some recursion twists since the migrate-parsetree machinery is being defined: but I don't think it is worth changing this, because it is not clear that the new semantics (supposing that the given AST is for the current OCaml version) is better than the old one (even if it does not look very useful to be able to cross-compile a PPX transformer for another version of OCaml, who knows...)

Not using Ast_xxx.Ast_mapper.register is essentially what I proposed in #221.

[gasche]

I haven't followed your tracks yet, but yet another naive question: there should be a magic number in serialized ASTs, that differs between language versions. If this is really an incompatible-format mismatch, how come that we didn't see a magic-number failure instead of a segfault?

I initially wondered if it was an upstream issue, not updating magic numbers at this point of the 4.11 release process (so they would be wrongly identical to the 4.10 ones). But Florian did update the magic numbers, and in any case they are distinct from the 4.08 ones.

[gasche]

P.S.: It does not go without saying: thanks a lot for your work on the ppxlib port and tracking this new issue.

[gasche]

I suspect a bug in ocaml-migrate-parsetree; in ast_408.ml, the magic number checks of apply_lazy are implemented by comparing with Config module, where Config appears to point to the compiler libraries Config module, for the current OCaml version, instead of a 4.08-specific Config module. There is a Config submodule in the file that should override the compiler-libs Config, but it is only defined later.

[thierry-martinez]

Oh, sorry, I suspected that initially but I missed the fact that utils/config.mlp was regenerated by ./configure in OCaml source tree, and I was lead to the surprising observation that the magic numbers did not change between versions. Thank you for correcting me! So yes, the fix should be not to use Config in Ast_xxx modules but version specific magic numbers instead. May I submit a patch to ocaml-migrate-parsetree? The fix I proposed for ppx_deriving still seems correct.

[gasche]

May I submit a patch to ocaml-migrate-parsetree?

Yes, feel free to. I created an issue (not a PR) there as ocaml-ppx/ocaml-migrate-parsetree#97 . If you fix this, you should fix it for all affected OCaml versions. I think it would also be nice to include a device to ensure that the error would lead to a compilation failure if the modules get reordered again. For example the Config submodule could include a value let in_ast_408 = () (with a comment that refers to the issue/PR), and then the magic-check code would first let () = Config.in_ast_408 in.