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
16 changes: 16 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 13 additions & 3 deletions docs/notes/di-captive-extractor.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>()` /
`GetRequiredService<T>()` **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<T>()`), 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<T>()` and non-generic `GetService(typeof(T))` resolution forms, and a
Expand Down
8 changes: 6 additions & 2 deletions docs/proposals/P-006-di-lifetimes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>()`), 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*
Expand Down
32 changes: 23 additions & 9 deletions frontend/roslyn/OwnSharp.Extractor/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -953,6 +953,10 @@
// through a scope (`scope.ServiceProvider.GetRequiredService<T>()`) has a different
// receiver and is deliberately NOT recorded — that pattern is correct.
var classRootResolves = new Dictionary<string, List<string>>();
// 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<string, List<object>>();
// 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
Expand Down Expand Up @@ -1042,14 +1046,19 @@
&& providerNames.Contains(fInit.Identifier.Text))
providerNames.Add(v.Identifier.Text);
var rootResolves = new List<string>();
var rootResolveSites = new List<object>();
if (providerNames.Count > 0)
{
var seenResolve = new HashSet<string>();
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.
Expand All @@ -1072,6 +1081,8 @@
? wd : new List<string>();
var rootResolves = impl is not null && classRootResolves.TryGetValue(impl, out var rr)
? rr : new List<string>();
var rootResolveSites = impl is not null
&& classRootResolveSites.TryGetValue(impl, out var rrs) ? rrs : new List<object>();
var (ctorFile, ctorLine) = impl is not null && classCtorLoc.TryGetValue(impl, out var cl)
? cl : ("?", 0);
services.Add(new
Expand All @@ -1087,6 +1098,8 @@
// 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
Expand Down Expand Up @@ -1191,12 +1204,13 @@
_ => null,
};

// The service types a class resolves BY HAND off an injected IServiceProvider: every
// `recv.GetService<T>()` / `recv.GetRequiredService<T>()` 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<string> RootResolvedTypes(
// The service types a class resolves BY HAND off an injected IServiceProvider, with the
// 1-based line of each `recv.GetService<T>()` / `recv.GetRequiredService<T>()` 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<string> providerNames)
{
foreach (var inv in cls.DescendantNodes().OfType<InvocationExpressionSyntax>())
Expand All @@ -1208,7 +1222,7 @@
|| !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);
}
}

Expand Down Expand Up @@ -1276,7 +1290,7 @@
.Split(Path.PathSeparator, StringSplitOptions.RemoveEmptyEntries)
.Where(p => p.EndsWith(".dll", StringComparison.OrdinalIgnoreCase))
.ToList();
var refNames = new HashSet<string>(tpa.Select(Path.GetFileName), StringComparer.OrdinalIgnoreCase);

Check warning on line 1293 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / C# leak extractor (Roslyn) -> OwnIR -> core

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1293 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check SARIF -> GitHub code scanning (dog-food)

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1293 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check SARIF -> GitHub code scanning (dog-food)

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1293 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1293 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1293 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / C# leak extractor (Roslyn) -> OwnIR -> core

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.
var references = tpa.Select(p => (MetadataReference)MetadataReference.CreateFromFile(p)).ToList();
// P-004 WPF profile: widen the reference set with assemblies named by the
// OWN_EXTRA_REF_DIRS env var (colon-separated dirs) — e.g. the WindowsDesktop ref
Expand Down
23 changes: 21 additions & 2 deletions ownlang/di.py
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>()` / `GetRequiredService<T>()` 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)
Expand Down Expand Up @@ -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(
Expand All @@ -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
Expand All @@ -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
Expand Down
56 changes: 50 additions & 6 deletions ownlang/ownir.py
Original file line number Diff line number Diff line change
Expand Up @@ -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", [])
Expand Down Expand Up @@ -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<T>()` **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."""
Expand Down Expand Up @@ -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)
]
Expand Down Expand Up @@ -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


Expand Down
Loading
Loading