extractor: address Codex review on PR #159 (no-op writers + using-throw bail)

Two P2 review findings from Codex:

1. Exclude buffered writers from the no-op-dispose allowlist. StreamWriter /
   BinaryWriter FLUSH buffered output on Dispose, so a never-disposed writer
   field can leave even an in-memory backing missing buffered data — a real
   correctness bug, not a managed-memory-only no-op. IsNoOpDisposeWrapper now
   allows only the read-only adapters StreamReader / BinaryReader, whose Dispose
   just discards a read-ahead buffer and closes the (in-memory) backing.

2. Don't bail the whole method on an explicit throw inside a `using (existingLocal)`
   body. Threading a non-null bodyOnThrow makes the ThrowStatementSyntax case
   return false (it refuses a non-null onThrow — a throw inside a try might be
   caught), which skipped the method's entire flow analysis and lost UNRELATED
   leaks. The using-local release is now gated on the body having no explicit
   escaping throw; otherwise it falls through to the plain lowering (method stays
   analysed). Routing using-throws through the release would need invasive
   onThrow-is-unconditional threading — deferred and documented.

Docs/fixtures updated to readers-only + the explicit-throw caveat. Selftests:
oracle 31/31, corpus 29/29, ruff clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm

Author: claude

corpus/real-world/field-noop-dispose-wrapper/notes.md

@@ -1,11 +1,12 @@
 # field-noop-dispose-wrapper
 
 An owned `IDisposable` field whose `Dispose()` releases **nothing real**, because it is a
