MTMacroAtom and two-phase finalized

[kostub]

Part 2 of 3 in the modular-arithmetic stack.

Plan: docs/plans/2026-07-25-modular-arithmetic.md
LLD: docs/lld/2026-07-13-modular-arithmetic.md

Goal

The model layer gains a macro atom and the ability to expand it. MTMathList.finalized becomes phase 1 (raw-expand every top-level macro) followed by phase 2 (the existing reclassifying loop, run exactly once). No parser changes — every test in this PR builds MTMacroAtoms by hand, which is precisely what isolates the model-layer contract from the parser.

Commits

1. [item 3] Add MTMacroParameterAtom template placeholder
2. [item 4] Add MTMacroAtom and kMTMathAtomMacro
3. [item 5] Serialize MTMacroAtom as \<command>{arg}
4. [item 6] Split -[MTMathList finalized] into expand + reclassify phases
5. [item 7] Expand macros to raw atoms in finalize phase 1
6. [item 8] Transfer macro scripts onto the last scriptable expansion atom
7. [item 9] Test one-pass expansion equivalence at the model layer

Deviations from the plan (all documented in commit messages)

• NS_UNAVAILABLE kept, plus a runtime @throw (item 4). The plan's Task 4 header snippet marks initWithType:value: NS_UNAVAILABLE, but its own testMacroAtomRejectsGenericInitializer calls that selector directly and expects a runtime throw. NS_UNAVAILABLE is compile-time only, so an id-typed caller walks straight past it — both guards are needed and both are present. (This entry previously said NS_UNAVAILABLE was dropped; that was never true of the shipped code.)
• templateList renamed to templateExpression (item 4, review round 1). The LLD names it template, which is a C++ keyword and so unusable in a public header; the plan's templateList was rejected in review because the property is not a list/array. templateExpression is the third name and the one in the code. PR 3 (Macro registry, template parsing, and the three commands #266) is stacked on the pre-rename tip of this branch and still uses templateList in 4 files — it needs a rebase and rename before it will compile. The plan text at docs/plans/2026-07-25-modular-arithmetic.md also still says templateList in PR 3's chunks.
• Phase-1 hasMacro fast path removed (item 7, review round 1). The plan has expandMacros return self untouched when the list holds no macro. Removed on review direction — a second scan to save a pass is not worth it — so every -finalized now allocates and re-adds. Measured at ~6% on a microbenchmark of 2000 finalized calls; accepted deliberately. The plan's XCTAssertEqual(expanded, list) is inverted to match (testExpandingMacrosCopiesListWithoutMacros).
• Declaration placement (items 4, 6, 7). Several plan snippets place declarations where they do not compile: MTMacroAtom after MTMathGroup/before MTMathSpace (unsatisfiable — MTMathSpace comes first in the header); an @interface...@end category nested inside @implementation...@end in the test file; and the MTMacroAtom () extension after its own use site in MTMathList.m. Each moved to the nearest valid location, no behavioral change.
• Corrected test expectation (item 7). The plan's testFinalizedTracksArgumentMutation expects "(n)"; actual correct output is "\mkern8.0mu(n)", because the template's leading 8mu space has no named command in +spaceToCommands (only 3/4/5/18/36/-3 do) and so serializes via the \mkern%.1fmu fallback. Fixed the assertion, not the code.

Review rounds

• Round 1 (0736def) — 11 inline comments, each answered in its thread.
• Round 2 (c2bd777) — assertion policy. InvalidTemplate and InvalidMacroArgumentIndex demoted from @throw to NSAssert (only library-authored templates can violate them); the depth guard stays a throw because \pmod{\pmod{…}} nests from user LaTeX. Restored the phase-2 kMTMathAtomMacro guard that round 1 dropped. Plus boundary/idempotency tests, selector caching, import-form consistency, and header notes.

Stack

