Add driver flag to force opting in to unused code warnings in derived code

[mbarbin]

This is a draft PR that was prompted by the discussion in #473

In particular, I am interested to see what is the result of -unused-code-warnings=true without requiring the PPX author opt-in part of it.

[mbarbin]

By the way, something that didn't occur to me initially, if you prefer this can be merged into the existing flag, by making the cli ui -unused-code-warnings=force rather than adding a new flag.

Edit: I did that in the PR. changing the existing Arg.Bool into a Arg.Symbol. As far as I can tell from looking at the implementation of arg.ml in the stdlib, by using the symbols "true", "false", ... this makes for a new flag that is compatible with the former cli (as in, any existing use of -flag=true, -flag true, -flag=false, -flag false are all supported). I did try to supply just the flag -flag to a command set with Arg.Bool and this is complaining that "flag expects an argument", so this shouldn't be an issue. cc @NathanReb

[mbarbin]

Noting some action item for me to look into next on this PR:

• [x] Rebase on top of #492
• [x] Rebase on top of #493
• [x] Extract characterization tests for -deriving-keep-w32 into its own separate PR
• [x] Look into merging the value force into the existing flag, making sure not to break when the flag is passed with no value (look into how stdlib's arg works, Arg.Bool, Arg.String, etc.)

[mbarbin]

@NathanReb thanks for #492 ! I like your proposed new tests a lot, and found it makes adding tests for this PR easier.

By the way, do you like to use cram tests as characterization tests in the ppxlib project? If so, I'd like to propose to include to the new run.t examples characterizing the use of deriving-keep-w32 and how it combines with the existing -unused-code-warnings. I've added one section for it in this PR but if you'd like I can make this bit separate.

[NathanReb]

I think the -deriving-keep-w32 is indeed completely untested and characterization tests for it would be a welcome addition! test/deriving_warning/run.t also seems like the right place for such tests.

If it's not asking too much though, I would prefer if those were added in a separate PR from this one!

[NathanReb]

@mbarbin do you want to keep working on this? I'd like to include this into the next release!

I'm of course happy to take over if you're too busy at the moment!

[mbarbin]

Hi @NathanReb thanks so much for the heads up !

I am traveling at the moment and it would be difficult for me to allocate time to get it done before the release. I appreciate your offer to take over, and would be glad if you do for this time.

Looking at my action items, last time I checked:

• [x] Rebase on top of https://github.com/ocaml-ppx/ppxlib/pull/492
• [x] Rebase on top of Add an -unused-type-warnings flag to the driver #493

The first one was merged, but I noticed there was some formatting change further done in 493, thus I waited to be sure it would be included before rebasing onto it. I think the formatting change were minimal and shouldn't be an issue.

• [x] Remove characterization tests for -deriving-keep-w32 (to be later proposed as a separate PR)

You can simply delete the changes related to this, there's no time pressure on this. I'll simply resume this in a separate PR at a later point.

• [x] Look into merging the value force into the existing flag, making sure not to break when the flag is passed with no value (look into how stdlib's arg works, Arg.Bool, Arg.String, etc.)

I did this, and left some remark for you to review, with the understanding that adding a third value was backward compatible.

Again, thanks a lot. I appreciate your work on ppxlib!

[mbarbin]

@NathanReb One more thing while I am thinking about it : do as it is the most convenient for you, please do not worry about keeping or rebasing my commits, especially if it is just simpler to start over from a clean slate in another PR. I can close my draft and/or salvage things I'd like to extract later. Thank you!

[mbarbin]

Hi @NathanReb just the heads up I found some time to make progress on the action items and pushed an update.

[NathanReb]

Awesome, I'll take a look!

[NathanReb]

@mbarbin could you rebase on top of main? I'm actually thinking about dropping the -unused-type-warnings flag and instead disable all warnings through this single flag.

The type one is likely to be harmless to the vast majority of users so I'm not sure how worth it is to complexify this whole thing with yet another flag.

Some users want the same transition scenario than with the other warnings so it also allows to reuse the Deriving.make argument without modifying the API.

[mbarbin]

@NathanReb thank you for the review!

@mbarbin could you rebase on top of main? I'm actually thinking about dropping the -unused-type-warnings flag and instead disable all warnings through this single flag.

The latest update rebased on top of the -unused-type-warnings commit. I can create a version with these parts left off, however please read on.

The type one is likely to be harmless to the vast majority of users so I'm not sure how worth it is to complexify this whole thing with yet another flag.

I agree with the harmless part of the type one, however I am under the impression that disabling the code warning is likely to require more work on the user side of things. For example, I tried on my code base to use -deriving-keep-w32=both and noticed this would require some non trivial number of tweaks to get it adoptable (I'm thinking that unused-code-warnings=force is also going to require some similar work). I was hoping I would be able to close #473 with the next release of ppxlib, as an incremental step without having to go on the journey to solve all ppxlib related unused code warnings in my project just yet (I still hope to do this as future work!).

If you are thinking about dropping the ability to control the unused type warning in isolation, then, I would advocate for resurrecting the PR #495 that changes the nature of the code, so we can close #473.

Some users want the same transition scenario than with the other warnings so it also allows to reuse the Deriving.make argument without modifying the API.

This sounds interesting. Can you say a bit more about this, I'd like to understand this in more details.

Thanks a lot!

[mbarbin]

@mbarbin could you rebase on top of main?

@NathanReb I removed the part about -unused-type-warnings and rebased on top of main.

[NathanReb]

Ok I undestand your point about merging the two flags and I agree user will have an easier time disabling the unused type warning silencing than the other two.

How about making the -unused-code-warnings flag control all three of warnings 32, 34 and 60 silencing and adding a separate -unused-type-warnings flag that controls only the warning 34 one?

This sounds interesting. Can you say a bit more about this, I'd like to understand this in more details.

It's simply the feature where both the user and the derivers being used need to opt out of the warning silencing (except when force is passed of course) for it to be disabled, as you might have read in the tests suite. I think it'll be simpler to control all warnings from that same argument.

The idea is that users can pass the flag with true and it will initially have no impact. As they patch the derivers they use so that they opt out, the driver won't silence warnings for code generated for by those but will keep generating it for other derivers. This allow to quickly opt out for the ones that weren't causing any troubles while taking the time to fix the derivers that need changes in order not to cause unwanted warnings. Ultimately once all derivers have been fixed and have opted out, only the user decides whether to silence warnings or not. At that point, we can decide to change the default, both for the flag and for the Deriving.make argument (potentially in two separate steps) so that we don't silence warnings unless the user or deriver opt in.

[mbarbin]

How about making the -unused-code-warnings flag control all three of warnings 32, 34 and 60 silencing and adding a separate -unused-type-warnings flag that controls only the warning 34 one?

If I understand correctly, this is a slight twist to the earliest proposal that had -unused-code-warnings controls 32 & 60, and -unused-type-warnings controls 34. Instead, with this new proposal, -unused-code-warnings also controls 34.

• This allows users that desire to do that, to treat the type parts (34) as a first and independent step
• while allowing things to stay simple (one single flag) for those who want to treat all warnings (32, 34, 60) in the same step.

This sounds good to me!