diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 82f061b7..6221e0a3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -132,6 +132,7 @@ jobs: frontend/roslyn/samples/StaticHandlerViewModel.cs \ frontend/roslyn/samples/StaticEventEscapeViewModel.cs \ frontend/roslyn/samples/WhenAnyValueViewModel.cs \ + frontend/roslyn/samples/DiCaptiveSample.cs \ frontend/roslyn/samples/SampleTypes.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" @@ -273,7 +274,40 @@ jobs: if echo "$out" | grep -q "CleanStaticEventViewModel"; then echo "FAIL: an unsubscribed (released) static-event subscription was wrongly reported"; exit 1 fi - echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) at the C# location" + # P-006 DI001 (captive dependency): the registration + constructor graph + # extracted from DiCaptiveSample.cs feeds ownlang/di.py. A singleton that + # captures a scoped service — directly, transitively through a transient, + # or through an interface registration — is flagged at the registration + # site; a singleton->singleton edge and the clean registrations stay silent. + echo "$out" | grep -q "DI001" \ + || { echo "FAIL: expected DI001 captive-dependency findings"; exit 1; } + echo "$out" | grep -q "singleton 'EmailSender' captures scoped service 'AppDbContext'" \ + || { echo "FAIL: expected the direct captive (singleton EmailSender -> scoped AppDbContext)"; exit 1; } + # the transitive capture must thread through the transient UnitOfWork. + echo "$out" | grep -q "ReportService -> UnitOfWork -> AppDbContext" \ + || { echo "FAIL: expected the transitive captive path via the transient UnitOfWork"; exit 1; } + # the interface registration (AddScoped) must map so the + # singleton consuming IRepo is caught. + echo "$out" | grep -q "singleton 'CacheService' captures scoped service 'IRepo'" \ + || { echo "FAIL: expected the interface-registration captive (CacheService -> IRepo)"; exit 1; } + # C# 12 primary-constructor injection (deps on the class declaration, not a + # ctor member) must be read too. + echo "$out" | grep -q "singleton 'PrimaryCtorService' captures scoped service 'AppDbContext'" \ + || { echo "FAIL: expected the primary-constructor captive (PrimaryCtorService -> AppDbContext)"; exit 1; } + echo "$out" | grep -q "DiCaptiveSample.cs" \ + || { echo "FAIL: expected the DI001 findings at the DiCaptiveSample.cs registration site"; exit 1; } + # NOT captive: singleton->singleton (Metrics->Clock), and PublicCtorOnly — + # DI resolves its public parameterless ctor, so the wider PRIVATE ctor's + # scoped dependency is never used. None of these may be flagged. + if echo "$out" | grep -qE "captures scoped service '(Clock|Metrics)'" \ + || echo "$out" | grep -q "'PublicCtorOnly'"; then + echo "FAIL: a singleton->singleton, public-ctor-only, or clean registration was wrongly flagged captive"; exit 1 + fi + # exactly four captive dependencies (direct + transitive + interface + primary-ctor). + nd=$(echo "$out" | grep -cE "DiCaptiveSample\.cs:[0-9]+:.*\[DI001\]") + [ "$nd" = "4" ] \ + || { echo "FAIL: expected exactly 4 DI001 captive findings, got $nd"; exit 1; } + echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) + DI001 (captive dependency) at the C# location" - name: Flow-sensitive local IDisposables (--flow-locals, P-016 B0b/B2) run: | # Path-sensitive flow analysis of local IDisposables — bugs the flat D1 diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 8f965336..00ddc3b4 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -97,8 +97,8 @@ architectural strictness, and the borrow-checker showcase): 1. `WPF001` — event/subscription `+=` without `-=` (the WPF spike; P-001 v0 ✅) 2. `WPF002` — `DispatcherTimer`/`Timer` `Tick`/`Elapsed` without stop/detach ✅ 3. `OWN001` — `IDisposable` field the class `new`s but never disposes ✅ -4. `DI001` — singleton captures a scoped dependency ✅ (core check built; - C# registration-graph extractor pending) +4. `DI001` — singleton captures a scoped dependency ✅ (core check + C# + registration-graph extractor built; end-to-end on real C#) 5. `POOL001` — `ArrayPool` buffer `Rent`ed but never `Return`ed ✅ (`POOL002` `Span`/view used after `Return` next) @@ -142,6 +142,12 @@ architectural strictness, and the borrow-checker showcase): migrating the *injected*-source subscription tier (today an honest OWN001 warning) once lifetime modelling can prove or refute those sources. 3. **DI lifetimes** — registration + constructor graph; captive dependency (P-006). + ◑ *In progress* — DI001 lands end to end: the C# extractor builds the + registration + constructor graph from `Add{Singleton,Scoped,Transient}` (generic + and `typeof(...)` forms) and the core flags the captive — direct, transitive + through a transient, or through an interface registration — CI-validated on + `DiCaptiveSample.cs`. Remaining: DI002 (weak-ref), DI003 (transient-`IDisposable` + from root), and the consuming-constructor anchor. 4. **Pool/Span** — `Rent`/`Return`, borrowed views, return-invalidates-views, known-bug replay corpus (P-007). The borrow checker on stage at full height. 5. **Effects** — `pure` / `use !Db` / `use !Log` / `use Clock`, layer policies @@ -211,7 +217,7 @@ own scan. Label them as estimates wherever they appear. | [P-003](proposals/P-003-lifetime-visualization.md) | Lifetime visualization (RustOwl-style) | horizon | draft | | [P-004](proposals/P-004-wpf-lifetime-profile.md) | WPF / UI lifetime leak profile | P0 | in progress (WPF001–005 built) | | [P-005](proposals/P-005-idisposable-ownership.md) | `IDisposable` ownership profile | P0 | draft | -| [P-006](proposals/P-006-di-lifetimes.md) | DI lifetime / captive dependency | P0 | in progress (DI001 core check built) | +| [P-006](proposals/P-006-di-lifetimes.md) | DI lifetime / captive dependency | P0 | in progress (DI001 end-to-end: core + extractor) | | [P-007](proposals/P-007-arraypool-span.md) | ArrayPool / Span borrow-view | P1 | draft | | [P-008](proposals/P-008-effects-and-resources.md) | Effects & resources (`Own.Effects`) | P1/P2 | draft | | [P-009](proposals/P-009-nogc-regions.md) | No-GC / allocation-free regions | horizon | draft | diff --git a/docs/notes/di-captive-extractor.md b/docs/notes/di-captive-extractor.md new file mode 100644 index 00000000..08d3aae0 --- /dev/null +++ b/docs/notes/di-captive-extractor.md @@ -0,0 +1,94 @@ +# DI001 end-to-end — the captive-dependency extractor (P-006) + +The DI001 captive-dependency check has lived in the core (`ownlang/di.py`) for a +while, validated only on hand-written `services` facts. This slice gives it a +**C# front end**: the Roslyn extractor now builds the registration + constructor +graph from real code, so **DI001 fires end-to-end on C#** — no hand-authored +facts. + +## The bug it catches + +A **singleton** that depends on a **scoped** service captures that scoped instance +for the whole application lifetime — the classic ASP.NET Core *"Cannot consume +scoped service from singleton"* bug (a `DbContext` held by a singleton is the +canonical case: an open connection / request state promoted to process lifetime). +It is a deterministic property of the **registration graph**, which is exactly +OwnLang's lifetime ordering spelled in DI terms (`Transient ≲ Scoped < Singleton`). + +The core rule (`di.py`, unchanged): + +- `singleton -> scoped` — captive (the edge is the bug); +- `singleton -> transient -> scoped` — captive (a transient resolved by a + singleton is itself singleton-lived, and drags the scoped along); +- `singleton -> singleton -> scoped` — **not** reported here (the inner singleton + is the captor and is flagged on its own pass). + +## What the extractor now reads + +A purely **syntactic** pass over the same parsed trees (no SemanticModel needed — +in the spirit of the narrow frontend), in two steps: + +1. **Constructor graph** — every class's widest **public** constructor's parameter + types (the dependency edges), including a C# 12 **primary constructor** + (`class Foo(Dep d)`, whose parameters live on the declaration, not a ctor + member). DI's default provider resolves through public ctors only, so a wider + non-public ctor's parameters are deliberately not counted (no false captive). +2. **Registrations** — every conventional `IServiceCollection` call: + `AddSingleton` / `AddScoped` / `AddTransient`, in the generic + `Add*` form or the `Add*(typeof(TService)[, typeof(TImpl)])` + form. + +Each registration emits one `services` fact — the shape the core already consumes: + +```json +{"name": "EmailSender", "lifetime": "singleton", "deps": ["AppDbContext"], + "file": "DiCaptiveSample.cs", "line": 40} +``` + +`name` is the **service** type others inject (so an `AddScoped` keys +under `IRepo`, and a consumer injecting `IRepo` resolves to it); `deps` are the +**implementation**'s constructor parameter types; `file`/`line` anchor the finding +at the registration site. + +### Non-goals stay silent, never guessed + +Factory lambdas, reflection/assembly scanning, Scrutor, open generics and +config-driven wiring defeat a static graph. The extractor records what it *can* +read and treats the rest as an **unknown-dep node** (`deps: []`) — a node others +can still capture, but one that contributes no edges of its own. Per the P-006 +non-goals: report the conventional shape, stay silent (not wrong) on the rest. + +## Validation + +- **Bridge-side, locally** — the `services` facts the extractor emits, fed through + the real core, produce exactly the four expected DI001s (direct, transitive, + interface, primary-ctor) and stay silent on the singleton→singleton edge and the + public-ctor-only service. (No local .NET SDK, so the C#→facts step itself is + validated in CI, like the rest of the frontend.) +- **End-to-end, in CI** — `frontend/roslyn/samples/DiCaptiveSample.cs` is wired + into the `wpf-extractor` job. The assertions pin: a direct capture + (`EmailSender -> AppDbContext`), a transitive one through a transient + (`ReportService -> UnitOfWork -> AppDbContext`), an interface-registration one + (`CacheService -> IRepo`), a C# 12 primary-constructor one + (`PrimaryCtorService -> AppDbContext`), **exactly four** findings, and silence on + `Metrics -> Clock` (singleton→singleton) and `PublicCtorOnly` (DI uses its public + parameterless ctor; the wider private ctor's scoped dep is never resolved). + +## Why it matters for differentiation + +The captive dependency is an **ASP.NET-specific lifetime contract** that +general-purpose analyzers don't model — the same complementary story as the +subscription-leak class the cross-tool oracle already documented (CodeQL and +Infer# cover the Dispose/RAII family; neither flags these). It is also a clean +reuse win: a whole new diagnostic class on real C# with **zero core changes** — +the frontend just produces the `services` facts the lifetime core already checks. + +## Next (separate slices) + +- **DI002** — a singleton capturing a scoped dependency *weakly* + (`WeakReference`): a warning, since a weak reference fixes retention but + not the lifetime-contract violation. +- **DI003** — a transient `IDisposable` resolved from the **root** provider, never + disposed until the app exits (a slow leak). +- Anchoring the finding at the **consuming constructor** as well as the + registration site (P-006 open question #1), with the capture path shown. diff --git a/docs/proposals/P-006-di-lifetimes.md b/docs/proposals/P-006-di-lifetimes.md index 106fd596..73e33fd6 100644 --- a/docs/proposals/P-006-di-lifetimes.md +++ b/docs/proposals/P-006-di-lifetimes.md @@ -2,10 +2,15 @@ - **Status:** in progress (P0 — clean lifetime model, little R&D, sells to ASP.NET). DI001 captive-dependency check built in the core (`ownlang/di.py`) - over an OwnIR `services` registration graph, surfaced through the bridge with - hand-written facts + tests. Next: the C# extractor that builds the registration - graph from `services.Add{Singleton,Scoped,Transient}` + constructor injection - (CI-only, like the rest of the extractor). + over an OwnIR `services` registration graph, surfaced through the bridge. The + C# extractor now **builds that graph from real code** — `services.Add{Singleton, + Scoped,Transient}` (the generic `` and `typeof(...)` forms) + plus each implementation's constructor parameters — so **DI001 fires end-to-end + on C#**, CI-validated on `frontend/roslyn/samples/DiCaptiveSample.cs` (direct, + transitive-via-transient, and interface-registration captures flagged; + singleton→singleton and clean registrations silent). See + [docs/notes/di-captive-extractor.md](../notes/di-captive-extractor.md). Next: + DI002 (weak-ref) and DI003 (transient-`IDisposable`-from-root). - **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). diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 595ff926..ad400545 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -718,6 +718,143 @@ t is "IDisposable" or "IAsyncDisposable" or "CancellationTokenSource" || t.EndsWith("Stream") || t.EndsWith("Reader") || t.EndsWith("Writer") || t.EndsWith("Subscription"); +// --- P-006: DI registration + constructor graph (DI001 captive dependency) --- +// A syntactic pass over the same trees, independent of the event/disposable +// detectors: collect each class's constructor parameter types (the dependency +// graph), then each conventional IServiceCollection registration +// (Add{Singleton,Scoped,Transient}, the generic `` form or the +// `typeof(...)` form) and emit `services` facts {name, lifetime, deps, file, line} +// that ownlang/di.py checks for captive dependencies. The registration's `name` +// is the SERVICE type others inject; `deps` are the IMPLEMENTATION's constructor +// parameter types. Factory/reflection/open-generic shapes we cannot read are +// recorded as unknown-dep nodes (deps: []) — silent, never guessed (P-006 scope). +static List ExtractServices(List<(string file, SyntaxTree tree)> parsed) +{ + // 1. class name -> its widest constructor's parameter type names (the DI ctor). + var ctorDeps = new Dictionary>(); + foreach (var (_, tree) in parsed) + foreach (var node in tree.GetRoot().DescendantNodes()) + { + if (node is not ClassDeclarationSyntax cls) + continue; + // Candidate parameter lists: the C# 12 primary constructor (on the + // class declaration itself) plus the PUBLIC explicit constructors — + // take the widest. Primary-constructor injection (`class Foo(Dep d)`) + // has no ConstructorDeclarationSyntax member, so it must be read off + // the declaration or DI001 misses modern .NET 8 services; and the + // default IServiceProvider only uses public constructors, so a wider + // non-public ctor's parameters must not count as real deps (Codex). + ParameterListSyntax? widest = cls.ParameterList; + foreach (var m in cls.Members) + if (m is ConstructorDeclarationSyntax ctor + && IsPublicCtor(ctor.Modifiers) + && (widest is null + || ctor.ParameterList.Parameters.Count > widest.Parameters.Count)) + widest = ctor.ParameterList; + var deps = new List(); + if (widest is not null) + foreach (var p in widest.Parameters) + { + var tn = p.Type is null ? null : DiTypeName(p.Type); + if (tn is not null) + deps.Add(tn); + } + ctorDeps[cls.Identifier.Text] = deps; // last decl wins (core dedups by name) + } + + // 2. registrations -> service facts at the registration site. + var services = new List(); + foreach (var (file, tree) in parsed) + foreach (var node in tree.GetRoot().DescendantNodes()) + { + if (node is not InvocationExpressionSyntax inv + || inv.Expression is not MemberAccessExpressionSyntax ma) + continue; + var lifetime = RegistrationLifetime(ma.Name.Identifier.Text); + if (lifetime is null) + continue; + ResolveRegistration(ma.Name, inv.ArgumentList, out var service, out var impl); + if (service is null) // not a conventional typed registration -> skip + continue; + var deps = impl is not null && ctorDeps.TryGetValue(impl, out var d) + ? d : new List(); + services.Add(new + { + name = service, + lifetime, + deps, + file, + line = node.GetLocation().GetLineSpan().StartLinePosition.Line + 1, + }); + } + return services; +} + +// The DI lifetime for an IServiceCollection registration method, or null when the +// method name is not one of the three conventional registrations. +static string? RegistrationLifetime(string method) => method switch +{ + "AddSingleton" => "singleton", + "AddScoped" => "scoped", + "AddTransient" => "transient", + _ => null, +}; + +// Service + implementation type for a registration, from the generic type +// arguments (`Add*`) or the `typeof(...)` arguments +// (`Add*(typeof(TService)[, typeof(TImpl)])`). A null service means it is not a +// typed registration we can read (a bare instance / factory) -> the caller skips. +static void ResolveRegistration(SimpleNameSyntax name, ArgumentListSyntax args, + out string? service, out string? impl) +{ + service = null; + impl = null; + if (name is GenericNameSyntax gen) + { + var targs = gen.TypeArgumentList.Arguments; + if (targs.Count >= 1) + service = DiTypeName(targs[0]); + impl = targs.Count >= 2 ? DiTypeName(targs[1]) : service; + return; + } + string? first = null, second = null; + var seen = 0; + foreach (var a in args.Arguments) + if (a.Expression is TypeOfExpressionSyntax tof) + { + if (seen == 0) first = DiTypeName(tof.Type); + else if (seen == 1) second = DiTypeName(tof.Type); + seen++; + } + if (first is not null) + { + service = first; + impl = second ?? first; + } +} + +// The simple (rightmost) name of a type, for matching a registration's service +// type to a constructor parameter's type. A generic name drops its arguments +// (`ILogger` -> `ILogger`); a predefined type (`int`) yields null. +static string? DiTypeName(TypeSyntax t) => t switch +{ + IdentifierNameSyntax id => id.Identifier.Text, + GenericNameSyntax g => g.Identifier.Text, + QualifiedNameSyntax q => DiTypeName(q.Right), + AliasQualifiedNameSyntax aq => DiTypeName(aq.Name), + _ => null, +}; + +// 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) +{ + foreach (var m in modifiers) + if (m.IsKind(SyntaxKind.PublicKeyword)) + return true; + return false; +} + var components = new List(); // P-016 B0b/B2: per-method flow bodies (only when --flow-locals). var flowFunctions = new List(); @@ -1141,6 +1278,9 @@ or ImplicitObjectCreationExpressionSyntax } init ownir_version = 0, module = "Extracted", components, + // P-006: the DI registration + ctor graph (empty when the scan has no + // Add{Singleton,Scoped,Transient} calls). ownlang/di.py turns it into DI001. + services = ExtractServices(parsed), functions = flowFunctions, stats = new { diff --git a/frontend/roslyn/samples/DiCaptiveSample.cs b/frontend/roslyn/samples/DiCaptiveSample.cs new file mode 100644 index 00000000..2323bb8f --- /dev/null +++ b/frontend/roslyn/samples/DiCaptiveSample.cs @@ -0,0 +1,88 @@ +using System; + +// P-006 — DI captive-dependency classifier (DI001). A *singleton* that depends — +// directly, transitively through a *transient*, or through an interface +// registration — on a *scoped* service captures that scoped instance for the +// whole application lifetime (the classic ASP.NET Core "Cannot consume scoped +// service from singleton" bug). The extractor reads the conventional +// `IServiceCollection` registration graph (`Add{Singleton,Scoped,Transient}`, the +// generic `` form and the `typeof(...)` form) plus each +// implementation's constructor parameters, and ownlang/di.py flags the capture at +// the registration site. A singleton->singleton edge and the clean registrations +// stay silent. The extraction is purely syntactic, so the DI surface need not be +// the real Microsoft.Extensions.DependencyInjection — the calls only have to +// parse. See docs/proposals/P-006-di-lifetimes.md. + +namespace Sample +{ + public sealed class AppDbContext { } // scoped + public sealed class Clock { } // singleton, no deps + + public interface IRepo { } + public sealed class Repo : IRepo { public Repo(AppDbContext db) { } } // scoped impl + + // singleton captors: + public sealed class EmailSender { public EmailSender(AppDbContext db) { } } // -> scoped (direct) + public sealed class UnitOfWork { public UnitOfWork(AppDbContext db) { } } // transient + public sealed class ReportService { public ReportService(UnitOfWork uow) { } } // -> transient -> scoped + public sealed class CacheService { public CacheService(IRepo repo) { } } // -> IRepo (scoped) + public sealed class Metrics { public Metrics(Clock clock) { } } // -> singleton (clean) + + // primary-constructor (C# 12) injection — the dependency is on the class + // declaration, not a ConstructorDeclarationSyntax member. + public sealed class PrimaryCtorService(AppDbContext db) // -> scoped (direct, primary ctor) + { public AppDbContext Db { get; } = db; } + + // DI's default provider uses the PUBLIC ctor (no deps); the wider PRIVATE ctor's + // scoped dependency is never resolved, so it must not be a captive. + public sealed class PublicCtorOnly + { + public PublicCtorOnly() { } + private PublicCtorOnly(AppDbContext db) { } + } + + public static class Startup + { + public static void ConfigureServices(IServiceCollection services) + { + services.AddScoped(); // scoped leaf + services.AddSingleton(); // SILENT — singleton, no deps + services.AddScoped(); // interface -> impl, scoped + + // FLAGGED — singleton captures a scoped service directly. + services.AddSingleton(); + + services.AddTransient(); // transient + + // FLAGGED — transitive: singleton -> transient UnitOfWork -> scoped AppDbContext. + services.AddSingleton(); + + // FLAGGED — through the interface registration: singleton -> IRepo (scoped). + services.AddSingleton(); + + // FLAGGED — primary-constructor injection: singleton -> scoped AppDbContext. + services.AddSingleton(); + + // SILENT — DI resolves the public parameterless ctor; the private ctor's + // scoped dependency is never used (no false captive). + services.AddSingleton(); + + // SILENT — singleton -> singleton (Clock); the typeof(...) registration form. + services.AddSingleton(typeof(Metrics), typeof(Metrics)); + } + } + + // Minimal in-sample stand-ins so the registrations resolve cleanly (no real + // Microsoft.Extensions.DependencyInjection dependency). The extractor matches + // the calls syntactically, so these only need to parse/bind. + public interface IServiceCollection { } + + public static class ServiceCollectionExtensions + { + public static IServiceCollection AddSingleton(this IServiceCollection s) => s; + public static IServiceCollection AddScoped(this IServiceCollection s) => s; + public static IServiceCollection AddScoped(this IServiceCollection s) => s; + public static IServiceCollection AddTransient(this IServiceCollection s) => s; + public static IServiceCollection AddSingleton(this IServiceCollection s, Type service, Type impl) => s; + } +}