Skip to content

Commit a221bbd

Browse files
authored
Merge pull request #92 from PhysShell/claude/fp-semaphore-field-optional
fix(extractor): SemaphoreSlim field dispose-optional unless AvailableWaitHandle is read (mined Npgsql)
2 parents a6c5490 + 5d577d4 commit a221bbd

3 files changed

Lines changed: 109 additions & 0 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@ jobs:
146146
frontend/roslyn/samples/AppDomainShutdownSample.cs \
147147
frontend/roslyn/samples/AliasDisposeSample.cs \
148148
frontend/roslyn/samples/CloseReleaseSample.cs \
149+
frontend/roslyn/samples/SemaphoreFieldSample.cs \
149150
-o "$RUNNER_TEMP/facts.json"
150151
cat "$RUNNER_TEMP/facts.json"
151152
- name: Check facts through the core
@@ -275,6 +276,24 @@ jobs:
275276
# receiver-stripping name match that a same-class ContainingType check would also miss).
276277
echo "$out" | grep -qE "CloseReleaseSample\.cs:[0-9]+:.*\[OWN001\].*'_xconn'" \
277278
|| { echo "FAIL: other-instance .Close() must not credit this field (receiver-scoped to this/alias)"; exit 1; }
279+
# P-004 SemaphoreSlim FIELD dispose-optional (mined: Npgsql NpgsqlDataSource._setupMappingsSemaphore):
280+
# a SemaphoreSlim field used only for Wait/Release (AvailableWaitHandle never read) frees nothing on
281+
# Dispose -> must be SILENT.
282+
if echo "$out" | grep -q "'_optionalSem'"; then
283+
echo "FAIL: a SemaphoreSlim field whose AvailableWaitHandle is never read was wrongly reported (dispose-optional)"; exit 1
284+
fi
285+
# gate control: a SemaphoreSlim field whose AvailableWaitHandle IS read allocates a handle Dispose
286+
# must release -> it must STILL warn (proves the exemption is gated, not blanket — Codex).
287+
echo "$out" | grep -qE "SemaphoreFieldSample\.cs:[0-9]+:.*\[OWN001\].*'_handleSem'" \
288+
|| { echo "FAIL: a SemaphoreSlim field whose AvailableWaitHandle is read must still warn"; exit 1; }
289+
# Codex control: an AvailableWaitHandle read THROUGH A FIELD ALIAS must credit the field -> still warn.
290+
echo "$out" | grep -qE "SemaphoreFieldSample\.cs:[0-9]+:.*\[OWN001\].*'_aliasedSem'" \
291+
|| { echo "FAIL: an aliased AvailableWaitHandle read must keep the field tracked (alias-aware gate)"; exit 1; }
292+
# type-scope control: a non-SemaphoreSlim owned IDisposable (CTS) never disposed must STILL warn.
293+
echo "$out" | grep -qE "SemaphoreFieldSample\.cs:[0-9]+:.*\[OWN001\].*'_ctsControl'" \
294+
|| { echo "FAIL: a non-SemaphoreSlim owned IDisposable field must still warn (exemption stays SemaphoreSlim-scoped)"; exit 1; }
295+
# field-scoped: the existing method-bounded LOCAL SemaphoreSlim leak (FlowLocalsSample.semLeak) must
296+
# be UNAFFECTED — checked in the --flow-locals step below; this exemption never touches IsDisposeOptional.
278297
# WPF004: an ignored `X.Subscribe(...)` result leaks; the captured+
279298
# disposed one stays silent. "ignored" is unique to the WPF004 message.
280299
echo "$out" | grep -q "MessengerViewModel.cs" \

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

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

