Skip to content

Commit f545f3d

Browse files
committed
fix(extractor): SemaphoreSlim FIELD is dispose-optional unless AvailableWaitHandle is read (mined Npgsql)
The sound, scoped version of the SemaphoreSlim FP (per review on #91 + the user's choice). SemaphoreSlim.Dispose() only frees a LAZILY-allocated wait handle — allocated solely when AvailableWaitHandle is read — so a SemaphoreSlim field used purely for Wait/WaitAsync/Release leaks nothing and is dispose-optional. Mined: NpgsqlDataSource._setupMappingsSemaphore. Two guards keep it sound, both flagged by reviewers on the earlier attempt: - FIELD-scoped: the exemption lives in the field-disposable detector, NOT in the shared IsDisposeOptional — so the flow-locals detector and the deliberate method-bounded `semLeak` control (a prior ShareX decision: SemaphoreSlim stays tracked for locals) are left untouched (CodeRabbit). - AvailableWaitHandle-GATED: if `.AvailableWaitHandle` is read on the field, the wait handle exists and Dispose must release it, so the field STAYS tracked (Codex). The gate keys on a this/bare-receiver `.AvailableWaitHandle` access. Regression sample SemaphoreFieldSample.cs: OptionalSemaphore._optionalSem (Wait/ Release only) is SILENT; controls still warn — WaitHandleSemaphore._handleSem (reads AvailableWaitHandle) and HoldsCtsField._ctsControl (a non-SemaphoreSlim CTS). The existing FlowLocalsSample.semLeak local control is unaffected (separate detector path), asserted in the --flow-locals step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
1 parent f9605da commit f545f3d

3 files changed

Lines changed: 82 additions & 0 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ jobs:
145145
frontend/roslyn/samples/EventSourceCountersSample.cs \
146146
frontend/roslyn/samples/AppDomainShutdownSample.cs \
147147
frontend/roslyn/samples/AliasDisposeSample.cs \
148+
frontend/roslyn/samples/SemaphoreFieldSample.cs \
148149
-o "$RUNNER_TEMP/facts.json"
149150
cat "$RUNNER_TEMP/facts.json"
150151
- name: Check facts through the core
@@ -261,6 +262,21 @@ jobs:
261262
# same-named local disposed in another method must NOT credit the field, so it still leaks.
262263
echo "$out" | grep -qE "AliasDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'_scopedLeak'" \
263264
|| { echo "FAIL: a same-named local in another scope must not be miscredited (symbol-scoped aliases)"; exit 1; }
265+
# P-004 SemaphoreSlim FIELD dispose-optional (mined: Npgsql NpgsqlDataSource._setupMappingsSemaphore):
266+
# a SemaphoreSlim field used only for Wait/Release (AvailableWaitHandle never read) frees nothing on
267+
# Dispose -> must be SILENT.
268+
if echo "$out" | grep -q "'_optionalSem'"; then
269+
echo "FAIL: a SemaphoreSlim field whose AvailableWaitHandle is never read was wrongly reported (dispose-optional)"; exit 1
270+
fi
271+
# gate control: a SemaphoreSlim field whose AvailableWaitHandle IS read allocates a handle Dispose
272+
# must release -> it must STILL warn (proves the exemption is gated, not blanket — Codex).
273+
echo "$out" | grep -qE "SemaphoreFieldSample\.cs:[0-9]+:.*\[OWN001\].*'_handleSem'" \
274+
|| { echo "FAIL: a SemaphoreSlim field whose AvailableWaitHandle is read must still warn"; exit 1; }
275+
# type-scope control: a non-SemaphoreSlim owned IDisposable (CTS) never disposed must STILL warn.
276+
echo "$out" | grep -qE "SemaphoreFieldSample\.cs:[0-9]+:.*\[OWN001\].*'_ctsControl'" \
277+
|| { echo "FAIL: a non-SemaphoreSlim owned IDisposable field must still warn (exemption stays SemaphoreSlim-scoped)"; exit 1; }
278+
# field-scoped: the existing method-bounded LOCAL SemaphoreSlim leak (FlowLocalsSample.semLeak) must
279+
# be UNAFFECTED — checked in the --flow-locals step below; this exemption never touches IsDisposeOptional.
264280
# WPF004: an ignored `X.Subscribe(...)` result leaks; the captured+
265281
# disposed one stays silent. "ignored" is unique to the WPF004 message.
266282
echo "$out" | grep -q "MessengerViewModel.cs" \

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2296,6 +2296,19 @@ or ImplicitObjectCreationExpressionSyntax
22962296
&& ThisFieldName(a.Left) is { } cfn) // THIS instance's field only — `other._c` must not exempt our field (CodeRabbit)
22972297
eventSourceCounters.Add(cfn);
22982298

