Census viewer fan-outs by shape rather than by their join (#3019) - #3023
Conversation
EveryViewerFanOut_DeclaresItsWidth iterated Task.WhenAll matches and added offenders only inside that loop, so a fan-out that never joins contributed zero iterations and could not be reported. The census now recognises three spellings of "N reads in flight together" — the join, two or more fire-and-forget discards, and two or more deferred task starts with no await between them — and pairs each against a declaration in the same MEMBER BODY. Member-body pairing replaces the per-file positional count, which let declarations launder across unrelated members: MainWindow carries three declarations against one join, so the declaration that actually paired with that join could be deleted and the two unrelated unjoined ones covered for it. The widened census reports four members that fan out without declaring a width. Three spell it as deferred task starts and each already carried a doc comment saying the reads run concurrently; the fourth fires two unawaited reads. NoUnmodelledConcurrencyPrimitive_ReachesTheViewer pins WhenAny, Parallel, Task.Factory and ContinueWith absent, so a fan-out spelled with one of those goes red here rather than silently widening the gap.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed the diff against Production call sites ( Test-suite classifier ( No security, injection, or secrets concerns — this is local static analysis over source text plus in-process |
The discard scan is unconstrained about what it discards while the deferred scan requires an Async suffix. Requiring the suffix on discards is not available: OpenPlanTab, OnHeatmapDrillDown and PlanViewerController.LoadPlanIntoSubTab are async without it, so the suffix rule would blind the census to three of the project's fifteen discarded call names. The cost of leaving it unconstrained is that two discarded synchronous helpers in one member read as a two-wide fan-out. That shape is now a fixture asserting the classifier's actual behaviour, so it reads as a known cost rather than a surprise to whoever first writes one. Also records the remaining under-reporting gap: an await nested in a lambda between two deferred starts breaks the run.
|
Reviewed. This is Darling-only C# test/lint infrastructure plus four production call sites ( Verified against the actual production changes: the four newly-declared widths ( Traced through the new classifier logic (
Neither blocks the fix for #3019 itself, which is correctly targeted (member-body pairing replacing the per-file positional count, plus the two previously-uncensused shapes). |
The member walk required only whitespace between the parameter list and the body, and a bare name before it. A generic method satisfies neither: its type parameters sit after the name and its constraints sit before the body. Three members were dropped from the walk entirely -- ViewerSettingsFile.Load<T> and Save<T>, SettingsWindow.TryReadSectionAsync<T>. None holds a fan-out marker, so the census stayed green while seeing less than it claimed, and the first one to grow a marker would have tripped the unattributed assertion instead of being reported as an offender. The two halves are interdependent, measured on the project: admitting the constraint clause alone gains nothing, type parameters alone gain one member, and both together gain three. The type-parameter group excludes = and newlines deliberately. Allowing them ran the pattern from a field's declared type through its initializer to the collection initializer's brace, reading CollectorSchedulePresets.Presets as a method whose body is the initializer. A phantom body is worse than a missing one, since Owner takes the outermost match and could attribute a real member's markers to something that can never declare a width. Both properties are pinned. The generic fixture spells its constraint without parentheses on purpose: where T : class, new() contains a ), the parameter-list group is greedy, and it absorbs the whole clause -- so a fixture written that way passes with constraint support removed. That layout is pinned separately instead. Separately, `var _ = SomeAsync();` satisfied both read shapes, so one call could be counted into two fan-out tallies. A bare _ is now excluded from the deferred name, leaving the call to the discard shape, which is what it is: nothing holds the task. Member bodies go 1,231 to 1,234; the fan-out census is unchanged at 21.
|
Reviewed the diff (test-only classifier rewrite in Correctness — Walked the three regexes ( Cross-checked the four newly-declared widths against the actual task counts in each member:
One minor/non-blocking note on that last one: the Lite/Darling parity — No parity concern. Security / T-SQL style — N/A, no SQL or external input handling in this diff. Performance — None of the production changes alter behavior; they only widen an No blocking issues found. |
The markers are matched against the file and attributed afterwards, so a marker no member body owns is reported rather than skipped. Scoped the other way -- searched within each braced body -- a marker in a shape the walk cannot represent would never be looked at, and the unattributed count would sit at zero vacuously while covering less. That distinction decides an expression-bodied join, which is a legal fan-out site with no braced body at all: this project has 378 expression-bodied members against 1,234 braced ones. None holds a join today, and the census does not depend on that staying true. The new pin asserts the reporting path rather than coverage: the join is matched, nothing owns it, so the sweep lists it. The guard's summary now says why the tree-wide scan is what makes its unattributed assertion mean anything.
|
Reviewed. This is entirely test-tooling + Darling-only production wiring (no T-SQL, no Lite files touched), and Correctness
Security / performance: no external input, no SQL, no perf-sensitive path — this only runs at test time over local source files. Solid, unusually well-verified PR (the mutation-testing table in the description is a good sign). Nothing blocking. |
s_deferredRead ends on the call's opening paren, so the window ConcurrentRun scanned for an intervening await began inside the first call and covered the rest of its own argument list. An await in those arguments -- var t1 = FooAsync(await Bar()); -- broke the run and dropped a real fan-out silently, which is this pin's own failure class inside the detector built for it. The window was wrong at all 63 deferred sites in the project, not at an edge case; no site currently puts an await in its arguments, so the census is unchanged at 21 fan-outs and 0 offenders either way. DeferredReads now carries the end of the STATEMENT, via the paren-balanced EndOfParenthesisedStatement this file already used for the join scan, so a lambda argument cannot terminate it early. The sweep and its fixtures share that helper deliberately: a fixture computing the window differently from the sweep would pin a property the sweep does not have. Measuring from the statement end introduces an ordering hazard the match end did not have. A deferred read nested inside an earlier statement -- a task started in a lambda handed to another started task -- puts the previous end PAST the next start, and the naive slice throws rather than mis-reporting. The window is clamped and the previous end kept monotonic. No site is written that way today. Three pins rather than one, because measuring from the wrong end satisfies half the property: an await inside the first call's arguments must still count as a fan-out, an await genuinely between the two statements must still break the run, and the nested case must return at all.
|
Reviewed. This is a Darling-only, test-infra-heavy change (622 of 639 lines are in Lite/Darling parity: No concern. Regex/census logic: Traced the three new shape detectors (
No correctness bugs found in the detection logic. Production one-liners (the actual behavior fix):
No security, injection, or missing-index concerns — nothing here touches SQL or external input. |
1 participant
Closes #3019. Closing keywords are a no-op on a
devmerge, so this is stated rather than relied on.The defect
EveryViewerFanOut_DeclaresItsWidthiteratedTask.WhenAllmatches and added offenders only inside that loop. A fan-out that never joins therefore contributed zero iterations and could not be reported however wrong it was — while being the very shape whose discovery took #3007's census from one site to seventeen. The guard's claim was "every joined fan-out", and its name said otherwise.What changed
A fan-out is N reads in flight together, not a syntax. This project spells that three ways, and the census now recognises all three:
await Task.WhenAll(...). One occurrence makes the member a fan-out._ = SomethingAsync();. One is the project's ordinary single-flight-guarded refresh (27 such sites, none of them fan-outs); two is a fan-out, because a discarded task has nobody to await it and is still running when the next starts. That is also why anawaitbetween two discards does not separate them.var t = SomethingAsync();starts with noawaitbetween them. Semantically the joined form with the awaits written one per line.Each is paired against a declaration in the same member body, which replaces the per-file positional count. That count let declarations launder across unrelated members:
MainWindow.xaml.cscarries several declarations against one join, so the declaration that actually paired with that join could be deleted while two unrelated unjoined ones covered for it. Member-body pairing is also immune to the nesting that defeats same-block pairing —CorrelatedTimelineLanesControldeclares its width in an outertryand awaits the join in a nested one, and both are the same member.NoUnmodelledConcurrencyPrimitive_ReachesTheViewerpinsTask.WhenAny,Parallel.For/Invoke,Task.FactoryandContinueWithabsent from the project. That is what lets the census claim "every" rather than "every one of three spellings": a fan-out spelled with one of those goes red here rather than quietly widening the gap.The widened census reports four undeclared fan-outs
These are live, not latent. Three spell the fan-out as deferred task starts and each already carried a doc comment stating the reads run concurrently; the fourth fires two unawaited reads.
ViewerServerTab.Blocking.LoadBlockingAsyncswitchcasesViewerServerTab.Charts.LoadTempDbAsyncViewerServerTab.RunningJobs.LoadRunningJobsAsyncMainWindow.SettingsButton_ClickEach now declares its width. The blocking cases take braces so each declares its own — the branches are mutually exclusive, and one method-level declaration would over-declare for the two branches that read solo, which
Of()'s own summary calls the one way to misuse it. The braces are required, not stylistic: ausing vardirectly in an unbracedswitchsection is CS8647, and the unbraced cases had also been sharing one declaration space. Behaviour is unchanged.The failure direction was safe throughout: an undeclared fan-out clamps to
Math.Clamp(..., 1, ...), the solo floor, which is what these reads already got. So nothing regressed — the reads were priced against a ceiling derived from a read measured alone while running three-wide.What the census does NOT cover
Stated in the guard's own summary as well, because each gap is real:
LoadBlockingAsync's three declarations leaves this pin green, which is why all three are declared by hand rather than left to the guard to demand.awaitbetween two starts, including one nested inside a lambda rather than sequencing the starts at member level. That direction under-reports — the same direction as the defect itself — and is left because the joined and fire-and-forget rules overlap it and no member is written that way today.Asyncsuffix. Verified to hold for all 55Task-returningViewerDataServicemembers, so it is a convention the rule leans on rather than an assumption about one file.NoFanOutScope_OutlivesItsJoinremains join-only and now says so. "Outlives its join" needs a join to measure against, andMainWindow.OnRefreshTimerTickdeliberately holds its scope to the end of the tick because the visible-tab load below really does contend with reads still in flight — so widening that scan would report a deliberate choice as a defect.The fire-and-forget shape is unconstrained about its callee, on purpose
Raised in review. The discard scan matches
_ = <anything>(...)while the deferred scan requires anAsyncsuffix. Requiring the suffix on discards is not available:OpenPlanTab(private async Task),OnHeatmapDrillDownandPlanViewerController.LoadPlanIntoSubTabare genuinely async without it, so the suffix rule would blind the census to three of the project's fifteen discarded call names — the same silent gap this PR exists to close.The cost paid instead is a false-positive path:
_ =compiles for any non-void call, so two discarded synchronous helpers in one member read as a two-wide fan-out. Nothing in the Viewer does that today (every_ = X(...)site is Task-returning). The shape is now a fixture asserting the classifier's actual behaviour, so it reads as a known cost rather than a surprise, and the trade is argued on the regex itself.The member walk was missing generic methods, and the two read shapes overlapped
Both raised in review, both fixed rather than documented, because both are the defect this PR exists to close reproduced one level up — a census that cannot see part of what it sweeps, staying green because that part happens to be empty.
Generic methods. The walk required a bare name before the parameter list and only whitespace before the body. A generic method satisfies neither: type parameters sit after the name, constraints sit before the body. Three members were dropped entirely —
ViewerSettingsFile.Load<T>andSave<T>,SettingsWindow.TryReadSectionAsync<T>. The two halves are interdependent, measured on the project: admitting the constraint clause alone gains nothing, type parameters alone gain one, both together gain three. Member bodies go 1,231 → 1,234; the fan-out census is unchanged at 21.The type-parameter group excludes
=and newlines deliberately. Allowing them ran the pattern from a field's declared type, through= new Dictionary<...>(comparer), to the collection initializer's brace — readingCollectorSchedulePresets.Presets, a field, as a method whose body is the initializer. A phantom body is worse than a missing one:Ownertakes the outermost match, so one could swallow a real member's markers and report a fan-out at a line where no edit makes the build pass. Pinned.Overlapping shapes.
var _ = SomeAsync();satisfied both the discard and deferred patterns, so one physical call could be counted into two fan-out tallies. A bare_is now excluded from the deferred name, leaving the call to the discard shape — which is what it is, since nothing holds the task.One of these pins first passed for the wrong reason and a mutation caught it: the generic fixture was spelled
where T : class, new(), whose)the greedy parameter-list group absorbs, so it matched even with constraint support removed. The real member is paren-freewhere T : class. The fixture now uses that shape, and thenew()layout is pinned separately.Why
unattributed: 0is coverage rather than silenceThe markers are matched against the file and attributed afterwards, not searched for inside each braced body. A marker nothing owns is therefore reported, not skipped. Scoped the other way round, a marker in a shape the walk cannot represent would never be looked at and the count would sit at zero vacuously while covering less.
That decides the expression-bodied join —
Task Both() => Task.WhenAll(a, b);is a legal fan-out with no braced body at all, and this project has 378 expression-bodied members against 1,234 braced ones, so the shape is ordinary rather than exotic. None holds a join today, and the census does not depend on that staying true: planting one produces a red build naming its line. Pinned byTheCensus_ReportsAJoinInAMemberWithNoBracedBody, which asserts the reporting path — the join is matched, nothing owns it, so the sweep lists it.The load-bearing part is demonstrated rather than argued: planting an expression-bodied join and neutering the unattributed reporting leaves the suite green with an undeclared fan-out in the tree, which is exactly the vacuous zero. With reporting intact the same plant is red.
The deferred window was measured from the wrong end
s_deferredReadends on the call's opening paren, so the windowConcurrentRunscanned for an interveningawaitbegan inside the first call and covered the rest of its own argument list. Anawaitin those arguments —var t1 = FooAsync(await Bar());— broke the run and dropped a real fan-out silently. That is this pin's own failure class inside the detector built for it, and the window was wrong at all 63 deferred sites in the project rather than at an edge case. No site currently puts anawaitin its arguments, so the census is unchanged at 21 fan-outs and 0 offenders under either window.DeferredReadsnow carries the end of the statement, via the paren-balancedEndOfParenthesisedStatementthis file already used for the join scan, so a lambda argument cannot terminate it early. The sweep and its fixtures share that helper deliberately: a fixture computing the window differently from the sweep would pin a property the sweep does not have.Measuring from the statement end introduces an ordering hazard the match end did not have. A deferred read nested inside an earlier statement — a task started in a lambda handed to another started task — puts the previous end past the next start, and the naive slice throws rather than mis-reporting. The window is clamped and the previous end kept monotonic.
Three pins rather than one, because measuring from the wrong end satisfies half the property, and the two halves must fail separately.
Verification
Red-first, one mutation at a time, each restored byte-identical via
redproof.shwith a content witness:MainWindow's only joinTask.WhenAny(=into the type-parameter group_exclusionawaitThe third mutation is the positional-residual evidence: run under the old k-th-join rule it reports green (two declarations precede the only join, and the rule required one), and under the new rule it is red.
The Windows-only suite cannot run on macOS, so the real
ViewerCommandTimeoutTests.cswas compiled into a throwawaynet10.0xunit v3 host with<AssemblyName>Darling.Tests</AssemblyName>, staged outside the tree,<Compile Include>-ing the real repo paths alongside the realViewerCommandDeadlines.csandViewerReadFanOut.cs. Counts moved 32 → 42 across the change, so the assembly was not stale.ViewerFleetTimerFanOutPositionTests,ViewerFleetTimerGuardTests,CommandDeadlineScannerAdoptionTestsandFleetIdentifierScrubTestswere run against the same tree (52 total) because all four read files this diff touches. CI is the arbiter for the rest.CHANGELOG entry (not committed — for the coordinator to place)