From 521334bd5d9be636626b51d542814f3f81379515 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Sat, 25 Jul 2026 10:51:31 -0400 Subject: [PATCH] Fix InvalidCastException when a rewrite/metathesis rule has a top-level Quantifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Constraint and Quantifier are both direct subclasses of PatternNode (src/SIL.Machine/Matching/Constraint.cs:12-13, src/SIL.Machine/Matching/Quantifier.cs:13-14) -- siblings, not related by inheritance. The DTD's PhoneticSequence element allows an OptionalSegmentSequence (loaded by XmlLanguageLoader as a Quantifier) in exactly the same positions as a plain Segment/SimpleContext (HermitCrabInput.dtd:515, 560), including as the target (Lhs) or replacement (Rhs) of a rewrite rule, or as a metathesis switch group -- not just in a rule's environment. The rule specs assumed every top-level Lhs/Rhs child was a Constraint and cast accordingly with `Children.Cast>()` or a direct cast, so a DTD-legal Quantifier there crashed with an unhandled InvalidCastException: - PhonologicalRules/SynthesisRewriteRuleSpec.cs:33 (and :93, lazily) - PhonologicalRules/FeatureAnalysisRewriteRuleSpec.cs:42-43 - PhonologicalRules/NarrowAnalysisRewriteRuleSpec.cs:45 (lazily, in Unapply) - PhonologicalRules/EpenthesisAnalysisRewriteRuleSpec.cs:18 - PhonologicalRules/SynthesisMetathesisRuleSpec.cs:31 - PhonologicalRules/AnalysisMetathesisRuleSpec.cs:53 Depending on which of these ran first, the raw InvalidCastException surfaced either unhandled directly out of `new Morpher(...)` (the synthesis side has no try/catch around rule compilation), or already wrapped in a CompileException by the pre-existing catch-all in AnalysisStratumRule.CompilePhonologicalRule -- but even then the message was uninformative (just named the rule; the actual cause was buried in an opaque InnerException). Fix: add PatternNodeCastExtensions (CastToConstraints/AsConstraint) and use it at every one of the sites above instead of the unchecked cast. Behavior for any grammar built entirely from Constraints (i.e. every currently-working grammar) is unchanged -- the helper returns the same Constraint it would have cast to. When a Quantifier (or any other non-Constraint PatternNode) shows up, it now raises a clear, typed CompileException naming the unsupported construct and where it appeared (e.g. "The target (left-hand side) of a rewrite rule contains a 'Quantifier`2', which is not supported there...") instead of an opaque InvalidCastException. Genuinely supporting a Quantifier in this position was considered but rejected as too large a semantic change (matching/deletion of a variable-width sequence during rewrite application) for this fix; a clear diagnostic is the conservative, reviewable option. Adds two regression tests to RewriteRuleTests.cs (QuantifierInTargetIsRejectedWithClearError, QuantifierInReplacementIsRejectedWithClearError) that build such a rule directly (bypassing the XML loader, matching the rest of the file) and confirm constructing a Morpher over it now fails fast with the new CompileException. Verified both fail (one with a raw InvalidCastException, one via the pre-existing CompileException wrapper around it) against the unmodified code, and pass after the fix. Found while building an independent FST-based morphological analyzer/proposer (PanGloss) against this library -- a real grammar under construction hit this, not a hypothetical. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- .../AnalysisMetathesisRuleSpec.cs | 3 +- .../EpenthesisAnalysisRewriteRuleSpec.cs | 7 +- .../FeatureAnalysisRewriteRuleSpec.cs | 8 +- .../NarrowAnalysisRewriteRuleSpec.cs | 5 +- .../PatternNodeCastExtensions.cs | 42 +++++++++ .../SynthesisMetathesisRuleSpec.cs | 4 +- .../SynthesisRewriteRuleSpec.cs | 10 ++- .../PhonologicalRules/RewriteRuleTests.cs | 85 +++++++++++++++++++ 8 files changed, 154 insertions(+), 10 deletions(-) create mode 100644 src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/PatternNodeCastExtensions.cs diff --git a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/AnalysisMetathesisRuleSpec.cs b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/AnalysisMetathesisRuleSpec.cs index 6581ffff4..b98495a65 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/AnalysisMetathesisRuleSpec.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/AnalysisMetathesisRuleSpec.cs @@ -50,7 +50,8 @@ private void AddGroup(Dictionary> groups, string { var newGroup = new Group(name); foreach ( - Constraint constraint in groups[name].Children.Cast>() + Constraint constraint in groups[name] + .Children.CastToConstraints("A switch group of a metathesis rule") ) { Constraint newConstraint = constraint.Clone(); diff --git a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/EpenthesisAnalysisRewriteRuleSpec.cs b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/EpenthesisAnalysisRewriteRuleSpec.cs index 6c29369b9..2a643282a 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/EpenthesisAnalysisRewriteRuleSpec.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/EpenthesisAnalysisRewriteRuleSpec.cs @@ -1,4 +1,3 @@ -using System.Linq; using SIL.Machine.Annotations; using SIL.Machine.FeatureModel; using SIL.Machine.Matching; @@ -15,7 +14,11 @@ public EpenthesisAnalysisRewriteRuleSpec(MatcherSettings matcherSetti Pattern.Acceptable = IsUnapplicationNonvacuous; _targetCount = subrule.Rhs.Children.Count; - foreach (Constraint constraint in subrule.Rhs.Children.Cast>()) + foreach ( + Constraint constraint in subrule.Rhs.Children.CastToConstraints( + "The replacement (right-hand side) of an epenthesis rewrite subrule" + ) + ) { Constraint newConstraint = constraint.Clone(); newConstraint.FeatureStruct.AddValue(HCFeatureSystem.Modified, HCFeatureSystem.Clean); diff --git a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/FeatureAnalysisRewriteRuleSpec.cs b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/FeatureAnalysisRewriteRuleSpec.cs index e98d95ec6..588f12fdf 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/FeatureAnalysisRewriteRuleSpec.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/FeatureAnalysisRewriteRuleSpec.cs @@ -39,8 +39,12 @@ Tuple, PatternNode> tuple in lhs.C ) ) { - var lhsConstraint = (Constraint)tuple.Item1; - var rhsConstraint = (Constraint)tuple.Item2; + Constraint lhsConstraint = tuple.Item1.AsConstraint( + "The target (left-hand side) of a rewrite rule" + ); + Constraint rhsConstraint = tuple.Item2.AsConstraint( + "The replacement (right-hand side) of a rewrite subrule" + ); if (lhsConstraint.Type() == HCFeatureSystem.Segment && rhsConstraint.Type() == HCFeatureSystem.Segment) { diff --git a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/NarrowAnalysisRewriteRuleSpec.cs b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/NarrowAnalysisRewriteRuleSpec.cs index 469f3020a..9d913b06c 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/NarrowAnalysisRewriteRuleSpec.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/NarrowAnalysisRewriteRuleSpec.cs @@ -1,4 +1,3 @@ -using System.Linq; using SIL.Extensions; using SIL.Machine.Annotations; using SIL.Machine.FeatureModel; @@ -42,7 +41,9 @@ private void Unapply(Match targetMatch, Range range, { ShapeNode curNode = IsTargetEmpty ? range.Start : range.End; foreach ( - Constraint constraint in _analysisRhs.Children.Cast>() + Constraint constraint in _analysisRhs.Children.CastToConstraints( + "The target (left-hand side) of a rewrite rule" + ) ) { FeatureStruct fs = constraint.FeatureStruct.Clone(); diff --git a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/PatternNodeCastExtensions.cs b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/PatternNodeCastExtensions.cs new file mode 100644 index 000000000..19498c5ff --- /dev/null +++ b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/PatternNodeCastExtensions.cs @@ -0,0 +1,42 @@ +using System.Collections.Generic; +using SIL.Machine.Annotations; +using SIL.Machine.Matching; + +namespace SIL.Machine.Morphology.HermitCrab.PhonologicalRules +{ + /// + /// Rewrite and metathesis rule specs assume that every top-level child of a rule's target/replacement + /// pattern (or a metathesis switch group) is a simple (a + /// segment, natural class, or boundary marker). and + /// are both direct subclasses of + /// -- siblings, not related by inheritance -- but the DTD and + /// XmlLanguageLoader allow an OptionalSegmentSequence (loaded as a ) + /// in exactly the same positions as a plain segment. These helpers replace unchecked + /// Cast<Constraint<Word, ShapeNode>>() calls and direct casts with a checked + /// conversion that raises a clear, actionable naming the unsupported + /// construct and where it appeared, instead of an opaque . + /// + internal static class PatternNodeCastExtensions + { + public static IEnumerable> CastToConstraints( + this IEnumerable> nodes, + string context + ) + { + foreach (PatternNode node in nodes) + yield return node.AsConstraint(context); + } + + public static Constraint AsConstraint(this PatternNode node, string context) + { + if (node is Constraint constraint) + return constraint; + + throw new CompileException( + $"{context} contains a '{node.GetType().Name}', which is not supported there. Only simple " + + "segments, natural classes, and boundary markers are supported; optional or repeated " + + "segment sequences (quantifiers) are only supported within a rule's left/right environment." + ); + } + } +} diff --git a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs index 312e0a677..e2031f64e 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisMetathesisRuleSpec.cs @@ -28,7 +28,9 @@ string rightGroupName { var newGroup = new Group(group.Name); foreach ( - Constraint constraint in group.Children.Cast>() + Constraint constraint in group.Children.CastToConstraints( + "A switch group of a metathesis rule" + ) ) { Constraint newConstraint = constraint.Clone(); diff --git a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisRewriteRuleSpec.cs b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisRewriteRuleSpec.cs index 87fa10955..b201b92a1 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisRewriteRuleSpec.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/PhonologicalRules/SynthesisRewriteRuleSpec.cs @@ -30,7 +30,11 @@ IEnumerable subrules } else { - foreach (Constraint constraint in lhs.Children.Cast>()) + foreach ( + Constraint constraint in lhs.Children.CastToConstraints( + "The target (left-hand side) of a rewrite rule" + ) + ) { var newConstraint = constraint.Clone(); if (isIterative) @@ -90,7 +94,9 @@ Tuple> tuple in match .Zip(lhs.Children) ) { - var constraints = (Constraint)tuple.Item2; + Constraint constraints = tuple.Item2.AsConstraint( + "The target (left-hand side) of a rewrite rule" + ); if (tuple.Item1.Annotation.Type() != constraints.Type()) return false; } diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/RewriteRuleTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/RewriteRuleTests.cs index f7c56ecaa..5e0bf867b 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/RewriteRuleTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/PhonologicalRules/RewriteRuleTests.cs @@ -1923,4 +1923,89 @@ public void MultipleApplicationRules() morpher = new Morpher(TraceManager, Language); AssertMorphsEqual(morpher.ParseWord("gigugi"), "44"); } + + // Constraint and Quantifier are both direct subclasses of + // PatternNode (siblings, not related by inheritance). The DTD permits an + // OptionalSegmentSequence (loaded as a Quantifier) anywhere a plain Segment/SimpleContext is + // permitted in a PhoneticSequence, including as the target (Lhs) or replacement (Rhs) of a rewrite + // rule -- not just in its environments. Before the fix, the rule specs assumed every top-level + // Lhs/Rhs child was a Constraint and cast accordingly, so a DTD-legal Quantifier there crashed with + // an unhandled InvalidCastException deep inside Morpher construction. These tests build such rules + // directly (bypassing the XML loader, like the rest of this file) and confirm that constructing a + // Morpher over them now fails fast with a clear, typed CompileException instead. + [Test] + public void QuantifierInTargetIsRejectedWithClearError() + { + var highVowel = FeatureStruct + .New(Language.PhonologicalFeatureSystem) + .Symbol(HCFeatureSystem.Segment) + .Symbol("cons-") + .Symbol("voc+") + .Symbol("high+") + .Value; + + var rule = new RewriteRule + { + Name = "quantifierInLhs", + // The Lhs is a single top-level Quantifier (an optional segment). The subrule has no Rhs + // (a deletion, 0 children), so the Lhs/Rhs child counts intentionally differ: this steers + // the analysis side to NarrowAnalysisRewriteRuleSpec, which does not cast its Lhs/Rhs + // children eagerly, isolating the crash to SynthesisRewriteRuleSpec, which always validates + // every Lhs child up front regardless of subrule shape. + Lhs = Pattern.New().Annotation(highVowel).Optional.Value, + }; + Allophonic.PhonologicalRules.Add(rule); + rule.Subrules.Add(new RewriteSubrule()); + + CompileException ex = Assert.Throws(() => new Morpher(TraceManager, Language)); + Exception innermost = Innermost(ex); + Assert.That(innermost, Is.TypeOf()); + Assert.That(innermost.Message, Does.Contain("Quantifier")); + } + + [Test] + public void QuantifierInReplacementIsRejectedWithClearError() + { + var highVowel = FeatureStruct + .New(Language.PhonologicalFeatureSystem) + .Symbol(HCFeatureSystem.Segment) + .Symbol("cons-") + .Symbol("voc+") + .Symbol("high+") + .Value; + var backRnd = FeatureStruct + .New(Language.PhonologicalFeatureSystem) + .Symbol(HCFeatureSystem.Segment) + .Symbol("back+") + .Symbol("round+") + .Value; + + var rule = new RewriteRule + { + Name = "quantifierInRhs", + Lhs = Pattern.New().Annotation(highVowel).Value, + }; + Allophonic.PhonologicalRules.Add(rule); + rule.Subrules.Add( + // Lhs and Rhs child counts match (1 == 1), so this exercises FeatureAnalysisRewriteRuleSpec, + // which casts both the Lhs and Rhs children eagerly. + new RewriteSubrule { Rhs = Pattern.New().Annotation(backRnd).Optional.Value } + ); + + // Before the fix, this already surfaced as a CompileException at the top level (an unrelated, + // pre-existing catch-all in AnalysisStratumRule wraps *any* exception from compiling a + // phonological rule), but its InnerException was a raw, uninformative InvalidCastException. The + // fix replaces that with a clear, typed CompileException naming the unsupported construct. + CompileException ex = Assert.Throws(() => new Morpher(TraceManager, Language)); + Exception innermost = Innermost(ex); + Assert.That(innermost, Is.TypeOf()); + Assert.That(innermost.Message, Does.Contain("Quantifier")); + } + + private static Exception Innermost(Exception ex) + { + while (ex.InnerException != null) + ex = ex.InnerException; + return ex; + } }