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.