Settle a pending inline snapshot by member when its line has moved

A queue entry is keyed by source file and line, and the line stops being
true the moment an accept inserts a literal above it: a snapshot is
several lines of source, so accepting one call site moves every later one
in that file. The entries left behind could then never be settled by any
later run, because the run reports the line as it is now and the entry
still holds the line it was queued at. Verify already carries the member
on a patch for exactly this reason, but only the patcher used it.

Settle now takes the member and falls back to it when the key matches
nothing, resolving the entry within the same file. Only where the member
names exactly one entry: a member holding several inline snapshots cannot
say which of them a settle was for, and dropping the wrong one loses a
pending snapshot outright, so an ambiguous member settles nothing.

The member rides the wire as an added optional field, which an owner that
predates it reads straight past, so a newer sender still settles by key
alone against an older one.

Also adds RetireInline, for a call site that is no longer an inline
snapshot at all rather than one whose test now passes. It carries no
framework, because the statement is not 'this framework passes' but
'there is no snapshot here for any of them', so the owner takes the whole
entry instead of stripping one variant and leaving it standing on the
others. Verify calls it when a verification resolves as not inline, which
is the other half of this fix and lands there.

Author: SimonCropp

src/DiffEngine.Tests/InlineQueueClientTests.cs

@@ -369,11 +369,11 @@ int IQueueOwner.Enqueue(InlinePatch patch)
             }
         }
 
-        void IQueueOwner.Settle(string key, string? origin)
+        void IQueueOwner.Settle(string key, string? origin, string? member)
         {
             lock (gate)
             {
-                queue = queue.Settle(key, origin);
+                queue = queue.Settle(key, origin, member);
             }
         }
 

src/DiffEngine.Tests/InlineQueueTests.cs

@@ -10,11 +10,13 @@ static InlinePatch Patch(
         int line = 42,
         string content = "new",
         string? framework = null,
-        string? testName = null) =>
+        string? testName = null,
+        string? member = null) =>
         new(source, line, "\"old\"", content)
         {
             Framework = framework,
-            TestName = testName
+            TestName = testName,
+            MemberName = member
         };
 
     static InlineApplyResult Fails(InlinePatch patch) =>
@@ -95,6 +97,69 @@ public async Task SettleForAnUnknownKeyReturnsTheSameQueue()
         await Assert.That(queue.Settle("nothing")).IsSameReferenceAs(queue);
     }
 
+    /// <summary>
+    /// Accepting a snapshot inserts several lines of source, so every later call site in that file
+    /// moves and the entries queued against them can never be named by their line again. The
+    /// member is what still points at them.
+    /// </summary>
+    [Test]
+    public async Task SettleFindsAnEntryWhoseLineHasMovedByMember()
+    {
+        var queue = InlineQueue.Empty
+            .Enqueue(Patch(line: 42, member: "MyTest"))
+            .Settle(InlineKey.For("Sample.cs", 807), null, "MyTest");
+
+        await Assert.That(queue.Count).IsEqualTo(0);
+    }
+
+    /// <summary>
+    /// A member holding several inline snapshots cannot say which of them the settle was for, and
+    /// dropping the wrong one loses a pending snapshot outright.
+    /// </summary>
+    [Test]
+    public async Task SettleLeavesAnAmbiguousMemberAlone()
+    {
+        var queue = InlineQueue.Empty
+            .Enqueue(Patch(line: 42, member: "MyTest"))
+            .Enqueue(Patch(line: 48, member: "MyTest"));
+
+        await Assert.That(queue.Settle(InlineKey.For("Sample.cs", 807), null, "MyTest"))
+            .IsSameReferenceAs(queue);
+    }
+
+    [Test]
+    public async Task SettleDoesNotMatchTheSameMemberInAnotherFile()
+    {
+        var queue = InlineQueue.Empty.Enqueue(Patch(source: "Sample.cs", member: "MyTest"));
+
+        await Assert.That(queue.Settle(InlineKey.For("Other.cs", 807), null, "MyTest"))
+            .IsSameReferenceAs(queue);
+    }
+
+    [Test]
+    public async Task SettleWithoutAMemberStillOnlyMatchesTheKey()
+    {
+        var queue = InlineQueue.Empty.Enqueue(Patch(line: 42, member: "MyTest"));
+
+        await Assert.That(queue.Settle(InlineKey.For("Sample.cs", 807))).IsSameReferenceAs(queue);
+    }
+
+    /// <summary>
+    /// A call site that is no longer inline at all carries no framework, because the statement is
+    /// not "this framework now passes" but "there is no snapshot here for any of them". So it
+    /// takes the whole entry, not one variant of it.
+    /// </summary>
+    [Test]
+    public async Task SettleWithoutAnOriginTakesEveryVariant()
+    {
+        var queue = InlineQueue.Empty
+            .Enqueue(Patch(content: "eight", framework: "net8.0", member: "MyTest"))
+            .Enqueue(Patch(content: "nine", framework: "net9.0", member: "MyTest"))
+            .Settle(InlineKey.For("Sample.cs", 807), null, "MyTest");
+
+        await Assert.That(queue.Count).IsEqualTo(0);
+    }
+
     [Test]
     public async Task AcceptAppliesAndRemoves()
     {

src/DiffEngine.Tests/ViewerProtocolTests.cs

@@ -252,6 +252,31 @@ public async Task SettleCarriesTheOriginInTheBody()
         await Assert.That(message.Body).IsEqualTo("net9.0");
     }
 
