From 4305d6631f7647d32c6c925da4a3f3630cc6c191 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Thu, 27 Aug 2026 06:08:10 +0300 Subject: [PATCH] Enforce the thread-safe collector rather than rely on remembering it The concurrent-callback contract was documented and still broken in twenty test files. Documenting it again would be the same bet. HarnessInvariantTests reads the test sources and fails when a List's Add is handed to ScanDirectory or ConvertFiles, the two calls whose callbacks the engine documents as concurrent. Scoped to those: callbacks passed to a plain sequential enumeration are not this rule's business, and flagging them would make it noise people learn to ignore. The first version was not scoped and produced seven false positives, including a file whose EntrySink was condemned by an unrelated List of the same name elsewhere in it - so declarations are now resolved to the nearest one before the use. Two things the rule asserts about itself. That it found call sites to examine at all, because a search looking in the wrong place is indistinguishable from a clean result. And that EntrySink is still actually used, because a ban with nothing to point at is one people work around. Verified against a deliberately reintroduced violation: it names the file and line. 424 passing. Co-Authored-By: Claude Opus 5 --- .../HarnessInvariantTests.cs | 173 ++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 sources/EncodingChecker.Tests/HarnessInvariantTests.cs diff --git a/sources/EncodingChecker.Tests/HarnessInvariantTests.cs b/sources/EncodingChecker.Tests/HarnessInvariantTests.cs new file mode 100644 index 0000000..4ef7c49 --- /dev/null +++ b/sources/EncodingChecker.Tests/HarnessInvariantTests.cs @@ -0,0 +1,173 @@ +using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; + +namespace EncodingChecker.Tests; + +/// +/// Rules about the test suite itself, checked by the test suite itself. +/// +/// and invoke +/// their callbacks concurrently from worker threads and say so. The suite passed +/// List<T>.Add anyway, in twenty files, and lost entries — measured at three +/// runs in forty over two hundred files. It surfaced once, as a CI failure whose message +/// blamed the product: a file modified after planning was reported as converted rather +/// than stale, because that file had never reached the plan for the staleness check to +/// look at. +/// +/// The rest of the time it did not surface at all, which is the part worth guarding +/// against. A dropped entry does not throw; it quietly removes a file from what a test +/// then asserts about, so the test passes while asserting less than it claims. A +/// concurrent test that silently loses an input does not merely flake — it can report a +/// safety property as verified when it was never exercised. +/// +/// So the rule is enforced here rather than left to each future author to remember. +/// +public sealed class HarnessInvariantTests +{ + /// + /// The calls whose callbacks the engine documents as concurrent. Callbacks handed to + /// anything else — a plain sequential enumeration, say — are not this rule's business, + /// and flagging them would make the rule noise that people learn to ignore. + /// + private static readonly string[] ConcurrentCalls = + [ + "ScanEngine.ScanDirectory(", + "ScanEngine.ConvertFiles(", + ]; + + /// + /// Passes NAME.Add as a method group — an argument, not a call. A call is + /// followed by '('; a callback being handed over is followed by ',' or ')'. + /// + private static readonly Regex AddAsCallback = new( + @"\b(\w+)\.Add\s*(?=[,)\r\n])", RegexOptions.Compiled); + + private static string TestSourceDirectory([CallerFilePath] string thisFile = "") => + Path.GetDirectoryName(thisFile)!; + + [Fact] + public void NoTestHandsAPlainListToAConcurrentCallback() + { + string directory = TestSourceDirectory(); + + Assert.True( + Directory.Exists(directory), + $"the test sources were not found at '{directory}'; this check reads them, " + + "so it has to run from a source checkout"); + + var offences = new List(); + var scanned = 0; + + foreach (string file in Directory.EnumerateFiles(directory, "*.cs")) + { + string text = File.ReadAllText(file); + + foreach ((int start, string arguments) in ConcurrentCallArguments(text)) + { + scanned++; + + foreach (Match match in AddAsCallback.Matches(arguments)) + { + string name = match.Groups[1].Value; + + if (!IsDeclaredAsAPlainList(text, start, name)) + continue; + + offences.Add( + $"{Path.GetFileName(file)}:{LineOf(text, start)}: " + + $"'{name}.Add' is a List's Add handed to a concurrent callback"); + } + } + } + + // A rule that silently matched nothing would look identical to a rule that + // passed, so the search itself is checked before its result is trusted. + Assert.True( + scanned > 20, + $"only {scanned} concurrent call site(s) were found to check; the search is " + + "not looking where it thinks it is"); + + Assert.True( + offences.Count == 0, + "A List is being handed to a callback the scan engine documents as " + + "concurrent, so entries can be dropped without anything failing. Collect " + + "with EntrySink instead." + + Environment.NewLine + + string.Join(Environment.NewLine, offences)); + } + + [Fact] + public void TheSanctionedSinkIsActuallyUsed() + { + // A ban with nothing to point at is a ban people work around. If the replacement + // ever stops being used, the rule above has quietly become unenforceable rather + // than satisfied. + int users = Directory + .EnumerateFiles(TestSourceDirectory(), "*.cs") + .Count(f => File.ReadAllText(f).Contains( + "new EntrySink()", StringComparison.Ordinal)); + + Assert.True( + users > 10, + $"only {users} test file(s) collect through EntrySink; the concurrent " + + "callbacks are being collected some other way"); + } + + /// + /// The argument list of every call whose callback runs concurrently, with the index + /// the call starts at. + /// + private static IEnumerable<(int Start, string Arguments)> ConcurrentCallArguments( + string text) + { + foreach (string call in ConcurrentCalls) + { + int from = 0; + + while (true) + { + int start = text.IndexOf(call, from, StringComparison.Ordinal); + + if (start < 0) + break; + + int open = start + call.Length - 1; + int depth = 0; + int i = open; + + for (; i < text.Length; i++) + { + if (text[i] == '(') + depth++; + else if (text[i] == ')' && --depth == 0) + break; + } + + yield return (start, text[(open + 1)..Math.Min(i, text.Length)]); + from = start + call.Length; + } + } + } + + /// + /// Whether the nearest declaration of before + /// makes it a plain List. + /// + /// + /// Nearest, not anywhere in the file: one method's List named entries + /// must not condemn another method's EntrySink of the same name, which is the + /// false positive the first version of this rule produced. + /// + private static bool IsDeclaredAsAPlainList(string text, int before, string name) + { + MatchCollection declarations = Regex.Matches( + text[..before], + $@"\b(?:var|List<[^>]*>)\s+{Regex.Escape(name)}\s*=\s*(new\s+)?(?\w+)"); + + return declarations.Count > 0 + && declarations[^1].Groups["type"].Value == "List"; + } + + private static int LineOf(string text, int index) => + text[..index].Count(c => c == '\n') + 1; +}