Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
66 changes: 61 additions & 5 deletions docs/notes/di-captive-extractor.md
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,65 @@ WeakReference<Clock>` 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<T>()` / `GetRequiredService<T>()`, 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<T>()` /
`GetRequiredService<T>()` 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<T>()`
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<T>()`), 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<T>()` 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<T>()` and the `CreateScope()` → scope-provider form).
39 changes: 31 additions & 8 deletions docs/proposals/P-006-di-lifetimes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>` — 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<T>()` / `GetRequiredService<T>()`, 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).
Expand Down Expand Up @@ -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<T>()` 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<T>()` / `GetRequiredService<T>()`), 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
Expand Down Expand Up @@ -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
Expand All @@ -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<T>()` / `GetRequiredService<T>()` 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.)
Loading
Loading