Claude/fp field release recognition#86
Merged
Merged
Conversation
…l release (mined ImageSharp #2,#3/4) Two more field-detector precision fixes from the ImageSharp mining run. #2 — null-conditional disposal. The disposed-field set matched only a plain `field.Dispose()`; the dominant `field?.Dispose()` (a ConditionalAccess whose WhenNotNull is the .Dispose() invocation) was missed, so a field disposed that way was reported as a leak. Mined across ImageSharp: `this.memoryStream?.Dispose()` (ZipExrCompressor, DeflateCompressor, IccDataWriter) and the BufferedStreams benchmark's `[GlobalCleanup]`. #3 — cross-member pool release. The per-member pool pass required the Return in the same member as the Rent, but a FIELD buffer is rented in the ctor and returned in Dispose (different members). A field is now also released if `pool.Return(field)` appears ANYWHERE in the class (a field name is class-unique, so no cross-masking; locals keep per-member scoping), or if the buffer is TRANSFERRED into a `new Guard(field)` that the object stores in a field — the #80 escaping-ctor transfer at field level. Mined: BufferedReadStream (returns this.readBuffer in Dispose(bool)) and SharedArrayPoolBuffer (LifetimeGuard). Regression sample FieldReleaseSample.cs: `field?.Dispose()` and the cross-member / guard-transferred pooled fields stay silent; an undisposed field and a rented-never-returned pooled field still warn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
…eturn only (Codex) Codex P2: treating a rented field passed to ANY field-stored `new X(field)` as released wrongly suppresses a real leak for NON-owning wrappers (`_view = new ReadOnlyMemory<byte>(_buf)` and other cached views), where the array is never returned. Soundly distinguishing an owning guard (that Returns the buffer) from a non-owning view is not worth it for the single SharedArrayPoolBuffer FP, so the guard-transfer is removed entirely. Fix #3 keeps only the sound, high-value part: a pooled FIELD is released if `pool.Return(field)` appears anywhere in the class (cross-member ctor-rent + Dispose-return — BufferedReadStream). SharedArrayPoolBuffer's indirect release via its LifetimeGuard is left as an honest known limitation. Sample/CI drop the PoolFieldTransferredToGuard case accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
…ned ImageSharp FP #4/4) A `static class` has no instance, so a static-source subscription from it cannot promote an instance to the source's lifetime — the OWN014 region escape is vacuous. Mined: SixLabors/ImageSharp's MemoryAllocatorValidator, a `static class` whose static ctor hooks the static `MemoryDiagnostics.MemoryAllocated`/`MemoryReleased` events, was wrongly reported as "promotes MemoryAllocatorValidator to process lifetime" — there is no instance to promote. The static-source escape skip now also fires when the enclosing type is a static class (next to the existing process-lived `App` case). Scoped to the OWN014 escape only — OWN001 token leaks are untouched — and to non-timers, like the App case. (The two MemoryDiagnosticsTests OWN014s — lambdas in `static void RunTest` local functions of an INSTANCE class — are left honest: no `this` is captured so the named-instance claim is wrong, but the closure is retained, so this is the murky static-context territory, not a clean static-class drop.) Regression sample StaticClassEscapeSample.cs: a static class subscribing a LAMBDA (not covered by the static-method-handler exemption) to a static event stays silent; the existing StaticEventEscapeViewModel proves an INSTANCE class on the same shape still raises OWN014. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
…op static-class escape suppression (Codex) Codex P2: blanket-suppressing OWN014 for ALL static-source subscriptions in a static class is unsound — a CAPTURING lambda still retains its compiler-generated display-class instance for the process, even with no `this`. Codex's pointer is right: the mined case (MemoryAllocatorValidator) uses static-METHOD handlers, which the existing IsStaticHandler is meant to exempt; it missed them because a method group's symbol can surface as a member group (Symbol == null, CandidateSymbols populated). So: make IsStaticHandler fall back to CandidateSymbols (requiring ALL candidates static, so a mixed overload set is not wrongly exempted), and revert the static-class suppression entirely. A static-method handler in a static class is now exempted by IsStaticHandler (null target, no instance), while a capturing lambda there still escapes — sound. Sample now uses a static-METHOD handler on a static event from a static class's static ctor (replicating MemoryAllocatorValidator) and must stay silent; StaticEventEscapeViewModel still proves an instance handler on a static event escapes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
fix(extractor): resolve member-group symbols in IsStaticHandler (mined ImageSharp FP #4/4)
![github-advanced-security[bot]](https://cdn.github.com/in/57789?s=60&v=4){kind=small}
| // #2 control: an IDisposable field the class new's but never disposes -> must WARN. | ||
| public sealed class NeverDisposesField | ||
| { | ||
| private readonly MemoryStream stream = new(); |
| { | ||
| private readonly byte[] leakedBuf; | ||
|
|
||
| public PoolFieldLeaked(int n) => this.leakedBuf = ArrayPool<byte>.Shared.Rent(n); |
📝 WalkthroughWalkthroughThree extractor logic fixes are added to ChangesExtractor Logic, Fixtures, and CI Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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://cdn.github.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: eeafdb6ebf
ℹ️ 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".
Comment on lines
+2391
to
+2394
| if (inv.Expression is MemberAccessExpressionSyntax rm | ||
| && rm.Name.Identifier.Text == "Return" | ||
| && inv.ArgumentList.Arguments.Count > 0 | ||
| && model.GetSymbolInfo(inv.ArgumentList.Arguments[0].Expression).Symbol is IFieldSymbol rfs) |
There was a problem hiding this comment.
When a field rented from ArrayPool<T> is assigned in one member, this class-wide release set treats any invocation named Return with that field as the first argument as a release, without verifying that the receiver is actually System.Buffers.ArrayPool<T>. In a class that calls something like someCache.Return(this._buf) or another unrelated Return(_buf), fieldReleased marks _buf released and the later pool fact is suppressed, so a real rented-but-never-returned buffer is missed. Please reuse the semantic ArrayPool check used for rents/PoolReturnBuffer before adding the field to this set.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/roslyn/OwnSharp.Extractor/Program.cs (1)
2399-2415: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider renaming
isFieldfor clarity.The
isFieldflag is set totruefor allAssignmentExpressionSyntaxmatches, butFieldName()extracts names syntactically without semantic verification. A local variable assigned viabuf = pool.Rent(...)would also setisField=true.The current behavior is correct in practice because
fieldReleasedonly contains actual fields (theIFieldSymbolcheck at line 2394 ensures this), so a local won't match. However, there's a rare edge case: if a local shadows a field name, and that field is returned elsewhere, the local's rent would be incorrectly marked released.Consider either:
- Renaming to
isAssignmentto reflect actual semantics, or- Using semantic resolution on the LHS to verify it's actually a field
🤖 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 2399 - 2415, The variable name isField is misleading because the flag is set to true for all AssignmentExpressionSyntax cases regardless of whether the assignment target is actually a field or a local variable - the FieldName() method only extracts names syntactically without semantic verification. To fix this, either rename isField to isAssignment throughout this block (in the tuple definition, the switch expression, and the rented.Add call) to accurately reflect that it indicates an assignment expression rather than a guaranteed field, or alternatively implement semantic resolution on the assignment's left-hand side using the model parameter to verify that it actually refers to an IFieldSymbol before setting the flag to true.
🤖 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.
Nitpick comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 2399-2415: The variable name isField is misleading because the
flag is set to true for all AssignmentExpressionSyntax cases regardless of
whether the assignment target is actually a field or a local variable - the
FieldName() method only extracts names syntactically without semantic
verification. To fix this, either rename isField to isAssignment throughout this
block (in the tuple definition, the switch expression, and the rented.Add call)
to accurately reflect that it indicates an assignment expression rather than a
guaranteed field, or alternatively implement semantic resolution on the
assignment's left-hand side using the model parameter to verify that it actually
refers to an IFieldSymbol before setting the flag to true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c9e73a1-7ca3-4b99-b9bc-17d2555c6130
📒 Files selected for processing (4)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FieldReleaseSample.csfrontend/roslyn/samples/StaticClassEscapeSample.cs
PhysShell
pushed a commit
that referenced
this pull request
Jun 22, 2026
…handlers (Codex P1) Codex: the first cut keyed only off the EVENT (the AppDomain source), ignoring the handler — so it also dropped a real region escape, e.g. `AppDomain.CurrentDomain.ProcessExit += (_,_) => _field++` or an instance-method handler, whose delegate target is the subscriber and is pinned to the process until shutdown. The Npgsql case is safe only because its lambda is non-capturing (a static `ClearAll()` call). HandlerRetainsNoInstance now gates the exemption: a static method group (null target) or a lambda that captures neither `this` (explicit, or implicit via an instance member) nor an enclosing local/parameter. A capturing handler stays OWN014. Sample: ShutdownCleanup gains the 4th event (FirstChanceException, CodeRabbit) and a new CapturingShutdownSubscriber (`(_,_) => _count++`) that must STILL warn. CI: assert no OWN014 anywhere in the sample file (format-insensitive, CodeRabbit) for the exempt cases, and OWN014 for both the non-AppDomain and the capturing controls. (Rebased onto current main — #86/#87 landed; resolved the ci.yml sample-list conflict.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
This was referenced Jun 22, 2026
Merged
fix(extractor): recognize a field disposed through a local alias (mined Npgsql NpgsqlDataSource)
#90
Merged
oracle: fix dotted-test-dir filter + dispose-optional StringWriter/StringReader (+ field notes)
#147
Merged
9 tasks
This was referenced Jul 10, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests