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

[WIP] Deprecate the ppx_deriving API in favour of ppxlib #250

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kit-ty-kate
Copy link
Collaborator

This PR has two goals:

  1. Add a deprecation warning shown to the users of the ppx_deriving API (note that the warning is fatal by default in dune)
  2. TODO: Use ppxlib for the builtin deriving plugins (show, eq, ord, …). This will help pinpoint the possible issues that users will encounter when migrating and help the ppxlib API itself by adding helpful functions if something is missing there.

I don't have time or motivation to implement the last point right now, so if anyone has either of those things feel free to try it out in the meantime.

@gasche
Copy link
Contributor

gasche commented Feb 2, 2021

I'm not too enthusiastic about breaking people's projects (with a deprecation-warning-turned-into-an-error). What do we gain here, and what are the costs?

There is already a note at the beginning of the README of the project (Kate of course knows, but for the context):

Note: since deriving was released by whitequark in 2014, the OCaml ppx ecosystem has changed a lot. For new projects wishing to create a new deriving plugin, we recommend using ppxlib directly. The module Ppxlib.Deriving provide functionality similar to deriving, better integrated with ppxlib, and offers a nicer API in some places. deriving is still maintained to keep existing plugins working as well as possible. Although note that the above deprecation note only covers the API and not the plugins (e.g. ppx_deriving.show, ppx_deriving.eq, ...).

My impression is that it is reasonably possible to do what is proposed in the note: encourage ppxlib.Deriving for new deriving projects, but keep old deriving-projects compiling. If we can do this, why bother the authors of existing ppx-deriving plugins with a breakage in their codebase?

Your point 2, if I understand correctly, is to rewrite the builtin ppx_deriving plugins to use ppxlib.Deriving directly instead of the ppx_deriving API. I think that this is a good idea, because it reduces the chance that end-users are hit with the limitations of the ppxlib implementation of ppx_deriving. In general we could try to gradually port existing ppx_deriving plugins to ppxlib.Deriving (those that don't have a replacement already), and close the light when there are no dependencies on the ppx_deriving API on opam (add a deprecation warning, and eventually retire the API).

@c-cube
Copy link
Contributor

c-cube commented Feb 12, 2022

I realize this is an old issue, but 2. would be super useful because of the deriving inline feature. Having 0 actual dependency on ppx, for such an important and foundational set of derivers as ord, eq, show, map, iter, would be amazing. For 1., there's at least visitors.ppx out there that seems to rely on ppx_deriving's legacy API, which is a bit of a shame because the same arguments would apply.

@pitag-ha
Copy link
Member

Hey @c-cube, I definitely agree with you that

  1. would be super useful

Have you seen our WIP Ppxlib.Deriving standard derivers project? It's still in its very beginnings, but I think it's pretty much what you are looking for, isn't it?

Having 0 actual dependency on ppx

Right, thanks for reminding me of deriving_inline! I've just updated the README of the Ppxlib.Deriving standard derivers project to mention that super important feature.

Btw, cc @@ayc9 who is working on the Ppxlib.Deriving standard derivers project.

@pitag-ha
Copy link
Member

For 1., there's at least visitors.ppx out there that seems to rely on ppx_deriving's legacy API, which is a bit of a shame because the same arguments would apply.

Is there some problem due to which visitors.ppx can't be ported to Ppxlib.Deriving?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants