Skip to content

More if-then-else=fit-or-vertical fixes #2810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MisterDA wants to merge 5 commits into
base: main
Choose a base branch
from
Open

More if-then-else=fit-or-vertical fixes #2810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
47367a0
Params: deduplicate branch_pro_with_cmts across if-then-else modes
MisterDA Jun 24, 2026
199bd38
Fix indentation of bare match branch after comment with fit-or-vertical
MisterDA Jun 24, 2026
e3babbb
Fix begin match/if branch layout with fit-or-vertical
MisterDA Jun 24, 2026
26c1058
Revert "Fix begin match/if branch layout with fit-or-vertical"
MisterDA Jun 24, 2026
be77c01
Rework begin match/if branch layout (review follow-up)
MisterDA Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,18 @@ profile. This started with version 0.26.0.

### Fixed

- Fix `begin match … end` (and `begin if … end`) branches: with
`if-then-else=fit-or-vertical` the `match … with` header no longer splits
over several lines and `end` is aligned with `begin`; and a leading comment
on the body no longer reindents it — `begin` keeps the body one indent in
instead of gluing the keyword to the comment.
(#2810, @MisterDA)

- Fix indentation of a bare `match`/`function`/`try` branch preceded by a
comment with `if-then-else=fit-or-vertical`: the branch is no longer
indented relative to the comment's end column.
(#2810, @MisterDA)

- Fix formatting oscillation with `if-then-else=fit-or-vertical` and
`begin...end`.
(#2800, @MisterDA)
Expand Down
30 changes: 24 additions & 6 deletions lib/Fmt_ast.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2581,6 +2581,7 @@ and fmt_expression c ?(box = true) ?(pro = noop) ?eol ?parens
in
let p =
Params.get_if_then_else c.conf ~cmts_before_opt
~has_cmts_before:(Cmts.has_before c.cmts)
~pro:(fmt_if first pro_inner) ~first ~last
~parens_bch ~parens_prev_bch:!parens_prev_bch
~xcond ~xbch ~expr_loc:pexp_loc
Expand All @@ -2593,7 +2594,12 @@ and fmt_expression c ?(box = true) ?(pro = noop) ?eol ?parens
let branch_pro =
match raw_cmts_after_kw with
| Some cmts ->
Params.raw_cmts_branch_pro c.conf cmts
let bare_branch =

@Julow Julow Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's the fix for if-then-else. This does:

       if gf then
         (* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa *)
-      begin if
-        a
-      then
-        b
-      end
+        begin if a then b
+        end

I think the right output would be:

       if gf then
         (* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa *)
       begin
         if a then b
       end

because without the comment it is:

       if gf then begin
         if a then b
       end

The body of the begin-end shouldn't reindent if a comment is added around it. This should be doable with boxes instead of adding a new special case.

@MisterDA MisterDA Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the LLM seems to like

if gf then begin
  (* comment *)
  if a then b
end

and I'm not sure how to go against it

@Julow Julow Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably fine thought we tried to avoid this in the past. If it changes existing code (eg. in the testsuite), then it's not OK.

@MisterDA MisterDA Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hoping that I won't offend you by copy-pasting the LLM's answer:

  1. Only one test case changed, nothing else. All 39 modified ref files are the same case (ite.ml's begin … (comment) … if a then b … end regression test from Fix formatting oscillation in if-then-else branches #2800), repeated across every profile × if-then-else variant. No test input files changed; no other test files (exp_grouping, etc.) changed; no other case within ite.ml changed. The else begin match e with … end case is byte-identical to before.

  2. That case was never properly formatted — it's the bug being fixed. On main, fit-or-vertical produced the broken over-split:

      if gf then
        (* comment *)
      begin if
        a
      then
        b
      end

My output:

      if gf then begin
        (* comment *)
        if a then b
      end

which is exactly the input layout (test/passing/tests/ite.ml:206-209) — so it's now idempotent, where before it wasn't.

  1. @runtest exits 0 — every test case formats to its ref and is stable (the harness checks idempotency). If I'd disturbed any already-correct, stable formatting, the suite would fail.

So the rule is satisfied: the only behavioural change is to the one case that was misformatted, and it now round-trips its own input. Everything that was already correct is untouched.

@Julow Julow Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem :) Thanks for improving ocamlformat.
This output is fine. We'll see the impact in test-branch after you pushed

@MisterDA MisterDA Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already pushed ?...

Params.is_bare_branch ~parens_bch
xbch.ast.pexp_desc
in
Params.raw_cmts_branch_pro ~bare_branch c.conf
cmts
| None -> p.branch_pro
in
let wrap_beginend =
Expand Down Expand Up @@ -3034,11 +3040,23 @@ and fmt_beginend c ~loc ?(box = true) ?(pro = noop) ~ctx ~ctx0 ~fmt_atrs
$
match e.pexp_desc with
| Pexp_match _ | Pexp_try _ | Pexp_function _ | Pexp_ifthenelse _ ->
beginend_box
(fmt_expression c
~pro:(pro $ begin_ $ str " ")
~box:false ?eol ~parens:false ~indent_wrap (sub_exp ~ctx e) )
$ end_
(* In an [if-then-else] branch the branch break indents [begin] one
level in (e.g. [fit-or-vertical], whose branch box is [hovbox 0]),
while [end] follows the branch box; wrap the body and [end] together
so [end] lines up with [begin]. Other contexts (e.g. [map x begin
fun … end] as an application argument) must keep their own
indentation. *)
let box =
match ctx0 with
| Exp {pexp_desc= Pexp_ifthenelse _; _} -> hvbox 0
| _ -> Fn.id
in
box
( beginend_box
(fmt_expression c
~pro:(pro $ begin_ $ str " ")
~box:false ?eol ~parens:false ~indent_wrap (sub_exp ~ctx e) )
$ end_ )
| _ ->
beginend_box
( hvbox 0 (pro $ begin_)
Expand Down
134 changes: 83 additions & 51 deletions lib/Params.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,9 +516,22 @@ let is_special_or_nested_special_beginend = function
| Pexp_beginend ({pexp_desc; _}, _) -> is_special_beginend pexp_desc
| exp -> is_special_beginend exp

let raw_cmts_branch_pro (c : Conf.t) cmts =
(* An if-then-else branch is rendered "bare" when it is neither wrapped in
[begin]/[end] (including the [begin match end] shortcut, a
[Pexp_beginend]) nor parenthesized. *)
let is_bare_branch ~parens_bch desc =
(not parens_bch) && match desc with Pexp_beginend _ -> false | _ -> true

let raw_cmts_branch_pro ?(bare_branch = false) (c : Conf.t) cmts =
match c.fmt_opts.if_then_else.v with
| `Compact -> break 1000 0 $ cmts $ break 1000 0
(* A bare [match]/[function]/[try]/[if] branch breaks before its own body
with a [break_unless_newline], which is a no-op unless we are already at
the beginning of a line; without a trailing break here the branch box
would open at the comment's end column and indent the body relative to
it. A [begin]/[end]- or paren-wrapped branch instead emits its opening
delimiter right after the comment, so we leave the break to it. *)
| _ when bare_branch -> break 1000 2 $ cmts $ break 1000 2
| _ -> break 1000 2 $ cmts

type cases =
Expand Down Expand Up @@ -858,15 +871,22 @@ type if_then_else =
; break_end_branch: Fmt.t
; space_between_branches: Fmt.t }

let get_if_then_else (c : Conf.t) ~cmts_before_opt ~pro ~first ~last
~parens_bch ~parens_prev_bch ~xcond ~xbch ~expr_loc ~fmt_infix_ext_attrs
~infix_ext_attrs ~fmt_cond ~cmts_before_kw ~cmts_after_kw =
let get_if_then_else (c : Conf.t) ~cmts_before_opt ~has_cmts_before ~pro
~first ~last ~parens_bch ~parens_prev_bch ~xcond ~xbch ~expr_loc
~fmt_infix_ext_attrs ~infix_ext_attrs ~fmt_cond ~cmts_before_kw
~cmts_after_kw =
let imd = c.fmt_opts.indicate_multiline_delimiters.v in
let beginend_loc, infix_ext_attrs_beginend, branch_expr =
let ast = xbch.Ast.ast in
match ast with
| {pexp_desc= Pexp_beginend ({pexp_desc; _}, _); _}
when is_special_beginend pexp_desc ->
| {pexp_desc= Pexp_beginend (nested_exp, _); _}
when is_special_beginend nested_exp.pexp_desc
&& not (has_cmts_before nested_exp.pexp_loc) ->
(* [begin match/try/function/if … end] shortcut: keep [begin] glued
to the body keyword. A leading comment on the body breaks the
glue, so fall through to the plain [begin]/[end] machinery below,
which puts [begin] on its own line and the body (comment included)
one indent in. *)
(None, None, xbch)
| { pexp_desc= Pexp_beginend (nested_exp, infix_ext_attrs)
; pexp_attributes= []
Expand All @@ -878,6 +898,19 @@ let get_if_then_else (c : Conf.t) ~cmts_before_opt ~pro ~first ~last
| _ -> (None, None, xbch)
in
let has_beginend = Option.is_some beginend_loc in
(* A [begin]/[end] branch whose body is a [match]/[try]/[function]/[if]
(the [begin match … end] shortcut, or its plain form when a leading
comment de-glues [begin] from the body). Such a body has a header
([match … with], [if … then]) that the branch [break_unless_newline
1000] would wrongly split, so it provides its own break after [begin]
instead. Simple-bodied [begin … end] keeps the regular branch break. *)
let is_special_beginend_branch =
match xbch.ast.pexp_desc with
| Pexp_beginend (nested_exp, _) ->
is_special_beginend nested_exp.pexp_desc
| _ -> false
in
let bare_branch = is_bare_branch ~parens_bch xbch.ast.pexp_desc in
let wrap_parens ~wrap_breaks k =
if has_beginend then
let infix_ext_attrs_beginend =
Expand Down Expand Up @@ -929,31 +962,40 @@ let get_if_then_else (c : Conf.t) ~cmts_before_opt ~pro ~first ~last
| None when parens_bch -> str " "
| None -> break 1 indent
in
(* When a comment precedes a multi-line
[match]/[function]/[try]/[if-then-else] branch body (or a [begin]/[end]
wrapping one), emit it from [branch_pro] with forced breaks, so the
branch indentation stays stable regardless of where the comment was
initially attached. [default] is the comment-less [branch_pro] for the
current mode, [guard] an extra mode-specific applicability condition. *)
let branch_pro_with_cmts ~default ~guard =
if
(not has_beginend)
&& (not (Location.is_single_line expr_loc c.fmt_opts.margin.v))
&& (not has_cmts_after_kw) && guard
then
match cmts_before_opt xbch.ast.pexp_loc with
| Some cmts -> raw_cmts_branch_pro ~bare_branch c cmts
| None -> (
match xbch.ast.pexp_desc with
| Pexp_beginend ({pexp_loc; pexp_desc; _}, _)
when is_special_beginend pexp_desc -> (
match cmts_before_opt pexp_loc with
| Some cmts -> raw_cmts_branch_pro ~bare_branch c cmts
| None -> default )
| _ -> default )
else default
in
match c.fmt_opts.if_then_else.v with
| `Compact ->
let branch_pro_with_cmts =
if
(not has_beginend) && (not parens_bch)
&& (not (Location.is_single_line expr_loc c.fmt_opts.margin.v))
&& (not has_cmts_after_kw)
&& is_special_or_nested_special_beginend xbch.ast.pexp_desc
then
match cmts_before_opt xbch.ast.pexp_loc with
| Some cmts -> break 1000 0 $ cmts $ break 1000 0
| None -> (
match xbch.ast.pexp_desc with
| Pexp_beginend ({pexp_loc; pexp_desc; _}, _)
when is_special_beginend pexp_desc -> (
match cmts_before_opt pexp_loc with
| Some cmts -> break 1000 0 $ cmts $ break 1000 0
| None -> branch_pro ~indent:0 () )
| _ -> branch_pro ~indent:0 () )
else branch_pro ~indent:0 ()
in
{ box_branch= hovbox ~name:"Params.get_if_then_else `Compact" 2
; cond= cond ()
; box_keyword_and_expr= Fn.id
; branch_pro= branch_pro_with_cmts
; branch_pro=
branch_pro_with_cmts ~default:(branch_pro ~indent:0 ())
~guard:
( (not parens_bch)
&& is_special_or_nested_special_beginend xbch.ast.pexp_desc )
; wrap_parens=
wrap_parens
~wrap_breaks:
Expand All @@ -973,7 +1015,7 @@ let get_if_then_else (c : Conf.t) ~cmts_before_opt ~pro ~first ~last
; branch_pro= branch_pro ~begin_end_offset:0 ()
; wrap_parens= wrap_parens ~wrap_breaks:(wrap (break 1000 2) noop)
; beginend_loc
; box_expr= Some has_beginend
; box_expr= Some (has_beginend && not is_special_beginend_branch)
; expr_pro= None
; expr_eol= Some (break 1 2)
; branch_expr
Expand All @@ -982,32 +1024,17 @@ let get_if_then_else (c : Conf.t) ~cmts_before_opt ~pro ~first ~last
; space_between_branches= fmt_if (has_beginend || parens_bch) (str " ")
}
| `Fit_or_vertical ->
let branch_pro_with_cmts =
if
(not has_beginend)
&& (not (Location.is_single_line expr_loc c.fmt_opts.margin.v))
&& not has_cmts_after_kw
then
match cmts_before_opt xbch.ast.pexp_loc with
| Some cmts -> break 1000 2 $ cmts
| None -> (
match xbch.ast.pexp_desc with
| Pexp_beginend ({pexp_loc; pexp_desc; _}, _)
when is_special_beginend pexp_desc -> (
match cmts_before_opt pexp_loc with
| Some cmts -> break 1000 2 $ cmts
| None -> branch_pro ~begin_end_offset:0 () )
| _ -> branch_pro ~begin_end_offset:0 () )
else branch_pro ~begin_end_offset:0 ()
in
{ box_branch=
hovbox
( match imd with
| `Closing_on_separate_line when parens_prev_bch -> -2
| _ -> 0 )
; cond= cond ()
; box_keyword_and_expr= Fn.id
; branch_pro= branch_pro_with_cmts
; branch_pro=
branch_pro_with_cmts
~default:(branch_pro ~begin_end_offset:0 ())
~guard:true
; wrap_parens=
wrap_parens
~wrap_breaks:
Expand All @@ -1016,10 +1043,13 @@ let get_if_then_else (c : Conf.t) ~cmts_before_opt ~pro ~first ~last
; beginend_loc
; box_expr= Some false
; expr_pro=
Some
(fmt_if
(not (Location.is_single_line expr_loc c.fmt_opts.margin.v))
(break_unless_newline 1000 2) )
( if is_special_beginend_branch then None
else
Some
(fmt_if
(not
(Location.is_single_line expr_loc c.fmt_opts.margin.v) )
(break_unless_newline 1000 2) ) )
; expr_eol= Some (break 1 2)
; branch_expr
; break_end_branch= noop
Expand All @@ -1040,7 +1070,9 @@ let get_if_then_else (c : Conf.t) ~cmts_before_opt ~pro ~first ~last
~cls_hint:((1, 0), (1000, 0)) )
; beginend_loc
; box_expr= None
; expr_pro= Some (break_unless_newline 1000 2)
; expr_pro=
( if is_special_beginend_branch then None
else Some (break_unless_newline 1000 2) )
; expr_eol= None
; branch_expr
; break_end_branch= noop
Expand Down
13 changes: 11 additions & 2 deletions lib/Params.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ type if_then_else =
val get_if_then_else :
Conf.t
-> cmts_before_opt:(Location.t -> Fmt.t option)
-> has_cmts_before:(Location.t -> bool)
-> pro:Fmt.t
-> first:bool
-> last:bool
Expand All @@ -242,10 +243,18 @@ val is_special_or_nested_special_beginend : expression_desc -> bool
the keyword should be extracted without breaks (raw) to prevent oscillation
between "after keyword" and "before expression" placements. *)

val raw_cmts_branch_pro : Conf.t -> Fmt.t -> Fmt.t
val is_bare_branch : parens_bch:bool -> expression_desc -> bool
(** [is_bare_branch ~parens_bch desc] is whether an if-then-else branch with
expression description [desc] is rendered "bare", i.e. neither wrapped in
[begin]/[end] (including the [begin match end] shortcut) nor parenthesized. *)

val raw_cmts_branch_pro : ?bare_branch:bool -> Conf.t -> Fmt.t -> Fmt.t
(** [raw_cmts_branch_pro c cmts] returns the branch_pro for raw comments
extracted after a keyword, using the correct indentation for the current
if-then-else mode. *)
if-then-else mode. [bare_branch] (default [false]) indicates the branch is
a bare [match]/[function]/[try]/[if] (not wrapped in [begin]/[end] or
parentheses); such a branch needs a forced break after the comment so that
its body is not indented relative to the comment's end column. *)

val match_indent : ?default:int -> Conf.t -> parens:bool -> ctx:Ast.t -> int
(** [match_indent c ~ctx ~default] returns the indentation used for the
Expand Down
27 changes: 24 additions & 3 deletions test/passing/refs.ahrefs/ite-compact.ml.ref
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,10 @@ let test =
let f =
match x with
| A ->
if gf then
if gf then begin
(* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa *)
begin if a then b
end
if a then b
end

(* Long comment before match in else branch (regression test for
formatting oscillation) *)
Expand All @@ -230,3 +230,24 @@ let _ =
iter (i + 1) ~a:s ~b:a ~c:b ~d:c ~fa:fs ~fb:fa ~fc:fb mflag
else iter (i + 1) ~a ~b:s ~c:b ~d:c ~fa ~fb:fs ~fc:fb mflag
else g

(* Short comment before a bare match in the else branch: the match must stay
indented under the comment, not pushed to its end column (regression test) *)
let _ =
if cond then x
else (
(* a comment *)
match e with
| A -> a
| B -> b)

(* A bare [begin match ... end] branch must keep [match ... with] on one line,
not split it as [begin match] / scrutinee / [with], and must align [end]
with [begin] (regression test) *)
let _ =
if cond then x
else
begin match e with
| A -> function_with_a_long_enough_name_to_force_a_vertical_break a
| B -> b
end
28 changes: 25 additions & 3 deletions test/passing/refs.ahrefs/ite-compact_closing.ml.ref
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,10 @@ let test =
let f =
match x with
| A ->
if gf then
if gf then begin
(* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa *)
begin if a then b
end
if a then b
end

(* Long comment before match in else branch (regression test for
formatting oscillation) *)
Expand All @@ -250,3 +250,25 @@ let _ =
iter (i + 1) ~a:s ~b:a ~c:b ~d:c ~fa:fs ~fb:fa ~fc:fb mflag
else iter (i + 1) ~a ~b:s ~c:b ~d:c ~fa ~fb:fs ~fc:fb mflag
else g

(* Short comment before a bare match in the else branch: the match must stay
indented under the comment, not pushed to its end column (regression test) *)
let _ =
if cond then x
else (
(* a comment *)
match e with
| A -> a
| B -> b
)

(* A bare [begin match ... end] branch must keep [match ... with] on one line,
not split it as [begin match] / scrutinee / [with], and must align [end]
with [begin] (regression test) *)
let _ =
if cond then x
else
begin match e with
| A -> function_with_a_long_enough_name_to_force_a_vertical_break a
| B -> b
end
2 changes: 1 addition & 1 deletion test/passing/refs.ahrefs/ite-fit_or_vertical.ml.err
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@ Warning: ite-fit_or_vertical.ml:116 exceeds the margin
Warning: ite-fit_or_vertical.ml:121 exceeds the margin
Warning: ite-fit_or_vertical.ml:126 exceeds the margin
Warning: ite-fit_or_vertical.ml:247 exceeds the margin
Warning: ite-fit_or_vertical.ml:260 exceeds the margin
Warning: ite-fit_or_vertical.ml:257 exceeds the margin
Loading
Loading