adding new formatters can break older projects

[avsm]

In the ocaml-ci, we have a "lint fmt" step which probes the project metadata to determine the formatters to run over the project. This is primarily for dune files and ml[i] files via ocamlformat.

The problem arises when we try a dune build @fmt with ocamlformat installed but without a .ocamlformat file, at which point dune tries to invoke ocamlformat even if there is no way it can work:

File x.ml          
Warning: Ocamlformat disabled because [--enable-outside-detected-project] is not set and no [.ocamlformat] was found within the project (root: ../../../x)

I think there's an erroneous default in dune that makes this difficult to work around:

By default, formatting will be enabled for all languages and dialects present in the project that dune knows about.

This default means that projects have to specifically add (formatting disabled) to turn it off for OCaml code, but to enable it for dune files.

Wouldn't it be better to specifically version formatters used by a project, so that a future dune client understanding a new formatter wouldn't cause older format lint tests to fail? Specifically, this means flagging that ocamlformat is enabled specifically for a project. We could minimise breakage by taking the presence of an .ocamlformat file to mean (formatting enabled), and the absence to be (formatting disabled).

See ocurrent/ocaml-ci#224

[ghost]

The set of enabled formatters is tied to the (lang dune x,y) line, so there is no risk of breaking projects by adding support for new formatters. However, I agree with you that Dune calling ocamlformat when it is not enabled via an .ocamlformat file is not good. We should change that.

We could minimise breakage by taking the presence of an .ocamlformat file to mean (formatting enabled), and the absence to be (formatting disabled).

Something like this seems good. To be more precise, when the user doesn't explicitly enables ocamlformat in the dune-project file, we should only apply it in sub-trees where an .ocamlformat file is present. That should be simple to change given that we already look for .ocamlformat files to declare dependencies. @emillon I'm assigning this to you as you worked on this code in the past.

[craigfe]

It might be nice to consider an .ocamlformat file that contains a disable entry as being "not present" for the purposes of this check, so as to not require an ocamlformat binary to be installed for such projects. That would require Dune parsing OCamlformat's config format, which may not be worthwhile here (although that's what we're currently doing in OCaml-CI).

[ghost]

IIUC, there are two ways of disabling ocamlformat:

• by not having a .ocamlformat file
• by having a .ocamlformat file containing a disable entry

Is that correct?

[craigfe]

That is my understanding, but @emillon will know for certain as an OCamlformat maintainer. (I believe he's on vacation this week.)

[jberdine]

It is also possible to specify files that should be ignored by ocamlformat using .ocamlformat-ignore files. See e.g. https://github.com/ocaml-ppx/ocamlformat/blob/master/ocamlformat-help.txt#L32 .

Also, note that ocamlformat will act even without an .ocamlformat file if passed --enable-outside-detected-project, which is also sensitive to existence of .git, .hg and dune-project, see https://github.com/ocaml-ppx/ocamlformat/blob/master/ocamlformat-help.txt#L517 .

I appreciate that these things are all complications for interoperation with dune. I'm not sure what the best plan ought to be.

[ghost]

In general we want to support one workflow and support it well rather than try to support every possible combination. I propose to simply update the Dune documentation here to clarify what is supported.

[jberdine]

Agreed. I think that --enable-outside-detected-project is clearly very context sensitive, and so ignoring it in the dune interop is the right thing. If dune calls ocamlformat with explicit specification of the root with --root (because dune must know where dune-project is), then there is no searching involved when looking for existence of the .ocamlformat file. I'm not sure or the current or planned situation, but making sure that dune passes --root seems like it would make the interoperation more robust.

When it comes to having a disable entry in .ocamlformat versus the .ocamlformat-ignore files, there is clearly redundancy. Historically, disable entries were put in .ocamlformat files to ignore entire subtrees of the file system, while .ocamlformat-ignore was used for more fine-grained selection within a single directory or project. It would perhaps be better to deprecate disable entries and generalize .ocamlformat-ignore if needed, in order to have only one mechanism. I guess that ideally the parsing and interpretation of these files could be more easily shared, as they are just sequences of globs matching relative path names. In particular, doing this might be progress toward dune not needing to look at the contents of the .ocamlformat files at all, just check their existence.

@gpetiot, what do you think?

[ghost]

I like the idea of Dune passing --root to make the interoperation more robust.

That + skipping ocamlformat in sub-tree without a .ocamlformat file should be enough to solve the issues raised in this thread. Having Dune understand .ocamlformat-ignore files would be nice as well, but it'd be only an optimisation at this point. i.e. to skip a few useless calls to ocamlformat.

[tmcgilchrist]

Bump @emillon any update on this?

[Alizter]

@tmcgilchrist You can enable formatting only for dune files: https://dune.readthedocs.io/en/stable/dune-files.html#formatting

[emillon]

This ticket can have a pretty wide scope so allow me to explain how this works today and if we agree on the problem we're trying to solve (before discussing potential solutions).

Let me describe the situation with formatting rules. There are 3 steps that happen when formatting a project:

1. configuration is read from dune-project
2. formatting rules are set up in the project
3. formatting tools are invoked and output formatted files

What happen in step 1 depends on the version of (lang dune) and the contents of a (formatting) stanza in there. Formatting can be enabled for each dialect (by default Dune knows about ocaml, using ocamlformat; and reason, using refmt) and also for dune files. The default is to format everything known to dune. If an ancient (I believe < 2.0) version of lang dune is used, nothng is set up by default. It can be configured through (formatting disabled) or (formatting (enabled_for ...)).

In step 2, we consider all files to be formatted. These correspond to all files with the extension corresponding to enabled dialects (by default *.ml *.mli *.re *.rei and dune). For each of these files, we set up a formatting rule: a rule attached to the @fmt alias that will run a formatting tool and diff the result, offering a promotion.

Step 3 happens when we call dune build @fmt or dune fmt: by asking the @fmt alias to be built, all the rules set up in the previous step are executed. Formatting tools like ocamlformat are looked up in the workspace (so that it works if they are vendored; or to format ocamlformat itself) or in PATH. ocamlformat will then look for, and read its configuration. It will error if .ocamlformat is not found or if a different version is requested. It is possible that the formatting tool, based on its own configuration, will do nothing; in that case no diff is found and no promotion is offered.

My understanding is that in the context of ocaml-ci you're trying to answer 2 questions:

• when should we create a formatting job?
• what are the opam prerequisites to this formatting job?

Is this correct?