Introduce ocamlformat

[matijapretnar]

I am suggesting here to use ocamlformat on our codebase for the following reasons:

• cleaner diffs & easier merges
• faster development (no need to think hard in order to follow the existing style of the code you are modifying)
• consistent style in general
• probably a bunch of other reasons that people repeatedly advertise about auto formatters
  There are of course disadvantages, but I'll leave them to those who are against.

I tried finding a configuration that would match our current style as much as possible, which was not easy since we were not consistent in the first place :) The main changes I've made with regard to the default ocamlformat configuration are:

• list sum variants on separate lines even if they all fit in a single one
• similar for top level match/function
• do not put spaces around lists, records, etc., so [1; 2; 3] instead of the default [ 1; 2; 3 ]
• keep begin/end that delimit nested matches (instead of using parentheses by default)
• do not further indent cases that do not fit in the same line as the pattern
  I am more than happy to revert any of those options to default.

You can see the resulting diff here: 84e0d97. The one change I am not entirely happy with is the formatting of comments describing sum variants (which BTW should be documentation comments, ie. (** ... *) instead of (* ... *).), which are no longer aligned, but I hope this is not a show-stopper.

The PR is currently marked as draft. If we decide to go ahead with it, I'll also add CI and precommit hooks. Otherwise, it's really easy to format the code, you just run dune fmt. In fact, it's so fast, we can run it on each make. Also, if we do it, I would suggest doing it sooner rather than later while we still have a small number of branches.

[rossberg]

I primarily checked only one larger file (render.ml). Most changes, I could probably live with, but there are several showstoppers for me:

• removal of multiple empty lines,
• odd and unhelpful layout of parens spanning multiple lines,
• change of layout of string concatenations, which is a readability killer in the rendering module,
• what it does to some if's,
• breaking of single-line datatypes, matches, expression sequences.

Also, this tool sometimes violates the "indent by multiples of 2" rule, e.g., when reformatting out (a; b). In fact, in one case below the indentation is negative, which looks like a straight-up bug to me.

A general problem I have with that auto-formatters is that they always follow a naive eager strategy and do not care for coherent layout across consecutive definitions/matches/branches/etc.

[rossberg]

Is it possible to prevent removal of empty lines? I intentionally use the convention of using two empty lines before the start of another code "section" in a file, in order to emphasise the structure.

[matijapretnar]

As far as I see, there is no way of doing this. There is no configuration option, and having doc comments (** ... *) or doc comment section headers (** {1 ...} *) doesn't change anything.

[rossberg]

Can type definitions with record r.h.s.'s be laid out in a manner that is consistent with value definitions of the same shape, such as the one below?

[matijapretnar]

Yes, ed9bcb8 formats both as

type record =
  { fld1 : typ1;
    fld2 : typ2 }

and

let r =
    { fld1 = v1;
      fld2 = v2 }

[matijapretnar]

This is now

type record =
  { fld1 : typ1;
    fld2 : typ2
  }

and

let r =
  { fld1 = v1;
    fld2 = v2
  }

due to 90b7a69

[rossberg]

This is a bit annoying.

[matijapretnar]

Does this refer to having a separate line for each constructor? We can write type declarations that fit in a single line compactly (for both sums and records). This is actually the default. Done in d134bc5.

[rossberg]

I'd much prefer if the closing paren was below the opening one.

[matijapretnar]

As far as I see, there is no option to do that.

[matijapretnar]

Found it! 90b7a69

[rossberg]

This also is rather annoying, turning a one-liner into 6-liner.

[matijapretnar]

This is again the default which I have overriden (I went with the approach that seemed more common in the existing code). Functions that fit on a single line now do so in 5ec0fe6.

[rossberg]

Is there an option to preserve absence/presence of tuple parens?

[matijapretnar]

No, one can only configure (separately for patterns and for expressions) if parentheses are present always or only when the tuple spreads over multiple lines.

[rossberg]

This change, like similar ones, is highly undesirable, as it makes the already hard to read code even less readable.

[matijapretnar]

8037caf breaks such list of expressions such that each case is in a separate line.

[rossberg]

Is there a wrap option here instead of going fully vertical?

[matijapretnar]

This is already done by 5ec0fe6, though it doesn't work in this particular case since the line is 130 characters wide. We can increase the margin if we want.

[rossberg]

What?

[matijapretnar]

I think this is because of a match, and couldn't find a way to change it.

[rossberg]

Not sure what's going on, but the indentation here seems buggy.

[matijapretnar]

I think this was fixed by 8037caf

[matijapretnar]

I have now changed what I could. The only major open issue is removal of multiple empty lines. Perhaps this can be remedied by having larger block comments such as

(************
 Section
 ************)

[rossberg]

Thanks for looking into this more. It's much improved, but I'm afraid my main gripes are still the uncalled-for changes:

• Empty lines: it's not just sections, I routinely use empty lines for grouping definitions. But the formatter bluntly removes all empty lines between similarly-formatted declarations, with absolutely no regards for logical program structure.

• Coherence: I very much want related consecutive definitions to use consistent layout, even if one of them just so happens to fit on a single line.

• Binary operators: the naive way of line-breaking binops is a pain, at least in the renderer. Now everything is vertical in most cases, but that is only marginally better in terms of logical structuring.

Of course, the fundamental problem is that handling these cases correctly requires understanding of the meaning of the code. A formatter can't understand the purpose of anything. So I wish it would at least leave these things alone.

Is it a possibility to set up the formatter on a by-directory basis? Why not let directory "owners" choose individually whether they want to trade style and readability for submission and automation?

[matijapretnar]

Perhaps that is the best option. Looking at the options, you can list the files in .ocamlformat-ignore. I think it also works for directories using either /* or /**.

[rossberg]

This issue was auto-closed by Github because its smart AI doesn't understand the concept of multiple repositories and its separate issue name spaces. Reopening.