pa_sexp_conv and wildcards

[trefis]

Hi again!

I noticed a problem when using with sexp in presence of unused type parameters (unused since pa_sexpconv can't handle GADTs), e.g. `# type t = int with sexp;; Error: Failure: "get_tparamid: not a type parameter\n"`

This problem (which is also present in other syntax extensions like bin_prot) can be easily fixed by ignoring these wildcards (because we obviously don't need to know how to convert values of such types to and from sexps).

Just 3 lines of code were necessary to "fix" the problem:

diff --git a/syntax/pa_sexp_conv.ml b/syntax/pa_sexp_conv.ml
index af498db..1f002e2 100644
--- a/syntax/pa_sexp_conv.ml
+++ b/syntax/pa_sexp_conv.ml
@@ -13,6 +13,8 @@ open Syntax
 module Gen = Pa_type_conv.Gen

 (* Utility functions *)
+let remove_unused_parameters tps =
+  List.filter tps ~f:(function Ast.TyAny _loc -> false | _ -> true)

 let mk_rev_bindings loc fps =
   let coll (i, bindings, patts, vars) fp =
@@ -558,6 +560,7 @@ module Generate_sexp_of = struct
       | `Match matchings ->
           <:expr@loc< fun [ $matchings$ ] >>
     in
+    let tps = remove_unused_parameters tps in
     let patts =
       List.map tps
         ~f:(fun ty -> <:patt@loc< $lid:"_of_" ^ Gen.get_tparam_id ty$>>)
@@ -1328,6 +1331,7 @@ module Generate_of_sexp = struct
     in
     let external_name = type_name ^ "_of_sexp" in
     let internal_name = type_name ^ "_of_sexp__" in
+    let tps = remove_unused_parameters tps in
     let arg_patts, arg_exprs =
       List.split (
         List.map ~f:(function tp ->

I'm not doing a merge request because the remove_unused_parameters function could probably be included in the type_conv library, especially if you modify the other extensions the same way.

The result being :

# type _ t = int with sexp;;
type _ t = int
val t_of_sexp__ : Sexplib.Sexp.t -> int = <fun>
val t_of_sexp : Sexplib.Sexp.t -> int = <fun>
val sexp_of_t : int -> Sexplib.Sexp.t = <fun>

Which is different from what is produced when you write type 'a t = int with sexp but I don't think that is a problem. On the contrary it offers (imho) more expressivity.

PS: I included the patch (which takes a lot of screen space here) because, while being only minor, the work doesn't need to be done twice.

[ghost]

One (minor) issue I can think of is that we would need to use [t] instead of [_ t] inside [<:sexp_of< >>]. It may be confusing if the type is abstract.

[ghost]

forwarded the question internally, here is a better response:

I am not sure how this would play with gadts, if we wanted to support them.

And them obviously this is a change of the call convention and since we don't have any typing information available, uses of the type would have to show that the type parameter is ignored.

Using t instead of _ t doesn't work in general because what about:

type _ t = int with sexp
type 'a u = 'a t with sexp

?

You would have to have some more magic types in pa_sexp:

type _ t = int with sexp
type 'a u = 'a sexp_phantom t with sexp

or something.

It is not obvious that this is preferable to writing the few wrappers to drop the type arguments by hand.

[trefis]

For GADTs, the situation is already problematic (and I see no good ways to handle them in general), so I don't think the change I proposed would have been the major issue.

For the other points : I didn't think of these two situations, my bad. I never used the [<:sexp_of...>] syntax so I don't know how much of a problem it would be, but the second issue (where you alias [t] with [u]) is obviously problematic and I'm a bit ashamed not to have thought about it.

So yes, I think I agree with you that writting the wrappers manually is probably the most reasonable solution.

Thank you for your time!