fix(extractor): resolve member-group symbols in IsStaticHandler (mined ImageSharp FP #4/4)#85
Conversation
…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
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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: 8e80d668ce
ℹ️ 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".
| // mined: ImageSharp MemoryAllocatorValidator). Scoped to NON-timers: a timer is | ||
| // forced to source "static" above, but a never-stopped timer is still a real | ||
| // leak (CodeRabbit). | ||
| if (!isTimer && source == "static" && (clsIsApp || clsIsStaticClass)) |
There was a problem hiding this comment.
When a static class subscribes with a lambda or delegate that closes over a local, e.g. from a static ctor, the static event still retains the generated display-class instance for the process even though there is no this instance. This new condition skips every static-source subscription in a static class, so those closure-retention cases no longer emit the capture fact/OWN014; the earlier IsStaticHandler exemption already covers handlers proven to have no delegate target, so this suppression should be limited to non-capturing handlers.
Useful? React with 👍 / 👎.
…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
PhysShell
changed the title
eeafdb6
into
claude/fp-field-release-recognition
2 participants
Mined from
SixLabors/ImageSharp— fix 4 of 4 (final)Stacked on #84.
The bug
ImageSharp's
MemoryAllocatorValidator(astatic classwhose static ctor hooks the staticMemoryDiagnostics.MemoryAllocated/MemoryReleasedevents with static-method handlers) was reported with OWN014 region-escape — "promotesMemoryAllocatorValidatorto process lifetime" — but a static-method handler stores a null-target delegate (no instance retained), so the existingIsStaticHandlerexemption should have silenced it.Why it slipped through
A method group's symbol can surface as a member group (
GetSymbolInfo(handler).Symbol == null,CandidateSymbolspopulated) instead of the bound method, soIsStaticHandler's.Symbol is IMethodSymbol { IsStatic: true }check missed it.The fix
IsStaticHandlernow falls back toCandidateSymbols(requiring all candidates static, so a mixed overload set isn't wrongly exempted). A static-method handler in a static class is exempted (null target); a capturing lambda there still escapes (its closure display-class is retained) — sound.Regression guard
StaticClassEscapeSample.cs— a static class's static ctor subscribing a static-method handler to a static event stays silent; the existingStaticEventEscapeViewModelproves an instance handler on a static event still raises OWN014.Batch (4/4) + validation
After the stack merges I'll re-mine ImageSharp — that's the real validation that
MemoryAllocatorValidatorclears (the member-group hypothesis is best-effort; I can't run the extractor locally). Left honest: theMemoryDiagnosticsTestsclosures,SharedArrayPoolBuffer's guard-indirection, and theWrapMemoryprojection-escape.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED