feat(di): DI002 — scoped service captured weakly by a singleton (WeakReference is not a fix)#63
Merged
Merged
Conversation
…Reference is not a fix) A new DI lifetime diagnostic, squarely in the differentiation zone (no general-purpose analyzer models DI container lifetime contracts): a singleton that holds a SCOPED service via WeakReference<T>. The weak reference is the usual "fix" for a DI001 captive — it stops the singleton pinning the scoped instance for the GC. But it does NOT fix the lifetime contract: the scoped service is still resolved from the root provider and lives for the application lifetime; the weak ref only hides the GC-retention symptom (and may go dead under the consumer). "Your fix isn't a fix." A warning, distinct from the strong DI001 capture. The extractor reads a `WeakReference<X>` constructor parameter (WeakRefInner) into a separate `weak_deps` list, deliberately kept OFF the DI001 strong graph, so the same scoped service is either a strong captive (DI001) or a weak captive (DI002), never both. ownlang/di.py `find_weak_captive_dependencies` flags a singleton whose `weak_deps` names a scoped service (the common direct WeakReference<Scoped> shape; the transitive weak-through-transient form is a noted future slice). Surfaced as severity="warning" (real verdict, shown soft). Pinned end-to-end by DiCaptiveSample.cs (WeakCache -> WeakReference<AppDbContext> flagged; WeakClockHolder -> WeakReference<Clock> silent, a weak ref to a singleton is no mismatch) in the wpf-extractor CI job (exactly 1 DI002, the existing exactly-4-DI001 / exactly-1-DI003 counts unchanged — the weak edge is off the strong graph), and at the graph level by tests/test_ownir.py. Validated locally: ruff + mypy clean, ownir 96/96, the full DI002 bridge (WeakCache DI002 warning, no double-flag as DI001, WeakClockHolder silent). 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}
|
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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds DI002 detection for singletons that capture scoped services via ChangesDI002 WeakReference captive-scope detection
Sequence Diagram(s)sequenceDiagram
participant Extractor as C# Extractor
participant Bridge as OwnIR Bridge
participant Core as di.py Core
participant Finding as Finding Report
Extractor->>Extractor: Parse WeakReference constructor params
Extractor->>Bridge: Emit service with weak_deps
Bridge->>Core: find_weak_captive_dependencies(services)
Core->>Core: Filter singletons capturing scoped services
Core-->>Bridge: WeakCaptiveDependency list
Bridge->>Finding: Emit DI002 Warning findings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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: 5e4a6455de
ℹ️ 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".
![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)
docs/notes/di-captive-extractor.md (1)
116-116: 💤 Low valueOptional: Consider rewording to reduce reliance on "exactly".
Line 116 uses "exactly" to emphasize the differentiation point. While technically precise, a simpler phrasing might improve readability:
- Current:
"...which is exactly the differentiation."- Alternative:
"...which is the key differentiation."This is a minor style suggestion from static analysis; the current phrasing is defensible in a technical context.
🤖 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 `@docs/notes/di-captive-extractor.md` at line 116, In the file docs/notes/di-captive-extractor.md, locate the phrase "which is exactly the differentiation" and reword it to improve readability. Replace "exactly" with a simpler alternative such as "key" to make the sentence flow better while maintaining the technical meaning. The revised phrase should read "which is the key differentiation" to reduce reliance on the word "exactly" while preserving the emphasis on the differentiation point.Source: Linters/SAST tools
🤖 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 `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 1082-1099: The WeakRefInner function doesn't handle nullable type
wrappers, causing it to miss detection of WeakReference<X>? and
WeakReference<X?> patterns. Fix this by first unwrapping any NullableTypeSyntax
wrapper from the input parameter t before applying the existing pattern matching
against GenericNameSyntax, QualifiedNameSyntax, and AliasQualifiedNameSyntax.
Additionally, when extracting the type argument from the generic type using
g.TypeArgumentList.Arguments[0], unwrap any NullableTypeSyntax wrapper from that
argument before passing it to DiTypeName to ensure nullable type arguments are
also handled correctly.
---
Nitpick comments:
In `@docs/notes/di-captive-extractor.md`:
- Line 116: In the file docs/notes/di-captive-extractor.md, locate the phrase
"which is exactly the differentiation" and reword it to improve readability.
Replace "exactly" with a simpler alternative such as "key" to make the sentence
flow better while maintaining the technical meaning. The revised phrase should
read "which is the key differentiation" to reduce reliance on the word "exactly"
while preserving the emphasis on the differentiation point.
🪄 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: 5dcc9990-b4a3-4fa6-8f7e-22d324971d05
📒 Files selected for processing (8)
.github/workflows/ci.ymldocs/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
…odex #63) Two Codex P2s on the DI002 PR: 1. Service.weak_deps was added BEFORE `disposable`, shifting the dataclass's positional constructor: `Service("Conn", "transient", (), True)` would set weak_deps=True instead of disposable=True (silently dropping a DI003). Current callers all pass `disposable=` by keyword so nothing broke, but it is a latent footgun — moved weak_deps to LAST so the positional contract (name, lifetime, deps, disposable, file, line) is preserved. 2. load() validated `deps` (array of strings) but not the new `weak_deps`, so a malformed `weak_deps` (null / scalar / dict) passed the schema and then raised a RAW TypeError in `python -m ownlang ownir` (or, for a string, was silently char-split by tuple()). Added the same array-of-strings check for weak_deps, so it now fails loudly with a controlled OwnIRError exactly like deps. Pinned by a load-validation test (weak_deps: "abc"). Validated: ruff + mypy clean, ownir 97/97, Service("Conn","transient",(),True) -> disposable preserved, and `python -m ownlang ownir` rejects null/scalar/dict/string weak_deps with OwnIRError (parity with deps). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
…eRabbit #63) CodeRabbit: WeakRefInner ignored nullable annotations, so a `WeakReference<X>?` ctor param (and the same gap pre-existed in DiTypeName for a nullable direct dep `X? db`) was silently dropped from BOTH the strong DI001 graph and the weak DI002 graph. A `?` annotation does not change the injected service type, so the dep must still be seen. Two small, general fixes: DiTypeName now unwraps NullableTypeSyntax (`X?` -> X), which covers a nullable direct dep (DI001) and a nullable type argument (`WeakReference<X?>`); and WeakRefInner unwraps an outer nullable wrapper (`WeakReference<X>?`) before its pattern match. Pinned by a new WeakCacheOpt sample (`WeakReference<AppDbContext>?` -> DI002), so the wpf-extractor step now asserts exactly 2 DI002 (WeakCache + WeakCacheOpt); the exactly-4-DI001 / exactly-1-DI003 counts and WeakClockHolder silence are unchanged. Also reworded a docs line ("exactly" -> "key", CodeRabbit nit). Validated: braces balanced, ownir 97/97, the 2-DI002 bridge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
| // the weak ref hides the GC-pinning symptom, but scoped AppDbContext is still | ||
| // root-resolved and app-lived (the captive lifetime violation remains). NOT a | ||
| // DI001 (the weak edge is off the strong graph). | ||
| services.AddSingleton<WeakCache>(); |
| services.AddSingleton<WeakCache>(); | ||
| // FLAGGED (DI002) — a NULLABLE WeakReference<AppDbContext>? is the same weak captive | ||
| // (the `?` annotation is unwrapped, so the scoped service is still seen). | ||
| services.AddSingleton<WeakCacheOpt>(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The unique theme: DI lifetime contracts
A new DI lifetime diagnostic, squarely in the differentiation zone — no general-purpose analyzer (CodeQL, Infer#) models DI container lifetime contracts, so this is a class only Own.NET catches.
DI002 — a singleton that holds a scoped service via
WeakReference<T>. The weak reference is the usual "fix" for a DI001 captive (it stops the singleton pinning the scoped instance for the GC). But it does not fix the lifetime contract: the scoped service is still resolved from the root provider and lives for the application lifetime — the weak ref only hides the GC-retention symptom (and may go dead under the consumer). "Your fix isn't a fix." A warning, distinct from the strong DI001 capture.How it's built
WeakReference<X>ctor parameter (WeakRefInner) is read into a separateweak_depslist, deliberately kept off the DI001 strong graph — so the same scoped service is either a strong captive (DI001) or a weak captive (DI002), never both.ownlang/di.py):find_weak_captive_dependenciesflags a singleton whoseweak_depsnames a scoped service (the common directWeakReference<Scoped>shape; the transitive weak-through-transient form is a noted future slice).ownir.py): parsesweak_deps, emits DI002 atseverity="warning".Pinned in CI
DiCaptiveSample.cs, asserted in thewpf-extractorjob:WeakCache → WeakReference<AppDbContext>(scoped)WeakClockHolder → WeakReference<Clock>(singleton)Exactly 1 DI002; the existing exactly-4-DI001 and exactly-1-DI003 counts are unchanged (the weak edge is off the strong graph, so
WeakCacheis not a 5th DI001).Validation
Local: ruff + mypy clean, ownir 96/96 (new DI002 unit + bridge tests), and the full DI002 bridge —
WeakCache→ DI002 warning, not double-flagged as DI001,WeakClockHoldersilent. The extractor source→facts step is validated in CI.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
Release Notes
New Features
WeakReference<T>, including nullable and the transitive variant.Documentation
Tests
weak_deps.CI
Samples