Fix position of expressions regarding of comments in infix-op expressions

[gpetiot]

Improve this part of the code before working on #961

[gpetiot]

55f3f32 should fix #961
@craigfe how does this look on your codebase?

[Julow]

Looks good !

[craigfe]

Hmm... I'm getting this behaviour now:

let () =
  (* Open the repo *)
  initialise
  >>= (* Perform a subsequent action *)
      subsequent_action
  >|= (* Keep going... *)
      another_action
  |> fun t ->
  (* And finally do this *)
  final_action t

Which is perhaps not ideal either? The indentation on the line after the infix operators looks off to me.

[gpetiot]

I think it is fixed and ready to be reviewed now, here is the diff with test_branch: (this was before doing the same for `|> and other short operators, now the diff is obviously longer but still looks good)

diff --git a/infer/src/biabduction/interproc.ml b/infer/src/biabduction/interproc.ml
index 0e2dff723..2d6baa9db 100644
--- a/infer/src/biabduction/interproc.ml
+++ b/infer/src/biabduction/interproc.ml
@@ -707,8 +707,9 @@ let prop_init_formals_seed tenv new_formals (prop : 'a Prop.t) : Prop.exposed Pr
     List.map ~f:do_formal new_formals
   in
   let sigma_seed =
-    create_seed_vars ((* formals already there plus new ones *)
-                      prop.Prop.sigma @ sigma_new_formals)
+    create_seed_vars
+      ( (* formals already there plus new ones *)
+        prop.Prop.sigma @ sigma_new_formals )
   in
   let sigma = sigma_seed @ sigma_new_formals in
   let new_pi = prop.Prop.pi in
diff --git a/infer/src/clang/ClangWrapper.ml b/infer/src/clang/ClangWrapper.ml
index 450b693b3..234ccdcbd 100644
--- a/infer/src/clang/ClangWrapper.ml
+++ b/infer/src/clang/ClangWrapper.ml
@@ -89,8 +89,9 @@ let clang_driver_action_items : ClangCommand.t -> action_item list =
       CanonicalCommand
         ( (* massage line to remove edge-cases for splitting *)
         match
-          "\"" ^ line ^ " \"" |> (* split by whitespace *)
-                                 Str.split (Str.regexp_string "\" \"")
+          "\"" ^ line ^ " \""
+          |> (* split by whitespace *)
+             Str.split (Str.regexp_string "\" \"")
         with
         | prog :: args ->
             ClangCommand.mk ~is_driver:false ClangQuotes.EscapedDoubleQuotes ~prog ~args
diff --git a/compiler/lib/generate.ml b/compiler/lib/generate.ml
index 728cf7e7b..22c886acf 100644
--- a/compiler/lib/generate.ml
+++ b/compiler/lib/generate.ml
@@ -1835,9 +1835,9 @@ let generate_shared_value ctx =
             | None -> []
             | Some v ->
                 [J.V v, Some (J.EDot (s_var Constant.global_object, "jsoo_runtime"), J.N)])
-          @ List.map
-              (StringMap.bindings ctx.Ctx.share.Share.vars.Share.strings)
-              ~f:(fun (s, v) -> v, Some (str_js s, J.N))
+           @ List.map
+               (StringMap.bindings ctx.Ctx.share.Share.vars.Share.strings)
+               ~f:(fun (s, v) -> v, Some (str_js s, J.N))
            @ List.map
                (StringMap.bindings ctx.Ctx.share.Share.vars.Share.prims)
                ~f:(fun (s, v) -> v, Some (runtime_fun ctx s, J.N))))

[craigfe]

I'm not familiar with the existing code but this makes me uneasy; what about |>?

[gpetiot]

at first I wanted to avoid doing it for |> because it makes a long diff but it is more consistent indeed, fixed with my last commit.

[Julow]

This causes a break just after |> operators

-      |> List.sort
-           ~compare:(Comparable.lift Location.compare_start ~f:Cmt.loc)
+      |>
+      List.sort ~compare:(Comparable.lift Location.compare_start ~f:Cmt.loc)

[gpetiot]

Yes, that is the new behavior for all these operators, that's why I was filtering a bit (size > 2, checking the last character), so should |> be treated like the rest or not?
I don't really have an opinion on that, and @craigfe has a diverging opinion, so if the both of you could agree, that would be great :D

[craigfe]

FWIW, I also think this break looks odd. My toy example was using infix operators at the end of each line, a la:

(* Open the repo *)
initialise >>=
(* Perform a subsequent action *)
subsequent_action >|=

but when I'm writing this sort of code personally, I keep the infix operators at the start:

x
>>= foo
>>= bar

(or the same with |>). I don't really mind which of the two OCamlformat chooses to pick (this is probably a reasonably important decision), but I do care about its interaction with comments. i.e., this looks wrong to me:

initialise
>>= (* Perform a subsequent action *)
    subsequent_action 
>|= (* Keep going... *)
    another_action

And if OCamlformat chooses to enforce one or the other, there is the question of what to do with input flies that are doing it the other way (comments before/after the infix operator).

[craigfe]

(this may be an instance where it's worth having an option to select between prepending or appending infix operators)

[gpetiot]

Yes that would be good to have that but it requires a bigger rewrite of the code, the sequences of infix operations are currently sugared into a weird structure that mix different operators (as long as they have the same precedence, e.g. >>= and >|=), so it's not as simple as it was for sequences of items separated by ; only.

[Julow]

I don't mind the indentation in

initialise
>>= (* Perform a subsequent action *)
    subsequent_action 
>|= (* Keep going... *)
    another_action

Multiline expressions will render the same and here the first comment is part of the subsequent_action expression.

I don't think it's worth adding a special case that move the operator at the end of the line. It will probably be considered as a bug by users. Comments could be moved before instead:

initialise
(* Perform a subsequent action *)
>>= subsequent_action 
(* Keep going... *)
>|= another_action

(which does not work currently: if the whole expression fit, the comment will not be on its line)

[gpetiot]

I think I will merge after bf9b046 because this is clearly an improvement, but keep the issue open, so we can maybe improve it again later.
Edit: afterall I found an alternative in #986

[gpetiot]

Replaced by #986