Additional fixes on top of #2810#2815
Open
yakobowski wants to merge 7 commits into
Open
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>
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>
When a comment precedes a parenthesized match/try/function/if branch (janestreet and ahrefs profiles with parens_ite or exp_grouping=parens), glue the opening paren to the content instead of putting it alone on a line. Before: `(* cmt *)\n(\nmatch e with\n| ...)` After: `(* cmt *)\n(match e with\n | ...)` Also simplify raw_cmts_branch_pro: remove the bare_branch distinction (always emit trailing break), since paren-glue now handles the case that bare_branch was partially covering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests the paren-glue behavior: with exp_grouping=parens (janestreet,
ahrefs), [begin match ... end] is parsed as a bare match needing
parenthesization; a preceding comment must not glue to the opening
paren.
Without the fix, this renders as:
(* a comment *)begin match some_long_scrutinee_expression with
| A -> a
| B -> b
end
or (with exp_grouping=parens):
(* a comment *)
(
match some_long_scrutinee_expression with
| A -> a
| B -> b)
Note: without the preceding fix, janestreet + k-r mode has a
pre-existing over-indentation issue for match-in-parens branches (match
indented +2 relative to the comment inside the parens). This caused
non-convergence with max-iters=3 in the unfixed code; the paren-glue
fix resolves the oscillation.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This MR fixes a few additional problems for the
if-then-else=fit-or-verticalmode. It is stacked on top of #2180.The first commit partially reverts some changes (
bare_branchlogic) in order to fix some additional less-than-ideal reformatting found on our codebase. I have added testcases, but I was not able to add the more interesting ones in a standalone commit, as ocamlformat without the fix does not stabilize in some configurations. (See the last commit.)