Change construct generated by driver to silence warning 34

[mbarbin]

Change the construct generated by the ppx driver to silence warning 34:

-|  let _ = fun (_ : t) -> ()
+|  let _ = (`None : [ `None | `Some of t ])

The new one works better with coverage tools such as bisect_ppx since it avoids generating an unvisitable coverage point. Discussed in #473.

[mbarbin]

There is an ongoing plan to add a new option to entirely remove this construct in #493 , so perhaps we don't need this change. I am however putting this PR for us to consider in case we are not sure how much time it could take to make progress on this. Arguably, there's still a case that can be made about fixing the issue of unvisitable coverage points if this is done in a way that doesn't require any change at all on the PPX user side.

Using #473 will require the user to add a new flag to every dune files in a project, which, for large project may cause some friction. This can be discussed with the dune devs as a separate question. The benefits of this change is that upon installing a ppxlib with this change, the issue of #473 would be fixed without further changes. cc @NathanReb for your consideration.

[mbarbin]

Action items for me:

• Rebase on top of Make -unused-code-warning tests actual tests #492
• Make the changelog commit respect the DCO
• Test the PR with a project to confirm this actually solves the coverage issue

[NathanReb]

I agree there's going to be a usability issue with the new warning related flags. I was indeed planning on reaching out to the dune team to see if we could add a global ppxlib driver config.

[mbarbin]

I talked only about the flags here ocaml/dune#10601 but now that you are saying it, a more complete config would probably work best.

For example, one issue I ran with earlier when trying to add the flag -deriving-keep-w32 systematically was that in one instance, the list of ppx configured in a specific dune file were not deriving, but I'm guessing another kind of ppx, and thus the instance of the ppx "handler" (I don't know how to call it ?) looks to be of a different nature than the "./driver.exe" from the tests, and thus did not recognize the flag. I can try to be more specific if this rings a bell.

[NathanReb]

I talked only about the flags here ocaml/dune#10601 but now that you are saying it, a more complete config would probably work best.

This is exactly what I had in mind!

The "handler" is simply called the driver. Yes indeed I recall not always seeing those flags in the driver's --help output. It's probably worth looking into it. Do you have a working example of this?

[mbarbin]

The "handler" is simply called the driver. Yes indeed I recall not always seeing those flags in the driver's --help output. It's probably worth looking into it. Do you have a working example of this?

I created two PRs where I make use of the new flag unused-type-warnings, one where it works, and one that exhibits the issue I am talking about, if you want to have a look.

• ✅ Unused type warning mbarbin/error-log#5
• ❌ Unused type warning mbarbin/letfun#1

[NathanReb]

If that's fine by you, I think we can close this PR as we'll instead be going for a flag to disable the feature entirely.

[mbarbin]

@NathanReb do you think the next ppxlib release will ship with the flag? Do you think it is likely to become the default that the unused-type-warnings be activated? I am having second thoughts about not changing the construct, perhaps it'll help smooth things out to in fact merge this change. Do you foresee any potential issue with it?

[NathanReb]

The flag should make it to the next release which should happen this week or the next. We can't disable it by default just yet unfortunately, some users still rely on this.

I'm happy to include a change to the item if you'd prefer that to the flag though.

[mbarbin]

I'm happy to include a change to the item if you'd prefer that to the flag though.

I would not want to discourage you to add the flag, because I think it's cleaner to remove the construct entirely, and I think that having the option to is desirable.

We can't disable it by default just yet unfortunately, some users still rely on this

I guess I'm more asking about longer term, about what you are going to communicate to users regarding whether you intend a future push to change the default behavior.

The flag should make it to the next release which should happen this week or the next.

That timeline sounds amazing to me. I'll close this PR then. Thanks a lot!

[mbarbin]

Closing again, to be alleviated by #511.