Inline append already applied (#857)

* An append onto the same content is AlreadyApplied

A multi-targeted project transitioning to inline snapshots fails the same
call site under every framework, and each one queues an append. Accepting
the first writes the literal the rest are still carrying, so the second
accept found a Snapshot call in the way and refused - reporting a failure
over a source file that was already right, and sending the reader off to
re-run a test with nothing left to say.

TryAppend now reads the chained call's argument before refusing. Same
content is AlreadyApplied; different content is still NotFound, since that
one genuinely cannot say what it wants until it has been re-run against the
literal now in the source. Compared by value rather than by text, so a
literal written in another shape still counts and F# answers as C# does
despite the layout of its triple quoted literal.

WalkChain reports the position of the call it found rather than the fact of
it, because a caller deciding what to do about one has to read its argument.

* Move the AlreadyApplied paragraph into inline.source.md

docs/inline.md is generated, so the paragraph was written into the output
and the next build stripped it back out. It belongs in the mdsource the
generator reads; the generated file is unchanged either way.

* Update Directory.Build.props

Author: SimonCropp

docs/inline.md

@@ -211,6 +211,8 @@ For the staging fallback, where no viewer could be resolved and the patch is a f
 
 `Apply` returns `Applied`, `AlreadyApplied` (the literal already matches), `NotFound` (the source changed since the test run — tell the user to re-run rather than retrying), or a failure with a message (locked file, unreadable source), which is retryable.
 
+`AlreadyApplied` covers an `Append` onto a call that already has a `Snapshot` call holding this same content, which is what a multi-targeted project transitioning to inline meets: every framework fails the call site and queues an append, and whichever is accepted first writes the literal the rest are carrying. Only a chained call holding *different* content is `NotFound` — that one genuinely cannot say what it wants until it has been re-run against the literal now in the source. Accepting one framework's append before the others have run does mean the queue never sees them together, so a real disagreement between frameworks is reported as that `NotFound` rather than as a conflict to pick from.
+
 `Remove` mode patches are configuration changes with nothing to review: apply them directly; `AddInlineAsync` refuses them.
 
 

docs/mdsource/inline.source.md

@@ -204,6 +204,8 @@ For the staging fallback, where no viewer could be resolved and the patch is a f
 
 `Apply` returns `Applied`, `AlreadyApplied` (the literal already matches), `NotFound` (the source changed since the test run — tell the user to re-run rather than retrying), or a failure with a message (locked file, unreadable source), which is retryable.
 
+`AlreadyApplied` covers an `Append` onto a call that already has a `Snapshot` call holding this same content, which is what a multi-targeted project transitioning to inline meets: every framework fails the call site and queues an append, and whichever is accepted first writes the literal the rest are carrying. Only a chained call holding *different* content is `NotFound` — that one genuinely cannot say what it wants until it has been re-run against the literal now in the source. Accepting one framework's append before the others have run does mean the queue never sees them together, so a real disagreement between frameworks is reported as that `NotFound` rather than as a conflict to pick from.
+
 `Remove` mode patches are configuration changes with nothing to review: apply them directly; `AddInlineAsync` refuses them.
 
 

src/DiffEngine.Tests/InlinePatcherFsTests.cs

@@ -480,6 +480,30 @@ public async Task AppendIsRefusedWhenOneIsAlreadyChained()
         await Assert.That(reason).Contains("already has a Snapshot call");
     }
 
