Repository navigation
Find tied containers' descendants regardless of insertion order - #557
johnml1135 wants to merge 1 commit into
Conversation
AnnotationList.FindDepthFirst only descended into the single container that Find returned. When several containers share a range and only one owns the children, the result depended on insertion order and skip-list levels. Try every container with the same range as the one Find returned. Fixes #554 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #557 +/- ##
==========================================
+ Coverage 74.57% 74.58% +0.01%
==========================================
Files 453 453
Lines 38314 38341 +27
Branches 5280 5285 +5
==========================================
+ Hits 28573 28597 +24
- Misses 8576 8579 +3
Partials 1165 1165 ☔ 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}
| } | ||
|
|
||
| if (!candidate.IsLeaf && candidate.Range.Contains(offset, dir)) | ||
| return candidate.Children.FindDepthFirst(offset, dir, out result); |
There was a problem hiding this comment.
FYI: F1. On a miss, result still comes only from candidate, so the start that Matcher derives from it (Matcher.cs:329-334) still depends on which tie sorts last. Ties A (owns n0) and B (owns n3), offset n2: with B as candidate, the result is B's Begin, so Matcher starts at n3. With A as candidate, the result is n0, so Matcher starts at A's End. No test covers the miss path. Unverified: traced, not run.
| } | ||
| } | ||
|
|
||
| if (!candidate.IsLeaf && candidate.Range.Contains(offset, dir)) |
There was a problem hiding this comment.
FYI: F2. On a miss, candidate's subtree is searched twice: once in the loop, because GetSameRange yields it first, and again here. The cost compounds across nested failing levels: about (ties + 1)^depth searches, where the old code did one per level. Keep the loop's inner for candidate and return that instead.
| // Several containers can share the same range while only one of them owns the children that lead to the | ||
| // offset, so every one of them has to be tried, not just the one Find happened to return. | ||
| Annotation<TOffset> candidate = result; | ||
| foreach (Annotation<TOffset> tied in GetSameRange(candidate)) |
There was a problem hiding this comment.
FYI: F3. Only containers with exactly the same range are tried, so an earlier sibling with a wider range that owns the offset is still missed. Example: A[n0,n3] owns n0..n3 and B[n0,n2] is a leaf, both added with subsume: false. The list order is A, B, Find(n1) returns B, and FindDepthFirst(n1) returns false. This is pre-existing and deterministic. Traced, not run.
| { | ||
| var annotations = new AnnotationList<ShapeNode>(); | ||
| typeof(BidirList<Annotation<ShapeNode>>) | ||
| .GetField("_rand", BindingFlags.Instance | BindingFlags.NonPublic)! |
There was a problem hiding this comment.
FYI: F4. Because _rand is set by reflection, renaming that private field would make all 252 cases fail with a NullReferenceException from the !. SIL.Machine already declares InternalsVisibleTo("SIL.Machine.Tests"), so an internal seed constructor on BidirList would give the tests a typed hook instead.
|
Verdict: approve. The hit path now tries every container with the same range, and the new tests cover each owner position, seed and direction. Most important: F1 ( Counts: 0 Critical, 0 Important, 4 Low. Ran: Not verified: F1 and F3 were traced, not run, because this session could not write a scratch test. I did not run this branch combined with #556. Public API, target framework, package, machine.py parity: None verified. F1 new, unverified. F2 new. F3 new, unverified. F4 new. Reviewed at 04b7986 |
Quick summary
AnnotationList.FindDepthFirstnow tries every container that shares the range of the oneFindreturned. Previously it descended only into that single container, so a segment inside an equal-range sibling was found or missed depending on insertion order and skip-list levels. The failing-lookup result (nearest descendant) is unchanged.Where to look
AnnotationList.FindDepthFirstand the new privateGetSameRange: walks the equal-range group in both list directions, so it does not depend on which tied nodeFindpicks. The change is independent of theBidirList.Findboundary change in Fix directional annotation tie boundaries #556.AnnotationTiedContainerTests: four tied containers, the owner at each position, six fixed skip-list seeds (set by reflection on the test instance only), both search directions. Covers flat ties, nested ties (tied containers inside the owner), and ties that do not own the offset.Deliberately not included
BidirList.Find(HermitCrab: investigate unstable zero-width morpheme identities across fresh processes #506, Fix directional annotation tie boundaries #556).Shape.Remove(Shape.Remove can leave a spanning annotation anchored to a removed end segment #555).GetNodes,Shape.UpdateAnnotationsandMatcher: not part of AnnotationList.FindDepthFirst misses descendants in tied containers depending on insertion order #554's acceptance criteria.Validation
./local_check.sh --agent-strict: passed (comment hygiene, csharpier, build, tests).AnnotationTiedContainerTests: 252 passed, 0 failed with the fix.AnnotationList.csreverted to master: 216 of 252 failed, 36 passed.Issue context
Related: #506, #500, #555, #556.
Fixes #554
🤖 Generated with Claude Code
This change is