Skip to content

Add LaTeX array environment (PR 2/3): parser + serialization#253

Merged
kostub merged 6 commits into
masterfrom
feature/array-environment-pr2
Jul 9, 2026
Merged

Add LaTeX array environment (PR 2/3): parser + serialization#253
kostub merged 6 commits into
masterfrom
feature/array-environment-pr2

Conversation

@kostub

@kostub kostub commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Goal

Parse \begin{array}{<spec>} … \end{array} end-to-end: register array in the merged #248 argument hook, interpret the raw column spec into structured alignments/verticalLines, count \hline per boundary (array-only, loud error otherwise), build the table via PR 1's constructor, and round-trip via appendLaTeXToString:. No renderer changes yet — the produced atom tree is validated structurally.

Commits

  • [item 4] Register array in argument hook + column-spec error codes
  • [item 5] Interpret array column spec into alignments + verticalLines
  • [item 6] Count \hline per boundary; reject outside array
  • [item 7] End-to-end array parse + error-table coverage
  • [item 8] Round-trip array spec + \hline in appendLaTeXToString

Notes

  • Per the plan's load-bearing control-flow correction, \hline is intercepted at the buildInternal \\-command dispatch site (via -recordHorizontalLine + continue), not inside -stopCommand:.
  • Item 6 added a small array-scoped trim in -buildTable: that drops a trailing all-empty row (so a bottom \hline with a trailing \\ yields the correct numRows). Scoped to array only; matrix/eqalign/split/aligned behavior is unchanged. Full suite green (397/397).

Stack

  1. Add LaTeX array environment (PR 1/3): model fields + factory constructor #251 — PR 1/3: model fields + array factory constructor (base: master)
  2. This PR — PR 2/3: parser + serialization (base: feature/array-environment-pr1)

References

  • Plan: docs/plans/2026-07-03-array-environment.md (PR 2, items 4-8)
  • LLD: docs/lld/2026-06-29-array-environment.md

🤖 Generated with Claude Code

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for the LaTeX array environment in iosMath, enabling the parsing and serialization of column alignments, vertical borders, and horizontal lines (\hline). It also adds comprehensive unit tests to verify the new parsing logic and error handling. The review feedback suggests two important improvements: adding a nil-guard for _currentEnv.envName to prevent potential crashes when checking environment capabilities, and ignoring whitespace characters within the array column specification to align with standard LaTeX parsing behavior.

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.

