Skip to content

Commit 7a607a7

Browse files
committed
fix(extractor): PipeReader/PipeWriter are not IDisposable — stop flagging them as leaks (mined FP)
Mining Pipelines.Sockets.Unofficial surfaced a systematic false positive: the field-disposable name heuristic in IsDisposableType matches anything ending in "Reader"/"Writer", so `System.IO.Pipelines` PipeReader/PipeWriter fields (SocketConnection._input/_output, assigned `new WrappedReader/WrappedWriter`) were reported as undisposed-IDisposable leaks (OWN001). But PipeReader/PipeWriter are NOT IDisposable — they finish via Complete(), not Dispose() — so this is a false alarm, and it would fire on essentially any code using System.IO.Pipelines. New IsNonDisposableReaderWriter excludes PipeReader/PipeWriter (matched on the simple name, so a qualified `System.IO.Pipelines.PipeReader` is covered too) from the EndsWith("Reader"/"Writer") heuristic. The curated IDisposable names and the …Stream/…Subscription families are unchanged; no existing corpus/sample uses these types, so behaviour elsewhere is unaffected. Regression guard: frontend/roslyn/samples/PipeFieldsSample.cs (a class that constructs PipeReader/PipeWriter fields and never disposes them) is added to the wpf-extractor CI job, which asserts it produces NO finding. Found by `scripts/mine.sh` over a real repo; not reproducible without a .NET SDK locally, so validated by CI + a re-mine. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
1 parent 70376da commit 7a607a7

3 files changed

Lines changed: 67 additions & 2 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,7 @@ jobs:
135135
frontend/roslyn/samples/WhenAnyValueViewModel.cs \
136136
frontend/roslyn/samples/DiCaptiveSample.cs \
137137
frontend/roslyn/samples/SampleTypes.cs \
138+
frontend/roslyn/samples/PipeFieldsSample.cs \
138139
-o "$RUNNER_TEMP/facts.json"
139140
cat "$RUNNER_TEMP/facts.json"
140141
- name: Check facts through the core
@@ -154,6 +155,13 @@ jobs:
154155
if echo "$out" | grep -q "OrdersViewModel.cs"; then
155156
echo "FAIL: disposed subscription wrongly reported"; exit 1
156157
fi
158+
# Mined FP regression (Pipelines.Sockets.Unofficial): System.IO.Pipelines PipeReader/PipeWriter
159+
# END WITH Reader/Writer but are NOT IDisposable (they finish via Complete(), not Dispose()), so
160+
# an undisposed PipeReader/PipeWriter FIELD must NOT be flagged as a leak —
161+
# IsNonDisposableReaderWriter excludes them from the field-disposable name heuristic.
162+
if echo "$out" | grep -q "PipeFieldsSample.cs"; then
163+
echo "FAIL: PipeReader/PipeWriter field wrongly reported as an undisposed-disposable leak"; exit 1
164+
fi
157165
# a lambda handler has no stored delegate, so it can NEVER be `-=`'d — the
158166
# finding says so. (Same injected source as Customer -> also a warning.)
159167
echo "$out" | grep -qE "LambdaHandlerViewModel\.cs:[0-9]+: warning: \[OWN001\]" \

