ocaml-ppx / ppx_deriving

Type-driven code generation for OCaml
MIT License
466 stars 89 forks source link

Remove extensions from fold, iter and map plugins #278

Closed sim642 closed 7 months ago

sim642 commented 8 months ago

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 (we'll see about opam-repository CI...).

Removing map avoids a superficial conflict with ppx_let: https://github.com/janestreet/ppx_let/issues/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.

NathanReb commented 8 months ago

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.

sim642 commented 8 months ago

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:

  1. Declare a conflict with ppx_let and wait. opam-repository CI shows that this combination is used in practice. However, this also means that ppx_deriving.eq, etc (everything that's not ppx_deriving.map) cannot be co-used with ppx_let because the conflict would be at the opam package level (preventing simultaneous installation), while all other derivers from here are perfectly fine. Alternatively, every such project could declare the conflict themselves, but that would just create another result <1.5 situation.
  2. Do this. Technically it is a breaking change, but I have been unable to find any use of the [%map: ...] extension (not in the wild, nor in the tests here). Since it's practically useless, it's a lot less likely to be used together with ppx_deriving than ppx_let.

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. It's always possible to revert this if opam-repository CI proves my hypothesis wrong and reveals breakage.

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.

NathanReb commented 8 months ago

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 [%show ...].

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.

NathanReb commented 7 months ago

After giving this some thoughts, let's just remove them!