Restore ocamlfind loader

[kit-ty-kate]

No description provided.

[ghost]

We had a look at this with @kit-ty-kate. To be complete, we need a Ppxlib.Driver.map_signature function, and we need to use it here, but otherwise this looks be good to merge.

[gasche]

Thanks @kit-ty-kate and @diml for looking at this. Tell me if I understand correctly:

• the change is motivated by breakage among projects that use ppx_deriving with ocamlfind (rather than dune)
• the main aspect of the change is to restore previous driver-registration code (is it exactly the older code, or have some things changed?)
• the change reverts to ppxlib 0.16 rather than the newer 0.18 (which I suppose doesn't have the API for such driver registration)
• currently this only supports rewriting .ml files, not .mli files, due to limitations in Ppxlib

Are there plans to lift the limitations in ppxlib? (Would that then take the form of a minor 0.16 release, rather than continuing from 0.18?)

[ghost]

My guess is that the loader was removed prematurely by accident. It is required to use deriving plugins with ocamlfind. It is indeed not used by Dune. It is quite complex and could be replaced by a simpler system, but that would require a bit of preparation.

the main aspect of the change is to restore previous driver-registration code (is it exactly the older code, or have some things changed?)

Not really, it restores the dynamic loader. In a nutshell, when you use ppx rewriters with ocamlfind, you get one -ppx argument per ppx rewriters. However, that doesn't work for deriving plugins because they must all be applied in the same process. So ocamlfind construct a single -ppx argument of the form -ppx "<ppx_deriving-dir>/ppx_deriving package:ppx_sexp_conv package:ppx_deriving.show ...". The ppx_deriving tool then computes the transitive closure of the various package given on the command line (using ocamlfind again, but this time as a library) and dynamically load them.

Without this loader, deriving plugins cannot be used with ocamlfind at all.

the change reverts to ppxlib 0.16 rather than the newer 0.18 (which I suppose doesn't have the API for such driver registration)

I didn't notice that, but I'm not sure that was the intent. It might have been just for testing. I'll let @kit-ty-kate comment on that.

Are there plans to lift the limitations in ppxlib? (Would that then take the form of a minor 0.16 release, rather than continuing from 0.18?)

We talked about it, Kate is going to add the missing function in ppxlib and do any necessary minor release.

[kit-ty-kate]

@jeremiedimino actually we might not need any change in ppxlib. There was no mention of signature in the original and the mention we saw in the code was probably a quirk added by my previous failed attempt to port it to ppxlib.

Here is the diff between the v5.1 tag and the current branch (git diff v5.1 -- src/ppx_deriving_main.cppo.ml). Does this looks correct to you?

diff --git a/src/ppx_deriving_main.cppo.ml b/src/ppx_deriving_main.cppo.ml
index 375889e..3ca76de 100644
--- a/src/ppx_deriving_main.cppo.ml
+++ b/src/ppx_deriving_main.cppo.ml
@@ -6,12 +6,10 @@ open Ast_helper
 module Ast_mapper = Ocaml_common.Ast_mapper
 
 module From_current =
-  Migrate_parsetree.Convert (Migrate_parsetree.OCaml_current)
-    (Migrate_parsetree.OCaml_410)
+  Ppxlib_ast.Selected_ast.Of_ocaml
 
 module To_current =
-  Migrate_parsetree.Convert (Migrate_parsetree.OCaml_410)
-    (Migrate_parsetree.OCaml_current)
+  Ppxlib_ast.Selected_ast.To_ocaml
 
 let raise_errorf = Ppx_deriving.raise_errorf
 
@@ -70,21 +68,15 @@ let add_plugins plugins =
 let mapper argv =
   get_plugins () |> List.iter load_plugin;
   add_plugins argv;
-  let copy_structure_item item =
-    match From_current.copy_structure [item] with
-    | [item] -> item
-    | _ -> failwith "Ppx_deriving_main.copy_structure_item" in
-  let module Current_ast = Migrate_parsetree.OCaml_current.Ast in
-  let omp_mapper = Migrate_parsetree.Driver.run_as_ast_mapper [] in
-  let structure mapper s =
+  let module Current_ast = Ppxlib_ast.Selected_ast in
+  let structure s =
     match s with
     | [] -> []
     | hd :: tl ->
         match
-          try Some (copy_structure_item hd)
-          with Migrate_parsetree.Def.Migration_error (_, _) -> None
+          hd
         with
-        | Some ([%stri [@@@findlib.ppxopt [%e? { pexp_desc = Pexp_tuple (
+        | ([%stri [@@@findlib.ppxopt [%e? { pexp_desc = Pexp_tuple (
             [%expr "ppx_deriving"] :: elems) }]]]) ->
             elems |>
             List.map (fun elem ->
@@ -93,9 +85,15 @@ let mapper argv =
                   file
               | _ -> assert false) |>
             add_plugins;
-            mapper.Current_ast.Ast_mapper.structure mapper tl
-        | _ -> omp_mapper.Current_ast.Ast_mapper.structure mapper s in
-  { omp_mapper with Current_ast.Ast_mapper.structure }
+            Ppxlib.Driver.map_structure tl
+        | _ -> Ppxlib.Driver.map_structure s
+  in
+  let structure _ st =
+    Current_ast.of_ocaml Structure st
+    |> structure
+    |> Current_ast.to_ocaml Structure
+  in
+  { Ast_mapper.default_mapper with structure }
 
 let () =
   Ast_mapper.register "ppx_deriving" mapper

@gasche The code reverts to ppxlib 0.16 to allow users to have a more choice in their opam switches as some packages don't have stable versions compatible with ppxlib 0.18 yet and it might conflicts with them. So having two releases of ppx_deriving, 5.2 for ppxlib 0.16 and 5.3 for ppxlib 0.18 allow users to have this choice and avoid them to be in a configuration where they require older versions of everything to satisfy the constraints.
In theory we could use cppo to have conditional code depending on the version of ppxlib but dune doesn't seem to support that (%{version:ppxlib} doesn't work for packages installed by opam)

If @jeremiedimino gives me the go-ahead I'll merge this, and re-release ppx_deriving 5.2 and 5.3 right away.

[kit-ty-kate]

Ah nevermind, I just tested and we really need Ppxlib.map_signature. I'll add that later today and will merge this as soon as ppxlib.0.19.1 is out.

[ghost]

We indeed need map_signature. In the previous code, the signature field of the ast mapper was filled by Migrate_parsetree.Driver.run_as_ast_mapper [].

The diff looks correct to me.

[kit-ty-kate]

Final diff compared to v5.1:

diff --git a/src/ppx_deriving_main.cppo.ml b/src/ppx_deriving_main.cppo.ml
index 375889e..762dcb8 100644
--- a/src/ppx_deriving_main.cppo.ml
+++ b/src/ppx_deriving_main.cppo.ml
@@ -6,12 +6,10 @@ open Ast_helper
 module Ast_mapper = Ocaml_common.Ast_mapper
 
 module From_current =
-  Migrate_parsetree.Convert (Migrate_parsetree.OCaml_current)
-    (Migrate_parsetree.OCaml_410)
+  Ppxlib_ast.Selected_ast.Of_ocaml
 
 module To_current =
-  Migrate_parsetree.Convert (Migrate_parsetree.OCaml_410)
-    (Migrate_parsetree.OCaml_current)
+  Ppxlib_ast.Selected_ast.To_ocaml
 
 let raise_errorf = Ppx_deriving.raise_errorf
 
@@ -53,7 +51,7 @@ let get_plugins () =
       | { pexp_desc = Pexp_tuple exprs } ->
         exprs |> List.map (fun expr ->
           match expr with
-          | { pexp_desc = Pexp_constant (Pconst_string (file, None)) } -> file
+          | { pexp_desc = Pexp_constant (Pconst_string (file, _, None)) } -> file
           | _ -> assert false)
       | _ -> assert false
 
@@ -64,38 +62,42 @@ let add_plugins plugins =
   let loaded  = loaded @ plugins in
   Ast_mapper.set_cookie "ppx_deriving"
     (To_current.copy_expression
-       (Exp.tuple (List.map (fun file ->
-          Exp.constant (Pconst_string (file, None))) loaded)))
+       (Exp.tuple (List.map (Ast_builder.Default.estring ~loc:Location.none) loaded)))
 
 let mapper argv =
   get_plugins () |> List.iter load_plugin;
   add_plugins argv;
-  let copy_structure_item item =
-    match From_current.copy_structure [item] with
-    | [item] -> item
-    | _ -> failwith "Ppx_deriving_main.copy_structure_item" in
-  let module Current_ast = Migrate_parsetree.OCaml_current.Ast in
-  let omp_mapper = Migrate_parsetree.Driver.run_as_ast_mapper [] in
-  let structure mapper s =
+  let module Current_ast = Ppxlib_ast.Selected_ast in
+  let structure s =
     match s with
     | [] -> []
     | hd :: tl ->
         match
-          try Some (copy_structure_item hd)
-          with Migrate_parsetree.Def.Migration_error (_, _) -> None
+          hd
         with
-        | Some ([%stri [@@@findlib.ppxopt [%e? { pexp_desc = Pexp_tuple (
+        | ([%stri [@@@findlib.ppxopt [%e? { pexp_desc = Pexp_tuple (
             [%expr "ppx_deriving"] :: elems) }]]]) ->
             elems |>
             List.map (fun elem ->
               match elem with
-              | { pexp_desc = Pexp_constant (Pconst_string (file, None))} ->
+              | { pexp_desc = Pexp_constant (Pconst_string (file, _, None))} ->
                   file
               | _ -> assert false) |>
             add_plugins;
-            mapper.Current_ast.Ast_mapper.structure mapper tl
-        | _ -> omp_mapper.Current_ast.Ast_mapper.structure mapper s in
-  { omp_mapper with Current_ast.Ast_mapper.structure }
+            Ppxlib.Driver.map_structure tl
+        | _ -> Ppxlib.Driver.map_structure s
+  in
+  let structure _ st =
+    Current_ast.of_ocaml Structure st
+    |> structure
+    |> Current_ast.to_ocaml Structure
+  in
+  let signature _ si =
+    Current_ast.of_ocaml Signature si
+    |> Ppxlib.Driver.map_signature
+    |> Current_ast.to_ocaml Signature
+  in
+  { Ast_mapper.default_mapper with structure; signature }
 
 let () =
   Ast_mapper.register "ppx_deriving" mapper

Thanks a lot @jeremiedimino!