-
Notifications
You must be signed in to change notification settings - Fork 0
extractor: sound throw-routing for using / finally-only-try bodies (onThrowDefinite) #160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| using System; | ||
|
|
||
| sealed class Guard : IDisposable { public void Dispose() { } } | ||
| sealed class Res : IDisposable { public void Work() { } public void Dispose() { } } | ||
|
|
||
| static class Demo | ||
| { | ||
| // FIX: `conn` is disposed (using-declared). The `using (guard)` body still throws, and the | ||
| // extractor routes that throw through guard's release — the method stays analysed (the throw | ||
| // does not bail it) and is clean: `guard` released on every path, `conn` auto-disposed. | ||
| public static void Run(bool bad) | ||
| { | ||
| var guard = new Guard(); | ||
| using var conn = new Res(); | ||
| using (guard) | ||
| { | ||
| if (bad) throw new InvalidOperationException(); | ||
| conn.Work(); | ||
| } | ||
| } | ||
| } |
22 changes: 22 additions & 0 deletions
22
corpus/real-world/using-statement-throw-releases/before.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| using System; | ||
|
|
||
| sealed class Guard : IDisposable { public void Dispose() { } } | ||
| sealed class Res : IDisposable { public void Work() { } public void Dispose() { } } | ||
|
|
||
| static class Demo | ||
| { | ||
| // The `using (guard)` body contains an explicit `throw`. The extractor must NOT bail the | ||
| // whole method on that throw (which would hide the UNRELATED `conn` leak) — it routes the | ||
| // throw through the using's release instead (onThrowDefinite). So `conn`, never disposed | ||
| // on any path, is still caught → OWN001; `guard` is released on every path and is silent. | ||
| public static void Run(bool bad) | ||
| { | ||
| var guard = new Guard(); | ||
| var conn = new Res(); // BUG: never disposed -> OWN001 | ||
| using (guard) | ||
| { | ||
| if (bad) throw new InvalidOperationException(); | ||
| conn.Work(); | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| // OwnLang model: a method with an explicit throw inside a `using (guard)` body must still | ||
| // report an UNRELATED undisposed local (`conn`). `guard` is acquired and released (the using's | ||
| // scope-exit dispose); `conn` is acquired and never released -> OWN001. The throw-routing | ||
| // itself — release on the throw path, no whole-method bail — is an EXTRACTOR-level concern | ||
| // (the onThrowDefinite flag) exercised end-to-end by before.cs/after.cs in the dotnet | ||
| // benchmark; this pure-OwnLang reduction pins the ownership OUTCOME the extractor must produce: | ||
| // guard balanced, conn leaked. See notes.md. | ||
| module Corpus | ||
| resource Guard { | ||
| acquire create | ||
| release dispose | ||
| kind "disposable" | ||
| emit_type "Guard" | ||
| emit_acquire "new Guard()" | ||
| emit_release "{0}.Dispose()" | ||
| } | ||
| resource Res { | ||
| acquire create | ||
| release dispose | ||
| kind "disposable" | ||
| emit_type "Res" | ||
| emit_acquire "new Res()" | ||
| emit_release "{0}.Dispose()" | ||
| } | ||
| fn Run(bad: int) { | ||
| let guard = acquire Guard(); // var guard = new Guard(); | ||
| let conn = acquire Res(); // var conn = new Res(); never disposed -> OWN001 | ||
| release guard; // using (guard) { ... } disposes guard on every exit | ||
| } | ||
1 change: 1 addition & 0 deletions
1
corpus/real-world/using-statement-throw-releases/expected-diagnostics.txt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| OWN001 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| # using-statement-throw-releases | ||
|
|
||
| The sound handling of an **explicit `throw` inside a `using (existingLocal)` body** — the | ||
| follow-up to [`local-dispose-via-using-statement`](../local-dispose-via-using-statement/notes.md), | ||
| which first deferred this case (Codex P2 on PR #159). | ||
|
|
||
| When the `using`-local release was threaded as a non-null `bodyOnThrow`, the | ||
| `ThrowStatementSyntax` lowering bailed the **whole method** (it refuses a non-null `onThrow` | ||
| because a throw inside a `try` might be caught). That lost detection of UNRELATED leaks in | ||
| any method with such a body. This fixture pins the fixed behaviour. | ||
|
|
||
| - **before.cs** — an unrelated local `conn` is never disposed → `OWN001`. Crucially, the | ||
| `using (guard)` body contains `if (bad) throw …;`. The bug is only caught if the throw | ||
| does **not** bail the method — so this fixture is a direct regression test for the | ||
| no-bail fix. `guard` is released on every path (normal, throw, return) and stays silent. | ||
| - **after.cs** — `conn` is `using`-declared (auto-disposed); the body still throws, and the | ||
| method stays analysed and **clean**. | ||
|
|
||
| ## Recognition rule | ||
|
|
||
| `LowerFlowStmt`/`LowerFlowStatements`/`LowerSwitchSection` thread an `onThrowDefinite` flag | ||
| beside `onThrow`. 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). The `ThrowStatementSyntax` case: | ||
|
|
||
| - `onThrow is null` (method level) → bare exit, as before; | ||
| - `onThrow` non-null **and** `onThrowDefinite` → route the throw through `onThrow` (the | ||
| release continuation, ending in exit), so the cleanup's resource is released on the throw | ||
| path and the method stays analysed; | ||
| - `onThrow` non-null and **not** definite (try-with-catch) → still bail. | ||
|
|
||
| ## Honesty caveat | ||
|
|
||
| The definite routing covers `using` statements and finally-only `try`s. A `try` with a | ||
| `catch` still bails an explicit throw in its body — modelling that soundly needs matching | ||
| the thrown type against each catch clause, which the lowering does not do. That is the same | ||
| conservative posture as before; this change only *adds* the provably-uncaught cases. |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.