feat(di): DI003 — transient IDisposable captured by a singleton (new DI lifetime warning)#56
Conversation
…lifetime warning) A transient IDisposable captured by a singleton is resolved from the root (via the singleton), promoted to application lifetime, and disposed only at root disposal -- held far longer than its `transient` registration implies. A new DI lifetime diagnostic (warning), a class no general-purpose analyzer models -- the ASP.NET niche. Reuses the DI001 registration-graph DFS almost verbatim (ownlang/di.py find_captured_transient_disposables, target = transient AND disposable instead of scoped). Surfaced as a warning (severity="warning", a real verdict shown soft -- the framework allows it; the lifetime promotion is the smell), not advisory. - ownlang/di.py: Service gains `disposable`; new find_captured_transient_disposables. - ownlang/ownir.py: _di_findings parses `disposable` and emits DI003 (warning). - extractor (ExtractServices): emits `disposable` per service -- the impl's OWN base list names IDisposable/IAsyncDisposable (syntactic; an inherited disposable is not guessed, so DI003 fires only where ownership is certain). - DiCaptiveSample.cs: a DI003 case (singleton ConnectionWarmer captures transient IDisposable PooledConnection); ci.yml asserts it end-to-end (and that it does NOT inflate the DI001 captive count). Precision by construction: `disposable` is a NEW fact field (defaults false), so existing facts can't fire DI003 -- zero regression; DiCaptiveSample's existing transient (UnitOfWork) is not IDisposable, so the 4 DI001 captives are unchanged. Validated locally: di.py + bridge via check_facts; test_ownir 91/91. The extractor `disposable` emission + the sample are validated by the wpf-extractor CI job. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // transient IDisposable PooledConnection: promoted to application lifetime, | ||
| // disposed only at root disposal. NOT a DI001 (no scoped captured). | ||
| services.AddTransient<PooledConnection>(); | ||
| services.AddSingleton<ConnectionWarmer>(); |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughImplements the DI003 diagnostic end-to-end. The Roslyn extractor gains a ChangesDI003: Transient IDisposable Captured by Singleton
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ruff I001 (the new find_captured_transient_disposables import sat on its own line) and E501 (DI003 test Service rows). No behaviour change; suite + mypy --strict green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![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: 682e93779a
ℹ️ 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".
| ctorDisposable[cls.Identifier.Text] = cls.BaseList is { } bl | ||
| && bl.Types.Any(bt => DiTypeName(bt.Type) is "IDisposable" or "IAsyncDisposable"); |
There was a problem hiding this comment.
When scanning a directory containing partial service implementations, a later partial declaration without a base list overwrites an earlier true value here. For partial class Conn : IDisposable plus partial class Conn { ... } (common with generated/designer files), the transient registration is emitted with disposable=false, so a singleton depending on it no longer produces DI003. Aggregate this flag with any existing value instead of assigning the current declaration's base-list result.
Useful? React with 👍 / 👎.
| name=str(s.get("name", "?")), | ||
| lifetime=str(s.get("lifetime", "")), | ||
| deps=tuple(s.get("deps", [])), | ||
| disposable=bool(s.get("disposable", False)), |
There was a problem hiding this comment.
If OwnIR is produced by anything other than this extractor and serializes the new field as "false" (or any non-empty string), bool(...) turns it into True; load() currently doesn't reject that type, so the CLI emits a DI003 warning and exits with findings for a service explicitly marked non-disposable. Treat only the boolean value true as true and validate/reject non-boolean disposable values.
Useful? React with 👍 / 👎.
…rcion - Program.cs: aggregate the IDisposable flag across partial class declarations (any part naming IDisposable wins), so a later partial without a base list (a generated/designer file) can't clear an earlier `partial C : IDisposable`. - ownir.py: parse `disposable` as `is True` (only the JSON boolean true), so a stray string from a non-extractor producer can't coerce to disposable=True. Suite + ruff + mypy --strict green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/roslyn/OwnSharp.Extractor/Program.cs (1)
907-908: Fix is optional: partial class overwrite concern is valid but no partial classes exist in main source.The logic at lines 907-908 unconditionally overwrites
ctorDisposable[cls.Identifier.Text]per class declaration. For partial classes with mixedIDisposable/ non-disposable base lists, the last part would determine the value, potentially settingdisposable=falseincorrectly. However, verification found zero partial classes in the main source code—all partial classes are test corpus only.The suggested fix using
prev || declaresDisposableis defensively sound and recommended for future-proofing, but not urgent. The code comment "last decl wins" already acknowledges the overwrite behavior, though it doesn't account for partial class semantics.Suggested fix (optional)
- ctorDisposable[cls.Identifier.Text] = cls.BaseList is { } bl - && bl.Types.Any(bt => DiTypeName(bt.Type) is "IDisposable" or "IAsyncDisposable"); + var declaresDisposable = cls.BaseList is { } bl + && bl.Types.Any(bt => DiTypeName(bt.Type) is "IDisposable" or "IAsyncDisposable"); + ctorDisposable[cls.Identifier.Text] = + ctorDisposable.TryGetValue(cls.Identifier.Text, out var prev) + ? prev || declaresDisposable + : declaresDisposable;🤖 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 `@frontend/roslyn/OwnSharp.Extractor/Program.cs` around lines 907 - 908, In the ctorDisposable dictionary assignment at line 907-908, change the unconditional overwrite of ctorDisposable[cls.Identifier.Text] to use a preserving assignment pattern. Instead of directly assigning the result of the IDisposable check, use a logical OR operation to preserve true values across multiple declarations: change the assignment to account for both the existing value (if any) and the current class declaration's disposable status. This ensures that for partial classes, if any declaration implements IDisposable or IAsyncDisposable, the class remains marked as disposable rather than being overwritten to false by a later partial class declaration without those interfaces.
🤖 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.
Inline comments:
In `@ownlang/ownir.py`:
- Line 1236: The issue is in the line where `disposable=bool(s.get("disposable",
False))` uses truthiness coercion instead of strict boolean parsing. This causes
string values like "false" to evaluate to True (since non-empty strings are
truthy), leading to false-positive DI003 findings. Replace the bool() coercion
with proper boolean parsing that explicitly checks for boolean true values or
their string representations, defaulting to False when the field is absent or
has a falsy value. This ensures that string "false" or actual False values are
correctly parsed as False, not True.
---
Nitpick comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 907-908: In the ctorDisposable dictionary assignment at line
907-908, change the unconditional overwrite of
ctorDisposable[cls.Identifier.Text] to use a preserving assignment pattern.
Instead of directly assigning the result of the IDisposable check, use a logical
OR operation to preserve true values across multiple declarations: change the
assignment to account for both the existing value (if any) and the current class
declaration's disposable status. This ensures that for partial classes, if any
declaration implements IDisposable or IAsyncDisposable, the class remains marked
as disposable rather than being overwritten to false by a later partial class
declaration without those interfaces.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e83c1c2-226f-4708-b1e0-0eb942a82dbe
📒 Files selected for processing (9)
.github/workflows/ci.ymldocs/ROADMAP.mddocs/notes/di-captive-extractor.mddocs/proposals/P-006-di-lifetimes.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/DiCaptiveSample.csownlang/di.pyownlang/ownir.pytests/test_ownir.py
…ractor First wild hunt after the recall run. ShareX exercises the disposable / subscription / DI suite on unseen code beyond WPF -- a generalization + precision spot-check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
3 participants
What
A new DI lifetime diagnostic — DI003: a transient
IDisposablecaptured by a singleton. The container resolves the disposable once (from the root, via the singleton) and holds it for the whole app — disposed only at root disposal, far longer than itstransientregistration implies. A class no general-purpose analyzer models (the ASP.NET-lifetime niche, same complementary story as DI001).It reuses the DI001 registration-graph DFS almost verbatim — target = transient ∧ disposable instead of scoped — so it's mostly core-side (
ownlang/di.py), which means it's validated end-to-end locally, not blind-in-CI.Surfaced as a warning (
severity="warning"— a real verdict shown soft; the framework allows it, the lifetime promotion is the smell), not advisory.How
ownlang/di.py—Servicegains adisposableflag; newfind_captured_transient_disposables(the DI001 DFS, retargeted).ownlang/ownir.py—_di_findingsparsesdisposableand emits DI003 findings atSeverity"warning".ExtractServices) — emitsdisposableper service: the impl's own base list namesIDisposable/IAsyncDisposable(syntactic — an inherited disposable is not guessed, so DI003 fires only where ownership is certain).DiCaptiveSample.cs— a DI003 case (singletonConnectionWarmercaptures transientIDisposablePooledConnection);ci.ymlasserts it end-to-end and that it does not inflate the DI001 captive count.Precision, by construction
disposableis a new fact field (defaultsfalse), so existing facts can't fire DI003 — zero regression.DiCaptiveSample's existing transient (UnitOfWork) is notIDisposable, so the 4 DI001 captives are unchanged (the CI count check proves it). DI003 only fires when an impl is syntactically: IDisposable.Validation
di.py+ the bridge viacheck_facts(direct, transitive-through-transient, non-disposable-silent, scoped-not-flagged);test_ownir91/91 (added DI003 unit + bridge tests); full suite green (wpf4/4,lifetimes10/10,spec22/22).disposableemission + the sample are validated by thewpf-extractorjob (real C# → facts →[DI003] warningat the registration site).Files
ownlang/di.py,ownlang/ownir.py— the check + bridge.frontend/roslyn/OwnSharp.Extractor/Program.cs—disposableemission.frontend/roslyn/samples/DiCaptiveSample.cs,.github/workflows/ci.yml— the CI case + assertion.tests/test_ownir.py— DI003 unit + bridge pins.docs/— P-006, ROADMAP,di-captive-extractor.md(DI003 → shipped; next: DI002, the explicitroot.GetService<T>()form).This is a new diagnostic class, not a corpus row (DI is graph-based, not
.own-expressible), so the corpus recall number is unchanged by design.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
New Features
IDisposableservices captured by singleton services, including end-to-end CI validation.Documentation
Tests
CI