Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions src/SIL.Machine/Annotations/AnnotationList.cs
Original file line number Diff line number Diff line change
Expand Up @@ -227,12 +227,49 @@ public bool FindDepthFirst(TOffset offset, Direction dir, out Annotation<TOffset
if (Find(offset, dir, out result))
return true;

if (!result.IsLeaf && result.Range.Contains(offset, dir))
return result.Children.FindDepthFirst(offset, dir, out result);
// 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))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
if (
!tied.IsLeaf
&& tied.Range.Contains(offset, dir)
&& tied.Children.FindDepthFirst(offset, dir, out Annotation<TOffset> inner)
)
{
result = inner;
return true;
}
}

if (!candidate.IsLeaf && candidate.Range.Contains(offset, dir))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return candidate.Children.FindDepthFirst(offset, dir, out result);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return false;
}

private IEnumerable<Annotation<TOffset>> GetSameRange(Annotation<TOffset> ann)
{
yield return ann;
if (ann == Begin || ann == End)
yield break;

for (
Annotation<TOffset> cur = ann.Prev;
cur != Begin && cur.Range.CompareTo(ann.Range) == 0;
cur = cur.Prev
)
{
yield return cur;
}

for (Annotation<TOffset> cur = ann.Next; cur != End && cur.Range.CompareTo(ann.Range) == 0; cur = cur.Next)
{
yield return cur;
}
}

public IEnumerable<Annotation<TOffset>> GetNodes(TOffset start, TOffset end)
{
return GetNodes(Range<TOffset>.Create(start, end));
Expand Down
102 changes: 102 additions & 0 deletions tests/SIL.Machine.Tests/Annotations/AnnotationTiedContainerTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
using System.Reflection;
using NUnit.Framework;
using SIL.Machine.DataStructures;
using SIL.Machine.FeatureModel;

namespace SIL.Machine.Annotations;

[TestFixture]
public class AnnotationTiedContainerTests
{
private const int ContainerCount = 4;

private static Shape CreateShape(int seed, out ShapeNode[] nodes)
{
var annotations = new AnnotationList<ShapeNode>();
typeof(BidirList<Annotation<ShapeNode>>)
.GetField("_rand", BindingFlags.Instance | BindingFlags.NonPublic)!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.SetValue(annotations, new Random(seed));
var shape = new Shape(begin => new ShapeNode(FeatureStruct.New().Value), annotations);
nodes = new ShapeNode[3];
for (int i = 0; i < nodes.Length; i++)
nodes[i] = shape.Add(FeatureStruct.New().Value);
return shape;
}

private static Annotation<ShapeNode> NewContainer(ShapeNode[] nodes)
{
return new Annotation<ShapeNode>(Range<ShapeNode>.Create(nodes[0], nodes[2]), FeatureStruct.New().Value);
}

// Only the tied container at ownerIndex owns the segments, so the one Find returns varies with insertion order.
[Test]
public void FindDepthFirst_TiedContainers_FindsSegmentInOwner(
[Values(0, 1, 2, 3, 4, 5)] int seed,
[Range(0, ContainerCount - 1)] int ownerIndex,
[Values] Direction dir
)
{
Shape shape = CreateShape(seed, out ShapeNode[] nodes);
for (int i = 0; i < ContainerCount; i++)
{
Annotation<ShapeNode> container = NewContainer(nodes);
if (i == ownerIndex)
container.Children.AddRange(nodes.Select(n => n.Annotation));
shape.Annotations.Add(container, false);
}

Assert.That(shape.Annotations.FindDepthFirst(nodes[1], dir, out Annotation<ShapeNode> result), Is.True);
Assert.That(result, Is.SameAs(nodes[1].Annotation));
}

// The owner is itself a container of tied containers, only one of which owns the segment annotations.
[Test]
public void FindDepthFirst_NestedTiedContainers_FindsSegmentInInnermostOwner(
[Values(0, 1, 2, 3, 4, 5)] int seed,
[Range(0, ContainerCount - 1)] int outerOwnerIndex,
[Range(0, ContainerCount - 1)] int innerOwnerIndex,
[Values] Direction dir
)
{
Shape shape = CreateShape(seed, out ShapeNode[] nodes);
for (int i = 0; i < ContainerCount; i++)
{
Annotation<ShapeNode> outer = NewContainer(nodes);
if (i == outerOwnerIndex)
{
for (int j = 0; j < ContainerCount; j++)
{
Annotation<ShapeNode> inner = NewContainer(nodes);
if (j == innerOwnerIndex)
inner.Children.AddRange(nodes.Select(n => n.Annotation));
outer.Children.Add(inner, false);
}
}
shape.Annotations.Add(outer, false);
}

Assert.That(shape.Annotations.FindDepthFirst(nodes[1], dir, out Annotation<ShapeNode> result), Is.True);
Assert.That(result, Is.SameAs(nodes[1].Annotation));
}

// Tied containers that own only the other segments must not hide, or stand in for, an annotation that is not
// inside any of them.
[Test]
public void FindDepthFirst_TiedContainersNotOwningTheOffset_ReturnsTheUnownedAnnotation(
[Values(0, 1, 2, 3, 4, 5)] int seed,
[Values] Direction dir
)
{
Shape shape = CreateShape(seed, out ShapeNode[] nodes);
for (int i = 0; i < ContainerCount; i++)
{
Annotation<ShapeNode> container = NewContainer(nodes);
if (i == 0)
container.Children.AddRange(new[] { nodes[0].Annotation, nodes[2].Annotation });
shape.Annotations.Add(container, false);
}

Assert.That(shape.Annotations.FindDepthFirst(nodes[1], dir, out Annotation<ShapeNode> result), Is.True);
Assert.That(result, Is.SameAs(nodes[1].Annotation));
}
}
Loading