Fix crash with exp-grouping and if-then-else (ocaml-ppx#2369)

* Fix crash with exp-grouping and if-then-else

The formatting for if-then-else did not account for the begin-end in
some cases and generated invalid syntax.

CHANGES.md

@@ -8,6 +8,7 @@
 
 ### Bug fixes
 
+- Fix invalid formatting of `then begin end` (#2369, @Julow)
 - Protect match after `fun _ : _ ->` (#2352, @Julow)
 - Fix invalid formatting of `(::)` (#2347, @Julow)
 - Fix formatting of string literals in code blocks (#2338, #2349, @Julow)

lib/Params.ml

@@ -426,15 +426,15 @@ let get_if_then_else (c : Conf.t) ~first ~last ~parens_bch ~parens_prev_bch
     else if parens_bch then wrap "(" ")" (wrap_breaks k)
     else k
   in
-  let get_parens_breaks ~opn_hint:(oh_space, oh_other)
-      ~cls_hint:(ch_sp, ch_sl) =
+  let get_parens_breaks ~opn_hint_indent ~cls_hint:(ch_sp, ch_sl) =
     let brk hint = fits_breaks "" ~hint "" in
+    let oh_other = ((if beginend then 1 else 0), opn_hint_indent) in
     if beginend then
       let _, offset = ch_sl in
       wrap_k (brk oh_other) (break 1000 offset)
     else
       match imd with
-      | `Space -> wrap_k (brk oh_space) (brk ch_sp)
+      | `Space -> wrap_k (brk (1, opn_hint_indent)) (brk ch_sp)
       | `No -> wrap_k (brk oh_other) noop
       | `Closing_on_separate_line -> wrap_k (brk oh_other) (brk ch_sl)
   in
@@ -460,8 +460,7 @@ let get_if_then_else (c : Conf.t) ~first ~last ~parens_bch ~parens_prev_bch
       ; wrap_parens=
           wrap_parens
             ~wrap_breaks:
-              (get_parens_breaks
-                 ~opn_hint:((1, 0), (0, 0))
+              (get_parens_breaks ~opn_hint_indent:0
                  ~cls_hint:((1, 0), (1000, -2)) )
       ; box_expr= Some false
       ; expr_pro= None
@@ -492,8 +491,7 @@ let get_if_then_else (c : Conf.t) ~first ~last ~parens_bch ~parens_prev_bch
       ; wrap_parens=
           wrap_parens
             ~wrap_breaks:
-              (get_parens_breaks
-                 ~opn_hint:((1, 2), (0, 2))
+              (get_parens_breaks ~opn_hint_indent:2
                  ~cls_hint:((1, 0), (1000, 0)) )
       ; box_expr= Some false
       ; expr_pro=
@@ -516,8 +514,7 @@ let get_if_then_else (c : Conf.t) ~first ~last ~parens_bch ~parens_prev_bch
       ; wrap_parens=
           wrap_parens
             ~wrap_breaks:
-              (get_parens_breaks
-                 ~opn_hint:((1, 2), (0, 2))
+              (get_parens_breaks ~opn_hint_indent:2
                  ~cls_hint:((1, 0), (1000, 0)) )
       ; box_expr= None
       ; expr_pro= Some (break_unless_newline 1000 2)
@@ -546,8 +543,7 @@ let get_if_then_else (c : Conf.t) ~first ~last ~parens_bch ~parens_prev_bch
       ; wrap_parens=
           wrap_parens
             ~wrap_breaks:
-              (get_parens_breaks
-                 ~opn_hint:((1, 0), (0, 0))
+              (get_parens_breaks ~opn_hint_indent:0
                  ~cls_hint:((1, 0), (1000, -2)) )
       ; box_expr= Some false
       ; expr_pro= None

test/passing/tests/exp_grouping-parens.ml.ref

@@ -204,6 +204,8 @@ let _ =
   [| (let a = b in
       c ) |]
 
+let () = if a then b (* asd *)
+
 [@@@ocamlformat "if-then-else=compact"]
 
 let _ =
@@ -215,6 +217,8 @@ let _ =
     foo.fooooo <- Fooo.foo fooo foo.fooooo ;
     Fooo fooo )
 
+let () = if a then b (* asd *)
+
 [@@@ocamlformat "if-then-else=fit-or-vertical"]
 
 let _ =
@@ -227,6 +231,10 @@ let _ =
     foo.fooooo <- Fooo.foo fooo foo.fooooo ;
     Fooo fooo )
 
+let () =
+  if a then
+    b (* asd *)
+
 [@@@ocamlformat "if-then-else=keyword-first"]
 
 let _ =
@@ -240,6 +248,8 @@ let _ =
     foo.fooooo <- Fooo.foo fooo foo.fooooo ;
     Fooo fooo )
 
+let () = if a then b (* asd *)
+
 [@@@ocamlformat "if-then-else=k-r"]
 
 let _ =
@@ -295,3 +305,5 @@ let _ =
     market_data_items := ()
   end
   [@landmark "parse_constant_dividends"]
+
+let () = if a then b (* asd *)

test/passing/tests/exp_grouping.ml

@@ -122,6 +122,11 @@ let _ = a :: ( let a = b in c )
 let _ = [ ( let a = b in c ) ]
 let _ = [| ( let a = b in c ) |]
 
+let () =
+  if a then begin b
+    (* asd *)
+  end
+
 [@@@ocamlformat "if-then-else=compact"]
 
 let _ =
@@ -136,6 +141,11 @@ let _ =
     Fooo fooo
   end
 
+let () =
+  if a then begin b
+    (* asd *)
+  end
+
 [@@@ocamlformat "if-then-else=fit-or-vertical"]
 
 let _ =
@@ -150,6 +160,11 @@ let _ =
     Fooo fooo
   end
 
+let () =
+  if a then begin b
+    (* asd *)
+  end
+
 [@@@ocamlformat "if-then-else=keyword-first"]
 
 let _ =
@@ -164,6 +179,11 @@ let _ =
     Fooo fooo
   end
 
+let () =
+  if a then begin b
+    (* asd *)
+  end
+
 [@@@ocamlformat "if-then-else=k-r"]
 
 let _ =
@@ -224,3 +244,8 @@ let _ =
   begin[@landmark "parse_constant_dividends"]
     market_data_items := ()
   end
+
+let () =
+  if a then begin b
+    (* asd *)
+  end

test/passing/tests/exp_grouping.ml.ref

@@ -214,6 +214,11 @@ let _ =
   [| (let a = b in
       c ) |]
 
+let () =
+  if a then begin b
+    (* asd *)
+  end
+
 [@@@ocamlformat "if-then-else=compact"]
 
 let _ =
@@ -229,6 +234,11 @@ let _ =
     Fooo fooo
   end
 
+let () =
+  if a then begin b
+    (* asd *)
+  end
+
 [@@@ocamlformat "if-then-else=fit-or-vertical"]
 
 let _ =
@@ -244,6 +254,12 @@ let _ =
     Fooo fooo
   end
 
+let () =
+  if a then begin
+    b
+  (* asd *)
+  end
+
 [@@@ocamlformat "if-then-else=keyword-first"]
 
 let _ =
@@ -261,6 +277,12 @@ let _ =
     Fooo fooo
   end
 
+let () =
+  if a
+  then begin b
+    (* asd *)
+  end
+
 [@@@ocamlformat "if-then-else=k-r"]
 
 let _ =
@@ -333,3 +355,9 @@ let _ =
     market_data_items := ()
   end
   [@landmark "parse_constant_dividends"]
+
+let () =
+  if a then begin
+    b
+  (* asd *)
+  end