From 41f319f3d0498ecd8044ea375eb08f1dc1119078 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 23 Oct 2026 15:01:27 -0400 Subject: [PATCH 1/2] Fix directional annotation tie boundaries --- src/SIL.Machine/DataStructures/BidirList.cs | 9 +- .../Annotations/AnnotationTieTests.cs | 244 ++++++++++++++++++ 2 files changed, 251 insertions(+), 2 deletions(-) create mode 100644 tests/SIL.Machine.Tests/Annotations/AnnotationTieTests.cs diff --git a/src/SIL.Machine/DataStructures/BidirList.cs b/src/SIL.Machine/DataStructures/BidirList.cs index a3dad1ce3..173888810 100644 --- a/src/SIL.Machine/DataStructures/BidirList.cs +++ b/src/SIL.Machine/DataStructures/BidirList.cs @@ -264,8 +264,13 @@ public bool Find(TNode start, TNode example, Direction dir, out TNode result) break; if (res == 0) { - result = next; - return true; + if (i == 0) + { + result = next; + return true; + } + + break; } cur = next; next = dir == Direction.LeftToRight ? cur.GetNext(i) : cur.GetPrev(i); diff --git a/tests/SIL.Machine.Tests/Annotations/AnnotationTieTests.cs b/tests/SIL.Machine.Tests/Annotations/AnnotationTieTests.cs new file mode 100644 index 000000000..60eefa928 --- /dev/null +++ b/tests/SIL.Machine.Tests/Annotations/AnnotationTieTests.cs @@ -0,0 +1,244 @@ +using System.Globalization; +using System.Reflection; +using NUnit.Framework; +using SIL.Machine.DataStructures; +using SIL.Machine.FeatureModel; + +namespace SIL.Machine.Annotations; + +[TestFixture] +public class AnnotationTieTests +{ + private const int ExtraCount = 5; + private const string TagFeatureID = "test-tag"; + private static readonly FeatureSystem TagFeatureSystem = new() { new StringFeature(TagFeatureID) }; + private static readonly FieldInfo RandomField = typeof(BidirList>).GetField( + "_rand", + BindingFlags.Instance | BindingFlags.NonPublic + )!; + + private static Shape CreateEmptyShape(int seed) + { + var annotations = new AnnotationList(); + // Seed before Shape construction inserts its margins and later annotations. + RandomField.SetValue(annotations, new Random(seed)); + return new Shape(begin => new ShapeNode(TaggedFeatures(begin ? "margin-begin" : "margin-end")), annotations); + } + + private static (Shape Shape, ShapeNode[] Nodes) BuildShape(int seed) + { + var shape = CreateEmptyShape(seed); + var nodes = new ShapeNode[3]; + for (int i = 0; i < nodes.Length; i++) + nodes[i] = shape.Add(TaggedFeatures(IndexTag("node", i))); + + foreach (int nodeIndex in new[] { 1, 2 }) + { + for (int i = 0; i < ExtraCount; i++) + { + var ann = new Annotation( + Range.Create(nodes[nodeIndex], nodes[nodeIndex]), + TaggedFeatures(IndexTag("point-" + nodeIndex.ToString(CultureInfo.InvariantCulture), i)) + ); + shape.Annotations.Add(ann, false); + } + } + + return (shape, nodes); + } + + private static ( + Shape Shape, + ShapeNode[] Nodes, + Annotation Owner, + Annotation[] TiedAnnotations + ) BuildNestedEndpointTieShape(int seed, int tiedEndpointNodeIndex) + { + var shape = CreateEmptyShape(seed); + var nodes = new ShapeNode[4]; + for (int i = 0; i < nodes.Length; i++) + nodes[i] = shape.Add(TaggedFeatures(IndexTag("nested-node", i))); + + var owner = new Annotation( + Range.Create(nodes[1], nodes[3]), + TaggedFeatures("multi-node-owner") + ); + RandomField.SetValue(owner.Children, new Random(seed + 200)); + owner.Children.AddRange(new[] { nodes[1].Annotation, nodes[2].Annotation, nodes[3].Annotation }); + var tiedAnnotations = new Annotation[ExtraCount]; + for (int i = 0; i < ExtraCount; i++) + { + tiedAnnotations[i] = new Annotation( + Range.Create(nodes[tiedEndpointNodeIndex], nodes[tiedEndpointNodeIndex]), + TaggedFeatures(IndexTag("endpoint-tie", i)) + ); + owner.Children.Add(tiedAnnotations[i], false); + } + + shape.Annotations.Add(owner, false); + return (shape, nodes, owner, tiedAnnotations); + } + + private static FeatureStruct TaggedFeatures(string tag) + { + return FeatureStruct.New(TagFeatureSystem).Feature(TagFeatureID).EqualTo(tag).Value; + } + + private static string IndexTag(string prefix, int index) + { + return prefix + "-" + index.ToString(CultureInfo.InvariantCulture); + } + + private static AnnotationSnapshot Snapshot(Annotation annotation) + { + return new AnnotationSnapshot( + annotation.Range.Start, + annotation.Range.End, + annotation.FeatureStruct.GetValue(TagFeatureID).Values.Single() + ); + } + + private static void AssertMappedAnnotations(Shape source, Shape destination) + { + ShapeNode[] sourceNodes = source.ToArray(); + ShapeNode[] destinationNodes = destination.ToArray(); + Assert.That(destinationNodes, Has.Length.EqualTo(sourceNodes.Length)); + + var nodeMap = new Dictionary(); + for (int i = 0; i < sourceNodes.Length; i++) + nodeMap.Add(sourceNodes[i], destinationNodes[i]); + + AnnotationSnapshot[] expected = source + .Annotations.Where(annotation => + annotation != source.Begin.Annotation && annotation != source.End.Annotation + ) + .Select(annotation => new AnnotationSnapshot( + nodeMap[annotation.Range.Start], + nodeMap[annotation.Range.End], + Snapshot(annotation).Tag + )) + .Concat(new[] { Snapshot(destination.Begin.Annotation), Snapshot(destination.End.Annotation) }) + .ToArray(); + AnnotationSnapshot[] actual = destination.Annotations.Select(Snapshot).ToArray(); + + Assert.That(actual, Is.EquivalentTo(expected)); + Assert.That(actual.All(annotation => destination.Contains(annotation.Start)), Is.True); + Assert.That(actual.All(annotation => destination.Contains(annotation.End)), Is.True); + } + + private static void AssertNestedEndpointTieShrinks(int seed, int removedNodeIndex, int expectedStartNodeIndex) + { + (Shape shape, ShapeNode[] nodes, Annotation owner, Annotation[] tiedAnnotations) = + BuildNestedEndpointTieShape(seed, removedNodeIndex); + Annotation[] expectedRootAnnotations = shape + .Annotations.Where(annotation => annotation != owner) + .ToArray(); + Annotation[] expectedChildAnnotations = owner + .Children.Where(annotation => + annotation != nodes[removedNodeIndex].Annotation && !tiedAnnotations.Contains(annotation) + ) + .ToArray(); + + shape.Remove(nodes[removedNodeIndex]); + + Annotation[] actualRootAnnotations = shape.Annotations.ToArray(); + Assert.That(actualRootAnnotations, Has.Length.EqualTo(expectedRootAnnotations.Length + 1)); + foreach (Annotation annotation in expectedRootAnnotations) + Assert.That(actualRootAnnotations, Does.Contain(annotation)); + Assert.That(actualRootAnnotations, Does.Not.Contain(owner)); + + Annotation resizedOwner = actualRootAnnotations.Single(annotation => + Snapshot(annotation).Tag == "multi-node-owner" + ); + ShapeNode expectedEnd = removedNodeIndex == 3 ? nodes[2] : nodes[3]; + Assert.That(resizedOwner.Range.Start, Is.SameAs(nodes[expectedStartNodeIndex])); + Assert.That(resizedOwner.Range.End, Is.SameAs(expectedEnd)); + Assert.That(resizedOwner.Children.Count, Is.EqualTo(expectedChildAnnotations.Length)); + Assert.That(resizedOwner.Children, Is.EquivalentTo(expectedChildAnnotations)); + foreach (Annotation annotation in tiedAnnotations) + Assert.That(resizedOwner.Children, Does.Not.Contain(annotation)); + Assert.That( + resizedOwner.Children.Any(annotation => + Snapshot(annotation).Tag.StartsWith("endpoint-tie-", StringComparison.Ordinal) + ), + Is.False + ); + } + + [Test] + public void Find_ReturnsFirstMatchingAnnotationInSearchDirection([Range(0, 199)] int seed) + { + (Shape shape, ShapeNode[] nodes) = BuildShape(seed); + Annotation[] tied = shape + .Annotations.Where(ann => ann.Range.Start == nodes[1] && ann.Range.End == nodes[1]) + .ToArray(); + var probe = new Annotation(Range.Create(nodes[1], nodes[1]), TaggedFeatures("probe")); + + Assert.That( + shape.Annotations.Find(probe, Direction.LeftToRight, out Annotation leftToRight), + Is.True + ); + Assert.That(leftToRight, Is.SameAs(tied[0])); + Assert.That( + shape.Annotations.Find(probe, Direction.RightToLeft, out Annotation rightToLeft), + Is.True + ); + Assert.That(rightToLeft, Is.SameAs(tied[^1])); + } + + [Test] + public void GetNodes_ReturnsAnnotationsAtBothTiedBoundaries([Range(0, 199)] int seed) + { + (Shape shape, ShapeNode[] nodes) = BuildShape(seed); + + Assert.That(shape.Annotations.GetNodes(nodes[0], nodes[2]).Count(), Is.EqualTo(3 + 2 * ExtraCount)); + Assert.That(shape.Annotations.GetNodes(nodes[1], nodes[2]).Count(), Is.EqualTo(2 + 2 * ExtraCount)); + Assert.That(shape.Annotations.GetNodes(nodes[0], nodes[1]).Count(), Is.EqualTo(2 + ExtraCount)); + Assert.That(shape.Annotations.GetNodes(nodes[1], nodes[1]).Count(), Is.EqualTo(1 + ExtraCount)); + } + + [Test] + public void CopyAndClone_PreserveMappedAnnotationRangesAndPayloads([Range(0, 199)] int seed) + { + (Shape shape, _) = BuildShape(seed); + Shape copy = CreateEmptyShape(seed + 200); + + shape.CopyTo(copy); + AssertMappedAnnotations(shape, copy); + + Shape clone = shape.Clone(); + AssertMappedAnnotations(shape, clone); + } + + [Test] + public void RemoveNode_PreservesExactUnaffectedAnnotations([Range(0, 199)] int seed) + { + (Shape shape, ShapeNode[] nodes) = BuildShape(seed); + Annotation[] expectedSurvivors = shape + .Annotations.Where(annotation => annotation.Range.Start != nodes[1] && annotation.Range.End != nodes[1]) + .ToArray(); + + shape.Remove(nodes[1]); + + Assert.That(shape.Annotations.Count, Is.EqualTo(expectedSurvivors.Length)); + Assert.That(shape.Annotations, Is.EquivalentTo(expectedSurvivors)); + Assert.That( + shape.Annotations.Any(annotation => annotation.Range.Start == nodes[1] || annotation.Range.End == nodes[1]), + Is.False + ); + } + + [Test] + public void RemoveStartEndpoint_ShrinksMultiNodeOwnerAndRemovesEveryTie([Range(0, 199)] int seed) + { + AssertNestedEndpointTieShrinks(seed, removedNodeIndex: 1, expectedStartNodeIndex: 2); + } + + [Test] + public void RemoveEndEndpoint_ShrinksMultiNodeOwnerAndRemovesEveryTie([Range(0, 199)] int seed) + { + AssertNestedEndpointTieShrinks(seed, removedNodeIndex: 3, expectedStartNodeIndex: 1); + } + + private readonly record struct AnnotationSnapshot(ShapeNode Start, ShapeNode End, string Tag); +} From 731385e734cb39eea765dc52c45c19ecf09958ff Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 6 Nov 2026 23:43:37 -0500 Subject: [PATCH 2/2] Keep directional searches at or after their start Check the starting node before descending from its skip-list predecessor, which can expose an earlier tie or miss result. --- src/SIL.Machine/DataStructures/BidirList.cs | 18 +++- src/SIL.Machine/DataStructures/IBidirList.cs | 6 +- .../DataStructures/BidirListTests.cs | 82 +++++++++++++++++++ 3 files changed, 103 insertions(+), 3 deletions(-) create mode 100644 tests/SIL.Machine.Tests/DataStructures/BidirListTests.cs diff --git a/src/SIL.Machine/DataStructures/BidirList.cs b/src/SIL.Machine/DataStructures/BidirList.cs index 173888810..509afe735 100644 --- a/src/SIL.Machine/DataStructures/BidirList.cs +++ b/src/SIL.Machine/DataStructures/BidirList.cs @@ -240,7 +240,7 @@ public bool Find(TNode start, TNode example, out TNode result) public bool Find(TNode example, Direction dir, out TNode result) { - return Find( + return FindCore( dir == Direction.LeftToRight ? _begin.GetNext(_begin.Levels - 1) : _end.GetPrev(_end.Levels - 1), example, dir, @@ -248,7 +248,23 @@ out result ); } + /// + /// Finds the first matching node at or after in the specified direction. + /// On a miss, the result is at or after in that direction. + /// public bool Find(TNode start, TNode example, Direction dir, out TNode result) + { + int comparison = _comparer.Compare(start, example); + if (comparison == 0 || (dir == Direction.LeftToRight ? comparison > 0 : comparison < 0)) + { + result = start; + return comparison == 0; + } + + return FindCore(start, example, dir, out result); + } + + private bool FindCore(TNode start, TNode example, Direction dir, out TNode result) { TNode cur = dir == Direction.LeftToRight ? start.GetPrev(start.Levels - 1) : start.GetNext(start.Levels - 1); diff --git a/src/SIL.Machine/DataStructures/IBidirList.cs b/src/SIL.Machine/DataStructures/IBidirList.cs index 225432b02..b48f328c8 100644 --- a/src/SIL.Machine/DataStructures/IBidirList.cs +++ b/src/SIL.Machine/DataStructures/IBidirList.cs @@ -82,7 +82,7 @@ public interface IBidirList : ICollection /// /// Finds the node that matches the specified example node. /// - /// The node to start searching from. + /// The node to start searching from, inclusive. /// The example node. /// The result. /// @@ -100,7 +100,9 @@ public interface IBidirList : ICollection /// /// Finds the node that matches the specified example node. /// - /// The node to start searching from. + /// + /// The node to start searching from, inclusive. A match is at or after this node in the search direction. + /// /// The example node. /// The direction. /// The result. diff --git a/tests/SIL.Machine.Tests/DataStructures/BidirListTests.cs b/tests/SIL.Machine.Tests/DataStructures/BidirListTests.cs new file mode 100644 index 000000000..a8673763b --- /dev/null +++ b/tests/SIL.Machine.Tests/DataStructures/BidirListTests.cs @@ -0,0 +1,82 @@ +using System.Reflection; +using NUnit.Framework; +using SIL.Machine.DataStructures; + +namespace SIL.Machine.Tests.DataStructures; + +[TestFixture] +public class BidirListTests +{ + [TestCase(Direction.LeftToRight)] + [TestCase(Direction.RightToLeft)] + public void Find_WithStart_DoesNotReturnEarlierTiedNode(Direction direction) + { + var list = new TestList(); + var left = new TestNode(1); + var start = new TestNode(1); + var right = new TestNode(1); + list.Add(left); + list.Add(start); + list.Add(right); + + Assert.That(list.Find(start, new TestNode(1), direction, out TestNode result), Is.True); + Assert.That(result, Is.SameAs(start)); + } + + [TestCase(Direction.LeftToRight, 0)] + [TestCase(Direction.RightToLeft, 2)] + public void Find_WithStartAndEarlierExample_DoesNotReturnEarlierNode(Direction direction, int example) + { + var list = new TestList(); + list.Add(new TestNode(1)); + var start = new TestNode(1); + list.Add(start); + list.Add(new TestNode(1)); + + Assert.That(list.Find(start, new TestNode(example), direction, out TestNode result), Is.False); + Assert.That(result, Is.SameAs(start)); + } + + [TestCase(Direction.LeftToRight)] + [TestCase(Direction.RightToLeft)] + public void Find_WithStart_ReturnsFirstLaterTie(Direction direction) + { + var list = new TestList(); + var left = new TestNode(direction == Direction.LeftToRight ? 0 : 1); + var middle = new TestNode(1); + var right = new TestNode(direction == Direction.LeftToRight ? 1 : 2); + list.Add(left); + list.Add(middle); + list.Add(right); + TestNode start = direction == Direction.LeftToRight ? left : right; + + Assert.That(list.Find(start, new TestNode(1), direction, out TestNode result), Is.True); + Assert.That(result, Is.SameAs(middle)); + } + + private sealed class TestNode(int value) : BidirListNode + { + public int Value { get; } = value; + } + + private sealed class TestList : BidirList + { + public TestList() + : base( + Comparer.Create((x, y) => x.Value.CompareTo(y.Value)), + begin => new TestNode(begin ? int.MinValue : int.MaxValue) + ) + { + typeof(BidirList) + .GetField("_rand", BindingFlags.Instance | BindingFlags.NonPublic)! + .SetValue(this, new LevelRandom()); + } + } + + private sealed class LevelRandom : Random + { + private readonly Queue _values = new(new[] { 0, 1, 0 }); + + public override int Next() => _values.Dequeue(); + } +}