2299+
// P-004 SemaphoreSlim field exemption (mined: Npgsql NpgsqlDataSource._setupMappingsSemaphore).
2300+
// SemaphoreSlim.Dispose() only frees a LAZILY-allocated wait handle — allocated solely when
2301+
// `.AvailableWaitHandle` is read — so a SemaphoreSlim field used purely for Wait/WaitAsync/Release
2302+
// leaks nothing and is dispose-optional. GATE: if `.AvailableWaitHandle` IS read on the field,
2303+
// that handle exists and Dispose must release it, so the field STAYS tracked (Codex). Collect the
2304+
// field names whose AvailableWaitHandle is read (this/bare receiver) so the loop below keeps them.
2305+
// Scoped to FIELDS only — the shared IsDisposeOptional (and the flow-locals detector / the
2306+
// deliberate method-bounded `semLeak` control) is intentionally left untouched (CodeRabbit).
2307+
var waitHandleSemaphores = new HashSet<string>(StringComparer.Ordinal);
2308+
foreach (var ma in cls.DescendantNodes().OfType<MemberAccessExpressionSyntax>())
2309+
if (ma.Name.Identifier.Text == "AvailableWaitHandle" && ThisFieldName(ma.Expression) is { } whf)
2310+
waitHandleSemaphores.Add(whf);
2311+
22992312
foreach (var fd in cls.Members.OfType<FieldDeclarationSyntax>())
23002313
{
23012314
// a `static` IDisposable field is a process-lifetime singleton (a shared
@@ -2323,6 +2336,13 @@ or ImplicitObjectCreationExpressionSyntax
23232336
if (eventSourceCounters.Contains(v.Identifier.Text)
23242337
&& DerivesFromDiagnosticCounter(model.GetTypeInfo(fd.Declaration.Type).Type))
23252338
continue;
2339+
// a SemaphoreSlim field whose `.AvailableWaitHandle` is never read leaks nothing —
2340+
// Dispose() only frees that lazy handle — so it is dispose-optional and silent; if the
2341+
// handle IS read (in waitHandleSemaphores) it stays tracked. FIELD-scoped (Npgsql).
2342+
if (!waitHandleSemaphores.Contains(v.Identifier.Text)
2343+
&& model.GetTypeInfo(fd.Declaration.Type).Type is { Name: "SemaphoreSlim" } st
2344+
&& st.ContainingNamespace?.ToString() == "System.Threading")
2345+
continue;
23262346
subs.Add(new
23272347
{
23282348
@event = v.Identifier.Text,
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
using System;
2+
using System.Threading;
3+
using System.Threading.Tasks;
4+
5+
namespace Own.Samples;
6+
7+
// P-004 SemaphoreSlim FIELD dispose-optional (mined: Npgsql NpgsqlDataSource._setupMappingsSemaphore).
8+
// SemaphoreSlim.Dispose() only frees a lazily-allocated wait handle (allocated solely when
9+
// AvailableWaitHandle is read), so a SemaphoreSlim field used purely for Wait/WaitAsync/Release leaks
10+
// nothing and is dispose-optional. GATED on AvailableWaitHandle: if the field's AvailableWaitHandle is
11+
// read, the handle exists and Dispose must release it -> the field STAYS tracked (Codex). Scoped to
12+
// FIELDS only — method-bounded LOCAL SemaphoreSlims remain tracked via FlowLocalsSample.semLeak.
13+
14+
// a SemaphoreSlim field used only for WaitAsync/Release, never disposed -> SILENT (dispose-optional).
15+
public sealed class OptionalSemaphore
16+
{
17+
private readonly SemaphoreSlim _optionalSem = new SemaphoreSlim(1, 1);
18+
19+
public async Task RunAsync()
20+
{
21+
await _optionalSem.WaitAsync();
22+
try { /* critical section */ }
23+
finally { _optionalSem.Release(); }
24+
}
25+
}
26+
27+
// control (the gate): the field's AvailableWaitHandle IS read, so the wait handle is allocated and
28+
// Dispose must release it -> the field must STILL warn OWN001.
29+
public sealed class WaitHandleSemaphore
30+
{
31+
private readonly SemaphoreSlim _handleSem = new SemaphoreSlim(0, 1);
32+
33+
public void Block()
34+
{
35+
_handleSem.AvailableWaitHandle.WaitOne(); // reads AvailableWaitHandle -> stays tracked
36+
}
37+
}
38+
39+
// control (type scope): a non-SemaphoreSlim owned IDisposable (CancellationTokenSource) never disposed
40+
// must STILL warn — the exemption is SemaphoreSlim-specific, not a blanket "any field".
41+
public sealed class HoldsCtsField
42+
{
43+
private readonly CancellationTokenSource _ctsControl = new CancellationTokenSource();
44+
45+
public void Cancel() => _ctsControl.Cancel();
46+
}

0 commit comments

Comments
 (0)