Added ?warning flag to Deriving.Generator.make to enable warnings in derived code

[ceastlund]

Adds the ability for ppx derivers to opt in to unused code warnings, which ppxlib would otherwise disable for derived code. This will help, over time, clean up unused code from derivers.

We made this property opt-in because currently, many ppxes are not compatible with these warnings. For example, ppx_hash derives hash and hash_fold_t. If one were used and not the other, there's no way to derive "half" of the ppx. So we can't opt ppx_hash in until we have a way to request it piecemeal. Over time, we'll have to make more ppxes compatible with this option in order to clean up unused code.

[ceastlund]

@pitag-ha and @panglesd, this is part of an intern project here at janestreet by @jacksonzou123. Let us know how this looks. We should also think how upgrading will go for external clients as we opt ppxes into this. I mostly think it won't be that hard, because manually silencing warnings is not hard if people need to.

[panglesd]

Code looks good to me, with minor comments (and the need to format with the right version/profile of ocamlformat)!

I'm curious about the reason for willing to restrict the generated code.
Is it to improve compile time performances?

(I expect the compiler to remove unused values anyway, so this should not affect runtime performances).

[panglesd]

This test is to avoid adding removing warnings in an empty structure, in case no code is generated, such as in:

include struct [@@@ocaml.warning "-32"] end[@@ocaml.doc "@inline"][@@merlin.hide]

That was not the original behaviour: the useless inline struct was added in any case. So this is an unrelated bonus feature from this PR!

However, those chains of shadowing can be hard to read: we now have three wrap_sig and wrap_str one after the other, and we don't know which function does what.
Maybe you could add at least a comment on what are the invariant the function expect, and what it does?
For instance, the last wrap_sig function is handling the empty case, and the middle wrap_sig invariant is that it takes a non-empty signature.

[ceastlund]

I'm curious about the reason for willing to restrict the generated code.
Is it to improve compile time performances?

I don't understand what you're asking here, @panglesd.

[ceastlund]

If you mean, why do we want to remove unused code, then yes, it's for compilation performance. It's also to restrict final binary size, which can even improve runtime performance.

[pitag-ha]

We should also think how upgrading will go for external clients as we opt ppxes into this. I mostly think it won't be that hard, because manually silencing warnings is not hard if people need to.

Do I understand correctly that you're planning to opt the public releases of the Jane Street derivers into this? Have you had a look at how big the impact would be?

[ceastlund]

No we haven't. We can leave it off in the public release if need be. But probably we're not the only ones who want the benefit.

[ceastlund]

It's definitely worth some thought about how we proceed with this rollout, though.

[pitag-ha]

But probably we're not the only ones who want the benefit.

Yes, I definitely agree. Still, I'd expect quite a widespread breakage, both inside the opam-repo and for other users. Let's indeed give this some thought.

Two of the first things we can do when the time comes are get a clearer idea of the breakage on the opam-repo by running a health-check, and get an impression on what community members think about this by asking on discuss. Also, I'll keep this change in mind for the release highlights on the ocaml.org changelog for the next release to already give this change some visibility.