feat(p1a): recognise ADO.NET owned-returning members (ExecuteReader/CreateCommand/BeginTransaction)#154
Conversation
…reateCommand/BeginTransaction) P1a ADO.NET tranche — the single most common real-world .NET resource leak: a DbDataReader from `cmd.ExecuteReader()` (or a DbCommand from `CreateCommand()`, a DbTransaction from `BeginTransaction()`) dropped without Dispose holds the reader / command / server-side cursor open until finalization. These are INSTANCE methods on a connection/command, unlike the existing static File/crypto/Xml/Json factories — but the acquire path (op="acquire") is static-vs-instance agnostic and the receiver is not an argument, so nothing is dropped (spike-confirmed). Recognise them in `IsOwningFactory` by method name + the RESOLVED return type implementing the System.Data contract interface (IDataReader / IDbCommand / IDbTransaction), which is provider-agnostic: it covers every concrete provider (SqlDataReader, NpgsqlDataReader, …), the abstract base (DbDataReader/DbCommand/DbTransaction), and the interface itself. Guarded by !AnyDisposableArgument (args are CommandBehavior/IsolationLevel enums anyway). Corpus fixture `ado-executereader-leak`: before.cs leaks the reader (OWN001), after.cs disposes it with `using` (clean), case.own reduces the pattern. Locally verified: case.own → OWN001 (test_corpus 21/21), full suite green, mypy --strict, ruff. The C# extractor branch and the before.cs/after.cs end-to-end run are verified by CI (no dotnet locally): golden C# compiles, C# leak extractor, and the corpus benchmark (before caught + after clean, no specificity regression). 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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR extends extractor ownership detection for ADO.NET factory methods and adds a corpus case for ChangesADO.NET reader ownership leak
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://cdn.github.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: 963af37d17
ℹ️ 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 == "ExecuteReader" && ImplementsSystemDataInterface(sym.ReturnType, "IDataReader")) | ||
| || (sym.Name == "CreateCommand" && ImplementsSystemDataInterface(sym.ReturnType, "IDbCommand")) | ||
| || (sym.Name == "BeginTransaction" && ImplementsSystemDataInterface(sym.ReturnType, "IDbTransaction")))) |
There was a problem hiding this comment.
For any non-ADO wrapper/helper that happens to expose ExecuteReader, CreateCommand, or BeginTransaction and returns a cached or otherwise borrowed IDataReader/IDbCommand/IDbTransaction, this now mints a fresh acquire solely from the method name and return interface. That produces false OWN001 reports when callers correctly do not dispose a borrowed object, unlike the existing factory branches that also verify the declaring BCL type/namespace; this should check the receiver/containing type is the relevant ADO.NET command/connection/factory before assigning ownership.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 4ce410a. You're right that matching on method name + return interface alone would mint a false acquire for a non-ADO helper exposing an ExecuteReader that hands back a borrowed/cached reader. Now both ends are pinned, like the other factory branches verify their declaring type: the receiver (containing type) must implement the matching System.Data interface too — IDbCommand for ExecuteReader, IDbConnection for CreateCommand/BeginTransaction — in addition to the return-type interface. Still provider-agnostic (DbCommand / SqlCommand / NpgsqlCommand all implement IDbCommand). The corpus fixture is unaffected (DbCommand implements IDbCommand → case.own still OWN001, corpus 21/21). C# verified by CI.
Generated by Claude Code
…return type The ADO branch matched on method name + the RETURN type's System.Data interface only, so a non-ADO helper exposing an `ExecuteReader`/`CreateCommand`/ `BeginTransaction` that returns a borrowed/cached IDataReader/IDbCommand/ IDbTransaction would be minted as an owned acquire -> a false OWN001 when the caller correctly does not dispose the borrowed object. (Codex P2.) Pin BOTH ends, like every other factory branch verifies its declaring type: the RECEIVER (containing type) must implement the matching System.Data interface too -- IDbCommand for ExecuteReader, IDbConnection for CreateCommand/BeginTransaction -- in addition to the return-type interface. Still provider-agnostic (DbCommand / SqlCommand / NpgsqlCommand all implement IDbCommand). The corpus fixture is unaffected (DbCommand implements IDbCommand): case.own -> OWN001 still holds (corpus 21/21), full suite green, mypy --strict, ruff. C# branch verified by CI (golden C# / leak extractor / corpus benchmark). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
2 participants
Что и зачем
P1a, ADO.NET-транш — самая частая реальная утечка ресурсов в .NET:
DbDataReaderизcmd.ExecuteReader()(илиDbCommandизCreateCommand(),DbTransactionизBeginTransaction()), брошенный безDispose, держит reader / команду / серверный курсор открытыми до финализации.Это instance-методы на connection/command (в отличие от существующих static-фабрик File/crypto/Xml/Json) — но acquire-путь экстрактора (
op="acquire") безразличен к static-vs-instance, а receiver не является аргументом, поэтому ничего не дропается (подтверждено spike'ом). Распознаём вIsOwningFactoryпо имени метода + разрешённому типу возврата, реализующему контрактный интерфейс System.Data (IDataReader/IDbCommand/IDbTransaction) — это provider-agnostic: покрывает любой конкретный провайдер (SqlDataReader,NpgsqlDataReader, …), абстрактную базу и сам интерфейс. Под guard'ом!AnyDisposableArgument.Тип изменения
Как проверено
python tests/run_tests.py— зелёный; новый corpus-кейсado-executereader-leakчерезcase.ownдаёт OWN001 (corpus 21/21), bridge 208/208, остальные сюитыruff check .иmypy(--strict) — чистоВАЖНО: нет dotnet локально → C#-половина (ветка
IsOwningFactory) и прогонbefore.cs/after.csчерез реальный экстрактор проверяются CI:golden C# compiles & runs,C# leak extractor (Roslyn), иcorpus benchmark(before пойман как OWN001, after чист, без регресса specificity). Локально верифицирована pure-Python половина (case.own→ OWN001).Связанные issue
Нет issue для закрытия. Refs: P-005 / P-006 P1a (recall ladder); продолжает серию owned-factory распознавания (#153 — Xml/Json).
Чеклист
corpus/real-world/ado-executereader-leak/: before/after/case.own/expected/notes)IsOwningFactory+ notes.md фикстуры)feat(p1a): …)🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
ExecuteReader()results, along with the contract-matching rationale.