fix(extractor): confine the empty-Dispose exemption to enumerators — source emptiness ≠ runtime no-op (Closes #238)#240
Conversation
…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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 34 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 (8)
✨ 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: 0c1c2f3bab
ℹ️ 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".
…rces (Codex on #240) A Fody-enabled project can compile a linked source that lives OUTSIDE its directory (<Compile Include="../Shared/Foo.cs">) — the ancestor walk from the FILE path never sees the FodyWeavers.xml sitting next to the .csproj, so a linked empty-Dispose enumerator kept the exemption the kill-switch exists to withhold. ProjectCsFiles now records every file a weaver-enabled project compiles (Program.WeaverOwnedFiles, filled while expanding <Compile> items); SourceTreeHasWeaverConfig consults that registry before the ancestor walk. Directory/bare-file scans carry no project info and keep today's honest per-path behaviour. Pinned by the weaved-linked/ fixture (Proj/Linked.csproj + FodyWeavers.xml linking ../Shared/SharedEnumerator.cs) + CI assertion: analysed THROUGH the .csproj the local is flagged; verified locally both ways, full-sample flat and weaved outputs byte-identical to the pre-fix run. Refs #238 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LsWw4Ay8KLTHFom1HvRu3U
|
@coderabbitai review |
…spose gate (review) Follow-up review on #240 found the hotfix still left soundness gaps. All fixed: - Inherited IAsyncDisposable (P1): the async-cleanup check only looked at members declared on the type, so an enumerator with an empty sync Dispose whose real DisposeAsync lives on a BASE (that base not implementing IDisposable) was silently exempted — the exact #238 false-negative class. Now ANY IAsyncDisposable in AllInterfaces (inherited included) disqualifies. New flagged control InheritedAsyncEnumerator ('ia'), flow count 5 -> 6. - Over-broad enumerator gate (P2): non-generic System.Collections.IEnumerator does NOT extend IDisposable, so pairing them is author-chosen, not forced — the motivating proof only covers IEnumerator<T>. Narrowed to the generic interface, matched by name/arity/namespace (the well-known-type SpecialType is unset in the source-only extraction, so a SpecialType check would wrongly reject legitimate generic enumerators). - Sticky static registry (P2): WeaverOwnedFiles was never reset, so a file seen as weaver-owned leaked into a later in-process invocation (sticky FP). Cleared at program start next to the existing BodyThrowEdges reset. - Fail-open weaver walk (P3): File.Exists returns false on access-denied, swallowing the case the comment claimed to fail closed on. Switched to File.GetAttributes, which throws on a permission error while still reporting a plain not-found — genuinely fail-closed now. Verified with the real extractor: flow flags exactly s,r,lr,d,ar,ia with the two generic enumerators (e,x) silent; weaved + weaved-linked fixtures flagged; full-sample-set diff vs main byte-identical. run_tests 276/276, ruff, mypy green. Refs #238 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LsWw4Ay8KLTHFom1HvRu3U
| // owns a real DisposeAsync -> an inherited IAsyncDisposable must still leak. | ||
| public int LeakInheritedAsync() | ||
| { | ||
| var ia = new InheritedAsyncEnumerator(); |
|
Thanks for the review — every point was correct. All four code findings fixed in 1. Inherited 2. Over-broad enumerator gate (P2) — fixed, narrowed to 3. Sticky 4. 5. Acceptance / ClosedXML remeasure — you were right that deferring the main regression test was backwards. I ran it on real ClosedXML source (HEAD
The restored findings are exactly the swallowed class — One correction to the acceptance criterion itself: it expected "−5 Slice points removed". That's actually unsound here — ClosedXML ships its own Generated by Claude Code |
…r regression pin (review r2) Second review round on #240: - Bare DisposeAsync on a BASE (P2): the belt-and-braces async check used nt.GetMembers() (current type only), so a base with a bare `public ValueTask DisposeAsync()` that does NOT implement IAsyncDisposable slipped its real async cleanup through — AllInterfaces misses it (no interface) and the member scan misses it (inherited). Now walks the whole base chain. New flagged control BareInheritedAsyncEnumerator ('ba'). - Non-generic IEnumerator regression pin (P2): the IEnumerator<T>-only narrowing had no negative fixture. Added LegacyEnumerator (IEnumerator + IDisposable, empty Dispose) -> must stay flagged ('ng'). Flow count 6 -> 8. - Stale docs (P3): the HasEmptyDisposeBody header, the sample-file header still said "generalises to any type" — the exact over-broad framing that produced #238. Rewritten to the actual IEnumerator<T>-only scope. Verified with the real extractor: flow flags exactly s,r,lr,d,ar,ia,ba,ng with the two generic enumerators (e,x) silent; full-sample diff vs main byte-identical. run_tests 276/276, ruff, mypy green. Refs #238 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LsWw4Ay8KLTHFom1HvRu3U
| // base-chain scan must catch it -> LEAK. | ||
| public int LeakBareInheritedAsync() | ||
| { | ||
| var ba = new BareInheritedAsyncEnumerator(); |
| // the IEnumerator<T>-only narrowing must keep it flagged -> LEAK. | ||
| public int LeakLegacy() | ||
| { | ||
| var ng = new LegacyEnumerator(); |
|
Acceptance amended by PR #240: because ClosedXML itself is Fody-enabled, the sound result is restore-all/remove-none, not −5. The five Slice points must remain flagged under the project-level weaver kill-switch. |
Two wording inaccuracies flagged on PR #242, both verified against frontend/roslyn/OwnSharp.Extractor/Program.cs HasEmptyDisposeBody: - The #240 gate matches only the generic System.Collections.Generic. IEnumerator<T> (checked by name + TypeArguments.Length == 1 + namespace), not the non-generic System.Collections.IEnumerator. Fixed in both docs/notes/field-notes-patterns.md entry 19 and docs/notes/precision-remeasure-2026-07-11.md's resolution addendum. - DisposeAsync() is not "recognized as empty" — any IAsyncDisposable or bare DisposeAsync (declared, explicit-impl, or inherited) disqualifies the exemption outright. Only explicit-interface synchronous Dispose() is recognized as empty (the actual #238 coverage-gap fix).
- 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
Что и зачем
Хотфикс issue #238 — soundness-регрессии от #225/PR #233, пойманной перезамером (PR #235):
XLWorkbook.Dispose()пуст только в исходнике (Janitor.Fody вплетает очистку на компиляции), аHasEmptyDisposeBodyверил source-дереву → 263 тихо проглоченные находки на ClosedXML. Гейт сужен до одного мотивирующего класса —IEnumerator<T>(единственный интерфейс, который принуждает реализоватьIDisposable); всё остальное сохраняет честный warning. Плюс defense-in-depth:FodyWeavers.xmlнад исходником (или у владеющего проекта — для linked-источников) отключает льготу даже для энумератора. Explicit-interfaceDispose/DisposeAsyncраспознаются. По двум раундам ревью закрыты все soundness-щели: наследованныйIAsyncDisposable(черезAllInterfaces), наследованный bareDisposeAsync(через обход base chain), sticky static-registry (Clear()на старте), fail-open weaver-walk (File.GetAttributesвместоFile.Exists).Тип изменения
Как проверено
python tests/run_tests.py— 276/276;ruff+mypycleanлокально через настоящий экстрактор (.NET 8), оба режима — flow флагает ровно 8 контролей (
s,r,lr,d,ar,ia,ba,ng), оба энумератора (eplain,xexplicit-interface) молчат; weaved + weaved-linked фикстуры флагаются; байт-дифф по всем остальным сэмплам против main идентиченконтрольный перезамер на реальном ClosedXML (HEAD
4e89dce, библиотека +ClosedXML.Examplesвместе,--flow-locals), branch vs main(fix(extractor): exempt locals of user types with a provably-empty Dispose() (Closes #225) #233):workbook+ 39wb+ 1wbSource+ 2 enumerator)Восстановленные — ровно проглоченный класс (
XLWorkbook-локали в Examples). Удалено 0: soundness-фикс только возвращает находки. Детали и вывод — в комментарии к PR.Поправка к acceptance issue
Исходный acceptance «−5 Slice-точек, ровно 263 назад» оказался сам несаунд: ClosedXML везёт свой
FodyWeavers.xml/<Janitor/>(ClosedXML/FodyWeavers.xml, надSlice.csиXLWorkbook.cs), поэтому kill-switch корректно снимает льготу со всего ClosedXML — Slice-энумераторы остаются флагнутыми (они среди 2 восстановленных enumerator-локалей, а не удалённых): Janitor-вплетаемыйDisposeнельзя доказать no-op из исходника. Саунд-исход — «ClosedXML полностью восстановлен до состояния до #233, ничего не проглочено», а не «−5». Расхождение 121-vs-263 — новее HEAD + только--flow-locals+ loose-file resolution против полногоown-checksweep'а; несущее свойство (restore-all, remove-none) держится.Связанные issue
Closes #238. Refs #225, PR #233, PR #235.
Чеклист
IEnumerator<T>-only scope🤖 Generated with Claude Code
https://claude.ai/code/session_01LsWw4Ay8KLTHFom1HvRu3U