-BCL pass-through reader/writer wrapping an **in-memory** backing. Generalises the existing
-`StringWriter` / `StringReader` no-op exemption (`IsDisposeOptional`) to the wrapper case:
-a `StreamReader` / `StreamWriter` / `BinaryReader` / `BinaryWriter` constructed over a
-`MemoryStream` / `StringWriter` / `StringReader` cascades its disposal only to that
-managed-memory backing.
+BCL **read-only** pass-through reader wrapping an **in-memory** backing. Generalises the
+existing `StringWriter` / `StringReader` no-op exemption (`IsDisposeOptional`) to the
+wrapper case: a `StreamReader` / `BinaryReader` constructed over a `MemoryStream` /
+`StringWriter` / `StringReader` cascades its disposal only to that managed-memory backing.
+**Writers** (`StreamWriter` / `BinaryWriter`) are excluded — their `Dispose` flushes
+buffered output, so skipping it can drop data (a correctness bug worth flagging).
 
 - **before.cs** — a `StreamReader` field over a real `FileStream` (an OS handle), never
   disposed → `OWN001` (the bug is caught; the backing is **not** in-memory, so the no-op
@@ -16,16 +17,17 @@ managed-memory backing.
 ## Recognition rule
 
 The field-disposal scan exempts a field when **every** construction of it is a no-op
-wrapper (`IsNoOpDisposeWrapper`): the constructed type is one of the four BCL pass-through
-adapters (`StreamReader` / `StreamWriter` / `BinaryReader` / `BinaryWriter`, namespace
-`System.IO`) **and** its first constructor argument resolves to an in-memory
-dispose-optional backing (`MemoryStream` / `StringWriter` / `StringReader`). Requiring
-*all* constructions to qualify keeps it sound: a field also assigned `new StreamReader(
-path)` (which opens a real `FileStream`) on some path still leaks.
+wrapper (`IsNoOpDisposeWrapper`): the constructed type is one of the two BCL read-only
+pass-through adapters (`StreamReader` / `BinaryReader`, namespace `System.IO`) **and** its
+first constructor argument resolves to an in-memory dispose-optional backing
+(`MemoryStream` / `StringWriter` / `StringReader`). Requiring *all* constructions to
+qualify keeps it sound: a field also assigned `new StreamReader(path)` (which opens a real
+`FileStream`) on some path still leaks.
 
-The allowlist is deliberately **closed** to those four types. Other BCL streams that also
-wrap a stream — `GZipStream`, `DeflateStream`, `CryptoStream` — own their *own* extra
-resource (a native deflater, a crypto transform), so they are **not** exempt.
+The allowlist is deliberately **closed**. Writers (`StreamWriter` / `BinaryWriter`) are
+out — their `Dispose` flushes buffered output, so skipping it can drop data. Other BCL
+streams that also wrap a stream — `GZipStream`, `DeflateStream`, `CryptoStream` — own their
+*own* extra resource (a native deflater, a crypto transform), so they are **not** exempt.
 
 ## Honesty caveat — what this does and does not reach
 

corpus/real-world/local-dispose-via-using-statement/notes.md

@@ -33,3 +33,12 @@ Scoped to a `using (identifier)` whose identifier is a **tracked local**. A
 `using (SomeExpr())` whose resource is a method call or member access is not an
 identifier and is not threaded — that resource is the callee's to dispose and is not a
 tracked local here. The declaration forms are unchanged.
+
+Also gated on the body having **no explicit escaping `throw`**. Threading a release onto
+the throw path requires a non-null `bodyOnThrow`, which makes the lowering's
+`ThrowStatementSyntax` case bail the whole method (it refuses a non-null `onThrow` because
+a throw inside a `try` might be caught — a distinction a `using` doesn't need but the
+lowering can't tell apart without invasive threading). So when the body throws explicitly
+we fall through to the plain lowering rather than bail — the method stays analysed (no lost
+recall on unrelated locals), at the cost of leaving `r` itself possibly shown as undisposed
+on the throw path (no worse than before this fix). Caught by Codex P2 on the first cut.

docs/notes/no-op-dispose-wrapper.md

@@ -22,20 +22,29 @@ so disposing the whole chain frees nothing and leaving `_reader` undisposed is n
 
 A field is exempt when **every** construction of it is a no-op wrapper:
 
-1. the constructed type is one of exactly four BCL pass-through adapters —
-   `StreamReader`, `StreamWriter`, `BinaryReader`, `BinaryWriter` (namespace `System.IO`);
-   **and**
+1. the constructed type is one of exactly two BCL **read-only** pass-through adapters —
+   `StreamReader` or `BinaryReader` (namespace `System.IO`); **and**
 2. its **first** constructor argument resolves to an in-memory dispose-optional backing —
    `MemoryStream`, `StringWriter`, or `StringReader` (`IsInMemoryDisposableBacking`).
 
 Requiring *all* constructions to qualify keeps it sound: a field also assigned
 `new StreamReader(path)` on some path opens a real `FileStream`, so it still leaks.
 
+### Why readers only — writers are NOT a no-op
+
+`StreamWriter` and `BinaryWriter` are deliberately **excluded**. A writer's `Dispose`
+**flushes** buffered output to the underlying stream (documented behaviour), so a
+never-disposed writer field can leave even an in-memory backing missing buffered
+characters / encoder state — a real **correctness** bug, not a managed-memory-only no-op.
+The `OWN001` on an undisposed writer is therefore worth keeping (Codex P2 on the first
+cut, which included writers). Only the read-only adapters — whose `Dispose` just discards
+a managed read-ahead buffer and closes the (in-memory) backing — are genuinely no-op.
+
 ### Why a closed allowlist, not "any BCL stream wrapper"
 
 `GZipStream`, `DeflateStream`, and `CryptoStream` also wrap a stream, but each owns its
 **own** extra resource (a native deflater, a crypto transform) whose `Dispose` is not a
-no-op. So the allowlist is deliberately the four pure adapters, never "any `Stream`
+no-op. So the allowlist is deliberately the two read-only adapters, never "any `Stream`
 subtype". The first-argument check is on the backing *type*, so a string path
 (`new StreamReader("file.txt")`, which opens a `FileStream`) and a real-stream backing
 (`new StreamReader(fileStream)`) both fail it and correctly keep leaking.

docs/notes/oracle-known-fps.md

@@ -154,13 +154,15 @@ entry and let the oracle re-confirm clean.
 2. **No-op `Dispose` not modelled — PARTLY FIXED** *(Newtonsoft `TraceJsonReader.
    _textWriter`).* We flag any undisposed `IDisposable` structurally, without modelling
    that the concrete `Dispose` releases nothing. **Shipped fix:** `IsNoOpDisposeWrapper`
-   extends the existing `StringWriter`/`StringReader` field exemption to BCL **pass-through
-   wrappers** — a `StreamReader`/`StreamWriter`/`BinaryReader`/`BinaryWriter` field whose
-   *every* construction wraps an in-memory backing (`MemoryStream`/`StringWriter`/
-   `StringReader`) cascades disposal only to managed memory, so it is dispose-optional. The
-   allowlist is closed to those four adapters (not "any BCL stream": `GZipStream`/
-   `CryptoStream` own a native/extra resource), and a path that builds `new StreamReader(
-   path)` (a real file handle) keeps the field flagged. Corpus fixture
+   extends the existing `StringWriter`/`StringReader` field exemption to BCL **read-only
+   pass-through readers** — a `StreamReader`/`BinaryReader` field whose *every* construction
+   wraps an in-memory backing (`MemoryStream`/`StringWriter`/`StringReader`) cascades
+   disposal only to managed memory, so it is dispose-optional. **Writers** (`StreamWriter`/
+   `BinaryWriter`) are excluded: their `Dispose` flushes buffered output, so a never-disposed
+   writer can drop data — a real bug the OWN001 keeps flagging (Codex P2). The allowlist is
+   closed to those two readers (not "any BCL stream": `GZipStream`/`CryptoStream` own a
+   native/extra resource), and a path that builds `new StreamReader(path)` (a real file
+   handle) keeps the field flagged. Corpus fixture
    `field-noop-dispose-wrapper`; full rationale
    [`no-op-dispose-wrapper.md`](no-op-dispose-wrapper.md). **Still open — the soundness
    wall:** Newtonsoft's `_textWriter` is a `JsonTextWriter` over a `StringWriter` —

frontend/roslyn/OwnSharp.Extractor/Program.cs

@@ -886,24 +886,28 @@ t is not null
     && t.ContainingNamespace?.ToString() == "System.IO"
     && t.Name is "MemoryStream" or "StringWriter" or "StringReader";
 
-// A BCL "pass-through" reader/writer — StreamReader / StreamWriter / BinaryReader /
-// BinaryWriter — whose only disposable state is the stream/writer it wraps (plus managed
-// char/byte buffers). Its Dispose() merely cascades to that backing, so when the backing is
-// an in-memory dispose-optional value the whole chain frees nothing and an undisposed field
-// of this shape is not a leak. Deliberately a CLOSED allowlist of these four, NOT "any BCL
-// stream": GZipStream / DeflateStream / CryptoStream wrap a stream too but own their OWN
-// extra resource (a native deflater, a crypto transform), so they must STAY flagged. A
-// THIRD-PARTY wrapper is also excluded — e.g. Newtonsoft's `JsonTextWriter` over a
-// StringWriter is structurally the same no-op, but we cannot prove ITS Dispose is
-// pass-through without modelling its body, so it stays a (baselined) finding, never a silent
-// drop. The backing is the FIRST ctor argument: `new StreamReader(path)` (a string path opens
-// a real FileStream) and `new StreamReader(fileStream)` both fail the in-memory check and so
-// correctly keep leaking. See docs/notes/no-op-dispose-wrapper.md.
+// A BCL READ-ONLY "pass-through" reader — StreamReader / BinaryReader — whose only
+// disposable state is the stream it wraps (plus a managed read-ahead buffer that is simply
+// discarded). Its Dispose() merely cascades to that backing, so when the backing is an
+// in-memory dispose-optional value the whole chain frees nothing and an undisposed field of
+// this shape is not a leak. WRITERS are deliberately EXCLUDED: StreamWriter / BinaryWriter
+// FLUSH buffered output to the underlying stream on Dispose (documented behaviour), so a
+// never-disposed writer field can leave the backing missing buffered characters / encoder
+// state — a real correctness bug, not a managed-memory-only no-op, which the OWN001 still
+// usefully flags (Codex P2). A CLOSED allowlist of those two readers, NOT "any BCL stream":
+// GZipStream / DeflateStream / CryptoStream wrap a stream too but own their OWN extra
+// resource (a native deflater, a crypto transform), so they must STAY flagged. A THIRD-PARTY
+// wrapper is also excluded — e.g. Newtonsoft's `JsonTextWriter` over a StringWriter is
+// structurally a no-op, but we cannot prove ITS Dispose is pass-through without modelling its
+// body, so it stays a (baselined) finding, never a silent drop. The backing is the FIRST ctor
+// argument: `new StreamReader(path)` (a string path opens a real FileStream) and
+// `new StreamReader(fileStream)` both fail the in-memory check and so correctly keep leaking.
+// See docs/notes/no-op-dispose-wrapper.md.
 static bool IsNoOpDisposeWrapper(BaseObjectCreationExpressionSyntax oce, SemanticModel model)
 {
     if (model.GetTypeInfo(oce).Type is not { } wt
         || wt.ContainingNamespace?.ToString() != "System.IO"
-        || wt.Name is not ("StreamReader" or "StreamWriter" or "BinaryReader" or "BinaryWriter"))
+        || wt.Name is not ("StreamReader" or "BinaryReader"))
         return false;
     var arg0 = oce.ArgumentList?.Arguments.FirstOrDefault()?.Expression;
     return arg0 is not null && IsInMemoryDisposableBacking(model.GetTypeInfo(arg0).Type);
@@ -1470,8 +1474,22 @@ or ImplicitObjectCreationExpressionSyntax
             // was already acquired at its `new`; we only add the missing release. The `using
             // (var x = ...)` declaration form needs nothing: `x` is auto-disposed and never
             // tracked as a leak candidate.
+            //
+            // GATED on the body having no EXPLICIT escaping `throw`: threading a non-null
+            // bodyOnThrow makes the `ThrowStatementSyntax` case bail the whole method (it
+            // refuses a non-null onThrow — a throw inside a `try` might be caught, which a bare
+            // exit can't model). A `using` has no catch, so routing a throw through the release
+            // IS sound, but the lowering can't tell using-onThrow from try-onThrow without
+            // invasive threading. Rather than regress recall (bailing every method with an
+            // explicit throw in such a body, losing UNRELATED leaks too — Codex P2), fall
+            // through to the plain body lowering when the body throws explicitly; that keeps the
+            // method analysed (status quo for this rare expression form — `r` itself may still
+            // show as undisposed on the throw path, no worse than before this fix).
             if (us.Expression is IdentifierNameSyntax usingId
-                && tracked.Contains(usingId.Identifier.Text))
+                && tracked.Contains(usingId.Identifier.Text)
+                && !(us.Statement?.DescendantNodes(n =>
+                            n is not (AnonymousFunctionExpressionSyntax or LocalFunctionStatementSyntax))
+                        .OfType<ThrowStatementSyntax>().Any() ?? false))
             {
                 var owner = usingId.Identifier.Text;
                 var exit = new List<object> { new { op = "return", var = (string?)null, line = LineOf(us) } };