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

mbarbin · 2024-05-25T06:02:20Z

To quote the documentation recently added for the disable warning flags:

Ppxlib driver has a variety of ways to disable warnings that can be triggered when using [@@deriving ...]. These are all enabled by default but we added flags to let driver users disable them. To allow smooth transition from always adding them to never do so and let individual ppx-es do what they must to avoid triggering warnings, we also added optional arguments to Deriving.make so that the ppx themselves can declare whether they need the driver to disable warnings or not.

One such flag and optional argument pair is the -unused-code-warnings flag and ?unused_code_warning Deriving.V2.make argument. Both of those default to false and control whether we disable warnings 32 and 60 for generated code.

This PR adds a new value for the flag, in addition to true|false, it adds force which allows to force enabling unused code warnings even when the deriver arg was set to false. See the 2 new lines in the table documenting the combination of Deriver arg and Driver Flag:

 | Deriver arg | Driver Flag | Unused Code Warnings
 |-------------|-------------|----------------------
 | true        | true        | Enabled
 | true        | false       | Disabled*
+| true        | force       | Enabled
 | false       | true        | Disabled
 | false       | false       | Disabled
+| false       | force       | Enabled

Motivation:

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

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

CHANGES.md

mbarbin · 2024-05-27T07:47:07Z

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 · 2024-05-28T18:11:33Z

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

Rebase on top of Make -unused-code-warning tests actual tests #492
Rebase on top of Add an -unused-type-warnings flag to the driver #493
Extract characterization tests for -deriving-keep-w32 into its own separate PR
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 · 2024-05-29T06:07:40Z

@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 · 2024-05-29T07:50:32Z

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 · 2024-07-08T09:06:59Z

@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 · 2024-07-08T14:16:28Z

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:

Rebase on top of Make -unused-code-warning tests actual tests #492
Rebase on top of Add an -unused-type-warnings flag to the driver 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.

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.

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 · 2024-07-08T14:36:16Z

@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 · 2024-07-13T09:33:18Z

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

NathanReb · 2024-07-15T09:07:00Z

Awesome, I'll take a look!

NathanReb · 2024-07-15T09:10:18Z

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

NathanReb

The code looks good to me! Thanks for taking the time to add this feature!

Except for the Options submodule name this is good to go once we've rebased it on top of main. I'll work on adding the unused type warning control under this same flag!

src/options.ml

mbarbin · 2024-07-15T09:46:32Z

@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!

…n derived code - Add tests - Add new flag in doc - Update CHANGES.md Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Mathieu Barbin <[email protected]>

mbarbin · 2024-07-15T12:02:48Z

@mbarbin could you rebase on top of main?

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

NathanReb · 2024-07-15T14:43:53Z

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.

NathanReb

Looks good, thanks!

mbarbin · 2024-07-15T15:42:24Z

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!

@NathanReb

