janestreet / ppx_sexp_conv

Generation of S-expression conversion functions from type definitions
MIT License
88 stars 17 forks source link

`[%sexp_of: t]` expands to code that does not use `t` as a type #34

Open jberdine opened 3 years ago

jberdine commented 3 years ago

There is an irregularity between %sexp_of and e.g. %compare. Consider:

  let compare (type k v) compare_k compare_v = [%compare: (k * v) list]
  let sexp_of_t (type k v) sexp_of_k sexp_of_v = [%sexp_of: (k * v) list]

This triggers

Warning 34 [unused-type-declaration]: unused type k.

(and the same for v).

On the other hand, omitting the type parameters:

  let compare compare_k compare_v = [%compare: (k * v) list]
  let sexp_of_t sexp_of_k sexp_of_v = [%sexp_of: (k * v) list]

triggers

Error: Unbound type constructor k

on the definition of compare, while sexp_of_t is ok.

aalekseyev commented 3 years ago

This is indeed desirable, and in fact we attempted this change at some point. However, the change ended up too massive and too disruptive because of how often people abuse this "feature" to derive sexp conversions for types that don't exist, so we gave up.

Perhaps some day we'll try again, if we can come up with a way to make the change incrementally, but I'm afraid that day is not today.

jberdine commented 3 years ago

Thanks for the context! This is indeed very tempting to abuse, so not so surprising. I wonder if a way forward would be to add support to ppx_sexp to accept a cookie so that if e.g. --cookie 'ppx_sexp_strict="1"' is passed then the strict interpretation is used. Then projects could add a cookies clause to their dune files to indicate that they conform to the stricter interpretation, and maybe one day the default could be switched.

aalekseyev commented 3 years ago

Yeah, that sounds like a workable approach.