diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e0de5280..56a91db2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -140,6 +140,7 @@ jobs: frontend/roslyn/samples/ViewOwnsVmSample.xaml.cs \ frontend/roslyn/samples/InjectedDcViewSample.xaml.cs \ frontend/roslyn/samples/ResolvedDisposableSample.cs \ + frontend/roslyn/samples/FieldReleaseSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" - name: Check facts through the core @@ -223,6 +224,20 @@ jobs: if echo "$out" | grep -q "HolderWithDisposeOptional"; then echo "FAIL: a dispose-optional (Task/DataTable) field was wrongly flagged"; exit 1 fi + # field release recognition (mined: ImageSharp). #2 null-conditional dispose + # `field?.Dispose()` must be recognized -> silent; the undisposed control still warns. + if echo "$out" | grep -q "DisposesViaConditional"; then + echo "FAIL: a field disposed via null-conditional field?.Dispose() was wrongly flagged"; exit 1 + fi + echo "$out" | grep -q "NeverDisposesField" \ + || { echo "FAIL: an undisposed IDisposable field control must still warn"; exit 1; } + # #3 a pooled FIELD released cross-member (ctor rent + Dispose Return) must be silent; + # the rented-never-returned control still warns. + if echo "$out" | grep -q "pooled buffer 'returnedBuf'"; then + echo "FAIL: a pooled field returned in Dispose was wrongly flagged"; exit 1 + fi + echo "$out" | grep -q "pooled buffer 'leakedBuf'" \ + || { echo "FAIL: a pooled field rented but never returned must still warn"; exit 1; } # WPF004: an ignored `X.Subscribe(...)` result leaks; the captured+ # disposed one stays silent. "ignored" is unique to the WPF004 message. echo "$out" | grep -q "MessengerViewModel.cs" \ diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index aea10637..a94e544e 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -2128,6 +2128,16 @@ or ImplicitObjectCreationExpressionSyntax && m.Name.Identifier.Text is "Dispose" or "DisposeAsync" && FieldName(m.Expression) is { } df) disposed.Add(df); + // Also the NULL-CONDITIONAL form `field?.Dispose()` (a ConditionalAccess whose + // WhenNotNull is the `.Dispose()` invocation, NOT a plain MemberAccess) — the + // dominant disposal shape the match above misses. Mined as an FP across ImageSharp: + // `this.memoryStream?.Dispose()` (ZipExrCompressor/DeflateCompressor/IccDataWriter) + // and the BufferedStreams benchmark's `[GlobalCleanup]` `field?.Dispose()` calls. + foreach (var cae in cls.DescendantNodes().OfType()) + if (FieldName(cae.Expression) is { } cdf + && cae.WhenNotNull is InvocationExpressionSyntax { Expression: MemberBindingExpressionSyntax mb } + && mb.Name.Identifier.Text is "Dispose" or "DisposeAsync") + disposed.Add(cdf); foreach (var fd in cls.Members.OfType()) { @@ -2355,25 +2365,41 @@ or ImplicitObjectCreationExpressionSyntax // tracks local declarations, so field/assignment-backed rents still need // this syntactic pass; the local-declaration rents are skipped below to // avoid double-reporting them (Codex). + // A FIELD-backed pooled buffer is legitimately rented in one member (the ctor) and + // Returned in another (Dispose), so for FIELDS the `pool.Return(field)` is searched + // CLASS-WIDE (a field name is unique to the class, so this cannot cross-mask — unlike + // same-named LOCALS in different methods, which keep the per-member scoping below). + // Mined: ImageSharp BufferedReadStream returns this.readBuffer in Dispose(bool). (An + // INDIRECT release via a lifetime-guard object — SharedArrayPoolBuffer — is left honest: + // a `new X(field)` is NOT assumed to own/return the buffer, since a non-owning view like + // `new ReadOnlyMemory(field)` would otherwise hide a real leak — Codex.) + var fieldReleased = new HashSet(); + foreach (var inv in cls.DescendantNodes().OfType()) + if (inv.Expression is MemberAccessExpressionSyntax rm + && rm.Name.Identifier.Text == "Return" + && inv.ArgumentList.Arguments.Count > 0 + && model.GetSymbolInfo(inv.ArgumentList.Arguments[0].Expression).Symbol is IFieldSymbol rfs) + fieldReleased.Add(rfs.Name); + foreach (var member in cls.Members) { - var rented = new List<(string Name, int Line)>(); + var rented = new List<(string Name, int Line, bool IsField)>(); foreach (var inv in member.DescendantNodes().OfType()) if (IsPoolRent(inv, model)) { - string? name = inv.Parent switch + (string? name, bool isField) = inv.Parent switch { // a local-declaration rent is the flow pass's job under // --flow-locals; skip it here so it is not double-reported. EqualsValueClauseSyntax { Parent: VariableDeclaratorSyntax vd } - => flowLocals ? null : vd.Identifier.Text, + => (flowLocals ? null : vd.Identifier.Text, false), // a field/assignment rent (`_buf = pool.Rent(...)`) is NOT a // flow candidate, so this pass keeps it in both modes. - AssignmentExpressionSyntax asg => FieldName(asg.Left), - _ => null, + AssignmentExpressionSyntax asg => (FieldName(asg.Left), true), + _ => ((string?)null, false), }; if (name != null) - rented.Add((name, LineOf(inv))); + rented.Add((name, LineOf(inv), isField)); } if (rented.Count == 0) continue; @@ -2384,12 +2410,14 @@ or ImplicitObjectCreationExpressionSyntax && inv.ArgumentList.Arguments.Count > 0 && FieldName(inv.ArgumentList.Arguments[0].Expression) is { } rn) returned.Add(rn); - foreach (var (name, line) in rented) + foreach (var (name, line, isField) in rented) subs.Add(new { @event = name, line, - released = returned.Contains(name), + // locals stay per-member; a field is also released if returned/transferred + // anywhere in the class (cross-member ctor-rent + Dispose-return). + released = returned.Contains(name) || (isField && fieldReleased.Contains(name)), resource = "pool", }); } diff --git a/frontend/roslyn/samples/FieldReleaseSample.cs b/frontend/roslyn/samples/FieldReleaseSample.cs new file mode 100644 index 00000000..b8509208 --- /dev/null +++ b/frontend/roslyn/samples/FieldReleaseSample.cs @@ -0,0 +1,48 @@ +using System; +using System.Buffers; +using System.IO; + +namespace Own.Samples; + +// Field release recognition (mined: ImageSharp). Two shapes the field detectors missed: +// #2 null-conditional disposal `field?.Dispose()`, and +// #3 a pooled FIELD released in a DIFFERENT member than the rent (ctor rent + Dispose +// return), or transferred into a field-stored guard that owns/returns it. + +// #2: an IDisposable field disposed via the null-conditional `?.Dispose()` -> SILENT. +public sealed class DisposesViaConditional : IDisposable +{ + private readonly MemoryStream stream = new(); + + public void Dispose() => this.stream?.Dispose(); // null-conditional -> now recognized +} + +// #2 control: an IDisposable field the class new's but never disposes -> must WARN. +public sealed class NeverDisposesField +{ + private readonly MemoryStream stream = new(); + + public long Use() => this.stream.Length; +} + +// #3: a pooled buffer FIELD rented in the ctor and Returned in Dispose (cross-member) -> SILENT. +public sealed class PoolFieldReturnedInDispose : IDisposable +{ + private readonly byte[] returnedBuf; + + public PoolFieldReturnedInDispose(int n) => this.returnedBuf = ArrayPool.Shared.Rent(n); + + public void Dispose() => ArrayPool.Shared.Return(this.returnedBuf); + + public byte First() => this.returnedBuf[0]; +} + +// #3 control: a pooled buffer FIELD rented but NEVER returned -> must WARN. +public sealed class PoolFieldLeaked +{ + private readonly byte[] leakedBuf; + + public PoolFieldLeaked(int n) => this.leakedBuf = ArrayPool.Shared.Rent(n); + + public byte First() => this.leakedBuf[0]; +}