TeX-faithful brace grouping (MTMathGroup) — fix #177

[kostub]

TeX-faithful brace grouping (MTMathGroup)

Fixes #177.

Makes a main-list {…} brace group build a nested Ord subformula (MTMathGroup / kMTMathAtomOrdGroup) instead of flattening, so \scriptstyle (and Bin/Ord reclassification) is scoped to the group and scripts target the whole group. This is the honest analog of TeX's Ord-noad-with-sub_mlist / KaTeX's ordgroup. Field braces (^{…}, _{…}, \frac{…}, command args) keep flattening.

Goal: Introduce the MTMathGroup atom, wire it into the parser for main-list braces, and render it in the typesetter — fixing the x{\scriptstyle y}z style-leak, with the branch buildable and all tests green at each commit.

Design docs

• Plan: docs/plans/2026-06-30-tex-faithful-brace-grouping.md
• LLD: docs/lld/2026-06-30-tex-faithful-brace-grouping.md

Commits

• [item 1] Add MTMathGroup atom (kMTMathAtomOrdGroup) for brace groups
• [item 2] Wrap main-list {…} as MTMathGroup in the parser
• [item 3] Render MTMathGroup in the typesetter (fix #177 style leak)

Notes / deviations from the plan

Beyond the plan's stated 5 data rows, three additional tests needed handling to keep the suite TeX-faithful and green:

• testSqrtInGroup — {\sqrt} now correctly wraps the radical in an MTMathGroup; expectation updated to MTMathGroup{Radical} / {\sqrt{}}.
• testOverInParens / testAtopInParens — \over/\atop-family commands transform the enclosing group into a parent-level fraction (TeX group-transformation), so these groups must NOT be wrapped. A private _groupWasTransformedByStopCommand flag skips wrapping in that case; both tests keep their original bare-fraction expectations.

Full test suite passes (all 7 test classes).

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces MTMathGroup (representing kMTMathAtomOrdGroup) to properly wrap brace groups {...} in math mode as Ord subformulas. This change scopes interior style nodes (fixing issue #177), allows scripts to target the entire group, and prevents Bin/Ord reclassification from crossing brace boundaries. Feedback on these changes highlights two critical issues in MTMathListBuilder.m: first, prevAtom is not updated when a group is transformed, which can break subsequent scripting; second, the _groupWasTransformedByStopCommand flag can leak to parent groups and should be reset immediately after being read. Additionally, a redundant kMTMathAtomOrdGroup case was identified in MTTypesetter.m since these atoms are already reclassified to kMTMathAtomOrdinary before spacing lookup.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

There are two critical issues in this block:

1. prevAtom is not updated: In the transformed path, sublist is appended to list and the loop continues, but prevAtom is never updated to the last atom of sublist. This causes any subsequent scripts (like ^2) or operators to attach to the wrong atom (or create a new empty atom) instead of the transformed fraction.
2. _groupWasTransformedByStopCommand leak: Since _groupWasTransformedByStopCommand is a shared instance variable, when a nested group is transformed (e.g., {a {b \over c}}), the flag remains YES even after returning to the parent group. This causes grandparent groups to also skip wrapping in MTMathGroup. Resetting it to NO immediately after reading prevents this leak.

            BOOL transformed = _groupWasTransformedByStopCommand;
            _groupWasTransformedByStopCommand = NO;
            if (oneCharOnly || transformed) {
                // Field brace (^{…}, _{…}, \frac{…}, command argument): the {…}
                // *is* the field. Flatten and return it as the field — unchanged.
                // Also: a group-transforming command (\over, \atop, \choose,
                // \brack, \brace) fired inside this group. The resulting fraction
                // replaces the group in the parent list (TeX behavior) — do NOT
                // wrap in MTMathGroup. Fall through to continue after appending.
                prevAtom = [sublist.atoms lastObject];
                [list append:sublist];
                if (oneCharOnly) {
                    return list;
                }
                continue;
            }

[kostub-goldcast]

Verified both points against the current head (adbe7f9).

1. prevAtom not updated — real bug, fixed in adbe7f9. Confirmed by reproducing it: {a \over b}^2 serialized as \frac{a}{b}{}^{2} — the ^2 attached to a spurious empty Ord instead of the fraction, because the transformed branch appended sublist without setting prevAtom (the ^/_/' branches all key off prevAtom). Restored prevAtom = [sublist.atoms lastObject] in that path, matching the pre-grouping behavior and the shared append path below. Regression test testScriptAfterOverTransformedGroupAttachesToFraction added.

2. _groupWasTransformedByStopCommand leak — already fixed in f99517a. This review was filed against b7a705a (3rd commit). The 4th commit f99517a ("Fix stale _groupWasTransformedByStopCommand leaking into enclosing group") landed afterward and does exactly the read-and-clear suggested here — BOOL transformed = _groupWasTransformedByStopCommand; _groupWasTransformedByStopCommand = NO; at MTMathListBuilder.m:244–245, read and cleared in the {…} branch before any fallthrough. The {{a \over b}\scriptstyle c}z leak variant is covered by testBraceGroupingAroundOverTransform. No further change.

[gemini-code-assist]

This case is redundant and never reached because MTMathGroup atoms are always reclassified to kMTMathAtomOrdinary in MTTypesetter.m (line 723) before any inter-element spacing lookup is performed.

[kostub-goldcast]

Confirmed redundant — removed in adbe7f9. createDisplayAtoms reclassifies kMTMathAtomOrdGroup → kMTMathAtomOrdinary (MTTypesetter.m:722–723) before any inter-element lookup, and passes currentType:kMTMathAtomOrdinary explicitly, so this case was unreachable. This matches the direct analog kMTMathAtomBox, which has no switch entry and relies on the same reclassify-before-lookup pattern; the reclassification in createDisplayAtoms is now the single source of truth.

[kostub]

Code review — TeX-faithful brace grouping (MTMathGroup), PR #247

Reviewed against docs/lld/2026-06-30-tex-faithful-brace-grouping.md and the plan. Built and ran the full SPM suite on the branch: 363 tests, 0 failures. The core design (dedicated MTMathGroup / kMTMathAtomOrdGroup, wrap only oneCharOnly == NO main-list braces, render via the existing style-scoping recursion spaced as Ordinary) faithfully implements the LLD and cleanly fixes the base #177 case. Nice work.

However, one of the flagged deviations has a blocking correctness bug.

---

🔴 Blocking: _groupWasTransformedByStopCommand is never reset after it is consumed

MTMathListBuilder.m sets the flag to NO only at the top of each buildInternal: call (line 179) and to YES in stopCommand: (line 1213). The { branch reads it into transformed (line 237) but never clears it. So once an inner group is transformed by \over/\atop/\choose/\brack/\brace, the flag stays YES and leaks to the enclosing group's { branch — because nothing resets it between the inner group returning and the outer group's post-recursion check.

Empirically verified on this branch (temporary probes, reverted):

• {{a \over b}c} → top level [Fraction, Variable] (2 atoms). The outer group is silently dropped; expected a single wrapping MTMathGroup.
• {{a \over b}\scriptstyle c}z → top level [Fraction, Style, Variable, Variable]. The outer group is not wrapped, so \scriptstyle becomes a top-level in-list switch and leaks onto the trailing z — i.e. issue Embedded { } environments are not working #177 is reintroduced, gated behind a leading \over-group.

Root cause: the flag is meant to describe "was the group I just built transformed?", but it is read without being reset, so a YES set by a nested stopCommand survives into the parent's check.

Fix (read-and-clear at the point of consumption, MTMathListBuilder.m:237):

BOOL transformed = _groupWasTransformedByStopCommand;
_groupWasTransformedByStopCommand = NO;   // consume it, so it can't leak to an enclosing group

This restores the sibling-safe behavior that already works ({a\over b}{c} is fine because each buildInternal resets at its top) to the nested case as well. Please add a regression test, e.g. {{a \over b}c} → single MTMathGroup and {{a \over b}\scriptstyle c}z → no leak onto z.

---

🟢 Deviations that are correct / improvements

• appendLaTeXToString: emitting only {inner} (no scripts) is a correct departure from the plan's [str appendString:self.stringValue]. mathListToString: (MTMathListBuilder.m:1504-1511) already appends each atom's ^{…}/_{…}, so the plan's version would have doubled scripts. The split (scripts in stringValue for standalone/error use, none in appendLaTeXToString: for the list serializer) is right and matches how MTFraction etc. behave. Round-trip tests confirm it.
• The flag's primary purpose is sound and TeX-faithful. Keeping testOverInParens / testAtopInParens at their bare-fraction expectations (5+\frac{1}{c}+8, 5+{1 \atop c}+8) correctly models TeX group-transformation, where \over replaces the enclosing group with a generalized fraction rather than nesting it. Only the missing reset above is wrong.
• testSqrtInGroup update ({\sqrt} → MTMathGroup{Radical}, round-trips as {\sqrt{}}) is the semantically correct new behavior.

🟡 Minor / nits

• The flag is a side-channel ivar coupling stopCommand: to the { branch — fragile. Beyond the reset fix, a short comment on the invariant ("set by stopCommand, consumed-and-cleared by the { branch") would help future maintainers, and the nested-group tests above will lock it in.
• The child MTMathListDisplay added directly to _displayAtoms (MTTypesetter.m:729) carries the inner list's 0-based range rather than the group's indexRange. Consistent with the existing \color case and editing/cursor traversal is explicitly out of scope, so acceptable — noting it for whoever later adds MTMathListIndex support.
• The LLD/plan in the repo don't document this \over-in-group interaction or its nesting edge case (LLD §6 covers plain {{…}} only). Worth updating LLD §6 so the deviation and its edge cases are captured with the design.

Verdict

Requesting changes: the stale-flag bug is a real reintroduction of the very leak this PR fixes and should be fixed (one-line reset + regression tests) before merge. Everything else looks good and the suite is green.

[kostub]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces the MTMathGroup class and the kMTMathAtomOrdGroup atom type to represent brace groups {...} in math mode, aligning with TeX's Ord noad with a sub-mlist and KaTeX's ordgroup. This change scopes interior style nodes (such as \scriptstyle) to the group, preventing style leaks to the outer context and resolving issue #177. The math list builder is updated to wrap brace groups appropriately while handling TeX group-transformation commands like \over, and the typesetter is updated to render these groups and apply scripts correctly. Comprehensive unit tests have been added to verify brace grouping, scoping, and spacing. I have no feedback to provide as there are no review comments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.