Skip to content

Commit 957dcb7

Browse files
committed
docs: Newtonsoft _textWriter is not soundly auto-fixable — shelve the recursive Dispose-no-op idea
Investigated the deferred "recursive Dispose-is-a-no-op" feature against its only motivating case (Newtonsoft TraceJsonReader._textWriter). Reading the real JsonTextWriter source settles it: disposal is NOT a no-op type. - Close() -> base.Close() auto-completes open JSON tokens (writes closing brackets to the underlying writer), then CloseBufferAndWriter(). - CloseBufferAndWriter() RETURNS the rented _writeBuffer to _arrayPool when an ArrayPool is configured — a real pooled-buffer release (the POOL-leak class Own.NET tracks) — and closes the underlying writer. So a recursive Dispose-no-op recognizer would be unsound (it can leak a pooled buffer) or would correctly decline — either way it does not clear this finding. The _textWriter instance is benign only by instance facts (no ArrayPool set + the sink is a StringWriter), not a type-level no-op — the same reason writers are excluded from IsNoOpDisposeWrapper. Shelved as not worth building, not deferred. Sharpened the baseline entry reason and updated no-op-dispose-wrapper.md + oracle-known-fps.md root-cause #2 with the evidence. No code change. Selftests: oracle 31/31, corpus 30/30. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
1 parent 47c46a6 commit 957dcb7

3 files changed

Lines changed: 45 additions & 27 deletions

File tree

