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

Remove extensions from fold, iter and map plugins #278

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

sim642
Copy link
Contributor

@sim642 sim642 commented Mar 12, 2024

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: 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.

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.
@NathanReb
Copy link
Collaborator

NathanReb commented Mar 14, 2024

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

sim642 commented Mar 14, 2024

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

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

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

@NathanReb NathanReb merged commit 4de24cb into ocaml-ppx:master Mar 27, 2024
1 check passed
NathanReb added a commit to NathanReb/opam-repository that referenced this pull request Apr 15, 2024
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)
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.

2 participants