diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 13b3fdaf..e0de5280 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -139,6 +139,7 @@ jobs: frontend/roslyn/samples/AppLifetimeSample.cs \ frontend/roslyn/samples/ViewOwnsVmSample.xaml.cs \ frontend/roslyn/samples/InjectedDcViewSample.xaml.cs \ + frontend/roslyn/samples/ResolvedDisposableSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" - name: Check facts through the core @@ -203,6 +204,25 @@ jobs: if echo "$out" | grep -q "SharedTokenHolder"; then echo "FAIL: a static singleton IDisposable field was wrongly reported"; exit 1 fi + # P-004 resolve-aware disposability (mined: ImageSharp Vp8BitWriter/JpegBitReader): + # a field whose type NAME ends in Writer/Reader/Stream but is NOT IDisposable (and + # RESOLVES) must NOT be flagged — IsOwnedDisposableType asks the real interface. + if echo "$out" | grep -q "EncoderWithNonDisposableWriter"; then + echo "FAIL: a resolved non-IDisposable Writer/Reader field was wrongly flagged"; exit 1 + fi + # control: resolved IDisposable fields (MemoryStream / CancellationTokenSource) the + # class new's but never disposes must STILL warn — real detection intact. (CodeRabbit: + # tie the assertion to OWN001 + the disposable-field resource, not just the class name. + # Severity-agnostic on purpose — the disposable-field leak renders as error, not warning.) + echo "$out" | grep -qE "ResolvedDisposableSample\.cs:[0-9]+:.*\[OWN001\].*resource: disposable field" \ + || { echo "FAIL: expected the OWN001 disposable-field finding on the resolved IDisposable control"; exit 1; } + echo "$out" | grep -q "HolderWithRealDisposable" \ + || { echo "FAIL: the resolved IDisposable control (MemoryStream/CTS) must be flagged by owner name"; exit 1; } + # dispose-optional control (Codex): Task / DataTable ARE IDisposable but disposal is + # optional (IsDisposeOptional) — a new'd, undisposed field of these must stay SILENT. + if echo "$out" | grep -q "HolderWithDisposeOptional"; then + echo "FAIL: a dispose-optional (Task/DataTable) field was wrongly flagged"; exit 1 + fi # 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 a05fb7aa..aea10637 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -1524,6 +1524,27 @@ static bool IsNonDisposableReaderWriter(string t) => t is "PipeReader" or "PipeWriter" or "System.IO.Pipelines.PipeReader" or "System.IO.Pipelines.PipeWriter"; +// Is this field/local type an OWNED disposable? Prefer the RESOLVED type's real +// IDisposable/IAsyncDisposable interface: an in-project `…Writer`/`…Reader`/`…Stream` +// that is NOT actually IDisposable (ImageSharp's `Vp8BitWriter : BitWriterBase`, the +// `JpegBitReader` struct) must not be flagged by name alone. Generalises the #79 +// PipeReader/PipeWriter exclusion — any resolved non-IDisposable type is excluded, no +// curated list needed. Only when the type does NOT resolve (an unreferenced external +// assembly — WPF/DevExpress on the Linux runner) do we fall back to the syntactic name +// heuristic, which is the whole reason that heuristic exists. +static bool IsOwnedDisposableType(TypeSyntax type, SemanticModel model) +{ + var sym = model.GetTypeInfo(type).Type; + if (sym is null or IErrorTypeSymbol) + return IsDisposableType(type.ToString()); + // Resolved: demand a REAL IDisposable, but keep the optional-dispose exemption + // (Task/ValueTask/DataTable/DataSet/DataView) that the flat name path got for free — + // their Dispose is a no-op / they hold only a lazy wait handle, so an undisposed field + // of these is not a leak and must stay silent (Codex). Both helpers already exist and + // are shared with the flow detector. + return ImplementsIDisposable(sym) && !IsDisposeOptional(sym); +} + // --- P-006: DI registration + constructor graph (DI001 captive dependency) --- // A syntactic pass over the same trees, independent of the event/disposable // detectors: collect each class's constructor parameter types (the dependency @@ -2116,7 +2137,11 @@ or ImplicitObjectCreationExpressionSyntax if (fd.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword))) continue; var tname = fd.Declaration.Type.ToString(); - if (!IsDisposableType(tname)) + // Resolve-aware: when the field type binds to a real symbol, demand an actual + // IDisposable; only an UNRESOLVED type falls back to the name heuristic. (Stops + // the #1 ImageSharp FP class: undisposed `Vp8BitWriter`/`JpegBitReader` fields + // whose types merely end in Writer/Reader but are not IDisposable.) + if (!IsOwnedDisposableType(fd.Declaration.Type, model)) continue; foreach (var v in fd.Declaration.Variables) { diff --git a/frontend/roslyn/samples/ResolvedDisposableSample.cs b/frontend/roslyn/samples/ResolvedDisposableSample.cs new file mode 100644 index 00000000..3c57b612 --- /dev/null +++ b/frontend/roslyn/samples/ResolvedDisposableSample.cs @@ -0,0 +1,54 @@ +using System.Data; +using System.IO; +using System.Threading; +using System.Threading.Tasks; + +namespace Own.Samples; + +// P-004 resolve-aware disposability (mined: ImageSharp Vp8BitWriter / JpegBitReader). +// +// A field whose type NAME ends in Writer/Reader/Stream but does NOT actually implement +// IDisposable must NOT be flagged: the field-disposable detector now asks the RESOLVED +// type's real interface, not the name. The IDisposable control must still warn, proving +// real detection is intact (and the name heuristic still covers UNRESOLVED types, which +// the existing WPF samples exercise). + +// Resolved, NOT IDisposable, name ends in "Writer" (like ImageSharp's Vp8BitWriter). +public sealed class FancyBitWriter +{ + public void Write(int x) { } +} + +// Resolved, NOT IDisposable, a struct named "...Reader" (like ImageSharp's JpegBitReader). +public struct TokenReader +{ + public int Position; +} + +public sealed class EncoderWithNonDisposableWriter +{ + private readonly FancyBitWriter writer = new(); // resolved, not IDisposable -> SILENT + private TokenReader reader = new(); // resolved struct, not IDisposable -> SILENT + + public void Encode() => this.writer.Write(this.reader.Position); +} + +// Control: resolved IDisposable fields the class new's but never disposes -> must WARN. +public sealed class HolderWithRealDisposable +{ + private readonly MemoryStream stream = new(); // MemoryStream IS IDisposable + private readonly CancellationTokenSource cts = new(); // CancellationTokenSource IS IDisposable + + public long Use() => this.stream.Length + this.cts.Token.GetHashCode(); +} + +// Dispose-optional control (Codex): Task and DataTable ARE IDisposable, but disposal is +// conventionally optional (IsDisposeOptional) — a `new`'d, undisposed field of these must +// stay SILENT even though it resolves as IDisposable. +public sealed class HolderWithDisposeOptional +{ + private readonly Task task = new(() => { }); // Task: IDisposable but dispose-optional + private readonly DataTable table = new(); // DataTable: IDisposable but dispose-optional + + public int Use() => this.task.Id + this.table.Columns.Count; +}