- Fix a bug where `Code_path.main_module_name` would not properly remove extensions from the filename and therefore return an invalid module name. (ocaml-ppx/ppxlib#512, @NathanReb) - Add `-unused-type-warnings` flag to the driver to allow users to disable only the generation of warning 34 silencing structure items when using `[@@deriving ...]` on type declarations. (ocaml-ppx/ppxlib#511, @mbarbin, @NathanReb) - Make `-unused-code-warnings` flag to the driver also controls the generation of warning 34 silencing structure items when using `[@@deriving ...]` on type declarations. (ocaml-ppx/ppxlib#510, @mbarbin, @NathanReb) - Driver: Add `-unused-code-warnings=force` command-line flag argument. (ocaml-ppx/ppxlib#490, @mbarbin) - new functions `Ast_builder.{e,p}list_tail` that take an extra tail expression/pattern argument parameter compared to `Ast_builder.{e,p}list`, so they can build ASTs like `a :: b :: c` instead of only `[ a; b ]`. (ocaml-ppx/ppxlib#498, ocaml-ppx/ppxlib#502, @v-gb, @NathanReb) - Fix `Longident.parse` so it also handles indexing operators such as `.!()`, `.%(;..)<-`, or `Vec.(.%())` (ocaml-ppx/ppxlib#494, @Octachron) - Add a `special_function'` variant which directly takes a `Longident.t` argument to avoid the issue that `Longident.t` cover distinct syntaxic classes which cannot be easily parsed by a common parser (ocaml-ppx/ppxlib#496, @Octachron). - Keep location ranges consistent when migrating `Pexp_function` nodes from 5.2+ to older versions (ocaml-ppx/ppxlib#504, @jchavarri) - Fix `-locations-check` behaviour so it is no longer required to pass `-check` as well to enable location checks. (ocaml-ppx/ppxlib#506, @NathanReb) Signed-off-by: Nathan Rebours <[email protected]>

@NathanReb

- Fix a bug where `Code_path.main_module_name` would not properly remove extensions from the filename and therefore return an invalid module name. (ocaml-ppx/ppxlib#512, @NathanReb) - Add `-unused-type-warnings` flag to the driver to allow users to disable only the generation of warning 34 silencing structure items when using `[@@deriving ...]` on type declarations. (ocaml-ppx/ppxlib#511, @mbarbin, @NathanReb) - Make `-unused-code-warnings` flag to the driver also controls the generation of warning 34 silencing structure items when using `[@@deriving ...]` on type declarations. (ocaml-ppx/ppxlib#510, @mbarbin, @NathanReb) - Driver: Add `-unused-code-warnings=force` command-line flag argument. (ocaml-ppx/ppxlib#490, @mbarbin) - new functions `Ast_builder.{e,p}list_tail` that take an extra tail expression/pattern argument parameter compared to `Ast_builder.{e,p}list`, so they can build ASTs like `a :: b :: c` instead of only `[ a; b ]`. (ocaml-ppx/ppxlib#498, ocaml-ppx/ppxlib#502, @v-gb, @NathanReb) - Fix `Longident.parse` so it also handles indexing operators such as `.!()`, `.%(;..)<-`, or `Vec.(.%())` (ocaml-ppx/ppxlib#494, @Octachron) - Add a `special_function'` variant which directly takes a `Longident.t` argument to avoid the issue that `Longident.t` cover distinct syntaxic classes which cannot be easily parsed by a common parser (ocaml-ppx/ppxlib#496, @Octachron). - Keep location ranges consistent when migrating `Pexp_function` nodes from 5.2+ to older versions (ocaml-ppx/ppxlib#504, @jchavarri) - Fix `-locations-check` behaviour so it is no longer required to pass `-check` as well to enable location checks. (ocaml-ppx/ppxlib#506, @NathanReb) Signed-off-by: Nathan Rebours <[email protected]>

github-actions bot reviewed May 25, 2024

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

mbarbin mentioned this pull request May 25, 2024

Unused-type declaration warning and bisect_ppx coverage points #473

Closed

mbarbin force-pushed the force-unused-code-warnings branch from 3e50763 to 395b6ef Compare May 27, 2024 10:52

mbarbin force-pushed the force-unused-code-warnings branch from 395b6ef to 9ddfd49 Compare May 29, 2024 06:02

mbarbin force-pushed the force-unused-code-warnings branch from 9ddfd49 to 022c671 Compare July 13, 2024 09:31

mbarbin marked this pull request as ready for review July 13, 2024 09:45

mbarbin requested review from ceastlund, NathanReb, panglesd, patricoferris and pitag-ha as code owners July 13, 2024 09:45

NathanReb reviewed Jul 15, 2024

View reviewed changes

src/options.ml Outdated Show resolved Hide resolved

mbarbin force-pushed the force-unused-code-warnings branch from bed8100 to c0b1b82 Compare July 15, 2024 11:14

mbarbin requested a review from NathanReb July 15, 2024 11:31

Add driver flag argument to force opting in to unused code warnings i…

489daff

…n derived code - Add tests - Add new flag in doc - Update CHANGES.md Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Mathieu Barbin <[email protected]>

mbarbin force-pushed the force-unused-code-warnings branch from c0b1b82 to 489daff Compare July 15, 2024 12:00

NathanReb approved these changes Jul 15, 2024

View reviewed changes

NathanReb merged commit 4aadc5f into ocaml-ppx:main Jul 15, 2024
6 checks passed

This was referenced Jul 15, 2024

Make -unused-code-warnings also controls warning 34 suppression items #510

Merged

Add an -unused-type-warnings flag to the driver (bis) #511

Merged

NathanReb mentioned this pull request Jul 22, 2024

[new release] ppxlib.0.33.0 ocaml/opam-repository#26251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mbarbin commented May 25, 2024 •

edited

Loading

mbarbin commented May 27, 2024 •

edited

Loading

mbarbin commented May 28, 2024 •

edited

Loading

mbarbin commented May 29, 2024 •

edited

Loading

NathanReb commented May 29, 2024

NathanReb commented Jul 8, 2024

mbarbin commented Jul 8, 2024 •

edited

Loading

mbarbin commented Jul 8, 2024

mbarbin commented Jul 13, 2024

NathanReb commented Jul 15, 2024

NathanReb commented Jul 15, 2024

NathanReb left a comment

mbarbin commented Jul 15, 2024 •

edited

Loading

mbarbin commented Jul 15, 2024

NathanReb commented Jul 15, 2024

NathanReb left a comment

mbarbin commented Jul 15, 2024 •

edited

Loading

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

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

Conversation

mbarbin commented May 25, 2024 • edited Loading

mbarbin commented May 27, 2024 • edited Loading

mbarbin commented May 28, 2024 • edited Loading

mbarbin commented May 29, 2024 • edited Loading

NathanReb commented May 29, 2024

NathanReb commented Jul 8, 2024

mbarbin commented Jul 8, 2024 • edited Loading

mbarbin commented Jul 8, 2024

mbarbin commented Jul 13, 2024

NathanReb commented Jul 15, 2024

NathanReb commented Jul 15, 2024

NathanReb left a comment

Choose a reason for hiding this comment

mbarbin commented Jul 15, 2024 • edited Loading

mbarbin commented Jul 15, 2024

NathanReb commented Jul 15, 2024

NathanReb left a comment

Choose a reason for hiding this comment

mbarbin commented Jul 15, 2024 • edited Loading

mbarbin commented May 25, 2024 •

edited

Loading

mbarbin commented May 27, 2024 •

edited

Loading

mbarbin commented May 28, 2024 •

edited

Loading

mbarbin commented May 29, 2024 •

edited

Loading

mbarbin commented Jul 8, 2024 •

edited

Loading

mbarbin commented Jul 15, 2024 •

edited

Loading

mbarbin commented Jul 15, 2024 •

edited

Loading