From 261f5010d529715c7ca2108675bd158774e456d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 21 Jun 2026 08:44:58 +0000 Subject: [PATCH 1/2] =?UTF-8?q?feat(di):=20DI002=20transitive=20=E2=80=94?= =?UTF-8?q?=20a=20singleton=20weakly=20holding=20a=20transient=20that=20dr?= =?UTF-8?q?ags=20in=20a=20scoped?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends DI002 (shipped direct in #63) to the transitive form: a singleton holding a WeakReference 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) 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 -> 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) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 9 +++-- docs/notes/di-captive-extractor.md | 20 +++++------ frontend/roslyn/samples/DiCaptiveSample.cs | 8 +++++ ownlang/di.py | 42 ++++++++++++++-------- tests/test_ownir.py | 13 +++++-- 5 files changed, 63 insertions(+), 29 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bf930fec..c6775c68 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -329,9 +329,14 @@ jobs: # is unwrapped, so the scoped service is still seen (CodeRabbit review on #63). echo "$out" | grep -qE "\[DI002\].*'WeakCacheOpt' weakly captures scoped service 'AppDbContext'" \ || { echo "FAIL: expected DI002 on the nullable WeakReference (WeakCacheOpt)"; exit 1; } + # transitive DI002: a singleton weakly holds the transient UnitOfWork, which strongly + # drags in scoped AppDbContext (WeakReport -> UnitOfWork -> AppDbContext). The weak DFS + # follows the transient's strong edges like DI001 does. + echo "$out" | grep -qE "\[DI002\].*'WeakReport' weakly captures scoped service 'AppDbContext'" \ + || { echo "FAIL: expected transitive DI002 (WeakReport -> UnitOfWork -> AppDbContext)"; exit 1; } nwk=$(echo "$out" | grep -cE "DiCaptiveSample\.cs:[0-9]+:.*\[DI002\]") - [ "$nwk" = "2" ] \ - || { echo "FAIL: expected exactly 2 DI002 findings, got $nwk"; exit 1; } + [ "$nwk" = "3" ] \ + || { echo "FAIL: expected exactly 3 DI002 findings, got $nwk"; exit 1; } if echo "$out" | grep -q "WeakClockHolder"; then echo "FAIL: a weak ref to a singleton (WeakClockHolder) was wrongly flagged"; exit 1 fi diff --git a/docs/notes/di-captive-extractor.md b/docs/notes/di-captive-extractor.md index bf5c5e5b..5430353a 100644 --- a/docs/notes/di-captive-extractor.md +++ b/docs/notes/di-captive-extractor.md @@ -108,18 +108,18 @@ fix."* A **warning** (`severity="warning"` — real, shown soft), distinct from DI001 capture. The extractor reads a `WeakReference` 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. Pinned end-to-end by `DiCaptiveSample.cs` (`WeakCache` → -`WeakReference`, with `WeakClockHolder → WeakReference` staying silent — -a weak ref to a singleton is no mismatch) in the `wpf-extractor` CI job, and at the graph -level by `tests/test_ownir.py`. It is a contract no general-purpose analyzer models — even the -developer's WeakReference "fix" is still flagged, which is the key differentiation. +`ownlang/di.py` `find_weak_captive_dependencies` flags a singleton that *reaches* a scoped +service from a weak dep — directly (`WeakReference`) or **transitively** through a +weakly-held transient that strongly drags in the scoped, the same strong-edge DFS DI001 runs +but rooted at the weak edge. Pinned end-to-end by `DiCaptiveSample.cs` — `WeakCache` / +`WeakCacheOpt` (`WeakReference`, the second nullable), and `WeakReport` +(`WeakReference` → scoped `AppDbContext`, the transitive case); `WeakClockHolder → +WeakReference` stays silent (a weak ref to a singleton is no mismatch) — in the +`wpf-extractor` CI job, and at the graph level by `tests/test_ownir.py`. It is a contract no +general-purpose analyzer models — even the developer's WeakReference "fix" is still flagged, +which is the key differentiation. ## Next (separate slices) - -- **DI002, the transitive form** — a singleton holding a `WeakReference` whose - transient *drags in* a scoped service (the weak edge is one hop above the scoped); the - shipped slice flags the common **direct** `WeakReference` shape. - **DI003, the explicit form** — a transient `IDisposable` resolved by hand from the **root** provider (`root.GetService()`), which the graph form above does not see (it needs the resolution call sites, not just the registration graph). diff --git a/frontend/roslyn/samples/DiCaptiveSample.cs b/frontend/roslyn/samples/DiCaptiveSample.cs index bfed10a0..68164e60 100644 --- a/frontend/roslyn/samples/DiCaptiveSample.cs +++ b/frontend/roslyn/samples/DiCaptiveSample.cs @@ -58,6 +58,11 @@ public sealed class WeakCache { public WeakCache(WeakReference db) // a NULLABLE weak reference (`WeakReference?`) is the same weak captive — the // `?` annotation does not change the service type, so it is DI002 too (CodeRabbit review). public sealed class WeakCacheOpt { public WeakCacheOpt(WeakReference? db) { } } + // DI002 transitive — a singleton weakly holds a TRANSIENT (UnitOfWork) that strongly + // depends on a scoped service (AppDbContext). The weak edge enters the transient, but the + // scoped it drags in is still root-resolved and app-lived: DI002 (path WeakReport -> + // UnitOfWork -> AppDbContext). NOT a DI001 (the entry edge is weak). + public sealed class WeakReport { public WeakReport(WeakReference uow) { } } // control: a weak reference to a SINGLETON is no lifetime mismatch -> SILENT. public sealed class WeakClockHolder { public WeakClockHolder(WeakReference clock) { } } @@ -104,6 +109,9 @@ public static void ConfigureServices(IServiceCollection services) // FLAGGED (DI002) — a NULLABLE WeakReference? is the same weak captive // (the `?` annotation is unwrapped, so the scoped service is still seen). services.AddSingleton(); + // FLAGGED (DI002, transitive) — singleton weakly holds the transient UnitOfWork, + // which strongly drags in scoped AppDbContext (WeakReport -> UnitOfWork -> AppDbContext). + services.AddSingleton(); // SILENT — a weak reference to the SINGLETON Clock is no lifetime mismatch. services.AddSingleton(); } diff --git a/ownlang/di.py b/ownlang/di.py index 8d72c9d5..67a27c24 100644 --- a/ownlang/di.py +++ b/ownlang/di.py @@ -140,27 +140,41 @@ def message(self) -> str: def find_weak_captive_dependencies( services: list[Service]) -> list[WeakCaptiveDependency]: - """Return every scoped service a singleton holds via `WeakReference` (DI002). - The direct form: a singleton whose `weak_deps` names a scoped service. The weak - reference keeps it off the DI001 strong-capture graph, but the scoped instance is - still root-resolved and app-lived — a lifetime-contract violation, surfaced as a - warning. (Weakly-held transients that *drag in* a scoped are a separate, rarer - slice — not followed here, the direct weak-scoped edge is the common 'I wrapped my - captive in WeakReference' shape.)""" + """Return every scoped service a singleton reaches via `WeakReference` (DI002). + From each weak dependency, walk the STRONG dependency chain exactly as DI001 does: the + weak edge enters a service the singleton holds weakly, and a scoped service it reaches — + directly (`WeakReference`) or transitively through a weakly-held transient that + strongly depends on it — is still root-resolved and app-lived, a lifetime-contract + violation surfaced as a warning. Transients are followed (a transient resolved through + the singleton drags its scoped dep along); a singleton edge is another singleton's own + pass. Cycles are guarded. The weak entry edge keeps it off the DI001 strong graph.""" by_name = {s.name: s for s in services} findings: list[WeakCaptiveDependency] = [] for s in services: if s.lifetime != SINGLETON: continue reported: set[str] = set() - for dep in s.weak_deps: - dnode = by_name.get(dep) - if dnode is None or dnode.lifetime != SCOPED or dep in reported: + visited: set[str] = set() + # DFS rooted at the WEAK deps, then following STRONG transient edges (DI001-style). + stack: list[tuple[str, tuple[str, ...]]] = [ + (dep, (s.name, dep)) for dep in s.weak_deps] + while stack: + cur, path = stack.pop() + cnode = by_name.get(cur) + if cnode is None: continue - reported.add(dep) - findings.append(WeakCaptiveDependency( - singleton=s.name, captured=dep, path=(s.name, dep), - file=s.file, line=s.line)) + if cnode.lifetime == SCOPED: + if cur not in reported: + reported.add(cur) + findings.append(WeakCaptiveDependency( + singleton=s.name, captured=cur, path=path, + file=s.file, line=s.line)) + continue # the violating scoped edge is found; don't recurse past it + if cnode.lifetime == TRANSIENT and cur not in visited: + visited.add(cur) + for d in cnode.deps: # follow the transient's STRONG deps + stack.append((d, (*path, d))) + # a singleton edge is safe here (the inner singleton is reported on its own pass) findings.sort(key=lambda f: (f.file, f.line, f.singleton, f.captured)) return findings diff --git a/tests/test_ownir.py b/tests/test_ownir.py index 8c1a2fca..9021035d 100644 --- a/tests/test_ownir.py +++ b/tests/test_ownir.py @@ -530,18 +530,25 @@ def _sub(source: str | None) -> list[Finding]: # graph; a weak ref to a singleton is no mismatch, so it stays silent. from ownlang.di import find_weak_captive_dependencies wsvcs = [ - Service("WeakCache", "singleton", deps=(), weak_deps=("Db",)), # weak -> scoped: DI002 + Service("WeakCache", "singleton", deps=(), weak_deps=("Db",)), # weak->scoped: DI002 Service("Db", "scoped", ()), - Service("Strong", "singleton", deps=("Db",)), # strong -> scoped: DI001 + Service("Strong", "singleton", deps=("Db",)), # strong->scoped: DI001 + Service("WeakReport", "singleton", deps=(), weak_deps=("Uow",)), # weak->transient->scoped + Service("Uow", "transient", deps=("Db",)), Service("WeakClock", "singleton", deps=(), weak_deps=("Clk",)), # weak -> singleton: safe Service("Clk", "singleton", ()), ] di2 = find_weak_captive_dependencies(wsvcs) checks += 1 got2 = sorted((c.singleton, c.captured) for c in di2) - if got2 != [("WeakCache", "Db")]: + if got2 != [("WeakCache", "Db"), ("WeakReport", "Db")]: fails.append(f"DI002 set wrong: {got2}") checks += 1 + # the transitive weak captive carries the full path through the weakly-held transient. + wpath = next((c.path for c in di2 if c.singleton == "WeakReport"), None) + if wpath != ("WeakReport", "Uow", "Db"): + fails.append(f"DI002 transitive path wrong: {wpath}") + checks += 1 # the weak captive must NOT also be a strong DI001 (weak edge is off the strong graph). if any(c.singleton == "WeakCache" for c in find_captive_dependencies(wsvcs)): fails.append("DI002 weak captive wrongly also flagged as DI001") From c61592d26dfa6e868811aee2a01a9a27ca04fb3e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 21 Jun 2026 08:49:40 +0000 Subject: [PATCH 2/2] test(ci): pin the rendered transitive DI002 path string (CodeRabbit #64) 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) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c6775c68..d60550de 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -334,6 +334,10 @@ jobs: # follows the transient's strong edges like DI001 does. echo "$out" | grep -qE "\[DI002\].*'WeakReport' weakly captures scoped service 'AppDbContext'" \ || { echo "FAIL: expected transitive DI002 (WeakReport -> UnitOfWork -> AppDbContext)"; exit 1; } + # pin the rendered transitive PATH (not just the finding), so a path-rendering + # regression fails CI (CodeRabbit review on #64). + echo "$out" | grep -q "WeakReport -> UnitOfWork -> AppDbContext" \ + || { echo "FAIL: expected the transitive DI002 path text"; exit 1; } nwk=$(echo "$out" | grep -cE "DiCaptiveSample\.cs:[0-9]+:.*\[DI002\]") [ "$nwk" = "3" ] \ || { echo "FAIL: expected exactly 3 DI002 findings, got $nwk"; exit 1; }