fix(FUN-8): copy style/spacer atom per cell in tableWithEnvironment:rows:error:#240
fix(FUN-8): copy style/spacer atom per cell in tableWithEnvironment:rows:error:#240kostub wants to merge 6 commits into
Conversation
…ows:error: MTMathAtomFactory was inserting the same MTMathStyle/spacer object reference into every cell of matrix, cases, and aligned environments. Any mutation of one cell's leading atom would silently alias all other cells. Fix by calling [atom copy] at each insert site (3 one-line edits) so each cell owns an independent, deeply-copied atom. Adds a regression test asserting object non-identity across cells for all three affected environments. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an object aliasing issue in MTMathAtomFactory.m where the same style or spacer atom was inserted into multiple table cells. The code has been updated to insert a copy of the atom instead, ensuring each cell has an independent instance. Additionally, a new test suite testTableCellsHaveIndependentLeadingAtoms has been added to verify this fix across matrix, cases, and aligned environments. No review comments were provided, so there is no feedback to address.
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.
|
EM-REVIEW v1 Verdict: No blocking issues. Ready to merge. Reviewed the full diff and surrounding source in MTMathAtomFactory.m and MTMathList.m, and ran the regression test under SPM (including a revert-the-fix mutation check). (a) Correctness - PASS
(b) Regression test - PASS (genuinely catches the bug)
(c) Wiring - PASS
(d) Leaks / over-copy - PASS
Minor (non-blocking)
|
Per-cell atom independence is an object-identity property; XCTAssertNotIdentical expresses that intent more precisely than XCTAssertNotEqual. Available since Xcode 12.5; the project builds exclusively on Xcode 16.2 / Swift 6.0, so there is no availability risk. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Mutate one cell's leading atom and assert the other cells are unaffected, reproducing the actual aliasing failure mode rather than just checking that the objects are distinct instances. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…al semantics A style atom is a non-rendering control marker (style is readonly, scripts already throw), so storing a nucleus on it is meaningless. Override -setNucleus: to throw on a non-empty value (empty stays allowed so -copyWithZone: keeps working), completing its immutability. Reflect the asymmetry in the regression test: - matrix/cases insert the now-immutable MTMathStyle, so only object sharing can go wrong -> assert distinct instances. - eqalign/aligned insert a plain ordinary spacer whose nucleus renders, so exercise the real leak: mutate one cell's spacer, assert others unaffected. Add testStyleAtomRejectsNucleus covering the new enforcement. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…cases cells A style atom is a non-rendering control marker — the typesetter reads only its `style`, never nucleus/type/fontStyle, and scripts are already forbidden. Lock down -setType:/-setFontStyle: (in addition to -setNucleus:), each permitting only the value -copyWithZone: assigns. With the atom now immutable, sharing one instance across all matrix/cases cells is safe, so drop the per-cell [style copy] there. The eqalign/aligned spacer is a plain mutable ordinary atom, so it still needs a per-cell copy. Tests: - testTableSpacerCellsAreIndependent: mutate one cell's spacer, assert the others are unaffected (the real leak; only the mutable spacer needs copying). - testStyleAtomIsImmutable: assert nucleus/type/fontStyle/subscript are rejected while the copy-compatible values stay allowed. Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to fix an aliasing bug in table cell creation by copying the mutable spacer atom and making MTMathStyle immutable by overriding its setters to throw exceptions. However, the review feedback correctly notes that throwing exceptions in these setters violates the Liskov Substitution Principle (LSP) and introduces runtime crash risks. It is recommended to keep MTMathStyle mutable and copy it upon insertion instead, and to remove the corresponding unit tests that assert on these exceptions.
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.
| // A style atom is a non-rendering control marker: the typesetter reads only its `style`, never | ||
| // its nucleus/type/fontStyle, and scripts are already forbidden via -scriptsAllowed. Make it | ||
| // fully immutable so the same instance can be shared across table cells without aliasing bugs. | ||
| // Each setter permits only the value that the inherited -copyWithZone: assigns (so copying keeps | ||
| // working) and rejects anything else. | ||
| - (void)setNucleus:(NSString *)nucleus | ||
| { | ||
| if (nucleus.length > 0) { | ||
| @throw [[NSException alloc] initWithName:@"Error" | ||
| reason:@"Nucleus cannot be set on a style atom; it is a non-rendering control marker." | ||
| userInfo:nil]; | ||
| } | ||
| [super setNucleus:nucleus]; | ||
| } | ||
|
|
||
| - (void)setType:(MTMathAtomType)type | ||
| { | ||
| if (type != kMTMathAtomStyle) { | ||
| @throw [[NSException alloc] initWithName:@"Error" | ||
| reason:@"The type of a style atom cannot be changed." | ||
| userInfo:nil]; | ||
| } | ||
| [super setType:type]; | ||
| } | ||
|
|
||
| - (void)setFontStyle:(MTFontStyle)fontStyle | ||
| { | ||
| if (fontStyle != kMTFontStyleDefault) { | ||
| @throw [[NSException alloc] initWithName:@"Error" | ||
| reason:@"Font style cannot be set on a style atom; it is a non-rendering control marker." | ||
| userInfo:nil]; | ||
| } | ||
| [super setFontStyle:fontStyle]; | ||
| } |
There was a problem hiding this comment.
Overriding setters to throw exceptions in a subclass of a mutable class (MTMathAtom) violates the Liskov Substitution Principle (LSP) and introduces a significant risk of runtime crashes.
For example, any generic list traversal or manipulation code that dynamically updates properties (such as fontStyle or nucleus) on all atoms in a list will crash with an unhandled exception if it encounters an MTMathStyle atom:
for (MTMathAtom *atom in list.atoms) {
atom.fontStyle = kMTFontStyleBold; // Will crash here!
}Instead of enforcing runtime immutability via exceptions, MTMathStyle should remain a standard mutable atom. To prevent the cell-sharing aliasing bug, simply copy the MTMathStyle instance when inserting it into the table cells in MTMathAtomFactory.m, just like you did for the spacer atom:
[row[j] insertAtom:[style copy] atIndex:0];| // FUN-8: a style atom is a non-rendering control marker (the typesetter reads only its `style`). | ||
| // It must be fully immutable so matrix/cases can share one instance across all cells safely. | ||
| - (void) testStyleAtomIsImmutable | ||
| { | ||
| MTMathStyle *style = [[MTMathStyle alloc] initWithStyle:kMTLineStyleText]; | ||
| // nucleus, type and fontStyle are all meaningless for a style atom and must be rejected. | ||
| XCTAssertThrows(style.nucleus = @"x", @"style atom must reject a non-empty nucleus"); | ||
| XCTAssertThrows(style.type = kMTMathAtomOrdinary, @"style atom must reject a type change"); | ||
| XCTAssertThrows(style.fontStyle = kMTFontStyleBold, @"style atom must reject a font style"); | ||
| // Scripts are already forbidden for style atoms via -scriptsAllowed. | ||
| XCTAssertThrows(style.subScript = [MTMathListBuilder buildFromString:@"x"], @"style atom must reject a subscript"); | ||
| // The values copyWithZone: assigns must remain allowed so deep copying still works. | ||
| XCTAssertNoThrow(style.nucleus = @""); | ||
| XCTAssertNoThrow(style.type = kMTMathAtomStyle); | ||
| XCTAssertNoThrow(style.fontStyle = kMTFontStyleDefault); | ||
| XCTAssertNoThrow([style copy]); | ||
| } |
There was a problem hiding this comment.
Since we should remove the setter overrides that throw exceptions on MTMathStyle to avoid violating the Liskov Substitution Principle, this test asserting those exceptions are thrown is no longer needed and should be removed.
Code-review follow-up: superScript is gated by the same -scriptsAllowed predicate as subScript, but assert it explicitly so the immutability contract is fully covered. Co-Authored-By: Claude Opus 4.8 <[email protected]>
1 participant
Summary
Fixes FUN-8:
+[MTMathAtomFactory tableWithEnvironment:rows:error:]allocated a single leading atom — anMTMathStyle(matrix/cases) or an ordinary spacerMTMathAtom(eqalign/split/aligned) — and inserted that same object reference into every table cell. Any client that mutated one cell's leading atom would silently mutate every other cell in the table.The two inserted atoms are not the same kind of thing, so they get different treatment:
MTMathStyle(matrix / pmatrix / … / cases) — made immutable, sharing is now safeAn
MTMathStyleis a non-rendering control marker: the typesetter reads only itsstyle, never itsnucleus,type, orfontStyle, and scripts are already forbidden via-scriptsAllowed. Storing any of that state on it is meaningless. Rather than defensively copying it, we make the class genuinely immutable:-setNucleus:,-setType:, and-setFontStyle:now throw on any value other than the one-copyWithZone:legitimately assigns (empty nucleus,kMTMathAtomStyle,kMTFontStyleDefault), so deep copying still works.stylewas alreadyreadonly; sub/superscripts already throw.With the atom immutable, the same instance can be shared across every matrix/cases cell with no aliasing risk, so the per-cell
[style copy]is removed — sharing is intentional and documented.Ordinary spacer (eqalign / split / aligned) — copied per cell
The relation spacer is a plain
kMTMathAtomOrdinaryatom (empty nucleus) whose only job is to give the following=/relation its left inter-element spacing. It is a fully mutable atom and its nucleus renders, so it genuinely must not be shared — each cell gets[spacer copy].No public LaTeX-level behaviour changes:
MTTypesetterre-copies viafinalized, so rendering is unaffected.Test plan
testTableSpacerCellsAreIndependent— mutates one aligned cell's spacer and asserts the other cells are unaffected. Exercises the real failure mode (object sharing → mutation leak); fails when[spacer copy]is reverted.testStyleAtomIsImmutable— assertsnucleus/type/fontStyle/subscript are all rejected on a style atom, while the copy-compatible values stay allowed and[style copy]still works.swift testsuite: 294 tests, 0 failures.