LT-22613: fix analysis-length bound over-pruning for count-mismatched rewrite subrules#453
Open
johnml1135 wants to merge 4 commits into
Open
LT-22613: fix analysis-length bound over-pruning for count-mismatched rewrite subrules#453johnml1135 wants to merge 4 commits into
johnml1135 wants to merge 4 commits into
Conversation
…thesis grammars Two failing-first tests pinning the bug where Morpher.ParseWord's default (non-tracing) path returns 0 parses while the traced path returns the correct 1 parse: RewriteRuleTests.EpenthesisRuleWithMinimalLexicon (end-to-end: root 19 "b+ubu" alone in the lexicon, Simultaneous and Iterative epenthesis, so surface "buibui"/"biubiu" outgrow the auto-derived MaxAnalysisLength bound) and MorpherTests.ComputeMaxAnalysisLength_ReturnsNull_ForInsertionRewriteRule (the bound must be inadmissible when an insertion-type rewrite subrule exists). Isolated mechanism (not the nogood/memo cache -- verified by re-parsing with the memo active and only MaxAnalysisLength nulled): epenthesis unapplication marks inserted segments Optional without removing them, so Shape.SegmentCount at AnalysisStratumRule's Phase 4 Gate B still counts them and the gate prunes a candidate whose obligatory segments fit the lexicon fine. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e subrules Fixes the non-tracing/tracing divergence isolated in the previous commit: the Phase 4 Gate B bound (GrammarAnalyzer.ComputeMaxAnalysisLength) treated insertion-type rewrite subrules (Rhs longer than Lhs; epenthesis when Lhs is empty) as contributing nothing, but their ANALYSIS-side unapplication marks the possibly-rule-inserted surface segments Optional instead of removing them (EpenthesisAnalysisRewriteRuleSpec / NarrowAnalysisRewriteRuleSpec), so Shape.SegmentCount at AnalysisStratumRule's gate keeps counting them. A surface form that legally outgrew the longest root via epenthesis (fixture: root "b+ubu", surface "buibui", auto-derived bound 4 < 6 segments) was pruned as unreachable on the default path while the traced path -- which bypasses the gate -- parsed it correctly. Mode-independent: Iterative epenthesis over-prunes identically (verified), so the fix keys on subrule shape, not Simultaneous mode. Per GrammarAnalyzer's own ground rule (never guess an admissible bound), an insertion subrule now makes the bound null (gate off), same as compounding or unmeasurable patterns. The nogood/memo cache (AnalysisScope) was ruled out: with the memo fully active and only MaxAnalysisLength nulled, the parse is correct. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Independent adversarial review of the insertion-only fix (9a43d25) found the same over-prune survives for deletion/coalescence subrules (Lhs longer than Rhs): NarrowAnalysisRewriteRuleSpec.Unapply always adds Lhs.Count new (Optional, not Deleted) nodes per match site regardless of Rhs.Count, while leaving the matched Rhs-shaped region in place (also not Deleted) -- so real per-site growth is always Lhs.Count, never the Lhs.Count - Rhs.Count this method budgeted. That undercounts by Rhs.Count whenever Rhs.Count > 0, independent of which direction the count difference runs. Broadens the bail-out from "Rhs longer than Lhs" to "counts differ at all", which makes the now-unreachable deletion-reapplication compounding loop (and the DeletionReapplications parameter it depended on) dead code; removed both, along with the doc-comment claim that insertion was uniquely unsound. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI's "Check formatting" step was failing because several files from the recent parse-optimization commits weren't run through `dotnet csharpier format`. No logic changes, just re-wrapping to match the enforced style. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## parse-optimization #453 +/- ##
=====================================================
Coverage ? 73.61%
=====================================================
Files ? 451
Lines ? 38540
Branches ? 5357
=====================================================
Hits ? 28371
Misses ? 8998
Partials ? 1171 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes LT-22613:
Morpher.ParseWord's default (non-tracing) path returned 0 parses for words a grammar should legitimately parse, while the traced path (which happens to skip this gate) correctly returned 1. The JIRA issue's original write-up hypothesized the cause wasAnalysisScope's nogood-memoization cache — that was investigated and disproven (toggling the memo cache independently of the real cause changes nothing); see the corrective comment added to LT-22613 and the corrected analysis doc onbug/csharp-nogood-cache-tracing-divergence(rust/docs/bugs/csharp-nogood-cache-tracing-divergence.md).Actual root cause:
GrammarAnalyzer.ComputeMaxAnalysisLength(Phase 4 "Gate B" ofparse-optimization.md) computes a grammar-wide upper bound on analysis-form length, used byAnalysisStratumRule.Applyto prune candidates early — but only on the default, non-tracing path. For any phonological rewrite subrule whereLhs.Count != Rhs.Count, the analysis-side unapplication (EpenthesisAnalysisRewriteRuleSpec/NarrowAnalysisRewriteRuleSpec) never shrinks the candidate shape — it adds newOptional(notDeleted) nodes, whichShape.SegmentCount()still counts. The bound assumed growth ofLhs.Count - Rhs.Countper match site; real growth is alwaysLhs.Count, regardless of direction. This under-counted the true bound and pruned valid candidates as "too long."9a43d251fixes the insertion/expansion case (Rhs.Count > Lhs.Count, epenthesis whenLhsis empty).93e52dc9broadens the fix to any count mismatch after an independent adversarial review found the identical bug shape in deletion/coalescence subrules (Lhs.Count > Rhs.Count), and removes the now-unreachable deletion-reapplication compounding logic (and theDeletionReapplicationsparameter it consumed) as dead code.Base is
parse-optimization(notmaster):GrammarAnalyzer/AnalysisScope/MaxAnalysisLengthonly exist on this branch (PR #451, based onhc-rustifyPR #446).Test plan
dotnet build— full solution, 0 warnings/errorsdotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests— 71/71 passedComputeMaxAnalysisLength_ReturnsNull_ForInsertionRewriteRule/ForDeletionRewriteRule(unit-level bound assertions)EpenthesisRuleWithMinimalLexicon(end-to-end minimal-lexicon parse, default vs. traced path)🤖 Generated with Claude Code
This change is