extractor: sound throw-routing for using / finally-only-try bodies (onThrowDefinite)#160
Conversation
…hrowDefinite) Follow-up to PR #159's deferred item. An explicit throw inside a using (existingLocal) body previously had to bail the whole method (the ThrowStatementSyntax case refuses a non-null onThrow, since a throw inside a try might be caught), losing detection of unrelated leaks; #159 worked around it by gating the using-release out when the body threw. This threads an `onThrowDefinite` flag alongside `onThrow` through the lowering (LowerFlowStmt / LowerFlowStatements / LowerSwitchSection). It is true when a throw that runs `onThrow` definitely leaves the method uncaught — a `using` or a finally-only `try` (neither has a catch) — and false for a `try` with any `catch` (the throw may be caught, a match this lowering does not model). The throw case then routes the throw through the release continuation when definite, and still bails otherwise. Throws inside a `finally` keep bailing (IsInsideFinally guard, unchanged). Net: the using-local release no longer needs the explicit-throw gate (removed); finally-only `try` bodies with an explicit throw are now analysed instead of bailed (sound bonus). try-with-catch is unchanged (still conservative). No existing golden sample has an explicit throw in a using/finally-only-try body, so no expected-output changes. Fixture: using-statement-throw-releases (before.cs: an unrelated leak is still caught despite a throw in the using body — a direct no-bail regression test; after.cs clean). Selftests: corpus 30/30, oracle 31/31, ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
|
Warning Review limit reached
More reviews will be available in 4 minutes and 22 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an ChangesonThrowDefinite lowering and corpus fixture
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/using-statement-throw-releases/case.own`:
- Around line 8-18: The fixture only models Res, so the new tracked using(...)
throw-routing path is not actually covered; add a Guard resource mapping to
case.own alongside the existing resource definitions. Update the fixture so
Guard is acquired/released/emit-mapped like Res, ensuring any using(guard) flow
is tracked and the throw is forced through the release continuation rather than
being ignored.
🪄 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: 1bb95af1-5f07-461a-bbed-a196e8eb964b
📒 Files selected for processing (7)
corpus/real-world/local-dispose-via-using-statement/notes.mdcorpus/real-world/using-statement-throw-releases/after.cscorpus/real-world/using-statement-throw-releases/before.cscorpus/real-world/using-statement-throw-releases/case.owncorpus/real-world/using-statement-throw-releases/expected-diagnostics.txtcorpus/real-world/using-statement-throw-releases/notes.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
…abbit) The OwnLang reduction modelled only the leaked `conn` (Res); add the `Guard` resource and acquire/release it so the reduction mirrors before.cs (guard balanced via the using, conn leaked -> OWN001) rather than leaving the using's resource unmodelled. The extractor throw-routing itself stays validated by the before.cs/after.cs benchmark; case.own pins the ownership outcome. Still OWN001; corpus 30/30. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Что и зачем
Полностью sound-фикс отложенного хвоста из #159. Раньше явный
throwвнутри телаusing (existingLocal)заставлялThrowStatementSyntaxбейлить весь метод (он отказывается от non-nullonThrow, т.к. throw вtryможет быть пойман catch'ем) — терялись несвязанные утечки; в #159 это обходилось гейтом (выключали release при throw в теле).Теперь протягиваю флаг
onThrowDefiniteрядом сonThrowчерез lowering (LowerFlowStmt/LowerFlowStatements/LowerSwitchSection):onThrow, гарантированно покидает метод без перехвата:usingили finally-onlytry(catch'а нет);tryс любымcatch(throw может быть пойман — сопоставление типа с catch мы не моделируем).Throw-case маршрутизирует throw через release-continuation, когда definite, иначе по-прежнему бейлит. Throw внутри
finallyбейлит как раньше (IsInsideFinally).Тип изменения
Как проверено
python tests/test_corpus.py→ 30/30 (новый фикстурusing-statement-throw-releases)oracle_compare.py --selftest→ 31/31ruff check→ cleanСвязанные issue
Follow-up к #159 (отложенный пункт «sound throw-routing для using»).
Чеклист
using-statement-throw-releases(before.cs: несвязанная утечка ловится несмотря на throw в теле using — прямой regression-тест на no-bail; after.cs чисто)onThrowDefinite, caveat вlocal-dispose-via-using-statement)Влияние на существующие тесты
Ни один существующий golden-сэмпл не содержит явного throw в теле
using/finally-only-try(throw'ы там либо body-level —onThrownull, без изменений, либо внутриfinally—IsInsideFinallyбейлит, без изменений).ThrowInFinallyBailsостаётся silent.try-с-catchне меняется (по-прежнему консервативно бейлит). Так что expected-output голденов не меняется — добавляется только новый путь маршрутизации.🤖 Generated with Claude Code
https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Generated by Claude Code
Summary by CodeRabbit
usingblocks so exceptions can still flow through cleanup correctly.usingbody.