Comment thread iosMath/lib/MTMathListBuilder.m Outdated
Comment on lines +925 to +926
if (_currentEnv && [[MTMathListBuilder environmentsAllowingHorizontalLines]
containsObject:_currentEnv.envName]) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If _currentEnv.envName is nil (which occurs when parsing default tables), calling containsObject: with nil can lead to unexpected behavior or crashes depending on the Foundation version. It is safer to explicitly guard against nil before checking set membership.

    if (_currentEnv && _currentEnv.envName && [[MTMathListBuilder environmentsAllowingHorizontalLines]
                            containsObject:_currentEnv.envName]) {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the _currentEnv.envName guard in 741ee63 for consistency with the existing nil-envName handling (e.g. line 1473) and to avoid relying on containsObject:nil semantics. Note: on Apple Foundation [NSSet containsObject:nil] returns NO rather than crashing, so behavior was already correct here (falls through to the loud error) — this just makes the nil case explicit.

Comment on lines +990 to +1008
for (NSUInteger i = 0; i < raw.length; i++) {
unichar c = [raw characterAtIndex:i];
switch (c) {
case 'l': [alignments addObject:@(kMTColumnAlignmentLeft)]; [vLines addObject:@0]; break;
case 'c': [alignments addObject:@(kMTColumnAlignmentCenter)]; [vLines addObject:@0]; break;
case 'r': [alignments addObject:@(kMTColumnAlignmentRight)]; [vLines addObject:@0]; break;
case '|': {
NSUInteger idx = vLines.count - 1;
vLines[idx] = @(vLines[idx].integerValue + 1);
break;
}
default: {
NSString* message = [NSString stringWithFormat:
@"Unsupported array column specifier: %C", c];
[self setError:MTParseErrorInvalidColumnSpec message:message];
return NO;
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In LaTeX, whitespace characters inside the column specification (e.g., \begin{array}{c | c}) are ignored. Currently, any whitespace character in the column specifier will trigger the default case and raise an MTParseErrorInvalidColumnSpec error. We should explicitly ignore whitespace characters during parsing.

    for (NSUInteger i = 0; i < raw.length; i++) {
        unichar c = [raw characterAtIndex:i];
        switch (c) {
            case ' ':
            case '\t':
            case '\n':
            case '\r':
                break;
            case 'l': [alignments addObject:@(kMTColumnAlignmentLeft)];   [vLines addObject:@0]; break;
            case 'c': [alignments addObject:@(kMTColumnAlignmentCenter)]; [vLines addObject:@0]; break;
            case 'r': [alignments addObject:@(kMTColumnAlignmentRight)];  [vLines addObject:@0]; break;
            case '|': {
                NSUInteger idx = vLines.count - 1;
                vLines[idx] = @(vLines[idx].integerValue + 1);
                break;
            }
            default: {
                NSString* message = [NSString stringWithFormat:
                    @"Unsupported array column specifier: %C", c];
                [self setError:MTParseErrorInvalidColumnSpec message:message];
                return NO;
            }
        }
    }

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 741ee63. interpretArrayColumnSpec now skips space/tab/newline, so {c | c} parses as two columns. Added an assertion to testArrayColumnSpecInterpretation covering it.

@kostub

kostub commented Jul 3, 2026

Copy link
Copy Markdown
Owner Author

Code review — PR 2/3 (array parser + serialization)

Reviewed the delta on top of PR #251 (b9d87be..7570801), built the head in a scratch worktree, and ran swift testall 397 tests pass, clean rebase, no regressions to existing environments.

The core work is solid and matches the plan/LLD:

  • Column-spec interpreter is correct: l/c/r → alignments, | → per-boundary counts, repeated || accumulates (verified {||c||c|}[2,2,1]), zero-column and bad-specifier cases fail loud with the right codes.
  • \hline boundary counting at _currentEnv.numRows is correct for top/middle/bottom; array-only rejection works (loud error in matrix and at top level).
  • The deliberate array-scoped trailing-empty-row trim is correctly guarded by envName == "array"; matrix/eqalign/split/aligned/anonymous-table paths are untouched and their tests still pass.
  • Bottom-\hline round-trip (with the \\ terminator) works and re-parses: verified \begin{array}{c} a \\ \hline \end{array}\begin{array}{c}a\\ \hline \end{array}.

Four non-blocking fidelity/edge findings (all verified empirically against the head), none critical:

1. (Minor) appendLaTeXToString: drops trailing declared columns for an underfull array.
The spec is rebuilt from self.numColumns (derived from actual cells) rather than the authoritative alignments.count/verticalLines.count. The constructor permits rows with fewer cells than declared, so \begin{array}{ccc} a & b \end{array} round-trips as \begin{array}{cc}a&b\end{array} — the third column spec (and any vertical rules after the last used column) is lost. Since numColumns <= alignments.count always holds, reconstructing from alignments.count (or verticalLines.count - 1) would be lossless. MTMathList.m ~L1297.

2. (Minor) The array-scoped trim can drop an intentional all-empty final row.
The trim keys on emptiness, not on whether a phantom trailing \\ produced the row. Verified: \begin{array}{cc} a & b \\ & \end{array} yields numRows == 1, but LaTeX treats the explicit \\ & as a real (empty) second row → 2 rows. The primary target (trailing \\ before a bottom \hline) is handled correctly; this is just the ambiguous corner where an explicitly-declared empty last row is indistinguishable from a phantom one. Low severity (all-empty rows are near-invisible without rules), but it is a deviation from LaTeX row semantics. MTMathListBuilder.m ~L1476.

3. (Minor) Internal whitespace in the column spec is rejected.
readEnvironmentArgument: trims only surrounding whitespace; interpretArrayColumnSpec: sends any interior space to the default branch. Verified \begin{array}{c c}...MTParseErrorInvalidColumnSpec ("Unsupported array column specifier: "). Real LaTeX ignores spaces in the column spec ({c c} == {cc}). Consider skipping whitespace in the interpreter loop.

4. (Nit) Coverage gap. Tests cover top/middle/bottom \hline counting and a top-\hline round-trip, but there's no round-trip assertion for the bottom-\hline + trailing-\\ terminator path (the most intricate serialization branch). Worth a one-line test to lock it in — I confirmed by hand it currently works.

Not approving (comment-only per request). Nothing here blocks merge; items 1–3 are worth a brief look since the LLD frames round-trip fidelity as a goal.

kostub and others added 5 commits July 9, 2026 21:44
Adds MTParseErrorMissingColumnSpec/MTParseErrorInvalidColumnSpec, registers
"array" in +environmentsTakingArgument, and makes -readEnvironmentArgument:
env-aware so array gets its own loud, specific error messages instead of
alignedat's numeric-argument message.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5hd85BBs6ExZVyqevdpqu
Also trims a wholly-empty trailing array row: a trailing \\ is needed
so a bottom \hline is recognized before \end (matching real LaTeX
array semantics), but if nothing but \hline/whitespace followed it,
the row it opened has no content and must not count as an extra row.
@kostub kostub force-pushed the feature/array-environment-pr2 branch from 843a1c8 to 7972159 Compare July 9, 2026 16:14
@kostub kostub changed the base branch from feature/array-environment-pr1 to master July 9, 2026 16:15
…vName

- interpretArrayColumnSpec now skips space/tab/newline chars so specs like
  {c | c} parse as two columns (LaTeX ignores whitespace in column specs)
  instead of raising MTParseErrorInvalidColumnSpec.
- recordHorizontalLine explicitly guards _currentEnv.envName before the set
  membership check, matching the existing nil-envName handling elsewhere and
  removing reliance on containsObject:nil semantics.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kostub kostub merged commit e651850 into master Jul 9, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant