Skip to content

Commit 09d3365

Browse files
committed
flow: keep throw inside a finally bailing — fix false leak past an outer finally (Codex P2)
The explicit-throw exit keyed on `onThrow is null && canEscape`, but `finally` bodies are lowered with the default (null) onThrow — so a `throw` INSIDE a finally also took the bare-return branch, terminating before any ENCLOSING finally runs. For `try { try {} finally { throw; } } finally { s.Dispose(); }` that bare exit skips the outer `s.Dispose()` and falsely reports OWN001 on `s`. Guard the body-level throw branch with `!IsInsideFinally(thr)`: a throw lexically inside a finally keeps bailing the whole method (the sound honest-skip it had before this feature), since its real continuation is the OUTER cleanup the bare exit cannot run. Body-level throws (no enclosing try/finally) are unaffected. Sample ThrowInFinallyBails pins the Codex repro as silent; the four body-level throw samples keep their verdicts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
1 parent 7f598b0 commit 09d3365

3 files changed

Lines changed: 41 additions & 2 deletions

File tree

.github/workflows/ci.yml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -712,7 +712,10 @@ jobs:
712712
# tdClean: a Dispose BEFORE an explicit `throw` -> released at the abnormal exit ->
713713
# silent. vtc: a local acquired after a top-level validation throw AND disposed ->
714714
# analysed (no longer bailed) and balanced -> silent (the un-bail must not over-flag).
715-
for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull cda daci cif ctorLater lamPrior other doClean swAll ncf captured shaClean stopped defer ctorMoved handedOwner tdClean vtc; do
715+
# tif (Codex P2): a `throw` inside an inner finally propagates through the OUTER finally
716+
# that disposes it -> the throw-exit keeps BAILING the method (it can't run the enclosing
717+
# cleanup) rather than emit a false leak -> silent.
718+
for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull cda daci cif ctorLater lamPrior other doClean swAll ncf captured shaClean stopped defer ctorMoved handedOwner tdClean vtc tif; do
716719
if echo "$out" | grep -q "'$ok'"; then echo "FAIL: silent/exempt case '$ok' was reported"; exit 1; fi
717720
done
718721
echo "OK: flow-sensitive OWN001/002/003 on real C# (path-sensitive, loops via while/foreach/for, try/finally sequential, never-vs-every-path wording, dispose-optional exempt, beyond flat)"

frontend/roslyn/OwnSharp.Extractor/Program.cs

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -564,6 +564,24 @@ st.Parent is BlockSyntax b
564564
&& b.Statements.Count > 0 && b.Statements[^1] == st
565565
&& b.Parent is not StatementSyntax;
566566

567+
// True when `node` sits lexically inside a `finally { }` block (walking up to the enclosing
568+
// member / lambda boundary). A `throw` there is NOT a clean method exit: it propagates through
569+
// any ENCLOSING `finally`/`try` cleanup, which a bare-return exit would skip — and `finally`
570+
// bodies are lowered with the default (null) `onThrow`, so the body-level throw branch cannot
571+
// tell them apart from the method body. Such a throw therefore keeps BAILING the method (sound
572+
// honest-skip, as before this feature) rather than emit a false leak that misses the outer
573+
// finally's release (Codex P2: `try { try {} finally { throw; } } finally { s.Dispose(); }`).
574+
static bool IsInsideFinally(SyntaxNode node)
575+
{
576+
for (var p = node.Parent; p is not null; p = p.Parent)
577+
{
578+
if (p is FinallyClauseSyntax) return true;
579+
if (p is AnonymousFunctionExpressionSyntax or LocalFunctionStatementSyntax
580+
or BaseMethodDeclarationSyntax or AccessorDeclarationSyntax) return false;
581+
}
582+
return false;
583+
}
584+
567585
// Inject an exceptional-exit edge `if(*){ onThrow }` before a LEAF may-throw statement
568586
// (an expression statement or a local declaration) inside a `try` body. `onThrow` is the
569587
// continuation a throw here runs to leave the method — this try's `finally`, then any
@@ -962,7 +980,9 @@ or ImplicitObjectCreationExpressionSyntax
962980
// the `throw expr;` form.) The win is broad: a method whose only unmodelled
963981
// statement was a top-level validation throw (`if (x is null) throw …;`) is now
964982
// analysed instead of skipped, lighting up every detector on the rest of its body.
965-
if (canEscape && onThrow is null)
983+
// ...and a throw lexically inside a `finally` likewise keeps bailing (IsInsideFinally):
984+
// its real continuation is the OUTER finally/try cleanup, which a bare exit would skip.
985+
if (canEscape && onThrow is null && !IsInsideFinally(thr))
966986
{
967987
nodes.Add(new { op = "return", var = (string?)null, line = LineOf(thr) });
968988
return true;

frontend/roslyn/samples/FlowLocalsSample.cs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,22 @@ public void ValidatedThenClean(object arg)
321321
vtc.Dispose();
322322
}
323323

324+
// NOT a false leak (Codex P2): a `throw` inside the INNER finally is not a clean method exit
325+
// — it propagates through the OUTER finally, which disposes `tif`. A bare-return throw-exit
326+
// can't represent "run the enclosing finally first", and finally bodies are lowered with a
327+
// null onThrow, so a throw lexically inside a finally keeps BAILING the whole method (honest
328+
// skip) rather than emit a false OWN001 that misses the outer release -> silent.
329+
public void ThrowInFinallyBails()
330+
{
331+
var tif = new MemoryStream();
332+
try
333+
{
334+
try { }
335+
finally { throw new InvalidOperationException(); }
336+
}
337+
finally { tif.Dispose(); }
338+
}
339+
324340
// finally-before-return: the early `return` runs the finally (disposing `other`) FIRST,
325341
// then exits — so `other` is released on the return path and stays silent. But `earlyRet`
326342
// is disposed only AFTER the try, which the early return (and the throw on WriteByte) skip

0 commit comments

Comments
 (0)