diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ae1718c4..381abfcc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -377,6 +377,22 @@ jobs: n4=$(echo "$out" | grep -cE "DiCaptiveSample\.cs:[0-9]+:.*\[DI004\]") [ "$n4" = "3" ] \ || { echo "FAIL: expected exactly 3 DI004 findings, got $n4"; exit 1; } + # DI004's consumer is the GetRequiredService CALL SITE (not a ctor) — and the leak IS + # that call, so it is the PRIMARY anchor (the line prefix), not the registration site + # (Codex review): ConnectionResolver:79, ExprBodiedResolver:123, transitive + # WrapperResolver:137 (the entry MidConnection's call, not the dragged-in disposable). + echo "$out" | grep -qE "DiCaptiveSample\.cs:79: warning: \[DI004\].*'ConnectionResolver'" \ + || { echo "FAIL: expected DI004 ConnectionResolver ANCHORED at its call site (line 79)"; exit 1; } + echo "$out" | grep -qE "DiCaptiveSample\.cs:123: warning: \[DI004\].*'ExprBodiedResolver'" \ + || { echo "FAIL: expected DI004 ExprBodiedResolver anchored at its call site (line 123)"; exit 1; } + echo "$out" | grep -qE "DiCaptiveSample\.cs:137: warning: \[DI004\].*'WrapperResolver'" \ + || { echo "FAIL: expected transitive DI004 WrapperResolver anchored at the entry call site (line 137)"; exit 1; } + # the registration site rides along as the SECONDARY anchor (named in EACH DI004 + # message tail) — exactly 3, one per finding, so a partial-suffix regression (the tail + # on some findings but not all) fails CI too (CodeRabbit review; mirrors the counts above). + nreg=$(echo "$out" | grep -c "singleton registered at ") + [ "$nreg" = "3" ] \ + || { echo "FAIL: expected exactly 3 DI004 registration-site suffixes, got $nreg"; exit 1; } # the three controls each pin one precision guard and must stay SILENT: ScopedResolver # resolves from a SCOPE it creates (scope.ServiceProvider — the correct shape); # PlainResolver resolves a NON-disposable transient whose only dep is scoped (the root diff --git a/docs/notes/di-captive-extractor.md b/docs/notes/di-captive-extractor.md index 3305ff59..f39da915 100644 --- a/docs/notes/di-captive-extractor.md +++ b/docs/notes/di-captive-extractor.md @@ -203,10 +203,20 @@ already spells out. Pinned end-to-end by `ConnectionWarmer:50`, `WeakCache:57`) in the `wpf-extractor` CI job, the cross-file case by the `tests/fixtures/ownir/di.facts.json` fixture, and the SARIF related location by `tests/test_ownir.py`. +**DI004's consumer is a call site, not a ctor** — and the leak *is* that call, so unlike +DI001/2/3 (a registration-graph property, anchored at the registration) the `GetService()` / +`GetRequiredService()` **resolution call site is DI004's PRIMARY anchor**, with the +registration demoted to the secondary (the `Finding.related` location + a `[singleton registered +at …]` message tail). The extractor emits, alongside `root_resolves`, a parallel +**`root_resolve_sites`** (`{type, file, line}` per resolved type — the call's location); the core +makes that the finding's `file`/`line` (Codex review: a `related`-only call site still annotates +`Startup.cs`, not the leak). The site is the **entry** type's call (`path[1]`): for a transitive +leak (`WrapperResolver → MidConnection → PooledConnection`) it points at where `MidConnection` was +hand-resolved, not at the container-built `PooledConnection`. Falls back to the registration site +when the call is unknown. Pinned by `DiCaptiveSample.cs` (`ConnectionResolver:79`, +`ExprBodiedResolver:123`, transitive `WrapperResolver:137`). + ## Next (separate slices) -- The same **consumer anchor for DI004**, whose consumer is a **resolution call site** - (`GetRequiredService()`), not a constructor — the call-site location needs threading - through `root_resolves`; until then DI004 keeps its registration-site anchor. - Per-**parameter** precision for the captive anchor (the specific injecting parameter, not just the constructor). - The plural `GetServices()` and non-generic `GetService(typeof(T))` resolution forms, and a diff --git a/docs/proposals/P-006-di-lifetimes.md b/docs/proposals/P-006-di-lifetimes.md index c20530c7..1a2e48a4 100644 --- a/docs/proposals/P-006-di-lifetimes.md +++ b/docs/proposals/P-006-di-lifetimes.md @@ -122,8 +122,12 @@ rather than guessed. impl, not the ctor-less service interface). The extractor records each implementation's ctor location (the widest public ctor, or the class declaration for a primary/implicit ctor); the core appends it when known and degrades cleanly when not. - *Still open:* DI004's consumer is a **resolution call site**, not a ctor — anchoring it - there is the sibling follow-up (the call-site location is not yet threaded through). + **DI004** is also anchored, but the *other* way round: its consumer is a **resolution call site** + (`GetRequiredService()`), and the leak *is* that call — so the call site is DI004's **primary** + anchor (extractor threads each call's location through a parallel `root_resolve_sites` fact; the + finding lands at the *entry* type's call, even for a transitive leak), with the registration + demoted to the secondary. Registration-graph rules anchor at the registration; the call-site rule + anchors at the call. 2. How far to chase transitive captures through the constructor graph before the dynamic cases make it unreliable? (Bounded depth; stop at unknown edges.) 3. Is `IServiceScopeFactory` usage inside a singleton recognised as the *fix* diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 37dbfb07..a9801dca 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -953,6 +953,10 @@ static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) // through a scope (`scope.ServiceProvider.GetRequiredService()`) has a different // receiver and is deliberately NOT recorded — that pattern is correct. var classRootResolves = new Dictionary>(); + // class name -> the resolution CALL SITE of each root-resolved type ({type, file, line}). + // DI004's consumer is the GetRequiredService call site (not a ctor), so a finding anchors + // at it; emitted as `root_resolve_sites` alongside `root_resolves`. + var classRootResolveSites = new Dictionary>(); // class name -> its CONSUMING CONSTRUCTOR location (file, 1-based line): the widest // public ctor (or the class/primary-ctor declaration), where a captive dependency is // injected. A captive finding anchors at the registration site but names this too, so @@ -1042,14 +1046,19 @@ static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) && providerNames.Contains(fInit.Identifier.Text)) providerNames.Add(v.Identifier.Text); var rootResolves = new List(); + var rootResolveSites = new List(); if (providerNames.Count > 0) { var seenResolve = new HashSet(); - foreach (var resolved in RootResolvedTypes(cls, providerNames)) - if (seenResolve.Add(resolved)) + foreach (var (resolved, line) in RootResolvedTypes(cls, providerNames)) + if (seenResolve.Add(resolved)) // first call site per type wins + { rootResolves.Add(resolved); + rootResolveSites.Add(new { type = resolved, file = ctorFile, line }); + } } classRootResolves[cls.Identifier.Text] = rootResolves; + classRootResolveSites[cls.Identifier.Text] = rootResolveSites; } // 2. registrations -> service facts at the registration site. @@ -1072,6 +1081,8 @@ static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) ? wd : new List(); var rootResolves = impl is not null && classRootResolves.TryGetValue(impl, out var rr) ? rr : new List(); + var rootResolveSites = impl is not null + && classRootResolveSites.TryGetValue(impl, out var rrs) ? rrs : new List(); var (ctorFile, ctorLine) = impl is not null && classCtorLoc.TryGetValue(impl, out var cl) ? cl : ("?", 0); services.Add(new @@ -1087,6 +1098,8 @@ static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) // the IMPLEMENTATION's by-hand resolutions off its injected provider — for a // singleton, the root; a transient IDisposable resolved this way is DI004. root_resolves = rootResolves, + // the resolution CALL SITE of each — DI004 anchors at it (its real consumer). + root_resolve_sites = rootResolveSites, file, line = node.GetLocation().GetLineSpan().StartLinePosition.Line + 1, // the IMPLEMENTATION's consuming-constructor location (P-006 Q#1): where the @@ -1191,12 +1204,13 @@ static void ResolveRegistration(SimpleNameSyntax name, ArgumentListSyntax args, _ => null, }; -// The service types a class resolves BY HAND off an injected IServiceProvider: every -// `recv.GetService()` / `recv.GetRequiredService()` whose receiver is one of the -// injected-provider names (DI004). Single type argument only (the generic resolve form); -// a `scope.ServiceProvider.Get...` receiver is excluded by ReceiverIsProvider, so the -// correct scope-resolution pattern is never recorded. -static IEnumerable RootResolvedTypes( +// The service types a class resolves BY HAND off an injected IServiceProvider, with the +// 1-based line of each `recv.GetService()` / `recv.GetRequiredService()` call whose +// receiver is one of the injected-provider names (DI004). The line is the resolution call +// site — DI004's actual consumer. Single type argument only (the generic resolve form); a +// `scope.ServiceProvider.Get...` receiver is excluded by ReceiverIsProvider, so the correct +// scope-resolution pattern is never recorded. +static IEnumerable<(string type, int line)> RootResolvedTypes( ClassDeclarationSyntax cls, HashSet providerNames) { foreach (var inv in cls.DescendantNodes().OfType()) @@ -1208,7 +1222,7 @@ static IEnumerable RootResolvedTypes( || !ReceiverIsProvider(ma.Expression, providerNames)) continue; if (DiTypeName(gen.TypeArgumentList.Arguments[0]) is { } t) - yield return t; + yield return (t, inv.GetLocation().GetLineSpan().StartLinePosition.Line + 1); } } diff --git a/ownlang/di.py b/ownlang/di.py index e98908fb..bdc21818 100644 --- a/ownlang/di.py +++ b/ownlang/di.py @@ -85,6 +85,11 @@ class Service: # ctor is `Foo`'s — so the finding must name `Foo`, not the interface (Codex). Empty when # unknown, in which case the suffix names "the constructor" without a (wrong) type. ctor_type: str = "" + # for DI004 (service-location): where each `root_resolves` type was resolved by hand — + # `(type, file, line)` triples for the `GetService()` / `GetRequiredService()` call + # site. The consumer of a DI004 leak is this call site (not a ctor), so the finding anchors + # at it; optional presentation metadata, declared LAST (positional-contract safe). + root_resolve_sites: tuple[tuple[str, str, int], ...] = () @dataclass(frozen=True) @@ -305,16 +310,25 @@ class ExplicitRootResolution: path: tuple[str, ...] file: str line: int + # the GetService/GetRequiredService call site that hand-resolved the entry type — DI004's + # actual consumer (the leak *is* this call), so the bridge makes it the finding's PRIMARY + # anchor; `file`/`line` (the registration site) become the secondary. Unknown -> 0. + resolved_file: str = "?" + resolved_line: int = 0 @property def message(self) -> str: chain = " -> ".join(self.path) + # the call site is the primary anchor (set by the bridge); name the registration site + # (the secondary) in the tail when both it and the call site are known. + reg = (f" [singleton registered at {self.file}:{self.line}]" + if self.resolved_line >= 1 and self.line >= 1 else "") return (f"singleton '{self.singleton}' resolves transient IDisposable " f"'{self.resolved}' by hand from its injected root IServiceProvider " f"(GetService/GetRequiredService — the service-locator anti-pattern): the " f"root provider tracks every IDisposable it resolves and frees them only at " f"application shutdown, so each call leaks a transient that should be " - f"scope-lived — resolve it from an IServiceScope instead ({chain})") + f"scope-lived — resolve it from an IServiceScope instead ({chain}){reg}") def find_explicit_root_resolutions( @@ -340,6 +354,8 @@ def find_explicit_root_resolutions( for s in services: if s.lifetime != SINGLETON: continue + # the hand-resolution call site for each entry type (path[1]) — the DI004 consumer. + sites = {t: (f, ln) for (t, f, ln) in s.root_resolve_sites} reported: set[str] = set() visited: set[str] = set() # DFS rooted at each explicitly resolved type, following the transient deps the root @@ -353,9 +369,12 @@ def find_explicit_root_resolutions( continue # only transients are root-built/tracked (scoped is DI001's) if node.disposable and cur not in reported: reported.add(cur) + # the call site is where the ENTRY type (path[1]) was hand-resolved, even when + # the leaked disposable is dragged in transitively below it. + rf, rl = sites.get(path[1], ("?", 0)) if len(path) >= 2 else ("?", 0) findings.append(ExplicitRootResolution( singleton=s.name, resolved=cur, path=path, - file=s.file, line=s.line)) + file=s.file, line=s.line, resolved_file=rf, resolved_line=rl)) if cur not in visited: visited.add(cur) for dep in node.deps: # the root builds the transient's deps too diff --git a/ownlang/ownir.py b/ownlang/ownir.py index 37904d37..a88b77ee 100644 --- a/ownlang/ownir.py +++ b/ownlang/ownir.py @@ -490,6 +490,16 @@ def load(path: str) -> dict[str, Any]: raise OwnIRError("service 'ctor_line' must be an integer") if not isinstance(s.get("ctor_type", ""), str): raise OwnIRError("service 'ctor_type' must be a string") + # DI004 call-site metadata (optional): an array of {type, file, line} objects. + sites = s.get("root_resolve_sites", []) + if not isinstance(sites, list) or not all( + isinstance(x, dict) and isinstance(x.get("type", ""), str) + and isinstance(x.get("file", "?"), str) + and isinstance(x.get("line", 0), int) and not isinstance(x.get("line", 0), bool) + for x in sites): + raise OwnIRError( + "service 'root_resolve_sites' must be an array of " + "{type:str, file:str, line:int} objects") # Optional per-method flow bodies (P-016 B0b/B2 — local IDisposable # acquire/use/release over a CFG). Additive/optional; an older core ignores it. fns = result.get("functions", []) @@ -1272,6 +1282,37 @@ def _consumer_related(c: Any) -> tuple[tuple[str, int, str], ...]: return () +def _di004_primary(c: Any) -> tuple[str, int]: + """DI004's primary anchor — the `GetRequiredService()` **call site** (where the leak is + and where it is fixed), falling back to the registration site when the extractor did not + record the call. Unlike DI001/2/3 (a registration-graph property, anchored at the + registration), DI004 is a call-site property, so the call site is the primary (Codex).""" + if getattr(c, "resolved_line", 0) >= 1: + return (c.resolved_file, c.resolved_line) + return (c.file, c.line) + + +def _di004_related(c: Any) -> tuple[tuple[str, int, str], ...]: + """The DI004 **registration** site as a structured related location — the secondary anchor, + beside the call-site primary. Empty when the call site is unknown (then the registration is + already the primary) or the registration line is unknown.""" + if getattr(c, "resolved_line", 0) >= 1 and getattr(c, "line", 0) >= 1: + return ((c.file, c.line, f"registration of singleton '{c.singleton}'"),) + return () + + +def _resolve_sites(raw: Any) -> tuple[tuple[str, str, int], ...]: + """Parse a service's optional `root_resolve_sites` (DI004 call-site metadata) into + `(type, file, line)` triples. Tolerant for direct `check_facts` callers; `load()` does the + strict shape check.""" + if not isinstance(raw, list): + return () + return tuple( + (str(x.get("type", "")), str(x.get("file", "?")), _as_int(x.get("line", 0))) + for x in raw if isinstance(x, dict) + ) + + def _di_findings(facts: dict[str, Any]) -> list[Finding]: """Run the DI captive-dependency check over the facts' `services` graph and map each result to a DI001 Finding at its registration site.""" @@ -1301,6 +1342,9 @@ def _di_findings(facts: dict[str, Any]) -> list[Finding]: # the IMPLEMENTATION type owning that ctor — named in the finding instead of the # (possibly interface) service name, which would point at a ctor-less type (Codex). ctor_type=str(s.get("ctor_type", "")), + # DI004 call-site metadata: where each root_resolves type was hand-resolved, so the + # finding can anchor at the GetRequiredService call site (its real consumer). + root_resolve_sites=_resolve_sites(s.get("root_resolve_sites", [])), ) for s in raw if isinstance(s, dict) ] @@ -1342,13 +1386,13 @@ def _di_findings(facts: dict[str, Any]) -> list[Finding]: # call-site lifetime promotion is the smell), and a CALL SITE the registration graph # (DI001/002/003) cannot see — only resolutions off the injected provider, never a # scope's, so the correct scope-resolution pattern stays silent. - out += [ - Finding( - file=c.file, line=c.line, code="DI004", + for c in find_explicit_root_resolutions(services): + pf, pl = _di004_primary(c) # the call site (its real consumer), or registration if unknown + out.append(Finding( + file=pf, line=pl, code="DI004", component=c.singleton, event=c.resolved, handler="", - message=c.message, kind="DI lifetime", severity="warning") - for c in find_explicit_root_resolutions(services) - ] + message=c.message, kind="DI lifetime", severity="warning", + related=_di004_related(c))) return out diff --git a/tests/test_ownir.py b/tests/test_ownir.py index 6b4847ba..968d2167 100644 --- a/tests/test_ownir.py +++ b/tests/test_ownir.py @@ -589,9 +589,11 @@ def _sub(source: str | None) -> list[Finding]: # scope-resolved, scoped-class, or non-disposable+scoped-dep-only resolution stays silent. from ownlang.di import find_explicit_root_resolutions rsvcs = [ - Service("Resolver", "singleton", deps=(), root_resolves=("Conn",)), # direct: DI004 + Service("Resolver", "singleton", deps=(), root_resolves=("Conn",), + root_resolve_sites=(("Conn", "R.cs", 42),)), # direct: DI004, call site 42 Service("Conn", "transient", (), disposable=True), - Service("Wrap", "singleton", deps=(), root_resolves=("Mid",)), # transitive: DI004 + Service("Wrap", "singleton", deps=(), root_resolves=("Mid",), + root_resolve_sites=(("Mid", "R.cs", 88),)), # transitive: DI004, entry call site 88 Service("Mid", "transient", ("Pool",)), # non-disposable wrapper -> disposable Service("Pool", "transient", (), disposable=True), Service("Plain", "singleton", deps=(), root_resolves=("Uow",)), # silent: scoped dep only @@ -614,11 +616,26 @@ def _sub(source: str | None) -> list[Finding]: checks += 1 if not di4 or not all("service-locator" in c.message for c in di4): fails.append("DI004 message missing 'service-locator'") - # bridge: DI004 surfaces as a WARNING; `root_resolves` is parsed and checked. + checks += 1 + # DI004 records the GetRequiredService CALL SITE: the direct case its own site (42), and the + # transitive case the ENTRY type's site (Mid@88), NOT the dragged-in disposable's. + direct4 = next((c for c in di4 if c.singleton == "Resolver"), None) + trans4 = next((c for c in di4 if c.singleton == "Wrap"), None) + d4loc = (direct4.resolved_file, direct4.resolved_line) if direct4 else None + if direct4 is None or d4loc != ("R.cs", 42): + fails.append(f"DI004 direct call-site (resolved_*) wrong: {d4loc!r}") + checks += 1 + t4loc = (trans4.resolved_file, trans4.resolved_line) if trans4 else None + if trans4 is None or t4loc != ("R.cs", 88): + fails.append(f"DI004 transitive call-site (resolved_*) wrong: {t4loc!r}") + # bridge: DI004 surfaces as a WARNING, anchored at the CALL SITE (R.cs:42) — its real + # consumer (Codex) — with the REGISTRATION (S.cs:5) as the Finding.related secondary and + # named in the message tail. (registration site S.cs:5 differs from the call site R.cs:42.) di4facts = {"ownir_version": 0, "module": "X", "components": [], "functions": [], "services": [ {"name": "Resolver", "lifetime": "singleton", "deps": [], - "root_resolves": ["Conn"], "file": "S.cs", "line": 5}, + "root_resolves": ["Conn"], "file": "S.cs", "line": 5, + "root_resolve_sites": [{"type": "Conn", "file": "R.cs", "line": 42}]}, {"name": "Conn", "lifetime": "transient", "deps": [], "disposable": True, "file": "S.cs", "line": 6}, ]} @@ -626,9 +643,11 @@ def _sub(source: str | None) -> list[Finding]: checks += 1 di4only = [x for x in di4b if x.code == "DI004"] if (len(di4only) != 1 or di4only[0].severity != "warning" - or di4only[0].component != "Resolver"): + or (di4only[0].file, di4only[0].line) != ("R.cs", 42) + or di4only[0].related != (("S.cs", 5, "registration of singleton 'Resolver'"),) + or "[singleton registered at S.cs:5]" not in di4only[0].message): fails.append("DI004 bridge finding wrong: " - f"{[(x.component, x.severity) for x in di4only]}") + f"{[(x.file, x.line, x.related) for x in di4only]}") checks += 1 # the explicit root resolution is a CALL SITE, not a registration-graph edge: it must not # also produce a DI001/DI002/DI003 (the singleton has no scoped/transient ctor dependency). @@ -746,6 +765,12 @@ def _sub(source: str | None) -> list[Finding]: "services": [{"name": "X", "lifetime": "singleton", "ctor_type": 5}]}): fails.append("a non-string service ctor_type did not raise OwnIRError") + checks += 1 + # root_resolve_sites (DI004 call-site metadata) must be an array of {type,file,line} objects. + if not _load_raises({"ownir_version": OWNIR_VERSION, "components": [], + "services": [{"name": "X", "lifetime": "singleton", + "root_resolve_sites": [{"type": "T", "line": "NaN"}]}]}): + fails.append("a malformed service root_resolve_sites 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