+    [Test]
+    public async Task SettleCarriesTheMember()
+    {
+        var payload = new ViewerMessage(ViewerVerb.Settle, InlineKey.For("tests.cs", 42), "net9.0", "MyTest")
+            .Build();
+
+        await Assert.That(ViewerMessage.TryParse(payload, out var message)).IsTrue();
+        await Assert.That(message!.Key).IsEqualTo("tests.cs|42");
+        await Assert.That(message.Body).IsEqualTo("net9.0");
+        await Assert.That(message.Member).IsEqualTo("MyTest");
+    }
+
+    /// <summary>
+    /// The member is an added field, so a payload written before it existed still reads, which is
+    /// what lets a newer sender talk to an older owner.
+    /// </summary>
+    [Test]
+    public async Task SettleWithoutAMemberParses()
+    {
+        var payload = new ViewerMessage(ViewerVerb.Settle, InlineKey.For("tests.cs", 42), "net9.0").Build();
+
+        await Assert.That(ViewerMessage.TryParse(payload, out var message)).IsTrue();
+        await Assert.That(message!.Member).IsNull();
+    }
+
     [Test]
     public async Task AFullListingCarriesThePrimaryOrigins()
     {
@@ -469,7 +494,7 @@ class FakeOwner((bool ok, string? message) act) :
 
         public int Enqueue(InlinePatch patch) => 1;
 
-        public void Settle(string key, string? origin)
+        public void Settle(string key, string? origin, string? member)
         {
         }
 

src/DiffEngine/DiffRunner_Inline.cs

@@ -78,14 +78,40 @@ public static async Task<InlineResult> AddInlineAsync(InlinePatch patch, Cancel
     /// conflicted entry; the other framework's differing content stays pending.
     /// </para>
     /// </summary>
-    public static void SettleInline(string sourceFile, int line)
+    /// <param name="memberName">
+    /// The member the call site sits in. Optional, and only used where the line no longer names
+    /// the entry, which is what happens once an accept inserts a literal above it.
+    /// </param>
+    public static void SettleInline(string sourceFile, int line, string? memberName = null)
     {
         if (Disabled)
         {
             return;
         }
 
-        ViewerClient.TrySend(new(ViewerVerb.Settle, InlineKey.For(sourceFile, line), RuntimeMoniker.Current));
+        ViewerClient.TrySend(
+            new(ViewerVerb.Settle, InlineKey.For(sourceFile, line), RuntimeMoniker.Current, memberName));
+    }
+
+    /// <summary>
+    /// Drops a pending inline snapshot for a call site that is no longer an inline snapshot at
+    /// all: the verification opted out, the global switch declined it, or its literal outgrew the
+    /// size limit and moved to a file.
+    /// <para>
+    /// Unlike <see cref="SettleInline" /> this carries no framework, because the statement is not
+    /// "this framework now passes" but "there is no inline snapshot here for any of them". A
+    /// per-framework settle would strip one label and leave the entry standing on the others,
+    /// pending against a call site that can never produce a snapshot again.
+    /// </para>
+    /// </summary>
+    public static void RetireInline(string sourceFile, int line, string? memberName = null)
+    {
+        if (Disabled)
+        {
+            return;
+        }
+
+        ViewerClient.TrySend(new(ViewerVerb.Settle, InlineKey.For(sourceFile, line), null, memberName));
     }
 
     static InlineResult CheckInline()

src/DiffEngine/Inline/InlineQueue.cs

@@ -215,16 +215,28 @@ static PendingInline Fold(PendingInline entry, InlinePatch patch)
     public InlineQueue Settle(string key) =>
         Settle(key, null);
 
+    public InlineQueue Settle(string key, string? origin) =>
+        Settle(key, origin, null);
+
     /// <summary>
     /// Origin-scoped settle. A framework that starts passing removes only its own label; a variant
     /// with no labels left is dropped, and the entry goes when its last variant does, so the other
     /// framework's still-failing content stays reviewable. A null origin, or an entry whose
     /// variants are all unlabeled, settles the whole entry.
     /// </summary>
-    public InlineQueue Settle(string key, string? origin)
+    /// <param name="member">
+    /// The member the settled call site sits in, used only when <paramref name="key" /> matches
+    /// nothing. See <see cref="FindByMember" />.
+    /// </param>
+    public InlineQueue Settle(string key, string? origin, string? member)
     {
         var items = Items.ToList();
         var index = items.FindIndex(_ => _.Key == key);
+        if (index < 0)
+        {
+            index = FindByMember(items, key, member);
+        }
+
         if (index < 0)
         {
             return this;
@@ -271,6 +283,62 @@ public InlineQueue Settle(string key, string? origin)
         return new(items);
     }
 
+    /// <summary>
+    /// The entry a settle was for when its key names no entry, found by the member instead.
+    /// </summary>
+    /// <remarks>
+    /// A key names a line, and a line stops being true the moment an accept inserts a literal
+    /// above it — a snapshot is several lines of source, so accepting one call site moves every
+    /// later one in the file. The entries left behind could then never be settled by any later
+    /// run: the run reports the line as it is now, and the entry still holds the line it was
+    /// queued at. The member survives that, which is why a patch carries it and why the patcher
+    /// already locates call sites by it.
+    /// <para>
+    /// Only when the member names exactly one entry in that file. A member holding several inline
+    /// snapshots cannot say which of them a settle was for, and dropping the wrong one loses a
+    /// pending snapshot outright — so an ambiguous member settles nothing, leaving the entries as
+    /// they were.
+    /// </para>
+    /// </remarks>
+    static int FindByMember(List<PendingInline> items, string key, string? member)
+    {
+        if (string.IsNullOrEmpty(member))
+        {
+            return -1;
+        }
+
+        var file = FileOf(key);
+        var found = -1;
+        for (var index = 0; index < items.Count; index++)
+        {
+            var entry = items[index];
+            if (entry.Patch.MemberName != member ||
+                FileOf(entry.Key) != file)
+            {
+                continue;
+            }
+
+            if (found >= 0)
+            {
+                return -1;
+            }
+
+            found = index;
+        }
+
+        return found;
+    }
+
+    /// <summary>
+    /// The file half of a key. Taken off the key rather than off the patch, so both sides are
+    /// case folded the way <see cref="InlineKey.For" /> folds them for this platform.
+    /// </summary>
+    static string FileOf(string key)
+    {
+        var separator = key.LastIndexOf('|');
+        return separator < 0 ? key : key[..separator];
+    }
+
     public InlineQueue Discard(string key, out string? message)
     {
         var entry = Find(key);

src/DiffEngine/Protocol/IQueueOwner.cs

@@ -13,11 +13,12 @@ interface IQueueOwner
     int Enqueue(InlinePatch patch);
 
     /// <summary>
-    /// Drop the entry for a key whose test started passing — or, with an origin, just that
-    /// framework's variant of it. An unknown key is a no-op, because the entry being gone is the
+    /// Drop the entry for a key whose test started passing, or is no longer an inline snapshot at
+    /// all — or, with an origin, just that framework's variant of it. An unknown key falls back to
+    /// <paramref name="member" />, and is otherwise a no-op, because the entry being gone is the
     /// goal state.
     /// </summary>
-    void Settle(string key, string? origin);
+    void Settle(string key, string? origin, string? member);
 
     /// <summary>
     /// Track a pending file move, replacing the entry for the same received file — a re-run

src/DiffEngine/Protocol/ViewerMessage.cs

@@ -5,14 +5,20 @@ namespace DiffEngine;
 /// the verbs that act on one; <paramref name="Body"/> carries an <see cref="InlinePatchFile"/>
 /// payload for <see cref="ViewerVerb.Inline"/>.
 /// </summary>
-record ViewerMessage(ViewerVerb Verb, string? Key = null, string? Body = null)
+/// <param name="Member">
+/// The member the call site sits in, on a <see cref="ViewerVerb.Settle"/>. A fallback for the key,
+/// which names a line and so stops being true once an accept inserts a literal above it. Optional,
+/// and read past by an owner that predates it, so an older one still settles by key alone.
+/// </param>
+record ViewerMessage(ViewerVerb Verb, string? Key = null, string? Body = null, string? Member = null)
 {
     public string Build()
     {
         var builder = new StringBuilder($"version: {ViewerPayload.Version}\n");
         builder.Append($"verb: {Verb.ToString().ToLowerInvariant()}\n");
         ViewerPayload.Append(builder, "key", Key);
         ViewerPayload.Append(builder, "body", Body);
+        ViewerPayload.Append(builder, "member", Member);
         return builder.ToString();
     }
 
@@ -28,6 +34,7 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerMessage?
         ViewerVerb? verb = null;
         string? key = null;
         string? body = null;
+        string? member = null;
         foreach (var (name, value) in lines)
         {
             switch (name)
@@ -55,6 +62,13 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerMessage?
                         return false;
                     }
 
+                    continue;
+                case "member":
+                    if (!ViewerPayload.TryDecode(value, out member))
+                    {
+                        return false;
+                    }
+
                     continue;
                 default:
                     // Unknown fields are ignored so a newer client can add one without breaking
@@ -68,7 +82,7 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerMessage?
             return false;
         }
 
-        message = new(verb.Value, key, body);
+        message = new(verb.Value, key, body, member);
         return true;
     }
 }

src/DiffEngine/Protocol/ViewerMessageHandler.cs

@@ -16,7 +16,7 @@ public static ViewerResponse Handle(IQueueOwner owner, ViewerMessage message)
             case ViewerVerb.Inline:
                 return Inline(owner, message.Body);
             case ViewerVerb.Settle:
-                return Settle(owner, message.Key, message.Body);
+                return Settle(owner, message.Key, message.Body, message.Member);
             case ViewerVerb.Move:
                 return Move(owner, message.Key, message.Body);
             case ViewerVerb.Delete:
@@ -74,14 +74,14 @@ static ViewerResponse Inline(IQueueOwner owner, string? body)
         return ViewerResponse.Success($"Queued {owner.Enqueue(patch)}");
     }
 
-    static ViewerResponse Settle(IQueueOwner owner, string? key, string? origin)
+    static ViewerResponse Settle(IQueueOwner owner, string? key, string? origin, string? member)
     {
         if (key is null)
         {
             return ViewerResponse.Error("Settle requires a key");
         }
 
-        owner.Settle(key, origin);
+        owner.Settle(key, origin, member);
         return ViewerResponse.Success();
     }
 

src/DiffEngineTray/OwnedInlineHost.cs

@@ -170,11 +170,11 @@ int IQueueOwner.Enqueue(InlinePatch patch)
         return count;
     }
 
-    void IQueueOwner.Settle(string key, string? origin)
+    void IQueueOwner.Settle(string key, string? origin, string? member)
     {
         lock (gate)
         {
-            var settled = queue.Settle(key, origin);
+            var settled = queue.Settle(key, origin, member);
             if (ReferenceEquals(settled, queue))
             {
                 return;

src/DiffEngineViewer/Ipc/MessageHandler.cs

@@ -27,8 +27,8 @@ int IQueueOwner.Enqueue(InlinePatch patch)
         return count;
     }
 
-    void IQueueOwner.Settle(string key, string? origin) =>
-        host.Mutate(_ => ViewerSession.Settle(_, key, origin));
+    void IQueueOwner.Settle(string key, string? origin, string? member) =>
+        host.Mutate(_ => ViewerSession.Settle(_, key, origin, member));
 
     /// <summary>
     /// The files are read here, on the listener thread, so the session stays IO free — the same