fix(extractor): static-class subscriber exemption (OWN014 FP)#157
fix(extractor): static-class subscriber exemption (OWN014 FP)#157PhysShell wants to merge 3 commits into
Conversation
A subscription to a process-lived static event (AppDomain.ProcessExit, Console.CancelKeyPress) from inside a `static class` is not an OWN014 region escape: a static class has NO instance, so the 'promotes the component instance to the source's lifetime' premise is vacuously false — its state is already process-lived by the language definition. Sound by guarantee, not heuristic. Extends the existing process-lived-subscriber exemption (the WPF `App` singleton, clsIsApp) with clsIsStatic, gated identically: only the source=="static" region escape, non-timers. Mined on CsvHelper's static ConsoleHost, whose ProcessExit/CancelKeyPress shutdown hooks were false OWN014s. New corpus fixture subscription-static-class-host: before.cs is an INSTANCE host subscribing this to Console.CancelKeyPress (real OWN014), after.cs is the same as a static class (silent). case.own models the token form (OWN001) like the sibling screentogif-systemevents-leak case. Newtonsoft TraceJsonReader._textWriter stays baselined: it is a JsonTextWriter over a StringWriter, no-op only because of what it wraps — recognising that needs wrapper-cascade analysis (unsound to blanket-exempt JsonTextWriter), so the honest call is to leave it. Re-pointing the oracle at CsvHelper to confirm the ConsoleHost FPs clear. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
…restore scaffolding Live CsvHelper oracle re-run confirms the clsIsStatic exemption clears both ConsoleHost hooks (AppDomain.ProcessExit / Console.CancelKeyPress) at the source: Own.NET leak total 2 -> 0, both findings absent from own-only and baselined. Delete the two CsvHelper baseline entries (no CsvHelper FP entries remain). Restore dev scaffolding: oracle-target.txt back to the systemevents-console fixture, remove the branch from the oracle.yml push filter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
📝 WalkthroughWalkthroughThe extractor gains a ChangesStatic-class subscriber exemption (OWN014)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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: 6dd6ad5e6d
ℹ️ 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".
| // Scoped to NON-timers: a timer is forced to source "static" above, but a | ||
| // never-stopped timer in `App` is still a real leak (CodeRabbit). | ||
| if (!isTimer && source == "static" && clsIsApp) | ||
| if (!isTimer && source == "static" && (clsIsApp || clsIsStatic)) |
There was a problem hiding this comment.
This skips every static-source subscription in a static class before checking whether the handler captures anything. In a static helper such as Attach(ViewModel vm) { SystemEvents.UserPreferenceChanged += (_, _) => vm.Refresh(); }, the process-lived event retains the closure and vm even though there is no this; before this change that subscription was emitted as a capture/OWN014, but now it is silently continued. The exemption should be limited to handlers that retain no instance/locals, using the same kind of guard as HandlerRetainsNoInstance, rather than suppressing all static-class lambdas.
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: 2
🤖 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 `@corpus/real-world/subscription-static-class-host/case.own`:
- Around line 1-9: The fixture only validates the existing OWN001 leak model and
does not exercise the new static-class extractor behavior. Update the corpus
test setup around this case so it asserts extractor output for the `static
class` variant from `before.cs`/`after.cs`, and verify that the static-class
subscriber path is silent (no diagnostic) instead of only presence-checking the
files. Use the existing `tests/test_corpus.py` flow and the
`case.own`/`expected-diagnostics.txt` comparison to add a golden/assertion that
covers the `clsIsStatic` exemption end-to-end.
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 3282-3287: The static-class suppression in the OWN014 extraction
logic is too broad because it only checks `clsIsStatic`/`source == "static"` and
can still hide handlers that capture locals through closures. Update the branch
around the static-class event subscription handling to only suppress when the
handler is proven capture-free, using the existing capture analysis such as
`HandlerRetainsNoInstance(...)` or equivalent, and keep static-class
subscriptions that retain closure state reported normally.
🪄 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: fd028009-ba76-4a72-929b-fa96028a9cc8
📒 Files selected for processing (8)
corpus/oracle-fp-baseline.txtcorpus/real-world/subscription-static-class-host/after.cscorpus/real-world/subscription-static-class-host/before.cscorpus/real-world/subscription-static-class-host/case.owncorpus/real-world/subscription-static-class-host/expected-diagnostics.txtcorpus/real-world/subscription-static-class-host/notes.mddocs/notes/oracle-known-fps.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
| // OwnLang model of the static-class shutdown-hook subscription. before.cs subscribes a | ||
| // `this`-capturing handler to a process-lived static event (Console.CancelKeyPress) from | ||
| // an INSTANCE host and never unsubscribes — a subscription leak. The real C# lands the | ||
| // REGION form (OWN014 escape, static source outlives the instance); here it is modelled | ||
| // in the equivalent TOKEN form as the generic OWN001 (a Subscription acquire with no | ||
| // release), the shape test_corpus.py checks — exactly as the sibling | ||
| // corpus/real-world/screentogif-systemevents-leak case does. The fix makes the host a | ||
| // `static class` (no instance to promote), which the extractor's static-class subscriber | ||
| // exemption drops; see notes.md for the recognition rule. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
This fixture doesn't assert the new extractor behavior.
tests/test_corpus.py:65-99 only checks case.own against expected-diagnostics.txt; before.cs and after.cs are just presence-checked. Since this model still encodes only the leaking instance-host path (OWN001), clsIsStatic can regress without this case failing. Please add an extractor-driven golden/assertion for the static-class variant being silent so the PR objective is covered end-to-end.
🤖 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 `@corpus/real-world/subscription-static-class-host/case.own` around lines 1 -
9, The fixture only validates the existing OWN001 leak model and does not
exercise the new static-class extractor behavior. Update the corpus test setup
around this case so it asserts extractor output for the `static class` variant
from `before.cs`/`after.cs`, and verify that the static-class subscriber path is
silent (no diagnostic) instead of only presence-checking the files. Use the
existing `tests/test_corpus.py` flow and the
`case.own`/`expected-diagnostics.txt` comparison to add a golden/assertion that
covers the `clsIsStatic` exemption end-to-end.
Both Codex and CodeRabbit independently flagged the clsIsStatic exemption as
unsound: it skipped EVERY static-source subscription in a static class without a
capture check, silently dropping real captured-local leaks — e.g.
static class Foo { void Attach(VM vm) {
SystemEvents.UserPreferenceChanged += (_, _) => vm.Refresh(); } }
where the process-lived event pins the captured vm forever (was OWN014).
A sound narrowing (suppress only when the handler captures nothing) would not
clear the CsvHelper ConsoleHost target either — its shutdown hooks capture
cts/resetEvent locals — so the static-class signal cannot soundly distinguish
that FP from a real captured-local leak. CsvHelper ConsoleHost therefore stays
baselined, same bucket as Newtonsoft serializer.Error.
Reverts the extractor change, the subscription-static-class-host fixture, the doc
update, and restores the two CsvHelper ConsoleHost baseline entries — branch
returns to main. PR #157 is closed (no net change). Caught pre-merge by review.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
|
Closing — the static class Foo {
void Attach(VM vm) {
SystemEvents.UserPreferenceChanged += (_, _) => vm.Refresh(); // process-lived event pins vm forever
}
}A sound narrowing (suppress only a capture-free handler) would not clear the CsvHelper Reverted on the branch ( Generated by Claude Code |
Document the unsound `|| clsIsStatic` exemption attempted in #157 (reverted in 488d505) so it is not retried. A static class only rules out an instance `this`; a lambda handler can still capture a local that a process-lived event pins for the whole process, so the exemption would silently swallow a real captured-local leak. A capture-gated narrowing would be sound but still would not clear CsvHelper ConsoleHost (it captures cts/resetEvent), which therefore stays baselined. - Program.cs: ANTI-PATTERN warning comment at the clsIsApp exemption site. - oracle-known-fps.md: new "Rejected approaches" section with the why. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
2 participants
Что и зачем
Подписка на процесс-вечный статический ивент (
AppDomain.ProcessExit,Console.CancelKeyPress) изнутриstatic class— не OWN014 region-escape: у статического класса нет экземпляра, поэтому посылка OWN014 «подписка промоутит экземпляр компонента до времени жизни источника» ложна по гарантии языка (его состояние и так процесс-вечное). Чиним этот класс FP в экстракторе (clsIsStatic-эксемпшн), подтверждено живым прогоном оракула на CsvHelper.Тип изменения
Как проверено
python tests/run_tests.py(зелёный; corpus 28/28 reductions)ruff check .иmypy(в CI: lint job — pass)python scripts/oracle_compare.py --selftest31/31)C# проверен в CI (golden C#, leak extractor, corpus benchmark — все зелёные). Сверх того — живой прогон оракула на
JoshClose/CsvHelperс отключённым baseline поConsoleHost: Own.NET leak total 2 → 0, обе находки (ProcessExit+CancelKeyPress) отсутствуют и в own-only, и в baselined → погашены в источнике.Связанные issue
Нет. Продолжение oracle-FP-триажа (
docs/notes/oracle-known-fps.md), поверх #156.Чеклист
subscription-static-class-hostdocs/notes/oracle-known-fps.mdfix:/oracle:scoped)Подробнее
Фикс (precision, ~6 строк, sound по гарантии языка)
IsSelfOwnedSourceуже снимал static-source region-escape для процесс-вечного подписчика — WPFAppsingleton (clsIsApp). Добавлен ровно такой же по смыслуclsIsStatic:static classне имеет экземпляра, значит промоутить нечего. Гейтится идентично (source == "static", не-таймеры). Найдено на статическомConsoleHostв CsvHelper, чьи shutdown-хукиProcessExit/CancelKeyPressбыли ложными OWN014.Фикстура
corpus/real-world/subscription-static-class-host:before.cs— instance-хост подписываетthisнаConsole.CancelKeyPress(настоящий OWN014),after.cs— то же вstatic class(молчит).case.ownмоделирует token-форму (OWN001), как соседнийscreentogif-systemevents-leak.Что НЕ чинилось (осознанно)
Newtonsoft
TraceJsonReader._textWriterостаётся в baseline: этоJsonTextWriterнадStringWriter— no-op только из-за обёртки;JsonTextWriterв общем виде может обернутьFileStream(реальная течь), поэтому blanket-exempt unsound. Нужен wrapper-cascade анализ ради одной находки — честнее оставить в baseline. Процесс-вечный instance-хост (нестатический singleton) тоже остаётся открытым (нужен lifetime-сигнал).🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation