diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e294e948..56daf921 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -308,7 +308,16 @@ jobs: nd=$(echo "$out" | grep -cE "DiCaptiveSample\.cs:[0-9]+:.*\[DI001\]") [ "$nd" = "4" ] \ || { echo "FAIL: expected exactly 4 DI001 captive findings, got $nd"; exit 1; } - echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) + DI001 (captive dependency) at the C# location" + # P-006 DI003 (transient IDisposable captured by a singleton, WARNING): the + # singleton ConnectionWarmer holds the transient IDisposable PooledConnection + # for the app lifetime (disposed only at root disposal). A warning, distinct + # from a DI001 — the "exactly 4 DI001" count above proves it is not miscounted. + echo "$out" | grep -qE "\[DI003\].*'ConnectionWarmer' captures transient IDisposable 'PooledConnection'" \ + || { echo "FAIL: expected DI003 (ConnectionWarmer captures transient IDisposable PooledConnection)"; exit 1; } + 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" - 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/ROADMAP.md b/docs/ROADMAP.md index 00ddc3b4..0d6f0cba 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -146,8 +146,9 @@ architectural strictness, and the borrow-checker showcase): registration + constructor graph from `Add{Singleton,Scoped,Transient}` (generic and `typeof(...)` forms) and the core flags the captive — direct, transitive through a transient, or through an interface registration — CI-validated on - `DiCaptiveSample.cs`. Remaining: DI002 (weak-ref), DI003 (transient-`IDisposable` - from root), and the consuming-constructor anchor. + `DiCaptiveSample.cs`. **DI003** (a transient `IDisposable` captured by a singleton, + warning) now also fires on the same sample. Remaining: DI002 (weak-ref), the explicit + root-`GetService` form of DI003, and the consuming-constructor anchor. 4. **Pool/Span** — `Rent`/`Return`, borrowed views, return-invalidates-views, known-bug replay corpus (P-007). The borrow checker on stage at full height. 5. **Effects** — `pure` / `use !Db` / `use !Log` / `use Clock`, layer policies diff --git a/docs/notes/di-captive-extractor.md b/docs/notes/di-captive-extractor.md index 08d3aae0..0f9fbff1 100644 --- a/docs/notes/di-captive-extractor.md +++ b/docs/notes/di-captive-extractor.md @@ -83,12 +83,27 @@ Infer# cover the Dispose/RAII family; neither flags these). It is also a clean reuse win: a whole new diagnostic class on real C# with **zero core changes** — the frontend just produces the `services` facts the lifetime core already checks. +## DI003 — transient `IDisposable` captured by a singleton (shipped) + +A **transient `IDisposable` captured by a singleton** is resolved from the root (via the +singleton), promoted to the application lifetime, and disposed only when the root provider +is disposed — held far longer than its `transient` registration implies. Detected by the +same registration-graph DFS as DI001 (`ownlang/di.py` `find_captured_transient_disposables`, +target = *transient ∧ disposable*), surfaced as a **warning** (`severity="warning"` — a real +verdict shown soft; the framework allows it, the lifetime promotion is the smell). The +extractor marks a service `disposable` when its implementation's **own** base list names +`IDisposable`/`IAsyncDisposable` (syntactic — an inherited disposable is not guessed, so the +warning fires only where ownership is certain). Pinned end-to-end by `DiCaptiveSample.cs` +(`ConnectionWarmer` → transient `PooledConnection`) in the `wpf-extractor` CI job, and at the +graph level by `tests/test_ownir.py`. + ## 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. -- **DI003** — a transient `IDisposable` resolved from the **root** provider, never - disposed until the app exits (a slow leak). +- **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). - Anchoring the finding at the **consuming constructor** as well as the registration site (P-006 open question #1), with the capture path shown. diff --git a/docs/proposals/P-006-di-lifetimes.md b/docs/proposals/P-006-di-lifetimes.md index 73e33fd6..cb88737f 100644 --- a/docs/proposals/P-006-di-lifetimes.md +++ b/docs/proposals/P-006-di-lifetimes.md @@ -9,8 +9,10 @@ on C#**, CI-validated on `frontend/roslyn/samples/DiCaptiveSample.cs` (direct, transitive-via-transient, and interface-registration captures flagged; singleton→singleton and clean registrations silent). See - [docs/notes/di-captive-extractor.md](../notes/di-captive-extractor.md). Next: - DI002 (weak-ref) and DI003 (transient-`IDisposable`-from-root). + [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). - **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). @@ -43,8 +45,13 @@ to a longer-lived region) already models it. 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."* -- **DI003 (warning):** a transient `IDisposable` resolved from the **root** - provider — never disposed until the app exits (a slow leak). +- **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 + implies. The same registration-graph DFS as DI001 (target = transient ∧ disposable); + the extractor marks a service `disposable` from its impl's own `: IDisposable` base. + (The explicit `root.GetService()` resolution-site form is a later slice — it needs + the call sites, not just the graph.) Suggested fix attached to DI001/DI002: inject `IServiceScopeFactory`, and per operation `using var scope = factory.CreateScope();` then resolve the scoped diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 804135f6..42d6703e 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -871,6 +871,11 @@ 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 -> 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 + // disposable impl, never a guessed one (precision over recall). + var ctorDisposable = new Dictionary(); foreach (var (_, tree) in parsed) foreach (var node in tree.GetRoot().DescendantNodes()) { @@ -899,6 +904,12 @@ static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) deps.Add(tn); } ctorDeps[cls.Identifier.Text] = deps; // last decl wins (core dedups by name) + // 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`. + ctorDisposable[cls.Identifier.Text] = ctorDisposable.GetValueOrDefault(cls.Identifier.Text) + || (cls.BaseList is { } bl + && bl.Types.Any(bt => DiTypeName(bt.Type) is "IDisposable" or "IAsyncDisposable")); } // 2. registrations -> service facts at the registration site. @@ -922,6 +933,10 @@ static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) name = service, lifetime, deps, + // 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). + disposable = impl is not null && ctorDisposable.TryGetValue(impl, out var disp) && disp, file, line = node.GetLocation().GetLineSpan().StartLinePosition.Line + 1, }); diff --git a/frontend/roslyn/samples/DiCaptiveSample.cs b/frontend/roslyn/samples/DiCaptiveSample.cs index 2323bb8f..5bd34fba 100644 --- a/frontend/roslyn/samples/DiCaptiveSample.cs +++ b/frontend/roslyn/samples/DiCaptiveSample.cs @@ -41,6 +41,14 @@ public PublicCtorOnly() { } private PublicCtorOnly(AppDbContext db) { } } + // DI003 — a singleton that captures a TRANSIENT IDisposable. The container builds + // the connection once with the singleton and holds it for the whole app; it is + // disposed only at root disposal, not per use. A warning (the lifetime promotion is + // the smell), distinct from the DI001 captives above. `PooledConnection` is + // `: IDisposable`, so the extractor marks its service `disposable`. + public sealed class PooledConnection : System.IDisposable { public void Dispose() { } } // transient, IDisposable + public sealed class ConnectionWarmer { public ConnectionWarmer(PooledConnection c) { } } // singleton -> captures it + public static class Startup { public static void ConfigureServices(IServiceCollection services) @@ -69,6 +77,12 @@ public static void ConfigureServices(IServiceCollection services) // SILENT — singleton -> singleton (Clock); the typeof(...) registration form. services.AddSingleton(typeof(Metrics), typeof(Metrics)); + + // FLAGGED (DI003, warning) — singleton ConnectionWarmer captures the + // transient IDisposable PooledConnection: promoted to application lifetime, + // disposed only at root disposal. NOT a DI001 (no scoped captured). + services.AddTransient(); + services.AddSingleton(); } } diff --git a/ownlang/di.py b/ownlang/di.py index 2d8a8556..25f78ab6 100644 --- a/ownlang/di.py +++ b/ownlang/di.py @@ -36,12 +36,15 @@ @dataclass(frozen=True) class Service: """One DI registration: a service `name`, its `lifetime`, and the service - names it depends on (constructor injection). `file`/`line` point at the - registration site so a finding lands there.""" + names it depends on (constructor injection). `disposable` is whether the + implementation type is `IDisposable`/`IAsyncDisposable` (so its disposal is the + container's concern). `file`/`line` point at the registration site so a finding + lands there.""" name: str lifetime: str deps: tuple[str, ...] = () + disposable: bool = False file: str = "?" line: int = 0 @@ -102,3 +105,62 @@ def find_captive_dependencies(services: list[Service]) -> list[CaptiveDependency # a singleton dependency is safe here (captor reported on its own) 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 + transient is resolved from the root (via the singleton), promoted to the + application lifetime, and disposed only when the root provider is disposed — held + until the app exits, far longer than its `transient` registration implies.""" + + 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}' captures transient IDisposable " + f"'{self.captured}': it is promoted to application lifetime and " + f"disposed only when the root provider is disposed ({chain})") + + +def find_captured_transient_disposables( + services: list[Service]) -> list[CapturedTransientDisposable]: + """Return every transient `IDisposable` captured by a singleton (DI003). For each + singleton, walk its dependency chain through transients (a transient held by a + singleton is itself singleton-lived, resolved from the root); a transient that is + `IDisposable` is reported — it is disposed only at root disposal (app exit). A + scoped edge belongs to DI001 (not followed here); a singleton edge is another + singleton's own pass. Cycles are guarded.""" + by_name = {s.name: s for s in services} + findings: list[CapturedTransientDisposable] = [] + for s in services: + if s.lifetime != SINGLETON: + continue + reported: set[str] = set() + visited: set[str] = set() + stack: list[tuple[str, tuple[str, ...]]] = [(s.name, (s.name,))] + while stack: + cur, path = stack.pop() + node = by_name.get(cur) + if node is None: + continue + for dep in node.deps: + dnode = by_name.get(dep) + if dnode is None or dnode.lifetime != TRANSIENT: + continue # scoped -> DI001; singleton -> its own pass + npath = (*path, dep) + if dnode.disposable and dep not in reported: + reported.add(dep) + findings.append(CapturedTransientDisposable( + singleton=s.name, captured=dep, path=npath, + file=s.file, line=s.line)) + if dep not in visited: + visited.add(dep) + stack.append((dep, npath)) + findings.sort(key=lambda f: (f.file, f.line, f.singleton, f.captured)) + return findings diff --git a/ownlang/ownir.py b/ownlang/ownir.py index 179cf8e7..858c1211 100644 --- a/ownlang/ownir.py +++ b/ownlang/ownir.py @@ -124,7 +124,7 @@ While, ) from .di import LIFETIMES as DI_LIFETIMES -from .di import Service, find_captive_dependencies +from .di import Service, find_captive_dependencies, find_captured_transient_disposables from .diagnostics import TITLES, Severity # The OwnIR schema version this core understands. Bump it whenever the fact @@ -1232,18 +1232,33 @@ def _di_findings(facts: dict[str, Any]) -> list[Finding]: name=str(s.get("name", "?")), lifetime=str(s.get("lifetime", "")), deps=tuple(s.get("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, file=str(s.get("file", "?")), line=_as_int(s.get("line", 0)), ) for s in raw if isinstance(s, dict) ] - return [ + out = [ Finding( file=c.file, line=c.line, code="DI001", component=c.singleton, event=c.captured, handler="", message=c.message, kind="DI lifetime") for c in find_captive_dependencies(services) ] + # DI003: a transient IDisposable captured by a singleton is promoted to application + # lifetime and disposed only at root disposal (P-006). A real verdict, but shown at + # `warning` level — the framework allows it; the lifetime promotion is the smell. + # Not `advisory` (that is for "not checked"): this IS checked and found. + out += [ + Finding( + file=c.file, line=c.line, code="DI003", + component=c.singleton, event=c.captured, handler="", + message=c.message, kind="DI lifetime", severity="warning") + for c in find_captured_transient_disposables(services) + ] + return out def _unresolved_findings(facts: dict[str, Any]) -> list[Finding]: diff --git a/tests/test_ownir.py b/tests/test_ownir.py index a7c1ae29..a24211e2 100644 --- a/tests/test_ownir.py +++ b/tests/test_ownir.py @@ -488,6 +488,43 @@ def _sub(source: str | None) -> list[Finding]: if cpath != ("C", "T", "B"): fails.append(f"transitive captive path wrong: {cpath}") + # --- DI003 (P-006): a transient IDisposable captured by a singleton is promoted + # to application lifetime (warning). The same DFS as DI001, target = transient + # AND disposable; a non-disposable transient and a scoped capture stay silent. + from ownlang.di import find_captured_transient_disposables + dsvcs = [ + Service("Cache", "singleton", ("Conn",)), # -> transient disposable: DI003 + Service("Conn", "transient", (), disposable=True), + Service("Warm", "singleton", ("Mid",)), # -> transient -> disposable + Service("Mid", "transient", ("Pool",), disposable=False), + Service("Pool", "transient", (), disposable=True), + Service("Plain", "singleton", ("Plumb",)), # transient, not disposable: silent + Service("Plumb", "transient", (), disposable=False), + Service("Cap", "singleton", ("Db",)), # singleton -> scoped: DI001, not DI003 + Service("Db", "scoped", (), disposable=True), + ] + di3 = find_captured_transient_disposables(dsvcs) + checks += 1 + got3 = sorted((c.singleton, c.captured) for c in di3) + if got3 != [("Cache", "Conn"), ("Warm", "Pool")]: + fails.append(f"DI003 set wrong: {got3}") + checks += 1 + if any("IDisposable" not in c.message for c in di3): + fails.append("DI003 message missing 'IDisposable'") + # bridge: DI003 surfaces as a WARNING-severity finding; `disposable` is parsed. + di3facts = {"ownir_version": 0, "module": "X", "components": [], "functions": [], + "services": [ + {"name": "Cache", "lifetime": "singleton", "deps": ["Conn"], + "file": "S.cs", "line": 7}, + {"name": "Conn", "lifetime": "transient", "deps": [], + "disposable": True, "file": "S.cs", "line": 8}, + ]} + di3b = [x for x in check_facts(di3facts) if x.code == "DI003"] + checks += 1 + if len(di3b) != 1 or di3b[0].severity != "warning" or di3b[0].component != "Cache": + fails.append(f"DI003 bridge finding wrong: " + f"{[(x.component, x.severity) for x in di3b]}") + # 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: