From 9cfa94f0e5454698fce16face6e006a8bf99839b Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 11 Aug 2026 14:08:12 -0400 Subject: [PATCH] #2190: count openings per doc run, not closing-tag pairs NoMemberCarriesTwoStackedSummaryBlocks keyed off a immediately followed by a reopening, so it saw a stacked pair only when the FIRST block was closed. Two instances sat on dev unseen, and the mixed form (a single-line summary followed by a multi-line one) is invisible to a closing-tag matcher entirely. The rule now counts openings inside each contiguous run of /// lines. A run documents exactly one member, so two openings means two summaries, closed or not, single-line or spread over many. Both live instances fixed: - DarlingManagedPostgres.ApplyProcessEnvironment carried a duplicated opening tag; the stray line is deleted. - QueryStoreSliceRepairService had a doc block that bb1f48c0 split, stranding its unclosed head above PromoteRewrittenFileAsync. Deleted rather than moved back onto FlushExternalFileCacheAsync: that same commit re-documented that member in place with a superset of the stranded sentence, so moving it would have recreated the duplicate this rule forbids. The detector gains a self-test pinning the five stacked shapes it must catch and the four legitimate ones it must leave alone, since this was a blind spot in the detector rather than in anyone's reading of the tree. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 2 + .../Darling.Tests/DocCommentHygieneTests.cs | 112 ++++++++++++++++-- .../DarlingManagedPostgres.cs | 1 - Lite/Services/QueryStoreSliceRepairService.cs | 3 - 4 files changed, 104 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9ff31fba..479de2335 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **The doc-comment hygiene pin now catches a stacked summary whose first block is never closed** ([#2190]) - `NoMemberCarriesTwoStackedSummaryBlocks` keyed off a `` immediately followed by a reopening, so it only ever saw a stacked pair when the FIRST block was closed, and two instances were sitting on dev unseen: a duplicated opening tag on `ApplyProcessEnvironment` in the Darling service, and a doc block that #1912's restructure split in `QueryStoreSliceRepairService`, stranding its unclosed head on top of `PromoteRewrittenFileAsync`. The rule now counts `` OPENINGS inside each contiguous run of `///` lines - a run documents exactly one member, so two openings means two summaries whether or not either is closed, and whether they are written single-line or spread over many. That mixed form is the one a closing-tag matcher cannot see at all, and it is what defeated the first attempt at this fix. Both live instances are repaired: the duplicate tag is deleted, and the stranded head is deleted rather than moved back, because the same restructure had already re-documented `FlushExternalFileCacheAsync` in place with a superset of that one sentence, so moving it would have recreated the exact duplicate this rule exists to forbid. The detector now carries its own self-test as well, pinning the five stacked shapes it must catch and the four legitimate ones it must leave alone, since this was a blind spot in the DETECTOR rather than in anyone's reading of the tree. - **The store's compressed plan format is now documented** ([#2171], reported by @argpna) - execution-plan XML has been gzip-compressed in `query_plan_dim.query_plan_gz` since 3.4.0, with `query_plan_xml` nullable, and the release notes never said so. A consumer reading the store directly over SQL therefore got nothing back for anything collected by a current build, with no clue why. The store section of the Darling README now states the format (gzip, magic `1f 8b`), that `query_plan_xml IS NULL` means read the compressed column rather than "no plan", and the three practical ways to get XML back - ask the product via `get_plan_xml`, decompress client-side, or ship a UDF into your own store if your tooling is SQL-only. In-product paths were never affected; this was a contract change for direct SQL consumers that shipped silently. - **tempdb no longer reports more than 100% used in FinOps Database Sizes** ([#2169], reported by @CatastropheOps) - the used percentage divided in-database usage by the size recorded in `sys.master_files`, which is the size set at configuration time and does not track autogrowth for tempdb. A tempdb that had grown was therefore measured against its startup size and rendered above 100%. The per-database probe now captures the file's current size in the same round trip it already makes for space-used, and the payload prefers it, so both halves of the ratio come from one snapshot; a database whose probe fails still falls back to the old source rather than vanishing from the grid. Affects the on-prem, RDS, and Managed Instance path - the Azure SQL Database path already read both numbers in-database. - **Backing out of Custom Range no longer strands an open calendar - both apps** ([#2154], reported in #2153) - a DatePicker's calendar dropdown is a popup living outside the visual tree's visibility, so collapsing the pickers when the user switched back to a preset range left an already-open calendar floating on screen; the dropdowns now close explicitly alongside the collapse, in Lite's ServerTab and the Darling Viewer's twin alike. @@ -2672,3 +2673,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#2166]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2166 [#2167]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2167 [#2138]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2138 +[#2190]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2190 diff --git a/Darling/Darling.Tests/DocCommentHygieneTests.cs b/Darling/Darling.Tests/DocCommentHygieneTests.cs index 6a892e2c5..ecfc75daf 100644 --- a/Darling/Darling.Tests/DocCommentHygieneTests.cs +++ b/Darling/Darling.Tests/DocCommentHygieneTests.cs @@ -9,6 +9,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Linq; using System.Text.RegularExpressions; using Xunit; @@ -31,6 +32,15 @@ namespace Darling.Tests; /// superseded duplicate safe to simply delete. A blind "remove the extra summary" sweep would have destroyed /// documentation at seven of the eight. /// +/// Counted by openings, not by closing tags (#2190). The first version of this rule keyed off a +/// </summary> immediately followed by a reopening, so it saw a stacked pair only when the FIRST +/// block was closed. Two instances sat on dev unseen: a duplicated opening tag, and a doc block that an +/// insertion had split, stranding its head on the following member with no closing tag near it. Both are +/// caught by counting <summary> OPENINGS inside each contiguous run of /// lines — a run +/// documents exactly one member, so two openings in one run means two summaries, whether or not either is +/// closed and whether they are written single-line or spread over many. The mixed form is the one a +/// closing-tag matcher cannot see at all: a single-line summary followed by a multi-line one. +/// /// Coverage limit, stated rather than assumed. CI path filters are per-project, so this runs on /// any pull request that trips the darling or core filter, and on every nightly and release /// build — but a change touching ONLY Lite or Installer will not run it, and would be caught on the next @@ -41,12 +51,13 @@ namespace Darling.Tests; public sealed class DocCommentHygieneTests { /// - /// A </summary> closed and immediately reopened. Deliberately narrow: it matches only the - /// stacked-block shape, never a legitimate <summary> followed by <param>, - /// <returns> or <remarks>. + /// One <summary> OPENING tag. Counting these per doc run, rather than pairing them against a + /// closing tag, is what lets the rule see an unclosed first block. Still deliberately narrow: a summary + /// followed by <param>, <returns>, <remarks> or any number of + /// <para> blocks is one opening and never matches twice, and an escaped mention in prose + /// (&lt;summary&gt;, as used throughout this very file) is not an opening at all. /// - private static readonly Regex StackedSummary = - new(@"\s*\r?\n\s*///\s*", RegexOptions.Compiled); + private static readonly Regex SummaryOpening = new(@"", RegexOptions.Compiled); [Fact] public void NoMemberCarriesTwoStackedSummaryBlocks() @@ -70,12 +81,13 @@ public void NoMemberCarriesTwoStackedSummaryBlocks() continue; } - var text = File.ReadAllText(file); - foreach (Match match in StackedSummary.Matches(text)) + foreach (var run in StackedSummaryRuns(File.ReadAllLines(file))) { - /* Line number of the match start, so the failure names somewhere you can actually open. */ - var line = text.AsSpan(0, match.Index).Count('\n') + 1; - offenders.Add($"{Path.GetRelativePath(root!, file)}:{line}"); + /* Name the run's first line — somewhere you can actually open — and every opening in it, since + the second one is usually the insertion point that caused the stacking. */ + offenders.Add( + $"{Path.GetRelativePath(root!, file)}:{run.Start} " + + $"( openings at lines {string.Join(", ", run.Openings)})"); } } @@ -86,9 +98,89 @@ public void NoMemberCarriesTwoStackedSummaryBlocks() "an insertion pushed it away from. Seven of the eight found in #1745 were displaced doc blocks " + "whose real member had been left undocumented, and deleting them would have lost the documentation " + "rather than deduplicating it.\n\n" + + "Where an insertion split a block, also check whether the member it came from has since been " + + "re-documented in place. If it has, the stray text is a stranded HEAD rather than the whole block, " + + "and moving it back would create the very duplicate this rule forbids — confirm sentence by " + + "sentence that nothing is lost, then delete it (#2190).\n\n" + string.Join("\n", offenders)); } + /// + /// The rule's own self-test. #2190 was a blind spot in the DETECTOR rather than in anyone's reading of the + /// tree, and it was a synthetic case that exposed it — so the shapes this must catch, and the ones it must + /// leave alone, are pinned here instead of being left to whatever the tree happens to contain. Every + /// true case below is a real shape that has appeared in this repo. + /// + [Theory] + /* Closed and immediately reopened: the only shape the pre-#2190 rule could see. */ + [InlineData(true, "/// \n/// A.\n/// \n/// \n/// B.\n/// \nvoid M();")] + /* A duplicated opening tag, first block never closed. */ + [InlineData(true, "/// \n/// \n/// A.\n/// \nvoid M();")] + /* An insertion split a block, stranding its unclosed head above the next member's whole block. */ + [InlineData(true, "/// \n/// A.\n///\n/// \n/// B.\n/// \nvoid M();")] + /* Single-line followed by multi-line — invisible to a closing-tag matcher, since the reopening does not + follow a on its own line. */ + [InlineData(true, "/// A.\n/// \n/// B.\n/// \nvoid M();")] + /* A stacked run reaching end of file with no member under it. Not valid C#, but it pins the scan's + one-past-the-end step: a run that never meets a non-doc line has to be closed, not dropped. */ + [InlineData(true, "/// \n/// A.\n/// \n/// B.")] + /* One summary plus the other doc tags that legitimately follow it. */ + [InlineData(false, "/// \n/// A.\n/// \n/// X.\n/// Y.\nvoid M(int x);")] + /* One summary carrying several blocks, as most of this repo's docs do. */ + [InlineData(false, "/// \n/// A.\n///\n/// B.\n///\n/// C.\n/// \nvoid M();")] + /* Two members, one summary each: the declarations between them end each run. */ + [InlineData(false, "/// A.\nint A;\n/// B.\nint B;")] + /* Escaped mentions in prose are not openings — this very file is full of them. */ + [InlineData(false, "/// \n/// Two <summary> mentions in one <summary> block.\n/// \nvoid M();")] + public void DetectorCountsSummaryOpeningsPerDocRun(bool stacked, string source) + { + var runs = StackedSummaryRuns(source.Split('\n')); + + Assert.True( + runs.Count == (stacked ? 1 : 0), + $"Expected {(stacked ? "one stacked run" : "no stacked run")}, found {runs.Count}, in:\n{source}"); + } + + /// + /// Every contiguous run of /// lines carrying more than one <summary> opening, as the + /// run's first line and the line of each opening. A run ends at the first line that is not a doc comment, + /// which is what ties it to exactly one member: the declaration itself terminates it. + /// + private static List<(int Start, List Openings)> StackedSummaryRuns(string[] lines) + { + var runs = new List<(int Start, List Openings)>(); + + /* 0 means "not currently inside a run"; line numbers reported to a human are 1-based. Iterating one + past the end closes a run that reaches EOF rather than dropping it. */ + var start = 0; + var openings = new List(); + + for (var i = 0; i <= lines.Length; i++) + { + if (i < lines.Length && lines[i].TrimStart().StartsWith("///", StringComparison.Ordinal)) + { + if (start == 0) + { + start = i + 1; + openings = new List(); + } + + openings.AddRange(Enumerable.Repeat(i + 1, SummaryOpening.Matches(lines[i]).Count)); + } + else if (start != 0) + { + if (openings.Count > 1) + { + runs.Add((start, openings)); + } + + start = 0; + } + } + + return runs; + } + /// /// Walks up from the test output directory to the repo root — the directory holding /// PerformanceMonitor.sln. Same walk-up idiom as ThemeParityTests.FindRepoRoot. diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs index 3805f3f8e..ad18a6fe3 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs @@ -2365,7 +2365,6 @@ style of full-pathing every PG tool. */ } } - /// /// /// Applies the optional per-invocation environment and working directory shared by both process /// runners. Values are ADDED to the inherited environment rather than replacing it — a PG tool still diff --git a/Lite/Services/QueryStoreSliceRepairService.cs b/Lite/Services/QueryStoreSliceRepairService.cs index 4d1526feb..70d5c8112 100644 --- a/Lite/Services/QueryStoreSliceRepairService.cs +++ b/Lite/Services/QueryStoreSliceRepairService.cs @@ -573,9 +573,6 @@ union_by_name view keeps reading the un-rewritten file with the #1907 read-side return file.RowsRemoved; } - /// - /// Drops DuckDB's cached view of every external file, by toggling the cache off and back on. - /// /// /// Promotes a rewritten file over the original, and makes the store able to READ it — the two are one /// operation, which is why they live in one helper rather than being two things a caller must remember.