+    // The second framework of a multi-targeted transition, appending onto the literal the first
+    // one's accept just wrote. Read by value, so F# gets the same answer C# does even though the
+    // layout of its triple quoted literal is nothing like the content it stands for
+    [Test]
+    public async Task AppendOntoTheSameContentIsAlreadyApplied()
+    {
+        var source = Test("    Verifier.Verify(15).Snapshot(\"same\").ToTask()");
+
+        var status = TryApply(source, 5, InlinePatchMode.Append, null, "same", out _, out _);
+
+        await Assert.That(status).IsEqualTo(PatchStatus.AlreadyApplied);
+    }
+
+    [Test]
+    public async Task AppendingTheSameContentTwiceIsAlreadyApplied()
+    {
+        var source = Test("    Verifier.Verify(15).ToTask()");
+
+        TryApply(source, 5, InlinePatchMode.Append, null, "a\nb", out var applied, out _);
+        var status = TryApply(applied, 5, InlinePatchMode.Append, null, "a\nb", out _, out _);
+
+        await Assert.That(status).IsEqualTo(PatchStatus.AlreadyApplied);
+    }
+
     [Test]
     public async Task AppendSkipsAVerifyOnAnotherReceiver()
     {

src/DiffEngine.Tests/InlinePatcherTests.cs

@@ -821,6 +821,36 @@ public async Task AppendIsRefusedWhenOneIsAlreadyChained()
         await Assert.That(reason).Contains("already has a Snapshot call");
     }
 
