Skip to content

fix(hermitcrab): make metathesis switch-name order not matter - #471

Merged
johnml1135 merged 4 commits into
masterfrom
fix/metathesis-morph-annotation-sort
Aug 18, 2026
Merged

fix(hermitcrab): make metathesis switch-name order not matter#471
johnml1135 merged 4 commits into
masterfrom
fix/metathesis-morph-annotation-sort

Conversation

@johnml1135

@johnml1135johnml1135 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

A MetathesisRule whose leftSwitch names the earlier of its two pattern groups throws instead of producing an analysis:

System.InvalidOperationException: Failed to compare two elements in the array.
---> System.ArgumentException: Only nodes from the same list can be compared. (Parameter 'other')
at SIL.Machine.Annotations.ShapeNode.CompareTo(ShapeNode other)
at SIL.Machine.Annotations.ShapeRangeFactory.Compare(ShapeNode x, ShapeNode y)
at SIL.Machine.Annotations.Range`1.CompareTo(Range`1 other)
at System.Linq.Enumerable.EnumerableSorter`2...
at SIL.Machine.Morphology.HermitCrab.Morpher.Synthesize(...)

Both metathesis specs silently required the opposite orientation. Every pre-existing test in MetathesisRuleTests names the later group as the left switch, so the intuitive order was never exercised.

Direction is not involved — this reproduces under both LeftToRight and RightToLeft.

Cause

MoveNodesAfter advances its cur anchor after every iterated node, whether or not that node was physically moved. In the failing orientation beforeRightGroup ends up equal to leftGroup.Range.End, so for a single-node group that node is simultaneously the move's anchor and the node being removed in the same iteration. node.Remove() sets its List to null and the following cur.AddAfter(node) never restores it. The orphaned node then reaches morph.Children.OrderBy(ann => ann.Range), and ShapeNode.CompareTo throws because the two nodes belong to different lists.

Separately, AnalysisMetathesisRuleSpec builds its pattern in leftSwitch-then-rightSwitch order, but that pattern must match the surface, where the two groups appear in the opposite order from the underlying form. With the switch names reversed the analysis pattern matched nothing — so even with the crash fixed there was still no analysis.

Fix

Both specs order the two switch groups by pattern position rather than by name, so a rule behaves identically whichever way its switch names are written.

For rules that already name the later group first the normalization is a no-op: the swap condition is false, and AnalysisMetathesisRuleSpec emits the same order it always did.

Verification

Full SIL.Machine.Morphology.HermitCrab.Tests suite: 71/71 pass. dotnet csharpier check clean.

Each half of the fix was reverted independently to confirm both are load-bearing:

RevertedResult
synthesis normalizationSimpleRule_LeftSwitchNamesEarlierGroup and its right-to-left variant reproduce the reported crash
analysis normalizationall three new tests fail with an empty analysis instead
neither71/71 pass

Tests added:

  • SimpleRule_LeftSwitchNamesEarlierGroup — differs from SimpleRule only in the switch-name order, and must give the same result
  • SimpleRule_LeftSwitchNamesEarlierGroup_RightToLeft — same, with Direction.RightToLeft
  • ComplexRule_LeftSwitchNamesEarlierGroup — reversed naming with a group between the two switches. Note this one passes even without the synthesis change; it guards the analysis-side ordering and the middle-group case rather than reproducing the crash.

Scope, and what this does not close

This fixes the switch-name-order crash class. Two pre-existing hazards in the same code are left untouched and are not claimed to be fixed:

  • MoveNodesAfter still advances cur past a skipped non-Segment node, so a switch group whose own range spans a boundary or anchor node could still anchor subsequent moves off a stale position. No current rule shape hits this.
  • An unmatched switch capture has a null Range.Start. beforeRightGroup's .Prev already dereferenced it before this change; the new comparison is guarded with GroupCapture.Success so it does not add a second such site, but the underlying assumption that both switches always match is unchanged.

How it was found

While building generated-coverage fixtures for the HermitCrab XML surface on the conformance-framework branch: a fixture written to exercise MetathesisRule@multipleApplicationOrder wrote its switch names in the natural order and hit this. The ordering attribute turned out to be irrelevant.

🤖 Generated with Claude Code


This change is Reviewable

@codecov-commenter

codecov-commenter commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.32%. Comparing base (3093bd2) to head (03026ed).

Files with missing linesPatch %Lines
...b/PhonologicalRules/SynthesisMetathesisRuleSpec.cs83.33%0 Missing and 1 partial ⚠️
Additional details and impacted files
@@ Coverage Diff @@## master #471 +/- ##
=======================================
Coverage 73.31% 73.32% =======================================
Files 445 445 Lines 37300 37310 +10 Branches 5115 5118 +3 =======================================
+ Hits 27346 27356 +10 
Misses 8827 8827 Partials 1127 1127 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ddaspitddaspit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:lgtm:

@ddaspit reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on johnml1135).

johnml1135and others added 4 commits August 18, 2026 13:53
A MetathesisRule whose leftSwitch names the EARLIER of the two pattern groups threw
InvalidOperationException "Failed to compare two elements in the array", wrapping
ArgumentException "Only nodes from the same list can be compared", instead of producing
an analysis.
Both metathesis specs silently required the opposite orientation, and every existing
test names the later group as the left switch, so the intuitive order was never
exercised.
SynthesisMetathesisRuleSpec.ApplyRhs splices by moving one group's nodes out to the
right and then moving the other into the gap. That only works when the group it is
handed first is the LATER of the two in shape order. Handed them the other way round,
the second move re-anchors a group after its own end, which detaches nodes and leaves
child annotation ranges spanning two lists; the subsequent OrderBy over those ranges is
what throws.
AnalysisMetathesisRuleSpec builds its pattern in leftSwitch-then-rightSwitch order, but
that pattern has to match the SURFACE, where the two groups appear in the opposite order
from the underlying form. With the switch names the other way round the analysis pattern
matched nothing, so even without the crash there was no analysis.
Both now order by pattern position rather than by name, so a rule behaves identically
whichever way its switch names are written. Rules that already named the later group
first are unaffected: for them the normalization is a no-op.
Direction is NOT involved; this reproduces under both LeftToRight and RightToLeft.
The added test differs from SimpleRule only in swapping the two switch names, and must
produce the same result. Verified that reverting either half alone reintroduces a
distinct failure: without the synthesis change the crash returns, and without the
analysis change the word gets no analysis.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rationale, history and the reproduction belong in the PR body, not in the source.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sked for
A middle-group case with the switch names reversed, a right-to-left variant, and a
Success guard so the new comparison does not dereference an unmatched capture.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…one line
Keeps the load-bearing ordering rationale; drops the extra sentence about
unmatched captures, which the adjacent Success checks already make plain.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@johnml1135
johnml1135force-pushed the fix/metathesis-morph-annotation-sort branch from 01add43 to 03026edCompareAugust 18, 2026 10:53
@johnml1135
johnml1135 merged commit 60a0925 into masterAug 18, 2026
4 checks passed
@johnml1135
johnml1135 deleted the fix/metathesis-morph-annotation-sort branch August 18, 2026 11:07
johnml1135 added a commit that referenced this pull request Aug 19, 2026
dataflow-obligations.tsv keys MC/DC to (writer attribute -> reader attribute) pairs, and that artifact fails in both directions: it emits four arms per chain where only one can be certified -- the arm is read off the reader attribute's spelling -- and it cannot see a condition no attribute reaches.
gate-obligations.tsv is keyed to the 23 FailureReason gates, each carrying MC/DC's two arms: a Blocked arm where a word fails for that gate and severing its feeding construct flips it to a parse, and a Control arm where the same rule applies successfully so the block is attributable to the gate rather than to the rule never firing. The Blocked arm is stronger evidence than the old chain pairing because the trace NAMES the reason: idil is credited here to RequiredSyntacticFeatureStruct, the reason the engine actually gives, where the old ledger recorded a PartOfSpeech chain.
46 obligations, 42 worth covering, 9 arms evidenced -- but only ONE gate has both, so MC/DC is complete for 1 of 23. That number is what this ledger exists to make visible and the old one could not express. dataflow-obligations.tsv is kept as a cross-check on writer-reader chains, which the gate ledger does not measure, and the docs say which number is the claim.
The published figure is a funnel rather than a fraction: 346 cells enumerated, 28 certifiable by the generator, 18 also producible by FieldWorks, 4 satisfied. The outstanding list is 14, not 342.
Also retires the metathesis crash expectation. #471 fixed the comparator invariant violation and the fixture said its crash expectation should come out at that point; it did not, so rebasing onto master turned it red. A PanGloss adapter run had flagged it as an engine divergence and it was not one -- that engine produced exactly this result while our expectation was stale. A stale expectation does not read as stale; it reads as the other implementation being wrong.
Sign up for freeto 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.

3 participants

@johnml1135@codecov-commenter@ddaspit