smlfmt icon indicating copy to clipboard operation
smlfmt copied to clipboard

Infix style with large expressions

Open HarrisonGrodin opened this issue 4 years ago • 1 comments

Currently, this is produced by the autoformatter:

val _ =
  f x + case g x of
          nil => 0
        | x :: xs => x + List.length xs

I'm not sure if it should change, though.

One principle I'm thinking we might want to adopt is:

Modifying code should not move independent code.

This would help modification to formatted code maintain a diff primarily relevant to the code which was actually modified.

Using the current style, modifying the expression f x (e.g., turning it into (f x * 2), or separately renaming f to fLongerName) would move the entire case expression forward, which seems somewhat undesirable since the case branches haven't changed at all. One idea would be to have the style be

e1
+ e2

which may be grouped if possible; this is consistent with how other infix operators are used (such as |>), and it makes sure the infix operator doesn't get lost at the end of a line.

One downside to this style is that renaming the infix operator itself could move blocks of code; not sure if there's a good fix to this aside from

e1
+
e2

which seems a bit verbose.

HarrisonGrodin avatar Dec 26 '21 21:12 HarrisonGrodin

Revisiting this now that smlfmt is quite a bit more mature.

Re: don't move independent code? I think in practice this is really really difficult to accomplish without making huge sacrifices to average code readability.

As for our current handling of infix expressions, I think it's generally pretty good, but could use some improvements. Here's the current output for the above example:

val _ =
  f x
  +
  case g x of
    nil => 0
  | x :: xs => x + List.length xs

Here's also a real-world example from the smlfmt source, showing a fairly complex expression with multiple occurrences of an infix operator <&>: https://github.com/shwestrick/smlfmt/blob/ee6a59df2470678b7459a0a0fdf7047de323b7c5/src/ast/CompareAst.sml#L78-L83

This generally seems pretty good, but I think it could be improved. For example, here's an instance I find tricky to parse because it's easy to miss the <&> at #delims ... buried in the middle: https://github.com/shwestrick/smlfmt/blob/ee6a59df2470678b7459a0a0fdf7047de323b7c5/src/ast/CompareAst.sml#L375-L381

I would prefer for it to be formatted like this.

val checker =
  at #exp equal_exp <&> at #handlee equal_tok
  <&>
  at #elems (Seq.equal
    (at #pat equal_pat <&> at #arrow equal_tok
     <&> at #exp equal_exp))
  <&> at #delims (Seq.equal equal_tok)  (* on its own line *)
  <&> at #optbar (equal_op equal_tok)

This seems doable. One idea to try: ensure that, as soon as an element of an infix-chain requires a new-line to align, then all later <op> <exp> ... elements of the chain will be put on their own new-line as well.

shwestrick avatar Jan 23 '23 16:01 shwestrick