Port standard plugins to ppxlib registration and attributes #263

sim642 · 2022-07-19T09:16:56Z

This realizes part of #250:

All the standard deriving plugins defined here are switched to register themselves directly with ppxlib and declare their attributes directly with ppxlib.
Deprecates only parts of ppx_deriving API, namely ppx_deriving deriver registration and attribute support. All the other utility functions remain undeprecated since many are still missing from ppxlib (Ppx_deriving utility functions ppxlib#317).
Delegates ppx_deriving quoter to ppxlib quoter.

Notably, this makes [@@deriving_inline ...] work on these standard derivers.

TODO

Re-add derive.* extensions support?
Fix missing optional derivers support? Separate issue really: new bug in ppx_deriving ("optional" no longer works) #247.

src/api/ppx_deriving.cppo.ml

src/api/ppx_deriving.cppo.mli

src_plugins/create/ppx_deriving_create.cppo.ml

src_test/deriving/test_ppx_deriving.ml

sim642 · 2022-07-19T10:02:28Z

src_plugins/show/ppx_deriving_show.cppo.ml

+(* TODO: add to ppxlib? *)
+let ebool: _ Ast_pattern.t -> _ Ast_pattern.t =
+  Ast_pattern.map1 ~f:(function
+    | [%expr true] -> true
+    | [%expr false] -> false
+    | _ -> failwith "not bool")
+let args () = Deriving.Args.(empty +> arg "with_path" (ebool __))


Handling the arguments in show { with_path = false } using ppxlib was surprisingly annoying. Especially since the argument is always option, so the default case needs to be handled below separately. Maybe would be useful to have some arguments with defaults in ppxlib?

src_plugins/show/ppx_deriving_show.cppo.ml

pitag-ha · 2022-09-06T15:27:12Z

Hey @sim642, even though I'm still not finding the time to review (as we've mentioned already, finding time to work on PPX-related things is rare), I at least already wanted to take the time to thank you. Thanks for the PR! I personally would have preferred to write a new bunch of standard derivers to have more freedom to improve things (as started here, but never finished/gotten far). However, I appreciate the work and it's definitely good to have these standard derivers written in ppxlib. It might still take a bit, but when we some have time for PPX we'll review this - if not me, then @panglesd. Feel free to ping us once in a while.

sim642 · 2023-03-09T07:30:52Z

@pitag-ha @panglesd ping!

panglesd · 2023-03-10T13:14:27Z

Very sorry about the delay. I'll put that on the top of my todo list. (Although next week I'm on holidays).

panglesd

