Supporting project-wide ppx flags configuration

[mbarbin]

Desired Behavior

There are some interesting flags that can be supplied to ppx rewriters to tweak their behavior.

For context, a new one is under work here: https://github.com/ocaml-ppx/ppxlib/pull/493

More generally I'd be interested in trying some of the following:

  -deriving-keep-w32 {impl|intf|both}
                               Do not try to disable warning 32 for the generated code
  -deriving-disable-w32-method {code|attribute}
                               How to disable warning 32 for the generated code
  -deriving-keep-w60 {impl|intf|both}
                               Do not try to disable warning 60 for the generated code
  -unused-code-warnings _      Allow ppx derivers to enable unused code warnings
  -unused-type-warnings {true|false}
                               Allow unused type warnings for types with [@@deriving ...]

I don't mind adding them to all dune files in my project, but I thought that perhaps it could make sense to extend dune to allow for ppx flags to be configured more globally (project-wide).

I was thinking doing a parallel with other flags that do have such support via env such as c_flags or link_flags.

Example

Example in root dune file, something like:

(env
 (dev
  (ppx_flags "-deriving-keep-w32=impl")))

What do you think?

[rgrinberg]

Seems alright. No reason to not allow this for this specific set of flags.

[mbarbin]

I'll rephrase here something that came up in the linked issue that may be more relevant to this feature request:

When trying to add a ppx driver flag systematically to all dune files in a repo, I ran into an issue where some instances of ppx drivers do not accept the same set of flags as in other directories. I am guessing this depends on which ppx are enabled in the (preprocess (pps ...)) stanza.

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

• ✅ https://github.com/mbarbin/error-log/pull/5
• ❌ https://github.com/mbarbin/letfun/pull/1

If the automatic addition of the flags is implemented in dune directly, it would have to make sure it can distinguish the cases that look like the second pr above, and not supply the flags then.

Another option is to make the new stanza more like a config rather than a pure flag list, if things get too confusing. idk.

[rgrinberg]

it would have to make sure it can distinguish the cases that look like the second pr above, and not supply the flags then.

Yeah, that seems rather magical. If the set of flags depends on the preprocessors provided, then it's not really global and i'm not sure dune should help you here.

[mbarbin]

OK yes, I agree.

I'm not sure how many different ppx_driver kinds you can build with ppxlib (it could simply be a historical artifact). But you'd most likely want dune to be able to accommodate pps that are built with other libraries, not just ppxlib (is that still a thing?). On top of this (I would have to check but) I think it's possible for pps built with ppxlib to define their own custom flags.

Given all this, I still think there's a case for a feature that lets specific pps always be called a certain way within a project.

Here's a revised proposal for your consideration:

(env
 (dev
  (pps_flags
    (ppx_jane "-deriving-keep-w32=impl")
    ((ppx_john ppx_eve) "-some-john-and-eve-flag")
    (ppx_eve "-some-special-eve-flag"))))

The idea here is that this config would define a map from ppx-name -> flag list, and dune would resolve this, given a list of ppx-name found in the pps stanza, using find_multi |> concat |> dedup.

By the way, I'm using ppx_js_style via (lint (pps ...)) these days, and always invoke it with -check-doc-comments. If a pps_flags configuration (like the one above) existed for that ppx, maybe under this proposal we could get dune attach it to ppx_js_style whether it's used as a preprocess or a lint.

(env
 (dev
  (pps_flags
    ...
    (ppx_js_style "-check-doc-comments")
    ...)))

If you find this a bit too messy, I am also quite happy to keep exploring things on my side, and re-open this issue later if need be. Thank you!

[rgrinberg]

At first glance this seems like it's going to create you more work than it's ever going to save in the long run. Conceptually it's simple enough to understand for users though.

[mbarbin]

At first glance this seems like it's going to create you more work than it's ever going to save in the long run.

You may be right about that!

Besides, I'm currently working on a similar ppx_flag configuration in dunolint. It's possible that I might end up preferring a per-monorepo setup over the per-project one I proposed here, idk.

I'm inclined to cancel my feature request for now and reassess after making some progress with my setup. Sounds OK?

Thank you for your time and for the discussion.

[rgrinberg]

Nothing wrong with your original issue, so there's no reason to close it. It's your call though.

[rgrinberg]

I thought about this a little more, and I think that this type of issue comes up very often. If you don't mind, I'll attempt to generalize to the following use case "I would like to configure a large set of projects in a consistent way". While we accept one off features to help out with that, I think the real way to solve is a feature that is more ambitious. For example https://github.com/ocaml/dune/discussions/8707. If you think it would help, you are welcome to comment on it.

[mbarbin]

That generalization makes sense to me. In spirit, configuring a large set of projects in a consistent way is a good part of dunolint's motivations, and I have made that connection myself.

Thank you for pointing me to that dune Discussion, I wasn't aware.

[NathanReb]

Circling back to that, would fixing ppxlib's driver to provide a consistent set of flags, regardless of what ppx-es are being linked help toward having a simple variable to store ppx driver flags so one can set flags project wide?

We recently released ppxlib.0.33.0 and would like to encourage user to try the -unused-code-warnings flag but this is a bit hard to do atm. I even suspect most users don't even know they can pass flags via the (preprocess (pps ...)) field.

[mbarbin]

would fixing ppxlib's driver to provide a consistent set of flags, regardless of what ppx-es are being linked help toward having a simple variable to store ppx driver flags so one can set flags project wide?

I agree this would simplify project wide configuration indeed. That is true regardless of the way such ppx flags configuration is implemented by the way, that being in dune, or in a third party tool.

For what it is worth, I have made some progress with dunolint, and while I am able to assume the driver will support the flag I want in this particular config in other personal projects I sometimes have to restrict the application of the -unused-code-warnings=force flag to conditions like:

       (dune
        (and (has_field preprocess)
         (preprocess
          (pps
           (or (pp ppx_compare) (pp ppx_enumerate) (pp ppx_fields_conv)
            (pp ppx_hash) (pp ppx_quickcheck) (pp ppx_sexp_conv)
            (pp ppx_variants_conv)))))))

which I have to remember to update if I have new combinations of ppx (although unlikely because in practice I have at least ppx_compare or ppx_sexp_conv enabled).

All that being said, I now have something that works reliably for me and I am preparing to close this issue. I am no longer much convinced that doing something in dune itself is the way to go here.