‎corpus/oracle-fp-baseline.txt‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,16 @@ protobuf-net/protobuf-net | ProtoTranscoder.cs | OWN001 | 'sync' | non-product (
4949
protobuf-net/protobuf-net | Page.xaml.cs | OWN001 | local 'timer' | non-product (assorted/ Silverlight perf demo); TRUE POSITIVE — `timer` is a custom IDisposable Nuxleus.Performance.Stopwatch genuinely never disposed (the using(timer) block is commented out). Baselined as non-product, not as an FP
5050

5151
# --- JamesNK/Newtonsoft.Json ----------------------------------------------------
52-
# STAYS BASELINED (soundness wall): JsonTextWriter is a THIRD-PARTY wrapper — the BCL pass-through
53-
# exemption (IsNoOpDisposeWrapper) covers only StreamReader/StreamWriter/BinaryReader/BinaryWriter,
54-
# whose disposal contract is known. Proving JsonTextWriter.Dispose is a no-op needs body analysis we
55-
# don't do. See docs/notes/no-op-dispose-wrapper.md.
56-
JamesNK/Newtonsoft.Json | TraceJsonReader.cs | OWN001 | _textWriter | no-op dispose: a JsonTextWriter over an in-memory StringWriter/StringBuilder holds no unmanaged resource, so leaving it undisposed releases nothing
52+
# STAYS BASELINED — and NOT soundly auto-fixable (investigated 2026-06-28, JsonTextWriter source).
53+
# JsonTextWriter is NOT a no-op type to dispose: Close() runs base.Close() (auto-completes open JSON
54+
# tokens — writes closing brackets to the underlying writer) and CloseBufferAndWriter(), which RETURNS
55+
# its rented `_writeBuffer` to `_arrayPool` when an ArrayPool is configured — a real pooled-buffer
56+
# release (the very POOL-leak class Own.NET tracks elsewhere) — and closes the underlying writer. So a
57+
# recursive "Dispose-is-a-no-op" recognizer would be UNSOUND (it can leak a pooled buffer) or would
58+
# correctly DECLINE — either way it does not clear this. The `_textWriter` instance is benign only by
59+
# INSTANCE facts (no ArrayPool set + the sink is a StringWriter), not a type-level no-op, so it stays
60+
# baselined — consistent with excluding writers from IsNoOpDisposeWrapper. See no-op-dispose-wrapper.md.
61+
JamesNK/Newtonsoft.Json | TraceJsonReader.cs | OWN001 | _textWriter | benign-by-instance, NOT a no-op type: JsonTextWriter.Close() returns a (possibly pooled) write buffer and auto-completes JSON tokens; harmless here only because no ArrayPool is set and the sink is a StringWriter — kept baselined (not soundly auto-fixable)
5762
JamesNK/Newtonsoft.Json | JsonSerializer.cs | OWN001 | serializer.Error | intra-call self-subscription: the serializer is freshly created from the same JsonSerializerSettings whose .Error handler it subscribes; source and handler are co-lifetimed
5863

5964
# --- JoshClose/CsvHelper --------------------------------------------------------

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

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -55,22 +55,31 @@ The motivating oracle finding was Newtonsoft.Json's `TraceJsonReader._textWriter
5555

5656
```csharp
5757
_sw = new StringWriter(CultureInfo.InvariantCulture);
58-
_textWriter = new JsonTextWriter(_sw); // a no-op in truth — but JsonTextWriter is third-party
58+
_textWriter = new JsonTextWriter(_sw); // looks like a no-op — but JsonTextWriter is NOT a no-op type
5959
```
6060

61-
Structurally identical, but `JsonTextWriter` is a **third-party** wrapper (`JsonWriter`,
62-
not a BCL `TextWriter`). We cannot prove *its* `Dispose` is pass-through without modelling
63-
its body — and asserting a no-op we cannot prove is exactly the unsound over-reach that
64-
sank the static-class subscriber exemption (see
65-
[`oracle-known-fps.md` → Rejected approaches](oracle-known-fps.md)). So Newtonsoft's
66-
`_textWriter` stays a **baselined** finding in
67-
[`oracle-fp-baseline.txt`](../../corpus/oracle-fp-baseline.txt), not a silent drop.
68-
69-
Retiring that baseline entry would need a general, recursive "Dispose is a no-op"
70-
analysis over a first-party type's body (it disposes only dispose-optional members and
71-
holds no unmanaged handle). That is a larger, higher-risk capability; until it lands, only
72-
the BCL pass-through adapters — whose disposal contract is documented and fixed — are
73-
exempted.
61+
It looks structurally identical to the BCL reader case, but a 2026-06-28 read of the real
62+
`JsonTextWriter` source shows it is **not a no-op type to dispose**:
63+
64+
- `Close()` runs `base.Close()` — which **auto-completes open JSON tokens** (writes closing
65+
brackets to the underlying writer) — then `CloseBufferAndWriter()`;
66+
- `CloseBufferAndWriter()` **returns its rented `_writeBuffer` to `_arrayPool`** when an
67+
`ArrayPool` is configured — a real **pooled-buffer release** (the very POOL-leak class
68+
Own.NET tracks elsewhere) — and closes the underlying writer.
69+
70+
So disposal has genuine side effects (a conditional pooled-buffer return, token-completion
71+
writes). This is the same reason we **exclude writers** (`StreamWriter`/`BinaryWriter`) from
72+
`IsNoOpDisposeWrapper` — a writer's `Dispose` does real work. A recursive "Dispose is a
73+
no-op" recognizer over the body would therefore either be **unsound** (it can leak a pooled
74+
buffer) or correctly **decline** — so it would not clear this finding regardless. Asserting a
75+
no-op we cannot prove is exactly the unsound over-reach that sank the static-class subscriber
76+
exemption (see [`oracle-known-fps.md` → Rejected approaches](oracle-known-fps.md)).
77+
78+
The `_textWriter` *instance* is benign only by **instance facts** — no `ArrayPool` is set on
79+
it, and its sink is a `StringWriter` — not by any type-level no-op property. So it stays a
80+
**baselined** finding in [`oracle-fp-baseline.txt`](../../corpus/oracle-fp-baseline.txt), not
81+
a silent drop, and the "recursive Dispose-no-op analysis" idea is **shelved as not worth
82+
building** (it wouldn't soundly clear the one case that motivated it).
7483

7584
## Corpus
7685

‎docs/notes/oracle-known-fps.md‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -178,14 +178,18 @@ entry and let the oracle re-confirm clean.
178178
native/extra resource), and a path that builds `new StreamReader(path)` (a real file
179179
handle) keeps the field flagged. Corpus fixture
180180
`field-noop-dispose-wrapper`; full rationale
181-
[`no-op-dispose-wrapper.md`](no-op-dispose-wrapper.md). **Still open — the soundness
182-
wall:** Newtonsoft's `_textWriter` is a `JsonTextWriter` over a `StringWriter` —
183-
structurally the same no-op, but `JsonTextWriter` is a **third-party** wrapper whose
184-
`Dispose` we cannot prove is pass-through without modelling its body. Suppressing it
185-
would be the same unsound over-reach as the rejected static-class exemption, so it
186-
**stays baselined**. Retiring it needs a general recursive "Dispose-is-a-no-op" body
187-
analysis (a first-party type that disposes only dispose-optional members and holds no
188-
unmanaged handle) — larger and higher-risk; deferred. Note the NLog `_xmlSource` /
181+
[`no-op-dispose-wrapper.md`](no-op-dispose-wrapper.md). **Stays baselined — and NOT
182+
soundly auto-fixable (investigated 2026-06-28):** Newtonsoft's `_textWriter` is a
183+
`JsonTextWriter` over a `StringWriter`, but reading the real `JsonTextWriter` source shows
184+
it is **not a no-op type** — `Close()` runs `base.Close()` (auto-completes open JSON
185+
tokens, writing closing brackets) and `CloseBufferAndWriter()`, which **returns its rented
186+
`_writeBuffer` to `_arrayPool`** when an `ArrayPool` is set (a real pooled-buffer release,
187+
the POOL-leak class we track) and closes the writer. So a recursive "Dispose-is-a-no-op"
188+
recognizer would be **unsound** (it can leak a pooled buffer) or correctly **decline** —
189+
either way it would not clear this. The instance is benign only by **instance facts** (no
190+
`ArrayPool` set + the sink is a `StringWriter`), not a type-level no-op — the same reason
191+
we exclude writers from `IsNoOpDisposeWrapper`. The recursive-analysis idea is therefore
192+
**shelved as not worth building**, not merely deferred. Note the NLog `_xmlSource` /
189193
`_reusable*Stream` reals are also third-party wrappers (`CharEnumerator`,
190194
`ReusableStreamCreator`) of the same benign shape and stay **kept visible** for the
191195
same reason — we can't prove their disposal is a no-op, so we don't silently drop them.

0 commit comments

Comments
 (0)