feat(p1a): curate XmlReader/XmlWriter/JsonDocument as owned factories#153
Conversation
P1a stdlib contract pack (producer side): extend the curated owned-returning
factory recognition with three high-value, low-FP BCL families whose result the
caller must dispose — `XmlReader.Create`, `XmlWriter.Create` (System.Xml) and
`JsonDocument.Parse` (System.Text.Json, which pools memory and is a common real
leak when dropped). Same producer-half contract as File.Open* / crypto Create*:
a `var doc = JsonDocument.Parse(json)` that drops `doc` now surfaces OWN001 at
the factory call, and use/double-dispose of the result is OWN002/OWN003.
Both synced halves are updated in lockstep:
- extractor `IsOwningFactory` (Program.cs) — gates whether to EMIT the factory
call fact, by resolved symbol (static + result implements IDisposable + the
type/namespace), so a Task-returning `JsonDocument.ParseAsync` is excluded;
- bridge `_BCL_FRESH_BY_NS` (ownir.py) — recognises the callee name as `fresh`.
Bridge half is covered by new tests (leak → OWN001 for each, bare and
namespace-qualified; a disposed result stays clean): ownir 206/206, ownership
44/44, mypy --strict, ruff all green locally.
NOTE: this environment has no dotnet, so the extractor (C#) half is not built
locally — it is verified by CI (golden C# compiles & runs, C# leak extractor,
and the real-C# corpus benchmark for recall/specificity).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFactory detection now recognizes ChangesBCL factory recognition update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_ownir.py (1)
1697-1709: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit fully-qualified
System.Xmlregressions.Lines 1697-1709 only cover the qualified form for
JsonDocument.Parse. Since this PR also addsSystem.Xml.XmlReader.CreateandSystem.Xml.XmlWriter.Create, please include those two FQNs here as well so the new entries are pinned end-to-end.Suggested diff
- for fresh_callee, ln in (("XmlReader.Create", 5), ("XmlWriter.Create", 6), - ("JsonDocument.Parse", 7), - ("System.Text.Json.JsonDocument.Parse", 8)): + for fresh_callee, ln in (("XmlReader.Create", 5), + ("System.Xml.XmlReader.Create", 6), + ("XmlWriter.Create", 7), + ("System.Xml.XmlWriter.Create", 8), + ("JsonDocument.Parse", 9), + ("System.Text.Json.JsonDocument.Parse", 10)):🤖 Prompt for 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. In `@tests/test_ownir.py` around lines 1697 - 1709, Extend the regression coverage in the existing P1a acquire/leak test loop by adding explicit fully-qualified System.Xml cases for XmlReader.Create and XmlWriter.Create, alongside the current JsonDocument.Parse FQNs. Update the fresh_callee list in the test block around _bcl so both System.Xml.XmlReader.Create and System.Xml.XmlWriter.Create are asserted to leak as OWN001 when unreleased, matching the pattern already used for System.Text.Json.JsonDocument.Parse.
🤖 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.
Nitpick comments:
In `@tests/test_ownir.py`:
- Around line 1697-1709: Extend the regression coverage in the existing P1a
acquire/leak test loop by adding explicit fully-qualified System.Xml cases for
XmlReader.Create and XmlWriter.Create, alongside the current JsonDocument.Parse
FQNs. Update the fresh_callee list in the test block around _bcl so both
System.Xml.XmlReader.Create and System.Xml.XmlWriter.Create are asserted to leak
as OWN001 when unreleased, matching the pattern already used for
System.Text.Json.JsonDocument.Parse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5eaed82d-49f6-4385-9bbe-10694d99a301
📒 Files selected for processing (3)
frontend/roslyn/OwnSharp.Extractor/Program.csownlang/ownir.pytests/test_ownir.py
CodeRabbit: the P1a leak loop only exercised the FQN form for JsonDocument.Parse. Add System.Xml.XmlReader.Create and System.Xml.XmlWriter.Create FQNs too, so each new table entry is pinned end-to-end (bare + namespace-qualified). ownir 208/208, ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77bfc07d69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && ((sym.Name == "Create" | ||
| && sym.ContainingType is { Name: "XmlReader" or "XmlWriter" } xt | ||
| && IsInNamespace(xt, "System", "Xml")) |
There was a problem hiding this comment.
Please don't classify all XmlReader.Create/XmlWriter.Create overloads as pure factories without also handling their disposable inputs. The extractor's escape pass removes any candidate disposable passed as an argument, and this factory branch later emits only an acquire for the reader/writer; in var fs = File.OpenRead(p); var xr = XmlReader.Create(fs); xr.Dispose();, fs is dropped from tracking even though XmlReaderSettings.CloseInput defaults to false, so the caller-owned stream leak is suppressed. Either restrict the matched overloads or model these inputs as borrowed/non-escaping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 83edff6. You're right that the Stream/TextReader/TextWriter overloads don't own their input (CloseInput/CloseOutput default false, and JsonDocument.Parse never closes its stream), so classifying them as pure factories let the escape pass drop the caller-owned input and suppress its leak.
Went with precision over recall (the project's stated floor): the new factory branch now declines the claim whenever any argument resolves to an IDisposable (!AnyDisposableArgument). So XmlReader.Create(fs) / JsonDocument.Parse(stream) are no longer treated as factories — fs stays tracked and its leak is reported — while the common no-disposable-arg overloads (XmlReader.Create("file.xml"), JsonDocument.Parse(jsonString)) still resolve and catch a dropped reader/document. Modelling the input as borrowed-non-escaping (your option b) would keep recall on those overloads too, but it's a deeper escape-pass change; I took the conservative narrowing for now. No dotnet locally, so CI (golden C# / leak extractor / corpus benchmark) verifies the build + specificity.
Generated by Claude Code
…pure factories XmlReader.Create / XmlWriter.Create / JsonDocument.Parse have overloads that take a caller-owned disposable input (a Stream / TextReader / TextWriter) which they do NOT dispose by default (XmlReaderSettings.CloseInput and XmlWriterSettings.CloseOutput default false; JsonDocument.Parse never closes its stream). Classifying those as pure owned factories let the extractor's escape pass drop the input argument from leak tracking, suppressing a real leak of the caller-owned stream — e.g. `var fs = File.OpenRead(p); var xr = XmlReader.Create(fs); xr.Dispose();` leaked `fs` invisibly. (Codex P2.) Gate the new factory branch on `!AnyDisposableArgument`: decline the factory claim whenever any argument resolves to an IDisposable. The common string / URI / path overloads have no disposable arg and still resolve (recall preserved there); precision over recall on the wrapping overloads — never suppress an input leak. C#-only (no dotnet locally): verified by CI (golden C#, C# leak extractor, corpus benchmark). Bridge table + tests unchanged — the table is consulted only when the extractor emits the factory fact, which now excludes the disposable-input overloads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
2 participants
Что и зачем
P1a stdlib contract pack (producer-сторона): расширяю распознавание owned-возвращающих фабрик тремя высокоценными, низко-FP семействами BCL, чьи результаты вызывающий обязан диспоузить —
XmlReader.Create,XmlWriter.Create(System.Xml) иJsonDocument.Parse(System.Text.Json, который пулит память и часто течёт при потере ссылки). Тот же producer-контракт, что уFile.Open*/cryptoCreate*:var doc = JsonDocument.Parse(json)с потерейdocтеперь даёт OWN001 на месте вызова фабрики, а use/double-dispose результата — OWN002/OWN003.Обе синхронизированные половины обновлены в лок-степе: экстрактор
IsOwningFactory(Program.cs) решает, эмитить ли факт вызова фабрики (по разрешённому символу: static + результат реализует IDisposable + тип/namespace, так что Task-возвращающийJsonDocument.ParseAsyncисключён); мост_BCL_FRESH_BY_NS(ownir.py) распознаёт имя callee какfresh.Тип изменения
Как проверено
python tests/run_tests.py— зелёный (ownir bridge 206/206, +5 новых P1a-проверок; ownership 44/44, остальные сюиты)ruff check .иmypy(--strict, 17 файлов) — чистоВАЖНО: в окружении разработки нет dotnet, поэтому C#-половина (экстрактор) не собиралась локально — она проверяется CI:
golden C# compiles & runs,C# leak extractor (Roslyn) → OwnIR → core, и real-C#corpus benchmark (recall + specificity). Bridge-половина покрыта новыми тестами полностью (leak → OWN001 для каждой фабрики, bare и namespace-qualified; диспоуз результата — чисто).Связанные issue
Нет issue для закрытия. Refs: дизайн-нота P-005 / P-006 (P1a — «recall ladder over a fixed precision floor», producer-половина контрактов).
Чеклист
tests/test_ownir.py: +5 P1a-кейсов на bridge-стороне)feat(p1a): …)🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
XmlReader.Create,XmlWriter.Create, andJsonDocument.Parseare now more reliably flagged when results aren’t disposed, while properreleasedisposal is treated as clean.release.