-
Notifications
You must be signed in to change notification settings - Fork 0
feat(di): DI005 (scope-factory misuse) + first real-world captive corpus case #107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
e967309
docs: correct stale DI roadmap status (DI002–DI004 shipped, not remai…
claude 8c5ad57
feat(di): first real-world captive-dependency corpus case (DI001)
claude 04a8a92
feat(di): DI005 — scoped service cached from a created scope (the fix…
claude 02d9192
fix(di): DI005 follows the transient graph + review hardening (#107)
claude 7c39962
fix(di): DI005 scope-cache site uses the assignment's own file (CodeR…
claude d73c33a
docs(test): narrow DI005 test header for the transitive case (CodeRab…
claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| // AFTER (fixed). The standard remedy for a singleton that needs a scoped service: | ||
| // inject `IServiceScopeFactory` (a singleton itself) instead of the scoped | ||
| // `AppDbContext`, and open a fresh scope per operation — `using var scope = | ||
| // _scopes.CreateScope();` — resolving the DbContext inside it so it lives and is | ||
| // disposed within that operation. The singleton's constructor no longer depends on | ||
| // a scoped service, so there is no captive edge in the registration graph (DI001 | ||
| // silent), and the resolve is off the scope's provider (not an injected root | ||
| // `IServiceProvider`), so the service-locator rule (DI004) stays silent too. | ||
| using System; | ||
|
|
||
| namespace Corpus | ||
| { | ||
| public sealed class AppDbContext { } | ||
|
|
||
| public sealed class NotificationService | ||
| { | ||
| private readonly IServiceScopeFactory _scopes; | ||
| public NotificationService(IServiceScopeFactory scopes) { _scopes = scopes; } // no scoped captured | ||
|
|
||
| public void Notify() | ||
| { | ||
| using var scope = _scopes.CreateScope(); | ||
| var db = scope.ServiceProvider.GetRequiredService<AppDbContext>(); // per-operation scope | ||
| // ... use db within the scope ... | ||
| } | ||
| } | ||
|
|
||
| public static class Startup | ||
| { | ||
| public static void ConfigureServices(IServiceCollection services) | ||
| { | ||
| services.AddScoped<AppDbContext>(); | ||
| services.AddSingleton<NotificationService>(); // SILENT — injects IServiceScopeFactory, not the scoped service | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| // BEFORE (buggy). The canonical ASP.NET Core captive dependency (P-006 DI001): | ||
| // a SINGLETON service takes a SCOPED EF Core `DbContext` in its constructor, so | ||
| // the container builds one `AppDbContext` with the singleton and holds it for the | ||
| // whole application lifetime — an open DB connection pinned for the process, and | ||
| // request state shared across requests. Microsoft calls this "Cannot consume | ||
| // scoped service 'AppDbContext' from singleton 'NotificationService'." The | ||
| // extractor reads the conventional `IServiceCollection` registration graph | ||
| // (`Add{Singleton,Scoped}`) plus each implementation's constructor parameters, and | ||
| // ownlang/di.py flags the capture at the registration site, naming the consuming | ||
| // constructor. Representative of the pattern (a singleton background/notification | ||
| // service injecting a scoped DbContext), not verbatim from one project. The fix is | ||
| // a scope boundary — inject `IServiceScopeFactory` and resolve per operation (see | ||
| // after.cs). | ||
| using System; | ||
|
|
||
| namespace Corpus | ||
| { | ||
| public sealed class AppDbContext { } // scoped (an EF Core DbContext is scoped) | ||
|
|
||
| // registered as a SINGLETON below, but it captures the scoped DbContext: | ||
| public sealed class NotificationService | ||
| { | ||
| public NotificationService(AppDbContext db) { } // <-- captures scoped (DI001) | ||
| } | ||
|
|
||
| public static class Startup | ||
| { | ||
| public static void ConfigureServices(IServiceCollection services) | ||
| { | ||
| services.AddScoped<AppDbContext>(); // scoped | ||
| services.AddSingleton<NotificationService>(); // FLAGGED: singleton -> scoped (DI001) | ||
| } | ||
| } | ||
| } |
1 change: 1 addition & 0 deletions
1
corpus/di/singleton-captures-scoped-dbcontext/expected-diagnostics.txt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| DI001 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| # Singleton captures a scoped DbContext (DI001) | ||
|
|
||
| **Pattern:** the canonical ASP.NET Core *captive dependency*. A service registered | ||
| `AddSingleton` takes a `AddScoped` service (here an EF Core `AppDbContext`) in its | ||
| constructor. The container builds **one** scoped instance with the singleton and | ||
| holds it for the whole application lifetime — a DB connection pinned for the | ||
| process, request-specific state shared across requests, and a `DbContext` used | ||
| concurrently from multiple threads (it is not thread-safe). Microsoft surfaces it | ||
| at startup as *"Cannot consume scoped service 'AppDbContext' from singleton | ||
| 'NotificationService'."* | ||
|
|
||
| **Why it is exactly OwnLang's lifetime model.** A captive dependency *is* the | ||
| OWN014 region-escape rule in DI clothing: `Scoped < Singleton` (request < app), and | ||
| storing a shorter-lived value into a longer-lived owner is the violation. The core | ||
| runs the same lifetime ordering it uses for OWN014; `ownlang/di.py` | ||
| (`find_captive_dependencies`) walks the registration + constructor graph and flags | ||
| the capture **at the registration site**, naming the consuming constructor. | ||
|
|
||
| **The fix (after.cs).** Inject `IServiceScopeFactory` (a singleton) instead of the | ||
| scoped service, and open a fresh scope per operation | ||
| (`using var scope = _scopes.CreateScope();`), resolving the `DbContext` inside it. | ||
| The singleton's constructor no longer depends on a scoped service, so the captive | ||
| edge is gone (DI001 silent), and the resolve is off the scope's provider rather than | ||
| an injected root `IServiceProvider`, so the service-locator rule (DI004) is silent | ||
| too. | ||
|
|
||
| **Honesty / scope.** This is the DI family's first **real-world** corpus case; the | ||
| captive classifier was previously pinned only on the synthetic | ||
| `frontend/roslyn/samples/DiCaptiveSample.cs`. There is **no `case.own`**: the `.own` | ||
| DSL has no service-registration surface (DI lives in the `services` fact graph, not | ||
| the resource/flow language), so the captive cannot be hand-reduced to `.own` the way | ||
| an ownership bug can — `corpus/di/` is therefore scanned by the **dotnet | ||
| `corpus-benchmark` job only** (extractor → `services` graph → DI001), not the | ||
| Python `test_corpus` `.own` runner. `before.cs` / `after.cs` are representative of | ||
| the pattern, not a verbatim diff. The transitive, interface-registration, weak | ||
| (`DI002`), transient-`IDisposable` (`DI003`), and service-locator (`DI004`) variants | ||
| remain pinned on the synthetic sample. | ||
|
|
||
| Reference: [P-006](../../../docs/proposals/P-006-di-lifetimes.md); Microsoft "DI | ||
| guidelines — scoped service as singleton" (the captive-dependency anti-pattern). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.