diff --git a/Darling/Darling.Tests/AnalysisPassTokenThreadingTests.cs b/Darling/Darling.Tests/AnalysisPassTokenThreadingTests.cs index 713a88bcea..54fa2ae010 100644 --- a/Darling/Darling.Tests/AnalysisPassTokenThreadingTests.cs +++ b/Darling/Darling.Tests/AnalysisPassTokenThreadingTests.cs @@ -98,29 +98,29 @@ public void NoStoreCallOnTheAnalysisPassRunsWithoutThePassToken() { var offenders = new List(); - foreach (var (file, lines) in AnalysisSources()) + foreach (var (file, code, source) in AnalysisSources()) { var enclosing = string.Empty; - for (var i = 0; i < lines.Length; i++) + for (var i = 0; i < code.Length; i++) { - var declaration = s_memberDeclaration.Match(lines[i]); + var declaration = s_memberDeclaration.Match(code[i]); if (declaration.Success) { enclosing = declaration.Groups["name"].Value; } - if (!s_untokenedStoreCall.IsMatch(lines[i])) + if (!s_untokenedStoreCall.IsMatch(code[i])) { continue; } - if (s_exempt.ContainsKey(enclosing) && DocBlockAbove(lines, IndexOfDeclaration(lines, i)).Contains(ExemptionMarker, StringComparison.Ordinal)) + if (s_exempt.ContainsKey(enclosing) && DocBlockAbove(source, IndexOfDeclaration(code, i)).Contains(ExemptionMarker, StringComparison.Ordinal)) { continue; } - offenders.Add($"{file}:{i + 1} in {enclosing}(): {lines[i].Trim()}"); + offenders.Add($"{file}:{i + 1} in {enclosing}(): {source[i].Trim()}"); } } @@ -144,25 +144,25 @@ public void EveryExemptionIsStillEarningItsPlace() var untokenedBy = new Dictionary(StringComparer.Ordinal); var markedMethods = new HashSet(StringComparer.Ordinal); - foreach (var (_, lines) in AnalysisSources()) + foreach (var (_, code, source) in AnalysisSources()) { var enclosing = string.Empty; var enclosingAt = -1; - for (var i = 0; i < lines.Length; i++) + for (var i = 0; i < code.Length; i++) { - var declaration = s_memberDeclaration.Match(lines[i]); + var declaration = s_memberDeclaration.Match(code[i]); if (declaration.Success) { enclosing = declaration.Groups["name"].Value; enclosingAt = i; - if (DocBlockAbove(lines, enclosingAt).Contains(ExemptionMarker, StringComparison.Ordinal)) + if (DocBlockAbove(source, enclosingAt).Contains(ExemptionMarker, StringComparison.Ordinal)) { markedMethods.Add(enclosing); } } - if (s_untokenedStoreCall.IsMatch(lines[i]) && enclosingAt >= 0) + if (s_untokenedStoreCall.IsMatch(code[i]) && enclosingAt >= 0) { untokenedBy[enclosing] = untokenedBy.TryGetValue(enclosing, out var n) ? n + 1 : 1; } @@ -194,18 +194,18 @@ public void EveryCatchOnTheFactCollectorLetsAnAbandonmentThrough() @"^\s*catch\s*\(\s*Exception\s+ex\s*\)\s*when\s*\(\s*!AnalysisShutdown\.IsExpectedAbandon\(", RegexOptions.Compiled | RegexOptions.CultureInvariant); - foreach (var (file, lines) in AnalysisSources()) + foreach (var (file, code, source) in AnalysisSources()) { if (!Path.GetFileName(file).StartsWith("PgFactCollector.", StringComparison.Ordinal)) { continue; } - for (var i = 0; i < lines.Length; i++) + for (var i = 0; i < code.Length; i++) { - if (opensCatch.IsMatch(lines[i]) && !classified.IsMatch(lines[i])) + if (opensCatch.IsMatch(code[i]) && !classified.IsMatch(code[i])) { - bare.Add($"{file}:{i + 1}: {lines[i].Trim()}"); + bare.Add($"{file}:{i + 1}: {source[i].Trim()}"); } } } @@ -219,7 +219,7 @@ public void EveryCatchOnTheFactCollectorLetsAnAbandonmentThrough() straight to the pass, which is the same outcome by a shorter route. */ Assert.Equal(27, AnalysisSources() .Where(s => Path.GetFileName(s.File).StartsWith("PgFactCollector.", StringComparison.Ordinal)) - .SelectMany(s => s.Lines) + .SelectMany(s => s.Code) .Count(line => classified.IsMatch(line))); } @@ -325,7 +325,11 @@ private static int IndexOfDeclaration(string[] lines, int callLine) return 0; } - /// The contiguous /// block immediately above a declaration, as one string. + /// The contiguous /// block immediately above a declaration, as one string. Reads the + /// file AS WRITTEN, not the walked code: the exemption marker lives in a comment, so stripping comments + /// is exactly what must not happen here. The /// prefix is the right test for the same reason — + /// a doc comment is prefixed on every line by definition, which is what a /* … */ body in this + /// codebase is not. private static string DocBlockAbove(string[] lines, int declarationLine) { var doc = new List(); @@ -337,7 +341,18 @@ private static string DocBlockAbove(string[] lines, int declarationLine) return string.Join("\n", doc); } - private static IEnumerable<(string File, string[] Lines)> AnalysisSources() + /// + /// Two views of each file, line-for-line aligned. Code is + /// 's output and is what every regex here reads, + /// so a commented-out call and a doc comment naming ReadAsync() are neither offenders nor + /// counted. Source is the file as written, and exists for the two things that are ABOUT the + /// prose: , which reads the exemption marker out of a doc comment, and the + /// offender messages, which should quote what someone will find when they open the file. + /// + /// The alignment is what makes one index serve both: the walk blanks comments and literal text to + /// spaces and leaves every newline in place, so line i is line i in either view. + /// + private static IEnumerable<(string File, string[] Code, string[] Source)> AnalysisSources() { var files = Directory.GetFiles(AnalysisDirectory(), "*.cs", SearchOption.TopDirectoryOnly); Assert.NotEmpty(files); @@ -345,7 +360,12 @@ private static string DocBlockAbove(string[] lines, int declarationLine) foreach (var file in files.OrderBy(f => f, StringComparer.Ordinal)) { /* The working copy is CRLF; split on the LF so a line never carries a stray CR. */ - yield return (Path.GetFileName(file), ReadSource(file).Replace("\r\n", "\n", StringComparison.Ordinal).Split('\n')); + var source = ReadSource(file).Replace("\r\n", "\n", StringComparison.Ordinal); + + yield return ( + Path.GetFileName(file), + CSharpSourceWalker.StripCommentsAndStrings(source).Split('\n'), + source.Split('\n')); } } diff --git a/Darling/Darling.Tests/CommentFilterAdoptionTests.cs b/Darling/Darling.Tests/CommentFilterAdoptionTests.cs new file mode 100644 index 0000000000..ae9421de81 --- /dev/null +++ b/Darling/Darling.Tests/CommentFilterAdoptionTests.cs @@ -0,0 +1,299 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; +using Xunit; + +namespace Darling.Tests; + +/// +/// Which source-scanning pins separate code from comment by LINE PREFIX, and what bound each one rests on +/// (#3052). +/// +/// Why the shape needs watching at all. Dropping lines that start with //, ///, +/// / or * works for line comments and doc comments, which are prefixed on every line. It does +/// not work for block comments, because this codebase does not put an asterisk on the continuation +/// lines: +/// +/// /* Released here rather than at the closing brace: the six sub-tab loads at the end of this method +/// run after these five have finished, so they do not contend with them. */ +/// +/// Only the first line starts with /. Every line after it is indented prose that a prefix +/// filter hands to the scanner as code, so a comment mentioning the thing being banned becomes an +/// offender. +/// +/// The shape is not banned, because legitimate uses of it exist. Collecting a /// run is +/// not the same act as excluding comments from a code scan — a doc comment IS prefixed on every line, so the +/// prefix is the definition of the thing being gathered rather than an approximation of it. Reading a bound +/// off a named file is legitimate too, when the bound is stated and measured. And a filter over SQL text is +/// not this problem at all, because the C# walk cannot read SQL. So each site is listed with its reason and +/// the build re-derives the set, the same agreement +/// holds over the command-timeout family. +/// +/// The failure direction, since it decides how much this is worth. Every instance #3052 found +/// UNDER-strips: prose reads as code, the offender list gains an entry that is not one, and an +/// Assert.Empty fails loudly. That is the safe direction — a spurious red, not a hidden offender. The +/// instance that actually cost something was not an assertion: during #3047's review a check of this shape +/// reported 45 changed lines in a comments-only diff, nothing went red because nothing was asserting, and +/// the number nearly produced a wrong verdict about whether that PR touched code. The same filter is loud +/// when it drives an assertion and silent when it drives a number, and the silent half lives in ad-hoc +/// review checks that no test can reach. This guard covers the half that is committed. +/// +/// No build.yml filter entry, deliberately. This reads BOTH test projects, so a +/// Lite.Tests-only change fires lite and not darling, and the "Run Darling tests" step +/// is skipped. Naming Lite.Tests/** in the darling filter would fix that and also gate the +/// live-Postgres darling-pg job on all 298 of Lite's test files, which is a real cost for a guard +/// that reads source. It belongs instead in the category build.yml's whole-tree-guards job +/// already exists for — guards whose input is the repository rather than a path, like +/// FleetIdentifierScrubTests — and that job runs this suite exactly when the build job's step reports +/// skipped. Note that CrossAppGuardCiGateTests cannot see this reference either way: +/// its detector keys on Lite/ and Darling/, and Lite.Tests is a sibling of Lite +/// rather than a directory inside it. +/// +public sealed class CommentFilterAdoptionTests +{ + /// + /// The prefixes that mean "this line is a comment" in C#. * and / are here because the + /// existing sites use them: * catches a banner-style continuation and / catches //, + /// /// and /* at once. + /// + private static readonly string[] s_commentPrefixes = { "//", "///", "/*", "*", "/" }; + + /// + /// Every pin allowed to split code from comment by line prefix, and the bound it rests on. An + /// enumeration rather than a wildcard, for the reason the exemption list in + /// gives: the way a wildcard grows is that nobody has to + /// name a new entry. + /// + /// Four kinds live here and they are not the same kind. Two collect a doc-comment run, + /// where the prefix is what defines the run. One reads a stated, measured bound off a named file. + /// One filters SQL, which the C# walk cannot help with. One demonstrates the shape on an + /// arranged fixture, which is this file. + /// + private static readonly Dictionary s_bounded = new(StringComparer.Ordinal) + { + ["Darling.Tests/CommentFilterAdoptionTests.cs"] = + "DEMONSTRATES the shape. The control below builds the prefix filter deliberately, over an " + + "arranged fixture, to show that it reads a block comment's body as code and that the walk does " + + "not. Written with the literal rather than with s_commentPrefixes on purpose: routing it " + + "through the array would slip past this guard's own detector, and a guard that demonstrates " + + "the loophole in its stated bound is worse than one that names itself. The cost of the entry " + + "is that a REAL prefix filter added to this file would not be noticed.", + + ["Darling.Tests/AnalysisPassTokenThreadingTests.cs"] = + "COLLECTS a doc run. DocBlockAbove gathers the contiguous /// block above a declaration to read the " + + "#2443 exemption marker out of it. A doc comment is prefixed on every line by definition, and the " + + "marker lives IN a comment, so stripping comments here is the one thing that would break it. The " + + "regexes in this file read the walked code.", + + ["Lite.Tests/AnalysisPassTokenThreadingTests.cs"] = + "COLLECTS a doc run. The Lite twin of the above, same helper, same reason.", + + ["Darling.Tests/DocCommentHygieneTests.cs"] = + "COLLECTS a doc run. DocRuns groups contiguous /// lines into the run that documents one member, " + + "which is the subject of the whole class rather than an approximation of it.", + + ["Lite.Tests/LiteSidebarDotRendersTheCardStatusTests.cs"] = + "STATED BOUND, and asking for the walker would BREAK it. Its doc comment records the measurement: " + + "ServerConnection.cs carries exactly one block comment, the licence header, so dropping " + + "//-prefixed lines cannot eat a literal. It then asserts that four status WORDS appear in no " + + "string literal there - so blanking literal text would satisfy Assert.DoesNotContain vacuously, " + + "which is a pass for the wrong reason. Measurement-dependent, not permanent: a second block " + + "comment in that file makes the bound wrong.", + + ["Darling.Tests/Pg18IoBytesTests.cs"] = + "NOT C#. The filter runs over DarlingPgIoReader.PgIoSql - SQL text already extracted from a raw " + + "string literal - and drops SQL block-comment lines from the outer SELECT list before counting " + + "its items. CSharpSourceWalker cannot read SQL, so adopting it here would be a category error. " + + "The window it cuts (the outer SELECT through FROM differenced) holds no comment today, so the " + + "/* and * arms are inert; a comment added there whose continuation line contains ' AS ' would " + + "over-count the select list and fail the 19 pin loudly.", + }; + + /// + /// A real call to StartsWith, excluding Assert.StartsWith. The exclusion is the distinction + /// #3052's own census got wrong twice: Assert.StartsWith("/*", sql) asserts that GENERATED T-SQL + /// opens with a risk-disclosure header, which is the opposite act from filtering comments out of source. + /// Same substring, different direction, and two files were put on the list for having it. + /// + private static readonly Regex s_startsWithCall = new( + @"(?= 600, + $"the sweep found only {files.Count} test source file(s) — it is not reading both test projects"); + + var found = new List(); + + foreach (var (name, text) in files) + { + if (PrefixFilterSites(text).Count > 0) + { + found.Add(name); + } + } + + Assert.Equal( + s_bounded.Keys.OrderBy(k => k, StringComparer.Ordinal).ToArray(), + found.OrderBy(k => k, StringComparer.Ordinal).ToArray()); + } + + /// + /// The control, and the only thing here that can fail on an arranged input rather than on the tree: the + /// prefix form really does read a block comment's body as code, and the walk really does not. Without + /// this, "the walker fixes it" would be a claim in a doc comment with nothing checking it. + /// + /// The fixture is the shape the issue describes — a token inside /* … */ whose continuation + /// lines carry no asterisk — plus one real call, so a scan that finds NOTHING cannot pass either half. + /// + /// + [Fact] + public void TheLinePrefixForm_ReadsABlockCommentsBodyAsCode_AndTheWalkDoesNot() + { + const string fixture = """ + public void Probe() + { + /* The deadline is set by the caller rather than here, because a nested + CreateCommand(sql) would take the outer command's budget and the two + are not the same measurement. */ + Run(); + CreateCommand(other); + } + """; + + var prefixFiltered = string.Join("\n", fixture.Split('\n') + .Where(l => !l.TrimStart().StartsWith("/", StringComparison.Ordinal) + && !l.TrimStart().StartsWith("*", StringComparison.Ordinal))); + + /* The defect: the continuation line survives the filter, so the prose reads as a second call. */ + Assert.Equal(2, Occurrences(prefixFiltered, "CreateCommand(")); + + /* And the fix: the walk leaves the real call and nothing else. */ + var walked = CSharpSourceWalker.StripCommentsAndStrings(fixture); + Assert.Equal(1, Occurrences(walked, "CreateCommand(")); + + /* Stated as a line number too, because the pins report offenders by line and a walk that dropped a + newline would send the reader to innocent code. */ + Assert.Equal( + fixture.Split('\n').Length, + walked.Split('\n').Length); + } + + /// + /// Every StartsWith call in whose first argument is a comment prefix, as + /// the offset of the call. + /// + /// Matched over 's output so that prose + /// ABOUT the shape does not count as the shape — which this very file needs, since it quotes the filter + /// it is guarding. The argument is then read out of the ORIGINAL text at the same offset, because the + /// walk blanks literal TEXT and the argument is exactly that; the two are character-aligned, which is + /// what makes reading one at the other's index sound. + /// + /// Only a plain "…" argument is recognised. A comment prefix spelled any other way — a + /// verbatim literal, a const, a char — would slip past, and there is no such site in + /// either project. That is a stated bound on this guard and not a claim about C#. + /// + private static List PrefixFilterSites(string text) + { + var code = CSharpSourceWalker.StripCommentsAndStrings(text); + var sites = new List(); + + foreach (Match call in s_startsWithCall.Matches(code)) + { + var i = call.Index + call.Length; + + while (i < text.Length && char.IsWhiteSpace(text[i])) + { + i++; + } + + if (i >= text.Length || text[i] != '"') + { + continue; + } + + var close = text.IndexOf('"', i + 1); + if (close < 0) + { + continue; + } + + if (s_commentPrefixes.Contains(text[(i + 1)..close], StringComparer.Ordinal)) + { + sites.Add(call.Index); + } + } + + return sites; + } + + private static int Occurrences(string text, string needle) + { + var count = 0; + for (var i = text.IndexOf(needle, StringComparison.Ordinal); i >= 0; i = text.IndexOf(needle, i + 1, StringComparison.Ordinal)) + { + count++; + } + + return count; + } + + /// + /// Both test projects' sources, keyed by the project-relative name the allow-list uses — so an entry + /// names one project's file and not "whichever twin the sweep reached first". + /// + private static List<(string Name, string Text)> TestSources() + { + var root = RepoRoot(); + var sources = new List<(string, string)>(); + + foreach (var (project, directory) in new[] + { + ("Darling.Tests", Path.Combine(root, "Darling", "Darling.Tests")), + ("Lite.Tests", Path.Combine(root, "Lite.Tests")), + }) + { + foreach (var path in Directory.EnumerateFiles(directory, "*.cs", SearchOption.AllDirectories) + .Where(p => !p.Contains($"{Path.DirectorySeparatorChar}bin{Path.DirectorySeparatorChar}", StringComparison.Ordinal)) + .Where(p => !p.Contains($"{Path.DirectorySeparatorChar}obj{Path.DirectorySeparatorChar}", StringComparison.Ordinal)) + .OrderBy(p => p, StringComparer.Ordinal)) + { + sources.Add(($"{project}/{Path.GetFileName(path)}", File.ReadAllText(path))); + } + } + + return sources; + } + + private static string RepoRoot([CallerFilePath] string thisFile = "") + { + var dir = Path.GetDirectoryName(thisFile)!; + while (dir is not null && !File.Exists(Path.Combine(dir, "PerformanceMonitor.sln"))) + { + dir = Path.GetDirectoryName(dir); + } + + Assert.NotNull(dir); + return dir!; + } +} diff --git a/Darling/Darling.Tests/McpConfigReadAvoidsSecretColumnsTests.cs b/Darling/Darling.Tests/McpConfigReadAvoidsSecretColumnsTests.cs index 9d0c4fe9a8..58564c3998 100644 --- a/Darling/Darling.Tests/McpConfigReadAvoidsSecretColumnsTests.cs +++ b/Darling/Darling.Tests/McpConfigReadAvoidsSecretColumnsTests.cs @@ -75,9 +75,12 @@ public void TheMcpSurfaceUsesNeitherSmtpNorWebhooks() var offenders = Directory.GetFiles(mcpDir, "*.cs", SearchOption.AllDirectories) .Where(f => { - var text = File.ReadAllText(f); - /* The comment explaining the skip names both types, so only real USES count. */ - var code = string.Join("\n", text.Split('\n').Where(l => !l.TrimStart().StartsWith("/", StringComparison.Ordinal) && !l.TrimStart().StartsWith("*", StringComparison.Ordinal))); + /* The comment explaining the skip names both types, so only real USES count. Walked rather + than filtered by line prefix: a block comment's continuation lines in this codebase carry + no asterisk, so a prefix filter reads the second line of an explanatory block as code and + reports the prose as a use. Both needles are property accesses, so blanking literal text + along with the comments costs nothing here. */ + var code = CSharpSourceWalker.StripCommentsAndStrings(File.ReadAllText(f)); return code.Contains(".Smtp", StringComparison.Ordinal) || code.Contains(".Webhooks", StringComparison.Ordinal); }) .Select(Path.GetFileName) diff --git a/Darling/Darling.Tests/ServerIdentityFromStoreTests.cs b/Darling/Darling.Tests/ServerIdentityFromStoreTests.cs index 0325ad7093..5e181e0fcb 100644 --- a/Darling/Darling.Tests/ServerIdentityFromStoreTests.cs +++ b/Darling/Darling.Tests/ServerIdentityFromStoreTests.cs @@ -135,10 +135,16 @@ public void TheServiceNoLongerDerivesIdentity(string relativePath) var root = FindRepoRoot(); Assert.True(root is not null, "repo root not found -- the source pin cannot run"); - var offenders = File.ReadAllLines(Path.Combine(root!, relativePath)) + /* Comments name the helper on purpose (the cadence-jitter doc explains what its input is), so the + scan reads CODE only. Walked rather than filtered by line prefix: a `///` filter sees doc comments + and nothing else, and a block comment's continuation lines in this codebase carry no asterisk, so + an explanatory note about the helper reads as a call to it. Line numbers still line up because the + walk preserves every newline. */ + var offenders = CSharpSourceWalker + .StripCommentsAndStrings( + File.ReadAllText(Path.Combine(root!, relativePath)).Replace("\r\n", "\n", StringComparison.Ordinal)) + .Split('\n') .Select((line, index) => (Line: line, Number: index + 1)) - /* Doc comments name the helper on purpose (the cadence-jitter doc explains what its input is). */ - .Where(l => !l.Line.TrimStart().StartsWith("///", StringComparison.Ordinal)) .Where(l => l.Line.Contains("GetDeterministicHashCode(", StringComparison.Ordinal)) .Select(l => $"{relativePath}:{l.Number}") .ToList(); @@ -176,10 +182,11 @@ public void IdentityIsMintedInExactlyThreePlaces() .SelectMany(dir => Directory.EnumerateFiles(Path.Combine(root!, dir), "*.cs", SearchOption.AllDirectories)) .Where(path => !path.Contains($"{Path.DirectorySeparatorChar}obj{Path.DirectorySeparatorChar}", StringComparison.Ordinal) && !path.Contains($"{Path.DirectorySeparatorChar}bin{Path.DirectorySeparatorChar}", StringComparison.Ordinal)) - .Where(path => File.ReadAllLines(path) - .Any(line => !line.TrimStart().StartsWith("///", StringComparison.Ordinal) - && !line.TrimStart().StartsWith("*", StringComparison.Ordinal) - && line.Contains("GetDeterministicHashCode(", StringComparison.Ordinal))) + /* Same walk, same reason as the pin above: a block comment naming the helper would otherwise + mint a fourth allocation site out of prose. */ + .Where(path => CSharpSourceWalker + .StripCommentsAndStrings(File.ReadAllText(path)) + .Contains("GetDeterministicHashCode(", StringComparison.Ordinal)) .Select(path => Path.GetRelativePath(root!, path).Replace(Path.DirectorySeparatorChar, '/')) .OrderBy(path => path, StringComparer.Ordinal) .ToList(); diff --git a/Darling/Darling.Tests/TimescaleEnableConnectionSafetyTests.cs b/Darling/Darling.Tests/TimescaleEnableConnectionSafetyTests.cs index 1d5c73aca0..c55bc9766c 100644 --- a/Darling/Darling.Tests/TimescaleEnableConnectionSafetyTests.cs +++ b/Darling/Darling.Tests/TimescaleEnableConnectionSafetyTests.cs @@ -52,15 +52,24 @@ public void NoLiveTest_KeepsUsingAConnectionItHandedToTryEnableAsync() { var name = Path.GetFileName(file); - /* The probe is where the guarded call is SUPPOSED to live, and this file names the method in - prose throughout. */ - if (name is nameof(LiveTimescaleProbe) + ".cs" or nameof(TimescaleEnableConnectionSafetyTests) + ".cs") + /* The probe is where the guarded call is SUPPOSED to live, so its own call is not an offender. + This file used to be skipped beside it because it names the method throughout in prose and in + regex literals; the walk below removes both, so the exemption stopped earning its place and an + unsafe call written HERE is now caught like any other. */ + if (name is nameof(LiveTimescaleProbe) + ".cs") { continue; } - var text = File.ReadAllText(file); - var lines = File.ReadAllLines(file); + /* Comments and literal text are blanked before anything is matched, because prose about the call + is not a call. A line-prefix filter cannot deliver that: a block comment's continuation lines + in this codebase carry no asterisk, and the worked example in this very file's + `IsCheckedOnTheSpot` note is one — an indented `await TimescaleSupport.TryEnableAsync(...)` + inside a block comment, which a prefix filter reads as an unchecked call. Newlines survive the + walk, so the reported line is still the file's own. */ + var text = CSharpSourceWalker.StripCommentsAndStrings( + File.ReadAllText(file).Replace("\r\n", "\n", StringComparison.Ordinal)); + var lines = text.Split('\n'); /* Whitespace-tolerant, and matched against the whole FILE rather than line by line: a call split as `TimescaleSupport\n .TryEnableAsync(...)` is the same call, and a guard that a line break @@ -69,13 +78,6 @@ can slip past is not a guard. Same reason the window below is flattened before i { var lineIndex = text.Take(call.Index).Count(c => c == '\n'); - /* A doc comment mentioning it is not a call. */ - var trimmed = lines[lineIndex].TrimStart(); - if (trimmed.StartsWith("///", StringComparison.Ordinal) || trimmed.StartsWith("*", StringComparison.Ordinal)) - { - continue; - } - if (!IsCheckedOnTheSpot(lines, lineIndex)) { offenders.Add($"{name}:{lineIndex + 1}"); @@ -148,6 +150,11 @@ the gate does. */ /// rather than reading it. The capture form is tied to its OWN variable, and the check must be the very /// NEXT statement: "checked somewhere nearby" is not the property that matters, since anything between /// the capture and the check would itself be running on a possibly-dead connection. + /// + /// is 's output, + /// which is what makes the statement split below sound: a semicolon inside a comment between the call + /// and its assertion would otherwise end the statement early and report a checked site as an + /// offender. /// private static bool IsCheckedOnTheSpot(string[] lines, int index) { diff --git a/Lite.Tests/AnalysisPassTokenThreadingTests.cs b/Lite.Tests/AnalysisPassTokenThreadingTests.cs index 574b94173b..b2e38928be 100644 --- a/Lite.Tests/AnalysisPassTokenThreadingTests.cs +++ b/Lite.Tests/AnalysisPassTokenThreadingTests.cs @@ -15,6 +15,7 @@ using System.Text.RegularExpressions; using System.Threading; using System.Threading.Tasks; +using Darling.Tests; using PerformanceMonitorLite.Analysis; using PerformanceMonitorLite.Database; using Xunit; @@ -93,30 +94,30 @@ public void NoStoreCallOnTheAnalysisPassRunsWithoutThePassToken() { var offenders = new List(); - foreach (var (file, lines) in AnalysisSources()) + foreach (var (file, code, source) in AnalysisSources()) { var enclosing = string.Empty; - for (var i = 0; i < lines.Length; i++) + for (var i = 0; i < code.Length; i++) { - var declaration = s_memberDeclaration.Match(lines[i]); + var declaration = s_memberDeclaration.Match(code[i]); if (declaration.Success) { enclosing = declaration.Groups["name"].Value; } - if (!s_untokenedStoreCall.IsMatch(lines[i])) + if (!s_untokenedStoreCall.IsMatch(code[i])) { continue; } if (s_exempt.ContainsKey(enclosing) - && DocBlockAbove(lines, IndexOfDeclaration(lines, i)).Contains(ExemptionMarker, StringComparison.Ordinal)) + && DocBlockAbove(source, IndexOfDeclaration(code, i)).Contains(ExemptionMarker, StringComparison.Ordinal)) { continue; } - offenders.Add($"{file}:{i + 1} in {enclosing}(): {lines[i].Trim()}"); + offenders.Add($"{file}:{i + 1} in {enclosing}(): {source[i].Trim()}"); } } @@ -138,25 +139,25 @@ public void EveryExemptionIsStillEarningItsPlace() var untokenedBy = new Dictionary(StringComparer.Ordinal); var markedMethods = new HashSet(StringComparer.Ordinal); - foreach (var (_, lines) in AnalysisSources()) + foreach (var (_, code, source) in AnalysisSources()) { var enclosing = string.Empty; var enclosingAt = -1; - for (var i = 0; i < lines.Length; i++) + for (var i = 0; i < code.Length; i++) { - var declaration = s_memberDeclaration.Match(lines[i]); + var declaration = s_memberDeclaration.Match(code[i]); if (declaration.Success) { enclosing = declaration.Groups["name"].Value; enclosingAt = i; - if (DocBlockAbove(lines, enclosingAt).Contains(ExemptionMarker, StringComparison.Ordinal)) + if (DocBlockAbove(source, enclosingAt).Contains(ExemptionMarker, StringComparison.Ordinal)) { markedMethods.Add(enclosing); } } - if (s_untokenedStoreCall.IsMatch(lines[i]) && enclosingAt >= 0) + if (s_untokenedStoreCall.IsMatch(code[i]) && enclosingAt >= 0) { untokenedBy[enclosing] = untokenedBy.TryGetValue(enclosing, out var n) ? n + 1 : 1; } @@ -188,18 +189,18 @@ public void EveryCatchOnTheFactCollectorLetsAnAbandonmentThrough() @"^\s*catch\s*\(\s*Exception\s+ex\s*\)\s*when\s*\(\s*!AnalysisAbandon\.IsExpected\(", RegexOptions.Compiled | RegexOptions.CultureInvariant); - foreach (var (file, lines) in AnalysisSources()) + foreach (var (file, code, source) in AnalysisSources()) { if (!Path.GetFileName(file).StartsWith("DuckDbFactCollector.", StringComparison.Ordinal)) { continue; } - for (var i = 0; i < lines.Length; i++) + for (var i = 0; i < code.Length; i++) { - if (opensCatch.IsMatch(lines[i]) && !classified.IsMatch(lines[i])) + if (opensCatch.IsMatch(code[i]) && !classified.IsMatch(code[i])) { - bare.Add($"{file}:{i + 1}: {lines[i].Trim()}"); + bare.Add($"{file}:{i + 1}: {source[i].Trim()}"); } } } @@ -213,7 +214,7 @@ public void EveryCatchOnTheFactCollectorLetsAnAbandonmentThrough() straight to the pass, which is the same outcome by a shorter route. */ Assert.Equal(27, AnalysisSources() .Where(s => Path.GetFileName(s.File).StartsWith("DuckDbFactCollector.", StringComparison.Ordinal)) - .SelectMany(s => s.Lines) + .SelectMany(s => s.Code) .Count(line => classified.IsMatch(line))); } @@ -362,7 +363,11 @@ private static int IndexOfDeclaration(string[] lines, int callLine) return 0; } - /// The contiguous /// block immediately above a declaration, as one string. + /// The contiguous /// block immediately above a declaration, as one string. Reads the + /// file AS WRITTEN, not the walked code: the exemption marker lives in a comment, so stripping comments + /// is exactly what must not happen here. The /// prefix is the right test for the same reason — + /// a doc comment is prefixed on every line by definition, which is what a block comment's body in this + /// codebase is not. private static string DocBlockAbove(string[] lines, int declarationLine) { var doc = new List(); @@ -374,7 +379,18 @@ private static string DocBlockAbove(string[] lines, int declarationLine) return string.Join("\n", doc); } - private static IEnumerable<(string File, string[] Lines)> AnalysisSources() + /// + /// Two views of each file, line-for-line aligned. Code is + /// CSharpSourceWalker.StripCommentsAndStrings's output and is what every regex here reads, so a + /// commented-out call and a doc comment naming ReadAsync() are neither offenders nor counted. + /// Source is the file as written, and exists for the two things that are ABOUT the prose: + /// , which reads the exemption marker out of a doc comment, and the offender + /// messages, which should quote what someone will find when they open the file. + /// + /// The alignment is what makes one index serve both: the walk blanks comments and literal text to + /// spaces and leaves every newline in place, so line i is line i in either view. + /// + private static IEnumerable<(string File, string[] Code, string[] Source)> AnalysisSources() { var files = Directory.GetFiles(AnalysisDirectory(), "*.cs", SearchOption.TopDirectoryOnly); Assert.NotEmpty(files); @@ -382,8 +398,12 @@ private static string DocBlockAbove(string[] lines, int declarationLine) foreach (var file in files.OrderBy(f => f, StringComparer.Ordinal)) { /* The working copy is CRLF; split on the LF so a line never carries a stray CR. */ - yield return (Path.GetFileName(file), - File.ReadAllText(file).Replace("\r\n", "\n", StringComparison.Ordinal).Split('\n')); + var source = File.ReadAllText(file).Replace("\r\n", "\n", StringComparison.Ordinal); + + yield return ( + Path.GetFileName(file), + CSharpSourceWalker.StripCommentsAndStrings(source).Split('\n'), + source.Split('\n')); } } diff --git a/Lite.Tests/AuroraOnlySqlIsGatedTests.cs b/Lite.Tests/AuroraOnlySqlIsGatedTests.cs index f24c7f60e9..7b169f2ed5 100644 --- a/Lite.Tests/AuroraOnlySqlIsGatedTests.cs +++ b/Lite.Tests/AuroraOnlySqlIsGatedTests.cs @@ -11,6 +11,7 @@ using System.IO; using System.Linq; using System.Text.RegularExpressions; +using Darling.Tests; using Xunit; namespace Lite.Tests; @@ -142,6 +143,64 @@ public void APairedFileReallyReadsTheVanillaViewToo(string fileName) Assert.Contains("isaurora", source, StringComparison.OrdinalIgnoreCase); } + /// + /// The control for how reads a file, on an arranged + /// input rather than on the tree — because both of the plausible one-line filters get this wrong, in + /// opposite directions, and neither failure is visible in a passing sweep. + /// + /// The fixture puts the surface name in a SQL literal (which IS a dependency) and in a block + /// comment whose continuation lines carry no asterisk (which is not). A line-prefix filter finds the + /// comment; StripCommentsAndStrings alone finds neither, because the dependency lives in the + /// literal it blanks. Only asking both non-comment views gives the answer, and the second assertion + /// below is the one that stops someone "simplifying" this to the walker's stripper on the grounds that + /// #3052 named it as the fix. + /// + [Fact] + public void TheReadFindsASurfaceInSql_AndNotOneInABlockComment() + { + const string fixture = """ + internal static class Fixture + { + /* Aurora exposes aurora_stat_statements, which community PostgreSQL does not have; the + vanilla path reads pg_stat_statements instead and types the missing columns as NULL. */ + private const string Sql = "SELECT calls FROM aurora_stat_wait_event"; + } + """; + + var path = Path.Combine(Path.GetTempPath(), $"pm-3052-{Guid.NewGuid():N}.cs"); + + try + { + File.WriteAllText(path, fixture); + + /* The literal is found. */ + Assert.True(NamesAnAuroraSurfaceOutsideAComment(path)); + + /* And the reason the read cannot be the stripper alone: it hides exactly that literal. */ + Assert.DoesNotContain( + "aurora_stat_wait_event", + CSharpSourceWalker.StripCommentsAndStrings(fixture), + StringComparison.Ordinal); + + /* The comment half, asked the other way round: the walk removes the paragraph a line-prefix + filter would have handed over as code, continuation line and all. The prefix filter's own + failure on this shape is pinned in Darling.Tests/CommentFilterAdoptionTests. */ + Assert.DoesNotContain( + "aurora_stat_statements", + CSharpSourceWalker.StripCommentsAndStrings(fixture), + StringComparison.Ordinal); + + Assert.Contains( + "aurora_stat_wait_event", + string.Join("\n", CSharpSourceWalker.StringLiteralBodies(fixture).Select(b => b.Text)), + StringComparison.Ordinal); + } + finally + { + File.Delete(path); + } + } + private static string[] FilesNamingAuroraSurfaces() { var root = RepoRoot(); @@ -159,23 +218,52 @@ the opposite of depending on it. */ } /// - /// Comment lines are skipped, or every explanatory paragraph about why Aurora is different would count - /// as a dependency on it — which would make the guard so noisy that the allow-list stopped being read. + /// Comments are skipped, or every explanatory paragraph about why Aurora is different would count as a + /// dependency on it — which would make the guard so noisy that the allow-list stopped being read. + /// + /// Asked of code and of string LITERALS, and never of prose. That combination is the whole + /// subtlety, because the two halves fail in opposite directions here. A line-prefix filter under-strips: + /// a block comment's continuation lines in this codebase carry no asterisk, so the second line of a + /// paragraph about aurora_stat_statements reads as a dependency and the file arrives unaccounted + /// (#3052). But alone over-strips for THIS + /// question, and catastrophically: every Aurora surface name in this repo is SQL inside a string + /// literal, so blanking literal text finds nothing at all — four accounted files become zero, which + /// fails Assert.NotEmpty loudly today and would otherwise be a guard that had quietly stopped + /// looking. + /// + /// So the walk is used for what it is exact about — where the comments are — and both of the + /// non-comment views it exposes are asked: the code, and the literal bodies. Blanking interpolation + /// holes inside those bodies costs nothing, because a surface NAME is never assembled out of a + /// hole. + /// + /// The raw-text gate is not an optimisation to taste. This read enumerates every + /// *.cs file in the repository — 1,345 of them — once per test, four times over, and the walk is a + /// per-character pass with a StringBuilder behind it. Measured: walking unconditionally took the + /// class from 0.55s to 5.53s, a 10x regression on a guard that is not the thing under test. The gate is + /// EXACT rather than approximate, which is what makes it safe to skip the walk on: blanking only ever + /// replaces a character with a space or a newline, and no surface name contains either, so a needle + /// absent from the raw text is absent from every view of it. Roughly six files in the tree get past it, + /// and the class is back to 0.49s. /// private static bool NamesAnAuroraSurfaceOutsideAComment(string path) - => File.ReadLines(path).Any(line => + { + var text = File.ReadAllText(path); + + if (!AuroraSurfaces.Any(s => text.Contains(s, StringComparison.Ordinal))) { - var trimmed = line.TrimStart(); + return false; + } - if (trimmed.StartsWith("//", StringComparison.Ordinal) - || trimmed.StartsWith("*", StringComparison.Ordinal) - || trimmed.StartsWith("/*", StringComparison.Ordinal)) - { - return false; - } + var code = CSharpSourceWalker.StripCommentsAndStrings(text); + + if (AuroraSurfaces.Any(s => code.Contains(s, StringComparison.Ordinal))) + { + return true; + } - return AuroraSurfaces.Any(s => line.Contains(s, StringComparison.Ordinal)); - }); + return CSharpSourceWalker.StringLiteralBodies(text) + .Any(body => AuroraSurfaces.Any(s => body.Text.Contains(s, StringComparison.Ordinal))); + } private static string RepoRoot() { diff --git a/Lite.Tests/Lite.Tests.csproj b/Lite.Tests/Lite.Tests.csproj index b3fbaa555a..24fe20d103 100644 --- a/Lite.Tests/Lite.Tests.csproj +++ b/Lite.Tests/Lite.Tests.csproj @@ -17,6 +17,18 @@ + + + + +