‎frontend/roslyn/OwnSharp.Extractor/Program.cs‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1412,8 +1412,18 @@ static bool DisposesLocal(SyntaxNode body, string name)
14121412
static bool IsDisposableType(string t) =>
14131413
t is "IDisposable" or "IAsyncDisposable" or "CancellationTokenSource"
14141414
or "HttpClient" or "SerialPort" or "SqlConnection"
1415-
|| t.EndsWith("Stream") || t.EndsWith("Reader") || t.EndsWith("Writer")
1416-
|| t.EndsWith("Subscription");
1415+
|| ((t.EndsWith("Stream") || t.EndsWith("Reader") || t.EndsWith("Writer")
1416+
|| t.EndsWith("Subscription"))
1417+
&& !IsNonDisposableReaderWriter(t));
1418+
1419+
// BCL `…Reader`/`…Writer` types that the EndsWith name heuristic above matches but that are NOT
1420+
// IDisposable: System.IO.Pipelines `PipeReader`/`PipeWriter` finish via `Complete()`, not `Dispose()`.
1421+
// Excluding them stops the field-disposable detector flagging an undisposed PipeReader/PipeWriter
1422+
// field as a leak — a FALSE POSITIVE mined on Pipelines.Sockets.Unofficial (SocketConnection's
1423+
// `_input`/`_output`). Matched on the SIMPLE name so a qualified `System.IO.Pipelines.PipeReader`
1424+
// is covered too.
1425+
static bool IsNonDisposableReaderWriter(string t) =>
1426+
(t.Contains('.') ? t[(t.LastIndexOf('.') + 1)..] : t) is "PipeReader" or "PipeWriter";
14171427

14181428
// --- P-006: DI registration + constructor graph (DI001 captive dependency) ---
14191429
// A syntactic pass over the same trees, independent of the event/disposable
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
using System;
2+
using System.IO.Pipelines;
3+
using System.Threading;
4+
using System.Threading.Tasks;
5+
6+
// Mined false-positive regression guard (Pipelines.Sockets.Unofficial). `System.IO.Pipelines`
7+
// PipeReader/PipeWriter END WITH "Reader"/"Writer", so the field-disposable NAME heuristic used to
8+
// classify them as IDisposable and flag an undisposed PipeReader/PipeWriter FIELD as a leak. But they
9+
// are NOT IDisposable — they finish via Complete(), not Dispose(). A class that constructs such fields
10+
// and never disposes them must produce NO finding (mirrors SocketConnection._input/_output, which the
11+
// checker wrongly reported before IsNonDisposableReaderWriter excluded these types).
12+
public sealed class PipeHolder
13+
{
14+
private readonly PipeReader _input;
15+
private readonly PipeWriter _output;
16+
17+
public PipeHolder()
18+
{
19+
_input = new NullReader(); // constructed (a `new`) -> a disposable-field candidate before the fix
20+
_output = new NullWriter();
21+
}
22+
23+
// No Dispose: PipeReader/PipeWriter are completed by the consumer, not disposed -> not a leak.
24+
}
25+
26+
// Minimal PipeReader/PipeWriter implementations so the sample is self-contained (the abstract BCL
27+
// types resolve from the framework reference set). PipeReader/PipeWriter expose Complete(), not
28+
// Dispose() — they are not IDisposable.
29+
internal sealed class NullReader : PipeReader
30+
{
31+
public override void AdvanceTo(SequencePosition consumed) => throw new NotImplementedException();
32+
public override void AdvanceTo(SequencePosition consumed, SequencePosition examined) => throw new NotImplementedException();
33+
public override void CancelPendingRead() => throw new NotImplementedException();
34+
public override void Complete(Exception exception = null) => throw new NotImplementedException();
35+
public override ValueTask<ReadResult> ReadAsync(CancellationToken cancellationToken = default) => throw new NotImplementedException();
36+
public override bool TryRead(out ReadResult result) => throw new NotImplementedException();
37+
}
38+
39+
internal sealed class NullWriter : PipeWriter
40+
{
41+
public override void Advance(int bytes) => throw new NotImplementedException();
42+
public override void CancelPendingFlush() => throw new NotImplementedException();
43+
public override void Complete(Exception exception = null) => throw new NotImplementedException();
44+
public override ValueTask<FlushResult> FlushAsync(CancellationToken cancellationToken = default) => throw new NotImplementedException();
45+
public override Memory<byte> GetMemory(int sizeHint = 0) => throw new NotImplementedException();
46+
public override Span<byte> GetSpan(int sizeHint = 0) => throw new NotImplementedException();
47+
}

0 commit comments

Comments
 (0)