From 31d247c19d6f6a9957453428814671610d288953 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 12 Aug 2026 07:02:25 -0400 Subject: [PATCH 1/4] fix(hermitcrab): make metathesis switch-name order not matter 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 --- .../AnalysisMetathesisRuleSpec.cs | 14 +++++++++-- .../SynthesisMetathesisRuleSpec.cs | 12 +++++++++ .../PhonologicalRules/MetathesisRuleTests.cs | 25 +++++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/AnalysisMetathesisRuleSpec.cs b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/AnalysisMetathesisRuleSpec.cs index b98495a65..f0b7f2795 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/AnalysisMetathesisRuleSpec.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/AnalysisMetathesisRuleSpec.cs @@ -31,8 +31,18 @@ PatternNode node in pattern.Children.TakeWhile(n => !(n is Grou _pattern.Children.Add(node.Clone()); } - AddGroup(groups, leftGroupName); - AddGroup(groups, rightGroupName); + // The analysis pattern has to match the SURFACE, where the two switch groups appear in the + // opposite order from the pattern that describes the underlying form. So the group that is + // later in the pattern is emitted first. Rules that name the later group as the left switch + // already satisfy that; ordering by pattern position instead of by name makes a rule behave + // the same whichever way round its switch names are written, and keeps this spec consistent + // with the synthesis spec's own normalization. + int leftIndex = Array.FindIndex(groupOrder, g => g.Name == leftGroupName); + int rightIndex = Array.FindIndex(groupOrder, g => g.Name == rightGroupName); + string firstName = leftIndex < rightIndex ? rightGroupName : leftGroupName; + string secondName = leftIndex < rightIndex ? leftGroupName : rightGroupName; + AddGroup(groups, firstName); + AddGroup(groups, secondName); foreach ( PatternNode node in pattern diff --git a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs index e2031f64e..6cd31bc90 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs @@ -100,6 +100,18 @@ public void ApplyRhs(Match targetMatch, Range range, GroupCapture leftGroup = targetMatch.GroupCaptures[_leftGroupName]; GroupCapture rightGroup = targetMatch.GroupCaptures[_rightGroupName]; + // The splice below only works when the first group it is handed is the LATER of the two in + // shape order: it moves that group's nodes out to the right, then moves the other group + // into the gap. Handed them the other way round, the second move re-anchors a group after + // its own end, which detaches nodes and leaves annotation ranges spanning two lists. + // Nothing in the switch names implies an order, so normalize instead of assuming one. + if (leftGroup.Range.Start.CompareTo(rightGroup.Range.Start) < 0) + { + GroupCapture earlier = leftGroup; + leftGroup = rightGroup; + rightGroup = earlier; + } + ShapeNode beforeRightGroup = rightGroup.Range.Start.Prev; MoveNodesAfter(targetMatch.Input.Shape, leftGroup.Range.End, rightGroup.Range); MoveNodesAfter(targetMatch.Input.Shape, beforeRightGroup, leftGroup.Range); diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/MetathesisRuleTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/MetathesisRuleTests.cs index 54d0e7451..def0995a4 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/MetathesisRuleTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/MetathesisRuleTests.cs @@ -28,6 +28,31 @@ public void SimpleRule() AssertMorphsEqual(morpher.ParseWord("mui"), "51"); } + // Regression: naming the EARLIER group as the left switch threw InvalidOperationException "Failed + // to compare two elements in the array", wrapping "Only nodes from the same list can be compared", + // instead of producing an analysis. Every pre-existing test here names the later group first, which + // is why the intuitive order was never exercised. Direction is irrelevant; this reproduces under + // both. + [Test] + public void SimpleRule_LeftSwitchNamesEarlierGroup() + { + var rule1 = new MetathesisRule + { + Name = "rule1", + Pattern = Pattern + .New() + .Group("1", group => group.Annotation(Character(Table3, "i"))) + .Group("2", group => group.Annotation(Character(Table3, "u"))) + .Value, + LeftSwitchName = "1", + RightSwitchName = "2", + }; + Morphophonemic.PhonologicalRules.Add(rule1); + + var morpher = new Morpher(TraceManager, Language); + AssertMorphsEqual(morpher.ParseWord("mui"), "51"); + } + [Test] public void ComplexRule() { From b8936b60559e025e8088cf780a644ea75b5d85bb Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 12 Aug 2026 07:08:48 -0400 Subject: [PATCH 2/4] style(hermitcrab): shorten the metathesis fix comments Rationale, history and the reproduction belong in the PR body, not in the source. Co-Authored-By: Claude Opus 5 --- .../PhonologicalRules/AnalysisMetathesisRuleSpec.cs | 7 +------ .../PhonologicalRules/SynthesisMetathesisRuleSpec.cs | 7 ++----- .../PhonologicalRules/MetathesisRuleTests.cs | 6 +----- 3 files changed, 4 insertions(+), 16 deletions(-) diff --git a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/AnalysisMetathesisRuleSpec.cs b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/AnalysisMetathesisRuleSpec.cs index f0b7f2795..352162f81 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/AnalysisMetathesisRuleSpec.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/AnalysisMetathesisRuleSpec.cs @@ -31,12 +31,7 @@ PatternNode node in pattern.Children.TakeWhile(n => !(n is Grou _pattern.Children.Add(node.Clone()); } - // The analysis pattern has to match the SURFACE, where the two switch groups appear in the - // opposite order from the pattern that describes the underlying form. So the group that is - // later in the pattern is emitted first. Rules that name the later group as the left switch - // already satisfy that; ordering by pattern position instead of by name makes a rule behave - // the same whichever way round its switch names are written, and keeps this spec consistent - // with the synthesis spec's own normalization. + // This pattern matches the surface, where the switch groups appear in reverse pattern order. int leftIndex = Array.FindIndex(groupOrder, g => g.Name == leftGroupName); int rightIndex = Array.FindIndex(groupOrder, g => g.Name == rightGroupName); string firstName = leftIndex < rightIndex ? rightGroupName : leftGroupName; diff --git a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs index 6cd31bc90..2a2093384 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs @@ -100,11 +100,8 @@ public void ApplyRhs(Match targetMatch, Range range, GroupCapture leftGroup = targetMatch.GroupCaptures[_leftGroupName]; GroupCapture rightGroup = targetMatch.GroupCaptures[_rightGroupName]; - // The splice below only works when the first group it is handed is the LATER of the two in - // shape order: it moves that group's nodes out to the right, then moves the other group - // into the gap. Handed them the other way round, the second move re-anchors a group after - // its own end, which detaches nodes and leaves annotation ranges spanning two lists. - // Nothing in the switch names implies an order, so normalize instead of assuming one. + // The splice below needs the later group in shape order first; reversed, its second move + // re-anchors a group after its own end. if (leftGroup.Range.Start.CompareTo(rightGroup.Range.Start) < 0) { GroupCapture earlier = leftGroup; diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/MetathesisRuleTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/MetathesisRuleTests.cs index def0995a4..79827e15c 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/MetathesisRuleTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/MetathesisRuleTests.cs @@ -28,11 +28,7 @@ public void SimpleRule() AssertMorphsEqual(morpher.ParseWord("mui"), "51"); } - // Regression: naming the EARLIER group as the left switch threw InvalidOperationException "Failed - // to compare two elements in the array", wrapping "Only nodes from the same list can be compared", - // instead of producing an analysis. Every pre-existing test here names the later group first, which - // is why the intuitive order was never exercised. Direction is irrelevant; this reproduces under - // both. + // Differs from SimpleRule only in the switch-name order, and must give the same result. [Test] public void SimpleRule_LeftSwitchNamesEarlierGroup() { From 76b0c83b47a079711bb8fcd8504e9206458acf69 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 12 Aug 2026 07:22:53 -0400 Subject: [PATCH 3/4] test(hermitcrab): add the metathesis coverage an adversarial review asked 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 --- .../SynthesisMetathesisRuleSpec.cs | 4 +- .../PhonologicalRules/MetathesisRuleTests.cs | 58 +++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs index 2a2093384..de927c41f 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs @@ -101,8 +101,8 @@ public void ApplyRhs(Match targetMatch, Range range, GroupCapture rightGroup = targetMatch.GroupCaptures[_rightGroupName]; // The splice below needs the later group in shape order first; reversed, its second move - // re-anchors a group after its own end. - if (leftGroup.Range.Start.CompareTo(rightGroup.Range.Start) < 0) + // re-anchors a group after its own end. An unmatched capture has no start to compare. + if (leftGroup.Success && rightGroup.Success && leftGroup.Range.Start.CompareTo(rightGroup.Range.Start) < 0) { GroupCapture earlier = leftGroup; leftGroup = rightGroup; diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/MetathesisRuleTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/MetathesisRuleTests.cs index 79827e15c..85b6faf2f 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/MetathesisRuleTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/MetathesisRuleTests.cs @@ -1,5 +1,6 @@ using NUnit.Framework; using SIL.Machine.Annotations; +using SIL.Machine.DataStructures; using SIL.Machine.FeatureModel; using SIL.Machine.Matching; using SIL.Machine.Morphology.HermitCrab.MorphologicalRules; @@ -49,6 +50,28 @@ public void SimpleRule_LeftSwitchNamesEarlierGroup() AssertMorphsEqual(morpher.ParseWord("mui"), "51"); } + // Differs from SimpleRule only in the switch-name order, plus a right-to-left direction. + [Test] + public void SimpleRule_LeftSwitchNamesEarlierGroup_RightToLeft() + { + var rule1 = new MetathesisRule + { + Name = "rule1", + Direction = Direction.RightToLeft, + Pattern = Pattern + .New() + .Group("1", group => group.Annotation(Character(Table3, "i"))) + .Group("2", group => group.Annotation(Character(Table3, "u"))) + .Value, + LeftSwitchName = "1", + RightSwitchName = "2", + }; + Morphophonemic.PhonologicalRules.Add(rule1); + + var morpher = new Morpher(TraceManager, Language); + AssertMorphsEqual(morpher.ParseWord("mui"), "51"); + } + [Test] public void ComplexRule() { @@ -83,6 +106,41 @@ public void ComplexRule() AssertMorphsEqual(morpher.ParseWord("mui"), "53 3SG"); } + // ComplexRule with the switch names reversed: a group sits between the two switches. + [Test] + public void ComplexRule_LeftSwitchNamesEarlierGroup() + { + var any = FeatureStruct.New().Symbol(HCFeatureSystem.Segment).Value; + + var rule1 = new MetathesisRule + { + Name = "rule1", + Pattern = Pattern + .New() + .Group("1", group => group.Annotation(Character(Table3, "i"))) + .Group("middle", group => group.Annotation(Character(Table3, "+"))) + .Group("2", group => group.Annotation(Character(Table3, "u"))) + .Group("rightEnv", group => group.Annotation(HCFeatureSystem.RightSideAnchor)) + .Value, + LeftSwitchName = "1", + RightSwitchName = "2", + }; + Morphophonemic.PhonologicalRules.Add(rule1); + + var uSuffix = new AffixProcessRule { Name = "u_suffix", Gloss = "3SG" }; + Morphophonemic.MorphologicalRules.Add(uSuffix); + uSuffix.Allomorphs.Add( + new AffixProcessAllomorph + { + Lhs = { Pattern.New("1").Annotation(any).OneOrMore.Value }, + Rhs = { new CopyFromInput("1"), new InsertSegments(Table3, "+u") }, + } + ); + + var morpher = new Morpher(TraceManager, Language); + AssertMorphsEqual(morpher.ParseWord("mui"), "53 3SG"); + } + [Test] public void SimpleRuleNotUnapplied() { From 01add43ede97470defc8f99c4d194e0ce63c61b8 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 12 Aug 2026 07:46:19 -0400 Subject: [PATCH 4/4] style(hermitcrab): collapse the synthesis metathesis swap comment to 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 --- .../PhonologicalRules/SynthesisMetathesisRuleSpec.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs index de927c41f..a58f3de66 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs @@ -100,8 +100,7 @@ public void ApplyRhs(Match targetMatch, Range range, GroupCapture leftGroup = targetMatch.GroupCaptures[_leftGroupName]; GroupCapture rightGroup = targetMatch.GroupCaptures[_rightGroupName]; - // The splice below needs the later group in shape order first; reversed, its second move - // re-anchors a group after its own end. An unmatched capture has no start to compare. + // The splice below needs the later shape-order group first, or its second move re-anchors past its own end. if (leftGroup.Success && rightGroup.Success && leftGroup.Range.Start.CompareTo(rightGroup.Range.Start) < 0) { GroupCapture earlier = leftGroup;