fix(extractor): recognize .Close() as a field release (receiver-scoped) — mined Npgsql#91
Conversation
…es a field (Npgsql re-mine) Two precision fixes for the field-disposable detector, both surfaced by re-mining npgsql/npgsql after #88-#90 (which confirmed those three FP classes were cleared): #2 SemaphoreSlim dispose-optional. SemaphoreSlim.Dispose() only frees a LAZILY- allocated wait handle (allocated solely if AvailableWaitHandle is read); the common WaitAsync/Release usage allocates nothing, so an undisposed SemaphoreSlim field is not a leak. Added to IsDisposeOptional alongside Task/DataTable, which share the "Dispose is a no-op / lazy wait handle" rationale. Mined: NpgsqlDataSource._setupMappingsSemaphore. #1 `.Close()` releases a field. The field-disposable `disposed` scan credited a field only on Dispose()/DisposeAsync(), but the LOCAL detector (DisposesLocal and the flow detector) has always accepted Dispose/Close/DisposeAsync. That asymmetry flagged a field cleaned up by Close() as a leak. Mirror the local set — add Close to both the direct and null-conditional field-dispose checks — so a Stream / DbConnection-style field released by Close() is silent. Mined: Npgsql's ReplicationConnection releases its NpgsqlConnection via `await _npgsqlConnection.Close(async: true)` (a real Own.NET false positive). Regression sample SemaphoreAndCloseSample.cs: HoldsSemaphore._gate (SemaphoreSlim) silent; ReleasesViaClose._closedConn / _closedConnQ (direct + null-conditional Close) silent; controls still warn — HoldsRealDisposable._ctsControl (a non-optional CTS) and LeaksUnclosed._leakedConn (a connection-like field never closed). CI asserts all. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughExtends the OwnSharp extractor to recognize ChangesClose() Field Release Detection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2f95994e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // SemaphoreSlim.Dispose() only frees a LAZILY-allocated wait handle (allocated solely if | ||
| // AvailableWaitHandle is read); the common WaitAsync/Release usage allocates nothing, so an | ||
| // undisposed SemaphoreSlim field is not a leak. Mined: Npgsql NpgsqlDataSource._setupMappingsSemaphore. | ||
| || (ns == "System.Threading" && t.Name == "SemaphoreSlim"); |
There was a problem hiding this comment.
With this type-only exemption, a SemaphoreSlim field/local is dropped before any usage is inspected. That hides the real leak when code reads AvailableWaitHandle (for example, to pass it to WaitHandle.WaitAny), because the wait handle is then allocated and the semaphore needs disposal to release it; the comment immediately above calls out that allocation path. Please keep SemaphoreSlim candidates tracked when AvailableWaitHandle is accessed rather than returning true for every instance.
Useful? React with 👍 / 👎.
| && m.Name.Identifier.Text is "Dispose" or "DisposeAsync" or "Close" | ||
| && FieldName(m.Expression) is { } df) |
There was a problem hiding this comment.
Adding Close here means other._conn.Close() now satisfies the release check for the current class: FieldName(m.Expression) strips the receiver and returns _conn for any member access, and C# permits another instance of the same class to access that private field. In that scenario a constructed this._conn that is never closed is marked released, so OWN001 is suppressed; the check should only credit Close() when the receiver is this instance's field or a validated alias.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/roslyn/OwnSharp.Extractor/Program.cs (1)
468-476: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScope
SemaphoreSlimexemption to field-disposable detection only.At Line 476, adding
SemaphoreSlimto sharedIsDisposeOptionalalso affects flow-locals (Line 2686), which suppresses real local leaks and conflicts with the existingsemLeakexpectation in CI. This should not be a global exemption.Suggested direction
static bool IsDisposeOptional(ITypeSymbol t) { var ns = t.ContainingNamespace?.ToString(); return (ns == "System.Threading.Tasks" && t.Name is "Task" or "ValueTask") || (ns == "System.Data" && t.Name is "DataTable" or "DataSet" or "DataView") - || (ns == "System.Threading" && t.Name == "SemaphoreSlim"); + ; } + +static bool IsFieldDisposeOptional(ITypeSymbol t) +{ + var ns = t.ContainingNamespace?.ToString(); + return IsDisposeOptional(t) + || (ns == "System.Threading" && t.Name == "SemaphoreSlim"); +} // field path -return ImplementsIDisposable(sym) && !IsDisposeOptional(sym); +return ImplementsIDisposable(sym) && !IsFieldDisposeOptional(sym);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/roslyn/OwnSharp.Extractor/Program.cs` around lines 468 - 476, The SemaphoreSlim exemption in the IsDisposeOptional method is being applied globally to both field-disposable detection and flow-locals analysis, which suppresses real leaks in local variables and breaks the existing semLeak test expectation. Remove the SemaphoreSlim condition from the IsDisposeOptional method at Line 476 (the condition checking ns == "System.Threading" && t.Name == "SemaphoreSlim"), and instead add this exemption only in the field-disposable detection logic around Line 2686 to keep the exemption scoped to fields only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 2273-2278: The issue is that the field name fallback (the df
variable) in the disposed.Add call allows calls like other._conn.Close() to
incorrectly mark this class's _conn field as released, creating false negatives
for OWN001 detection. To fix this, restrict the Close method (added at line
2275) to only credit releases when there is a proven local symbol alias mapping
through aliasToField, rather than using the field name fallback. Modify the
logic so that for Close calls specifically, only add to the disposed collection
when the ILocalSymbol branch succeeds with the aliasToField lookup, and skip the
fallback to df for Close. Apply the same restriction to the similar code block
mentioned at lines 2285-2290.
---
Outside diff comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 468-476: The SemaphoreSlim exemption in the IsDisposeOptional
method is being applied globally to both field-disposable detection and
flow-locals analysis, which suppresses real leaks in local variables and breaks
the existing semLeak test expectation. Remove the SemaphoreSlim condition from
the IsDisposeOptional method at Line 476 (the condition checking ns ==
"System.Threading" && t.Name == "SemaphoreSlim"), and instead add this exemption
only in the field-disposable detection logic around Line 2686 to keep the
exemption scoped to fields only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a766b06b-8044-47d0-b125-0b019e022786
📒 Files selected for processing (3)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/SemaphoreAndCloseSample.cs
…ped); drop the SemaphoreSlim exemption Scopes this PR to the uncontentious fix and addresses the review. #1 `.Close()` releases a field — the field-disposable `disposed` scan credited a field only on Dispose()/DisposeAsync(), but the LOCAL detector (DisposesLocal and the flow detector) has always accepted Dispose/Close/DisposeAsync. That asymmetry flagged a field cleaned up by Close() as a leak. Mirror the local set on both the direct and null-conditional field-release checks. Mined: Npgsql ReplicationConnection releases its NpgsqlConnection field via `await _npgsqlConnection.Close(async: true)`. Receiver-scoped (Codex/CodeRabbit): credit only THIS instance's field or a validated alias via ThisFieldName, so `other._conn.Close()` on ANOTHER instance of the SAME class cannot mark this object's `_conn` released. (A field-symbol ContainingType check would NOT catch this — same class, same ContainingType — so the receiver is keyed syntactically; this also tightens the pre-existing Dispose path.) Dropped: the SemaphoreSlim dispose-optional change. Putting SemaphoreSlim in the shared IsDisposeOptional also affected the flow-locals detector and tripped the deliberate `semLeak` control (a prior ShareX fix explicitly decided SemaphoreSlim stays tracked for method-bounded locals, since reading AvailableWaitHandle allocates a handle Dispose must release — Codex flagged the same). A sound, field-scoped + AvailableWaitHandle-gated version is a separate decision, deferred. Regression sample CloseReleaseSample.cs: ReleasesViaClose._closedConn / _closedConnQ (direct + null-conditional Close) silent; controls still warn — LeaksUnclosed._leakedConn (never closed) and ClosesOtherInstanceField._xconn (closed only on ANOTHER instance). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
PhysShell
changed the title
.Close() releases a field (Npgsql re-mine).Close() as a field release (receiver-scoped) — mined Npgsql
| // control: a field NEITHER closed NOR disposed must STILL warn OWN001. | ||
| public sealed class LeaksUnclosed : IDisposable | ||
| { | ||
| private readonly FakeConnection _leakedConn = new FakeConnection(); |
| // This object's own _xconn is never closed, so it STILL leaks. | ||
| public sealed class ClosesOtherInstanceField : IDisposable | ||
| { | ||
| private readonly FakeConnection _xconn = new FakeConnection(); |
# Conflicts: # .github/workflows/ci.yml
3 participants
Scoped to the uncontentious fix after review.
.Close()releases a fieldThe field-disposable
disposedscan credited a field only onDispose()/DisposeAsync()— but the local detector (DisposesLocal+ the flow detector) has always acceptedDispose/Close/DisposeAsync. That asymmetry flagged a field cleaned up byClose()as a leak. The fix mirrors the local set on both the direct and null-conditional field-release checks.Mined:
ReplicationConnectionreleases itsNpgsqlConnectionfield viaawait _npgsqlConnection.Close(async: true)— a genuine Own.NET false positive (the field is released, viaClose, notDispose).Receiver-scoped (Codex + CodeRabbit): credit only this instance's field or a validated alias via
ThisFieldName, soother._conn.Close()on another instance of the same class cannot mark this object's_connreleased. Note a field-symbolContainingTypecheck (as suggested in review) would not catch this — same class ⇒ sameContainingType— so the receiver is keyed syntactically. This also tightens the pre-existingDisposepath.Dropped from this PR: the
SemaphoreSlimexemptionPutting
SemaphoreSlimin the sharedIsDisposeOptionalalso affected the flow-locals detector and tripped a deliberate control (FlowLocalsSample.semLeak). A prior ShareX fix explicitly decided SemaphoreSlim stays tracked for method-bounded locals, because readingAvailableWaitHandleallocates a handle thatDisposemust release — Codex flagged exactly this. A sound version would be field-scoped +AvailableWaitHandle-gated; deferred as a separate decision.Regression sample (
CloseReleaseSample.cs)ReleasesViaClose._closedConn/_closedConnQ(direct + null-conditionalClose()) → silentLeaksUnclosed._leakedConn(never closed) andClosesOtherInstanceField._xconn(closed only on another instance — proves receiver-scoping)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
.Close()as a valid release operation, equivalent to.Dispose().Tests
.Close()release patterns and scope validation.