Repository navigation
Fix narrowing-rule unapplication across an interposed optional node - #540
johnml1135 wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #540 +/- ##
==========================================
- Coverage 74.36% 74.31% -0.05%
==========================================
Files 456 456
Lines 38325 38332 +7
Branches 5263 5266 +3
==========================================
- Hits 28499 28486 -13
- Misses 8669 8690 +21
+ Partials 1157 1156 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| GroupCapture<ShapeNode> capture = targetMatch.GroupCaptures["target" + i]; | ||
| if (!capture.Success) | ||
| continue; | ||
| foreach (ShapeNode node in targetMatch.Input.Shape.GetNodes(capture.Range)) |
There was a problem hiding this comment.
FYI: F1. A capture range can cover nodes that are not the replacement: when the matcher skips an optional node, the closing register takes the skipped annotation's end (TraversalMethodBase.cs:270), so in right-to-left analysis of n (z) + i the i capture spans z + i and GetNodes marks the non-optional + optional. FeatureAnalysisRewriteRuleSpec.cs:111 reads only Range.GetStart. Unverified: I could not run a scratch matcher test here, and whether an optional boundary changes any parse is unconfirmed.
| } | ||
|
|
||
| [Test] | ||
| public void MergeRuleUnappliesAcrossInterposedOptionalSegment() |
There was a problem hiding this comment.
FYI: F2. No test has an expansion rule matching across an interposed node, though expansion rules take the same group-capture path (AnalysisRewriteRule builds this spec when the LHS is not the same length as the RHS). ExpandRules passing shows only that adjacent matches still work. A u -> i i case with an optional z between the two is would cover it.
| } | ||
| ); | ||
|
|
||
| // The optional i captures nothing in "pu"; only p becomes optional and u must still be consumed. |
There was a problem hiding this comment.
FYI: F3. The comment states an invariant that the assertion does not check: pu parses to 52 whether or not u is marked optional, so the test only proves the empty capture no longer throws. Either reword the comment to say that, or assert the invariant.
This comment has been minimized.
This comment has been minimized.
ade7402 to
fef6889
Compare
This comment has been minimized.
This comment has been minimized.
3a63132 to
df2cf21
Compare
NarrowAnalysisRewriteRuleSpec.Unapply marked the matched replacement nodes optional by walking range.Start.Next once per replacement position. The analysis matcher skips optional nodes and filters out boundaries, so a multi-node replacement match can span one; the walk then marked that node instead of the real second replacement node, which stayed mandatory, and the underlying reading of the root was unreachable. Read each replacement position back from its own group capture in the analysis pattern, as FeatureAnalysisRewriteRuleSpec already does, and mark exactly those nodes optional. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A narrowing rule's replacement is not validated to be plain constraints, so a quantifier there (an optional segment sequence) compiles and may match no node. Reading the start of an empty capture dereferenced a null range. Skip such positions; a multi-node capture marks every node it covers optional. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
fef6889 to
4ed11ad
Compare
| AssertMorphsEqual(morpher.ParseWord("biiiibiiii"), "27"); | ||
| } | ||
|
|
||
| [Test] |
There was a problem hiding this comment.
FYI: F5. All three new tests use the default rule direction, so none checks the capture read-back when a RightToLeft rule makes analysis match left-to-right and the group registers swap order. A copy of MergeRuleUnappliesAcrossInterposedOptionalSegment with Direction = Direction.RightToLeft on the merge rule would cover it.
The PR has been retargeted from Out of scope, not a finding: Public API, target framework, package, machine.py parity: None verified. Findings: F1 open, unverified; F2 open; F3 open; F4 withdrawn; F5 new. Reviewed at 4ed11ad |
3 participants
Quick summary
A root is no longer lost in analysis when a deletion rule ordered after a narrowing rule reinserts an optional node inside the narrowing rule's replacement. Only parses that were silently dropped change; the HermitCrab suite is otherwise unchanged at 109/109. Synthesis, feature rules, and PR #539's union encoding are untouched.
Where to look
FeatureAnalysisRewriteRuleSpecalready does --RewriteRuleTests.MergeRuleUnappliesAcrossInterposedOptionalSegment(red on master, line 636 assertion; green here).RewriteRuleTests.MergeRuleUnappliesAcrossInterposedOptionalBoundary(red on master, green here).u -> i i) share this spec and this walk --RewriteRuleTests.ExpandRulesstill passes.RewriteRuleTests.MergeRuleWithOptionalReplacementSegmentSkipsEmptyCapture(NullReferenceException without theSuccessguard, green here; it also passes on master, whose raw walk happens to land on real nodes for this word).Deliberately not included
NarrowSynthesisRewriteSubruleSpec.ApplyRhs) that such a rule still trips when it fires. Both are pre-existing and separate from this walk..Nextwalk (F3 in that review). This fix is independent of Fix a performance bug in NarrowAnalysisRewriteRuleSpec #539 and applies to master as it stands; Fix a performance bug in NarrowAnalysisRewriteRuleSpec #539 would need the same group-capture read-back on itstargetNode.Nextsteps.Validation
dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests -c Release -m:4 -nodeReuse:falseon master with the two interposed-node tests -- Failed: 2, Passed: 106, Total: 108 (the two new tests).NullReferenceExceptioninNarrowAnalysisRewriteRuleSpec.Unapply.pwsh ./scripts/comment-hygiene.ps1-- clean (the--agent-strictgate).dotnet CSharpier.dll check .(csharpier 1.2.6 from the restored package;dotnet csharpierrefuses to resolve the local tool on this machine) -- Checked 720 files, exit 0.dotnet restore,dotnet build --no-restore -c Release,dotnet test --verbosity normal, each with-m:4 -nodeReuse:false(the remaininglocal_check.shsteps, run individually to keep the build-parallelism cap) -- restore exit 0; build exit 0; test exit 0, 881 passed / 3 skipped (SIL.Machine.Tests), 4 passed, 109 passed (HermitCrab), 93 passed.git diff --check origin/master...HEAD-- clean.Not CI-equivalent: no coverage collected.
Reading this a year from now
NarrowAnalysisRewriteRuleSpec.Unapplyhandles a rulel1..ln -> r1..rmin analysis by insertingl1..lnas optional nodes after the match and marking the matchedr1..rmoptional, so lexical lookup can read either the surface form or the underlying one. It foundr1..rmby walkingrange.Start.Nextm times. The analysis matcher skips optional nodes and never sees boundaries (its filter is Segment|Anchor), so a match ofr1 r2can span an interior optional node or a boundary. The raw walk then marked that node instead ofr2;r2stayed mandatory and thel1..lnreading was unreachable.Reproduction (test fixture language,
Morphophonemicstratum): synthesis orders a g -> n i, thenz -> 0 / n _ i, which is vacuous in synthesis. In analysis the deletion unapplies first and inserts(z)betweennandi; the merge then matchesn (z) iand the walk marksnand(z), leavingimandatory. Surfacenishould parse to root 32 (sag) and did not. Replacingzwith the boundary+reproduces the same loss through the matcher's filter instead of through an optional node.The fix wraps each replacement constraint in a named group in the analysis pattern and marks the captured node of each group optional. That is the encoding
FeatureAnalysisRewriteRuleSpecalready uses for its per-position read-back.Paths not taken
range.Start.Nextwhile skipping optional and boundary nodes. Rejected: it re-derives what the matcher already decided, and would disagree with the matcher whenever its skip rules change.🤖 Generated with Claude Code
This change is