Few comments, restricted to the API part (I'll continue with the plugins tomorrow).

Is Arg deprecated in favour of Ast_pattern?

I guess functions which have a direct equivalent in ppxlib should be deprecated. So Quoting functions should be deprecated, as well as mangle_lid, and mangle_type_decl.
(Or is there a reason not to do so?)

Finally, it could be nice to add a message to the deprecated alerts: [@@ocaml.deprecated "Use Ppxlib.register ... instead] (I think that is valid OCaml).

src/api/ppx_deriving.cppo.ml

ppx_deriving.opam

src/api/ppx_deriving.cppo.ml

src/api/ppx_deriving.cppo.mli

sim642 · 2023-03-21T20:23:35Z

Regarding the deprecations, it would be fine to also just exclude them from this PR. If I remember correctly, the original inspiration was #250, which proposes to deprecate the entire API. However, given warnings-as-errors in many projects, this might cause unnecessary breakage of derivers still built on ppx_deriving. Having everything ported would be nice, but these things don't have to be coupled.

I might've also used the deprecation warnings as a guide to find any usages to port within these standard plugins.

Is Arg deprecated in favour of Ast_pattern?

It's not even marked deprecated here right now, but I think I wanted to make as big step towards fully using ppxlib as possible, hence the switch to Ast_pattern which should replace it.
In a way, this PR serves as an exercise in porting such derivers and revealing places where ppx_deriving's old API is more convenient than ppxlib's new one.

panglesd

I went through the plugins, and the modifications look good!

However, I agree that they sometimes show some holes or defects in the ppxlib API. I summarize them here:

Quoter.sanitize should not use Recursive
Code_path.main_module_name should use the true input filename and not be subject to line directives
Add ebool : (bool, 'a, 'b) t -> (expression, 'a, 'b) t (there is already an open issue about that, Ast_pattern: augment API with ebool, pbool helper, and a new map. ppxlib#399)
Add some functions in the API to handle "unit" attributes:

val declare_flag : string -> 'a Context.t -> ('a, unit) t
val check_flag : ('a, unit) t -> ?mark_as_seen:bool -> 'a -> bool

Add a way to define arguments with defaults

val arg_with_default : 
  string ->
  (expression, 'a -> 'a, 'a) Ast_pattern.t ->
  default:'a ->
  'a param

fix on the optional issue new bug in ppx_deriving ("optional" no longer works) #247 -- optional parameter for ppx_bin_prot ppxlib#125.
Combine the extraction of attributes from two nodes of the AST. For this one, I'm less sure it should be in the ppxlib API...

Most of them are not too hard to solve. I will open the relevant issues, and label them as good first issues (can be useful for an Outreachy applicant)!
For turning Recursive to NonrecursiveinQuoter`, you can make a PR directly with this, since you offered it, thanks!

src_plugins/enum/ppx_deriving_enum.cppo.ml

src/api/ppx_deriving.cppo.mli

src_plugins/show/ppx_deriving_show.cppo.ml

panglesd · 2023-03-22T16:07:50Z

About the inclusion of deprecation of the API in this PR: I don't have a strong opinion at all...

If there is a clear indication of what should be used instead, and it's easy to fix (for instance, the mangle function, but not the Arg module), then I think it is OK: when built by opam, warnings are not errors, and when built locally, it is easy to follow the instructions to fix the deprecations warning.

But, I agree that it is not completely necessary, and might upset some people. Maybe, a mention in the ppx_deriving documentation is enough.

Co-authored-by: panglesd <[email protected]>

This reverts commit 4c9178c.

sim642 · 2023-03-23T08:18:01Z

I've just excluded all the deprecations from this PR right now to avoid blocking this by those additional discussions and decisions.

gasche · 2023-11-05T09:27:51Z

I'm one of the few maintainers of ppx_deriving, mostly inactive. I can give some time next week to look at this again and move towards merging, but I prefer to let other people take care of releases. Do we have a volunteer to cut out a new release? (I'm happy to give commit rights to enable this.)

sim642 · 2023-11-05T09:42:51Z

Hey! This has been sitting around for quite a while now. It'd be great to get this merged and released as well. It's been a long while since ppx_deriving has seen a release (even #252 hasn't made it).

kit-ty-kate · 2023-11-05T14:25:45Z

@pitag-ha @panglesd do you want the merge rights by any chance?

panglesd · 2023-11-08T09:08:23Z

Hello ! Sorry I'm off during one month, I will assess whether I accept this responsibility when I come back!

pitag-ha · 2023-11-13T18:05:42Z

@pitag-ha @panglesd do you want the merge rights by any chance?

I don't :) Let's see if @panglesd happens to want them when he's back. Btw, what's the current maintenance status of ppx_deriving? And what's the current general status of ppx_deriving? From what I remember, ppx_deriving is still used in a few contexts:

A few derivers are still registered with ppx_deriving. IIRC, ppx_deriving just forwards the registration to ppxlib in that case.
a) The ppx_deriving.std plug-ins are still registered with ppx_deriving (changes with this PR).
b) A few other derivers are still registered with ppx_deriving (e.g. visitors?).
The ppx_deriving driver is still used in a few cases.
a) utop, and possibly other toplevels as well, use the ppx_deriving driver for derivers rather than the ppxlib driver.
b) Is there any build system that uses ppx_deriving? (dune doesn't, but I don't know about other build systems or people who write their custom build Makefiles)

Is that right?

I can give some time next week to look at this again and move towards merging

Thank you, @gasche! If that would help, I could answer questions about Ppxlib if they come up.

panglesd · 2023-12-21T10:07:25Z

Sorry for the long delay, I'm back.

@NathanReb since you have joined as a ppxlib maintainer, there is a new variable in the equation! Would you be interested in any activity involving this repository?

If not, I will be happy to take care of cutting a release that includes this change. That being said, I can't commit to be an active maintainer in the long run!

NathanReb · 2024-01-04T09:34:24Z

I'd be happy to take part in smoothing things out here yes! Will need to catch up a bit but merging and releasing this work seems important for the ppx ecosystem so I'll gladly to take care of it!

kit-ty-kate · 2024-01-04T13:30:00Z

@gasche could you add @NathanReb ? I thought i had the rights to do that but turns out i only have access to some of the settings and not this one (github maintainer vs. admin)

gasche · 2024-01-05T15:11:32Z

Done, and I made you @kit-ty-kate an Admin. Thanks!

NathanReb · 2024-03-06T14:07:46Z

@sim642 sorry for the long wait. I will work on getting this merged and will start reviewing it.

Are you still willing to work on this if there are changes to be made?

sim642 · 2024-03-06T14:40:13Z

Are you still willing to work on this if there are changes to be made?

Yes, that shouldn't be an issue.

NathanReb

This looks good! I only have one suggestion to make the attribute_get2 a bit easier to use and more readable!

src_plugins/create/ppx_deriving_create.cppo.ml

src_plugins/show/ppx_deriving_show.cppo.ml

Co-authored-by: Nathan Rebours <[email protected]>

NathanReb · 2024-03-07T18:14:32Z

Sorry again for the long delays and thanks for taking the time to get back to this @sim642. Really appreciate it!

Let's merge this!

CHANGES: * 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)

@NathanReb

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)

sim642 added 7 commits July 19, 2022 09:33

Register plugins directly with ppxlib

2e4db40

Add ppxlib-based with_path arg to show plugin

5cd7fa8

Remove unused ~options and ~path arguments from plugins

8e1e68f

Declare plugin attributes directly with ppxlib

1ff769b

Delegate to Ppxlib.Quoter

2c08bed

Fix typo org -> ord

9a46e10

Deprecate non-ppxlib derivers and attributes

4c9178c

sim642 commented Jul 19, 2022

View reviewed changes

kit-ty-kate requested a review from pitag-ha July 19, 2022 11:17

sim642 mentioned this pull request Jul 19, 2022

Port to ppxlib registration and attributes ocaml-ppx/ppx_deriving_yojson#149

Merged

sim642 added 2 commits July 19, 2022 21:04

Restore "derive"-prefixed extensions via custom extension

9171d9a

Use input_name from ppxlib for show deriver path

3359fea

panglesd reviewed Mar 21, 2023

View reviewed changes

panglesd approved these changes Mar 22, 2023

View reviewed changes

src_plugins/enum/ppx_deriving_enum.cppo.ml Outdated Show resolved Hide resolved

src/api/ppx_deriving.cppo.mli Outdated Show resolved Hide resolved

panglesd reviewed Mar 22, 2023

View reviewed changes

src_plugins/show/ppx_deriving_show.cppo.ml Outdated Show resolved Hide resolved

sim642 mentioned this pull request Mar 22, 2023

Make quoter let binding non-recursive ocaml-ppx/ppxlib#401

Merged

sim642 and others added 6 commits March 22, 2023 19:49

Adapt quoter to ppxlib 0.29.0

f9a1e63

Remove unused attrs argument in enum plugin

a69c7a2

Co-authored-by: panglesd <[email protected]>

Replace failwith with raise_errorf

8ad6cc4

Co-authored-by: panglesd <[email protected]>

Revert "Deprecate non-ppxlib derivers and attributes"

dc74b49

This reverts commit 4c9178c.

Update test TODO about optional

e4a900e

Add PR ocaml-ppx#263 to CHANGELOG

ccfa830

This was referenced Mar 23, 2023

Code_path.main_module_name is sensitive to line directives ocaml-ppx/ppxlib#403

Closed

Having flag attributes is cumbersome with current API ocaml-ppx/ppxlib#404

Closed

panglesd mentioned this pull request Mar 27, 2023

Update ppx_deriving.cppo.mli #268

Open

shonfeder mentioned this pull request Feb 24, 2024

Regression test for incorrect behavior when deriving on mutually recursive types #273

Closed

sim642 added 4 commits March 6, 2024 18:24

Use Ppxlib.Ast_pattern.ebool from ppxlib 0.30.0

d5df400

Use Ppxlib.Attribute.declare_flag from ppxlib 0.32.0

45ec862

Use Ppxlib.Attribute.has_flag from ppxlib 0.32.0 for simple cases

34004c2

Use Ppxlib.Attribute.has_flag from ppxlib 0.32.0 for binary cases

0f41f8f

sim642 mentioned this pull request Mar 7, 2024

Add missing cppo 1.1.0 lower bound #274

Merged

sim642 added 2 commits March 7, 2024 09:04

Inline has_flag variables

5b6fedc

Remove duplicate extension for map plugin

bb25c98

NathanReb reviewed Mar 7, 2024

View reviewed changes

src_plugins/create/ppx_deriving_create.cppo.ml Outdated Show resolved Hide resolved

src_plugins/create/ppx_deriving_create.cppo.ml Outdated Show resolved Hide resolved

src_plugins/show/ppx_deriving_show.cppo.ml Outdated Show resolved Hide resolved

Simplify label-ct attributes in create and make plugins

5b7e6f5

Co-authored-by: Nathan Rebours <[email protected]>

NathanReb merged commit 4b5b9ff into ocaml-ppx:master Mar 7, 2024
1 check passed

sim642 mentioned this pull request Mar 8, 2024

Add missing unreleased CHANGELOG entries #275

Merged

NathanReb mentioned this pull request Mar 12, 2024

[new release] ppx_deriving (5.3.0) ocaml/opam-repository#25458

Closed

NathanReb mentioned this pull request Apr 15, 2024

[new release] ppx_deriving (6.0.0) ocaml/opam-repository#25675

Closed

sim642 mentioned this pull request Oct 13, 2024

README is a ambiguous as to current best practices for an end user #289

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port standard plugins to ppxlib registration and attributes #263

Port standard plugins to ppxlib registration and attributes #263

sim642 commented Jul 19, 2022 •

edited

Loading

sim642 Jul 19, 2022

pitag-ha commented Sep 6, 2022

sim642 commented Mar 9, 2023

panglesd commented Mar 10, 2023

panglesd left a comment

sim642 commented Mar 21, 2023

panglesd left a comment

panglesd commented Mar 22, 2023

sim642 commented Mar 23, 2023

gasche commented Nov 5, 2023 •

edited

Loading

sim642 commented Nov 5, 2023

kit-ty-kate commented Nov 5, 2023

panglesd commented Nov 8, 2023

pitag-ha commented Nov 13, 2023

panglesd commented Dec 21, 2023

NathanReb commented Jan 4, 2024 •

edited

Loading

kit-ty-kate commented Jan 4, 2024

gasche commented Jan 5, 2024

NathanReb commented Mar 6, 2024

sim642 commented Mar 6, 2024

NathanReb left a comment

NathanReb commented Mar 7, 2024

Port standard plugins to ppxlib registration and attributes #263

Port standard plugins to ppxlib registration and attributes #263

Conversation

sim642 commented Jul 19, 2022 • edited Loading

TODO

sim642 Jul 19, 2022

Choose a reason for hiding this comment

pitag-ha commented Sep 6, 2022

sim642 commented Mar 9, 2023

panglesd commented Mar 10, 2023

panglesd left a comment

Choose a reason for hiding this comment

sim642 commented Mar 21, 2023

panglesd left a comment

Choose a reason for hiding this comment

panglesd commented Mar 22, 2023

sim642 commented Mar 23, 2023

gasche commented Nov 5, 2023 • edited Loading

sim642 commented Nov 5, 2023

kit-ty-kate commented Nov 5, 2023

panglesd commented Nov 8, 2023

pitag-ha commented Nov 13, 2023

panglesd commented Dec 21, 2023

NathanReb commented Jan 4, 2024 • edited Loading

kit-ty-kate commented Jan 4, 2024

gasche commented Jan 5, 2024

NathanReb commented Mar 6, 2024

sim642 commented Mar 6, 2024

NathanReb left a comment

Choose a reason for hiding this comment

NathanReb commented Mar 7, 2024

sim642 commented Jul 19, 2022 •

edited

Loading

gasche commented Nov 5, 2023 •

edited

Loading

NathanReb commented Jan 4, 2024 •

edited

Loading