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 |
Start here:
src/SIL.Machine/Annotations/AnnotationList.cs,FindDepthFirst(+41 lines): the whole fix.Ready for review. All CI checks pass.
AnnotationList.FindDepthFirstnow tries every container that shares the range of the oneFindreturned. It used to descend 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.Reviewers arrive asking whether this changes lookups that already work. It does not: with one container at a range the group has one member and the path is the old one. The question worth your time: when several tied containers each own the offset, the walk returns the first match, trying the
Findresult, then earlier tied nodes, then later ones.Where to look:
FindDepthFirstand the new privateGetSameRange: walks the equal-range group in both list directions, so it does not depend on which tied nodeFindpicks. 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. Pins flat ties, nested ties, and ties that do not own the offset.Not here:
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: outside AnnotationList.FindDepthFirst misses descendants in tied containers depending on insertion order #554's acceptance criteria.Verification:
./local_check.sh --agent-strictpassed (comment hygiene, csharpier, build, tests). With onlyAnnotationList.csreverted to master, most of the new test cases fail. Not run: this branch combined with #556.Next: approve, or say which tied-ownership case is missing from the tests.
Reading this a year from now -- start here
Fixes #554 (open):
FindDepthFirstmissed descendants of tied containers depending on insertion order. Related open items: #506, #555. #500 and #556 are open PRs touching neighbouring tie behaviour. The fix is deliberately limited toAnnotationList.cs; the reasoning below is the only record.Decisions, and why
Findreturns: that choice is what Fix directional annotation tie boundaries #556 changes, and this fix must hold whichever nodeFindpicks.Evidence
./local_check.sh --agent-strict: passed (comment hygiene, csharpier, build, tests).AnnotationTiedContainerTestspassed with the fix in the earlier run; test case counts were not recomputed for this description.AnnotationList.csreverted to master, most of the new cases failed and a minority passed.🤖 Generated with Claude Code
This change is