fix(extractor): exempt locals of user types with a provably-empty Dispose() (Closes #225)#233
Conversation
…pose() (#225) A local of a user-defined type whose `Dispose()` body is literally empty was false-flagged as an OWN001 leak. Such a type (e.g. an `IEnumerator<T>` enumerator) implements IDisposable only to satisfy an interface, not because it holds a resource — never calling its empty Dispose cannot leak anything. The existing no-op exemption (IsNoOpDisposeWrapper / IsDisposeOptional) only reached a handful of NAMED BCL types. Confirmed FP in ClosedXML, 5 sites, single root cause (Slice.Enumerator locals); issue #201 oracle sweep, field-notes entry 19. Fix (extractor-only, no OwnIR change): HasEmptyDisposeBody recognises from SOURCE a type whose parameterless `Dispose()` is a block body of zero statements and whose base chain owns no Dispose to cascade to, and drops a LOCAL of it from both the flat (IsDisposableType) and --flow-locals (ImplementsIDisposable) paths. Deliberately scoped to LOCALS: a FIELD keeps its own disposal contract, so the OwnIgnoreSample `Handle` field stand-in (an empty-Dispose type used as a leak control in #209) is unaffected. Precision guards, pinned by EmptyDisposeSample.cs in both the wpf-extractor (flat) and flow-locals CI jobs: a non-empty Dispose (RealResource, LeakyReader) still leaks; an empty override over a base whose Dispose does real work (EmptyOverrideOverRealBase) still leaks; an expression-bodied / metadata-only Dispose we cannot read is not exempted (never a guessed drop). The empty enumerator (the ClosedXML shape) and an empty *Reader stay silent. Two existing fixtures had used an empty `Dispose()` as a modelling shortcut while their comments claimed a real resource; made faithful (a real Dispose body) so they stay genuine leak fixtures: UnitOfWork now owns+disposes a CancellationTokenSource; PixelOwner clears its backing. This keeps their existing by-name CI assertions ('uow'/'leakedOwner' still leak). Verified locally with the real extractor (.NET 8): EmptyDisposeSample goes silent on the two empty-Dispose locals, flags the three controls, in both flat and flow modes; a full existing-sample diff (both modes) is byte-identical after the fixture-fidelity fix (zero regression). Gates: run_tests.py exit 0, test_ownir 276/276, ruff clean, mypy --strict green. Closes #225 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cg6DVXahkyY68Ruu7f76rG
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // FLAGGED (control): a real IDisposable local never disposed -> OWN001. | ||
| public void LeakReal() | ||
| { | ||
| var r = new RealResource(); |
| // FLAGGED (control): a non-empty `*Reader` local never disposed -> OWN001 (flat path). | ||
| public int LeakReader() | ||
| { | ||
| var lr = new LeakyReader(); |
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 8630b4ddea
ℹ️ 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".
| m => m.Parameters.Length == 0 && m.ReturnsVoid && m.TypeParameters.Length == 0); | ||
| if (dispose is null || dispose.DeclaringSyntaxReferences.Length == 0) | ||
| return false; // no readable source Dispose (metadata-only) -> cannot prove empty | ||
| return dispose.DeclaringSyntaxReferences.All( | ||
| sref => sref.GetSyntax() is MethodDeclarationSyntax { Body.Statements.Count: 0 }); |
There was a problem hiding this comment.
This exemption only proves that synchronous Dispose() is empty; it does not rule out a non-empty DisposeAsync() on the same source-defined type. For a type that implements both IDisposable and IAsyncDisposable with Dispose() as a compatibility no-op and DisposeAsync() doing the real cleanup, a local such as var r = new AsyncReader(); is now dropped from the local-disposable candidates in both detector paths, so failing to call either dispose method becomes silent even though the existing flow logic already treats DisposeAsync() as a release.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 447dee4. You're right: HasEmptyDisposeBody only proved the sync Dispose() was empty, so a type with IDisposable (empty compat no-op) + IAsyncDisposable (real DisposeAsync) would have been silenced even though the real cleanup is async and the flow detector already treats DisposeAsync() as a release.
The guard now conservatively refuses to exempt any type that declares its own parameterless DisposeAsync. Pinned by a new AsyncReader control (empty sync Dispose, real DisposeAsync) — flagged in both the flat and --flow-locals CI jobs, while the two provably-empty-Dispose locals stay silent. The guard only ever flags more, so the existing-sample diff stays byte-identical (zero regression); gates green.
Generated by Claude Code
…a real DisposeAsync (#225) Codex review catch. HasEmptyDisposeBody proved only that the SYNCHRONOUS Dispose() body is empty; a type implementing both IDisposable and IAsyncDisposable can have an empty sync Dispose() as a compatibility no-op while DisposeAsync() does the real cleanup. The flow detector already treats DisposeAsync() as a release, so silencing such a local would drop a real leak. Now conservatively refuse to exempt any type that declares its own parameterless DisposeAsync. Pinned by a new AsyncReader control in EmptyDisposeSample.cs (empty sync Dispose, real DisposeAsync) — flagged in both the flat and --flow-locals CI jobs; the two provably-empty-Dispose locals stay silent. Verified locally: existing-sample diff still byte-identical (the guard only ever flags MORE, never fewer); gates green (test_ownir 276/276, ruff, mypy). Refs #225 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cg6DVXahkyY68Ruu7f76rG
| // FLAGGED (control): empty override, but the base Dispose does real work -> OWN001. | ||
| public void LeakDerived() | ||
| { | ||
| var d = new EmptyOverrideOverRealBase(); |
| // (sync or async) leaks; the empty sync body must not exempt it. | ||
| public int LeakAsync() | ||
| { | ||
| var ar = new AsyncReader(); |
…source emptiness is not a runtime no-op (#238) The post-merge re-measure caught a soundness regression from #225/PR #233: ClosedXML came out -265 instead of the expected -5. 263 of those were real findings silently swallowed — XLWorkbook.Dispose() is empty IN SOURCE only, Janitor.Fody weaves the actual cleanup into it at build time, and HasEmptyDisposeBody trusted the source tree. Three changes, all in the same gate: - SOUNDNESS (the 263): the exemption now applies ONLY to types implementing IEnumerator/IEnumerator<T> — the shape that motivated #225, where an empty Dispose is an idiomatic interface stub, not a weaving target. Everything else keeps the honest warning; source-level analysis cannot prove the absence of IL weaving for arbitrary types. - Defense in depth: a FodyWeavers.xml anywhere above the type's source file disables the exemption even for a perfect enumerator shape (weaved/ fixture + CI assertion). Unlocatable source -> conservatively no exemption. - COVERAGE (the missing 3 of -5): `void IDisposable.Dispose() { }` (explicit interface implementation — the actual Slice.Enumerator spelling) is now recognized as an empty Dispose; the DisposeAsync refusal likewise matches explicit `IAsyncDisposable.DisposeAsync`. Sample rework: ExplicitDisposeEnumerator ('x', silent) pins the coverage fix; ScratchReader — a NON-enumerator with an empty source Dispose, the XLWorkbook shape — flips from silent to a FLAGGED control on both detector paths; flow count assertion 4 -> 5; new weaved/WeavedEmptyDispose.cs fixture pins the kill-switch. Verified with the real extractor (.NET 8): flat flags s/lr/ar, flow flags exactly s/r/lr/d/ar with e/x silent, the weaved enumerator stays flagged; full-sample-set diff vs main (minus the reworked sample) is byte-identical. Gates: run_tests 276/276, ruff, mypy green. Post-merge acceptance: re-measure ClosedXML — delta vs the pre-#233 baseline must be exactly -5 with all 263 findings restored. Closes #238 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LsWw4Ay8KLTHFom1HvRu3U
Cloned ClosedXML at the remeasure note's pinned commit (4e89dced), built the core lib + Examples (net8.0), and ran the real own-check pipeline: 119 OWN001 findings, all undisposed XLWorkbook wb/workbook locals — the exact locals the #233 empty-Dispose/Janitor.Fody over-exemption had silenced. On current main they are flagged again, so the disappearing findings have returned on the real repository, not just the isolated #238 pins. Records the scope difference (Examples/net8.0/flow-locals) vs the note's 263-call-site figure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AZjB2ggU9qm8vmuTQRtZXR
- frozen reference marked PROVISIONALLY approved; add the Integration-gate dependency (final semantic-port merge gated on the separate post-batch OSS remeasure run by another agent; point-check does not substitute for it). - reframe the regression as "the #238 regression class introduced by #233 and repaired by #240" — #238 records the problem, it is not a version boundary. - replace the unrestricted "no new false positives" with the bounded claim: the two legitimate IEnumerator<T> empty-Dispose controls stay silent in the regression fixture; broader FP behavior is left to the full OSS remeasure. - add an explicit proven / not-proven guarantee split and the two-branch integration gate (full sweep green -> final; unexplained disappearance -> Python-first fix, stop merge, regenerate golden explicitly, re-parity Rust). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AZjB2ggU9qm8vmuTQRtZXR
3 participants
Что и зачем
Локаль user-defined типа, чей
Dispose()тело буквально пустое, ложно флагалась как OWN001-утечка. Такой тип (напримерIEnumerator<T>-энумератор) реализует IDisposable только ради интерфейса, а не потому что держит ресурс — не вызвать его пустой Dispose нельзя «утечь». Существующая no-op-льгота (IsNoOpDisposeWrapper/IsDisposeOptional) покрывала лишь горстку именованных BCL-типов. Подтверждённый FP в ClosedXML: 5 точек, один корень (Slice.Enumeratorлокали); sweep #201, field-notes запись 19.Фикс — только в экстракторе, без изменений OwnIR:
HasEmptyDisposeBodyраспознаёт по ИСХОДНИКУ тип, чей беспараметрическийDispose()— блок с нулём стейтментов, и в цепочке баз нет своего Dispose для каскада; локаль такого типа отбрасывается из обоих путей local-disposable (flatIsDisposableTypeи--flow-localsImplementsIDisposable).Тип изменения
Как проверено
python tests/run_tests.py— exit 0 (test_ownir276/276)ruff check .иmypy --strictнаownlang/— clean / SuccessГраницы (не перерасширяет)
Handle-поле изOwnIgnoreSample(Alpha follow-up: implement [OwnIgnore("reason")] suppression (P-004) end to end #209, пустой Dispose как leak-контроль) не затронуто.RealResource,LeakyReader).EmptyOverrideOverRealBase).Правка двух существующих фикстур (fidelity)
UnitOfWorkиPixelOwnerиспользовали пустойDispose(){}как упрощение, хотя их же комментарии заявляли реальный ресурс. Сделаны честными (реальное тело Dispose) — остаются настоящими leak-фикстурами, их by-name CI-ассерты ('uow'/'leakedOwner'всё ещё лик) не тронуты:UnitOfWorkтеперь владеет и диспозитCancellationTokenSource;PixelOwner.Disposeочищает бэкинг (Array.Clear).Связанные issue
Closes #225. Найден sweep'ом #201, каталог —
field-notes-patterns.mdзапись 19 (помечена shipped). Разные зоны экстрактора с #218 (rotation) — можно ревьюить параллельно.План валидации на живых репозиториях (после мержа — отдельной задачей)
Перегнать oracle-sweep по ClosedXML и показать дельту FP:
ClosedXML/Excel/Cells/Slice.cs:91,109,149,188,219— пятьSlice.Enumerator-локалей должны уйти изown-only(пустойDispose()вSlice.cs:324-416).Чеклист
field-notes-patterns.mdзапись 19 → shipped)fix:)🤖 Generated with Claude Code
Generated by Claude Code