diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 11976281..bf930fec 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -317,7 +317,25 @@ jobs: nw=$(echo "$out" | grep -cE "DiCaptiveSample\.cs:[0-9]+:.*\[DI003\]") [ "$nw" = "1" ] \ || { echo "FAIL: expected exactly 1 DI003 finding, got $nw"; exit 1; } - echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) + DI001 (captive dependency) + DI003 (transient IDisposable captured by a singleton) at the C# location" + # P-006 DI002 (scoped service held by a singleton via WeakReference, WARNING): + # the weak ref is the usual "fix" for a DI001 captive, but scoped AppDbContext is + # still root-resolved and app-lived — the lifetime contract is still violated. The + # weak edge is OFF the strong graph, so WeakCache is a DI002, NOT a 5th DI001 (the + # "exactly 4 DI001" count above proves it). A weak ref to a SINGLETON + # (WeakClockHolder -> Clock) is no mismatch -> silent. + echo "$out" | grep -qE "\[DI002\].*'WeakCache' weakly captures scoped service 'AppDbContext'" \ + || { echo "FAIL: expected DI002 (WeakCache weakly captures scoped AppDbContext)"; exit 1; } + # a NULLABLE WeakReference? is the same weak captive — the `?` annotation + # 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; } + nwk=$(echo "$out" | grep -cE "DiCaptiveSample\.cs:[0-9]+:.*\[DI002\]") + [ "$nwk" = "2" ] \ + || { echo "FAIL: expected exactly 2 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 + echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) + DI001 (captive dependency) + DI002 (scoped captured weakly) + DI003 (transient IDisposable captured by a singleton) at the C# location" - name: Flow-sensitive local IDisposables (--flow-locals, P-016 B0b/B2) run: | # Path-sensitive flow analysis of local IDisposables — bugs the flat D1 diff --git a/docs/notes/di-captive-extractor.md b/docs/notes/di-captive-extractor.md index 0f9fbff1..bf5c5e5b 100644 --- a/docs/notes/di-captive-extractor.md +++ b/docs/notes/di-captive-extractor.md @@ -97,11 +97,29 @@ warning fires only where ownership is certain). Pinned end-to-end by `DiCaptiveS (`ConnectionWarmer` → transient `PooledConnection`) in the `wpf-extractor` CI job, and at the graph level by `tests/test_ownir.py`. +## DI002 — scoped service captured *weakly* by a singleton (shipped) + +A singleton that holds a **scoped** service via **`WeakReference`** is the usual "fix" +for a DI001 captive — the weak reference 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 reference only hides +the GC-retention symptom (and the target may go dead under the consumer). *"Your fix isn't a +fix."* A **warning** (`severity="warning"` — real, shown soft), distinct from the strong +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. + ## Next (separate slices) -- **DI002** — a singleton capturing a scoped dependency *weakly* - (`WeakReference`): a warning, since a weak reference fixes retention but - not the lifetime-contract violation. +- **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/docs/proposals/P-006-di-lifetimes.md b/docs/proposals/P-006-di-lifetimes.md index cb88737f..ace9520e 100644 --- a/docs/proposals/P-006-di-lifetimes.md +++ b/docs/proposals/P-006-di-lifetimes.md @@ -11,8 +11,9 @@ singleton→singleton and clean registrations silent). See [docs/notes/di-captive-extractor.md](../notes/di-captive-extractor.md). **DI003** (a transient `IDisposable` captured by a singleton — promoted to application lifetime) - now also fires end-to-end as a **warning**, CI-validated on the same sample. Next: - DI002 (weak-ref). + and **DI002** (a scoped service held by a singleton via `WeakReference` — still a + captive: the weak ref hides the GC symptom, not the lifetime violation) now also fire + end-to-end as **warnings**, CI-validated on the same sample. - **Depends on:** `spec/Lifetimes.md` (the region-ordering model behind OWN014), [P-001](P-001-csharp-extractor.md) (the C# seam). See [`docs/ROADMAP.md`](../ROADMAP.md) (Milestone 3). @@ -39,12 +40,13 @@ to a longer-lived region) already models it. - **DI001 (error):** a singleton service captures a scoped dependency (directly, or transitively through the constructor graph). -- **DI002 (warning):** a singleton captures a scoped dependency **weakly** +- **DI002 (warning) — shipped:** a singleton captures a scoped dependency **weakly** (`WeakReference`). A weak reference fixes *retention* leaks, not a - *lifetime contract* violation — the scoped service is still invalid outside its - scope and may be disposed mid-use. Message: *"`WeakReference` does not make a - scoped service safe to use outside its scope; resolve it inside a fresh scope - via `IServiceScopeFactory`, or make the consumer scoped."* + *lifetime contract* violation — the scoped service is still root-resolved, lives + for the app lifetime, and may be disposed mid-use. The `WeakReference` ctor + parameter is read into a separate `weak_deps` list (off the DI001 strong graph), and + `find_weak_captive_dependencies` flags a singleton whose `weak_deps` names a scoped + service. CI-validated on `DiCaptiveSample.cs` (`WeakCache`). - **DI003 (warning) — shipped:** 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 diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 2a42333c..7a75cf29 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -937,6 +937,10 @@ static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) { // 1. class name -> its widest constructor's parameter type names (the DI ctor). var ctorDeps = new Dictionary>(); + // class name -> services injected via `WeakReference` (held weakly). Kept apart + // from ctorDeps so the strong DI001 graph never sees a weak edge; a weakly-held + // scoped service is DI002 (the weak ref hides the GC symptom, not the lifetime bug). + var ctorWeakDeps = new Dictionary>(); // class name -> does it implement IDisposable/IAsyncDisposable (so the container // owns its disposal)? Syntactic — its OWN base list names it; an inherited // disposable (`: Stream`) is not seen, so DI003 fires only on an explicitly @@ -962,14 +966,21 @@ static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) || ctor.ParameterList.Parameters.Count > widest.Parameters.Count)) widest = ctor.ParameterList; var deps = new List(); + var weakDeps = new List(); if (widest is not null) foreach (var p in widest.Parameters) { - var tn = p.Type is null ? null : DiTypeName(p.Type); - if (tn is not null) + if (p.Type is null) + continue; + // a `WeakReference` parameter is a WEAK dep on X (not a strong dep): + // it keeps X off the DI001 graph, but a weakly-held scoped X is DI002. + if (WeakRefInner(p.Type) is { } weakInner) + weakDeps.Add(weakInner); + else if (DiTypeName(p.Type) is { } tn) deps.Add(tn); } - ctorDeps[cls.Identifier.Text] = deps; // last decl wins (core dedups by name) + ctorDeps[cls.Identifier.Text] = deps; // last decl wins (core dedups by name) + ctorWeakDeps[cls.Identifier.Text] = weakDeps; // OR across partial declarations: any part that names IDisposable makes the // type disposable, so a later `partial class C { }` (no base list, e.g. a // generated/designer file) cannot clear an earlier `partial class C : IDisposable`. @@ -994,11 +1005,14 @@ static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) continue; var deps = impl is not null && ctorDeps.TryGetValue(impl, out var d) ? d : new List(); + var weakDeps = impl is not null && ctorWeakDeps.TryGetValue(impl, out var wd) + ? wd : new List(); services.Add(new { name = service, lifetime, deps, + weak_deps = weakDeps, // the IMPLEMENTATION's disposability — the container constructs and // disposes the impl, so a transient-disposable impl captured by a // singleton is held to app exit (DI003). @@ -1062,9 +1076,33 @@ static void ResolveRegistration(SimpleNameSyntax name, ArgumentListSyntax args, GenericNameSyntax g => g.Identifier.Text, QualifiedNameSyntax q => DiTypeName(q.Right), AliasQualifiedNameSyntax aq => DiTypeName(aq.Name), + // a nullable annotation (`AppDbContext?`) does not change the injected service type — + // unwrap it so a nullable ctor param is still a real dep (CodeRabbit review on #63). + NullableTypeSyntax n => DiTypeName(n.ElementType), _ => null, }; +// If `t` is a `WeakReference` (or `System.WeakReference`, or a nullable +// `WeakReference?`), the simple name of its single type argument X; else null. +// Syntactic, single-arg — matches how a singleton holds a captive dependency weakly. +// (`System.WeakReference` non-generic has no element type and is not a DI dep.) +static string? WeakRefInner(TypeSyntax t) +{ + if (t is NullableTypeSyntax nt) // `WeakReference?` -> unwrap the nullable annotation + t = nt.ElementType; + var g = t switch + { + GenericNameSyntax gen => gen, + QualifiedNameSyntax { Right: GenericNameSyntax gen } => gen, + AliasQualifiedNameSyntax { Name: GenericNameSyntax gen } => gen, + _ => null, + }; + // the inner `X` (or a nullable `X?`) is resolved by DiTypeName, which unwraps `?`. + return g is { Identifier.Text: "WeakReference" } + && g.TypeArgumentList.Arguments.Count == 1 + ? DiTypeName(g.TypeArgumentList.Arguments[0]) : null; +} + // DI's default IServiceProvider resolves through PUBLIC constructors only — an // explicit ctor with no access modifier defaults to private and DI never uses it. static bool IsPublicCtor(SyntaxTokenList modifiers) diff --git a/frontend/roslyn/samples/DiCaptiveSample.cs b/frontend/roslyn/samples/DiCaptiveSample.cs index 5bd34fba..bfed10a0 100644 --- a/frontend/roslyn/samples/DiCaptiveSample.cs +++ b/frontend/roslyn/samples/DiCaptiveSample.cs @@ -49,6 +49,18 @@ private PublicCtorOnly(AppDbContext db) { } public sealed class PooledConnection : System.IDisposable { public void Dispose() { } } // transient, IDisposable public sealed class ConnectionWarmer { public ConnectionWarmer(PooledConnection c) { } } // singleton -> captures it + // DI002 — a singleton that holds a SCOPED service via WeakReference. A weak ref is + // the usual "fix" for a DI001 captive (it stops pinning the scoped instance for the + // GC), but the scoped service is still resolved from the root and lives for the app + // lifetime — the lifetime contract is still violated. A warning. The weak ref keeps it + // OFF the DI001 strong graph (`deps`), so it surfaces as DI002, not DI001. + public sealed class WeakCache { public WeakCache(WeakReference db) { } } // -> WeakReference : DI002 + // 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) { } } + // control: a weak reference to a SINGLETON is no lifetime mismatch -> SILENT. + public sealed class WeakClockHolder { public WeakClockHolder(WeakReference clock) { } } + public static class Startup { public static void ConfigureServices(IServiceCollection services) @@ -83,6 +95,17 @@ public static void ConfigureServices(IServiceCollection services) // disposed only at root disposal. NOT a DI001 (no scoped captured). services.AddTransient(); services.AddSingleton(); + + // FLAGGED (DI002, warning) — singleton holds a SCOPED service via WeakReference: + // 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(); + // FLAGGED (DI002) — a NULLABLE WeakReference? is the same weak captive + // (the `?` annotation is unwrapped, so the scoped service is still seen). + 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 25f78ab6..8d72c9d5 100644 --- a/ownlang/di.py +++ b/ownlang/di.py @@ -47,6 +47,12 @@ class Service: disposable: bool = False file: str = "?" line: int = 0 + # services injected via `WeakReference` — held WEAKLY, so they are NOT strong + # captive edges (DI001 must not see them), but a weakly-held scoped service is still + # a lifetime-contract violation: DI002. Declared LAST so the positional constructor + # contract (name, lifetime, deps, disposable, file, line) is preserved — callers pass + # `disposable`/etc. positionally, so a new field before them would shift their meaning. + weak_deps: tuple[str, ...] = () @dataclass(frozen=True) @@ -107,6 +113,58 @@ def find_captive_dependencies(services: list[Service]) -> list[CaptiveDependency return findings +@dataclass(frozen=True) +class WeakCaptiveDependency: + """A singleton that holds a **scoped** service via `WeakReference` (DI002). A + weak reference is the usual "fix" for the DI001 captive leak — it stops the + singleton from 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 reference only hides the + GC-retention symptom, not the captive cause (and may go dead under the consumer).""" + + singleton: str + captured: str + path: tuple[str, ...] + file: str + line: int + + @property + def message(self) -> str: + chain = " -> ".join(self.path) + return (f"singleton '{self.singleton}' weakly captures scoped service " + f"'{self.captured}' (WeakReference): '{self.captured}' is still resolved " + f"from the root provider and promoted to application lifetime — the weak " + f"reference avoids pinning it for the GC but does not fix the " + f"captive-dependency lifetime violation ({chain})") + + +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.)""" + 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: + continue + reported.add(dep) + findings.append(WeakCaptiveDependency( + singleton=s.name, captured=dep, path=(s.name, dep), + file=s.file, line=s.line)) + findings.sort(key=lambda f: (f.file, f.line, f.singleton, f.captured)) + return findings + + @dataclass(frozen=True) class CapturedTransientDisposable: """A singleton that captures a transient `IDisposable` service (DI003): the diff --git a/ownlang/ownir.py b/ownlang/ownir.py index 858c1211..5393177e 100644 --- a/ownlang/ownir.py +++ b/ownlang/ownir.py @@ -124,7 +124,12 @@ While, ) from .di import LIFETIMES as DI_LIFETIMES -from .di import Service, find_captive_dependencies, find_captured_transient_disposables +from .di import ( + Service, + find_captive_dependencies, + find_captured_transient_disposables, + find_weak_captive_dependencies, +) from .diagnostics import TITLES, Severity # The OwnIR schema version this core understands. Bump it whenever the fact @@ -448,6 +453,9 @@ def load(path: str) -> dict[str, Any]: deps = s.get("deps", []) if not isinstance(deps, list) or not all(isinstance(d, str) for d in deps): raise OwnIRError("service 'deps' must be an array of strings") + weak_deps = s.get("weak_deps", []) + if not isinstance(weak_deps, list) or not all(isinstance(d, str) for d in weak_deps): + raise OwnIRError("service 'weak_deps' must be an array of strings") if not isinstance(s.get("file", "?"), str): raise OwnIRError("service 'file' must be a string") ln = s.get("line", 0) @@ -1232,6 +1240,9 @@ def _di_findings(facts: dict[str, Any]) -> list[Finding]: name=str(s.get("name", "?")), lifetime=str(s.get("lifetime", "")), deps=tuple(s.get("deps", [])), + # services injected via WeakReference — held weakly, off the DI001 strong + # graph, but a weakly-held scoped service is still a captive (DI002). + weak_deps=tuple(s.get("weak_deps", [])), # only the JSON boolean `true` counts — a stray string ("false") or other # type from a non-extractor producer must not coerce to a disposable=True. disposable=s.get("disposable") is True, @@ -1258,6 +1269,17 @@ def _di_findings(facts: dict[str, Any]) -> list[Finding]: message=c.message, kind="DI lifetime", severity="warning") for c in find_captured_transient_disposables(services) ] + # DI002: a singleton holding a scoped service via WeakReference (P-006). The weak + # ref is the usual "fix" for a DI001 captive, but the scoped service is still + # root-resolved and app-lived — the lifetime contract is still violated. A real + # verdict shown at `warning` (the weak ref fixes the GC symptom, not the cause). + out += [ + Finding( + file=c.file, line=c.line, code="DI002", + component=c.singleton, event=c.captured, handler="", + message=c.message, kind="DI lifetime", severity="warning") + for c in find_weak_captive_dependencies(services) + ] return out diff --git a/tests/test_ownir.py b/tests/test_ownir.py index a24211e2..8c1a2fca 100644 --- a/tests/test_ownir.py +++ b/tests/test_ownir.py @@ -525,6 +525,47 @@ def _sub(source: str | None) -> list[Finding]: fails.append(f"DI003 bridge finding wrong: " f"{[(x.component, x.severity) for x in di3b]}") + # --- DI002 (P-006): a scoped service held by a singleton via WeakReference is a + # weak captive (warning). The weak edge lives in `weak_deps`, OFF the DI001 strong + # 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("Db", "scoped", ()), + Service("Strong", "singleton", deps=("Db",)), # strong -> scoped: DI001 + 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")]: + fails.append(f"DI002 set wrong: {got2}") + 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") + checks += 1 + if not di2 or "WeakReference" not in di2[0].message: + fails.append("DI002 message missing 'WeakReference'") + # bridge: DI002 surfaces as a WARNING; `weak_deps` is parsed and kept off DI001. + di2facts = {"ownir_version": 0, "module": "X", "components": [], "functions": [], + "services": [ + {"name": "WeakCache", "lifetime": "singleton", "deps": [], + "weak_deps": ["Db"], "file": "S.cs", "line": 9}, + {"name": "Db", "lifetime": "scoped", "deps": [], "file": "S.cs", "line": 10}, + ]} + di2b = check_facts(di2facts) + checks += 1 + di2only = [x for x in di2b if x.code == "DI002"] + if (len(di2only) != 1 or di2only[0].severity != "warning" + or di2only[0].component != "WeakCache"): + fails.append("DI002 bridge finding wrong: " + f"{[(x.component, x.severity) for x in di2only]}") + checks += 1 + if any(x.code == "DI001" for x in di2b): + fails.append("DI002 bridge wrongly also produced a DI001") + # bridge: the fixture surfaces exactly the two captive singletons as DI001 # at their registration lines; the clock/scoped-to-scoped stay silent. with open(_DI_FIXTURE, encoding="utf-8") as f: @@ -557,6 +598,13 @@ def _sub(source: str | None) -> list[Finding]: "services": [{"name": "X", "lifetime": "singleton", "line": "NaN"}]}): fails.append("a non-integer service line did not raise OwnIRError") + checks += 1 + # weak_deps (DI002) is validated like deps: a non-array (here a string, which would + # otherwise be char-split by tuple()) must fail loudly at load, not silently (Codex). + if not _load_raises({"ownir_version": OWNIR_VERSION, "components": [], + "services": [{"name": "X", "lifetime": "singleton", + "weak_deps": "abc"}]}): + fails.append("a non-array service weak_deps did not raise OwnIRError") # --- P-014 Tier A: an "unresolved-subscription" marker (the extractor could # not bind the `+=` LHS to an event) is NOT a leak — the lowering skips it