Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jberdine opened this issue Jun 24, 2021 · 3 comments
Open

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

jberdine opened this issue Jun 24, 2021 · 3 comments
Labels
forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system. not-clear-what-next-step-is This report is kept open as a reminder, but it is not clear what should happen next.

Comments

@jberdine
Copy link

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.

@github-iron github-iron added the forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system. label Jun 25, 2021
@aalekseyev
Copy link
Contributor

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.

@aalekseyev aalekseyev added the not-clear-what-next-step-is This report is kept open as a reminder, but it is not clear what should happen next. label Jun 28, 2021
@jberdine
Copy link
Author

jberdine commented Jun 28, 2021

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
Copy link
Contributor

Yeah, that sounds like a workable approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system. not-clear-what-next-step-is This report is kept open as a reminder, but it is not clear what should happen next.
Projects
None yet
Development

No branches or pull requests

3 participants