From 25dac3339163ffebe5de5c3da6f61710c34db0ce Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 10:03:29 +1000 Subject: [PATCH 1/2] Give C# the chain terminators it needs WalkChain appends a Snapshot call at the end of the chain unless the language names a terminating call to go in front of instead. F# named ToTask and said why: Snapshot returns the SettingsTask and ToTask does not, so appending after it appends to the wrong type. C# named nothing, and has the same shapes. A test that ends its chain by hand - GetAwaiter().GetResult(), ConfigureAwait(false), AsTask() - got the Snapshot appended after the terminator, which does not compile. Worse than not patching at all: the status came back Applied, so the snapshot is recorded as accepted while the file no longer builds. ChainTerminator becomes ChainTerminators, since C# has several, and C# names them. The first one in the chain wins, so GetAwaiter().GetResult() inserts ahead of GetAwaiter rather than between the two. --- src/DiffEngine.Tests/InlinePatcherTests.cs | 22 ++++++++++++++++++++++ src/DiffEngine/Inline/CsLanguage.cs | 16 ++++++++++++++++ src/DiffEngine/Inline/FsLanguage.cs | 2 +- src/DiffEngine/Inline/InlinePatcher.cs | 22 +++++++++++++++++----- src/DiffEngine/Inline/SourceLanguage.cs | 11 ++++++++--- 5 files changed, 64 insertions(+), 9 deletions(-) diff --git a/src/DiffEngine.Tests/InlinePatcherTests.cs b/src/DiffEngine.Tests/InlinePatcherTests.cs index 6c7db593..9a02e9b9 100644 --- a/src/DiffEngine.Tests/InlinePatcherTests.cs +++ b/src/DiffEngine.Tests/InlinePatcherTests.cs @@ -123,6 +123,28 @@ public async Task ASnapshotBeforeAVerbatimStringOpeningOnAnEscapedQuoteIsStillFo const string q3 = "\"\"\""; const string q4 = "\"\"\"\""; + /// + /// A chain the test ended by hand. Snapshot returns the SettingsTask and GetAwaiter does not, + /// so appending after the end of the chain produced source that does not compile - and + /// reported Applied while doing it, which leaves the snapshot recorded as accepted. + /// + [Test] + [Arguments("GetAwaiter().GetResult()")] + [Arguments("ConfigureAwait(false)")] + [Arguments("AsTask()")] + public async Task AppendGoesInFrontOfAChainTerminator(string tail) + { + var source = Method($" await Verify(x).{tail};"); + + var status = TryApply(source, 5, InlinePatchMode.Append, null, "new", out var newSource, out var reason); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(reason).IsEmpty(); + // In front of the terminator, so the chain the Snapshot is appended to is still a chain + await Assert.That(newSource).Contains("Snapshot("); + await Assert.That(newSource.IndexOf("Snapshot(", StringComparison.Ordinal)) + .IsLessThan(newSource.IndexOf(tail.Split('(')[0], StringComparison.Ordinal)); + } [Test] public async Task ReplaceRegularLiteral() { diff --git a/src/DiffEngine/Inline/CsLanguage.cs b/src/DiffEngine/Inline/CsLanguage.cs index f8756749..5450223f 100644 --- a/src/DiffEngine/Inline/CsLanguage.cs +++ b/src/DiffEngine/Inline/CsLanguage.cs @@ -19,6 +19,22 @@ public override bool TryParse(string expression, [NotNullWhen(true)] out string? internal override char NameSeparator => ':'; + /// + /// The calls a C# verify chain ends with when it stops being a verify chain: awaiting it by + /// hand, blocking on it, or converting it. Snapshot returns the SettingsTask and none of these + /// do, so an appended call goes in front of the first of them rather than after it - otherwise + /// the patch produces source that does not compile, which is worse than not patching at all + /// because the snapshot is reported as accepted. + /// + internal override string[] ChainTerminators => + [ + "GetAwaiter", + "GetResult", + "ConfigureAwait", + "AsTask", + "ToTask" + ]; + internal override bool IsIdentifierChar(char ch) => char.IsLetterOrDigit(ch) || ch == '_'; diff --git a/src/DiffEngine/Inline/FsLanguage.cs b/src/DiffEngine/Inline/FsLanguage.cs index 3f415493..8acd0c50 100644 --- a/src/DiffEngine/Inline/FsLanguage.cs +++ b/src/DiffEngine/Inline/FsLanguage.cs @@ -28,7 +28,7 @@ public override bool TryParse(string expression, [NotNullWhen(true)] out string? /// test ends the chain with ToTask. Snapshot returns the SettingsTask and ToTask does not, so /// an appended call goes in front of it rather than after it. /// - internal override string? ChainTerminator => "ToTask"; + internal override string[] ChainTerminators => ["ToTask"]; /// /// The F# compiler does not implement - it diff --git a/src/DiffEngine/Inline/InlinePatcher.cs b/src/DiffEngine/Inline/InlinePatcher.cs index e44cace5..7381d89c 100644 --- a/src/DiffEngine/Inline/InlinePatcher.cs +++ b/src/DiffEngine/Inline/InlinePatcher.cs @@ -451,13 +451,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 - /// when the chain ends in one. + /// when the chain ends in one. /// is set when one of them is a call to . /// static int WalkChain(string source, SourceScan scan, int index, string name, out bool found) { found = false; - var terminator = scan.Language.ChainTerminator; + var terminators = scan.Language.ChainTerminators; // 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 var beforeTerminator = -1; @@ -493,9 +493,8 @@ static int WalkChain(string source, SourceScan scan, int index, string name, out found = true; } - if (terminator != null && - beforeTerminator < 0 && - IsCall(source, nameStart, cursor, terminator)) + if (beforeTerminator < 0 && + IsTerminator(source, nameStart, cursor, terminators)) { beforeTerminator = index; } @@ -506,6 +505,19 @@ static int WalkChain(string source, SourceScan scan, int index, string name, out return beforeTerminator < 0 ? index : beforeTerminator; } + static bool IsTerminator(string source, int nameStart, int nameEnd, string[] terminators) + { + foreach (var terminator in terminators) + { + if (IsCall(source, nameStart, nameEnd, terminator)) + { + return true; + } + } + + return false; + } + static bool IsCall(string source, int nameStart, int nameEnd, string name) => nameEnd - nameStart == name.Length && string.CompareOrdinal(source, nameStart, name, 0, name.Length) == 0; diff --git a/src/DiffEngine/Inline/SourceLanguage.cs b/src/DiffEngine/Inline/SourceLanguage.cs index c2679d8c..45c66a44 100644 --- a/src/DiffEngine/Inline/SourceLanguage.cs +++ b/src/DiffEngine/Inline/SourceLanguage.cs @@ -86,10 +86,15 @@ public static SourceLanguage ForFile(string path) internal abstract char NameSeparator { get; } /// - /// A chained call that a Snapshot call has to be appended in front of rather than after, or - /// null when the end of the chain is always the insertion point. + /// Chained calls that a Snapshot call has to be appended in front of rather than after, or + /// empty when the end of the chain is always the insertion point. + /// + /// These are the calls that turn the verify chain into something that is no longer one - + /// awaiting it, blocking on it, converting it - so a Snapshot appended after one is appended + /// to the wrong type and the file stops compiling. + /// /// - internal virtual string? ChainTerminator => null; + internal virtual string[] ChainTerminators => []; /// /// Whether a patch from this language carries the source text of the expected argument, which From 55ed98f7b91edf77d9e67b56c05cf4580b67f7b3 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 12:31:09 +1000 Subject: [PATCH 2/2] Drop AsTask, and cite the guard the rest exist for AsTask is not a member of SettingsTask. I added it from the audit's suggestion without checking, and checking Verify's source says it never existed - so it was a name the patcher would only ever match on somebody's own extension method, and mis-position the insert if it did. The other three are real, public and [Pure] on SettingsTask, and the reason they matter is stronger than "the types do not line up". ToTask sets the task field, and CurrentSettings then throws "This SettingsTask instance has already been converted to a Task and can no longer be modified". So a Snapshot appended after a terminator fails at run time wherever it compiles - and where it does not compile, the patch still reported Applied and the snapshot was recorded as accepted. The test swaps its AsTask case for ToTask, which is the one C# shares with F#. --- src/DiffEngine.Tests/InlinePatcherTests.cs | 2 +- src/DiffEngine/Inline/CsLanguage.cs | 14 +++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/DiffEngine.Tests/InlinePatcherTests.cs b/src/DiffEngine.Tests/InlinePatcherTests.cs index 9fc4ea38..b3898492 100644 --- a/src/DiffEngine.Tests/InlinePatcherTests.cs +++ b/src/DiffEngine.Tests/InlinePatcherTests.cs @@ -131,7 +131,7 @@ public async Task ASnapshotBeforeAVerbatimStringOpeningOnAnEscapedQuoteIsStillFo [Test] [Arguments("GetAwaiter().GetResult()")] [Arguments("ConfigureAwait(false)")] - [Arguments("AsTask()")] + [Arguments("ToTask()")] public async Task AppendGoesInFrontOfAChainTerminator(string tail) { var source = Method($" await Verify(x).{tail};"); diff --git a/src/DiffEngine/Inline/CsLanguage.cs b/src/DiffEngine/Inline/CsLanguage.cs index 5450223f..0e745e2c 100644 --- a/src/DiffEngine/Inline/CsLanguage.cs +++ b/src/DiffEngine/Inline/CsLanguage.cs @@ -21,17 +21,21 @@ public override bool TryParse(string expression, [NotNullWhen(true)] out string? /// /// The calls a C# verify chain ends with when it stops being a verify chain: awaiting it by - /// hand, blocking on it, or converting it. Snapshot returns the SettingsTask and none of these - /// do, so an appended call goes in front of the first of them rather than after it - otherwise - /// the patch produces source that does not compile, which is worse than not patching at all - /// because the snapshot is reported as accepted. + /// hand, blocking on it, or converting it. Each is a real member of SettingsTask - checked + /// against Verify's source rather than guessed - and none of them returns one. + /// + /// So a Snapshot appended after any of them is not merely bad style. SettingsTask.ToTask sets + /// its task field, and CurrentSettings then throws "This SettingsTask instance has already + /// been converted to a Task and can no longer be modified" - so where such a patch compiles at + /// all it fails at run time, and where it does not compile the patch still reported Applied + /// and the snapshot was recorded as accepted. + /// /// internal override string[] ChainTerminators => [ "GetAwaiter", "GetResult", "ConfigureAwait", - "AsTask", "ToTask" ];