Unused-type declaration warning and bisect_ppx coverage points

[mbarbin]

While using [@@deriving sexp_of] and [@@deriving equal], I've observed that an unused function is being generated:

module Blah = struct
  type t = G [@@deriving_inline sexp_of]

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

module Blih = struct
  type t = H [@@deriving_inline equal]

  let _ = fun (_ : t) -> ()
  let equal = (Stdlib.(=) : t -> t -> bool)
  let _ = equal
[@@@end]
end

The specific part I'm referring to is let _ = fun (_ : t) -> ().

Despite my efforts to identify the code generating this part within ppx_sexp_conv and ppx_equal, I've been unsuccessful. I'm wondering if it could be a part of ppxlib itself?

Would you be willing to assist me in locating the code that generates this binding, and perhaps help me understand its purpose?

My inquiry stems from the observation that this construct interacts with bisect_ppx. When this code is instrumented, a non-visitable coverage point is inserted, thereby reducing the reported code coverage and preventing it from reaching 100%.

Here's an example of how this code gets instrumented:

let _ = fun (_ : t) -> ___bisect_visit___ 0; ()

I would be interested in looking for a solution to this issue, but first, I think I'd need to understand the rationale behind the let _ = fun (_ : t) -> () construct.

Thank you for your time!

[panglesd]

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!

[mbarbin]

Hi @panglesd,

Thanks for pinpointing the code and shedding light on the rationale behind it :smiley:. 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!

[panglesd]

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

[mbarbin]

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.

[NathanReb]

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.

[mbarbin]

Hi @NathanReb,

Great to hear you're looking into this :smiley:. 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?

[NathanReb]

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.

[mbarbin]

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!