-
Notifications
You must be signed in to change notification settings - Fork 90
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
Remove extensions from fold, iter and map plugins #278
Conversation
These don't seem very useful: 1. On types without free variables, they're just identity. 2. On types with free variables, they require specially-named poly_ functions present in the scope. These are usually declared at the type declaration level of the deriver. These also appear to be unused according to sherlocode. Removing map avoids a superficial conflict with ppx_let.
I agree they don't seem very useful but I'd rather have ppx_let fixed than just silently drop a feature. If we decide to remove those we should do so through a deprecation cycle at the very least. |
My fear was just that having ppx_deriving.5.3.0 declaring a flat-out conflict with ppx_let could turn out to be problematic for some projects. If a new ppx_let version eventually comes around, then this PR wouldn't be needed, but whether and when that happens is unclear. In terms of maximizing backwards-compatibility there are two options:
I may be wrong, but my hypothesis is that nothing out there uses these extensions, so their removal wouldn't be breaking to anything and allow ppx_deriving.5.3.0 to be without big conflicts. Personally, I'm fine with having a version with the conflict declared because I don't use ppx_let, but would like to benefit from the performance improvements included in ppx_deriving.5.3.0 (both compile time and runtime). I'm just cautious about the effects on the ecosystem. |
I totally agree with your analysis here, don't get me wrong. I'm just reluctant to do it because there are potentially other consumers of this that are not on opam and that's something we tend to forget. I can definitely see how one would use those extensions out of habit from using the other more useful ones such as It's easy to be tempted to just break seemingly unused part of APIs because it makes things simpler for us in my opinion this also has a bad impact on the quality of the ecosystem. The advantage of the conflict here is that, the worse that can happen is that some users keep installing the version they have been using for the last three years while all the rest can benefit from the new version. Note that a deprecation cycle does not have to last over a long period of time as long as there is the right range of versions allowing a smooth transition. |
After giving this some thoughts, let's just remove them! |
CHANGES: * Fix a bug in `[@@deriving make]` that caused errors when it was used on a set of type declarations containing at least one non record type. ocaml-ppx/ppx_deriving#281 (@NathanReb) * Embed errors instead of raising exceptions when generating code with `ppx_deriving.make` ocaml-ppx/ppx_deriving#281 (@NathanReb) * Remove `[%derive.iter ...]`, `[%derive.map ...]` and `[%derive.fold ...]` extensions ocaml-ppx/ppx_deriving#278 (Simmo Saan) * Port standard plugins to ppxlib registration and attributes ocaml-ppx/ppx_deriving#263 (Simmo Saan) * Optimize forwarding in eq and ord plugins ocaml-ppx/ppx_deriving#252 (Simmo Saan) * Delegate quoter to ppxlib ocaml-ppx/ppx_deriving#263 (Simmo Saan) * Introduce `Ppx_deriving_runtime.Stdlib` with OCaml >= 4.07. This module already exists in OCaml < 4.07 but was missing otherwise. ocaml-ppx/ppx_deriving#258 (Kate Deplaix)
These don't seem very useful:
poly_
functions present in the scope. These are usually declared at the type declaration level of the deriver.These also appear to be unused according to sherlocode (we'll see about opam-repository CI...).
Removing map avoids a superficial conflict with ppx_let: janestreet/ppx_let#14.
Of course, the morally right thing would be to declare a conflict with ppx_let for now and remove the conflict once ppx_let prefixes their extension. However, that would likely be inconvenient for many and for practical backwards-compatibility it's easier to remove something which doesn't seem to be used.