ocaml-ppx / ocamlformat

Auto-formatter for OCaml code
MIT License
630 stars 180 forks source link

Style suggestion: support infix operators at end of line #993

Open craigfe opened 5 years ago

craigfe commented 5 years ago

What is the reason for this new style? This is a follow-on to the discussion in https://github.com/ocaml-ppx/ocamlformat/issues/961. The outcome of that is that OCamlformat is now somewhat opinionated about the use of binds: it tries to put them at the start of the line. Given the following input:

let configure i =
  configure_myocamlbuild () >>=
  (* comment *)
  configure_opam ~name:opam_name i >>= fun () ->
  let no_depext = Key.(get ctx no_depext) in
  configure_makefile ~no_depext ~opam_name >>= fun result ->
  foo

Ocamlformat will now output this:

let configure i =
  configure_myocamlbuild ()
  >>= (* comment *)
  configure_opam ~name:opam_name i
  >>= fun () ->
  let no_depext = Key.(get ctx no_depext) in
  configure_makefile ~no_depext ~opam_name >>= fun result -> foo

Personally, I like the operators being at the start; but there are good reasons for wanting them at the end instead. In particular: let+ compatibility. The original input snippet (somewhat taken from the Mirage codebase) could easily be converted to let+ syntax since the result of a function is bound on the same line as the function, e.g:

let configure i =
  let+ () = configure_myocamlbuild in
  (* comment *)
  let+ () = configure_opam ~name:opam_name i () in
  let no_depext = Key.(get ctx no_depext) in
  let+ result = configure_makefile ~no_depext ~opam_name in
  foo

Note that the conversion to let+ syntax from the postfix form touches only single lines; the same is not true of converting from OCamlformat's preferred form.

Describe the solution you'd like I think this is perhaps a case for an option. Something along the lines of bind=[start|end] that allows choosing between the various behaviours. This is tricky, because there are uses of infix operators that always want to be at the start of the line, for instance when using 'combinator pattern matching' in the Irmin source code: https://github.com/mirage/irmin/blob/master/src/irmin/node.ml#L52. I think it's fairly justifiable to treat bind operators differently, since they have an agreed-upon semantics.

There is a related question about always putting binds on newlines, rather than trying to cram many of them into the same line, but perhaps this is already enough to be going on with :slightly_smiling_face:

CC: @Julow

jberdine commented 4 years ago

Does setting break-infix-before-func=false not work for you?

craigfe commented 4 years ago

It does not. I had break-infix-before-func=false set in that example (it's default in the conventional profile).

It seems that break-infix-before-func is not sufficient when not every function is a lambda:

let () =
  a____________________________________ >>=
  b____________________________________ >>= fun () ->
  c____________________________________ >>=
  d____________________________________ >>= fun () ->
  e

(* becomes *)
let () =
  a____________________________________
  >>= b____________________________________
  >>= fun () ->
  c____________________________________
  >>= d____________________________________
  >>= fun () -> e

Presumably because the left-alignment of infix operator chains supercedes break-infix-before-func, even when a comment splits the lines:

let () =
  a____________________________________ >>=
  (* Comment *)
  b____________________________________ >>= fun () ->
  ()

(* becomes *)
let () =
  a____________________________________
  >>= (* Comment *)
  b____________________________________
  >>= fun () -> ()

Looking back on this, having odd placement for point-free binds seems fine w.r.t. let+ compatibility, since that's not point-free. I do find the interaction a little odd regardless.

Perhaps the bigger issue demonstrated here is the return value being packed alongside the final bind:

let () =
  a____________________________________ >>= fun a ->
  b____________________________________ >>= fun b ->
  a <*> b

(* becomes *)
```ocaml
let () =
  a____________________________________ >>= fun a ->
  b____________________________________ >>= fun b -> a <*> b

These sorts of oddities are why I've been setting break-infix-before-func=true for a while now.

orbitz commented 4 years ago

I'd like to bump this as also a desired feature request

jberdine commented 4 years ago

I'm not sure that I understand the desired output, and I don't understand what is meant about "compatibility" with let operators, but for the case where the infix operators are on the left see what you think of the linked PR.

orbitz commented 4 years ago

I'll take a look at the PR.

To explain the output a bit: Given <value> and <expr> a let looks like:

let x = <value> in
<expr>

Or

let x =
    <value>
in
<expr>

The equivalent for this if the right hand side of an infix operator is an anonymous function would be:

<value> >>= fun x ->
<expr>

or

<value>
>>= fun x ->
<expr>

And (at least) I claim these match each other.

The issue I have is ocamlformat will try to put it all on a single line if possible so you'll get:

value >>= fun v -> something >>= fun y -> other thing

And I think this does not correspond to how people do let.

jberdine commented 4 years ago

Ok, so the main point about let is that with let, the bound value expression and the body expression are never placed on the same line, while with an infix operation they can be, right?

orbitz commented 4 years ago

Yes, that is correct.