Skip to content

feat(di): DI002 transitive — a singleton weakly holding a transient that drags in a scoped#64

Merged
PhysShell merged 2 commits into
mainfrom
claude/di002-transitive
Jun 21, 2026
Merged

feat(di): DI002 transitive — a singleton weakly holding a transient that drags in a scoped#64
PhysShell merged 2 commits into
mainfrom
claude/di002-transitive

Conversation

@PhysShell

@PhysShell PhysShell commented Jun 21, 2026

Copy link
Copy Markdown
Owner

The transitive form of DI002

Extends DI002 (shipped direct in #63) to the transitive case: a singleton holding a WeakReference<Transient> whose transient strongly depends on a scoped service.

public sealed class UnitOfWork { public UnitOfWork(AppDbContext db) { } }   // transient -> scoped
public sealed class WeakReport { public WeakReport(WeakReference<UnitOfWork> uow) { } }  // singleton
services.AddTransient<UnitOfWork>();
services.AddSingleton<WeakReport>();   // DI002 — WeakReport -> UnitOfWork -> AppDbContext

The weak edge enters the transient, but the scoped it drags in is still root-resolved and app-lived — a captive one hop below the weak reference. The weak ref keeps the entry edge off the DI001 strong graph, but the captive scoped underneath is the smell → DI002 (warning), with the full path shown.

How

find_weak_captive_dependencies now runs the same strong-edge DFS DI001 does, rooted at each weak dep instead of the strong deps: a scoped reached directly (WeakReference<Scoped>) or through a weakly-held transient's strong deps is reported; transients are followed, singleton edges are another singleton's own pass, cycles guarded. Core-only — the extractor already emits weak_deps and the transient's strong deps.

Pinned in CI

DiCaptiveSample.cs adds WeakReport, so the wpf-extractor job now asserts exactly 3 DI002:

singleton weak path
WeakCache → AppDbContext direct (#63)
WeakCacheOpt → AppDbContext nullable direct (#63)
WeakReport → UnitOfWork → AppDbContext transitive (this PR)

WeakReport carries the path WeakReport -> UnitOfWork -> AppDbContext; the existing exactly-4-DI001 / exactly-1-DI003 counts and WeakClockHolder silence are unchanged. The unit test asserts the set + the transitive path.

Validated: ruff + mypy clean, ownir 98/98, the 3-DI002 bridge with the transitive path.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED


Generated by Claude Code

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved DI002 detection to surface scoped services that are captured via transitive chains from weakly-held singleton dependencies.
  • Documentation

    • Refreshed the DI002 guidance to clarify direct versus transitive weak-capture behavior and how dependency paths are determined.
  • Tests

    • Expanded DI002 test scenarios to include a transitive case and validate the reported dependency path output.
    • Updated CI self-test expectations to match the enhanced detection results.

…hat drags in a scoped

Extends DI002 (shipped direct in #63) to the transitive form: a singleton holding a
WeakReference<Transient> whose transient strongly depends on a scoped service. The scoped is
still root-resolved and app-lived (a captive), reached one hop below the weak edge. The weak
ref keeps the entry edge off the DI001 strong graph, but the captive scoped underneath is the
smell -> DI002 (warning) with the full path shown.

find_weak_captive_dependencies now runs the same strong-edge DFS DI001 does, rooted at each
weak dep instead of the strong deps: a scoped reached directly (WeakReference<Scoped>) or
through a weakly-held transient's strong deps is reported; transients are followed, singleton
edges are another singleton's own pass, cycles guarded. Core-only — the extractor already
emits weak_deps + the transient's strong deps.

Pinned by WeakReport (WeakReference<UnitOfWork> -> scoped AppDbContext) in DiCaptiveSample.cs:
the wpf-extractor step now asserts exactly 3 DI002 (WeakCache direct, WeakCacheOpt nullable,
WeakReport transitive), with WeakReport carrying the path WeakReport -> UnitOfWork ->
AppDbContext; the exactly-4-DI001 / exactly-1-DI003 counts and WeakClockHolder silence are
unchanged. Unit test asserts the set + the transitive path. Validated: ruff + mypy clean,
ownir 98/98, the 3-DI002 bridge with the transitive path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9a05b6fe-55af-4aeb-9144-d2a4a5ff3c68

📥 Commits

Reviewing files that changed from the base of the PR and between 261f501 and c61592d.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/ci.yml

📝 Walkthrough

Walkthrough

find_weak_captive_dependencies (DI002) is extended from a single-hop check into a stack-based DFS that follows weak edges into transient nodes and then traverses their strong dependencies to find scoped services. A new WeakReport sample class demonstrates the transitive path; unit tests, CI self-test counts, and documentation are updated accordingly.

Changes

DI002 Transitive Weak Captive Detection

Layer / File(s) Summary
Sample class and DFS algorithm
frontend/roslyn/samples/DiCaptiveSample.cs, ownlang/di.py
WeakReport is added as a singleton holding WeakReference<UnitOfWork> and registered in ConfigureServices. find_weak_captive_dependencies is rewritten from a one-hop weak_deps loop into a stack-based DFS: seeds from each weak dependency, reports reached scoped nodes, follows transient nodes' strong dependencies, and deduplicates via reported/visited sets.
Tests, CI assertions, and docs
tests/test_ownir.py, .github/workflows/ci.yml, docs/notes/di-captive-extractor.md
Unit tests add WeakReport (weak dep on Uow) and assert two weak captives including the transitive path tuple ("WeakReport", "Uow", "Db"). CI self-test adds the WeakReport→UnitOfWork→AppDbContext assertion and raises the expected DI002 count from 2 to 3. Documentation is updated to describe both direct and transitive weak-captive detection semantics.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • PhysShell/Own.NET#63: Introduced the initial DI002 find_weak_captive_dependencies implementation that this PR extends with transitive detection.

Poem

🐇 A singleton gripped a weak reference tight,
Through transients it wandered, still holding the light—
A scoped service hiding two hops away,
The DFS found it without going astray.
Now count goes to three, the graph tells no lies!
~ hops with delight ~

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and precisely summarizes the main change: extending DI002 to handle transitive cases where a singleton weakly holds a transient that strongly depends on a scoped service.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/di002-transitive

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 @.github/workflows/ci.yml:
- Around line 335-336: The current CI assertion on line 335 verifies that the
DI002 error exists for WeakReport capturing AppDbContext, but does not verify
the rendered transitive dependency chain path. Add a second dedicated grep
assertion immediately after the existing check that explicitly validates the
transitive chain string is present in the output, ensuring it contains the full
path rendering of WeakReport -> UnitOfWork -> AppDbContext to catch regressions
in path rendering logic.
🪄 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: ca4d7b7c-ee20-47b2-bb13-f802cd0e02f3

📥 Commits

Reviewing files that changed from the base of the PR and between 6ff5f95 and 261f501.

📒 Files selected for processing (5)
  • .github/workflows/ci.yml
  • docs/notes/di-captive-extractor.md
  • frontend/roslyn/samples/DiCaptiveSample.cs
  • ownlang/di.py
  • tests/test_ownir.py

Comment thread .github/workflows/ci.yml
The existing assertion verified the WeakReport DI002 finding exists but not the rendered
transitive chain. Add a grep for "WeakReport -> UnitOfWork -> AppDbContext" so a path-rendering
regression (the whole point of the transitive slice) fails CI.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
@PhysShell
PhysShell merged commit 9f7fd8f into main Jun 21, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants