More if-then-else=fit-or-vertical fixes#2810
Conversation
The `Compact` and `Fit_or_vertical` arms of `get_if_then_else` each carried a near-identical block computing `branch_pro` for the case of a comment preceding a special branch body, and their inline break expressions (`break 1000 0 $ cmts $ break 1000 0` and `break 1000 2 $ cmts`) duplicated exactly what `raw_cmts_branch_pro` already computes per mode. Extract a single `branch_pro_with_cmts ~default ~guard` helper that reuses `raw_cmts_branch_pro`, parameterized by each mode's comment-less `branch_pro` and its extra applicability guard. Behavior is unchanged (the full test suite passes with no ref changes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
With `if-then-else=fit-or-vertical`, an `if … else (* cmt *) match … with` branch (a bare match/function/try/if, not wrapped in begin/end or parentheses) rendered the branch body indented to the comment's end column instead of under the comment. `Params.raw_cmts_branch_pro` emitted, for non-Compact modes, `break 1000 2 $ cmts` with no trailing break. A bare branch's body breaks with `break_unless_newline`, a no-op unless already at the beginning of a line, and `fmt_match` opens its box at the current column — the comment's end column. Add an optional `~bare_branch` flag that appends a trailing break so the body box opens at the line start. It is not set for begin/end- or paren-wrapped branches, which emit their opening delimiter right after the comment, keeping that rendering unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
With `if-then-else=fit-or-vertical`, an `else begin match … with … end` branch rendered the header split over several lines (`begin match` / scrutinee / `with`), `begin if … end` likewise, and `end` ended up less indented than `begin`. For the `begin match/try/function/if end` shortcut, `fmt_beginend` threaded the branch `pro` into the inner expression's `pro`. In fit-or-vertical (and vertical) that `pro` is a `break_unless_newline 1000`, and because the inner expression's ctx0 is the begin/end node, `match_inner_pro` places it inside the header box. `pp_print_or_newline` enqueues its full width (1000) as the token length, so the header box never fits and is forced to break. When the begin/end is an if-then-else branch, emit `pro` outside the box (the branch break already positions it) and wrap the body and `end` together so `end` lines up with `begin`. Other contexts (e.g. `map x begin fun … end`) keep `pro` inside to preserve their indentation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Julow
left a comment
Julow
left a comment
There was a problem hiding this comment.
The match/function fix is perfect :)
| match raw_cmts_after_kw with | ||
| | Some cmts -> | ||
| Params.raw_cmts_branch_pro c.conf cmts | ||
| let bare_branch = |
There was a problem hiding this comment.
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
+ endI think the right output would be:
if gf then
(* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa *)
begin
if a then b
endbecause without the comment it is:
if gf then begin
if a then b
endThe 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
hoping that I won't offend you by copy-pasting the LLM's answer:
-
Only one test case changed, nothing else. All 39 modified ref files are the same case (
ite.ml'sbegin … (comment) … if a then b … endregression test from Fix formatting oscillation in if-then-else branches #2800), repeated across every profile ×if-then-elsevariant. No test input files changed; no other test files (exp_grouping, etc.) changed; no other case withinite.mlchanged. Theelse begin match e with … end caseis byte-identical to before. -
That case was never properly formatted — it's the bug being fixed. On main,
fit-or-verticalproduced 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.
@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.
There was a problem hiding this comment.
No problem :) Thanks for improving ocamlformat.
This output is fine. We'll see the impact in test-branch after you pushed
There was a problem hiding this comment.
I have already pushed ?...
This reverts commit e3babbb.
Reworks the previous fix (e3babbb) per review. The earlier version special-cased `ctx0=ifthenelse` in `fmt_beginend` to handle a comment before a `begin match/if … end` branch body; it glued the body (`begin if a then b`) and reindented it when a comment was added. Why the comment case now goes through the plain begin/end path: `fmt_beginend` cannot see the comment — `branch_pro` consumes it in `Params` before the expression is formatted. `get_if_then_else`, however, runs before consumption, so the decision belongs there. A special `begin match/if … end` branch with a leading comment on its body no longer takes the `begin <kw>` shortcut arm; it falls through to the existing plain begin/end machinery (`has_cmts_before` predicate). That keeps `begin` on its own line with the body (comment included) one indent in, identically across all if-then-else modes, so adding a comment no longer reindents the body. Why expr_pro is dropped for special-bodied begin/end branches: the fit-or-vertical / vertical branch `expr_pro` is `break_unless_newline 1000`, which enqueues width 1000 even as a no-op at BOL and poisons the body's `match … with` / `if … then` header box, forcing it to split over several lines. Such a body already breaks after `begin`, so it provides its own break instead of the poisoning one. Simple-bodied `begin e end` keeps the regular branch break (no header to poison). K&R additionally dropped the extra body box that indented a comment-routed body by +2. Why a ctx0 box wrap remains in fmt_beginend: the no-comment shortcut path still needs `end` aligned with `begin`. In fit-or-vertical the branch box is `hovbox 0` while the branch break indents `begin` one level in, so the body and `end` are wrapped together. This is scoped to `ctx0=ifthenelse` because wrapping unconditionally reindents application arguments (`map x begin fun … end`, exp_grouping.ml); the scope is structural (alignment), not comment-handling logic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I just noticed this MR. I suspect we've been working on more or less the same problems. My fixes are in #2813. I will attempt a comparison. |
|
This MR was uniformly better than mine, so I have removed the corresponding commits from mine. That being said, testing on our codebase, I found another problem. Tentative fixes are: yakobowski@ab9f05a and yakobowski@ecaf57f |
Perhaps you should open a new PR stacked on this one? That way you can build on the changes here and work on your own. |
Sorry, my previous comment was unclear. Those 2 commits are already on top of your branch, in case you wanted to have a look in an easy way. |
|
#2815 created with those changes + my additional ones. |
3 participants
Fix
begin match … end(andbegin if … end) branch withif-then-else=fit-or-vertical: thematch … withheader no longer splits over several lines, andendis aligned withbegin.Fix indentation of a bare
match/function/trybranch preceded by a comment withif-then-else=fit-or-vertical: the branch is no longer indented relative to the comment's end column.