+    /// <summary>
+    /// The multi-targeted transition, which is where an append lands on a chained call that is
+    /// not in its way at all. Two frameworks fail the same snapshot, each queues an append, and
+    /// accepting the first writes the literal the second is still carrying. Refused, that reads
+    /// as a failure over a source file that is already right, and sends the reader off to re-run
+    /// a test with nothing left to say.
+    /// </summary>
+    [Test]
+    public async Task AppendOntoTheSameContentIsAlreadyApplied()
+    {
+        var source = Method("        await Verify(value)\n            .Snapshot(\"new\");");
+
+        var status = TryApply(source, 5, InlinePatchMode.Append, null, "new", out _, out _);
+
+        await Assert.That(status).IsEqualTo(PatchStatus.AlreadyApplied);
+    }
+
+    // The two halves of that have to agree, or the second framework's accept is refused over a
+    // literal the first one wrote from the very content being compared against it
+    [Test]
+    public async Task AppendingTheSameContentTwiceIsAlreadyApplied()
+    {
+        var source = Method("        await Verify(value);");
+
+        TryApply(source, 5, InlinePatchMode.Append, null, "a\nb", out var applied, out _);
+        var status = TryApply(applied, 5, InlinePatchMode.Append, null, "a\nb", out _, out _);
+
+        await Assert.That(status).IsEqualTo(PatchStatus.AlreadyApplied);
+    }
+
     [Test]
     public async Task AppendWithNoVerifyCall()
     {

src/DiffEngine/Inline/InlineApplier.cs

@@ -40,10 +40,12 @@ public static InlineApplyResult CanApply(InlinePatch patch) =>
     /// over. Applied for yes, NotFound for no, and Failed where the source could not be read.
     /// <para>
     /// Apart from CanApply in one way, and only for Append: a call that already has a Snapshot call
-    /// chained onto it answers yes here and is refused there. Both are right. An accept has nowhere
-    /// to put the literal it is carrying and says to re-run; a producer asking whether this call
-    /// site can host an inline snapshot has its answer, and taking the verification off inline
-    /// because another process got there first would be the wrong lesson to draw.
+    /// chained onto it holding other content answers yes here and is refused there. Both are right.
+    /// An accept has nowhere to put the literal it is carrying and says to re-run; a producer
+    /// asking whether this call site can host an inline snapshot has its answer, and taking the
+    /// verification off inline because another process got there first would be the wrong lesson
+    /// to draw. Where the chained call holds this same content there is nothing to tell apart and
+    /// both say yes, CanApply as AlreadyApplied.
     /// </para>
     /// </summary>
     public static InlineApplyResult CanAnchor(InlinePatch patch) =>

src/DiffEngine/Inline/InlinePatcher.cs

@@ -433,10 +433,22 @@ static PatchStatus TryAppend(
             return PatchStatus.Applied;
         }
 
-        var insertAt = WalkChain(source, scan, closeParen + 1, methodName, out var alreadyChained);
-        // Another process may have appended one between the run and the accept
-        if (alreadyChained)
+        var insertAt = WalkChain(source, scan, closeParen + 1, methodName, out var chained);
+        // Another process may have appended one between the run and the accept, and two
+        // frameworks failing the same call site is the ordinary way that happens: each queues an
+        // append, and accepting the first leaves the second with nowhere to put a literal that is
+        // already there. Only the content tells the two apart. The same snapshot is done, and
+        // saying so matters - a refusal reads as a failure, and the reader who sent two identical
+        // snapshots and got one applied and one rejected has no way to see that their source is
+        // already right. A different one is a call site that cannot say what it wants until it has
+        // been re-run against the literal it now has.
+        if (chained >= 0)
         {
+            if (HoldsContent(source, scan, chained, newContent))
+            {
+                return PatchStatus.AlreadyApplied;
+            }
+
             failReason = $"The call near line {lineHint} already has a {methodName} call. Re-run the test.";
             return PatchStatus.NotFound;
         }
@@ -454,6 +466,31 @@ static PatchStatus TryAppend(
         return PatchStatus.Applied;
     }
 
+    /// <summary>
+    /// Whether the call at <paramref name="openParen"/> already carries
+    /// <paramref name="content"/> as its expected argument.
+    /// <para>
+    /// What the argument means rather than what it says, so a literal the append would have
+    /// written in another shape - a different delimiter, a different indent - still counts as the
+    /// same snapshot. Anything that is not a literal at all, or is hidden behind another named
+    /// argument, is not this content: no answer can be read out of it, and the caller's other
+    /// branch says to re-run, which is where a call site nobody can make sense of belongs.
+    /// </para>
+    /// </summary>
+    static bool HoldsContent(string source, SourceScan scan, int openParen, string content)
+    {
+        if (!TryReadArguments(source, scan, openParen, out var expected) ||
+            expected.IsAbsent ||
+            expected.BlockedByName)
+        {
+            return false;
+        }
+
+        var argument = source.Substring(expected.Start, expected.End - expected.Start);
+        return scan.Language.TryParse(argument, out var value) &&
+               value == content;
+    }
+
     /// <summary>
     /// Removes the Snapshot call, along with the whitespace and line break that preceded it so no
     /// blank line is left behind.
@@ -533,11 +570,13 @@ static PatchStatus TryRemove(
     /// Walks the calls chained onto an invocation and returns where a call should be appended:
     /// the end of the chain, or the point in front of the language's
     /// <see cref="SourceLanguage.ChainTerminator"/> when the chain ends in one.
-    /// <paramref name="found"/> is set when one of them is a call to <paramref name="name"/>.
+    /// <paramref name="found"/> is the open paren of the first call to <paramref name="name"/>
+    /// among them, or -1 where there is none. The position rather than the fact of it, because a
+    /// caller deciding what to do about one has to read its argument.
     /// </summary>
-    static int WalkChain(string source, SourceScan scan, int index, string name, out bool found)
+    static int WalkChain(string source, SourceScan scan, int index, string name, out int found)
     {
-        found = false;
+        found = -1;
         var terminator = scan.Language.ChainTerminator;
         // Where the chain was before the terminating call, which is where an appended one goes:
         // in front of the terminator, and behind the whitespace and line break that introduced it
@@ -569,9 +608,10 @@ static int WalkChain(string source, SourceScan scan, int index, string name, out
                 break;
             }
 
-            if (IsCall(source, nameStart, cursor, name))
+            if (found < 0 &&
+                IsCall(source, nameStart, cursor, name))
             {
-                found = true;
+                found = paren;
             }
 
             if (terminator != null &&

src/Directory.Build.props

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project>
   <PropertyGroup>
-    <Version>20.1.1</Version>
+    <Version>20.1.2</Version>
     <AssemblyVersion>1.0.0</AssemblyVersion>
     <PackageTags>Testing, Snapshot, Diff, Compare</PackageTags>
     <Description>Launches diff tools based on file extensions. Designed to be consumed by snapshot testing libraries.</Description>