diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9cf2c8c3..639643d9 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 \ frontend/roslyn/samples/AppDomainShutdownSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" @@ -345,6 +346,24 @@ 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; } + # 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 163a6944..03d3d6fd 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -483,6 +483,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, @@ -2215,6 +2249,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) + && 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()) { // a `static` IDisposable field is a process-lifetime singleton (a shared @@ -2233,6 +2282,15 @@ or ImplicitObjectCreationExpressionSyntax { if (!constructed.Contains(v.Identifier.Text)) continue; + // 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 { @event = v.Identifier.Text, diff --git a/frontend/roslyn/samples/EventSourceCountersSample.cs b/frontend/roslyn/samples/EventSourceCountersSample.cs new file mode 100644 index 00000000..b2ac5868 --- /dev/null +++ b/frontend/roslyn/samples/EventSourceCountersSample.cs @@ -0,0 +1,80 @@ +using System; +using System.Diagnostics.Tracing; +using System.IO; +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(); + + // 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); + + 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", + }; + + // 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); + } +} + +internal sealed class OwnedScratch : IDisposable +{ + public void Dispose() { } +}