Feature request: delimit if expressions rather than just then/else clauses

[ceastlund]

Is your feature request related to a problem? Please describe.

Right now, ocamlformat can generate code like this:

if peacetime
then do_nothing ()
else
  get_launch_authorization_exn
    ~from:missile_silo
    ~for:target
    ~authorization:current_user;
launch_the_missiles
  ~from:missile_silo
  ~for:target
  ~authorization:current_user

The only visual indication that [launch_the_missiles] is not in the same clause as [get_launch_authorization_exn] is whitespace. That's not a very strong indicator, and whitespace also does not show up in our code review, so a change that added the "if peacetime then do_nothing () else" to this code might not make it clear how that effects the code's logic.

Describe the solution you'd like

It would be very helpful, and would have helped us avoid production bugs we've had in the recent past, if if expressions followed by the ; operator were delimited with parentheses. Like so:

(if peacetime
 then do_nothing ()
 else
   get_launch_authorization_exn
     ~from:missile_silo
     ~for:target
     ~authorization:current_user);
launch_the_missiles
  ~from:missile_silo
  ~for:target
  ~authorization:current_user

Additional context

The above example is, obviously, somewhat contrived, and only shows a snapshot of code, not exactly how this syntax issue has bit us in the process of code review. If more practical context would be helpful I can do some code archaeology to find out, but hopefully the above is sufficient.

[gpetiot]

I don't know about parentheses, but on this specific example using sequence-style=before (in addition to the janestreet profile) would give the following output:

let () =               
  if peacetime
  then do_nothing ()
  else
    get_launch_authorization_exn
      ~from:missile_silo
      ~for_:target
      ~authorization:current_user
  ; launch_the_missiles ~from:missile_silo ~for_:target ~authorization:current_user
;;

Considering the janestreet profile already sets break-separators=before I think it would be consistent to also set sequence-style=before.

[ceastlund]

Moving the semicolon to the next line still doesn't clarify whether it binds more or less tightly than "else", except via indentation. One way or another, a delimiter is needed to make the binding precedence explicit.

[craigfe]

For what it's worth from someone who does not maintain OCamlformat, I dislike the proposed style.

OCamlformat already has some such special cases for 'disambiguation' of syntax that is seen to be confusing to the user, and personally they are a constant source of frustration. In particular, I think they get in the way of refactoring, which is particularly problematic if (like me) you OCamlformat very frequently (such as on file save).

Existing poor UX with disambiguation

• inserting () around not's in a predicate, but only when on the LHS of an operator

If I write code like this,

let _ = a && not b
let _ = not b && a

I get the following diff

 let _ = a && not b

-let _ = not b && a
+let _ = (not b) && a

Whether you prefer parentheses or not, OCamlformat won't let you flip the arguments to an && without needing to add/remove parens. I can't, for instance, choose to have () in all cases and stop the tool getting in the way of refactoring.

• removing () around match expressions, but only when the last in an operator chain

If I write code like this,

let args =
  [ "--verbose" ]
  @ (match cpuset_mems with Some i -> [ "--cpuset-mems"; i ] | None -> [])
  @ (match cpuset_cpus with Some i -> [ "--cpuset-cpus"; i ] | None -> [])

I get the following diff

 let args =
   [ "--verbose" ]
   @ (match cpuset_mems with Some i -> [ "--cpuset-mems"; i ] | None -> [])
-  @ (match cpuset_cpus with Some i -> [ "--cpuset-cpus"; i ] | None -> [])
+  @ match cpuset_cpus with Some i -> [ "--cpuset-cpus"; i ] | None -> []

Now swapping the lines changes the semantics, and avoiding this requires adding/removing more parentheses.

UX issues with the new format

While these might seem innocent enough, they both have the property of changing the indentation of the line, which means that they can cause a multi-line expression to suddenly fit on a single line (and vice versa), cascading onto other style changes. This new formatting style has the same problem. Take this (admittedly very contrived) example:

let loop () =
  if delta > 1_000 then
    match a_opt with
    | Some a ->
        alpha >>= beta >>= (fun g -> gamma vr ~pull:false) >>= delta >>= epsilon
    | None -> (
        match err with `Json -> print_json_error !e | `Plain -> print_error !e )
  else loop ();
  Logs.app (fun m -> m "Doing something special")

Now I swap the lines, and get given this formatting diff:

 let loop () =
-  if delta > 1_000 then
-    match a_opt with
-    | Some a ->
-        alpha >>= beta >>= (fun g -> gamma vr ~pull:false) >>= delta >>= epsilon
-    | None -> (
-        match err with `Json -> print_json_error !e | `Plain -> print_error !e )
-  else loop ();
+  (if delta > 1_000 then
+     match a_opt with
+     | Some a ->
+        alpha
+        >>= beta
+        >>= (fun g -> gamma vr ~pull:false)
+        >>= delta
+        >>= epsilon
+    | None -> (
+        match eerr with
+        | `Json -> print_json_error !e
+        | `Plain -> print_error !e )
+  else loop ());
   Logs.app (fun m -> m "Doing something special")

Conclusion

In your particular example, I personally find the whitespace enough of a visual hint as to the binding precedence. Of course, this is very subjective and is driven by my particular use of OCamlformat.

Perhaps there is scope for a safe / disambiguate profile that captures this desire for concrete syntax that is less likely to be misconstrued. I just ask that development in this direction not make my refactor diffs even noisier :slightly_smiling_face:

[ceastlund]

I'm sympathetic to many of the above issues. Indeed, I wish some of the parenthesis handling were more uniform around infix operators. That might be worth a separate request.

I would be perfectly happy for this change (delimiters around conditionals) to be opt-in, and enabled in the janestreet profile. It's largely driven by our code review process, which filters out whitespace-only changes, so we often don't notice them.

[ceastlund]

I have added a separate feature request about symmetric use of delimiters: https://github.com/ocaml-ppx/ocamlformat/issues/1222