Unused-type declaration warning and bisect_ppx coverage points about ppxlib HOT 22 OPEN

Got it, thanks so much! If down the line you'd like me to check out a ppxlib built from a PR or something like this, please let me know - I'm happy to help with the testing if you'd like. Thank you!

from ppxlib.

One thing we could consider here if we can't rid ourselves of this would be to instead generate:

[@@@coverage off]
let _ = fun (_ : t) -> ()
[@@@coverage on]

Always generating this could lead to unused warnings in some cases so maybe we could find a way to only produce those if ppx_bisect is linked with the driver.

I'm not saying this is how we should solve this but it definitely is an intermediate solution to consider!

from ppxlib.

Hello 👋 There was a change as part of Ppxlib.0.31.0 that allows disabling the silencing of warnings such as warning 34. That disabling has to be done both on the PPX author and on the PPX user side. See ppxlib PRs #444 (for the PPX user side) and #440 (for the PPX author side).

I'd expect that when disabling the silencing of warnings on both sides, ppxlib doesn't add the line you're talking about. Does anyone here have a moment to check if that's true? E.g. a simple cram test would be amazing. Btw, let me know if you have questions, as I'm not sure how clear my message is.

from ppxlib.

Thank you @panglesd I found one that is resilient to any scope change, inspired by your empty list container construct:

let _ = (`None : [ `None | `Some of t ])

I am experimenting with this idea in #495

from ppxlib.

Thanks for your report!

The let _ = fun (_ : t) -> () line is added by ppxlib in this function.

The reason for it is to remove any Error (warning 34 [unused-type-declaration]): unused type t if the type is not used.

I guess that the reason we do that is that the type is, in some ways, used: if not by OCaml, it is used by ppxlib to generate some code.

In my opinion, this could be removed: the deriving should not "modify" the type it derives from, which includes its "usedness". If defining a type only for the functions it will generate, one can use an extension node type%ext_name t = .... but I might be missing something!

from ppxlib.

Hi @panglesd,

Thanks for pinpointing the code and shedding light on the rationale behind it 😃. Your explanation makes a lot of sense!

I've been pondering over this now and I'm wondering if you'd consider replacing the current mechanism with a directive attached to the type definition to suppress the warning. Here's what I'm thinking (thanks for providing the warning number by the way!):

module Blah = struct
  type t = G [@@deriving_inline sexp_of] [@@warning "-34"]

  let sexp_of_t = (fun G -> Sexplib0.Sexp.Atom "G" : t -> Sexplib0.Sexp.t)
  let _ = sexp_of_t

  [@@@end]
end

This would effectively remove the let _ = fun (_ : t) -> () construct. Do you think this would achieve the same effect? I know this directive, as written by the user, does eliminate the warning, but I'm not entirely sure about the order of the compilation pipeline. I'm guessing that the ppx transform happens before the compiler pass that would utilize the directive. Would you agree? Does it work with all OCaml versions that are aimed to be supported by ppxlib?

I also understand what you're saying about the "usedness". The proposal above is primarily aimed at providing an immediate solution to enhance the bisect_ppx experience, without putting ppxlib in a tight spot. This would leave you with the freedom to pursue the removal of the directive entirely as a separate endeavor.

Looking forward to hearing your thoughts!

from ppxlib.

First of all, let me answer the direct questions:

Do you think this would achieve the same effect?
I'm guessing that the ppx transform happens before the compiler pass that would utilize the directive. Would you agree?
Does it work with all OCaml versions that are aimed to be supported by ppxlib?

Yes, this would have the same effect. Indeed, the checks on those things happen after the PPXs are run on the AST. And (without checking) I would be very surprised if it would not work with all versions ppxlib supports.

However:

I'm wondering if you'd consider replacing the current mechanism with a directive attached to the type definition to suppress the warning.

I'm not sure that's something we would want: it would modify the node on which we apply the deriving. But deriving from a type should not modify it, if possible.
What I would prefer is that we simply remove the let _ = fun (_ : t) -> () and let the warning be triggered if the type is not used: I can't see a case where this warning is not desirable! However, as often in ppxlib, the code has been written by someone else which might (or might not) have had very good reasons to remove this warning, but that's not documented...

from ppxlib.

Hello @panglesd,

Thank you for responding to all my questions. I can only imagine the complexities involved in maintaining ppxlib, a package that plays such a pivotal role in the ecosystem. I appreciate your work.

I'm curious, do you have any tools or methodologies to gauge the impact of a new ppxlib release candidate on the rest of the opam packages? Something like ocaml-ci, perhaps? How do you build confidence when implementing changes that could potentially affect users?

From a user's perspective, I agree with your default stance. I would find it beneficial to receive warnings for unused types, allowing me to decide the best course of action, whether that's removing dead code, silencing the warning with a directive, or refactoring.

I can also empathize with users who might be overwhelmed by a sudden influx of new warnings when attempting to upgrade a large code base to this new hypothetical version of ppxlib.

I'm guessing @ceastlund might have valuable insights to contribute to this discussion.

On a related note, I wanted to mention that the platform-roadmap # W21 includes language about coverage reports. I suspect that this issue could arise regardless of the coverage instrumentation tool used, which might further justify dedicating time to address this in the next 2 years, considering its inclusion in the roadmap.

from ppxlib.

I can't tell why without digging further but I seem to recall cases where silencing this warning was actually extremely useful.

I'll try to look into it but I wouldn't assume that this was added lightly by the previous maintainers.

from ppxlib.

Hi @NathanReb,

Great to hear you're looking into this 😃. Perfect timing - I've been contemplating updating this ticket recently:

In my continued use of bisect_ppx, I've found myself resorting to a workaround in new code. Specifically, I've been transforming all instances of:

type t [@@deriving ...]

to:

module T0 = struct
  [@@@coverage off]
  type t [@@deriving ...]
end
include T0

It's a bit of a hack, but I decided that it was worth it to me due to the value I derive from closely monitoring bisect_ppx coverage results. Still, it's quite intrusive and I'd be very grateful if you could solve this issue!

If silencing is really useful, what do you think about the [@@warning "-34"] approach I mentioned before?

from ppxlib.

I agree with @panglesd on the warning matter and would prefer not to modify the type declaration itself.

I need to better understand why this was introduced to come up with the right fix I think.

I thought there was a flag you could pass to the driver to prevent it from generating this but it turns out I'm wrong. This is also something we could potentially add to solve your problem.

from ppxlib.

Hello @pitag-ha, and thank you for joining the conversation!

I appreciate you pointing out the change that allows disabling the silencing of warnings and linking the two PRs. I wasn't aware of this change, and I find it interesting.

To activate this new feature, my understanding of the opt-in process is as follows:

PPX user side

This involves adding a flag to the ppx configuration in the dune file of your library. For example:

(preprocess
  (pps ... -unused-code-warnings=true))

PPX author

This involves supplying Deriving.Generator.make with the ~unused_code_warnings:true argument in the ppx implementation.

My take for this ticket

I've spent some time looking at the mentioned PRs and trying to understand their implications for this ticket. Please note that I'm not very familiar with ppxlib, so I'd appreciate it if you could verify my understanding.

I agree that if the goal is to completely disable the silencing of warnings, the opt-in must be done both on the PPX author side and the PPX user side. If the ppx you're using has already opted in on the author side, adding the flag to your dune file will allow you to complete the opt-in for your library.

I also agree that disabling the silencing of warnings should suppress the generation of the line let _ = fun (_ : t) -> (). I'm not sure why you're asking about a separate test for this. Shouldn't this behavior already be covered by the tests for the -unused-code-warnings=true flag, which were included in the PR that introduced this flag?

However, as of v0.17.0, I don't think that the ppx-es that I use from ppx_jane are ready for this. For example, I looked at the implementation of [@@deriving sexp_of]. Moreover, for some ppx, this might require more work. In #440 there was a mention that many ppxes are currently not compatible with these warnings.

Perhaps, the disabling of warnings has more to do with the values generated, whereas the let _ = fun (_ : t) -> () is more about the type, and maybe they are related, but slightly distinguishable questions?

It'd be great if we could make some progress on this issue, including with ppx for which more work will be required for them to be compatible with the warnings (such as ppx_hash). However, requiring the opt-in on the PPX author side makes it challenging for users of bisect_ppx, as it might require opening PRs in many places. I'd be grateful for a solution that only requires changes on the PPX user side.

[@@@coverage off]
let _ = fun (_ : t) -> ()
[@@@coverage on]

The latest proposal by @NathanReb seems like a pragmatic workaround while we wait for ~unused_code_warnings:true to be adopted by more PPX authors. I'd be perfectly fine if this change was guarded by a flag provided in the dune file, making this opt-in very explicit.

from ppxlib.

[@@@coverage off]
let _ = fun (_ : t) -> ()
[@@@coverage on]

Perhaps this could be tweaked slightly, e.g. to:

let _ = (fun (_ : t) -> ()) [@@coverage off]

as I am afraid of what the former would do if inserted in a section that already disabled coverage at toplevel (I am specifically concern whether the last [@@@coverage on] would re-enable the coverage!)

from ppxlib.

I made an experiment in draft #490 (perhaps a case could be made for this flag, I'm not sure? Do you have plans to make ~unused_code_warnings:true the default in the Deriving API for ppx authors eventually?)

At any rate, the test in this PR would seem to indicate that:

1. the fun (_ : t) -> () is generated even when -force-unused-code-warnings is supplied (Should we fix that?)
2. it is generated only for type aliases in this test. I had missed this detail previously. Perhaps that can be taken into consideration somehow?

from ppxlib.

To help replicating my reasoning about the draft, if :

1. you were to be onboard with a flag such as the one in #490 , and
2. you believe we could make such flag remove the fun (_ : t) -> () construct

then, (unless activating it makes me die by a 1,000 cut of influx of warnings in my project), I'd be happy to switch to using such flag, and can close this issue, as resolved by this. In some way, given that I already pay attention to coverage of generated ppx code, my intuition is that I may as well activate the warnings too. I'll wait for your comments before thinking about this further. Thanks a lot!

from ppxlib.

I took a quick look and the features added in #440 and #444 and they do not control the unused-type-decl countermeasure that @mbarbin mentions here.

They do come from a similar place and have a similar intent I guess but still cover different cases and I'm not sure they should be linked to the same control flags.
I.e. #440 and #444 deal with unused code warnings, meaning they were there to deal with ppx-es that derive a set of modules and values all together, some of which users might not be interested in. Given these ppx-es don't necessarily allow to derive only a selected subset, a feature was added to silence those warnings for generated code.

The let _ = fun (_ : t) -> () feature is here to prevent an unused type declaration warning, meaning it's here to deal with ppx-es that derive values that neither consume nor produce values of the type they are derived from. E.g. I imagine something like:

type t = ... [@@deriving id]

expanded to:

type t = ...

let id = 7

In such a case, if the type is never used by user code, it would (legitimately) trigger warning 34 "unused type declaration". I assume such ppx-es have been used with types that served no other purpose than being fed to the deriver and led to the addition of this feature. I believe the proper way to deal with this in a ppx would be to add an extension point for such use cases, turning the above example into:

[%%derive_id type t = ...]

or to have the users disable the warning themselves when they do not use the types.

Having a bit more insight on where this feature comes from would be helpful to better understand how to fix it. Maybe @ceastlund knows more about its origins.

That being said, the feature is here and we'll need a mechanism to fix this issue. I feel like we need to provide a way to smoothly transition out from those warning disabling features so adding something similar to #440 and #444 for this is probably desirable. We can also, as a separate step, look into how to exclude that item from bisect_ppx's coverage as, for as long as we will generate this item, be it necessary or not, it will always polute coverage reports otherwise.

Perhaps this could be tweaked slightly, e.g. to:

let _ = (fun (_ : t) -> ()) [@@coverage off]

as I am afraid of what the former would do if inserted in a section that already disabled coverage at toplevel (I am specifically concern whether the last [@@@coverage on] would re-enable the coverage!)

@mbarbin I would also much prefer this solution but completely missed it from bisect_ppx's documentation!

I'll work on disabling coverage for this item and will ping you on the PR so you can give it a spin!

from ppxlib.

I wanted to add a note regarding one specific takeaway from the meeting. Not that it matters, since we are considering going a different direction, but I was thinking back at a point that was made during the meeting that we could try resorting to an expression that uses the type, that doesn't generating unvisitable coverage points.

That is replacing: let _ = fun (_ : t) -> () by:

let _ : t = Obj.magic ()

or

let _ : (t, t) Type.eq = Type.Equal

I realized that neither were going to be acceptable in practice, because they rely on the current scope, and unfortunately ppxlib cannot make assumptions about it. For example, if you are using Base:

Alert deprecated: module Base.Obj
[2016-09] this element comes from the stdlib distributed with OCaml.
Referring to the stdlib directly is discouraged by Base. You should either
use the equivalent functionality offered by Base, or if you really want to
refer to the stdlib, use Stdlib.Obj instead

from ppxlib.

As it was pointed out to me, using -deriving-keep-w32={impl|both} removes the unvisitable construct, so I am going to try this flag a bit in some of my projects to get some experience of how many warnings this is going to trigger for me and whether they are of interest. I will report back when I get a chance to do that. This should give me some way to make progress on my side for the time being. Thank you!

from ppxlib.

I wanted to add a note regarding one specific takeaway from the meeting. Not that it matters, since we are considering going a different direction, but I was thinking back at a point that was made during the meeting that we could try resorting to an expression that uses the type, that doesn't generating unvisitable coverage points.

That is replacing: let _ = fun (_ : t) -> () by:

let _ : t = Obj.magic ()

or

let _ : (t, t) Type.eq = Type.Equal

Good idea! let _ : t list = [] would also work :)

I realized that neither were going to be acceptable in practice, because they rely on the current scope, and unfortunately ppxlib cannot make assumptions about it. For example, if you are using Base:

Alert deprecated: module Base.Obj
[2016-09] this element comes from the stdlib distributed with OCaml.
Referring to the stdlib directly is discouraged by Base. You should either
use the equivalent functionality offered by Base, or if you really want to
refer to the stdlib, use Stdlib.Obj instead

I think ppxlib already has a mechanism for accessing the standard lib, without those kind of problems. I'm not sure whether this is just relying on the fact that Stdlib is not shadowed, which is in my opinion a reasonable assumption, or something else.

let _ : t Stdlib.List.t = Stdlib.List.empty

from ppxlib.