2307+
// P-004 SemaphoreSlim field exemption (mined: Npgsql NpgsqlDataSource._setupMappingsSemaphore).
2308+
// SemaphoreSlim.Dispose() only frees a LAZILY-allocated wait handle — allocated solely when
2309+
// `.AvailableWaitHandle` is read — so a SemaphoreSlim field used purely for Wait/WaitAsync/Release
2310+
// leaks nothing and is dispose-optional. GATE: if `.AvailableWaitHandle` IS read on the field,
2311+
// that handle exists and Dispose must release it, so the field STAYS tracked (Codex). Collect the
2312+
// field names whose AvailableWaitHandle is read (this/bare receiver) so the loop below keeps them.
2313+
// Scoped to FIELDS only — the shared IsDisposeOptional (and the flow-locals detector / the
2314+
// deliberate method-bounded `semLeak` control) is intentionally left untouched (CodeRabbit).
2315+
var waitHandleSemaphores = new HashSet<string>(StringComparer.Ordinal);
2316+
foreach (var ma in cls.DescendantNodes().OfType<MemberAccessExpressionSyntax>())
2317+
if (ma.Name.Identifier.Text == "AvailableWaitHandle")
2318+
{
2319+
// credit the FIELD whose AvailableWaitHandle is read, by SYMBOL: a this/bare access bound
2320+
// to a real field symbol (so `other._f` and a shadowing local/param are NOT conflated —
2321+
// CodeRabbit), OR a field-ALIAS local (`var s = _sem; s.AvailableWaitHandle` -> `_sem` —
2322+
// Codex). Anything else (an unrelated local, another instance's field) is ignored.
2323+
var recv = model.GetSymbolInfo(ma.Expression).Symbol;
2324+
if (recv is IFieldSymbol && ThisFieldName(ma.Expression) is { } whf)
2325+
waitHandleSemaphores.Add(whf);
2326+
else if (recv is ILocalSymbol ls && aliasToField.TryGetValue(ls, out var fa))
2327+
waitHandleSemaphores.Add(fa);
2328+
}
2329+
23072330
foreach (var fd in cls.Members.OfType<FieldDeclarationSyntax>())
23082331
{
23092332
// a `static` IDisposable field is a process-lifetime singleton (a shared
@@ -2331,6 +2354,13 @@ or ImplicitObjectCreationExpressionSyntax
23312354
if (eventSourceCounters.Contains(v.Identifier.Text)
23322355
&& DerivesFromDiagnosticCounter(model.GetTypeInfo(fd.Declaration.Type).Type))
23332356
continue;
2357+
// a SemaphoreSlim field whose `.AvailableWaitHandle` is never read leaks nothing —
2358+
// Dispose() only frees that lazy handle — so it is dispose-optional and silent; if the
2359+
// handle IS read (in waitHandleSemaphores) it stays tracked. FIELD-scoped (Npgsql).
2360+
if (!waitHandleSemaphores.Contains(v.Identifier.Text)
2361+
&& model.GetTypeInfo(fd.Declaration.Type).Type is { Name: "SemaphoreSlim" } st
2362+
&& st.ContainingNamespace?.ToString() == "System.Threading")
2363+
continue;
23342364
subs.Add(new
23352365
{
23362366
@event = v.Identifier.Text,
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
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+
// Codex control: AvailableWaitHandle read through a field ALIAS (`var s = _sem; s.AvailableWaitHandle`)
40+
// must credit the FIELD — the handle is allocated, so the field must STILL warn OWN001 (not be exempted
41+
// because the read went through the local name).
42+
public sealed class AliasedWaitHandleSemaphore
43+
{
44+
private readonly SemaphoreSlim _aliasedSem = new SemaphoreSlim(0, 1);
45+
46+
public void Block()
47+
{
48+
var sem = _aliasedSem;
49+
sem.AvailableWaitHandle.WaitOne(); // alias reads AvailableWaitHandle -> _aliasedSem stays tracked
50+
}
51+
}
52+
53+
// control (type scope): a non-SemaphoreSlim owned IDisposable (CancellationTokenSource) never disposed
54+
// must STILL warn — the exemption is SemaphoreSlim-specific, not a blanket "any field".
55+
public sealed class HoldsCtsField
56+
{
57+
private readonly CancellationTokenSource _ctsControl = new CancellationTokenSource();
58+
59+
public void Cancel() => _ctsControl.Cancel();
60+
}

0 commit comments

Comments
 (0)