• PR 1 — \bmod as a binary operator (\bmod as a binary operator #264)
• PR 2 (this one) — MTMacroAtom and two-phase finalized
• PR 3 — Macro registry, template parsing, and the three commands

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ts3f5UtUywaqimvE4U1rkw

Summary by CodeRabbit

• New Features

  • Added support for defining and expanding reusable math macros, including macros with arguments.
  • Added macro-aware math list finalization, script handling, nested expansion, and recursion protection.
  • Exposed macro command, arguments, and expansion templates for supported integrations.

• Bug Fixes

  • Improved handling of superscripts and subscripts generated during macro expansion.

• Tests

  • Added comprehensive coverage for macro expansion, serialization, copying, validation, recursion, and script placement.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds public and internal macro atom representations, expands macros during MTMathList finalization with script transfer and recursion protection, updates typesetting/build wiring, and introduces extensive tests for expansion, copying, serialization, and equivalence.

Changes

Macro expansion

Layer / File(s)	Summary
Macro atom contracts iosMath/lib/MTMathList.h, iosMath/lib/internal/MTMacroParameterAtom.h	Adds kMTMathAtomMacro, the public MTMacroAtom API, and the internal MTMacroParameterAtom placeholder type.
Expansion and finalization pipeline iosMath/lib/MTMathList.m, iosMath/render/internal/MTTypesetter.m, iosMath/lib/internal/MTUnicode.h, iosMath/module.modulemap	Expands macros before reclassification, substitutes copied arguments, transfers scripts, guards recursion, and prevents macro atoms from reaching typesetting.
Expansion validation and build wiring iosMathTests/MTModularArithmeticTest.m, Package.swift, iosMath.xcodeproj/project.pbxproj	Adds macro construction, expansion, script-transfer, serialization, and equivalence tests, and wires internal headers and the test source into package and Xcode builds.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MTMathList
  participant MTMacroAtom
  participant MTTypesetter
  MTMathList->>MTMacroAtom: expand invocation and substitute arguments
  MTMacroAtom-->>MTMathList: return expanded atoms
  MTMathList->>MTMathList: finalize and reclassify
  MTMathList-->>MTTypesetter: pass macro-free atoms

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and relevant, capturing the new MTMacroAtom work and the two-phase finalization change.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/modular-arithmetic-pr2

---

_{Comment @coderabbitai help to get the list of available commands.}

[kostub]

Code review — PR 2 (MTMacroAtom and two-phase finalized)

Reviewed the diff against feature/modular-arithmetic-pr1 only (gh pr diff 265, 7 commits, items 3–9), plus the plan's ## PR 2 section and LLD §3.1/§3.3/§3.4/§3.5/§6/§7/§8. Verified in a throwaway worktree at 34dd3f7: swift build, swift build -c release, swift test (480 tests, 0 failures), plus targeted probe tests for the deep-copy, nesting, and script-transfer questions.

Strengths

• The two-phase split is correct and the phase-2 loop provably runs exactly once per list. -finalized is the only caller of -finalizedAssumingNoMacros; every container's -finalized (MTFraction.m line 421, MTInner 637, MTMathGroup 1193, MTMathTable 1282, MTMathStack 1482, …) recurses through the public -finalized, so each sub-list gets exactly one expand + one reclassify. -[MTMathAtom finalized] (line 322) also routes superScript/subScript through the public -finalized, so a macro buried in a script list is expanded too — that path isn't tested but it works.
• Deep-copy correctness holds under repeated finalize, including nested macros. I probed a \pod{\pod{n}^7}^2 built by hand and finalized it three times: stable output ((n)^{7})^{2} every call, with the raw list still serializing as \pod{\pod{n}^{7}}^{2}. The chain that makes this work is worth calling out because it's easy to break: -expansion copies each template atom ([templateAtom copy], deep per MTFraction/MTInner/etc. overrides) and each argument ([self.arguments[i] copy], deep via -[MTMathList copyWithZone:]'s copyItems:YES), so -transferScriptsToExpansion: only ever mutates freshly-minted atoms.
• Recursion for nested macros is right. -expansion re-scans via [out mathListByExpandingMacros] before transferring scripts, so the outer script never lands on an unexpanded MTMacroAtom. Inner-then-outer ordering falls out correctly.
• The script-transfer target rule matches TeX. I checked the case the tests don't cover — \mod{n+1}^2, where the template ends in #1 — and it finalizes to \mathrm{mod}n+1^{2}, i.e. ^2 binds to the 1. That is exactly what amsmath's \newcommand{\mod}[1]{…\mkern18mu#1} does, since TeX strips the argument braces on substitution. Good.
• All three documented deviations are sound, and I verified the third empirically. (a) Dropping NS_UNAVAILABLE is the only resolution consistent with testMacroAtomRejectsGenericInitializer; MTMathColorbox is the right precedent. (b) The declaration moves are forced — MTMacroAtom now sits after MTMathGroup (line 708), which is the plan's intent minus the impossible "before MTMathSpace" clause. (c) \mkern8.0mu(n) is genuinely correct: 8 is not in +spaceToCommands, so MTMathSpace uses the \mkern%.1fmu fallback. Fixing the assertion rather than the code was the right call.
• MTMacroParameterAtom as an isKindOfClass:-detected sentinel rather than a new public enum value is the right trade, and -copyWithZone: correctly restores _argumentIndex after [super copyWithZone:] round-trips through -initWithType:value:.

---

Issues

Important

1. Three new compiler warnings on a previously warning-clean build — one of them load-bearing.

feature/modular-arithmetic-pr1 builds with zero warnings (swift build). This PR adds three, in both debug and release:

MTTypesetter.m:615:17: warning: enumeration value 'kMTMathAtomMacro' not handled in switch [-Wswitch]
MTMathList.m:1849:1:  warning: convenience initializer missing a 'self' call to another initializer [-Wobjc-designated-initializers]
MTMathList.m:1967:17: warning: method override for the designated initializer of the superclass '-initWithType:value:' not found [-Wobjc-designated-initializers]

The -Wswitch one matters beyond tidiness. -[MTTypesetter createDisplayAtoms:] has no default: clause — -Wswitch is the project's mechanism for guaranteeing every atom type is handled. Leaving kMTMathAtomMacro unhandled means a leaked macro atom is silently skipped by the display loop, then becomes prevNode, and the next atom's addInterElementSpace: calls getInterElementSpaceArrayIndexForType(22, …) (MTTypesetter.m:49), which falls to default: → NSCAssert(false) + return -1. With NS_BLOCK_ASSERTIONS=1 (set in iosMath.xcodeproj/project.pbxproj:694 for Release) that's a NSUInteger(-1) array index — an out-of-bounds read rather than a diagnosable failure.

Fix: add case kMTMathAtomMacro: alongside the existing kMTMathAtomNumber/kMTMathAtomVariable/kMTMathAtomUnaryOperator "should never show here" group at MTTypesetter.m:616-621. That silences the warning and documents the invariant at the only place it can be violated.

For the other two: MTMacroAtom's throwing -initWithType:value: needs NS_DESIGNATED_INITIALIZER on the header declaration (a class may have several), or the MTMathColorbox shape of calling [self init] first. MTMacroParameterAtom's warning goes away by dropping NS_DESIGNATED_INITIALIZER from -initWithArgumentIndex: — which is arguably more honest anyway, since its -copyWithZone: deliberately depends on MTMathAtom's -initWithType:value: still being reachable.

2. -[MTMacroAtom expansion] substitutes only top-level #N; a placeholder inside a container atom is silently carried into the output.

MTMathList.m -expansion walks self.templateList.atoms flat and never descends into innerList/numerator/script lists. I confirmed the behavior with a template ( {#1} ) where the #1 sits inside an MTMathGroup: phase 1 returns it unsubstituted, and the raw expansion serializes as ({#1}). The argument is dropped entirely.

Nothing in this PR reaches it, and PR 3's three registry templates (\mkern8mu(\mathrm{mod}\mkern6mu#1), \mkern12mu\mathrm{mod}\mkern6mu#1, \mkern8mu(#1)) are all flat — \mathrm{…} sets fontStyle rather than producing a container. But PR 3 is where templates become LaTeX strings, and {#1} or \frac{#1}{2} is exactly how a contributor would naturally write one. The failure mode is bad: in debug, the NSAssert in -finalizedAssumingNoMacros fires late and from a confusing place (the group's inner list, one recursion level down); in release, it renders a literal #1.

Cheapest durable fix: validate in -initWithCommand:arguments:templateList: that no MTMacroParameterAtom occurs below the top level of templateList, and @throw if one does (fail loud, at the point where the mistake was made). Alternatively make substitution recurse into sub-lists — but that's real scope, and rejecting the template is enough to make PR 3 safe.

3. iosMath.xcodeproj never sees the new files, so none of these tests run under the documented iOS test command.

MTMacroParameterAtom.h has 0 references in iosMath.xcodeproj/project.pbxproj, while its peer internal header MTUnicode.h has 2. More significantly, MTModularArithmeticTest.m also has 0, while MTTypesetterTest.m and MTMathListBuilderTest.m have 4 each. So xcodebuild test -project iosMath.xcodeproj -scheme iosMath … — the command CLAUDE.md documents for iOS — compiles and runs none of the 40+ tests this PR adds (or PR 1's). Only swift test exercises them.

The MTModularArithmeticTest.m omission originates in PR 1, not here, so fix it wherever is least disruptive to the stack — but it should be fixed before PR 3 lands, or the iOS target silently loses coverage for the whole feature.

Minor

4. +[MTMathAtom atomWithType:kMTMathAtomMacro value:] mints a malformed atom. atomWithType: (MTMathList.m:162) has a case for every other structured type (Fraction, Radical, Inner, Stack, Text, Box, OrdGroup, Space, Color, Colorbox…) but not Macro, so it falls to default: and returns a plain MTMathAtom with type == 22. -mathListByExpandingMacros dispatches on atom.type == kMTMathAtomMacro and then casts unconditionally, so [list finalized] on such an atom dies with -[MTMathAtom expansion]: unrecognized selector (verified). It's loud, but opaque for a library consumer. Two options, either is fine: add a case kMTMathAtomMacro: to the factory that throws the same clear "use -initWithCommand:arguments:templateList:" message; and/or have phase 1 dispatch on isKindOfClass:[MTMacroAtom class] (which is already the idiom used for MTMacroParameterAtom two lines away) instead of on the mutable public type property.

5. The out-of-range argument index degrades silently in shipping builds. In -expansion:

NSAssert(index >= 1 && index <= self.arguments.count, …);
if (index < 1 || index > self.arguments.count) {
    continue;
}

With NS_BLOCK_ASSERTIONS=1 the assert vanishes and the continue silently drops the placeholder — the argument disappears from the rendered output with no signal. Same applies to the two NSAsserts guarding the reclassifying loop. Given the repo's fail-loud stance, @throw (as -initWithType:value: already does) would be more consistent than NSAssert + fallback for the arity mismatch specifically, since it is unambiguously a programming error.

6. testFinalizedUnchangedForMacroFreeLists doesn't test what its comment claims. Its comment promises "same Bin/Unary reclassification, same number fusion, same index ranges," but the body only asserts (a) finalized != list and (b) that two successive mathListToString: calls agree. It would pass with phase 2 completely broken, as long as it were deterministic. Worse, it compares via serialization — the exact lossiness the file's own AtomSignature comment calls out ("\bmod and a demoted \bmod stringify identically"), so the Bin/Unary distinction it names is invisible to it. ListSignature() already exists further down the file; asserting literal expected signatures for x+, -x, 1\times 23 would make it a real regression test. (The suite's other 480 tests are the actual safety net here, so this is a strength-of-test issue, not a correctness one.)

7. -[MTMacroAtom copyWithZone:] hardcodes the class. [[MTMacroAtom allocWithZone:zone] initWithCommand:…] should be [[[self class] allocWithZone:zone] …] — every other -copyWithZone: in the file gets this via [super copyWithZone:]'s [self class]. A future MTMacroAtom subclass would silently copy to the wrong class.

8. Undocumented aliasing in phase 1's output. When the list does contain a macro, -mathListByExpandingMacros copies nothing for the non-macro atoms — [expanded addAtom:atom] shares the receiver's atom objects (verified). That's safe as used (phase 2 copies via [atom finalized]), but the header comment only mentions the no-macro identity case. Worth one sentence, since the tests call this private method directly and a future caller could reasonably assume the result is detached.

9. Zero-argument macros don't round-trip. -appendLaTeXToString: emits \command with no terminator when arguments is empty, so a macro followed by a variable serializes as \noargsx (verified). Unreachable with PR 3's arity-1 registry, but if \newcommand (LLD §8.2) ever lands it will need the trailing-space handling the rest of the serializer does.

Notes (no action needed)

• Unbounded expansion recursion. -expansion ↔ -mathListByExpandingMacros recurse with no depth budget, so a self-referential template would blow the stack. LLD §8 explicitly defers this to the \newcommand roadmap and states the three built-ins terminate by construction — correctly scoped out. Flagging only so it isn't forgotten when the registry becomes writable.
• indexRange divergence between the raw and expanded lists is a non-issue per LLD §3.5 (assigned by finalization, consumed only by the editor's cursor) — confirmed against -finalizedAssumingNoMacros's zero-range branch.
• Publicly re-declaring -initWithType:value: on MTMacroAtom widens the public surface slightly (MTMathAtom keeps it in a class extension). It's what makes the test compile and it's documented, so this is fine — just noting it's a deliberate API-surface choice, not an accident.

---

Assessment

Ready to merge: with fixes.

Reasoning: The core design — two-phase finalized, deep-copy-on-expand, re-scan-then-transfer-scripts — is correct, and I could not break the properties that matter (idempotent repeated finalize, pristine templates/arguments, nested macros, exactly-one reclassify pass) under direct probing. The blocking work is hygiene rather than logic: restore the warning-clean build (issue 1, where the -Wswitch gap removes the project's only guard against an unhandled atom type reaching the typesetter), close the nested-placeholder hole before PR 3 turns templates into LaTeX strings (issue 2), and get the new files into iosMath.xcodeproj so the iOS scheme actually runs this coverage (issue 3).

Reviewed against 34dd3f7.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

iosMath/lib/MTMathList.m (1)

1727-1754: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Phase 1 dispatches on type but hard-casts to MTMacroAtom.

kMTMathAtomMacro is a public enum value, so [MTMathAtom atomWithType:kMTMathAtomMacro value:@""] yields a plain MTMathAtom that passes this check and then gets -expansion sent to it — unrecognized selector. A class check makes the cast sound.

♻️ Class-based dispatch

-    BOOL hasMacro = NO;
-    for (MTMathAtom* atom in self.atoms) {
-        if (atom.type == kMTMathAtomMacro) {
+    BOOL hasMacro = NO;
+    for (MTMathAtom* atom in self.atoms) {
+        if ([atom isKindOfClass:[MTMacroAtom class]]) {
             hasMacro = YES;
             break;
         }
     }
@@
-    for (MTMathAtom* atom in self.atoms) {
-        if (atom.type != kMTMathAtomMacro) {
+    for (MTMathAtom* atom in self.atoms) {
+        if (![atom isKindOfClass:[MTMacroAtom class]]) {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@iosMath/lib/MTMathList.m` around lines 1727 - 1754, Update
mathListByExpandingMacros to dispatch only when an atom is both typed
kMTMathAtomMacro and an instance of MTMacroAtom before casting and calling
expansion. Leave non-MTMacroAtom instances in the carried-through path
unchanged.

iosMathTests/MTModularArithmeticTest.m (1)

634-661: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Signature ignores container sublists.

AtomSignature descends into scripts only, so a divergence inside a fraction/radical/inner list would compare equal. Recursing on the known container lists would keep these equivalence tests honest as arguments get richer.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@iosMathTests/MTModularArithmeticTest.m` around lines 634 - 661, Extend
AtomSignature to recurse into every supported container sublist, including
fraction numerator/denominator, radical radicand, and inner-list contents, in
addition to superScript and subScript. Use the existing ListSignature helper and
the atom-type-specific container properties so divergences within nested
structures produce different signatures.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@iosMath/lib/MTMathList.m`:
- Around line 1901-1927: Add a bounded expansion-depth guard to the MTMacroAtom
expansion flow centered on expansion and mathListByExpandingMacros, so
self-referential arguments cannot recurse indefinitely. Track depth across
nested macro expansions, assert or abort once a fixed maximum is exceeded, and
preserve normal expansion behavior below that limit.

---

Nitpick comments:
In `@iosMath/lib/MTMathList.m`:
- Around line 1727-1754: Update mathListByExpandingMacros to dispatch only when
an atom is both typed kMTMathAtomMacro and an instance of MTMacroAtom before
casting and calling expansion. Leave non-MTMacroAtom instances in the
carried-through path unchanged.

In `@iosMathTests/MTModularArithmeticTest.m`:
- Around line 634-661: Extend AtomSignature to recurse into every supported
container sublist, including fraction numerator/denominator, radical radicand,
and inner-list contents, in addition to superScript and subScript. Use the
existing ListSignature helper and the atom-type-specific container properties so
divergences within nested structures produce different signatures.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 143dcd46-bfaf-49cb-835e-961df4a70382

📥 Commits

Reviewing files that changed from the base of the PR and between e278be3 and 36d72d2.

📒 Files selected for processing (4)

• iosMath/lib/MTMacroParameterAtom.h
• iosMath/lib/MTMathList.h
• iosMath/lib/MTMathList.m
• iosMathTests/MTModularArithmeticTest.m

[kostub]

Remove these kind of comments which refer documents that aren't checked in.

[kostub]

Removed. Dropped the Design: docs/lld/... lines here and in MTMathList.h / MTMathList.m.

[kostub]

should this go into iosMath/lib/internal? like the render/internal?

[kostub]

Yes. Created iosMath/lib/internal/ and moved both MTMacroParameterAtom.h and MTUnicode.h into it — MTUnicode.h was sitting in the public lib/ dir for the same reason.

Wiring: .headerSearchPath("lib/internal") on the iosMath target and ../iosMath/lib/internal on both test targets in Package.swift; $(SRCROOT)/iosMath/lib/internal added to HEADER_SEARCH_PATHS (Debug + Release) plus a matching lib/internal group in iosMath.xcodeproj. Neither header is in module.modulemap, so the Swift surface is unchanged.

[kostub]

why is it called templateList? it is not a list/array. should be template or templateExpression

[kostub]

Renamed to templateExpression. Not plain template — this header is includable from ObjC++, where template is a keyword.

[kostub]

same comment as above. Not templateList

[kostub]

Renamed here too.

[kostub]

mark it NS_UNAVAILABLE instead.

[kostub]

Done — initWithType:value: is now NS_UNAVAILABLE on MTMacroAtom.

Kept the runtime @throw alongside it: NS_UNAVAILABLE is compile-time only, so an id-typed caller walks straight past it. testMacroAtomRejectsGenericInitializer covers that path. Same reasoning for +atomWithType:kMTMathAtomMacro, which now throws instead of falling through to default: and minting a plain MTMathAtom carrying type 22.

[kostub]

Don't call it phase 1 of "finalised". It is just expand macros. I think just rename the method to "expandMacros"

[kostub]

Renamed to expandMacros, and the phase wording is gone from the docs.

[kostub]

Keep the name of the method as "finalized". document that macros should already be expanded. The first line for finalized should be to expand macros. Separate method is not needed.

[kostub]

Done. finalized keeps its name, finalizedAssumingNoMacros is gone, and expansion is its first line:

- (MTMathList *)finalized
{
    MTMathList* expanded = [self expandMacros];
    ...

Its doc comment now states why expansion has to come first: finalization is irreversible and context-dependent (a Bin demoted to Unary at one boundary cannot be restored), so the reclassifying pass must see the flat raw stream a macro stands for rather than the macro atom.

[kostub]

remove references to LLD

[kostub]

Removed.

[kostub]

Why is this pass needed? Second pass handles this just fine.

[kostub]

Removed — the main loop handles it. expandMacros is now a single pass.

[kostub]

Review round addressed in 0736def. Inline threads have per-item replies; the two nitpicks from the review summary (not repliable in-thread) are covered here:

Class-based dispatch instead of type — done, and extended beyond the diff you flagged. expandMacrosAtDepth: dispatches on isKindOfClass:[MTMacroAtom class], and +atomWithType: now throws on kMTMathAtomMacro so the plain-MTMathAtom-carrying-type-22 object cannot be constructed in the first place (testAtomFactoryRejectsMacroType).

AtomSignature ignores container sublists — done. It now recurses through numerator, denominator, degree, radicand and innerList in addition to the scripts, via respondsToSelector:. The finalization-equivalence tests also assert literal signatures now rather than only comparing two computed ones, e.g. \frac{1+2}{3-} → [10:numerator[2:1, 5:+, 2:2]denominator[2:3, 6:−]].

Verification on this commit: swift build and swift build -c release clean (0 warnings), swift test 486 tests / 0 failures, xcodebuild test -project iosMath.xcodeproj -scheme iosMath -sdk iphonesimulator -destination 'platform=iOS Simulator,name=iPhone 16' 445 tests / 0 failures, xcodebuild build -project MacOSMath.xcodeproj -scheme MacOSMath -sdk macosx succeeds. The iOS count went up because MTModularArithmeticTest.m was previously SPM-only and is now in the Xcode test target.

[coderabbitai]

🧹 Nitpick comments (1)

iosMathTests/MTModularArithmeticTest.m (1)

323-335: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a boundary test at the exact depth limit.

Only far-under (8, no throw) and far-over (64, throw) cases are tested. depth >= kMTMaxMacroExpansionDepth is exactly the kind of comparison prone to off-by-one errors; a test at 32 vs 33 nested macros would directly pin down the boundary.

✅ Suggested boundary test

// Boundary: exactly at the limit should still succeed; one more should throw.
MTMathList* atLimit = [MTMathList new];
[atLimit addAtom:NestedPodChain(32)];
XCTAssertNoThrow([atLimit finalized]);

MTMathList* overLimit = [MTMathList new];
[overLimit addAtom:NestedPodChain(33)];
XCTAssertThrows([overLimit finalized]);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@iosMathTests/MTModularArithmeticTest.m` around lines 323 - 335, Add
exact-boundary coverage to testRunawayExpansionDepthThrows: verify
NestedPodChain(32) finalizes without throwing and NestedPodChain(33) throws,
preserving the existing shallow and runaway cases.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@iosMathTests/MTModularArithmeticTest.m`:
- Around line 323-335: Add exact-boundary coverage to
testRunawayExpansionDepthThrows: verify NestedPodChain(32) finalizes without
throwing and NestedPodChain(33) throws, preserving the existing shallow and
runaway cases.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 08fe2a1c-feb7-40b5-868c-d8169f3abc3c

📥 Commits

Reviewing files that changed from the base of the PR and between 36d72d2 and 0736def.

📒 Files selected for processing (10)

• Package.swift
• iosMath.xcodeproj/project.pbxproj
• iosMath/lib/MTMathList.h
• iosMath/lib/MTMathList.m
• iosMath/lib/MTUnicode.m
• iosMath/lib/internal/MTMacroParameterAtom.h
• iosMath/lib/internal/MTUnicode.h
• iosMath/module.modulemap
• iosMath/render/internal/MTTypesetter.m
• iosMathTests/MTModularArithmeticTest.m

🚧 Files skipped from review as they are similar to previous changes (2)

• iosMath/lib/MTMathList.h
• iosMath/lib/MTMathList.m

[kostub]

Code review — 0736def (head)

Reviewed e278be3..0736def (8 commits, 10 files). Verified by building and running the full suite from a throwaway worktree at head: swift build clean (no warnings), swift test 486/486 pass. Each finding below was confirmed with a throwaway probe test rather than inferred from the diff.

Verdict: ready to merge with fixes. No Critical issues.

---

Strengths

• The two-phase split is correct, and item 9 proves it rather than asserting it. signatureForModMacroWithArgument: vs signatureForWrittenOutModWithArgument: compares structural fingerprints, and the comment explaining why serialization can't be used (latexSymbolNameForAtom: collapses Unary back through the Bin cell) shows the trap was understood. x\mod{n+}y keeping + as Bin is exactly the defect the redesign exists to prevent.
• Deep-copy discipline holds everywhere. Init deep-copies both arguments and templateExpression; -expansionAtDepth: copies every template atom and every argument list; -transferScriptsToExpansion: copies the scripts and only mutates atoms it owns. All three paths traced and probed — the macro atom stays pristine across repeated -finalized.
• The depth bound is on the right axis. expandMacrosAtDepth: passes depth unchanged to siblings and -expansionAtDepth: passes depth + 1 down, so 32 counts nesting levels, not total expansions — a 40-placeholder template doesn't false-positive. It throws RunawayMacroExpansion with the command name before any state is mutated. Verified: NestedPodChain(64) throws, NestedPodChain(8) doesn't.
• The 0736def fixes are real improvements, not box-ticking. Converting the arity guard from NSAssert-and-continue to @throw is right under NS_BLOCK_ASSERTIONS (dropping a placeholder would make an argument silently vanish in shipping builds). +atomWithType: throwing for kMTMathAtomMacro closes a real hole. [[self class] allocWithZone:] instead of the plan's hardcoded [MTMacroAtom allocWithZone:] is subclass-correct. The zero-argument trailing space (\noargs x) is a genuine round-trip bug caught proactively.
• MTContainsMacroParameter's duck-typed key list is complete for every container in MTMathList.h today — including the least obvious one, MTMathTable.cells (nested NSArray<NSArray<MTMathList *>>), which probes confirm is correctly caught.
• Build-system hygiene is thorough. The lib/internal/ move is consistent across all three SwiftPM targets, both iOS Xcode configs, the group structure, and the modulemap comment. MacOSMath.xcodeproj correctly needed no change (it compiles only AppDelegate.m/main.m, not library sources). iosMathModuleConsumerTests passes, confirming no public-header leak.

---

Issues

Critical (must fix)

None. No LaTeX-reachable defect, no data loss, no crash on any path this PR exposes.

Important (should fix)

1. The placeholder-escape guard is NSAssert-only and is defeatable two ways the init-time validation doesn't cover — MTMathList.m:1791

Init-time nested-#N rejection (MTMathList.m:1891-1902) validates only templateExpression. Two escapes, both probed:

• A placeholder inside an argument — arguments are never validated at all.
• A template mutated after init — templateExpression is readonly but hands back a mutable MTMathList, so [macro.templateExpression addAtom:groupContainingPlaceholder] runs no validation.

Both reach phase 2 and trip the NSAssert at MTMathList.m:1791 in Debug. Under NS_BLOCK_ASSERTIONS (Release) both proceed silently and render a literal #1 glyph. This is the same case that this very commit fixed correctly one method away, at MTMathList.m:2001, where the arity guard became a @throw for exactly this reason — so the treatment is internally inconsistent.

Not LaTeX-reachable today (MTMacroParameterAtom is internal, and PR 3 parses arguments in normal mode where # isn't special). But PR 3 adds buildTemplate: and LLD §8.2 adds user-supplied templates, at which point it becomes reachable.

Fix: make it @throw (InvalidMacroParameter), matching line 2001; optionally also run MTContainsMacroParameter over arguments in the initializer so both inputs are validated symmetrically.

2. The planned kMTMathAtomMacro phase-2 guard was dropped, and the fallback silently drops the atom in Release — MTMathList.m:1782-1791, MTTypesetter.m:619-623

Commit a4b1801 had NSAssert(atom.type != kMTMathAtomMacro, …); 0736def removed it when switching to isKindOfClass: dispatch. The two changes are orthogonal — the dispatch fix didn't require dropping the guard.

Probed: a plain MTMathAtom with its settable public type forced to kMTMathAtomMacro sails through expandMacrosAtDepth: (which now dispatches on class), survives -finalized with type 22 intact, and reaches MTTypesetter -createDisplayAtoms:, where the new case kMTMathAtomMacro: is NSAssert(NO); break; — a silent drop in Release. Inconsistent with the +atomWithType: throw added in the same commit, which closes the other door to a type-22 non-MTMacroAtom.

Deliberate misuse is required, so severity is bounded — but it's a planned guard removed without being listed as a deviation, and it's exactly the "macro reaching the typesetter" risk this design set out to prevent.

Fix: restore the guard in phase 2 as a @throw, or reject kMTMathAtomMacro in -[MTMathAtom setType:].

3. Templates cannot forward #N into a nested macro — a hard blocker for LLD §8.2 that isn't documented — MTMathList.m:1891-1902

Probed: initWithCommand: rejects a template whose atoms include an MTMacroAtom carrying #1 in its arguments ("Template for \outer nests a #N placeholder inside MTMacroAtom"). Fail-loud is the right behavior given that -expansionAtDepth: substitutes only at the template's top level — leaking would be worse.

This does not block PR 3: the planned built-in templates (plan lines 1705, 1726, 1939, 1941) — \mkern8mu(\mathrm{mod}\mkern6mu#1) and \mkern12mu\mathrm{mod}\mkern6mu#1 — both keep #1 at the top level, since (/) parse as separate Open/Close atoms.

But it means the LLD §8.2 \newcommand follow-on cannot express the most common user template shape, \frac{#1}{#2} — it throws at definition time. Amsmath's own \pmod is \pod{{\rm mod}\mkern6mu#1} (LLD line 115), i.e. exactly the composite form this rejects; the plan sidesteps it by flattening. That constraint deserves a @note on MTMacroAtom in the public header and a line in the LLD's future-work section, so it isn't rediscovered mid-PR.

4. Undocumented deviation: templateList → templateExpression (public API)

The plan pins templateList and explains its own deviation from the LLD's template (C++ reserved word in a public header). templateExpression is a third name with no stated rationale. The name itself is fine — the issue is that it's public API on MTMathList.h, and the plan still references templateList in ~6 places inside PR 3's chunks (plan lines 1994, 2012, 2112, 2332), which will now fail to apply cleanly. Add it to the deviation list and update the plan text before PR 3 starts.

Minor (nice to have)

5. Phase-1 fast path removed; ~6% slower -finalized on every list — MTMathList.m:1843-1864, test at MTModularArithmeticTest.m:437-447

a4b1801/be62477 had if (!hasMacro) return self;. 0736def removed it and rewrote the plan's XCTAssertEqual(expanded, list) into testExpandingMacrosCopiesListWithoutMacros, asserting the opposite. Now every -finalized call — on every list and every nested sub-list, on the typesetting hot path — allocates an extra MTMathList + NSMutableArray and re-adds every atom through addAtom: (with its per-atom isAtomAllowed: check).

Measured with measureBlock, 2000 finalized calls on \frac{1+2}{3-4}+\sqrt{x^2+y^2}-\int_0^1 f(x)dx: 0.070s at head vs 0.066s with the fast path restored using isKindOfClass: as the predicate. Small, but a pure regression against an explicit plan step, and the review feedback that prompted it (dispatch on class, not type) doesn't require it — the scan can just use isKindOfClass:. Worth confirming whether this was intentional.

6. NSParameterAsserts on construction are Release no-ops — MTMathList.m:1885-1887, 2057

templateExpression:nil in Release yields a macro that expands to nothing, silently. initWithArgumentIndex:0 produces nucleus "#0" — though that one at least degrades to the InvalidMacroArgumentIndex throw at expansion time. Internal-construction-only paths, so low reachability, but the same class of issue this commit fixed elsewhere.

7. Stale comment references a method that no longer exists — MTMathList.m:2059

"(it must not — finalizedAssumingNoMacros asserts)". finalizedAssumingNoMacros was collapsed back into -finalized in this very commit.

8. MTContainsMacroParameter recomputes selectors per visited object — MTMathList.m:128-142

Only the key strings are dispatch_onced; NSSelectorFromString runs 13× per object visited, on every MTMacroAtom construction (i.e. per \pmod in the source). Hoist to a static SEL[]. Negligible today, but free to fix.

9. Include-path style is inconsistent — MTTypesetter.m:15 uses "../../lib/internal/MTUnicode.h" while MTUnicode.m:12 and MTMathList.m:15 use "internal/…". Since lib/internal is now on the header search path in all three build systems, a bare #import "MTUnicode.h" works everywhere. Pick one form.

10. Test gap: no re-finalize idempotency check. testExpansionLeavesMacroAtomPristine calls -finalized twice on the original list; nothing asserts list.finalized.finalized == list.finalized for a list that contained a macro. Two lines, and it's the invariant the typesetter depends on.

11. MTMacroAtom doesn't override -finalized, so [macroAtom finalized] on a lone atom returns another macro atom rather than the expansion. Defensible (no list context), but undocumented in the header.

---

Two corrections to the PR description

• Deviation 1 is stale. NS_UNAVAILABLE was not dropped — it's present at MTMathList.h:729, and the test reaches the runtime guard through an id (MTModularArithmeticTest.m:256-258), which gets both compile-time and dynamic protection. The code is better than the description claims.
• An undocumented deviation exists: the phase-1 fast-path removal (issue 5 above), which also inverted the plan's assertion.

Deviations 2 and 3 as described both check out: the declaration-placement moves are non-behavioral, and "\mkern8.0mu(n)" is genuinely correct — the 8mu space has no named command in +spaceToCommands (only 3/4/5/18/36/-3), so it serializes via the \mkern%.1fmu fallback.

---

Recommendations

1. Make the two invariant guards throw rather than assert (issues 1, 2). The rule this PR already applies correctly at line 2001 — an invariant a caller can violate must fail loud in Release — should apply uniformly to all three macro guards. That single principle resolves both Important items.
2. Record deviations 4 (templateExpression rename) and 5 (fast-path removal) in the PR description, and patch the PR 3 plan chunks that still say templateList before starting PR 3.
3. Document the flat-template constraint (issue 3) in the header now, while the reasoning is fresh, rather than during the §8.2 PR.

Assessment

Ready to merge: with fixes.

The architecture is sound, the deep-copy and depth-bounding hold under adversarial probing, and the equivalence tests genuinely lock the invariant that motivated the two-phase design — 486/486 tests pass on a warning-clean build. What holds it back is small and mechanical: two invariant guards that no-op in Release (one of which this same commit fixed correctly one method away), plus two undocumented deviations that will desync PR 3's plan if left unrecorded.

🤖 Generated with Claude Code

[kostub]

Correction to the review above — assertion policy

The review's central recommendation ("make the two invariant guards throw rather than assert") applies the wrong rule for this project. Correcting it here so the record isn't misleading.

The policy: NSAssert/NSParameterAssert guard internal invariants — programmer bugs, which should be caught in testing, not in production. Exceptions are reserved for errors a user can cause that must still be signalled in a Release build. That NS_BLOCK_ASSERTIONS makes an assert a no-op in Release is not a defect in itself; it's the intended outcome for a condition the user cannot trigger or fix. The right first question is who can violate this invariant, not does this fire in Release.

Re-scoring each guard

Guard	Who can trigger it	Correct form
MTMathList.m:1791 — placeholder escape	internal only (MTMacroParameterAtom is not public API)	NSAssert — as shipped
MTMathList.m:2001 — InvalidMacroArgumentIndex	built-in templates only	should be NSAssert
MTMathList.m:1891 — InvalidTemplate (nested #N)	built-in templates only	should be NSAssert
RunawayMacroExpansion (depth 32)	user LaTeX — \pmod{\pmod{…}} nests straight from source	@throw — correct
MTMathList.m:1885, :2057 — NSParameterAssert	internal construction	as shipped

What changes

• Issue 1 is withdrawn. NSAssert is the correct guard at :1791. Further, the Release behavior flagged there — rendering a literal #1 — is not a silent degradation but the visible fallback that MTMacroParameterAtom's own comment at :2052 describes as deliberate ("it shows up as literal #1 rather than crashing on an unhandled enum value"). That is exactly what a bug-guard's Release path should do: stay visible, never drop content.
• Issue 6 is withdrawn. Internal-construction-only NSParameterAsserts being no-ops in Release is intended, not a latent defect.
• Issue 2 stands, but the fix flips. The real defect is that 0736def deleted the phase-2 guard outright, so it no longer fires even in Debug — which is the build where it earns its keep. Restore it as an NSAssert, not a @throw.

And one thing the review got backwards

It listed under Strengths: "Converting the arity guard from NSAssert-and-continue to @throw is the right call under NS_BLOCK_ASSERTIONS." Under this policy that conversion went the wrong direction. The code comment 0736def added at :2001 justifies the throw by calling the condition "unambiguously a programming error" — which is precisely the case for an assert, not an exception. The same applies to the InvalidTemplate throw at :1891. Both are candidates for reverting to NSAssert.

Where the assert-only path would otherwise drop content silently in Release, the fix is to keep the assert and make the Release fallback visibly wrong (render the literal #N) rather than to escalate to an exception — which satisfies both this policy and the fail-loud principle.

Open judgment call

+atomWithType: rejecting kMTMathAtomMacro sits at a public-API boundary: it's an app developer's bug, caught in their testing, but it crosses the library seam. Assert or throw is defensible; the PR currently throws. Flagging rather than deciding.

Unaffected

Issues 3 (flat-template constraint), 4 (templateExpression rename desyncing PR 3's plan), 5 (phase-1 fast-path removal, ~6% on -finalized), and 7–11 stand as written, along with the two corrections to the PR description.

Revised assessment: still "ready to merge with fixes" — but the fix list shrinks to issue 2 (restore the deleted guard as an assert), the two undocumented deviations, and the minors.

🤖 Generated with Claude Code

[kostub]

Review round 2 — c2bd777

CodeRabbit nitpick: boundary test at the exact depth limit

Added, and the boundary is one off from the suggested reading. NestedPodChain(32) is the last chain that succeeds, not the first that fails: the outermost macro expands at depth == 0, so a 32-link chain reaches depth == 31 and clears depth >= kMTMaxMacroExpansionDepth. testRunawayExpansionDepthThrows now pins 8 / 32 (no throw) and 33 / 64 (throw).

Assertion policy — two guards demoted from @throw to NSAssert

The 0736def round escalated guards toward exceptions on the grounds that NS_BLOCK_ASSERTIONS makes an assert a no-op in Release. That is the wrong test for this project. The question is who can violate the invariant: NSAssert guards programmer bugs, meant to be caught in testing; exceptions are for errors the person writing the LaTeX can cause and that must still signal in a shipping build.

Guard	Who can trigger it	Now
InvalidTemplate — #N nested in a sub-list	library-authored templates only	NSAssert
InvalidMacroArgumentIndex — arity mismatch	library-authored templates only	NSAssert
RunawayMacroExpansion — depth 32	user LaTeX (\pmod{\pmod{…}})	@throw, unchanged
placeholder escape, NSParameterAsserts	internal only	NSAssert, unchanged

Where the assert-only path would otherwise lose content in Release, the fallback is made visibly wrong rather than lossy: an out-of-range placeholder is now carried into the output and renders as a literal #2, instead of being dropped so the argument silently vanishes. That satisfies fail-loud without escalating a bug-guard to an exception.

Restored the phase-2 macro guard

0736def deleted NSAssert(atom.type != kMTMathAtomMacro, …) while switching expansion to isKindOfClass: dispatch. The two changes were orthogonal and the guard still earns its keep in Debug: -type is a settable public property, so a plain MTMathAtom relabelled kMTMathAtomMacro is not an MTMacroAtom, walks through expansion untouched, and reaches MTTypesetter, where case kMTMathAtomMacro: drops it. testFinalizedRejectsNonMacroAtomTypedAsMacro covers it.

Also in this commit

• testRefinalizingExpandedListIsIdempotent — list.finalized.finalized on a list that contained a macro, which is the invariant the typesetter relies on and was previously only tested for macro-free lists.
• MTContainsMacroParameter caches its selectors. It ran NSSelectorFromString 13× per visited object, on every MTMacroAtom construction.
• Import form unified to bare #import "MTUnicode.h" / "MTMacroParameterAtom.h", matching the existing MTFont+Internal.h idiom now that lib/internal is on the header search path in all three build systems.
• Header notes on MTMacroAtom: -finalized on a lone macro atom does not expand (only -[MTMathList finalized] does), and templates must keep #N at the top level — so \frac{#1}{#2} is not expressible, which matters for the \newcommand follow-on but not for any built-in template.
• Removed a stale comment referencing finalizedAssumingNoMacros, which 0736def deleted.

Deliberately not done

The hasMacro fast path stays out. Per review direction on the round-1 thread. It costs ~6% on a microbenchmark of 2000 -finalized calls over a mixed expression (0.070s vs 0.066s) — not worth a second scan and a self-vs-copy return asymmetry. Now recorded as a deviation in the PR description, since the plan specifies it.

Flagged, not fixed

PR 3 (#266) will not compile against this head. Its branch is stacked on the pre-0736def tip and still uses templateList in 4 files. It needs a rebase plus the rename before it can build; the plan text carries the old name in PR 3's chunks too. Left as separate work rather than churning #266 while this PR is still moving.

Verification on c2bd777

swift build and swift build -c release clean (0 warnings) · swift test 488/488 · xcodebuild test -project iosMath.xcodeproj -scheme iosMath -sdk iphonesimulator -destination 'platform=iOS Simulator,name=iPhone 16' 447/447 · xcodebuild build -project MacOSMath.xcodeproj -scheme MacOSMath -sdk macosx succeeds.