From 72b6927bc53482f53300d27866c774c2c961b001 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 22 Jun 2026 14:01:42 +0000 Subject: [PATCH 1/2] fix(extractor): exempt EventSource-owned DiagnosticCounter fields from the field-disposable leak (mined Npgsql) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An EventSource's diagnostic counters — EventCounter / PollingCounter / IncrementingEventCounter / IncrementingPollingCounter — are constructed with `this` (the parent EventSource) as their owner argument. That registration hands the counter to the source: the runtime's CounterGroup pins it to the EventSource's lifetime, and an EventSource is a process-lived `static readonly Log` singleton, so a counter is a process-lived diagnostic that is idiomatically NEVER field-disposed (every BCL EventSource — RuntimeEventSource, the HTTP/ASP.NET counters — does the same). Reporting such a field as an undisposed IDisposable was a false positive. Mined: Npgsql's NpgsqlEventSource builds eight counters in OnEventCommand, none field-disposed — eight OWN001 FPs cleared. Scope (narrow, sound): the field is skipped only when the containing type derives from System.Diagnostics.Tracing.EventSource AND the field is assigned `new (... this ...)`. A counter is always built in a method body (OnEventCommand) — `this` is unavailable in a field initializer — so the assignment shape (matching the `constructed` set) is the only one that can match. Regression sample EventSourceCountersSample.cs: SampleEventSource's four counters stay SILENT; the Codex control `_scratch` — a plain owned IDisposable in the SAME EventSource — STILL raises the OWN001 disposable-field leak, proving the exemption keys off the DiagnosticCounter type handed to `this`, not the EventSource class. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 13 ++++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 53 +++++++++++++++ .../samples/EventSourceCountersSample.cs | 68 +++++++++++++++++++ 3 files changed, 134 insertions(+) create mode 100644 frontend/roslyn/samples/EventSourceCountersSample.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d17609aa..c109e758 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -142,6 +142,7 @@ jobs: frontend/roslyn/samples/ResolvedDisposableSample.cs \ frontend/roslyn/samples/FieldReleaseSample.cs \ frontend/roslyn/samples/StaticClassEscapeSample.cs \ + frontend/roslyn/samples/EventSourceCountersSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" - name: Check facts through the core @@ -330,6 +331,18 @@ jobs: if echo "$out" | grep -q "StaticAllocationCounter"; then echo "FAIL: a static-method handler on a static event was wrongly reported as a region escape"; exit 1 fi + # P-004 EventSource diagnostic-counter exemption (mined: Npgsql NpgsqlEventSource): a + # DiagnosticCounter (EventCounter / PollingCounter / Incrementing{Event,Polling}Counter) + # built with `this` is registered to the parent EventSource and shares its process + # lifetime -> idiomatically never field-disposed -> must NOT be flagged as an undisposed leak. + if echo "$out" | grep -qE "'_bytesPerSecond'|'_totalBytes'|'_commandDuration'|'_totalCommands'"; then + echo "FAIL: an EventSource-owned DiagnosticCounter field was wrongly reported as an undisposed leak"; exit 1 + fi + # scope control (Codex): the exemption keys off the DiagnosticCounter type handed to + # `this`, NOT the EventSource class — so a plain owned IDisposable field in the same + # EventSource (`_scratch`) must STILL raise the OWN001 disposable-field leak. + echo "$out" | grep -qE "EventSourceCountersSample\.cs:[0-9]+:.*\[OWN001\].*'_scratch'" \ + || { echo "FAIL: a non-counter owned IDisposable field in an EventSource must still warn (exemption stays counter-type-scoped)"; exit 1; } # P-004 process-lived-subscriber exemption (mined: ScreenToGif App + # Translator): the WPF `App` singleton hooking the process-lived # AppDomain.UnhandledException promotes nothing, so the static-source region diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index a716e00c..2c2a7ba9 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -428,6 +428,40 @@ static bool DerivesFromWinFormsForm(ITypeSymbol? t) return false; } +// A type that derives from System.Diagnostics.Tracing.EventSource (semantic, walks the +// base chain). An EventSource is the canonical process-lived `static readonly Log` +// diagnostics singleton; its DiagnosticCounter fields are owned by it (IsEventSourceOwnedCounter). +static bool DerivesFromEventSource(ITypeSymbol? t) +{ + for (var b = t; b is not null; b = b.BaseType) + if (b.Name == "EventSource" && b.ContainingNamespace?.ToString() == "System.Diagnostics.Tracing") + return true; + return false; +} + +// A type that derives from System.Diagnostics.Tracing.DiagnosticCounter — the abstract base of +// EventCounter / IncrementingEventCounter / PollingCounter / IncrementingPollingCounter. +static bool DerivesFromDiagnosticCounter(ITypeSymbol? t) +{ + for (var b = t; b is not null; b = b.BaseType) + if (b.Name == "DiagnosticCounter" && b.ContainingNamespace?.ToString() == "System.Diagnostics.Tracing") + return true; + return false; +} + +// P-004 EventSource-owned diagnostic counter. A DiagnosticCounter created with `this` — the +// parent EventSource — as its owner argument REGISTERS the counter with that source: the +// runtime's CounterGroup pins it to the EventSource's lifetime, and an EventSource is a +// process-lived `static readonly Log` singleton, so the counter is a process-lived diagnostic +// that is idiomatically NEVER field-disposed (every BCL EventSource — RuntimeEventSource, the +// ASP.NET / HttpConnection counters — does exactly this). Reporting such a field as an +// undisposed leak is therefore a false positive. Mined: Npgsql's NpgsqlEventSource (eight +// counters built in OnEventCommand, none field-disposed). +static bool IsEventSourceOwnedCounter(BaseObjectCreationExpressionSyntax oce, SemanticModel model) => + oce.ArgumentList is { } args + && args.Arguments.Any(arg => arg.Expression is ThisExpressionSyntax) + && DerivesFromDiagnosticCounter(model.GetTypeInfo(oce).Type); + static string MethodName(BaseMethodDeclarationSyntax m) => m switch { MethodDeclarationSyntax md => md.Identifier.Text, @@ -2152,6 +2186,21 @@ or ImplicitObjectCreationExpressionSyntax && mb.Name.Identifier.Text is "Dispose" or "DisposeAsync") disposed.Add(cdf); + // P-004 EventSource counter exemption: inside an EventSource, a DiagnosticCounter field + // constructed with `this` is registered to (and lifetime-owned by) the source — a + // process-lived diagnostic the source never field-disposes (see IsEventSourceOwnedCounter). + // Collect those field names so the loop below does not report them as undisposed leaks. + // A counter is always built in a method body (OnEventCommand) — `this` is unavailable in + // a field initializer — so only the assignment shape (matching `constructed`) can match. + var eventSourceCounters = new HashSet(); + if (DerivesFromEventSource(clsSymbol)) + foreach (var a in assigns) + if (a.IsKind(SyntaxKind.SimpleAssignmentExpression) + && a.Right is BaseObjectCreationExpressionSyntax aoce + && IsEventSourceOwnedCounter(aoce, model) + && FieldName(a.Left) is { } cfn) + eventSourceCounters.Add(cfn); + foreach (var fd in cls.Members.OfType()) { // a `static` IDisposable field is a process-lifetime singleton (a shared @@ -2170,6 +2219,10 @@ or ImplicitObjectCreationExpressionSyntax { if (!constructed.Contains(v.Identifier.Text)) continue; + // an EventSource's own DiagnosticCounter (registered to `this`) is process-lived + // and never field-disposed -> not an owned leak (mined: Npgsql NpgsqlEventSource). + if (eventSourceCounters.Contains(v.Identifier.Text)) + continue; subs.Add(new { @event = v.Identifier.Text, diff --git a/frontend/roslyn/samples/EventSourceCountersSample.cs b/frontend/roslyn/samples/EventSourceCountersSample.cs new file mode 100644 index 00000000..ef1fff6b --- /dev/null +++ b/frontend/roslyn/samples/EventSourceCountersSample.cs @@ -0,0 +1,68 @@ +using System; +using System.Diagnostics.Tracing; +using System.Threading; + +namespace Own.Samples; + +// P-004 EventSource diagnostic-counter exemption (mined: Npgsql NpgsqlEventSource). A +// DiagnosticCounter — EventCounter / PollingCounter / IncrementingEventCounter / +// IncrementingPollingCounter — constructed with `this` (the parent EventSource) registers +// itself with that source: the runtime's CounterGroup pins it to the EventSource's lifetime, +// and an EventSource is a process-lived `static readonly Log` singleton, so the counter is a +// process-lived diagnostic that is idiomatically NEVER field-disposed (every BCL EventSource — +// RuntimeEventSource, the HTTP/ASP.NET counters — does exactly this). The counter fields must +// therefore be SILENT. Contrast `_scratch` below: a plain owned IDisposable in the SAME +// EventSource STILL leaks, proving the rule keys off the DiagnosticCounter type handed to +// `this`, not "any field declared in an EventSource". +[EventSource(Name = "Own-Sample-Counters")] +internal sealed class SampleEventSource : EventSource +{ + public static readonly SampleEventSource Log = new SampleEventSource(); + + private long _bytesWritten; + + // The four counter kinds, all built in OnEventCommand with `this` -> owned by the source -> SILENT. + private IncrementingPollingCounter? _bytesPerSecond; + private PollingCounter? _totalBytes; + private EventCounter? _commandDuration; + private IncrementingEventCounter? _totalCommands; + + // Codex control: a NON-counter owned IDisposable field in the same EventSource is NOT exempt. + // The exemption keys off the DiagnosticCounter type handed to `this`, not the containing class, + // so this new'd-but-never-disposed resource STILL raises OWN001 ('disposable field'). + private readonly OwnedScratch _scratch = new OwnedScratch(); + + private SampleEventSource() { } + + public void RecordBytes(long n) => Interlocked.Add(ref _bytesWritten, n); + + protected override void OnEventCommand(EventCommandEventArgs command) + { + if (command.Command != EventCommand.Enable) + return; + + _bytesPerSecond = new IncrementingPollingCounter("bytes-per-second", this, () => Interlocked.Read(ref _bytesWritten)) + { + DisplayName = "Bytes Written Rate", + DisplayRateTimeScale = TimeSpan.FromSeconds(1), + }; + _totalBytes = new PollingCounter("total-bytes", this, () => Interlocked.Read(ref _bytesWritten)) + { + DisplayName = "Total Bytes Written", + }; + _commandDuration = new EventCounter("command-duration", this) + { + DisplayName = "Command Duration", + DisplayUnits = "ms", + }; + _totalCommands = new IncrementingEventCounter("total-commands", this) + { + DisplayName = "Total Commands", + }; + } +} + +internal sealed class OwnedScratch : IDisposable +{ + public void Dispose() { } +} From 08c11e4e1f860732ac7e146013584c713644f74e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 22 Jun 2026 14:10:07 +0000 Subject: [PATCH 2/2] fix(extractor): bind the EventSource-counter exemption to genuine counter fields (Codex P2 + CodeRabbit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on the over-broad first cut, both the over-suppression class: - Codex P2 — require the field's DECLARED type to derive from DiagnosticCounter before skipping. A field declared as a plain IDisposable that is merely assigned a counter once (`IDisposable _mixed = new MemoryStream(); _mixed = new EventCounter(..., this);`) would otherwise have its OTHER resource (the MemoryStream) silently suppressed by the name-only skip. The declared-type guard binds the exemption to actual counter fields (Npgsql declares them as `IncrementingPollingCounter?` etc.), so the non-counter leak still fires. - CodeRabbit (major) — populate the exemption set with ThisFieldName, not FieldName: the latter also matches `other._counter`, which could exempt this class's same-named field from another instance's assignment. Tightening a suppression to THIS instance's field (`_f` / `this._f`) is strictly safer. Regression: EventSourceCountersSample gains `_mixed` — declared IDisposable, initialized `new MemoryStream()` and later assigned `new EventCounter(..., this)` — which must STILL raise OWN001 on the MemoryStream. The four genuine counter fields stay SILENT; `_scratch` (non-counter) and `_mixed` (non-counter-declared) both warn. CI asserts all three. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 6 ++++++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 13 +++++++++---- .../roslyn/samples/EventSourceCountersSample.cs | 12 ++++++++++++ 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c109e758..0fba5469 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -343,6 +343,12 @@ jobs: # EventSource (`_scratch`) must STILL raise the OWN001 disposable-field leak. echo "$out" | grep -qE "EventSourceCountersSample\.cs:[0-9]+:.*\[OWN001\].*'_scratch'" \ || { echo "FAIL: a non-counter owned IDisposable field in an EventSource must still warn (exemption stays counter-type-scoped)"; exit 1; } + # declared-type control (Codex): a field DECLARED as a plain IDisposable that is ALSO + # assigned a counter once (`_mixed = new EventCounter(..., this)`) must STILL leak its + # earlier `new MemoryStream()` — the exemption requires the DECLARED field type to be a + # DiagnosticCounter, so a name-only skip cannot hide the non-counter resource. + echo "$out" | grep -qE "EventSourceCountersSample\.cs:[0-9]+:.*\[OWN001\].*'_mixed'" \ + || { echo "FAIL: a field declared as a non-counter IDisposable that is later assigned a counter must still leak (exemption requires the DECLARED type to be a DiagnosticCounter)"; exit 1; } # P-004 process-lived-subscriber exemption (mined: ScreenToGif App + # Translator): the WPF `App` singleton hooking the process-lived # AppDomain.UnhandledException promotes nothing, so the static-source region diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 2c2a7ba9..4e583b3e 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -2198,7 +2198,7 @@ or ImplicitObjectCreationExpressionSyntax if (a.IsKind(SyntaxKind.SimpleAssignmentExpression) && a.Right is BaseObjectCreationExpressionSyntax aoce && IsEventSourceOwnedCounter(aoce, model) - && FieldName(a.Left) is { } cfn) + && ThisFieldName(a.Left) is { } cfn) // THIS instance's field only — `other._c` must not exempt our field (CodeRabbit) eventSourceCounters.Add(cfn); foreach (var fd in cls.Members.OfType()) @@ -2219,9 +2219,14 @@ or ImplicitObjectCreationExpressionSyntax { if (!constructed.Contains(v.Identifier.Text)) continue; - // an EventSource's own DiagnosticCounter (registered to `this`) is process-lived - // and never field-disposed -> not an owned leak (mined: Npgsql NpgsqlEventSource). - if (eventSourceCounters.Contains(v.Identifier.Text)) + // an EventSource's own DiagnosticCounter (a field DECLARED as a DiagnosticCounter + // AND registered to `this`) is process-lived and never field-disposed -> not an + // owned leak (mined: Npgsql NpgsqlEventSource). The DECLARED-type guard keeps the + // skip bound to genuine counter fields: a field declared as a plain IDisposable + // that is merely assigned a counter once still leaks its OTHER resource (e.g. an + // initial `new MemoryStream()`), so it must NOT be suppressed by name alone (Codex). + if (eventSourceCounters.Contains(v.Identifier.Text) + && DerivesFromDiagnosticCounter(model.GetTypeInfo(fd.Declaration.Type).Type)) continue; subs.Add(new { diff --git a/frontend/roslyn/samples/EventSourceCountersSample.cs b/frontend/roslyn/samples/EventSourceCountersSample.cs index ef1fff6b..b2ac5868 100644 --- a/frontend/roslyn/samples/EventSourceCountersSample.cs +++ b/frontend/roslyn/samples/EventSourceCountersSample.cs @@ -1,5 +1,6 @@ using System; using System.Diagnostics.Tracing; +using System.IO; using System.Threading; namespace Own.Samples; @@ -32,6 +33,12 @@ internal sealed class SampleEventSource : EventSource // so this new'd-but-never-disposed resource STILL raises OWN001 ('disposable field'). private readonly OwnedScratch _scratch = new OwnedScratch(); + // Codex control: a field DECLARED as a plain IDisposable that is ALSO assigned a counter once + // must STILL leak. The exemption requires the DECLARED field type to be a DiagnosticCounter, so + // the `new MemoryStream()` owned here is not hidden by the later `_mixed = new EventCounter(...)` + // (a name-only skip would wrongly suppress the whole field). -> OWN001 on the MemoryStream. + private IDisposable _mixed = new MemoryStream(); + private SampleEventSource() { } public void RecordBytes(long n) => Interlocked.Add(ref _bytesWritten, n); @@ -59,6 +66,11 @@ protected override void OnEventCommand(EventCommandEventArgs command) { DisplayName = "Total Commands", }; + + // The `_mixed` field (declared IDisposable) is ALSO assigned a counter here. The exemption + // must NOT suppress it: its declared type is not a DiagnosticCounter, so its earlier + // `new MemoryStream()` is a real undisposed leak (Codex). + _mixed = new EventCounter("mixed-counter", this); } }