Add mangling utilities from ppx_deriving

[sim642]

Part of issue #317.

This is exactly as in ppx_deriving, just in a Mangle submodule for slightly better organization (like #95 went to Quoter).

ppx_show contains analogous port of these to ppxlib with the difference that it uses non-polymorphic variants and mangle_type_decl returns string loc instead of just string. Not sure if there's any preference to deviating/improving(?) from the ppx_deriving interface.

[sim642]

I'm surprised by the format CI failure:

ocamlformat: ignoring "test/mangle/test.ml" (syntax error)
File "test/mangle/test.ml", line 8, characters 0-6:
8 | Mangle.mangle (`Suffix "suf") "foo";;
    ^^^^^^
Error: Syntax error

It's perfectly fine syntax for building the tests themselves.

[pitag-ha]

Adding helpers for mangling sounds good, thanks for the PR! And thanks for all the nice work you've been doing on ppxlib and the PPX ecosystem on different levels!

About the formatting error: I guess the ocamlformat parser expects double semicolons also after the expect extension points. But the real problem is that we have a custom expect test which doesn't integrate very well with ocamlformat. Would you mind adding your new test file to .ocamlformat-ignore?

About polymorphic variants vs non-polymorphic variants for the type. I'm curious now: why have you chosen polymorphic ones? Is it to avoid having to open the module?

As an idea about the Mangle submoduel structure: what would you think about putting it inside a more general module? E.g. something like Expansion_helpers, which could also contain other utility functions such as the polymorphism handlers (we'd have to have a look at the common module though to see if there are already too many functions that would then be expected to be in the Expansion_helpers as well).

[sim642]

About polymorphic variants vs non-polymorphic variants for the type. I'm curious now: why have you chosen polymorphic ones? Is it to avoid having to open the module?

I didn't have a preference either way, so I just kept it like it was in ppx_deriving, but I'm open to changing it.
In this case the burden of non-polymorphic variants is probably minimal anyway: within a single deriver mangling is only used in a handful of places and type-directed constructor resolution would allow using them directly as the argument without module prefixing.

As an idea about the Mangle submoduel structure: what would you think about putting it inside a more general module? E.g. something like Expansion_helpers, which could also contain other utility functions such as the polymorphism handlers (we'd have to have a look at the common module though to see if there are already too many functions that would then be expected to be in the Expansion_helpers as well).

This is a good point, especially in the broader context of bringing remaining utilities over. Splitting them up too much isn't particularly useful either, given how in ppx_deriving they were all together at the top level anyway. And indeed, it would serve as a broader catch-all for other utilities that exist either in ppxlib's Common or ppx_deriving, which don't nicely belong to categories like the ones I listed in #317 originally.

[sim642]

I now implemented the suggested changes, thanks to the reminder from release.

[panglesd]

Thanks a lot for the PR, and sorry for the very long delay!

It looks good to me, but I still have some suggestions regarding the structure. I think the toplevel Ppxlib module is a bit large, making it not very readable (especially since there are not that much documentation), so adding an Expansion_helper module is a very good idea.
However, currently the Expansion_helper module would be confusing when reading the API: it only contains mangling-related functions, but does not contain other expansion helpers that can be found in other modules.
So, at some point we should move the expansion helpers of Ppxlib into the Expansion_helpers module. This can be tricky to do so while keeping retro-compatibility. An easy first step is to have Quoter inside Expansion_helper: from sherlocode/ppx_universe, it seems no one is using it yet.
We cannot really integrate Quoter inside Expansion_helpers with the current "flat" hierarchy: the name inside Quoter (type t, create) need to be inside a Quote module. So we need to wrap the mangling utilities of Expansion_helpers into Expansion_helpers.Mangle.

I opened a PR on your fork where I implemented the reorganization with Quoter inside Expansion_helper, since I tried it while reviewing: sim642#1

[panglesd]

I accidentally pushed my PR in this branch. I have reverted this, I prefer that you accept (or not) my suggested modifications on the PR on your fork.

[sim642]

An easy first step is to have Quoter inside Expansion_helper: from sherlocode/ppx_universe, it seems no one is using it yet.
We cannot really integrate Quoter inside Expansion_helpers with the current "flat" hierarchy: the name inside Quoter (type t, create) need to be inside a Quote module. So we need to wrap the mangling utilities of Expansion_helpers into Expansion_helpers.Mangle.

Well, if nothing is yet using the Quoter module, then it would still be possible to integrate its contents into Expansion_helpers as a flat hierarchy by renaming them to type quoter and val create_quoter. That is how ppx_deriving has them all together in one module.
Could even keep Ppxlib.Quoter.t, etc for backwards-compatibility by aliasing them to the Expansion_helpers versions individually.

Or could have Expansion_helpers.Quoter.t, etc and Expansion_helpers.mangle etc at different levels to avoid the redundancy in Expansion_helpers.Mangle.mangle.

But those are just a few thoughts. I personally don't have an issue with the fully nested structure you proposed. This just came to mind because @pitag-ha's idea above was to have one more general module rather than separate ones like Mangle, although with less nesting.

[panglesd]

Well, if nothing is yet using the Quoter module, then it would still be possible to integrate its contents into Expansion_helpers as a flat hierarchy by renaming them to type quoter and val create_quoter.

Hmm I think I got lost with backward compatibility... Of course if Quoter is not yet used, we can modify as we wish and have a flat structure.
I'm totally fine with a flat structure. I'll approve what you prefer (I agree that the redundancy in Mangle.mangle is not ideal!)

[sim642]

I agree that the redundancy in Mangle.mangle is not ideal!

Then the current state of this PR achieves the sensible approach with Expansion_helpers.mangle and I wouldn't change that.

Of course if Quoter is not yet used, we can modify as we wish and have a flat structure.

How about we leave any sort of move of Quoter out of this PR?
It it is to be moved, then I kind of like having a mixed structure with Expansion_helpers.Quoter.t because that groups everything related to the abstract quoter type nicely together, but there isn't such abstract type for mangling. I can open another PR with the move after this one and then that can be discussed further separately.

[panglesd]

Looks good to me! Thanks a lot.

I'll soon do a PR improving ppxlib's API landing page, I can do the Quoter move in this PR.

[pitag-ha]

Looks good to me as well!

@panglesd, is there a reason why you haven't merged the PR yet? (e.g. is there something else you'd like to merge first?) If not, let's go ahead and merge this PR today.

Also from my side: apologies for the strong delay on this, @sim642. We do appreciate this kind of PRs very much! Re-reviewing it just somehow slipped through.