From c13e87ec70e85c6300988655aff266f610ba75b2 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Wed, 23 Sep 2026 16:43:43 +1000 Subject: [PATCH] Fix the library and inline items from the review - MemberLine prefers a declaration whose span holds the hint, the outermost where several do, over one that is merely nearer. A same-named test in the next nested type, or an F# local named like the test, put the floor past the hint. - NextMemberLine compares indentation as columns, with tab stops of four. A tab indented body under a space indented member ended the member at its first local. - F# escapes that are malformed or not F#'s (\d, \0, \12, \e, \x4, \u12) keep the backslash as text, as fsi does, rather than making the literal unreadable. A trigraph past 255 wraps into the byte range, as fsi does. - An F# regular literal is read as the snapshot it holds, with the layout convention applied as the test library applies it, and layout-shaped content that has to be written as a regular literal is wrapped in layout of its own. The patcher called such a literal already applied, so it was never updated. - WildcardFileFinder finds nothing, rather than throwing, under a root that does not exist: an undefined Program Files variable threw out of DiffTools' static constructor. - ViewerLaunchGate.LaunchAsync runs the launch on the pool and does not capture the caller's context while it holds the gate, so a sync Launch behind it on a single threaded context no longer deadlocks. - InlineApplier answers Failed where opening the mutex or the patcher throws, rather than unwinding a viewer's loop. - Staged file names cut the test name to fit 255 bytes, and on .NET Framework the whole path to MAX_PATH. Longer names were silently not staged. - A reply on the viewer port that is not the protocol marks the port unowned and says once which variable moves DiffEngine off it, rather than being taken for an owner that silently took nothing. --- claude.md | 7 +- .../FsCompilerRoundTripTests.cs | 5 + src/DiffEngine.Tests/FsStringLiteralTests.cs | 18 +- src/DiffEngine.Tests/InlineApplierTests.cs | 60 +++++++ src/DiffEngine.Tests/InlinePatcherFsTests.cs | 64 +++++++ src/DiffEngine.Tests/InlinePatcherTests.cs | 131 +++++++++++++++ src/DiffEngine.Tests/InlineStagingTests.cs | 36 ++++ .../ViewerClientUnownedTests.cs | 157 ++++++++++++++++++ src/DiffEngine.Tests/ViewerLaunchGateTests.cs | 112 +++++++++++++ .../WildcardFileFinderTests.cs | 39 +++++ src/DiffEngine/Inline/CsStringLiteral.cs | 2 +- src/DiffEngine/Inline/FsStringLiteral.cs | 60 +++++-- src/DiffEngine/Inline/InlineApplier.cs | 54 ++++-- src/DiffEngine/Inline/InlinePatcher.cs | 74 ++++++++- src/DiffEngine/Inline/InlineStaging.cs | 69 +++++++- src/DiffEngine/Inline/StringLiteral.cs | 49 +++++- src/DiffEngine/Protocol/ViewerClient.cs | 63 ++++++- src/DiffEngine/Viewer/ViewerLaunchGate.cs | 18 +- src/DiffEngine/WildcardFileFinder.cs | 33 +++- todo.md | 47 +----- 20 files changed, 994 insertions(+), 104 deletions(-) diff --git a/claude.md b/claude.md index fd3c2228..b87c5486 100644 --- a/claude.md +++ b/claude.md @@ -245,10 +245,13 @@ apart. quotes, braces and newlines, and the `inline` body carries an `InlinePatchFile` payload verbatim. - Compiles for every DiffEngine target, so the socket calls carry `#if` branches for the frameworks with no cancellation overloads. `ViewerProtocolTests` runs on all of them. -- `ViewerServer`'s accept loop awaits with `ConfigureAwait(false)`, the one place that matters +- `ViewerServer`'s accept loop awaits with `ConfigureAwait(false)`, one of two places that matter in a repo that otherwise leaves it off. The Windows viewer starts listening on its UI thread, and resuming there left every connection waiting on the render loop to pump. - `AnOwnerAnswersWhileTheThreadThatStartedItIsBusy` pins it. + `AnOwnerAnswersWhileTheThreadThatStartedItIsBusy` pins it. The other is + `ViewerLaunchGate.LaunchAsync`, which also runs the launch on the pool: a sync `Launch` blocks + its thread on the same gate, and on a single threaded context that is the thread the held + gate's continuations would need. **Native shim (`native/`), used by the Mac and Linux heads only:** - `raylib` and `imgui` are fetched by CMake (`FetchContent`), pinned by tag in diff --git a/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs b/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs index 0d89a9b5..a8f5d74b 100644 --- a/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs +++ b/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs @@ -43,6 +43,11 @@ public class FsCompilerRoundTripTests "\"starts with a quote\nsecond", "ends with a quote\nsecond\"", "has \"\"\" inside\nsecond", + // Content only a regular literal can hold that is also in the layout shape: a blank first + // line and nothing but indentation after the last newline. The reader strips a value in + // that shape whatever literal held it, so it has to be written wrapped in layout of its own + "\nx = \"\"\"\n", + "\n x = \"\"\"\n ", "(* not a comment *)\nsecond", "// not a comment\nsecond", "'ticked'\nsecond", diff --git a/src/DiffEngine.Tests/FsStringLiteralTests.cs b/src/DiffEngine.Tests/FsStringLiteralTests.cs index d48dced0..1c52f874 100644 --- a/src/DiffEngine.Tests/FsStringLiteralTests.cs +++ b/src/DiffEngine.Tests/FsStringLiteralTests.cs @@ -232,6 +232,19 @@ public async Task StripLayoutLeavesOtherValuesAlone(string value) => [Arguments("\"\"\"a\"b\"\"\"", "a\"b")] // Ordinary F# strings may span lines [Arguments("\"a\nb\"", "a\nb")] + // A backslash that starts no F# escape, or one cut short, is kept with what follows it, as + // fsi keeps it and with no warning: a trigraph is three digits, \x two hex digits, \u four, + // \U eight, and F# has no \0, \d or \e + [Arguments("\"\\d+\"", "\\d+")] + [Arguments("\"\\0\"", "\\0")] + [Arguments("\"\\12\"", "\\12")] + [Arguments("\"\\e\"", "\\e")] + [Arguments("\"\\x4\"", "\\x4")] + [Arguments("\"\\u12\"", "\\u12")] + [Arguments("\"\\U0041\"", "\\U0041")] + [Arguments("\"a\\qb\"", "a\\qb")] + // Past 255 a trigraph wraps into the byte range, which fsi does with warning FS1252 + [Arguments("\"\\999\"", "\u00e7")] public async Task Parse(string expression, string expected) { var parsed = FsStringLiteral.TryParse(expression, out var value); @@ -289,11 +302,6 @@ public async Task ParseMultiLineVerbatim() // A byte string is not a string [Arguments("\"bytes\"B")] [Arguments("@\"bytes\"B")] - // A trigraph is three digits or nothing - [Arguments("\"\\0\"")] - [Arguments("\"\\12\"")] - // Not an F# escape - [Arguments("\"\\e\"")] public async Task ParseRejects(string expression) { var parsed = FsStringLiteral.TryParse(expression, out _); diff --git a/src/DiffEngine.Tests/InlineApplierTests.cs b/src/DiffEngine.Tests/InlineApplierTests.cs index 95403e0c..c8b8332b 100644 --- a/src/DiffEngine.Tests/InlineApplierTests.cs +++ b/src/DiffEngine.Tests/InlineApplierTests.cs @@ -1,3 +1,7 @@ +#if NET +using System.Security.AccessControl; +#endif + public class InlineApplierTests { static string WriteTemp(byte[] bytes, string extension = ".cs") @@ -1246,4 +1250,60 @@ public async Task ABadTestNameBase64Fails() await Assert.That(read).IsFalse(); } + +#if NET + /// + /// The per file mutex already exists and this process may not open it, which is what a + /// non elevated applier meets while an elevated one (an IDE run as administrator, say) is + /// applying to the same file: an elevated token's default DACL grants Administrators and + /// SYSTEM, and a filtered token has Administrators as deny only. Modelled with an empty DACL. + /// + [Test] + [RunOn(TUnit.Core.Enums.OS.Windows)] + public async Task AMutexThisProcessCannotOpenFailsTheApplyRatherThanThrowing() + { + if (!OperatingSystem.IsWindows()) + { + return; + } + + var directory = Path.Combine(Path.GetTempPath(), $"InlineApplierTests_mutex_{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + try + { + var source = Path.Combine(directory, "Sample.cs"); + File.WriteAllText(source, "class C\n{\n void M() => Verify(\"old\");\n}\n"); + + var name = (string) typeof(InlineApplier) + .GetMethod("MutexName", BindingFlags.NonPublic | BindingFlags.Static)! + .Invoke(null, [Path.GetFullPath(source).ToLowerInvariant()])!; + var security = new MutexSecurity(); + security.SetSecurityDescriptorSddlForm("D:P"); + using var held = MutexAcl.Create(false, name, out var created, security); + await Assert.That(created).IsTrue(); + + InlineApplyResult? result = null; + Exception? thrown = null; + try + { + result = InlineApplier.Apply( + new(source, 3, "\"old\"", "new") + { + TestName = null + }); + } + catch (Exception exception) + { + thrown = exception; + } + + await Assert.That(thrown).IsNull(); + await Assert.That(result!.Status).IsEqualTo(InlineApplyStatus.Failed); + } + finally + { + Directory.Delete(directory, true); + } + } +#endif } diff --git a/src/DiffEngine.Tests/InlinePatcherFsTests.cs b/src/DiffEngine.Tests/InlinePatcherFsTests.cs index 36ceec2c..bfa5bcfc 100644 --- a/src/DiffEngine.Tests/InlinePatcherFsTests.cs +++ b/src/DiffEngine.Tests/InlinePatcherFsTests.cs @@ -852,4 +852,68 @@ let MyTest () = await Assert.That(newSource).Contains("#nowarn \"0044\""); await Assert.That(newSource).Contains("#if INTERACTIVE"); } + + /// + /// An F# local shadowing the test's own name, declared one line below the hint while the test + /// is declared two above it. The local becomes the member, so the call at the hint is out of + /// bounds; with the anchor matching nothing past it, the accept is a miss. + /// + [Test] + public async Task AnFsLocalNamedLikeTheTestDoesNotFloorTheSearchPastTheHint() + { + var source = Source( + """ + module Tests + + [] + let result () = + task { + do! Verifier.Verify(1).Snapshot("one").ToTask() + let result = 2 + do! Verifier.Verify(result).Snapshot("two").ToTask() + } + """); + + var status = TryApply(source, 6, InlinePatchMode.Set, null, "uno", out var newSource, out var reason, originalValue: "one", memberName: "result"); + + await Assert.That((status, reason)).IsEqualTo((PatchStatus.Applied, "")); + await Assert.That(newSource).Contains("Verify(1).Snapshot(\"uno\")"); + } + + /// + /// A regular literal whose value is in the layout shape, as a writer that did not wrap it left + /// it. The test library compares against the stripped value and fails, and sends that value as + /// the anchor. A patcher that read the literal without stripping found it equal to the new + /// content and reported AlreadyApplied - so the entry was dropped, the next run failed the + /// same way, and it went round again. + /// + [Test] + public async Task FsPatcherDoesNotCallALayoutShapedRegularLiteralAlreadyApplied() + { + const string content = "\nx = \"\"\"\n"; + var source = Source("module Tests\n\n[]\nlet MyTest () =\n Verifier.Verify(value).Snapshot(\"\\nx = \\\"\\\"\\\"\\n\").ToTask()\n"); + // What F# hands the test library for that literal is the content itself (fsi), and this is + // what the library compares against and sends as the anchor + var seen = SourceLanguage.FSharp.SnapshotValue(content); + + var status = TryApply(source, 5, InlinePatchMode.Set, null, content, out _, out _, originalValue: seen, memberName: "MyTest"); + + await Assert.That(status).IsNotEqualTo(PatchStatus.AlreadyApplied); + } + + /// + /// What that costs an accept: a hand-written regex snapshot can never be updated. The anchor + /// matches nothing because the literal does not parse, and the insert path then refuses the + /// call as not holding a string literal. + /// + [Test] + public async Task FsPatcherUpdatesALiteralHoldingAnUnknownEscape() + { + var source = Source("module Tests\n\n[]\nlet MyTest () =\n Verifier.Verify(value).Snapshot(\"\\d+\").ToTask()\n"); + + var status = TryApply(source, 5, InlinePatchMode.Set, null, "\\d+x", out var newSource, out var reason, originalValue: "\\d+", memberName: "MyTest"); + + await Assert.That((status, reason)).IsEqualTo((PatchStatus.Applied, "")); + await Assert.That(newSource).Contains("Snapshot(\"\\\\d+x\")"); + } } diff --git a/src/DiffEngine.Tests/InlinePatcherTests.cs b/src/DiffEngine.Tests/InlinePatcherTests.cs index bdb4df0a..32c12202 100644 --- a/src/DiffEngine.Tests/InlinePatcherTests.cs +++ b/src/DiffEngine.Tests/InlinePatcherTests.cs @@ -2199,4 +2199,135 @@ public async Task AnAnchorMatchingOnlyTheNextMemberIsStale() await Assert.That(status).IsEqualTo(PatchStatus.NotFound); await Assert.That(reason).Contains("Re-run the test."); } + + /// + /// Two nested types each declaring Works, the scenario-per-class layout. The hint is + /// A.Works's call on line 11. A.Works is declared six lines above it and B.Works five below, + /// so the nearest declaration is the wrong one, the floor lands past the hint, and the only + /// call left in reach is B's - which holds the same literal, so it is rewritten instead. + /// + [Test] + public async Task ASameNamedMemberInTheNextNestedTypeDoesNotTakeThePatch() + { + var source = Source( + """ + class Tests + { + public class A + { + public async Task Works() + { + var value = Build(); + value.Add(1); + value.Add(2); + value.Add(3); + await Verify(value).Snapshot("dup"); + } + } + public class B + { + public Task Works() => + Verify(other).Snapshot("dup"); + } + } + """); + + var status = TryApply(source, 11, InlinePatchMode.Set, "\"dup\"", "new", out var newSource, out var reason, memberName: "Works"); + + await Assert.That((status, reason)).IsEqualTo((PatchStatus.Applied, "")); + await Assert.That(newSource).Contains("await Verify(value).Snapshot(\"new\");"); + await Assert.That(newSource).Contains("Verify(other).Snapshot(\"dup\");"); + } + + /// + /// The same layout with different literals. Nothing in B's span matches the anchor, and A's + /// call is never looked at, so the accept is a miss - and the same miss on every re-run, since + /// the re-run sends the same hint and member. + /// + [Test] + public async Task ASameNamedMemberInTheNextNestedTypeDoesNotHideTheCall() + { + var source = Source( + """ + class Tests + { + public class A + { + public async Task Works() + { + var value = Build(); + value.Add(1); + value.Add(2); + value.Add(3); + await Verify(value).Snapshot("old"); + } + } + public class B + { + public Task Works() => + Verify(other).Snapshot("other"); + } + } + """); + + var status = TryApply(source, 11, InlinePatchMode.Set, "\"old\"", "new", out var newSource, out var reason, memberName: "Works"); + + await Assert.That((status, reason)).IsEqualTo((PatchStatus.Applied, "")); + await Assert.That(newSource).Contains("await Verify(value).Snapshot(\"new\");"); + } + + + // Built by hand, because the subject is tabs: a space indented member whose body is tabs + static string SpaceMemberTabBody(string body) => + "class Tests\n{\n public async Task Test()\n {\n" + body + "\n }\n}\n"; + + /// + /// "\t\t" is two characters and the member's four spaces are four, so the local at line 5 + /// reads as the next member and ends this one before the call at line 6. The call the hint + /// names is then outside the span, and the accept is a miss on every re-run. + /// + [Test] + public async Task ATabIndentedLocalDoesNotEndASpaceIndentedMember() + { + var source = SpaceMemberTabBody("\t\tvar value = Build();\n\t\tawait Verify(value).Snapshot(\"old\");"); + + var status = TryApply(source, 6, InlinePatchMode.Set, "\"old\"", "new", out var newSource, out var reason, memberName: "Test"); + + await Assert.That((status, reason)).IsEqualTo((PatchStatus.Applied, "")); + await Assert.That(newSource).Contains("await Verify(value).Snapshot(\"new\");"); + } + + /// + /// The same cut with a sibling above it holding the same literal, which is ordinary for a + /// member verifying two values that serialise alike. The hint names line 7, the truncated span + /// ends at line 6, and the sibling on line 5 is the only match left, so it is rewritten. + /// + [Test] + public async Task ATabIndentedLocalDoesNotSendThePatchToTheSiblingAboveIt() + { + var source = SpaceMemberTabBody( + "\t\tawait Verify(a).Snapshot(\"dup\");\n\t\tvar b = Build();\n\t\tawait Verify(b).Snapshot(\"dup\");"); + + var status = TryApply(source, 7, InlinePatchMode.Set, "\"dup\"", "new", out var newSource, out var reason, memberName: "Test"); + + await Assert.That((status, reason)).IsEqualTo((PatchStatus.Applied, "")); + await Assert.That(newSource).Contains("Verify(a).Snapshot(\"dup\")"); + await Assert.That(newSource).Contains("Verify(b).Snapshot(\"new\")"); + } + + /// + /// Control: the same source with the body indented by eight spaces instead of two tabs. + /// + [Test] + public async Task ControlSpaceIndentedLocalLeavesTheSiblingAlone() + { + var source = SpaceMemberTabBody( + " await Verify(a).Snapshot(\"dup\");\n var b = Build();\n await Verify(b).Snapshot(\"dup\");"); + + var status = TryApply(source, 7, InlinePatchMode.Set, "\"dup\"", "new", out var newSource, out var reason, memberName: "Test"); + + await Assert.That((status, reason)).IsEqualTo((PatchStatus.Applied, "")); + await Assert.That(newSource).Contains("Verify(a).Snapshot(\"dup\")"); + await Assert.That(newSource).Contains("Verify(b).Snapshot(\"new\")"); + } } diff --git a/src/DiffEngine.Tests/InlineStagingTests.cs b/src/DiffEngine.Tests/InlineStagingTests.cs index f9313658..222bf2ec 100644 --- a/src/DiffEngine.Tests/InlineStagingTests.cs +++ b/src/DiffEngine.Tests/InlineStagingTests.cs @@ -347,4 +347,40 @@ public void Dispose() } } } + + /// + /// A 242 character test name makes a 272 character received file name, past the 255 every + /// file system here allows per component. The write throws IOException, TryPersist catches it, + /// and the entry is reported as not written with nothing said anywhere. + /// + [Test] + public async Task ALongTestNameIsStillPersisted() + { + var directory = Path.Combine(Path.GetTempPath(), $"InlineStagingTests_long_{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + try + { + File.WriteAllText(Path.Combine(directory, "Sample.csproj"), ""); + var source = Path.Combine(directory, "SampleTests.cs"); + File.WriteAllText(source, "// sample"); + + var patch = new InlinePatch(source, 42, "\"old\"", "new") + { + TestName = $"SampleTests.{new string('a', 230)}", + OriginalValue = "old", + Framework = "net10.0" + }; + + var written = InlineStaging.Persist([new(patch)]); + + var staging = Path.Combine(directory, "obj", InlineStaging.DirectoryName); + var files = Directory.Exists(staging) ? Directory.GetFiles(staging).Length : 0; + await Assert.That(written).IsEqualTo(1); + await Assert.That(files).IsEqualTo(3); + } + finally + { + Directory.Delete(directory, true); + } + } } diff --git a/src/DiffEngine.Tests/ViewerClientUnownedTests.cs b/src/DiffEngine.Tests/ViewerClientUnownedTests.cs index de65a449..32bff60d 100644 --- a/src/DiffEngine.Tests/ViewerClientUnownedTests.cs +++ b/src/DiffEngine.Tests/ViewerClientUnownedTests.cs @@ -234,4 +234,161 @@ public void Dispose() cancel.Dispose(); } } + + /// + /// Network UPS Tools' upsd holds 3493, which IANA assigns it. It answers every line it does + /// not understand with an error and closes once the client has finished sending, which is + /// what this does. The composition is AddInlineAsync's, with the port made explicit and the + /// launch observed rather than performed. + /// + [Test] + public async Task ANonViewerOnThePortIsReportedRatherThanTakenForAnOwner() + { + ViewerClient.ForgetUnowned(); + using var upsd = new FakeUpsd(); + var port = upsd.Port; + var trace = new CapturingListener(); + Trace.Listeners.Add(trace); + try + { + var settle = new ViewerMessage(ViewerVerb.Settle, InlineKey.For("Tests.cs", 1)); + for (var index = 0; index < 5; index++) + { + ViewerClient.TrySend(settle, out _, port, skipIfUnowned: true); + } + + var settleConnections = upsd.Connections; + + var patch = new InlinePatch(Path.Combine(Path.GetTempPath(), "ViewerClientNoProject", "Tests.cs"), 1, "\"old\"", "new") + { + TestName = "Tests.Method" + }; + var payload = InlinePatchFile.Build(patch, "net10.0"); + var inline = new ViewerMessage(ViewerVerb.Inline, Body: payload); + var sent = await ViewerClient.SendAsync(inline, Cancel.None, port, skipIfUnowned: true); + var launches = 0; + var gated = ViewerLaunchGate.LaunchAsync( + async () => await ViewerClient.SendAsync(inline, Cancel.None, port) == SendOutcome.Accepted, + () => + { + launches++; + return Task.FromResult(true); + }, + Cancel.None, + isOwned: () => ViewerClient.IsOwned(port)); + var first = await Task.WhenAny(gated, Task.Delay(TimeSpan.FromSeconds(30))); + await Assert.That(first == gated).IsTrue(); + var outcome = await gated; + + Console.WriteLine($"settle connections: {settleConnections}, inline send: {sent}, gate: {outcome}, launches: {launches}, result: {DiffRunner.InlineResultFor(outcome)}"); + + using (Assert.Multiple()) + { + // Either a hint that names the variable to move off the port... + await Assert.That(trace.Text).Contains(ViewerClient.PortVariable); + // ...or at least not paying a connect per settle for a port with no viewer on it + await Assert.That(settleConnections).IsEqualTo(1); + } + } + finally + { + Trace.Listeners.Remove(trace); + ViewerClient.ForgetUnowned(); + } + } + + sealed class CapturingListener : TraceListener + { + readonly StringBuilder builder = new(); + + public string Text + { + get + { + lock (builder) + { + return builder.ToString(); + } + } + } + + public override void Write(string? message) + { + lock (builder) + { + builder.Append(message); + } + } + + public override void WriteLine(string? message) + { + lock (builder) + { + builder.AppendLine(message); + } + } + } + + /// + /// What upsd does with a client that is not speaking its protocol: one error per line, and the + /// connection closed at end of input. One connection at a time, as its select loop is. + /// + sealed class FakeUpsd : IDisposable + { + readonly TcpListener listener = new(IPAddress.Loopback, 0); + int connections; + + public FakeUpsd() + { + listener.Start(); + new Thread(Serve) + { + IsBackground = true + }.Start(); + } + + public int Port => ((IPEndPoint) listener.LocalEndpoint).Port; + + public int Connections => Volatile.Read(ref connections); + + void Serve() + { + while (true) + { + TcpClient client; + try + { + client = listener.AcceptTcpClient(); + } + catch (Exception exception) + when (exception is SocketException or ObjectDisposedException or InvalidOperationException) + { + return; + } + + Interlocked.Increment(ref connections); + using (client) + { + try + { + var stream = client.GetStream(); + using var reader = new StreamReader(stream, Encoding.ASCII); + using var writer = new StreamWriter(stream, Encoding.ASCII); + writer.NewLine = "\n"; + writer.AutoFlush = true; + while (reader.ReadLine() != null) + { + writer.WriteLine("ERR UNKNOWN-COMMAND"); + } + } + catch (IOException) + { + } + } + } + } + + public void Dispose() => + listener.Stop(); + } } diff --git a/src/DiffEngine.Tests/ViewerLaunchGateTests.cs b/src/DiffEngine.Tests/ViewerLaunchGateTests.cs index ffc02490..8d109119 100644 --- a/src/DiffEngine.Tests/ViewerLaunchGateTests.cs +++ b/src/DiffEngine.Tests/ViewerLaunchGateTests.cs @@ -300,4 +300,116 @@ public bool Start() public bool IsUp() => elapsed.Elapsed.Ticks >= Interlocked.Read(ref upAt); } + + /// + /// DiffRunner.AddDeleteAsync's shape and then DiffRunner.AddDelete's, on the only thread a + /// single threaded context has, which is where xUnit v2 puts a second test once the first is + /// awaiting. The async launch holds the gate across WaitForBindAsync, whose delay resumes on + /// the captured context; the sync one blocks that context's thread waiting for the gate. + /// + [Test] + public Task SyncLaunchBehindAnAsyncDeleteOnTheSameContextFinishes() => + SyncBehindAsync(() => Task.FromResult(true)); + + /// + /// AddInlineAsync's shape: the launch is ViewerLauncher.LaunchAsync, whose stdin write and + /// flush are awaited on whatever context the caller had. So ConfigureAwait(false) inside the + /// gate alone does not help it: the gate is held until the launch task completes, and that + /// needs the context too. + /// + [Test] + public Task SyncLaunchBehindAnAsyncInlineOnTheSameContextFinishes() => + SyncBehindAsync(async () => + { + await Task.Delay(10); + return true; + }); + + static async Task SyncBehindAsync(Func> launch) + { + var previous = ViewerLaunchGate.BindWait; + ViewerLaunchGate.BindWait = TimeSpan.FromMilliseconds(300); + var context = new SingleThreadContext(); + Task? asyncLaunch = null; + ViewerLaunchOutcome? syncOutcome = null; + var thread = new Thread(() => + { + SynchronizationContext.SetSynchronizationContext(context); + asyncLaunch = ViewerLaunchGate.LaunchAsync( + retry: () => Task.FromResult(true), + launch, + Cancel.None, + isOwned: () => false, + canLaunch: () => true); + syncOutcome = ViewerLaunchGate.Launch( + retry: () => true, + launch: () => true, + isOwned: () => false, + canLaunch: () => true); + }) + { + IsBackground = true + }; + + bool finished; + try + { + thread.Start(); + // Ten bind waits, which is ten times what the two launches need between them + finished = thread.Join(TimeSpan.FromSeconds(3)); + } + finally + { + // Run what the context queued, from a thread that is not blocked, so the static gate is + // not left held for whatever runs in this process next + var pump = new Thread(() => context.Pump(() => !thread.IsAlive, TimeSpan.FromSeconds(10))) + { + IsBackground = true + }; + pump.Start(); + pump.Join(); + ViewerLaunchGate.BindWait = previous; + } + + await Assert.That(finished).IsTrue(); + await Assert.That(syncOutcome).IsEqualTo(ViewerLaunchOutcome.Launched); + await Assert.That(asyncLaunch!.IsCompleted).IsTrue(); + } + + /// + /// One thread, and a queue of what was posted to it. What xUnit v2's MaxConcurrencySyncContext + /// is with a single worker, and what a WinForms or WPF thread is. + /// + sealed class SingleThreadContext : SynchronizationContext + { + readonly BlockingCollection<(SendOrPostCallback Callback, object? State)> queue = new(); + + public override void Post(SendOrPostCallback d, object? state) => + queue.Add((d, state)); + + public override void Send(SendOrPostCallback d, object? state) => + throw new NotSupportedException(); + + public void Pump(Func done, TimeSpan timeout) + { + var previous = Current; + SetSynchronizationContext(this); + try + { + var elapsed = Stopwatch.StartNew(); + while (!done() && + elapsed.Elapsed < timeout) + { + if (queue.TryTake(out var item, TimeSpan.FromMilliseconds(20))) + { + item.Callback(item.State); + } + } + } + finally + { + SetSynchronizationContext(previous); + } + } + } } diff --git a/src/DiffEngine.Tests/WildcardFileFinderTests.cs b/src/DiffEngine.Tests/WildcardFileFinderTests.cs index bef21ec5..e78b7b60 100644 --- a/src/DiffEngine.Tests/WildcardFileFinderTests.cs +++ b/src/DiffEngine.Tests/WildcardFileFinderTests.cs @@ -75,4 +75,43 @@ public async Task WildCardInDir_missing() await Assert.That(WildcardFileFinder.TryFind(path, out var result)).IsFalse(); await Assert.That(result).IsNull(); } + + /// + /// ExamDiff's search directory, %ProgramFiles%\ExamDiff Pro*\, as ExpandProgramFiles + /// rewrites it for %ProgramW6432% and %ProgramFiles(x86)%. A variable nothing + /// defines stands in for one of those on a machine that lacks it: ExpandEnvironmentVariables + /// leaves it as written, and the wildcard segment right after it is then enumerated under a + /// relative directory that does not exist. + /// + [Test] + public async Task AnUndefinedVariableBeforeAWildcardIsNotFoundRatherThanThrown() + { + var path = Path.Combine("%DiffEngine_TestUndefined%", "ExamDiff Pro*", "ExamDiff.exe"); + + await Assert.That(WildcardFileFinder.TryFind(path, out var result)).IsFalse(); + await Assert.That(result).IsNull(); + } + + /// + /// The same thing one level up, through the call DiffTools' static constructor makes for every + /// definition. Thrown there, it is a TypeInitializationException for every later use of + /// DiffTools in the process. + /// + [Test] + [RunOn(TUnit.Core.Enums.OS.Windows)] + public async Task ResolvingATwoLevelWildcardUnderAnUndefinedVariableIsNotFound() + { + var windows = Definitions.Tools + .Single(_ => _.Tool == DiffTool.ExamDiff) + .OsSupport + .Windows!; + var support = new OsSupport( + Windows: windows with + { + SearchDirectories = [@"%DiffEngine_TestUndefined%\ExamDiff Pro*\"] + }); + + await Assert.That(OsSettingsResolver.Resolve("UndefinedExamDiff", support, out var path, out _)).IsFalse(); + await Assert.That(path).IsNull(); + } } diff --git a/src/DiffEngine/Inline/CsStringLiteral.cs b/src/DiffEngine/Inline/CsStringLiteral.cs index 64eb56d6..b9f3c9b6 100644 --- a/src/DiffEngine/Inline/CsStringLiteral.cs +++ b/src/DiffEngine/Inline/CsStringLiteral.cs @@ -122,7 +122,7 @@ static bool TryScanLiteral(string text, int start, out string? value, out int en // A regular literal cannot span lines static bool TryScanRegular(string text, int start, out string? value, out int end) => - StringLiteral.TryScanRegular(text, start, true, TryEscape, out value, out end); + StringLiteral.TryScanRegular(text, start, true, false, TryEscape, out value, out end); /// /// The escapes C# has that F# does not, plus \x, which both have and size differently: diff --git a/src/DiffEngine/Inline/FsStringLiteral.cs b/src/DiffEngine/Inline/FsStringLiteral.cs index d2fc4666..1e873a77 100644 --- a/src/DiffEngine/Inline/FsStringLiteral.cs +++ b/src/DiffEngine/Inline/FsStringLiteral.cs @@ -42,12 +42,34 @@ public static string Render(string content, string indent, string eol) // delimiter past the way C# can (FS1232), or a line terminator, which no delimiter // helps with. A regular literal on one source line always works, whatever it costs in // escapes - return StringLiteral.RenderRegular(SourceLanguage.NormalizeNewlines(content)); + return StringLiteral.RenderRegular(Wrapped(SourceLanguage.NormalizeNewlines(content))); } return StringLiteral.RenderMultiLine(content, indent, eol, "\"\"\""); } + /// + /// Content for a regular literal, wrapped in layout when it would otherwise read as layout. + /// + /// The reader's half of the convention sees values, not literals, so it strips a value in the + /// layout shape whichever kind of literal held it. Content in that shape written as it is - + /// starting with a blank line and ending with a newline, say - was read back as something + /// else, and the patcher, reading the literal as it stood, called the next accept already + /// applied: the queue dropped it and the next run failed the same way. A blank line either + /// side is layout that strips to exactly this content, since the closing indentation is + /// empty and every line starts with it. + /// + /// + static string Wrapped(string content) + { + if (StringLiteral.TryStripLayout(content, out _)) + { + return $"\n{content}\n"; + } + + return content; + } + /// /// Whether a triple-quoted literal can hold this content. A quote at either end would sit /// against the delimiter and be read as part of it, and a run of three anywhere would close @@ -124,7 +146,7 @@ static bool TryScanLiteral(string text, int start, out string? value, out int en { // There is no verbatim triple-quoted form, so a run of quotes after @" is an escaped // quote and the rest of the string, not a delimiter - return StringLiteral.TryScanVerbatim(text, index + 1, out value, out end); + return Snapshot(StringLiteral.TryScanVerbatim(text, index + 1, out value, out end), ref value); } var quotes = StringLiteral.QuoteRunLength(text, index); @@ -143,17 +165,36 @@ static bool TryScanLiteral(string text, int start, out string? value, out int en return true; } - return TryScanRegular(text, index + 1, out value, out end); + return Snapshot(TryScanRegular(text, index + 1, out value, out end), ref value); + } + + /// + /// A regular or verbatim literal's value as the snapshot it holds, which is the value with + /// applied: what a test library's + /// does with it, having no way to tell what kind of literal it came from. Read without it, the + /// patcher and the test library disagreed about any value in the layout shape, and an accept + /// the library still failed on was reported as already applied. The triple-quoted scan strips + /// as it reads, so it does not come through here. + /// + static bool Snapshot(bool scanned, ref string? value) + { + if (scanned) + { + value = StripLayout(value!); + } + + return scanned; } // An ordinary F# string may span lines, so a newline in one is content static bool TryScanRegular(string text, int start, out string? value, out int end) => - StringLiteral.TryScanRegular(text, start, false, TryEscape, out value, out end); + StringLiteral.TryScanRegular(text, start, false, true, TryEscape, out value, out end); /// /// What F# spells its own way: \x is exactly two hex digits, a backslash before a line /// break continues the string, and a backslash before three digits is a trigraph. F# has no - /// \0 or \e, so those fall through to being no escape at all. + /// \0 or \e, so those are no escape at all, and the scanner keeps the backslash + /// as text, as F# does. /// static bool TryEscape(string text, ref int index, char escape, StringBuilder builder) { @@ -213,12 +254,9 @@ static bool TryReadTrigraph(string text, ref int index, char first, out char res var value = (first - '0') * 100 + (text[index] - '0') * 10 + (text[index + 1] - '0'); index += 2; - if (value > 255) - { - return false; - } - - result = (char) value; + // Past 255 F# wraps into the byte range, with warning FS1252 saying a later version will + // make it an error. What it compiles to today is what the literal holds. + result = (char) (value % 256); return true; } } diff --git a/src/DiffEngine/Inline/InlineApplier.cs b/src/DiffEngine/Inline/InlineApplier.cs index 6ecda9f9..8701c6c7 100644 --- a/src/DiffEngine/Inline/InlineApplier.cs +++ b/src/DiffEngine/Inline/InlineApplier.cs @@ -81,7 +81,21 @@ static InlineApplyResult Run(InlinePatch patch, bool write, bool anchorOnly = fa var normalizedPath = fullPath.ToLowerInvariant(); lock (gates.GetOrAdd(normalizedPath, static _ => new())) { - using var mutex = OpenMutex(MutexName(normalizedPath)); + // Answered rather than thrown, as everything else here is. A mutex this process may + // not open - one an elevated applier created for the same file - threw out of Apply, + // and a viewer's single or group accept let that unwind its loop: the queue was staged + // and the window vanished mid review. + Mutex opened; + try + { + opened = OpenMutex(MutexName(normalizedPath)); + } + catch (Exception exception) + { + return InlineApplyResult.Failed($"Could not open the inline patch mutex for: {fullPath}", exception); + } + + using var mutex = opened; var owned = false; try { @@ -149,19 +163,31 @@ static InlineApplyResult LockedApply(string fullPath, InlinePatch patch, string return InlineApplyResult.Failed($"Failed to decode: {fullPath}", exception); } - var status = InlinePatcher.TryApply( - SourceLanguage.ForFile(fullPath), - source, - patch.LineHint, - patch.Mode, - patch.OriginalExpression, - patch.OriginalValue, - patch.MemberName, - patch.EntryPoints, - anchorOnly, - newContent, - out var newSource, - out var failReason); + PatchStatus status; + string newSource; + string failReason; + try + { + status = InlinePatcher.TryApply( + SourceLanguage.ForFile(fullPath), + source, + patch.LineHint, + patch.Mode, + patch.OriginalExpression, + patch.OriginalValue, + patch.MemberName, + patch.EntryPoints, + anchorOnly, + newContent, + out newSource, + out failReason); + } + catch (Exception exception) + { + // A patcher defect on some shape of source, reported against the file it met it in + // rather than thrown at whichever surface was accepting + return InlineApplyResult.Failed($"Failed to patch: {fullPath}", exception); + } switch (status) { diff --git a/src/DiffEngine/Inline/InlinePatcher.cs b/src/DiffEngine/Inline/InlinePatcher.cs index d2ae4ade..59984087 100644 --- a/src/DiffEngine/Inline/InlinePatcher.cs +++ b/src/DiffEngine/Inline/InlinePatcher.cs @@ -756,6 +756,29 @@ static string LeadingWhitespace(string source, List lineStarts, int offset) return source.Substring(lineStart, index - lineStart); } + /// + /// The column indentation reaches, with a tab advancing to the next multiple of four. Four + /// rather than eight because that is what the tab indented C# this has to measure is written + /// with. F# rejects tabs outright (FS1161), so only C# ever gets here with one. + /// + static int IndentWidth(string whitespace) + { + var width = 0; + foreach (var character in whitespace) + { + if (character == '\t') + { + width += 4 - width % 4; + } + else + { + width++; + } + } + + return width; + } + static readonly string[] snapshotName = [methodName]; /// @@ -955,6 +978,12 @@ static bool TryFindCall( /// for the ordinary shape of a test: a local, then a verify call on it. A sibling shares the /// member's own indentation, so the comparison is inclusive. /// + /// + /// Compared as columns () rather than characters. Two tabs are two + /// characters and four spaces are four, so a tab indented body under a space indented member + /// read as a sibling and ended the member at its first local: the call the hint named fell + /// outside the span, and a same literal call above it was patched instead. + /// /// static int NextMemberLine(string source, SourceScan scan, List lineStarts, int memberLine) { @@ -964,7 +993,7 @@ static int NextMemberLine(string source, SourceScan scan, List lineStarts, return lineCount + 1; } - var memberIndent = LeadingWhitespace(source, lineStarts, lineStarts[memberLine - 1]).Length; + var memberIndent = IndentWidth(LeadingWhitespace(source, lineStarts, lineStarts[memberLine - 1])); var start = lineStarts[memberLine]; var end = source.Length; for (var index = start; index < end; index++) @@ -977,7 +1006,7 @@ static int NextMemberLine(string source, SourceScan scan, List lineStarts, } if (scan.IsDeclaration(DeclarationStart(source, index)) && - LeadingWhitespace(source, lineStarts, index).Length <= memberIndent) + IndentWidth(LeadingWhitespace(source, lineStarts, index)) <= memberIndent) { return LineOf(lineStarts, index); } @@ -1077,7 +1106,12 @@ static int Clamp(int line, int lineCount) => /// . /// /// - /// The declaration nearest the hint wins, so overloads and partials pick the plausible one. + /// A declaration whose span holds the hint wins over one that is merely nearer: in the + /// scenario per class layout two nested types each declare the same test, and the one below + /// the hint can be the nearer, which put the floor past the hint and handed the patch to the + /// other type's call. Where several spans hold it - an F# local named like the test, inside + /// the test - the outermost is the member. Where none does, the nearest wins, so overloads and + /// partials still pick the plausible one. /// /// static int? MemberLine(string source, SourceScan scan, List lineStarts, int lineHint, string? memberName) @@ -1087,7 +1121,9 @@ static int Clamp(int line, int lineCount) => return null; } - var best = -1; + var nearest = -1; + var holding = -1; + var holdingIndent = int.MaxValue; var index = 0; while (true) { @@ -1106,17 +1142,39 @@ static int Clamp(int line, int lineCount) => scan.IsDeclaration(DeclarationStart(source, index))) { var line = LineOf(lineStarts, index); - if (best < 0 || - Math.Abs(line - lineHint) < Math.Abs(best - lineHint)) + if (nearest < 0 || + Math.Abs(line - lineHint) < Math.Abs(nearest - lineHint)) { - best = line; + nearest = line; + } + + if (line <= lineHint && + lineHint < NextMemberLine(source, scan, lineStarts, line)) + { + var indent = IndentWidth(LeadingWhitespace(source, lineStarts, index)); + if (indent < holdingIndent || + (indent == holdingIndent && line > holding)) + { + holding = line; + holdingIndent = indent; + } } } index = end; } - return best < 0 ? null : best; + if (holding >= 0) + { + return holding; + } + + if (nearest >= 0) + { + return nearest; + } + + return null; } /// diff --git a/src/DiffEngine/Inline/InlineStaging.cs b/src/DiffEngine/Inline/InlineStaging.cs index a4acdb6e..bfaf9d83 100644 --- a/src/DiffEngine/Inline/InlineStaging.cs +++ b/src/DiffEngine/Inline/InlineStaging.cs @@ -387,7 +387,7 @@ static bool TryPersist(InlinePatch patch, IReadOnlyList origins) // merged into carries both labels while the patch keeps its birth framework, and the // label is what a reader shows. var origin = origins.Count > 0 ? origins[0] : patch.Framework; - var baseName = BuildName(patch, origin); + var baseName = BuildName(patch, origin, directory); var encoding = new UTF8Encoding(false); File.WriteAllText( @@ -419,12 +419,75 @@ or ArgumentException /// overwrites, and the framework last, which is the segment conflict labels are read from. /// The framework's dots become underscores for the same reason Verify writes DotNet10_0 /// rather than a versioned moniker: the last dot has to be the one before it. + /// + /// The test name is cut to fit. A file name component is 255 characters on NTFS and 255 bytes + /// on ext4 and APFS, and a long F# sentence name with the rest of this added went past it: the + /// write threw, took that for a file it could not write, and the + /// snapshot was staged nowhere. Cut by UTF-8 bytes, which is the tighter of the two limits for + /// the same name. The call site hash keeps two names that are cut to the same prefix apart. + /// + /// + /// On .NET Framework the whole path is cut to fit as well. It holds a path to MAX_PATH unless + /// the machine has opted in to long paths, which most have not, and a name that fits its + /// component can still take the path past 260 characters under a deep project. + /// /// - static string BuildName(InlinePatch patch, string? origin) + static string BuildName(InlinePatch patch, string? origin, string directory) { var test = Sanitize(patch.TestName) ?? Path.GetFileNameWithoutExtension(patch.SourceFile); var runtime = Sanitize(origin)?.Replace('.', '_') ?? "unknown"; - return $"{test}.{Hash($"{patch.SourceFile}:{patch.LineHint}")}.{runtime}"; + var rest = $".{Hash($"{patch.SourceFile}:{patch.LineHint}")}.{runtime}"; + var bytes = maxComponentBytes - Encoding.UTF8.GetByteCount(rest + longestExtension); +#if NETFRAMEWORK + // One for the separator the name is joined to the directory with + var characters = maxFrameworkPath - directory.Length - 1 - rest.Length - longestExtension.Length; +#else + var characters = int.MaxValue; +#endif + return Truncate(test, bytes, characters) + rest; + } + + const int maxComponentBytes = 255; + +#if NETFRAMEWORK + /// + /// MAX_PATH less the terminating null. + /// + const int maxFrameworkPath = 259; +#endif + + /// + /// The longest of the three extensions a staged entry is written with. + /// + const string longestExtension = ".received.txt"; + + /// + /// The longest prefix of that is at most in + /// UTF-8 and long, never splitting a surrogate pair. + /// + static string Truncate(string value, int bytes, int characters) + { + var used = 0; + var index = 0; + while (index < value.Length) + { + var length = char.IsHighSurrogate(value[index]) && + index + 1 < value.Length && + char.IsLowSurrogate(value[index + 1]) + ? 2 + : 1; + var size = Encoding.UTF8.GetByteCount(value.ToCharArray(index, length)); + if (used + size > bytes || + index + length > characters) + { + break; + } + + used += size; + index += length; + } + + return value.Substring(0, index); } /// diff --git a/src/DiffEngine/Inline/StringLiteral.cs b/src/DiffEngine/Inline/StringLiteral.cs index b3fbc4cf..574e5cd3 100644 --- a/src/DiffEngine/Inline/StringLiteral.cs +++ b/src/DiffEngine/Inline/StringLiteral.cs @@ -354,7 +354,7 @@ public static bool TryParse(string expression, ScanLiteral scan, [NotNullWhen(tr /// An escape only one of the languages has, or has its own rule for. Called with /// just past the escape character, and free to move it: F#'s /// trigraph and line continuation both read further. False means the escape is not one this - /// language knows, which makes the literal unreadable. + /// language knows: an unreadable literal in C#, and in F# a backslash kept as text. /// public delegate bool TryLanguageEscape(string text, ref int index, char escape, StringBuilder builder); @@ -364,13 +364,23 @@ public static bool TryParse(string expression, ScanLiteral scan, [NotNullWhen(tr /// Written once because it was written twice, and the copies had to be kept in step by hand - /// the guard against a lone surrogate in \U was a fix that had to be made in both, and /// could as easily have been made in one. What genuinely differs is passed in: whether a - /// newline ends the literal (C# yes, F# no), and the escapes that are one language's own. + /// newline ends the literal (C# yes, F# no), the escapes that are one language's own, and what + /// a malformed escape means. + /// + /// + /// is F#'s rule, checked against fsi: a backslash + /// that starts no escape it defines, or one cut short (\x4, \u12, \12), is + /// kept as a backslash, and what follows it is read as ordinary text. "\d+" is the + /// three characters it looks like, with no warning. C# rejects the same literals at compile + /// time, so for C# a malformed escape is still an unreadable literal. A code point that is + /// well formed and out of range is unreadable in both. /// /// public static bool TryScanRegular( string text, int start, bool newlineEndsLiteral, + bool malformedEscapeIsText, TryLanguageEscape tryLanguageEscape, out string? value, out int end) @@ -408,6 +418,7 @@ public static bool TryScanRegular( return false; } + var escapeAt = index; var escape = text[index]; index++; switch (escape) @@ -445,14 +456,32 @@ public static bool TryScanRegular( case 'u': if (!TryReadHex(text, ref index, 4, 4, out var utf16)) { - return false; + if (!malformedEscapeIsText) + { + return false; + } + + builder.Append('\\'); + index = escapeAt; + continue; } builder.Append((char) utf16); continue; case 'U': - if (!TryReadHex(text, ref index, 8, 8, out var codePoint) || - !IsScalarValue(codePoint)) + if (!TryReadHex(text, ref index, 8, 8, out var codePoint)) + { + if (!malformedEscapeIsText) + { + return false; + } + + builder.Append('\\'); + index = escapeAt; + continue; + } + + if (!IsScalarValue(codePoint)) { return false; } @@ -461,9 +490,17 @@ public static bool TryScanRegular( continue; } + var length = builder.Length; if (!tryLanguageEscape(text, ref index, escape, builder)) { - return false; + if (!malformedEscapeIsText) + { + return false; + } + + builder.Length = length; + builder.Append('\\'); + index = escapeAt; } } diff --git a/src/DiffEngine/Protocol/ViewerClient.cs b/src/DiffEngine/Protocol/ViewerClient.cs index 2b3b68a0..9d1fe63e 100644 --- a/src/DiffEngine/Protocol/ViewerClient.cs +++ b/src/DiffEngine/Protocol/ViewerClient.cs @@ -144,8 +144,59 @@ static void Found(int port, bool owned) /// /// For tests, which share this process and its memory with every other test's ports. /// - internal static void ForgetUnowned() => + internal static void ForgetUnowned() + { unownedAt.Clear(); + reportedForeign.Clear(); + } + + /// + /// Ports already reported as held by something that is not a viewer, so the hint is written + /// once per process rather than once per send. + /// + static readonly ConcurrentDictionary reportedForeign = new(); + + /// + /// A connection accepted and answered with something that is not this protocol: another + /// program holds the port. 3493 is IANA's for Network UPS Tools, whose upsd answers every + /// line it does not understand with an error. + /// + /// Remembered as unowned, because for every purpose here it is: nothing on it will ever take + /// a settle, a move or a delete. Taken for an owner instead, every telling send connected to + /// it, and nothing said why inline snapshots had stopped reaching a viewer. The hint names + /// the variable that moves DiffEngine off the port, which is the only fix - a viewer + /// launched to take the queue cannot bind a port something else holds either. + /// + /// + /// An empty reply is not this. That is an owner that closed without answering, shutting down + /// or wedged, which is an owner behaving badly rather than no owner at all. + /// + /// + static void NotAViewer(int port, string reply) + { + if (string.IsNullOrWhiteSpace(reply)) + { + return; + } + + Found(port, false); + if (!reportedForeign.TryAdd(port, 0)) + { + return; + } + + var first = reply.Split('\n')[0].Trim(); + if (first.Length > 80) + { + first = first.Substring(0, 80); + } + + // Trace rather than Logging, because this file is linked into the viewer too + Trace.WriteLine( + $"Port {port} is held by something that is not a DiffEngine viewer: it answered \"{first}\". " + + "Inline snapshots and pending files cannot reach a viewer there. " + + $"Set the {PortVariable} environment variable to a free port to move DiffEngine off it."); + } /// /// Whether anything is listening, without sending it anything. For a caller that has just @@ -238,7 +289,14 @@ public static bool TrySend( stream.Flush(); HalfClose(client); using var reader = new StreamReader(stream, Encoding.UTF8); - return ViewerResponse.TryParse(reader.ReadToEnd(), out response); + var text = reader.ReadToEnd(); + if (ViewerResponse.TryParse(text, out response)) + { + return true; + } + + NotAViewer(endpointPort, text); + return false; } catch (Exception exception) when (Ignorable(exception)) @@ -344,6 +402,7 @@ public static async Task SendAsync( #endif if (!ViewerResponse.TryParse(text, out var response)) { + NotAViewer(endpointPort, text); return SendOutcome.NoOwner; } diff --git a/src/DiffEngine/Viewer/ViewerLaunchGate.cs b/src/DiffEngine/Viewer/ViewerLaunchGate.cs index 7da68b08..18cb0dbf 100644 --- a/src/DiffEngine/Viewer/ViewerLaunchGate.cs +++ b/src/DiffEngine/Viewer/ViewerLaunchGate.cs @@ -138,6 +138,16 @@ public static ViewerLaunchOutcome Launch( } /// + /// + /// Nothing done while the gate is held resumes on the caller's context: the launch runs on the + /// pool, and the waits do not capture. The sync blocks its thread on the + /// gate, and on a single threaded context - xUnit v2's with one worker, or a UI thread - that + /// thread is the only one a captured continuation could run on, so a sync caller behind an + /// async one waited for a gate that could only be released by the thread doing the waiting. + /// The launch goes to the pool rather than just being awaited without capture, because it + /// awaits things of its own (ViewerLauncher's stdin write) and those capture whatever context + /// is current when it starts. + /// public static async Task LaunchAsync( Func> retry, Func> launch, @@ -148,7 +158,7 @@ public static async Task LaunchAsync( isOwned ??= () => ViewerClient.IsOwned(); canLaunch ??= () => !MaxInstance.Reached(); bool owned; - await gate.WaitAsync(cancel); + await gate.WaitAsync(cancel).ConfigureAwait(false); try { owned = isOwned(); @@ -159,12 +169,12 @@ public static async Task LaunchAsync( return ViewerLaunchOutcome.Capped; } - if (!await launch()) + if (!await Task.Run(launch, cancel).ConfigureAwait(false)) { return ViewerLaunchOutcome.Failed; } - await WaitForBindAsync(isOwned, cancel); + await WaitForBindAsync(isOwned, cancel).ConfigureAwait(false); } } finally @@ -211,7 +221,7 @@ static async Task WaitForBindAsync(Func isOwned, Cancel cancel) return; } - await Task.Delay(Poll, cancel); + await Task.Delay(Poll, cancel).ConfigureAwait(false); } } diff --git a/src/DiffEngine/WildcardFileFinder.cs b/src/DiffEngine/WildcardFileFinder.cs index ee5db7b0..5d975133 100644 --- a/src/DiffEngine/WildcardFileFinder.cs +++ b/src/DiffEngine/WildcardFileFinder.cs @@ -29,8 +29,7 @@ static List EnumerateDirectories(string directory) { if (segment.Contains('*')) { - newRoots.AddRange(Directory.EnumerateDirectories(root, segment) - .OrderByDescending(Directory.GetLastWriteTime)); + newRoots.AddRange(Children(root, segment)); } else { @@ -53,6 +52,36 @@ static List EnumerateDirectories(string directory) return currentRoots; } + /// + /// The directories under matching a wildcard segment, or none when the + /// root cannot be listed. + /// + /// A root that does not exist is ordinary here, not an error: a variable the machine does not + /// define, such as %ProgramW6432% on 32 bit Windows, is left as written by + /// ExpandEnvironmentVariables and becomes a relative root. Thrown, it escaped through + /// DiffTools' static constructor, and every later use of DiffTools in the process was a + /// TypeInitializationException. + /// + /// + static IEnumerable Children(string root, string segment) + { + if (!Directory.Exists(root)) + { + return []; + } + + try + { + return Directory.EnumerateDirectories(root, segment) + .OrderByDescending(Directory.GetLastWriteTime) + .ToList(); + } + catch (Exception exception) when (exception is IOException or UnauthorizedAccessException) + { + return []; + } + } + public static bool TryFind( string path, [NotNullWhen(true)] out string? result) diff --git a/todo.md b/todo.md index 4a6d75a9..65c16d0f 100644 --- a/todo.md +++ b/todo.md @@ -9,7 +9,7 @@ Open findings from a review of `main` at 4244ebe6 (2026-09-23), rechecked on c37 ## Bugs -The repro tests are on the local branch `review-repros`, one class per area: `ReviewReproWindowsTests`, `ReviewReproTrayTests`, `ReviewReproPatcherTests`, `ReviewReproLibraryTests`. Each test fails on c37bf9e1 except a control (`ControlSpaceIndentedLocalLeavesTheSiblingAlone`) and a measurement (`HowLongADecodeHoldsTheFile`). +The repro tests are on the local branch `review-repros`, one class per area: `ReviewReproWindowsTests`, `ReviewReproTrayTests`, `ReviewReproPatcherTests`, `ReviewReproLibraryTests`. The fixed ones have moved into the topic test classes; the settle-by-member repros are still only there. Each test fails on c37bf9e1 except a control (`ControlSpaceIndentedLocalLeavesTheSiblingAlone`) and a measurement (`HowLongADecodeHoldsTheFile`). Viewer model @@ -37,51 +37,6 @@ Library and inline - In Verify's usual shape the failing check throws first and the passing one never runs, so mostly it costs a drop and re-add. The entry is lost when the failing check does not end the test, or when the sibling is a same-named method in another nested class, run later. - Not fixable in the queue, which only sees failing call sites: `SettleFindsAnEntryWhoseLineHasMovedByMember` feeds the same inputs and wants the opposite. The settle has to carry the passing call's value or expression, or the owner has to re-locate the entry's call site. -- [ ] **`MemberLine` takes the nearest same-named declaration, even below the hint** (repro) - - Nearest by line distance, with no check of which span holds the hint (`src/DiffEngine/Inline/InlinePatcher.cs:1083-1120`). Two nested types each declaring `Works`: A's patch goes to B, or misses. - - Tests: `ASameNamedMemberInTheNextNestedTypeDoesNotTakeThePatch`, `ASameNamedMemberInTheNextNestedTypeDoesNotHideTheCall`, `AnFsLocalNamedLikeTheTestDoesNotFloorTheSearchPastTheHint`. - - Fix as suggested, tried: the three pass and the 225 patcher and applier tests still do. When several spans hold the hint, prefer the outermost. - -- [ ] **`NextMemberLine` compares indentation by character count** (repro) - - `InlinePatcher.cs:967`, `:980`. A tab-indented body under a space-indented member ends the member early, so the call is missed on every re-run, or the sibling above it is patched instead. C# only: F# rejects tabs (FS1161). - - Tests: `ATabIndentedLocalDoesNotEndASpaceIndentedMember`, `ATabIndentedLocalDoesNotSendThePatchToTheSiblingAboveIt`, and the passing `ControlSpaceIndentedLocalLeavesTheSiblingAlone`, which differs only in indentation. - - Tab stops of 4 fixed both; matching braces would be sturdier. - -- [ ] **F#: a regular literal whose value looks like layout is AlreadyApplied forever** (repro) - - Content holding `"""` is written as a regular literal (`src/DiffEngine/Inline/FsStringLiteral.cs:39-46`). The test library strips it as layout (`FsLanguage.cs:16-17`); the patcher does not (`FsStringLiteral.cs:127`, `:146`), finds it equal to the new content, and reports AlreadyApplied. The queue drops it, and the next run fails the same way. - - Tests: `FsLayoutShapedRegularLiteralRoundTripsThroughTheCompiler` (through fsi), `FsPatcherDoesNotCallALayoutShapedRegularLiteralAlreadyApplied`. - - Needs both halves, which together passed everything: the writer wraps layout-shaped fallback content in `"\n"…"\n"`, and the patcher strips regular and verbatim values as `SnapshotValue` does. Add the case to `FsCompilerRoundTripTests`. - -- [ ] **F#: escapes F# does not define are rejected as "not a string literal"** (repro) - - fsi keeps `"\d+"`, `"\0"`, `"\12"`, `"\e"`, `"\x4"`, `"\u12"` and `"\U0041"` literally, with no warning. `TryEscape` rejects them (`FsStringLiteral.cs:162-169`, `:188-201`), and the shared scanner rejects `\u`/`\U` before that (`StringLiteral.cs:445-461`), so an accept returns NotFound, on every run. - - Tests: `FsUnknownEscapeIsKeptLiterally` (4 cases), `FsPatcherUpdatesALiteralHoldingAnUnknownEscape`. - - Fixing `TryEscape` alone leaves `\u12`, and breaks `FsStringLiteralTests.ParseRejects` for `\0`, `\12` and `\e`, which pin the belief fsi disproves; they become `Parse` cases. - -- [ ] **`WildcardFileFinder` throws out of `DiffTools`' static constructor under an undefined Program Files variable** (repro) - - The unexpanded `%ProgramW6432%` becomes a relative root (`src/DiffEngine/WildcardFileFinder.cs:20-32`), and `DirectoryNotFoundException` escapes through `OsSettingsResolver.cs:163` to `DiffTools.cs:18`. With `ProgramW6432` unset, `DiffToolsTest` fails with a `TypeInitializationException`. ExamDiff (`ExamDiff.cs:40`) and Beyond Compare put a wildcard right after the variable. - - Tests: `AnUndefinedVariableBeforeAWildcardIsNotFoundRatherThanThrown`, `ResolvingATwoLevelWildcardUnderAnUndefinedVariableIsNotFound`. - - Only 64-bit Windows defines both variables, so this needs 32-bit Windows or a trimmed environment. Fix as suggested. - -- [ ] **Sync `ViewerLaunchGate.Launch` deadlocks behind an async launch on a single threaded context** (repro) - - `gate.Wait()` (`src/DiffEngine/Viewer/ViewerLaunchGate.cs:106`) blocks the thread that `LaunchAsync`'s awaits (`:162`, `:167`, `:214`) need to resume on. - - Tests: `SyncLaunchBehindAnAsyncDeleteOnTheSameContextFinishes`, `SyncLaunchBehindAnAsyncInlineOnTheSameContextFinishes`. - - Verify 33.1.1 calls only the async APIs, so this needs a sync caller (ApprovalTests or Shouldly style) in the same xUnit v2 assembly. Rare. - - `ConfigureAwait(false)` in the gate fixes the delete case only: `ViewerLauncher.cs:28,30,32` resume on the caller's context too. - -- [ ] **`InlineApplier.Apply` throws instead of returning `Failed`** (repro; the `PersistOwned` half was fixed by #878) - - `OpenMutex` (`src/DiffEngine/Inline/InlineApplier.cs:410-413`) and `InlinePatcher.TryApply` (`:152-164`) are unguarded. In a viewer, a single or group accept then unwinds the loop: the queue is staged and the window vanishes mid review. Accept-all, the wire handler and the tray catch it. - - Test: `AMutexThisProcessCannotOpenFailsTheApplyRatherThanThrowing` (`UnauthorizedAccessException` from a mutex an elevated process holds). - -- [ ] **Staged file names over 255 characters are silently not written** (repro) - - `BuildName` (`src/DiffEngine/Inline/InlineStaging.cs:423-428`) adds about 30 characters to the test name; the write throws and `:407-414` swallows it. Long path support does not help: the limit is the 255 character file name component. - - Test: `ALongTestNameIsStillPersisted` (a 242 character test name). - - Above about 225 characters of test name, which is mostly F# sentence names, or fewer non-ASCII ones on ext4 and APFS, whose limit is in bytes. Fix: truncate by UTF-8 bytes; the call-site hash keeps names unique. - -- [ ] **Something other than a viewer on 3493 silently disables the viewer** (repro, against a fake upsd) - - `TrySend` marks the port owned as soon as the connect succeeds (`src/DiffEngine/Protocol/ViewerClient.cs:233`), so an unparseable reply returns false (`:241`) and the unowned-port memory never applies. `IsOwned` stays true, so the gate never launches, `AddInlineAsync` returns `NoViewerFound`, a pair whose tool is the viewer gets `NoDiffToolFound`, deletes are dropped, and nothing mentions `DiffEngine_ViewerPort`. - - Test: `ANonViewerOnThePortIsReportedRatherThanTakenForAnOwner`. - - The connects cost under a millisecond each; losing the viewer is the problem. Real upsd was not tried: if it held the connection open, every send would wait out its timeout. - ## Perf