Fraction data model + parser#202
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…MTFraction Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…le wrappers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kostub
left a comment
There was a problem hiding this comment.
Code Review — PR #202: Fraction data model + parser
Overall this is clean, well-scoped work. The dispatch-table approach for fractionMacroCommands is a clear improvement over the previous cascade of if/else if strings. The new properties are correctly copied in copyWithZone: and the tests cover the happy path well. I have a handful of issues across three severity levels.
Issues
1. Lossy round-trip for \cfrac is silent and under-tested (medium)
appendLaTeXToString: serialises \cfrac{a}{b} as \frac{\displaystyle{a}}{\displaystyle{b}}. This is acknowledged in testCfrac, but isContinuedFraction and numeratorAlignment are silently dropped — they cannot be recovered by re-parsing the output. That means any caller who goes latex → MTMathList → latex → MTMathList will lose the cfrac semantics silently.
This is flagged as intentional in the PR description ("round-trip is lossy"), but there is no API-level indication of the contract, and the mathListToString: doc comment says nothing about lossiness. At minimum, add a comment in the header (or at the appendLaTeXToString: call site) that isContinuedFraction and numeratorAlignment are not persisted through mathListToString:. Better still, serialise cfrac properly:
// If isContinuedFraction, emit \cfrac[l|c|r]{...}{...}If that's deferred to PR 2, mark it explicitly with a // TODO(PR2): comment at the call site so it doesn't get forgotten.
2. readOptionalAlignment: does not handle multi-character bracket bodies (medium)
\cfrac[zzz]{a}{b} exercises the invalid-first-letter path, but once z is read and the error is set, the ] and the remaining zz are not consumed. The parser then returns nil (correct), but the error is set to MTParseErrorInvalidCommand with a message that refers to the first invalid character only. The remaining characters (zz]{a}{b}) are abandoned mid-stream.
This is benign today because atomForCommand: returns nil immediately on _error, aborting the whole parse. But it is a correctness landmine: if _error handling ever becomes recoverable, [zz] debris will parse as unintended tokens. Consuming through the closing ] before returning would make the method self-contained:
default: {
// consume through ']' so the stream is left in a clean state
unichar c;
while ([self hasCharacters] && (c = [self getNextCharacter]) != ']') { }
[self setError:MTParseErrorInvalidCommand message:...];
return YES;
}3. \cfrac round-trip test is missing the alignment cases (minor)
testCfracLeftAlign and testCfracRightAlign each verify that numeratorAlignment is set correctly, but neither checks the serialised output. Since the alignment is silently dropped in serialisation, it would be valuable to assert the exact output string in those tests, both to document the lossiness and to prevent a future "fix" from accidentally emitting alignment data in a format the parser can't re-read.
4. testTfrac / testDbinom / testTbinom missing XCTAssertNotNil guard (minor)
testDfrac and testCfrac both start with XCTAssertNotNil(list) before indexing list.atoms[0]. testTfrac, testDbinom, and testTbinom skip that guard and would crash with a nil-dereference rather than a test failure if parsing unexpectedly returns nil. Follow the pattern set by testDfrac.
5. appendLaTeXToString: wraps operands rather than emitting \dfrac (design note)
The current serialisation of \dfrac{a}{b} is \frac{\displaystyle{a}}{\displaystyle{b}}. This is semantically inequivalent: \displaystyle inside the operand of \frac controls the rendering of a and b independently, but it does not force the fraction itself into display style (the bar height, minimum clearance, and surrounding spacing all differ). A real typesetter honouring styleOverride == kMTFractionStyleDisplay would render the fraction at display size; wrapping operands only affects what the next PR's typesetter sees inside the operands.
This is described as intentional deferral, but the round-trip test XCTAssertEqualObjects(latex, @"\\frac{\\displaystyle{1}}{\\displaystyle{c}}") encodes this lossy form as a pass condition. Once PR 2 changes serialisation to emit \dfrac, that test will need to be updated. Consider adding // NOTE: this round-trip encoding will change in PR 2 when \dfrac serialisation is added so it's obvious why the test exists in this form.
Nits
- In
fractionMacroCommands, the@"cfrac"entry has inconsistent leading spaces before@"style"and@"continued"keys (extra space vs. the other entries). Minor alignment issue. readOptionalAlignment:is a great private helper, but it hardcodes the error type asMTParseErrorInvalidCommandfor what is arguably a character-not-found scenario (MTParseErrorCharacterNotFoundmight be a better fit for the unterminated-bracket cases).
There was a problem hiding this comment.
Code Review
This pull request introduces support for AMSMath fraction commands, including \dfrac, \tfrac, \dbinom, \tbinom, and \cfrac with optional alignment. It adds new properties to the MTFraction atom to handle style overrides, alignment, and continued fraction flags, and updates the parser to recognize these commands via a lookup table. Feedback identifies that the current serialization logic in appendLaTeXToString: is semantically incorrect according to TeX rules and results in metadata loss for alignment and continued fraction flags. A refactor is suggested to use specific LaTeX macros to ensure faithful round-tripping and correct typesetting.
| NSString* (^wrap)(NSString*) = ^NSString*(NSString* inner) { | ||
| switch (self.styleOverride) { | ||
| case kMTFractionStyleDisplay: | ||
| return [NSString stringWithFormat:@"\\displaystyle{%@}", inner]; | ||
| case kMTFractionStyleText: | ||
| return [NSString stringWithFormat:@"\\textstyle{%@}", inner]; | ||
| case kMTFractionStyleScript: | ||
| return [NSString stringWithFormat:@"\\scriptstyle{%@}", inner]; | ||
| case kMTFractionStyleScriptScript: | ||
| return [NSString stringWithFormat:@"\\scriptscriptstyle{%@}", inner]; | ||
| case kMTFractionStyleAuto: | ||
| default: | ||
| return inner; | ||
| } | ||
| }; | ||
| NSString* numWrapped = wrap(numLatex); | ||
| NSString* denWrapped = wrap(denLatex); | ||
| if (self.hasRule) { | ||
| [str appendFormat:@"\\frac{%@}{%@}", [MTMathListBuilder mathListToString:self.numerator], [MTMathListBuilder mathListToString:self.denominator]]; | ||
| [str appendFormat:@"\\frac{%@}{%@}", numWrapped, denWrapped]; | ||
| return; | ||
| } | ||
| NSString* command = fractionCommandForDelimiterPair(self.leftDelimiter, self.rightDelimiter); | ||
| [str appendFormat:@"{%@ \\%@ %@}", [MTMathListBuilder mathListToString:self.numerator], command, [MTMathListBuilder mathListToString:self.denominator]]; | ||
| [str appendFormat:@"{%@ \\%@ %@}", numWrapped, command, denWrapped]; | ||
| } |
There was a problem hiding this comment.
The current implementation of appendLaTeXToString: for MTFraction has several issues:
- Semantic Correctness (TeX Rule 15): Wrapping operands in
\displaystyle{...}is not equivalent to setting the fraction's style. In TeX, if a fraction is in Display style, its operands are typeset in Text style (Rule 15). Forcing operands to\displaystyleresults in incorrect sizing and placement of sub-elements (like limits on sums) within the fraction. - Loss of Metadata: The
isContinuedFractionflag andnumeratorAlignment(used by\cfrac) are completely lost during serialization. This breaks the "round-trip" goal mentioned in the PR description. - Non-standard LaTeX:
\displaystyle{...}is a non-standard grouping (it's a switch, not a command taking an argument), although some parsers accept it. Canonical LaTeX would be{\displaystyle ...}. - Macro Preservation: Since the parser now supports
\dfrac,\tfrac,\cfrac,\dbinom, and\tbinom, the serializer should use these macros when the properties match. This ensures a faithful round-trip of the data model.
I suggest refactoring this method to use the specific macros based on the atom's properties.
- (void)appendLaTeXToString:(NSMutableString *)str
{
if (self.isContinuedFraction) {
[str appendString:@"\\cfrac"];
if (self.numeratorAlignment == kMTFractionAlignmentLeft) {
[str appendString:@"[l]"];
} else if (self.numeratorAlignment == kMTFractionAlignmentRight) {
[str appendString:@"[r]"];
}
[str appendFormat:@"{%@}{%@}", [MTMathListBuilder mathListToString:self.numerator], [MTMathListBuilder mathListToString:self.denominator]];
return;
}
if (self.hasRule) {
if (self.styleOverride == kMTFractionStyleDisplay) {
[str appendString:@"\\dfrac"];
} else if (self.styleOverride == kMTFractionStyleText) {
[str appendString:@"\\tfrac"];
} else {
[str appendString:@"\\frac"];
}
[str appendFormat:@"{%@}{%@}", [MTMathListBuilder mathListToString:self.numerator], [MTMathListBuilder mathListToString:self.denominator]];
return;
}
if ([self.leftDelimiter isEqualToString:@"("] && [self.rightDelimiter isEqualToString:@")"]) {
if (self.styleOverride == kMTFractionStyleDisplay) {
[str appendFormat:@"\\dbinom{%@}{%@}", [MTMathListBuilder mathListToString:self.numerator], [MTMathListBuilder mathListToString:self.denominator]];
return;
} else if (self.styleOverride == kMTFractionStyleText) {
[str appendFormat:@"\\tbinom{%@}{%@}", [MTMathListBuilder mathListToString:self.numerator], [MTMathListBuilder mathListToString:self.denominator]];
return;
}
}
NSString* command = fractionCommandForDelimiterPair(self.leftDelimiter, self.rightDelimiter);
[str appendFormat:@"{%@ \\%@ %@}", [MTMathListBuilder mathListToString:self.numerator], command, [MTMathListBuilder mathListToString:self.denominator]];
}Per PR #202 review feedback: - MTMathList.h: note on isContinuedFraction and numeratorAlignment that appendLaTeXToString:/+mathListToString: do not persist these fields. - testDfrac: comment explaining why the round-trip emits operand-wrapped \displaystyle rather than \dfrac directly (partial-fidelity trade-off per LLD 3.3.5 / 5.1). - testCfracLeftAlign / testCfracRightAlign: assert the serialized output to pin the lossy contract. - testTfrac / testDbinom / testTbinom: XCTAssertNotNil(list) guards so a parse failure becomes a clean test failure instead of a nil-deref. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Adds AMSMath fraction-style macros to iosMath's parser and data model. This is PR 1 of 4 in the AMSMath fractions + multi-integral stack.
Plan:
docs/plans/2026-05-25-amsmath-fractions-and-multi-integrals.mdLLD:
docs/lld/2026-05-25-amsmath-fractions-and-multi-integrals.mdGoal
All five new fraction macros (
\dfrac,\tfrac,\cfrac[l|c|r],\dbinom,\tbinom) parse toMTFractionatoms with correctstyleOverride/isContinuedFraction/numeratorAlignmentfields and round-trip throughappendLaTeXToString:. No rendering changes — the typesetter does not yet honor the new fields (that's PR 2). Existing\frac/\binombehavior is preserved byte-for-byte.Commits
[item 1]AddMTFractionStyleandMTFractionAlignmentpublic enums[item 2]AddstyleOverride/isContinuedFraction/numeratorAlignmenttoMTFraction[item 3]SerializeMTFraction.styleOverrideas\displaystyle/\textstylewrappers[item 4]Add+fractionMacroCommandsdispatch table[item 5]Add-readOptionalAlignment:helper for\cfrac[item 6]Route\fracand\binomthroughfractionMacroCommandsdispatch[item 7]Parser tests for\dfrac,\tfrac,\dbinom,\tbinom[item 8]Parser tests for\cfracand optional alignment[item 9]Nested\dfracparsing regression testTest plan
testDfrac,testTfrac,testDbinom,testTbinom— new fraction macros parse correctlytestCfrac,testCfracLeftAlign,testCfracRightAlign,testCfracCenterAlignExplicit— cfrac with optional alignmenttestCfracInvalidAlign— invalid[zzz]bracket returns parse errortestDfracInDfrac— nested fractions both carrystyleOverride == DisplaytestFrac,testBinom— existing round-trips unchanged (\frac{1}{c},{n \choose k})