diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d60550de..6902d261 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -344,7 +344,35 @@ jobs: 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" + # P-006 DI004 (transient IDisposable resolved BY HAND from the root IServiceProvider, + # WARNING): a singleton that service-locates a transient IDisposable off its injected + # root provider — tracked to app shutdown. This is a CALL SITE the registration graph + # (DI001/2/3) cannot see, so it is the unique slice. Three flagged shapes: + # - ConnectionResolver -> PooledConnection (block-bodied ctor, direct) + # - ExprBodiedResolver -> PooledConnection (EXPRESSION-bodied ctor; Codex) + # - WrapperResolver -> MidConnection -> PooledConnection (TRANSITIVE: the root builds + # the non-disposable wrapper's transient subtree; the DFS mirrors DI003; Codex) + echo "$out" | grep -qE "\[DI004\].*'ConnectionResolver' resolves transient IDisposable 'PooledConnection'" \ + || { echo "FAIL: expected DI004 (ConnectionResolver service-locates PooledConnection)"; exit 1; } + echo "$out" | grep -qE "\[DI004\].*'ExprBodiedResolver' resolves transient IDisposable 'PooledConnection'" \ + || { echo "FAIL: expected DI004 on the expression-bodied ctor (ExprBodiedResolver)"; exit 1; } + echo "$out" | grep -qE "\[DI004\].*'WrapperResolver' resolves transient IDisposable 'PooledConnection'" \ + || { echo "FAIL: expected transitive DI004 (WrapperResolver -> MidConnection -> PooledConnection)"; exit 1; } + # pin the rendered transitive PATH (not just the finding), like the DI002 transitive case. + echo "$out" | grep -q "WrapperResolver -> MidConnection -> PooledConnection" \ + || { echo "FAIL: expected the transitive DI004 path text"; exit 1; } + n4=$(echo "$out" | grep -cE "DiCaptiveSample\.cs:[0-9]+:.*\[DI004\]") + [ "$n4" = "3" ] \ + || { echo "FAIL: expected exactly 3 DI004 findings, got $n4"; 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 + # does not track it); RequestResolver is SCOPED (its injected provider is the request + # scope, not the root). MidConnection itself (a transient wrapper) is not a singleton. + if echo "$out" | grep -qE "(ScopedResolver|PlainResolver|RequestResolver)"; then + echo "FAIL: a correct/non-leaking resolver (scope-resolved, non-disposable, or scoped) was wrongly flagged DI004"; 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) + DI004 (transient IDisposable service-located from the root provider) 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 5430353a..6aa4e60e 100644 --- a/docs/notes/di-captive-extractor.md +++ b/docs/notes/di-captive-extractor.md @@ -119,9 +119,65 @@ WeakReference` stays silent (a weak ref to a singleton is no mismatch) general-purpose analyzer models — even the developer's WeakReference "fix" is still flagged, which is the key differentiation. +## DI004 — transient `IDisposable` service-located from the root provider (shipped) + +The graph checks above (DI001/2/3) read the **registration graph** — who is registered with +which lifetime, and who they inject. DI004 reads what the graph cannot see: a **call site**. A +**singleton** that injects an `IServiceProvider` and resolves a **transient `IDisposable`** from +it *by hand* — `_provider.GetService()` / `GetRequiredService()`, the **service-locator +anti-pattern** — leaks. For a singleton the injected provider *is* the root container, and the +root tracks every `IDisposable` it resolves and disposes them only at application shutdown; so +each such call accumulates a transient that its `transient` registration says should be +short-lived (the well-known "transient disposables captured by the root container" leak), made +worse by being a *repeated runtime* resolution. A **warning**, like DI003 — the framework allows +it; the lifetime promotion is the smell. + +It is filed as a **distinct code** (not "DI003, the explicit form"): the detection mechanism is +different (a resolution call site, not a constructor edge), the remediation is different (create +an `IServiceScope` and resolve from *its* provider), and one-code-per-rule keeps the SARIF +catalogue honest. + +**The extractor** (still purely syntactic) records, per class, the names that refer to an +injected `IServiceProvider` — the constructor parameters of type `IServiceProvider` (usable +directly in a primary-ctor class), plus any **real class field** assigned one of them in a +constructor (block- **or** expression-bodied, `=> _sp = sp;`) or via a field initializer +(`IServiceProvider _sp = sp;`). It then records every `name.GetService()` / +`GetRequiredService()` whose **receiver is one of those names** into a separate +**`root_resolves`** list on the service fact. `ownlang/di.py` `find_explicit_root_resolutions` +flags a **singleton** whose `root_resolves` reaches a **transient ∧ disposable** service — +either the resolved type itself or one its transient subtree drags in: the same transient-edge +DFS DI003 runs, but entered at the service-location call site instead of a constructor edge (the +root builds the resolved type's whole transient subtree, so a non-disposable transient *wrapper* +still leaks the disposable transient it depends on). + +**Precision (0 FP) is carried by guards**, each pinned by a silent control in +`DiCaptiveSample.cs`: + +- **singleton-only** — only a *singleton*'s injected provider is the root container, so a + singleton is what DI004 flags; the scoped `RequestResolver` is the silent control (a scoped + service is resolved inside a request scope, whose provider disposes what it resolves). +- **the injected provider, never a scope's** — `scope.ServiceProvider.GetRequiredService()` + has a different receiver (a member access, not the injected name), so creating a scope and + resolving from it — the *correct* pattern — stays silent (`ScopedResolver`). +- **a transient subtree, disposable, never scoped** — the root does not track non-disposables, + and a *scoped* edge is not followed (resolving scoped from the root is DI001's concern / a + runtime scope-validation error), so `PlainResolver` resolving the non-disposable `UnitOfWork` + (whose only dep is scoped) stays silent. +- **real fields only** — the alias capture restricts assignment targets to declared class + fields, so a constructor *local* alias never enters the provider-name set and cannot + same-name-match an unrelated receiver (no false positive). + +Aliases through locals, unknown receivers, and the non-generic `GetService(typeof(T))` form are +not guessed — they stay silent (recall left on the table to keep precision absolute). Pinned +end-to-end by `DiCaptiveSample.cs` — `ConnectionResolver` (block ctor), `ExprBodiedResolver` +(expression-bodied ctor), and the transitive `WrapperResolver → MidConnection → PooledConnection` +(primary-ctor field initializer), **exactly 3 DI004**, the three controls silent — in the +`wpf-extractor` CI job, and at the graph level by `tests/test_ownir.py`. No general-purpose +analyzer models this DI-container resolution contract. + ## Next (separate slices) -- **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. +- Anchoring the finding at the **consuming constructor** (DI001/2/3) or the **resolution call + site** (DI004) as well as the registration site (P-006 open question #1), with the path shown. +- The plural `GetServices()` and non-generic `GetService(typeof(T))` resolution forms, and a + directly-injected `IServiceScopeFactory` as the recognised fix (DI004 currently reads the + generic singular `Get(Required)Service()` and the `CreateScope()` → scope-provider form). diff --git a/docs/proposals/P-006-di-lifetimes.md b/docs/proposals/P-006-di-lifetimes.md index ace9520e..71f7896a 100644 --- a/docs/proposals/P-006-di-lifetimes.md +++ b/docs/proposals/P-006-di-lifetimes.md @@ -13,7 +13,11 @@ (a transient `IDisposable` captured by a singleton — promoted to application lifetime) 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. + end-to-end as **warnings**, CI-validated on the same sample. **DI004** (a transient + `IDisposable` resolved by hand from a singleton's injected **root** `IServiceProvider` — + `GetService()` / `GetRequiredService()`, the service-locator anti-pattern) extends + the family to a **call site** the registration graph cannot see, also a CI-validated + warning 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). @@ -52,8 +56,22 @@ to a longer-lived region) already models it. 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.) +- **DI004 (warning) — shipped:** the **explicit / service-locator** form of the + transient-`IDisposable` leak — a singleton that resolves it **by hand** from its injected + **root** `IServiceProvider` (`GetService()` / `GetRequiredService()`), which the + registration graph cannot see (it is a resolution call site, not a constructor edge). The + extractor records the injected-provider names per class (ctor params of type + `IServiceProvider` plus the real class fields assigned from them — in a block- or + expression-bodied ctor, or a field initializer) and reads each resolution off them into a + `root_resolves` list; `find_explicit_root_resolutions` walks the resolved type's transient + subtree exactly as DI003 does (so a non-disposable transient *wrapper* that drags in a + transient `IDisposable` is caught too) and flags the singleton. Filed as a **distinct code** + (not "DI003 explicit"): different detection, different fix (resolve from an `IServiceScope`). + Precision is held by guards — singleton-only, the injected provider (never a scope's + `.ServiceProvider`), transient ∧ disposable (scoped edges not followed), and alias capture + restricted to real fields (no local-alias false match) — each pinned by a control on + `DiCaptiveSample.cs` (`ConnectionResolver` / `ExprBodiedResolver` / transitive `WrapperResolver` + flagged; `ScopedResolver` / `PlainResolver` / `RequestResolver` silent). Suggested fix attached to DI001/DI002: inject `IServiceScopeFactory`, and per operation `using var scope = factory.CreateScope();` then resolve the scoped @@ -82,8 +100,8 @@ then checks the same region ordering it already uses for OWN014: a longer-lived region (Singleton) must not retain a value from a shorter-lived region (Scoped). ```text -Startup.cs / Program.cs --[extractor: registrations + ctor graph]--> facts.json - --[core: region ordering (OWN014 family)]--> DI001/DI002/DI003 @ registration site +Startup.cs / Program.cs --[extractor: registrations + ctor graph + resolution call sites]--> facts.json + --[core: region ordering (OWN014 family)]--> DI001/DI002/DI003/DI004 @ registration site ``` Could be its own `Own.DI` profile sharing the lifetime core. Factory and @@ -98,6 +116,11 @@ rather than guessed. 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* - (so we stay silent), as it should be? -4. Treat transient-`IDisposable`-from-root (DI003) as warning or error? (Warning - — it is a slow leak, not always a bug.) + (so we stay silent), as it should be? (For the explicit form (DI004): **yes**, by + construction — DI004 records only `GetService()` / `GetRequiredService()` on the + injected `IServiceProvider` names and **excludes** a scope's `.ServiceProvider` receiver, so + resolving from a scope created with `CreateScope()` is silent. Modelling a directly-injected + `IServiceScopeFactory` is not yet implemented — a natural future extension.) +4. Treat transient-`IDisposable`-from-root (DI003/DI004) as warning or error? (Warning + — it is a slow leak, not always a bug. DI004's call-site form is repeated at runtime, + arguably worse, but kept a warning for consistency with DI003.) diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 7a75cf29..dd82f1ca 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -946,6 +946,13 @@ static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) // 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(); + // class name -> service types it resolves BY HAND off an injected IServiceProvider + // (`provider.GetService()` / `GetRequiredService()`). For a SINGLETON that + // provider is the root container, so a transient IDisposable resolved this way is + // tracked to app shutdown: DI004 (the service-locator anti-pattern). A resolution + // through a scope (`scope.ServiceProvider.GetRequiredService()`) has a different + // receiver and is deliberately NOT recorded — that pattern is correct. + var classRootResolves = new Dictionary>(); foreach (var (_, tree) in parsed) foreach (var node in tree.GetRoot().DescendantNodes()) { @@ -987,6 +994,51 @@ static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) ctorDisposable[cls.Identifier.Text] = ctorDisposable.GetValueOrDefault(cls.Identifier.Text) || (cls.BaseList is { } bl && bl.Types.Any(bt => DiTypeName(bt.Type) is "IDisposable" or "IAsyncDisposable")); + // DI004 — the names that refer to an injected IServiceProvider (the ROOT provider + // for a singleton): the ctor parameters of type IServiceProvider (usable directly + // in a primary-ctor class), plus any real class field assigned one of them. A + // `name.GetService()` / `GetRequiredService()` call on such a name is then a + // hand resolution off the root; a `scope.ServiceProvider.Get...` call has a + // member-access receiver that is NOT one of these names, so it stays silent. + var providerNames = new HashSet(); + // the real class fields, so a ctor LOCAL alias can never enter providerNames: a + // bare-identifier assignment LHS that is not a field would otherwise false-match a + // same-named receiver in another scope and mint a DI004 false positive (CodeRabbit). + var classFieldNames = new HashSet( + cls.Members.OfType() + .SelectMany(f => f.Declaration.Variables) + .Select(v => v.Identifier.Text)); + if (widest is not null) + foreach (var p in widest.Parameters) + if (p.Type is not null && DiTypeName(p.Type) == "IServiceProvider") + providerNames.Add(p.Identifier.Text); + // `_field = sp;` in any ctor — BLOCK- or EXPRESSION-bodied (an expression-bodied + // ctor has a null Body, so scan the whole ctor's descendants, not just Body; Codex), + // restricted to a real field so a local alias never matches. + foreach (var mem in cls.Members) + if (mem is ConstructorDeclarationSyntax ctorDecl) + foreach (var asg in ctorDecl.DescendantNodes().OfType()) + if (asg.Right is IdentifierNameSyntax rhs + && providerNames.Contains(rhs.Identifier.Text) + && AssignedFieldName(asg.Left) is { } fld + && classFieldNames.Contains(fld)) + providerNames.Add(fld); + // field initializers that capture an injected provider param (the primary-ctor + // shape, e.g. `private readonly IServiceProvider _sp = sp;`). + foreach (var fdecl in cls.Members.OfType()) + foreach (var v in fdecl.Declaration.Variables) + if (v.Initializer?.Value is IdentifierNameSyntax fInit + && providerNames.Contains(fInit.Identifier.Text)) + providerNames.Add(v.Identifier.Text); + var rootResolves = new List(); + if (providerNames.Count > 0) + { + var seenResolve = new HashSet(); + foreach (var resolved in RootResolvedTypes(cls, providerNames)) + if (seenResolve.Add(resolved)) + rootResolves.Add(resolved); + } + classRootResolves[cls.Identifier.Text] = rootResolves; } // 2. registrations -> service facts at the registration site. @@ -1007,6 +1059,8 @@ static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) ? d : new List(); var weakDeps = impl is not null && ctorWeakDeps.TryGetValue(impl, out var wd) ? wd : new List(); + var rootResolves = impl is not null && classRootResolves.TryGetValue(impl, out var rr) + ? rr : new List(); services.Add(new { name = service, @@ -1017,6 +1071,9 @@ static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) // 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, + // 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, file, line = node.GetLocation().GetLineSpan().StartLinePosition.Line + 1, }); @@ -1103,6 +1160,50 @@ static void ResolveRegistration(SimpleNameSyntax name, ArgumentListSyntax args, ? DiTypeName(g.TypeArgumentList.Arguments[0]) : null; } +// The field a ctor assignment targets — `_field = ...` or `this._field = ...` — so a +// `_provider = provider;` copy of an injected IServiceProvider is tracked as a provider +// reference. A more complex LHS (indexer, nested member) is not a simple field -> null. +static string? AssignedFieldName(ExpressionSyntax lhs) => lhs switch +{ + IdentifierNameSyntax id => id.Identifier.Text, + MemberAccessExpressionSyntax { Expression: ThisExpressionSyntax, Name: IdentifierNameSyntax n } + => n.Identifier.Text, + _ => 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( + ClassDeclarationSyntax cls, HashSet providerNames) +{ + foreach (var inv in cls.DescendantNodes().OfType()) + { + if (inv.Expression is not MemberAccessExpressionSyntax ma + || ma.Name is not GenericNameSyntax gen + || gen.TypeArgumentList.Arguments.Count != 1 + || gen.Identifier.Text is not ("GetService" or "GetRequiredService") + || !ReceiverIsProvider(ma.Expression, providerNames)) + continue; + if (DiTypeName(gen.TypeArgumentList.Arguments[0]) is { } t) + yield return t; + } +} + +// Is the call receiver one of the injected-provider names — `provider` / `_provider` +// (identifier) or `this._provider` (this-qualified field)? A `scope.ServiceProvider` +// receiver is a member access NOT qualified by `this`, so it returns false: only the +// injected ROOT provider, never a scope's provider, is treated as a root resolution. +static bool ReceiverIsProvider(ExpressionSyntax recv, HashSet providerNames) => recv switch +{ + IdentifierNameSyntax id => providerNames.Contains(id.Identifier.Text), + MemberAccessExpressionSyntax { Expression: ThisExpressionSyntax, Name: IdentifierNameSyntax n } + => providerNames.Contains(n.Identifier.Text), + _ => false, +}; + // 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 68164e60..147045b8 100644 --- a/frontend/roslyn/samples/DiCaptiveSample.cs +++ b/frontend/roslyn/samples/DiCaptiveSample.cs @@ -66,6 +66,77 @@ 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) { } } + // DI004 — a singleton that resolves a TRANSIENT IDisposable from its injected ROOT + // IServiceProvider BY HAND (the service-locator anti-pattern). For a singleton the + // injected provider IS the root container; the root tracks every IDisposable it resolves + // and frees them only at app shutdown, so each GetRequiredService call accumulates a + // PooledConnection — an unbounded leak the registration graph (DI001/2/3) cannot see: it + // is a call site, not a constructor edge. A warning, read from the resolution call site. + public sealed class ConnectionResolver + { + private readonly IServiceProvider _sp; + public ConnectionResolver(IServiceProvider sp) { _sp = sp; } + public void Warm() { var c = _sp.GetRequiredService(); } // DI004 + } + + // control: a singleton that resolves the transient IDisposable from a SCOPE it creates + // (`scope.ServiceProvider`) — the CORRECT pattern; the scope owns and disposes it. The + // receiver is `scope.ServiceProvider`, not the injected provider, so DI004 stays SILENT. + public sealed class ScopedResolver + { + private readonly IServiceProvider _sp; + public ScopedResolver(IServiceProvider sp) { _sp = sp; } + public void Warm() + { + using var scope = _sp.CreateScope(); + var c = scope.ServiceProvider.GetRequiredService(); // SILENT (scope-resolved) + } + } + + // control: a singleton that resolves a NON-disposable transient (UnitOfWork) from the + // root — the root does not track non-disposables, so nothing leaks. SILENT (the target + // must be transient AND disposable). + public sealed class PlainResolver + { + private readonly IServiceProvider _sp; + public PlainResolver(IServiceProvider sp) { _sp = sp; } + public void Make() { var u = _sp.GetRequiredService(); } // SILENT (not disposable) + } + + // control: a SCOPED service resolving the transient IDisposable from its injected provider + // — that provider is the request scope (not the root), which disposes what it resolves. + // SILENT (only a SINGLETON's injected provider is the root). + public sealed class RequestResolver + { + private readonly IServiceProvider _sp; + public RequestResolver(IServiceProvider sp) { _sp = sp; } + public void Warm() { var c = _sp.GetRequiredService(); } // SILENT (scoped class) + } + + // DI004 (expression-bodied ctor) — the same leak as ConnectionResolver, but the injected + // provider is stored via an EXPRESSION-bodied constructor (`=> _sp = sp;`, whose Body is + // null), so the alias collection must scan the whole ctor, not just a block body (Codex). + public sealed class ExprBodiedResolver + { + private readonly IServiceProvider _sp; + public ExprBodiedResolver(IServiceProvider sp) => _sp = sp; + public void Warm() { var c = _sp.GetRequiredService(); } // DI004 + } + + // a NON-disposable transient that depends on the transient IDisposable PooledConnection — + // resolving it from the root still makes the root build and track PooledConnection. + public sealed class MidConnection { public MidConnection(PooledConnection c) { } } // transient wrapper + + // DI004 (transitive) — a singleton that service-locates the non-disposable transient wrapper + // MidConnection off the root; the root builds its whole transient subtree, so the disposable + // PooledConnection it drags in is tracked to app shutdown (WrapperResolver -> MidConnection -> + // PooledConnection). The root provider is captured by a primary-ctor FIELD INITIALIZER. + public sealed class WrapperResolver(IServiceProvider sp) + { + private readonly IServiceProvider _sp = sp; + public void Warm() { var m = _sp.GetRequiredService(); } // DI004 (transitive) + } + public static class Startup { public static void ConfigureServices(IServiceCollection services) @@ -114,6 +185,24 @@ public static void ConfigureServices(IServiceCollection services) services.AddSingleton(); // SILENT — a weak reference to the SINGLETON Clock is no lifetime mismatch. services.AddSingleton(); + + // FLAGGED (DI004, warning) — singleton ConnectionResolver resolves the transient + // IDisposable PooledConnection by hand off its injected ROOT IServiceProvider + // (service locator), tracked to app shutdown. A call site, not a ctor edge. + services.AddSingleton(); + // SILENT — resolves from a SCOPE it creates (scope.ServiceProvider), the correct shape. + services.AddSingleton(); + // SILENT — resolves a NON-disposable transient (UnitOfWork) from the root (untracked). + services.AddSingleton(); + // SILENT — a SCOPED service's injected provider is the request scope, not the root. + services.AddScoped(); + + // FLAGGED (DI004) — an expression-bodied ctor stores the injected root provider; same leak. + services.AddSingleton(); + // FLAGGED (DI004, transitive) — service-locates the non-disposable transient MidConnection + // off the root, which drags in the transient IDisposable PooledConnection (tracked to app exit). + services.AddTransient(); + services.AddSingleton(); } } @@ -130,4 +219,16 @@ public static class ServiceCollectionExtensions public static IServiceCollection AddTransient(this IServiceCollection s) => s; public static IServiceCollection AddSingleton(this IServiceCollection s, Type service, Type impl) => s; } + + // Stand-ins for the IServiceProvider resolution surface (DI004). `IServiceProvider` is the + // real System interface; the generic GetService/GetRequiredService and CreateScope are the + // Microsoft.Extensions.DependencyInjection extensions — provided here so the sample binds. + public interface IServiceScope : IDisposable { IServiceProvider ServiceProvider { get; } } + + public static class ServiceProviderExtensions + { + public static T GetRequiredService(this IServiceProvider sp) => default!; + public static T GetService(this IServiceProvider sp) => default!; + public static IServiceScope CreateScope(this IServiceProvider sp) => null!; + } } diff --git a/ownlang/di.py b/ownlang/di.py index 67a27c24..a7a5eecf 100644 --- a/ownlang/di.py +++ b/ownlang/di.py @@ -53,6 +53,12 @@ class Service: # 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, ...] = () + # service types this class resolves BY HAND from an injected `IServiceProvider` + # (`GetService()` / `GetRequiredService()`) — the service-locator pattern. For a + # SINGLETON the injected provider is the root container, so a transient `IDisposable` + # resolved this way is tracked to app shutdown: DI004. Off the registration graph (it is + # a call site, not a ctor edge); declared LAST, after `weak_deps`, for the same reason. + root_resolves: tuple[str, ...] = () @dataclass(frozen=True) @@ -236,3 +242,79 @@ def find_captured_transient_disposables( stack.append((dep, npath)) findings.sort(key=lambda f: (f.file, f.line, f.singleton, f.captured)) return findings + + +@dataclass(frozen=True) +class ExplicitRootResolution: + """A singleton that resolves a transient `IDisposable` BY HAND from its injected + **root** `IServiceProvider` — `GetService()` / `GetRequiredService()` (DI004, the + service-locator-from-root anti-pattern). For a singleton the injected provider *is* the + root container; the root tracks every `IDisposable` it resolves and disposes them only at + application shutdown, so each such call accumulates a transient that its `transient` + registration says should be short-lived — an unbounded leak the registration graph cannot + see (it is a call site, not a constructor edge). The disposable may be the resolved type + itself or a transient it drags in (the root builds the whole transient subtree); `path` + is the service-location chain (singleton -> resolved -> ... -> disposable).""" + + singleton: str + resolved: str + path: tuple[str, ...] + file: str + line: int + + @property + def message(self) -> str: + chain = " -> ".join(self.path) + 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})") + + +def find_explicit_root_resolutions( + services: list[Service]) -> list[ExplicitRootResolution]: + """Return every transient `IDisposable` a singleton resolves by hand from its injected + root `IServiceProvider` (DI004). Only **singletons** are considered: a singleton's + injected provider is the root container, whereas a scoped/transient service's injected + provider is its request scope (which disposes what it resolves — no leak, so it is left + silent). The extractor records only resolutions whose receiver is the injected provider + itself, never a scope's `.ServiceProvider`, so the service-locator-from-root pattern is + isolated from the correct scope-resolution pattern. + + From each resolved type, walk the STRONG transient graph exactly as DI003 does, but rooted + at the service-location call site instead of a constructor edge: the root provider builds + the resolved type's whole transient subtree, so a transient `IDisposable` reached directly + (`GetRequiredService()`) or through a non-disposable transient wrapper + (`GetRequiredService()` where `Mid` depends on a transient `IDisposable`) is tracked + to app shutdown and reported. A scoped edge is not followed (resolving scoped from the root + is DI001's concern / a runtime scope-validation error); a singleton edge is its own pass. + Cycles are guarded.""" + by_name = {s.name: s for s in services} + findings: list[ExplicitRootResolution] = [] + for s in services: + if s.lifetime != SINGLETON: + continue + reported: set[str] = set() + visited: set[str] = set() + # DFS rooted at each explicitly resolved type, following the transient deps the root + # provider builds and tracks (DI003's DFS, entered at the resolution call site). + stack: list[tuple[str, tuple[str, ...]]] = [ + (t, (s.name, t)) for t in s.root_resolves] + while stack: + cur, path = stack.pop() + node = by_name.get(cur) + if node is None or node.lifetime != TRANSIENT: + continue # only transients are root-built/tracked (scoped is DI001's) + if node.disposable and cur not in reported: + reported.add(cur) + findings.append(ExplicitRootResolution( + singleton=s.name, resolved=cur, path=path, + file=s.file, line=s.line)) + if cur not in visited: + visited.add(cur) + for dep in node.deps: # the root builds the transient's deps too + stack.append((dep, (*path, dep))) + findings.sort(key=lambda f: (f.file, f.line, f.singleton, f.resolved)) + return findings diff --git a/ownlang/ownir.py b/ownlang/ownir.py index 5393177e..b3b5c0a2 100644 --- a/ownlang/ownir.py +++ b/ownlang/ownir.py @@ -128,6 +128,7 @@ Service, find_captive_dependencies, find_captured_transient_disposables, + find_explicit_root_resolutions, find_weak_captive_dependencies, ) from .diagnostics import TITLES, Severity @@ -456,6 +457,10 @@ def load(path: str) -> dict[str, Any]: 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") + root_resolves = s.get("root_resolves", []) + if not isinstance(root_resolves, list) or not all( + isinstance(d, str) for d in root_resolves): + raise OwnIRError("service 'root_resolves' 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) @@ -1243,6 +1248,9 @@ def _di_findings(facts: dict[str, Any]) -> list[Finding]: # 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", [])), + # types resolved by hand from an injected IServiceProvider (GetService) — + # the service-locator call sites a singleton uses, checked for DI004. + root_resolves=tuple(s.get("root_resolves", [])), # 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, @@ -1280,6 +1288,20 @@ def _di_findings(facts: dict[str, Any]) -> list[Finding]: message=c.message, kind="DI lifetime", severity="warning") for c in find_weak_captive_dependencies(services) ] + # DI004: a singleton that resolves a transient IDisposable BY HAND from its injected + # root IServiceProvider (GetService/GetRequiredService — the service-locator + # anti-pattern). The root tracks every disposable it resolves and frees them only at + # app shutdown, so each call leaks a transient. A warning (the framework allows it; the + # 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", + component=c.singleton, event=c.resolved, handler="", + message=c.message, kind="DI lifetime", severity="warning") + for c in find_explicit_root_resolutions(services) + ] return out diff --git a/tests/test_ownir.py b/tests/test_ownir.py index 9021035d..f0e92f59 100644 --- a/tests/test_ownir.py +++ b/tests/test_ownir.py @@ -573,6 +573,59 @@ def _sub(source: str | None) -> list[Finding]: if any(x.code == "DI001" for x in di2b): fails.append("DI002 bridge wrongly also produced a DI001") + # --- DI004 (P-006): a transient IDisposable resolved BY HAND from a singleton's injected + # root IServiceProvider (the service-locator anti-pattern, warning). Only singletons are + # considered; the resolved type's transient subtree is walked like DI003, so a disposable + # reached directly OR through a non-disposable transient wrapper is reported. A + # 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("Conn", "transient", (), disposable=True), + Service("Wrap", "singleton", deps=(), root_resolves=("Mid",)), # transitive: DI004 + Service("Mid", "transient", ("Pool",)), # non-disposable wrapper -> disposable + Service("Pool", "transient", (), disposable=True), + Service("Plain", "singleton", deps=(), root_resolves=("Uow",)), # silent: scoped dep only + Service("Uow", "transient", ("Db",)), + Service("Db", "scoped", ()), + Service("Req", "scoped", deps=(), root_resolves=("Conn",)), # silent: scoped class + Service("Hold", "singleton", deps=(), root_resolves=("Clk",)), # silent: ->singleton + Service("Clk", "singleton", ()), + ] + di4 = find_explicit_root_resolutions(rsvcs) + checks += 1 + got4 = sorted((c.singleton, c.resolved) for c in di4) + if got4 != [("Resolver", "Conn"), ("Wrap", "Pool")]: + fails.append(f"DI004 set wrong: {got4}") + checks += 1 + # the transitive resolution carries the full path through the non-disposable wrapper. + rpath = next((c.path for c in di4 if c.singleton == "Wrap"), None) + if rpath != ("Wrap", "Mid", "Pool"): + fails.append(f"DI004 transitive path wrong: {rpath}") + 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. + di4facts = {"ownir_version": 0, "module": "X", "components": [], "functions": [], + "services": [ + {"name": "Resolver", "lifetime": "singleton", "deps": [], + "root_resolves": ["Conn"], "file": "S.cs", "line": 5}, + {"name": "Conn", "lifetime": "transient", "deps": [], + "disposable": True, "file": "S.cs", "line": 6}, + ]} + di4b = check_facts(di4facts) + 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"): + fails.append("DI004 bridge finding wrong: " + f"{[(x.component, x.severity) 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). + if any(x.code in ("DI001", "DI002", "DI003") for x in di4b): + fails.append("DI004 wrongly also produced a graph DI00x finding") + # 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: @@ -612,6 +665,12 @@ def _sub(source: str | None) -> list[Finding]: "services": [{"name": "X", "lifetime": "singleton", "weak_deps": "abc"}]}): fails.append("a non-array service weak_deps did not raise OwnIRError") + checks += 1 + # root_resolves (DI004) is validated the same way — a non-array must raise at load. + if not _load_raises({"ownir_version": OWNIR_VERSION, "components": [], + "services": [{"name": "X", "lifetime": "singleton", + "root_resolves": "abc"}]}): + fails.append("a non-array service root_resolves 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