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