diff --git a/docs/adr/0024-public-provider-extensibility-model.md b/docs/adr/0024-public-provider-extensibility-model.md new file mode 100644 index 0000000..8b60582 --- /dev/null +++ b/docs/adr/0024-public-provider-extensibility-model.md @@ -0,0 +1,708 @@ +# [ADR-0024] Public Provider Extensibility Model + +**Status:** Accepted + +**Date:** 2026-07-31 + +**Decision Makers:** Nick Cipollina, Claude (design review) + +## Context + +`docs/architecture.md`'s Resolution Pipeline reserves stage 5 (semantic value +providers) and stage 6 (test-double providers) for exactly this purpose, and both +have sat empty since Milestone 2 — wired end-to-end in `CompositionContext` +(`_semanticProviders`/`_testDoubleProviders` fields, `TryProviders` dispatch, +`PipelineStage.SemanticProvider`/`TestDoubleProvider` trace entries) but never fed +by anything, because nothing outside `Compono` itself has ever needed to populate +them. `docs/mvp.md`'s Milestone 3 section and `docs/architecture.md`'s own Open +Architectural Decisions list both record the same explicit deferral: Milestone 3's +own scope (registrations, profiles, type/member rules) needs no public provider +contract at all — [ADR-0020](0020-composition-configuration-rules.md) compiles +`.For()` rules into small, *internal*, Compono-authored `ICompositionProvider` +implementations, so a user never implements that interface directly. Milestone 5 +(`Compono.NSubstitute`) is the first real consumer that needs open-ended, +pattern-matching logic a closed-set rule can't express ("any interface type," "any +unsealed abstract class") — the deferred question this ADR now settles. + +The engine's own `ICompositionProvider` (`src/Compono/ICompositionProvider.cs`) is +deliberately `internal`: its `TryCompose(in CompositionRequest, ICompositionContext)` +signature takes the engine's internal `CompositionRequest` (a path, an `IsShared` +flag, and other pipeline plumbing no integration author should need to know about +or could construct correctly without reflection). Making that interface `public` +outright — the most literal reading of "add a provider extension point" — would +either force `CompositionRequest`/`CompositionPath`/`PathSegment` to become public +too (a large, ongoing-maintenance-cost surface, and exactly the "avoid exposing +source-generator/engine implementation details" rule `docs/public-api.md`'s API +Design Rules already state), or leave external implementors constructing a request +some other, undocumented way. This ADR designs a smaller, decoupled public contract +instead — mirroring the split [ADR-0010](0010-composition-request-pipeline-and-diagnostics-tracing.md) +already established between the public, compact `CompositionRequestDescriptor` +generated code passes and the internal, path-expanded `CompositionRequest` the +engine actually operates on. + +This ADR is scoped to the **mechanism** — the public contract, registration +surface, stage placement, ordering, diagnostics identity, and failure semantics +shared by any future stage-5/6 integration. [ADR-0025](0025-compono-nsubstitute-package-design.md) +is the first real consumer built on top of it (`Compono.NSubstitute`'s own +substitution rules, options, and package-specific diagnostics). Splitting the two +mirrors [ADR-0021](0021-row-composition-entry-point-for-test-framework-integrations.md)/ +[ADR-0022](0022-compono-xunit-package-design.md)'s precedent: a core-engine-extension +ADR, and a package-design ADR that consumes it. + +## Decision Drivers + +- `design-decisions.md` rule 3: the core `Compono` package must never reference or + know about an integration package. The contract this ADR designs has to be + something `Compono.NSubstitute` (and, unbuilt but validated against here, + `Compono.Bogus`) can implement **from the outside**, with zero core-to-integration + reference in either direction beyond the ordinary integration-depends-on-core + arrow already established. +- `docs/architecture.md`'s Resolution Pipeline: stage order is fixed product + contract, "not configurable — not by users or by providers reordering + themselves." Any design that lets a provider see or influence "what happens + next" beyond its own `NotHandled`/`Handled` answer (an AutoFixture-`ISpecimenBuilder`- + style recursive "keep asking further builders" hook, or an ASP.NET-middleware-style + `next()` delegate) would hand a provider implicit control over pipeline ordering — + explicitly out of bounds per the task that produced this ADR ("avoid designing + something that resembles AutoFixture's `ISpecimenBuilder` or a general middleware + pipeline unless those designs prove objectively superior" — no such proof arose + during this design). +- `docs/public-api.md`'s API Design Rules: minimal public surface, one obvious way + to do a thing, avoid exposing engine internals. The smallest contract that lets + NSubstitute (and, by construction, Bogus) express "any interface type"/"any + member named `Email`" pattern-matching is the actual target, not the largest one + that could theoretically be useful. +- Diagnostics (`docs/architecture.md`'s Diagnostics section, `ProviderAttempt.Provider` + per [ADR-0016](0016-provider-identity-restored-in-provider-attempt.md)): a + composition failure must still be able to name the concrete provider type that + was tried, exactly as it does for stage 4/7's internal providers today — this + can't regress just because the provider now originates outside the assembly. +- `ADR-0001`'s no-reflection-by-default rule governs `Compono` core's own + construction path; it does not (and per this design task's own framing, should + not) reach into what a third-party library like NSubstitute does internally to + do its own job. The core contract itself must stay reflection-free to implement + and to invoke; what a specific provider implementation does inside its own + `TryProvide` body is that provider's own concern. +- Stateless-provider expectation: `Composer`/`CompositionConfiguration` are + immutable and reused across every `Create()`/`CreateMany()`/`CreateRow` + call a composer ever serves, including concurrent calls (xUnit v3 supports + parallel test execution). A provider instance is constructed once and invoked + repeatedly, potentially concurrently — its `TryProvide` must not depend on or + mutate per-call instance state. + +## Considered Options + +### Provider contract shape + +1. **A new, minimal public interface** (`ICompositionValueProvider`) with its own + public request/result value types, decoupled from the engine's internal + `ICompositionProvider`/`CompositionRequest`/`CompositionResult` — compiled, at + `CompositionBuilder.Build()` time, into a small internal adapter registered into + stage 5/6's existing `ICompositionProvider` list, the same "public data compiles + into an internal provider" shape [ADR-0020](0020-composition-configuration-rules.md) + already established for `.For()` rules. +2. **Delegate-based registration only** — `builder.AddTestDoubleProvider(Func tryProvide)`, + no interface at all. +3. **Make the engine's own `ICompositionProvider` (and enough of `CompositionRequest`/ + `CompositionResult` to support it) public directly.** +4. **A middleware/chain-of-responsibility contract** — `TryProvide(request, context, next)`, + where a provider can call `next()` to continue past itself, AutoFixture-`ISpecimenBuilder`-style. + +### Stage placement and registration + +1. **One shared interface, two stage-scoped registration methods** — + `ICompositionValueProvider` is not stage-specific; `builder.AddSemanticProvider(provider)` + and `builder.AddTestDoubleProvider(provider)` are what decide which pipeline + stage (5 or 6) a given instance lands in. An integration's own `UseX()` extension + method calls whichever registration method matches its own package's purpose. +2. **Two marker sub-interfaces** — `ISemanticValueProvider : ICompositionValueProvider`, + `ITestDoubleProvider : ICompositionValueProvider`, with a single + `builder.AddProvider(...)` overloaded/dispatched by static type. + +### Diagnostic identity through the adapter + +1. **Add a `ProviderType` member to the internal `ICompositionProvider` interface**, + default-implemented as `=> GetType()`; the internal adapter wrapping a public + provider overrides it to report the wrapped instance's own concrete type. Every + trace/diagnostic call site that currently reads `candidate.GetType()` reads + `candidate.ProviderType` instead. +2. **Leave `ProviderAttempt.Provider` reporting the adapter's own type** (e.g. a + generic `PublicProviderAdapter`) for every public-provider attempt, accepting + the diagnostic regression. + +## Decision Outcome + +**Chosen: Option 1 for the contract shape, Option 1 for stage placement, Option 1 +for diagnostic identity** — confirmed directly with the user (a genuine fork +between a named public interface and delegate-based registration was raised and +resolved in favor of the interface, for exactly the reasons Option 2's Pros/Cons +below state). + +**What this ADR actually commits to, versus what's shown for illustration.** +Per this ADR's own Alpha Compatibility Policy (below) and explicit user direction +during review: `Compono` is intentionally alpha until the MVP milestones +(`docs/mvp.md`) complete, and this repo already has a process for revising a +shipped public contract when real evidence calls for it (see that section). This +ADR should not — and does not — over-design the provider contract to protect +against every hypothetical future metadata need. The architectural guarantees this +ADR actually commits to are: + +- A **named public interface** is the authoring shape for a stage-5/6 provider + (not a bare delegate). +- A **small request/result contract**, decoupled from the engine's own internal + request/path types — a provider author never sees `CompositionRequest`, + `CompositionPath`, or `PathSegment`. +- **Deterministic stage placement and registration order** — which stage (5 or 6) + a provider participates in is explicit at registration time, and multiple + providers in the same stage are tried in registration order. +- **Isolation from internal engine request/path types** — the public contract + never leaks an internal engine type into a provider author's code. +- **Diagnostics identify the logical provider** that produced or attempted a + result, not an opaque wrapper/adapter type. +- **Immutable, reusable provider registration** — a provider is constructed once + at configuration time and safe to invoke repeatedly (including concurrently) + for the lifetime of the `Composer` it's registered on. +- **`NotHandled` versus `Handled` semantics** — a provider can decline or produce + a value; it cannot report a stronger "failure" the way a context-owned + authoritative stage can. + +Everything else in this ADR's code samples below — the exact type names +`CompositionProviderRequest`/`CompositionProviderResult`, the internal +`PublicProviderAdapter` class name, the `ProviderType` default-interface-member +mechanism, `AddSemanticProvider`/`AddTestDoubleProvider` as two methods rather than +one overloaded/stage-inferred method, and every other dispatch micro-detail — is an +**illustrative implementation choice**, useful to prove the design is buildable and +to give `implement.md` a concrete starting point, not a separate guarantee this ADR +locks in. Milestone 6 (Bogus) or Milestone 7 (dogfooding) surfacing a real need to +reshape any of it is expected, not a process failure — see the Alpha Compatibility +Policy below for how that gets handled if it happens. + +### The public contract + +```csharp +namespace Compono; + +/// +/// A composition request, as seen by a public — decoupled +/// from the engine's own internal request shape (no path, no shared-scope flag, no pipeline +/// plumbing a provider author has no legitimate use for). +/// +public readonly struct CompositionProviderRequest +{ + /// The requested CLR type. + public Type RequestedType { get; } + + /// + /// The type whose constructor/required member declares this request, or + /// for a request with no member identity (a collection element, a + /// manual resolve, or the composition root itself) — the same field/same semantics + /// CompositionRequestDescriptor.DeclaringType already carries, per + /// docs/adr/0020-composition-configuration-rules.md. + /// + public Type? DeclaringType { get; } + + /// + /// The declaring constructor parameter/required member/test-method-parameter's own name, for + /// diagnostic display and name-based provider matching (e.g. a future Compono.Bogus + /// member-name convention) — for a request with no name of its own + /// (a collection element, dictionary key/value, or manual resolve). + /// + public string? Name { get; } + + /// Whether the requesting parameter or member is nullable-annotated. + public Nullability Nullability { get; } +} + +/// +/// What an reports for one . +/// +/// +/// Deliberately only two cases, mirroring the engine's own internal CompositionResult +/// contract for ordinary providers: a public provider can report that it doesn't apply, or that it +/// produced a value — never a stronger "failure." An unhandled exception a provider's own +/// implementation throws propagates uncaught, +/// exactly like an internal stage-4/7 provider's exception does today — see this ADR's Provider +/// Failure Semantics section. +/// +public readonly struct CompositionProviderResult +{ + /// The provider does not handle this request. + public static CompositionProviderResult NotHandled => default; + + /// The provider produced for this request. + public static CompositionProviderResult Handled(object? value) => new(isHandled: true, value); + + // IsHandled/Value are internal - a public provider constructs a result only through the two + // factory members above, never by inspecting or round-tripping one it didn't just receive. + internal bool IsHandled { get; } + internal object? Value { get; } + + private CompositionProviderResult(bool isHandled, object? value) + { + IsHandled = isHandled; + Value = value; + } +} + +/// +/// A public extension point for pipeline stage 5 (semantic value providers) or stage 6 +/// (test-double providers) — open-ended, pattern-matching composition logic a closed-set +/// .For<T>() rule can't express ("any interface type," "any member named Email"). +/// Registered via or +/// — which method an integration's own +/// UseX() extension calls decides which stage a given instance participates in; the +/// interface itself is not stage-specific. +/// +/// +/// An implementation must be safe to invoke repeatedly, including concurrently, once constructed — +/// a 's configuration (and every provider registered into it) is immutable and +/// reused across every composition call it ever serves, exactly like every other builder-compiled +/// piece of configuration (a .For<T>() rule, a registration factory). +/// +public interface ICompositionValueProvider +{ + /// + /// Attempts to produce a value for . Returns + /// for any request this provider doesn't + /// apply to, so a later provider or pipeline stage still gets a chance — never throws for an + /// expected non-match. + /// + /// The request to attempt. + /// + /// The active composition context - a provider may call context.Resolve<T>() to + /// compose part of its value from a nested request, exactly as an internal provider already + /// may (docs/architecture.md's Providers section). + /// + CompositionProviderResult TryProvide(in CompositionProviderRequest request, ICompositionContext context); +} +``` + +### Registration and compilation + +```csharp +public sealed class CompositionBuilder +{ + public CompositionBuilder AddSemanticProvider(ICompositionValueProvider provider); + public CompositionBuilder AddTestDoubleProvider(ICompositionValueProvider provider); +} +``` + +Both accumulate into an ordered list (registration order — the same "provider order +within an extensible stage is registration order" rule stage 4/7 already follow; +no priority/specificity system, matching `docs/architecture.md`'s existing "no +richer ordering rule exists yet because none has been needed" reasoning applied to +a new stage rather than invented for one). At `Build()` time, each accumulated +`ICompositionValueProvider` is wrapped, in order, into an internal +`PublicProviderAdapter : ICompositionProvider` and placed into +`CompositionConfiguration.SemanticProviders`/`TestDoubleProviders` +(`IReadOnlyList`, mirroring the existing `Rules` field's +shape exactly). `Composer.Create`/`CreateMany`/`CreateRow` thread these two +new lists into `CompositionContext`'s already-existing `_semanticProviders`/ +`_testDoubleProviders` constructor parameters — both fields have been wired since +Milestone 2 and have simply never been fed anything until now; no pipeline-dispatch +code changes at all, only what's constructed and passed in. + +```csharp +internal sealed class PublicProviderAdapter : ICompositionProvider +{ + private readonly ICompositionValueProvider _inner; + + internal PublicProviderAdapter(ICompositionValueProvider inner) => _inner = inner; + + public Type ProviderType => _inner.GetType(); + + public CompositionResult TryCompose(in CompositionRequest request, ICompositionContext context) + { + var publicRequest = new CompositionProviderRequest( + request.RequestedType, request.DeclaringType, NameOf(request.Path.Segment), request.Nullability); + + var result = _inner.TryProvide(in publicRequest, context); + + return result.IsHandled + ? new CompositionResult.Success(result.Value) + : CompositionResult.NotHandled.Instance; + } +} +``` + +This is the exact shape [ADR-0020](0020-composition-configuration-rules.md) +already established for value rules ("compile into internal, Compono-authored +`ICompositionProvider` instances — a user never implements `ICompositionProvider` +directly"), applied to a second public entry point into the same stage-4-adjacent +compilation step, not a new mechanism. + +### Diagnostic identity: `ProviderType` on `ICompositionProvider` + +The internal `ICompositionProvider` interface gains one default-implemented member: + +```csharp +internal interface ICompositionProvider +{ + Type ProviderType => GetType(); + + CompositionResult TryCompose(in CompositionRequest request, ICompositionContext context); +} +``` + +Every existing internal provider (`PrimitiveValueProvider`, `TypeRuleProvider`, +etc.) gets the default `GetType()` behavior for free, unchanged from today. +`PublicProviderAdapter` is the only implementation that overrides it, reporting the +*wrapped* provider's own concrete type (e.g. `NSubstituteProvider`), not the +adapter's own type. `CompositionContext.TryProviders` (and the one other read site, +`InvokeFactory`'s `provider` parameter passthrough) read `candidate.ProviderType` +instead of `candidate.GetType()` — a one-line internal change per call site. +`ProviderAttempt.Provider` therefore continues to name the real, meaningful +provider type for a public-provider attempt exactly as it already does for stage +4/7's internal ones — no diagnostic regression, and no public API change +(`ProviderAttempt` itself, and `ICompositionProvider`, both stay exactly as +`public`/`internal` as they already are). + +### Provider Failure Semantics + +An `ICompositionValueProvider` can only report `NotHandled` or `Handled(value)` — +never a stronger "failure" result, matching every other ordinary provider in this +engine (`docs/architecture.md`'s Providers section: "Ordinary providers cannot +report Failure"). If a provider's own `TryProvide` implementation throws — a bug in +the provider, or a third-party library it wraps throwing internally for an +input it superficially claimed but couldn't actually satisfy — that exception +propagates uncaught out of `Create()`/`CreateMany()`/`CreateRow`'s call, +exactly as an internal stage-4/7 provider's own thrown exception already does today +(`CompositionContext.TryProviders` calls `candidate.TryCompose(...)` with no +surrounding `try`/`catch` at all). This is a deliberate consistency choice, not a +gap newly introduced by public providers: stage 3's exact-registration factories +are the one place a thrown exception is caught and converted into an authoritative, +diagnostic-carrying `CompositionException` +([ADR-0010](0010-composition-request-pipeline-and-diagnostics-tracing.md)) +precisely *because* stage 3 is a context-owned authoritative stage; stages 4-7 are +not, and have never wrapped provider exceptions. A well-behaved provider (per this +interface's own contract, restated in its XML doc) should proactively check what it +can statically determine before doing any real work, and return `NotHandled` for +anything it can't actually satisfy, rather than attempting the work and letting a +third-party exception surface raw — [ADR-0025](0025-compono-nsubstitute-package-design.md) +follows exactly this pattern for `NSubstituteProvider`. + +### Interaction with existing stages and mechanisms + +None of these required any new engine mechanism — each is an existing one, simply +reachable from a new direction: + +- **Stage order.** Unchanged, fixed, per `docs/architecture.md`. Stage 5/6 sit + between stage 4 (configuration rules) and stage 7 (built-in providers), exactly + where they've always been reserved. A registration/rule/prior-stage match always + wins over a public provider for the same request; a public provider always gets + first refusal over stage 7/8's built-in/generated fallback. +- **Shared substitute reuse / composition scopes.** No new mechanism — stage 2 + (shared/scoped values, [ADR-0011](0011-composition-scope-shared-values-and-recursion-detection.md)) + already runs before stage 5/6 and already stores *any* stage's successful result + when the request `IsShared`. A `[Shared] IRepository` parameter that a test-double + provider satisfies is stored and reused exactly like a shared registration or + built-in value would be — `docs/mvp.md`'s "Shared substitute reuse" Milestone 5 + scope item is fully satisfied by the existing engine, zero new work. +- **Deterministic seeds / composition path.** A provider that calls + `context.Resolve()` for a nested value forks the random stream and path + exactly like any other nested request (existing behavior, unchanged). A provider + that produces a leaf value directly (M5's `NSubstituteProvider`, per + [ADR-0025](0025-compono-nsubstitute-package-design.md)'s explicit no-recursive- + configuration non-goal) never touches randomness at all. +- **Recursion.** Not a new concern — the active-construction-frame recursion check + ([ADR-0011](0011-composition-scope-shared-values-and-recursion-detection.md)) + is pushed only around generated-plan dispatch (stage 8), not stage 5/6. A public + provider that never calls `context.Resolve()` (M5's provider) can't + participate in a construction cycle at all; one that does is subject to the exact + same cycle detection any other nested request already is. +- **Open generic behavior.** Does not arise: `CompositionProviderRequest.RequestedType` + is always a closed, concrete runtime `Type` (composition requests are never for + an open generic definition), so a provider never has to special-case one. This is + unrelated to core's separate, already-deferred "open generic *registrations*" + non-goal (`docs/mvp.md`). +- **`IServiceProvider` fallback.** Unaffected — stage 3 (exact registrations, then + the configured `IServiceProvider` fallback, + [ADR-0019](0019-registrations-and-service-provider-injection.md)) runs entirely + before stage 4/5/6, so a consumer's own DI container or exact registration always + takes precedence over a test-double/semantic provider for the same type, with no + new interaction to design. +- **Caching.** None needed beyond what already exists. A provider's own match + decision (e.g. `RequestedType.IsInterface`) is cheap enough to recompute per + request; value *reuse* is already scope's job (above), not the provider's. + +### Package Boundaries and Reflection Boundary + +`ICompositionValueProvider`/`CompositionProviderRequest`/`CompositionProviderResult` +are defined in core `Compono`, `public`, with zero dependency on any integration +package — an integration package implements the interface from the outside, +matching the existing dependency-diagram arrow direction +(`docs/architecture.md`'s Package Dependency Diagram: every arrow points *into* +`Compono`, never out). The public contract itself requires and performs zero +reflection to implement or invoke (an ordinary interface method call through +`PublicProviderAdapter`). What a specific provider implementation does *inside* its +own `TryProvide` body — NSubstitute's own `Substitute.For()` internally uses +Castle DynamicProxy/IL emission, which is reflection- and codegen-heavy and not +Native-AOT-safe — is entirely that provider's own concern, confined to its own +integration package. This is the explicit framing this design task was given: +"treat reflection required by NSubstitute itself as an integration concern, not a +relaxation of the core runtime guarantees," and it falls out of the design directly +rather than needing a special carve-out — `docs/mvp.md`'s existing "Native AOT +certification" non-goal already covers the consequence for a consumer that opts +into `UseNSubstitute()`. + +### Performance Characteristics + +No new allocation or dispatch cost on the pipeline's existing hot path beyond what +stage 4/7 already pay for their own ordered-provider lists: `TryProviders`' +existing loop, `CompositionTraceBuffer`'s existing per-attempt recording, and +`PublicProviderAdapter`'s own `TryCompose` (one struct construction for +`CompositionProviderRequest`, one interface dispatch to `TryProvide`) are all +already-established costs of the same shape stage 4's `TypeRuleProvider`/ +`MemberRuleProvider` already pay. `PublicProviderAdapter` instances themselves are +constructed once per registered provider, at `Build()` time — not per request, per +`Create()` call, or per composition. Whatever a specific provider's own +`TryProvide` implementation allocates (e.g. NSubstitute's own proxy-generation +cost) is that provider's own cost, orthogonal to this design; `Compono.Benchmarks`' +existing `ResolutionBenchmarks` infrastructure is the right place to add a +representative test-double-composed benchmark once `Compono.NSubstitute` ships +(a `PLAN-0005` task, not a design decision). + +### Validated against Bogus (M6), not designed for it + +To confirm this contract doesn't need reshaping the moment a second consumer shows +up, it was sketched (not built) against Milestone 6's own scope +(`docs/mvp.md`: "Conservative member-name conventions," "Explicit member rules"): + +```csharp +// Compono.Bogus, sketch only — not part of this milestone's scope. +internal sealed class BogusMemberNameProvider : ICompositionValueProvider +{ + public CompositionProviderResult TryProvide(in CompositionProviderRequest request, ICompositionContext context) => + request switch + { + { RequestedType.FullName: "System.String", Name: "FirstName" } => + CompositionProviderResult.Handled(_faker.Name.FirstName()), + { RequestedType.FullName: "System.String", Name: "Email" } => + CompositionProviderResult.Handled(_faker.Internet.Email()), + _ => CompositionProviderResult.NotHandled, + }; +} +``` + +`CompositionProviderRequest.Name`/`DeclaringType` (present, but unused by M5's own +`NSubstituteProvider`) are exactly what this needs — no interface change +anticipated for Milestone 6 based on this sketch. Not a commitment that Bogus's +actual design will look like this; a real Milestone 6 design dive still owns that +decision, per this repo's own process. + +### Alpha Compatibility Policy + +`ICompositionValueProvider`/`CompositionProviderRequest`/`CompositionProviderResult` +are `public` — but `Compono` as a whole is still alpha, pre-`docs/mvp.md`-completion +software, and this contract is no exception to that: + +- **Provisional, not frozen.** This contract is public so `Compono.NSubstitute` + can build against it and so a consumer could write a custom provider today, not + because it's guaranteed to be byte-for-byte stable through the rest of the MVP. +- **Milestone 6 and Milestone 7 are expected to validate it further.** This ADR's + own Bogus sketch (below) is a lightweight check that the contract isn't + accidentally NSubstitute-shaped, not proof it's the final shape — a real + Milestone 6 design dive, and Milestone 7's dogfooding pass, are the actual tests + of whether `CompositionProviderRequest`'s fields, `ICompositionValueProvider`'s + method shape, or the two-registration-method split hold up against real use. +- **Breaking changes are allowed when justified by real integration or dogfooding + evidence** — a concrete need Milestone 6/7 (or `Compono.NSubstitute` itself) + surfaces, not a hypothetical one imagined ahead of time. This ADR deliberately + does not add abstraction, versioning indirection, or extra fields "just in case" + to avoid ever having to make such a change during alpha — see the framing note + under Decision Outcome above. +- **Any such change must be explicitly documented, never silently introduced.** + Concretely, during alpha: + - The ADR that introduced the contract (this one, or [ADR-0025](0025-compono-nsubstitute-package-design.md) + for the NSubstitute-specific surface) gets a dated `## Amendment N (YYYY-MM-DD)` + section recording what changed and why, per `design-decisions.md`'s existing + Amendment mechanic — never a silent edit to this ADR's original Decision + Outcome text. + - `docs/public-api.md`'s Provider Extensibility/NSubstitute Integration sections + and the affected plan's Notes are updated in the same PR. + - The PR itself is labeled and categorized per this repo's existing Release + Drafter breaking-change convention — see `PLAN-0005`'s Breaking-Change + Handling During Alpha section for the mechanics, which apply to any future PR + that revises this contract after it first ships, not only to + `Compono.NSubstitute`'s own initial implementation. + +### Positive Consequences + +- The smallest public surface that lets NSubstitute (and, by validated sketch, + Bogus) express open-ended pattern matching: one interface, one method, two small + value types, two builder registration methods. +- No engine-internal type (`CompositionRequest`, `CompositionPath`, `PathSegment`, + the internal `CompositionResult`) becomes public; the internal/public split this + repo already uses for descriptors ([ADR-0010](0010-composition-request-pipeline-and-diagnostics-tracing.md)) + is extended, not abandoned. +- Diagnostics keep real provider-type identity through the adapter, with no public + API cost (`ProviderType` is an internal-only addition). +- Zero new pipeline mechanism: stage order, shared-scope reuse, recursion + detection, the `IServiceProvider` fallback, and collection dispatch are all + reused completely unchanged — this milestone fills in two already-reserved, + already-wired, always-empty stages rather than inventing new plumbing. +- Explicitly does not resemble `ISpecimenBuilder`/a middleware pipeline: a provider + answers exactly one request with exactly one of two outcomes and has no way to + see, skip, or reorder any other stage or provider. + +### Negative Consequences + +- Two small new public value types (`CompositionProviderRequest`, + `CompositionProviderResult`) plus one new public interface add to the public + surface `docs/public-api.md` asks to keep minimal — accepted as the necessary + cost of a real, decoupled public extension point; the alternative (Option 3) + costs strictly more surface for a worse encapsulation outcome. +- A provider that throws is not wrapped into a diagnostic-carrying + `CompositionException` the way a stage-3 registration factory's exception is — + an accepted, pre-existing asymmetry (stages 4-7 have never wrapped provider + exceptions), not a new gap, but worth revisiting if it proves painful once + `Compono.NSubstitute` ships real usage. +- `ICompositionProvider.ProviderType`'s default-interface-member addition is a + binary-compatible but still real shape change to an `internal` interface — no + external consumer can observe it (the interface is `internal`), but it's the + first default-interface-member this codebase has used; noted as a small style + precedent, not a risk. + +## Pros and Cons of the Options + +### Contract shape — new minimal public interface (chosen) + +- Good, because it keeps every engine-internal type internal. +- Good, because it reuses this repo's own established descriptor/request-split + precedent (ADR-0010) rather than inventing a new pattern. +- Good, because it preserves diagnostic identity through a small, internal-only + adapter addition. +- Bad, because it's two new public value types plus one interface, more surface + than a delegate-only design — accepted, see Decision Outcome. + +### Contract shape — delegate-based registration only + +- Good, because it's the smallest possible public surface: one method parameter + type, zero new interfaces to learn. +- Good, because a trivial custom provider needs no class declaration at all. +- Bad, because a captured lambda has no meaningful `GetType()`/type identity for + `ProviderAttempt.Provider` — the exact diagnostic-naming problem + [ADR-0016](0016-provider-identity-restored-in-provider-attempt.md) already + solved for internal providers would regress for every public one, the single + biggest reason this option was rejected (confirmed directly with the user). +- Bad, because `NSubstituteProvider`/a future `BogusProvider` are reusable + integration components with their own configuration, diagnostics, and lifecycle + concerns (per the user's own framing) — a named, independently unit-testable, + independently documented type fits that better than an inline lambda a package's + `UseX()` extension method would otherwise have to construct and hand over. +- A delegate-based convenience overload may still be added later for a lightweight + ad hoc custom provider, adapting to `ICompositionValueProvider` internally rather + than defining the core extensibility model itself — noted as a Deferred Decision + below, not designed here. + +### Contract shape — make the engine's own `ICompositionProvider` public + +- Bad, because it forces `CompositionRequest` (and transitively `CompositionPath`, + `PathSegment`, `IsShared`) to become public too, or leaves external implementors + with no documented way to construct a request at all. +- Bad, because it couples every future internal pipeline refactor to a public + compatibility promise it doesn't need to make — the exact "avoid exposing + source-generator/engine implementation details" rule this repo's API Design + Rules already state. +- Bad, because `docs/public-api.md`'s minimal-surface goal is directly opposed by + exposing the engine's own richer internal request/result shapes wholesale. + +### Contract shape — middleware/chain-of-responsibility (`next()`) + +- Bad, because it lets a provider observe and influence what happens after its own + decision, which conflicts directly with `docs/architecture.md`'s explicit "stage + order... not configurable, by users or by providers reordering themselves" rule. +- Bad, because it's exactly the shape this design task was told to avoid absent an + objective advantage — none was found; the existing ordered-list-of-independent- + candidates dispatch (`TryProviders`) already achieves composability across + multiple registered providers without any provider needing visibility beyond its + own match/no-match decision. + +## Amendment 1 (2026-07-31): Nested resolution and self-recursion, fixed before any external consumer existed + +PR #28 review (Codex, two P1 findings against Phase 0's implementation of this +ADR, both against the same root cause) caught a real gap this ADR's own Decision +Outcome text glossed over: `ICompositionValueProvider.TryProvide`'s XML doc — and +this ADR's own "Interaction with existing stages and mechanisms" section — state +that a provider "may call `context.Resolve()` to compose part of its value from +a nested request, exactly as an internal provider already may." That's true for an +internal `TypeRuleProvider`/`MemberRuleProvider`, but was **not** true for a public +provider as originally implemented, for a reason this ADR didn't anticipate: + +1. **The descriptor-less `context.Resolve()` overload threw unconditionally.** + That overload requires an active manual-resolve frame + ([ADR-0019](0019-registrations-and-service-provider-injection.md)), pushed only + by `CompositionContext.InvokeFactory` — which `PublicProviderAdapter.TryCompose` + never called; it invoked the wrapped provider's `TryProvide` directly. A + provider following this ADR's own documented contract hit + `InvalidOperationException` every time. +2. **A provider recursing on its own requested type (via the descriptor-*based* + overload, which needs no frame) had no cycle protection at all and ran until + `StackOverflowException`.** An internal `TypeRuleProvider`/`MemberRuleProvider` + avoids this because `InvokeFactory`'s reentrance guard is keyed on **factory + delegate identity** — safe only because each of those provider instances is + compiled 1:1 for exactly one type, so "same delegate re-entered" and "same type + re-entered" are equivalent by construction. A public `ICompositionValueProvider` + instance has no such 1:1 guarantee (one instance can legitimately handle many + types, e.g. "any interface"), so that same keying would either miss real cycles + or — worse — wrongly block a provider composing an unrelated type as one of its + own legitimate nested dependencies. + +**Fix:** a new internal `CompositionContext.InvokeProvider` method, structurally +parallel to `InvokeFactory` but distinct from it in two ways: reentrance is keyed +on **`(provider instance, requested type)`**, not delegate identity alone; and it +never catches or wraps an exception `TryProvide` throws — `InvokeFactory`'s +blanket `catch (Exception ex)` block is deliberately not reused, since this ADR's +own Provider Failure Semantics section commits to a public provider's thrown +exception propagating uncaught, and reusing `InvokeFactory` wholesale would have +silently reversed that decision (a rejected alternative, considered explicitly +during the fix rather than applied by default — the reentrance-guard's own +diagnosed `CompositionException` is a different concern from wrapping the +provider's *own* thrown exception, and stays intact under this fix). Every +architectural guarantee this ADR's Decision Outcome commits to (named interface, +decoupled request/result contract, stage placement/order, diagnostics identity, +immutable/reusable registration, `NotHandled`/`Handled` semantics) is unchanged by +this fix — `PublicProviderAdapter`'s exact internal shape was already documented +as illustrative, not committed, per this ADR's own framing note under Decision +Outcome. + +Fixed entirely within Milestone 5 Phase 0, before `Compono.NSubstitute` (the +ADR's first real external consumer) exists — no external code has ever observed +the broken behavior. Regression coverage: +`Provider_CanComposeANestedValue_ViaTheDescriptorLessResolveOverload` (finding 1, +plus proves the `(provider, type)` key doesn't over-block a legitimate nested +different-type request through the same instance) and +`Provider_RecursivelyResolvingItsOwnRequestedType_ThrowsADiagnosedException_NotStackOverflow` +(finding 2), both in `CompositionValueProviderTests.cs`; the pre-existing +`ThrownException_FromAPublicProvider_PropagatesUncaught` test continues to pass +unchanged, confirming the fix didn't reverse the Provider Failure Semantics +decision. + +## Links + +- [docs/mvp.md](../mvp.md) — Milestone 5 scope, and Milestone 3's explicit + deferral of this exact question +- [docs/architecture.md](../architecture.md) — Resolution Pipeline stages 5/6, + Providers section, Open Architectural Decisions' deferred-extensibility entry + (resolved by this ADR) +- [docs/public-api.md](../public-api.md) — API Design Rules (minimal public + surface, avoid exposing engine internals), NSubstitute/Bogus Integration sketches +- [ADR-0010](0010-composition-request-pipeline-and-diagnostics-tracing.md) — the + public-descriptor/internal-request split this ADR's public-request/internal- + request split mirrors; the `Failure`-is-authoritative-only rule this ADR's + provider failure semantics follow +- [ADR-0011](0011-composition-scope-shared-values-and-recursion-detection.md) — + the shared-scope/recursion mechanisms this ADR reuses unchanged +- [ADR-0016](0016-provider-identity-restored-in-provider-attempt.md) — the + provider-identity diagnostics contract this ADR's `ProviderType` addition + preserves through the new public-provider adapter +- [ADR-0019](0019-registrations-and-service-provider-injection.md) — the stage-3 + fallback this ADR's stage-5/6 placement stays subordinate to +- [ADR-0020](0020-composition-configuration-rules.md) — the "compile public + builder data into an internal `ICompositionProvider`" precedent this ADR reuses + for public providers instead of value rules +- [ADR-0021](0021-row-composition-entry-point-for-test-framework-integrations.md)/ + [ADR-0022](0022-compono-xunit-package-design.md) — the core-extension-ADR + + package-design-ADR split this ADR/ADR-0025 mirror +- [ADR-0025](0025-compono-nsubstitute-package-design.md) — `Compono.NSubstitute`, + the first real consumer of this contract diff --git a/docs/adr/0025-compono-nsubstitute-package-design.md b/docs/adr/0025-compono-nsubstitute-package-design.md new file mode 100644 index 0000000..0e8a4c2 --- /dev/null +++ b/docs/adr/0025-compono-nsubstitute-package-design.md @@ -0,0 +1,438 @@ +# [ADR-0025] Compono.NSubstitute Package Design + +**Status:** Accepted + +**Date:** 2026-07-31 + +**Decision Makers:** Nick Cipollina, Claude (design review) + +## Context + +[ADR-0024](0024-public-provider-extensibility-model.md) settles the mechanism a +package like `Compono.NSubstitute` plugs into: `ICompositionValueProvider`, +registered via `CompositionBuilder.AddTestDoubleProvider(...)`, compiled into +stage 6. This ADR settles everything specific to the package itself — +`docs/mvp.md`'s Milestone 5 scope: "Test-double provider contract, Interface +substitutes, Optional abstract-class substitutes, Shared substitute reuse, +Integration-specific configuration, Clear diagnostics when substitution is +unsupported," and its explicit non-goals: "Recursive auto-configuration of +substitute members, NSubstitute API wrappers, Pinning NSubstitute versions in the +core package." `docs/public-api.md` already sketches the intended activation +shape: + +```csharp +builder.UseNSubstitute(); + +builder.UseNSubstitute(options => +{ + options.SubstituteAbstractClasses = false; +}); +``` + +This ADR turns that sketch into a real design: what `NSubstituteProvider` actually +matches (interface, abstract class, delegate — the "delegate handling" and +"abstract-class support"/"interface substitution" items this milestone's design +task called out explicitly), what `NSubstituteOptions` exposes, how "shared +substitute reuse" is satisfied (answer: entirely by ADR-0024/existing engine +mechanism, no new code), and what "clear diagnostics when substitution is +unsupported" means concretely. + +## Decision Drivers + +- `docs/mvp.md`'s explicit non-goal: "Recursive auto-configuration of substitute + members" — `NSubstituteProvider` produces a bare substitute via NSubstitute's own + `Substitute.For(...)`/equivalent API and returns it; it does not call + `context.Resolve()` to configure the substitute's own members/return values. +- `docs/mvp.md`'s explicit non-goal: "NSubstitute API wrappers" — this package + exposes activation (`UseNSubstitute`) and configuration (`NSubstituteOptions`) + only; it does not re-expose or wrap NSubstitute's own `Arg`/`Received`/`Returns` + surface, which a consumer already uses directly against the composed substitute. +- `docs/mvp.md`'s explicit non-goal: "Pinning NSubstitute versions in the core + package" — trivially satisfied by [ADR-0024](0024-public-provider-extensibility-model.md)'s + design: `Compono` core has no reference to `NSubstitute` at all; only + `Compono.NSubstitute` does, per the existing package-dependency diagram. + `Compono.NSubstitute`'s own `.csproj` still needs *some* version constraint on + its `PackageReference`, but that's ordinary package versioning, not a core-package + pin. +- Coding standard: exceptions are for the unexpected — an ordinary "this type isn't + substitutable" outcome must be `NotHandled`, not a thrown exception (see + [ADR-0024](0024-public-provider-extensibility-model.md)'s Provider Failure + Semantics). +- `design-decisions.md` rule 4/[ADR-0001](0001-source-generation-first.md): no + reflection-based fallback *in core*. `Compono.NSubstitute`'s own reliance on + NSubstitute's internal (reflection/codegen-heavy) proxy generation is an + integration-package concern this ADR treats as accepted, per this milestone's + own explicit framing, not a relaxation of that rule. + +## Considered Options + +### What `NSubstituteProvider` treats as substitutable + +1. **Defer entirely to what NSubstitute's own public API can construct**: any + interface type, any delegate type, and (only if + `NSubstituteOptions.SubstituteAbstractClasses` is `true`) any non-sealed + abstract class — matching NSubstitute's own `Substitute.For()` capability + surface as closely as a static `Type` check reasonably can, rather than + Compono re-deriving its own narrower notion of "substitutable." +2. **Interfaces only for the MVP** — no delegate or abstract-class support at all, + deferring both to a later milestone. +3. **Interfaces plus abstract classes only** — omitting delegate types, which + `docs/mvp.md`'s own scope list doesn't explicitly name (only this design task's + investigation list does). + +### Provider failure/diagnostics for an unsupported type + +1. **Static, cheap pre-check; `NotHandled` for anything that doesn't match** — the + provider never calls into NSubstitute at all for a type it can already tell, + from `Type` reflection alone, it can't substitute (a sealed concrete class, a + struct, `string`, etc.). The resulting "nothing could satisfy this" diagnostic + is the engine's own existing generic failure message + (`CompositionContext`'s `"No registration, configuration rule, semantic + provider, test-double provider, built-in provider, or generated plan could + satisfy '{type}'."`), which already names the exact failed type and its full + path — no NSubstitute-specific message needed for the common case. +2. **A custom `NSubstituteDiagnosticException`** thrown for any unsubstitutable + type, wrapping/replacing the engine's own diagnostic. + +### Options plumbing + +1. **A plain mutable options class, `NSubstituteOptions`, configured via + `Action? configure`** on `UseNSubstitute`, matching + `UseBogus`'s existing sketch in `docs/public-api.md` exactly — no `IOptions`/ + Microsoft.Extensions.Options dependency (core has none, and this package + doesn't need one either; `coding-standards.md`'s `IOptions` section applies + only "if a configuration class is bound via `IOptions`," which nothing here + does). + +## Decision Outcome + +### Substitutable shapes — Option 1, defer to NSubstitute's own capability + +```csharp +internal static bool IsSubstitutable(Type requestedType, NSubstituteOptions options) => + requestedType.IsInterface + || typeof(Delegate).IsAssignableFrom(requestedType) + || (options.SubstituteAbstractClasses && requestedType.IsAbstract && !requestedType.IsSealed && !requestedType.IsInterface); +``` + +**Corrected by Amendment 1 (below).** This original sketch has two defects a +pre-implementation review caught: the delegate check also matches the non- +substitutable framework base types `Delegate`/`MulticastDelegate` themselves, and +storing/reading a caller-owned mutable `NSubstituteOptions` directly conflicts with +[ADR-0024](0024-public-provider-extensibility-model.md)'s immutable-provider- +registration guarantee. See Amendment 1 for the corrected shape; this section's +prose (substitutability rules, precedence, rationale) is otherwise unchanged and +still accurate. + +- **Interfaces**: always substitutable (`RequestedType.IsInterface`) — the + milestone's headline case, and the pattern no closed-set `.For()` rule could + express (`docs/architecture.md`'s Open Architectural Decisions entry this + milestone resolves). +- **Delegates**: always substitutable + (`typeof(Delegate).IsAssignableFrom(requestedType)`) — NSubstitute natively + supports `Substitute.For()`; this design task's investigation list + named delegate handling explicitly, `docs/mvp.md`'s non-goals don't exclude it, + and it costs one extra branch in an already-cheap static check. Delegate + substitution is included in Milestone 5's scope on that basis. +- **Abstract classes**: substitutable only when + `NSubstituteOptions.SubstituteAbstractClasses` is `true` (default `true`, per + `docs/public-api.md`'s existing sketch showing it explicitly set to `false` + as the noteworthy case) — `docs/mvp.md`'s own wording, "Optional abstract-class + substitutes," names this as configuration, not an always-on behavior. +- **Everything else** (a sealed concrete class, a struct, `string`, a generic type + parameter already closed to something else entirely) — `NotHandled`, + unconditionally, with no attempt to call into NSubstitute at all. This is a + purely static check; it never allocates, never reflects beyond the handful of + `Type` property reads already shown above, and runs before any real + substitute-creation work. +- **Open generic behavior**: does not arise, per [ADR-0024](0024-public-provider-extensibility-model.md) — + `CompositionProviderRequest.RequestedType` is always closed + (`IRepository`, never `IRepository<>`), so `Substitute.For()` is + always called (indirectly, via NSubstitute's non-generic `Substitute.For(Type[], + object[])` overload, since `Compono.NSubstitute` only has a runtime `Type`, not a + compile-time generic parameter) with a fully closed type either way. + +```csharp +public sealed class NSubstituteProvider : ICompositionValueProvider +{ + private readonly NSubstituteOptions _options; + + public NSubstituteProvider(NSubstituteOptions options) + { + ArgumentNullException.ThrowIfNull(options); + _options = options; + } + + public CompositionProviderResult TryProvide(in CompositionProviderRequest request, ICompositionContext context) + { + if (!IsSubstitutable(request.RequestedType, _options)) + return CompositionProviderResult.NotHandled; + + return CompositionProviderResult.Handled(Substitute.For([request.RequestedType], [])); + } +} +``` + +**Corrected by Amendment 1 (below):** this constructor retains a reference to the +caller-supplied, mutable `NSubstituteOptions` instance — since `NSubstituteProvider` +is `public`, a caller holding that same `options` reference can mutate it after +`UseNSubstitute`/construction, silently changing this provider's substitution +behavior after the fact. See Amendment 1 for the snapshotting fix. + +No call to `context.Resolve()` anywhere in this provider — confirming the +"recursive auto-configuration of substitute members" non-goal directly in the +shape of the code, not just as a stated intent. A bare substitute is returned +exactly as NSubstitute constructs it; a consumer configures its behavior +(`Returns`, `When`/`Do`) in their own test body against the composed instance, +same as any hand-written `Substitute.For()` call today. + +### Diagnostics — Option 1, static pre-check + the engine's existing failure message + +No NSubstitute-specific exception type is introduced. `docs/mvp.md`'s "clear +diagnostics when substitution is unsupported" requirement is satisfied by the +static pre-check above returning `NotHandled` promptly (rather than attempting and +failing inside NSubstitute's own code) combined with the engine's own existing +terminal diagnostic — `CompositionContext`'s stage-9 failure message already names +the exact failed type and its full composition path +(`docs/architecture.md`'s Diagnostics example), which is precisely "clear +diagnostics" for the common case (a consumer accidentally requests a sealed class +where they meant an interface, say). The rarer residual case — NSubstitute's own +`Substitute.For(...)` throwing for a type that passed the static check but that +NSubstitute itself can't actually proxy for some internal reason — propagates +uncaught, per [ADR-0024](0024-public-provider-extensibility-model.md)'s Provider +Failure Semantics (an accepted, explicitly-stated limitation there, not +re-litigated here). No design attempts to catch and re-wrap that residual case +into a `CompositionException` for Milestone 5; revisit only if real usage shows +this rough edge actually matters in practice. + +### Shared substitute reuse — no new code + +Satisfied entirely by [ADR-0024](0024-public-provider-extensibility-model.md)'s +existing-engine-mechanism answer: a `[Shared] IRepository` parameter that +`NSubstituteProvider` satisfies is stored into the row's `CompositionScope` +exactly like any other stage's successful shared result, and a later ordinary +(non-`[Shared]`) request for the same type — including a generated plan's own +constructor parameter — transparently reuses it, per +[ADR-0021](0021-row-composition-entry-point-for-test-framework-integrations.md)'s +unconditional stage-2 read. `Compono.NSubstitute` contributes zero code toward this +requirement; it falls out of registering into stage 6 at all. + +### Configuration — Option 1, plain options class + +```csharp +public sealed class NSubstituteOptions +{ + /// + /// Whether an unsealed abstract class is substitutable, in addition to every interface and + /// delegate type. Defaults to . + /// + public bool SubstituteAbstractClasses { get; set; } = true; +} +``` + +```csharp +namespace Compono; + +public static class CompositionBuilderExtensions +{ + extension(CompositionBuilder builder) + { + public CompositionBuilder UseNSubstitute() => builder.UseNSubstitute(static _ => { }); + + public CompositionBuilder UseNSubstitute(Action configure) + { + ArgumentNullException.ThrowIfNull(configure); + var options = new NSubstituteOptions(); + configure(options); + return builder.AddTestDoubleProvider(new NSubstituteProvider(options)); + } + } +} +``` + +Matches `docs/public-api.md`'s existing sketch exactly. `NSubstituteProvider`'s own +options are constructed and owned entirely inside `Compono.NSubstitute` — core +`Compono` never sees `NSubstituteOptions`, only the already-configured +`ICompositionValueProvider` instance `AddTestDoubleProvider` receives. + +### Package boundary + +`Compono.NSubstitute` depends on `Compono` and `NSubstitute` only, matching the +existing package-dependency diagram (`docs/architecture.md`) exactly — +`NSubstituteProvider`/`NSubstituteOptions`/the `UseNSubstitute` extension are the +package's entire public surface for Milestone 5. `Compono.NSubstitute` never +implements or references `Compono`'s internal `ICompositionProvider` — only the +public `ICompositionValueProvider` [ADR-0024](0024-public-provider-extensibility-model.md) +defines. + +### Positive Consequences + +- Matches `docs/mvp.md`'s exit criterion directly: "a typical service test can + receive a shared substitute, a composed system under test, and a composed + request with no manual setup" — the `[Shared] IRepository` example in + `docs/public-api.md`'s xUnit v3 Experience section already demonstrates exactly + this, and needs zero code change to keep working once `NSubstituteProvider` + exists. +- No new diagnostic mechanism, no new exception type — reuses the engine's + existing, already-good failure message. +- `IsSubstitutable`'s static check is trivially unit-testable in isolation, with + no NSubstitute call involved for the negative cases. + +### Negative Consequences + +- Delegate substitution wasn't in `docs/mvp.md`'s own explicit scope list before + this ADR — a small, deliberate scope addition, justified above; flagged here so + it isn't mistaken for scope creep discovered later. `docs/mvp.md`'s Milestone 5 + section should be updated to mention it explicitly (a `PLAN-0005` doc task). +- The residual "NSubstitute itself throws for a superficially-matching type" + case has no bespoke diagnostics — an accepted rough edge, not solved here (see + Diagnostics above). + +## Pros and Cons of the Options + +### Substitutable shapes — defer to NSubstitute's capability (chosen) + +- Good, because it tracks NSubstitute's own actual capability rather than a + second, independently-maintained notion of "substitutable" that could drift + from what NSubstitute really supports. +- Good, because each check is a cheap, well-understood `Type` reflection property + read, not a runtime probe. +- Bad, because it means Milestone 5 quietly adds delegate support beyond + `docs/mvp.md`'s original bullet list — mitigated by updating that doc in the + same PR, per this repo's own documentation rule. + +### Substitutable shapes — interfaces only + +- Good, because it's the narrowest possible MVP scope. +- Bad, because it leaves "abstract-class support" (explicitly named in + `docs/mvp.md`'s own Milestone 5 scope) undesigned, just deferred again with no + new information gained by waiting. + +### Substitutable shapes — interfaces + abstract classes, no delegates + +- Good, because it matches `docs/mvp.md`'s original bullet list without any + doc update needed. +- Bad, because delegate substitution costs one extra, cheap static-check branch + and NSubstitute already supports it natively — excluding it buys nothing and + leaves a real, easy capability on the table for no stated reason. + +### Diagnostics — static pre-check + engine's existing message (chosen) + +- Good, because it adds no new type, exception, or message format to maintain. +- Good, because the engine's existing diagnostic already names the exact type and + path — genuinely "clear," not a placeholder. +- Bad, because the rare NSubstitute-internal-throw case still surfaces a raw, + non-`CompositionException` error with no seed/path context — accepted, see + Negative Consequences above. + +### Diagnostics — custom `NSubstituteDiagnosticException` + +- Bad, because it duplicates diagnostic machinery (`CompositionDiagnostic`, + seed-in-message, path rendering) the engine's existing failure path already + provides for free, for no case the pre-check-plus-`NotHandled` design doesn't + already handle. + +This package's own surface (`NSubstituteProvider`, `NSubstituteOptions`, +`UseNSubstitute`) is subject to the same alpha compatibility posture +[ADR-0024](0024-public-provider-extensibility-model.md)'s Alpha Compatibility +Policy states for `ICompositionValueProvider` itself — provisional during alpha, +revisable when Milestone 6/7 or real usage justifies it, any such change +explicitly documented per that policy and `PLAN-0005`'s breaking-change handling +section, never silently introduced. + +## Amendment 1 (2026-07-31): Options snapshotting and delegate-type matching, corrected before implementation + +A pre-implementation review of this ADR (before any `Compono.NSubstitute` code was +written) caught two defects in the sketches above. Both are corrected here, per +`design-decisions.md`'s Amendment mechanic — the original Decision Outcome text +above is left as written, since it's still accurate about *what* is substitutable +and *why*; only the two code sketches' exact shape is superseded. + +**1. `NSubstituteProvider` must snapshot `NSubstituteOptions`, never retain the +caller-owned mutable instance.** [ADR-0024](0024-public-provider-extensibility-model.md) +commits to "immutable, reusable provider registration" as one of its architectural +guarantees — a provider, once constructed, must be safe to invoke repeatedly +(including concurrently) for the lifetime of the `Composer` it's registered on. +`NSubstituteProvider` is `public` (per this ADR's own package-boundary section), so +a consumer can construct it directly, meaning the original sketch's +`private readonly NSubstituteOptions _options` field is reachable and mutable after +registration — a consumer holding the same `options` reference (or one constructed +and reused across multiple `UseNSubstitute(options => ...)` calls) could change +`SubstituteAbstractClasses` after the fact, silently changing already-registered +behavior and violating the guarantee directly. Corrected shape: the constructor +reads `options.SubstituteAbstractClasses` once and stores the extracted `bool`, +never the `NSubstituteOptions` reference itself: + +```csharp +public sealed class NSubstituteProvider : ICompositionValueProvider +{ + private readonly bool _substituteAbstractClasses; + + public NSubstituteProvider(NSubstituteOptions options) + { + ArgumentNullException.ThrowIfNull(options); + _substituteAbstractClasses = options.SubstituteAbstractClasses; + } + + public CompositionProviderResult TryProvide(in CompositionProviderRequest request, ICompositionContext context) + { + if (!IsSubstitutable(request.RequestedType, _substituteAbstractClasses)) + return CompositionProviderResult.NotHandled; + + return CompositionProviderResult.Handled(Substitute.For([request.RequestedType], [])); + } +} +``` + +`IsSubstitutable`'s signature changes to match — it now takes the already-extracted +`bool substituteAbstractClasses` rather than the mutable `NSubstituteOptions` +instance, so the static check itself can never observe a post-construction mutation +either: + +```csharp +internal static bool IsSubstitutable(Type requestedType, bool substituteAbstractClasses) => + requestedType.IsInterface + || requestedType.IsSubclassOf(typeof(MulticastDelegate)) + || (substituteAbstractClasses && requestedType.IsAbstract && !requestedType.IsSealed && !requestedType.IsInterface); +``` + +If `NSubstituteOptions` ever grows a second configurable value, the same rule +applies: snapshot every value the constructor needs into its own immutable field +(or into one small internal immutable settings record, if the number of extracted +values grows unwieldy as plain fields) — never retain the `NSubstituteOptions` +reference itself. + +**2. Delegate matching must exclude the framework base types.** +`typeof(Delegate).IsAssignableFrom(requestedType)` also returns `true` for +`requestedType == typeof(Delegate)` and `requestedType == typeof(MulticastDelegate)` +themselves — neither is a concrete, declared delegate type NSubstitute can actually +substitute; both are the framework's own abstract base types every real delegate +type derives from. `requestedType.IsSubclassOf(typeof(MulticastDelegate))` is the +correct check: every real declared delegate type (`Action`, `Func`, a +custom `delegate` declaration) is a subclass of `MulticastDelegate`, but +`MulticastDelegate`/`Delegate` are never subclasses of themselves, so `IsSubclassOf` +excludes exactly the two base types `IsAssignableFrom` wrongly included. Shown +combined with fix 1 above. + +**Test coverage added to `PLAN-0005`'s Phase 2** for both fixes: negative +`IsSubstitutable` cases for `typeof(Delegate)` and `typeof(MulticastDelegate)` +themselves (alongside the existing sealed-class/struct/`string` negative cases), a +positive case for a real custom delegate type, and a mutation test proving that +mutating an `NSubstituteOptions` instance after `UseNSubstitute(options => ...)` +returns does not change an already-registered `NSubstituteProvider`'s behavior. + +## Links + +- [docs/mvp.md](../mvp.md) — Milestone 5 scope, non-goals, exit criterion +- [docs/public-api.md](../public-api.md) — NSubstitute Integration section + (`UseNSubstitute`/`NSubstituteOptions` sketch this ADR finalizes) +- [docs/architecture.md](../architecture.md) — `Compono.NSubstitute` Package + Boundaries entry, stage 6 (test-double providers) +- [ADR-0011](0011-composition-scope-shared-values-and-recursion-detection.md) — + the shared-scope mechanism "shared substitute reuse" relies on unchanged +- [ADR-0021](0021-row-composition-entry-point-for-test-framework-integrations.md) — + the unconditional stage-2 read that makes a `[Shared]` substitute visible to an + ordinary nested constructor parameter +- [ADR-0024](0024-public-provider-extensibility-model.md) — the public provider + contract, registration surface, and provider-failure-semantics rule this ADR + implements against diff --git a/docs/adr/README.md b/docs/adr/README.md index e43ebe7..46202ec 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -85,3 +85,5 @@ the mechanics: numbering, status, and the index. | [0021](0021-row-composition-entry-point-for-test-framework-integrations.md) | Row Composition Entry Point for Test-Framework Integrations | Accepted | | [0022](0022-compono-xunit-package-design.md) | Compono.Xunit Package Design | Accepted | | [0023](0023-rename-compono-xunit-to-compono-xunitv3.md) | Rename Compono.Xunit to Compono.XunitV3 | Accepted | +| [0024](0024-public-provider-extensibility-model.md) | Public Provider Extensibility Model | Accepted | +| [0025](0025-compono-nsubstitute-package-design.md) | Compono.NSubstitute Package Design | Accepted | diff --git a/docs/architecture.md b/docs/architecture.md index ff54e9d..e2fcc35 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -191,15 +191,22 @@ not every stage is the same *kind* of thing: | 2 | Shared or scoped values | Context-owned deterministic check against the scope. Milestone 2/3 shipped this gated by the *current* request's own `IsShared` flag on both the read and write side; [ADR-0021](adr/0021-row-composition-entry-point-for-test-framework-integrations.md) (implemented, Milestone 4 Phase 0) changed the **read** side to an unconditional scope check (any request, `IsShared` or not, sees an already-shared value for its type) while leaving the **write** side unchanged (only an `IsShared` request ever populates scope) — required for a Milestone 4 `[Shared]` test parameter's value to reach an *ordinary*, unmarked nested constructor parameter of the same type. | | 3 | Exact registrations | **Hybrid**, per [ADR-0019](adr/0019-registrations-and-service-provider-injection.md) (implemented, Milestone 3 Phase 1): a context-owned deterministic lookup against the exact-registration table, then — only on a miss, if a consumer called `UseServiceProvider(...)` — a fallback `IServiceProvider.GetService(typeof(T))` call. Milestone 2 shipped this stage as internal-only with no public builder; Milestone 3 Phase 1 shipped its real public shape (`builder.Register(...)`, `builder.UseServiceProvider(...)`). | | 4 | Configuration rules | Ordered `ICompositionProvider` collection, per [ADR-0020](adr/0020-composition-configuration-rules.md) (implemented, Milestone 3 Phase 3). Renamed from "profile rules" (`PipelineStage.ConfigurationRule`): populated by type/member value rules compiled from `builder.For()...`, whether reached directly or via a profile's `Configure` — a profile is a reusable application mechanism over this stage, not its owner ([ADR-0018](adr/0018-composition-profiles.md)). Collection-size configuration does **not** populate this stage — see Configuration Rules, below. | -| 5 | Semantic value providers | Ordered `ICompositionProvider` collection — empty until Milestone 6 (Bogus). How an integration package populates this stage with open-ended, pattern-matching logic is deliberately undesigned until Milestone 5 gives it a real consumer — see Open Architectural Decisions, below. | -| 6 | Test-double providers | Ordered `ICompositionProvider` collection — empty until Milestone 5 (NSubstitute), same deferred-extensibility note as stage 5. | +| 5 | Semantic value providers | Ordered `ICompositionProvider` collection. The public registration surface (`builder.AddSemanticProvider(ICompositionValueProvider)`) is implemented as of Milestone 5 Phase 0 ([ADR-0024](adr/0024-public-provider-extensibility-model.md)) — but no `Compono`-shipped package registers anything into it by default, since `Compono.Bogus` (Milestone 6) doesn't exist yet. Empty in practice until then, populated in mechanism now. | +| 6 | Test-double providers | Ordered `ICompositionProvider` collection. Same status as stage 5: the public registration surface (`builder.AddTestDoubleProvider(ICompositionValueProvider)`) is implemented ([ADR-0024](adr/0024-public-provider-extensibility-model.md)), but empty in practice until `Compono.NSubstitute` (Milestone 5's own package, [ADR-0025](adr/0025-compono-nsubstitute-package-design.md)) ships — see [PLAN-0005](plans/0005-milestone-5-nsubstitute-integration.md) for its phase status. | | 7 | Built-in value providers | **Hybrid** (ADR-0014): an ordered `ICompositionProvider` collection (primitive/simple types, enums, nullable value types), populated internally by `Compono` itself, tried first — followed by a context-owned deterministic dispatch through `CollectionPlanCache` for the five built-in collection shapes (array, `List`, `IReadOnlyList`, `HashSet`, `Dictionary`), the same closed-generic-field-read mechanism stage 8 uses, since `ICompositionProvider` can't itself construct a generic collection without reflection | | 8 | Generated composition plans | Context-owned deterministic dispatch via `PlanCache` — **not** an `ICompositionProvider` (see Source-Generated Composition Plans, below) | | 9 | Diagnostic failure | Context-owned terminal stage | -Only stages 4/6/7 hold an actual ordered collection of providers in -Milestone 2 (and of those, only 7 has anything registered in it — 4/5/6 -are wired but empty until their owning milestone). Provider order +Stages 4/5/6/7 each hold an actual ordered collection of providers, and +every one of their public registration surfaces is implemented today +(`.For()` for stage 4, since Milestone 3; `AddSemanticProvider`/ +`AddTestDoubleProvider` for stages 5/6, since Milestone 5 Phase 0 — +[ADR-0024](adr/0024-public-provider-extensibility-model.md)) — but only 4 +and 7 have anything registered in them *by default*: stage 4 only when a +consumer actually calls `.For()`, stage 7 unconditionally +(`BuiltInProviders.Default`). Stages 5/6 stay empty until a consumer +registers a provider directly, or until `Compono.Bogus`/`Compono.NSubstitute` +(which don't exist yet) do it on their behalf. Provider order *within* an extensible stage is registration order; stage 7 alone already holds three real providers (`PrimitiveValueProvider`, `EnumValueProvider`, `NullableValueProvider` — `BuiltInProviders.Default`), so "no stage has @@ -862,19 +869,25 @@ Owns: [ADR-0018](adr/0018-composition-profiles.md): `ICompositionProfile` interface (not an abstract base class), eager in-order application, type-keyed cycle detection. -- **Public provider extensibility (how integration packages contribute open-ended - pattern-matching logic to stages 5/6)** — evaluated and explicitly deferred to - Milestone 5 during the Milestone 3 design review: Milestone 3's own scope - (registrations, profiles, type/member rules, `IServiceProvider` fallback) needs no - such interface — type/member value rules compile into internal, - Compono-authored providers ([ADR-0020](adr/0020-composition-configuration-rules.md)), - and service injection folds into stage 3 as a context-owned fallback - ([ADR-0019](adr/0019-registrations-and-service-provider-injection.md)) — neither - needs a public `ICompositionProvider`-shaped contract. NSubstitute (Milestone 5) - is expected to be the first real consumer that needs one (matching "any interface - type," a pattern no closed-set rule can express), so the interface gets designed - against that concrete need rather than speculatively now. No design has been - chosen yet. +- ~~Public provider extensibility (how integration packages contribute open-ended + pattern-matching logic to stages 5/6)~~ — evaluated and explicitly deferred to + Milestone 5 during the Milestone 3 design review, for the reasons this bullet + originally gave (type/member value rules compile into internal, + Compono-authored providers per [ADR-0020](adr/0020-composition-configuration-rules.md); + service injection folds into stage 3 per + [ADR-0019](adr/0019-registrations-and-service-provider-injection.md); neither + needed a public contract). **Resolved by + [ADR-0024](adr/0024-public-provider-extensibility-model.md)** (core contract: + `ICompositionValueProvider`, `CompositionBuilder.AddSemanticProvider`/ + `AddTestDoubleProvider`, compiled into stage 5/6 the same way ADR-0020's rules + compile into stage 4 — **implemented, PLAN-0005 Phase 0**) **and + [ADR-0025](adr/0025-compono-nsubstitute-package-design.md)** (`Compono.NSubstitute`, + the first real consumer of that contract — design only, not yet implemented; + see [PLAN-0005](plans/0005-milestone-5-nsubstitute-integration.md) for its + phase status). The core extension point itself is real and tested; stages + 5/6 in the Resolution Pipeline table above stay empty in practice only + because no `Compono`-shipped package (`Compono.Bogus`, `Compono.NSubstitute`) + registers anything into them yet. - **Richer `Microsoft.Extensions.DependencyInjection` integration** (`IServiceCollection` auto-registration, per-composition scoping, keyed services) — explicitly out of scope for `Compono` core per diff --git a/docs/mvp.md b/docs/mvp.md index 8e7d059..3cb86d4 100644 --- a/docs/mvp.md +++ b/docs/mvp.md @@ -262,10 +262,25 @@ internal Compono-authored stage-4 providers (`docs/adr/0020-composition-configuration-rules.md`). Deliberately not designed in Milestone 3, since it had no real consumer there. +Design: [ADR-0024](adr/0024-public-provider-extensibility-model.md) (the core +public provider contract — `ICompositionValueProvider`, registration into stages +5/6, diagnostics identity — reusable by Milestone 6 without a redesign), +[ADR-0025](adr/0025-compono-nsubstitute-package-design.md) (`Compono.NSubstitute` +package: substitutable-shape rules including delegate types, `NSubstituteOptions`, +diagnostics). **ADR-0024's core engine extension point is implemented +(PLAN-0005 Phase 0)** — `builder.AddSemanticProvider(...)`/ +`builder.AddTestDoubleProvider(...)` are real, tested public API today. +**`Compono.NSubstitute` itself (ADR-0025) is not yet implemented** — see +[PLAN-0005](plans/0005-milestone-5-nsubstitute-integration.md) for the +phase-by-phase tracker; this milestone's own exit criteria aren't met until +that package ships. + ### Scope - Test-double provider contract - Interface substitutes +- Delegate substitutes (added during ADR-0025's design — NSubstitute supports + this natively at negligible extra cost; not in this bullet list originally) - Optional abstract-class substitutes - Shared substitute reuse - Integration-specific configuration diff --git a/docs/plans/0005-milestone-5-nsubstitute-integration.md b/docs/plans/0005-milestone-5-nsubstitute-integration.md new file mode 100644 index 0000000..3a12435 --- /dev/null +++ b/docs/plans/0005-milestone-5-nsubstitute-integration.md @@ -0,0 +1,395 @@ +# [PLAN-0005] Milestone 5: NSubstitute Integration + +**Status:** In Progress + +**Implements:** [ADR-0024](../adr/0024-public-provider-extensibility-model.md) +(core public provider extensibility: `ICompositionValueProvider`, +`CompositionProviderRequest`/`CompositionProviderResult`, +`AddSemanticProvider`/`AddTestDoubleProvider`, the `PublicProviderAdapter`/ +`ProviderType` diagnostics-identity mechanism), [ADR-0025](../adr/0025-compono-nsubstitute-package-design.md) +(`Compono.NSubstitute` package: `NSubstituteProvider`, `NSubstituteOptions`, +`UseNSubstitute`, substitutable-shape rules, diagnostics) + +**Note:** both ADRs are `Accepted` as of the design review that produced this +plan (2026-07-31) — implementation may begin. + +## Goal + +```csharp +var composer = Composer.Create(builder => builder.UseNSubstitute()); + +[Theory] +[Compose] +public async Task Saves_order( + [Shared] IOrderRepository repository, + CreateOrderHandler handler, + CreateOrder command) +{ + await handler.Handle(command); + + await repository.Received(1).SaveAsync(Arg.Any(), Arg.Any()); +} +``` + +runs end-to-end: `repository` is a real NSubstitute substitute, reused as the +exact same instance inside `handler`'s own composed `IOrderRepository` +constructor parameter; a request for an unsubstitutable type (a sealed +concrete class) still produces the engine's existing clear diagnostic naming +the type and path; no manual `Substitute.For()` call appears anywhere in +the test. + +## Scope + +Per ADR-0024/ADR-0025's Decision Outcomes: + +- New core `Compono` surface: `ICompositionValueProvider`, + `CompositionProviderRequest`, `CompositionProviderResult`, + `CompositionBuilder.AddSemanticProvider`/`AddTestDoubleProvider`, the + internal `PublicProviderAdapter`, `ICompositionProvider.ProviderType` + (default-interface member), and threading `CompositionConfiguration + .SemanticProviders`/`TestDoubleProviders` through `Composer.Create`/ + `CreateMany`/`CreateRow` into `CompositionContext`'s already-existing + (always-empty-until-now) `_semanticProviders`/`_testDoubleProviders` + fields. +- New `Compono.NSubstitute` package: `NSubstituteProvider`, + `NSubstituteOptions`, `CompositionBuilderExtensions.UseNSubstitute`. +- New test project: `test/Compono.NSubstitute.Tests`. +- Doc updates: `docs/mvp.md`, `docs/architecture.md`, `docs/public-api.md`. + +Explicitly deferred/non-goals — see ADR-0024/ADR-0025's own Deferred +Decisions/Non-goals: + +- Recursive auto-configuration of substitute members. +- NSubstitute API wrappers (`Arg`/`Received`/`Returns` stay NSubstitute's own + surface). +- A delegate-based convenience overload for lightweight ad hoc custom + providers (ADR-0024's contract-shape Pros/Cons notes this as a possible + future addition, not designed here). +- Wrapping a provider's own thrown exception into a diagnostic-carrying + `CompositionException` (ADR-0024's Provider Failure Semantics — an accepted + asymmetry, not solved here). +- `Compono.Bogus`/Milestone 6 itself — ADR-0024 validates the contract against + a Bogus sketch but does not build it. +- A `Compono.Benchmarks` entry for a substitute-composed graph (nice-to-have, + tracked as an open item below, not required for this plan's exit criteria). + +## Phases + +Each phase ships as its own PR, per `design-decisions.md`'s phase rule. + +### Phase 0: Core engine extension point (ADR-0024) + +**Status:** Done + +- [x] `CompositionProviderRequest` (public readonly struct): + `RequestedType`/`DeclaringType`/`Name`/`Nullability`. +- [x] `CompositionProviderResult` (public readonly struct): `NotHandled` + static property, `Handled(object? value)` static factory, internal + `IsHandled`/`Value`. +- [x] `ICompositionValueProvider` (public interface): `TryProvide(in + CompositionProviderRequest, ICompositionContext)`. +- [x] `ICompositionProvider.ProviderType` default-interface member + (`=> GetType()`); `PublicProviderAdapter : ICompositionProvider` + overriding it to report the wrapped provider's real type. +- [x] `CompositionContext.TryProviders`/`InvokeFactory`'s `provider` + parameter passthrough updated to read `candidate.ProviderType` + instead of `candidate.GetType()`. +- [x] `CompositionBuilder.AddSemanticProvider(ICompositionValueProvider)`/ + `AddTestDoubleProvider(ICompositionValueProvider)`: accumulate into + ordered lists, same shape as existing rule-builder accumulators. +- [x] `CompositionBuilder.Build()`: wrap each accumulated provider (in + registration order) into a `PublicProviderAdapter`, assign to two new + `CompositionConfiguration` fields (`SemanticProviders`, + `TestDoubleProviders`, both `IReadOnlyList`, + defaulting to `[]`). +- [x] `Composer.Create`/`CreateMany`/`ComposeMany`/`CreateRow`: + thread `_configuration.SemanticProviders`/`TestDoubleProviders` + through to `CompositionContext`'s existing constructor parameters of + the same name (currently always passed `[]`) — no `CompositionContext` + signature change, only what's passed in at each of these four call + sites. +- [x] `Compono.Tests` coverage for this API in isolation, before any + `Compono.NSubstitute` code exists (`testing.md`'s "verifying a new + public entry point" rule): a hand-written `ICompositionValueProvider` + test double registered via each of `AddSemanticProvider`/ + `AddTestDoubleProvider`, proving stage placement (5 vs. 6), ordering + (registration order, first match wins), `NotHandled` falling through + to stage 7/8, `ProviderAttempt.Provider` naming the real provider type + through the adapter (not `PublicProviderAdapter` itself), a + `[Shared]`-equivalent request's successful public-provider result + reaching scope and being reused by a later request, and a thrown + exception from a public provider propagating uncaught (per ADR-0024's + Provider Failure Semantics). + +### Phase 1: `Compono.NSubstitute` package (ADR-0025) + +**Status:** Not Started + +- [ ] New `src/Compono.NSubstitute/Compono.NSubstitute.csproj` (matching + `Compono.XunitV3.csproj`'s TFM/packaging shape — `PackageReference` to + `Compono` + `NSubstitute`, `PrivateAssets="none"` on the `Compono` + reference per PLAN-0004 Phase 3's real packaging-bug lesson). +- [ ] `NSubstituteOptions`: `SubstituteAbstractClasses` (`bool`, default + `true`). +- [ ] `NSubstituteProvider : ICompositionValueProvider`: the + `IsSubstitutable` static check (interface / delegate / conditionally + unsealed-abstract-class), `Substitute.For(Type[], object[])` call for + a match, `NotHandled` otherwise, no `context.Resolve()` call + anywhere in the type. Per ADR-0025 Amendment 1: the constructor + **snapshots** `options.SubstituteAbstractClasses` into a `private + readonly bool` field — it must never store the `NSubstituteOptions` + reference itself, so a caller mutating that instance after + registration can't change an already-registered provider's behavior + (the guarantee [ADR-0024](../adr/0024-public-provider-extensibility-model.md) + commits to: immutable, reusable provider registration). Delegate + matching uses `requestedType.IsSubclassOf(typeof(MulticastDelegate))`, + **not** `typeof(Delegate).IsAssignableFrom(requestedType)` — the + latter wrongly matches the non-substitutable `Delegate`/ + `MulticastDelegate` base types themselves. +- [ ] `CompositionBuilderExtensions.UseNSubstitute()`/ + `UseNSubstitute(Action)` extension methods + (C# 14 extension-block syntax, per `coding-standards.md`). + +### Phase 2: Test suites and verification + +**Status:** Not Started + +- [ ] `test/Compono.NSubstitute.Tests`: `IsSubstitutable` unit coverage + (interface / delegate / unsealed abstract class with the option on / + unsealed abstract class with the option off / sealed class / struct / + `string`); `NSubstituteProvider.TryProvide` returns a real NSubstitute + substitute for each positive case (assert via `object is + ICallRouterProvider`/NSubstitute's own "is this a substitute" check, + not a hand-rolled heuristic); `UseNSubstitute()`/ + `UseNSubstitute(configure)` wiring a working `NSubstituteProvider` + into a real `Composer`. +- [ ] **ADR-0025 Amendment 1 regression coverage:** + - Negative `IsSubstitutable` cases for `typeof(Delegate)` and + `typeof(MulticastDelegate)` themselves (the framework base types), + alongside a positive case for a real custom `delegate` type — the + exact distinction `IsSubclassOf(typeof(MulticastDelegate))` exists + to draw. + - A mutation test: construct `NSubstituteOptions`, pass it to + `UseNSubstitute(...)` (or construct `NSubstituteProvider` directly), + then mutate `SubstituteAbstractClasses` on that same instance + afterward, and assert the already-registered provider's behavior is + unaffected — proving the snapshot, not just that it compiles. +- [ ] End-to-end composition tests against a real `Composer`: an interface + parameter composes as a substitute; an abstract class composes as a + substitute only when the option allows it (and produces the engine's + ordinary "could not satisfy" diagnostic, naming the type, when it + doesn't); a delegate parameter composes as a substitute; a `[Shared]` + substitute (via `CompositionRow`) is reused by a later nested + constructor parameter of the same type — the `[Shared] IRepository` + shape from this plan's own Goal section, run for real, not just + asserted against a hand-written fake provider (Phase 0 already covers + that in isolation). +- [ ] An API-surface/approval test locking `Compono.NSubstitute`'s public + shape (`NSubstituteProvider`, `NSubstituteOptions`, + `CompositionBuilderExtensions`, and nothing else), matching + `Compono.XunitV3.Tests`' existing pattern (PLAN-0004 Phase 3). +- [ ] A real end-to-end run through `test/Compono.XunitV3.SampleTests` (or a + new sibling sample project) proving `UseNSubstitute()` composes + correctly under a real xUnit v3 theory, not just `Compono.NSubstitute.Tests`' + own direct `Composer` calls — matching PLAN-0004 Phase 3's real- + packaged-consumer testing strategy, since that's exactly where + PLAN-0004 caught a real packaging bug (`PrivateAssets` on the + `Compono` `ProjectReference`) that a `ProjectReference`-only build + couldn't surface. + +### Phase 3: Docs and cleanup + +**Status:** Not Started + +- [x] `docs/architecture.md`: Resolution Pipeline stage 5/6 rows, the + stages-4/5/6/7 summary paragraph, and the "Public provider + extensibility" Open Architectural Decisions entry all updated to + stop claiming the core mechanism is "not yet implemented"/stages + 5/6 are unconditionally "empty" — done early, in response to PR #28 + review (Codex, P2): the design-phase wording became stale the + moment Phase 0 merged. Still accurately says `Compono.NSubstitute` + itself doesn't exist yet. +- [ ] `docs/architecture.md`: Providers section still needs the + public/internal provider split write-up (`ICompositionValueProvider` + alongside the existing internal `ICompositionProvider` sketch); a + Package Boundaries entry for `Compono.NSubstitute`'s real `Owns` + list — both deferred to this phase since they're additive + documentation, not corrections of a false claim. +- [x] `docs/mvp.md` Milestone 5 section: links ADR-0024/ADR-0025/PLAN-0005, + states ADR-0024's core mechanism is implemented (PLAN-0005 Phase 0) + versus `Compono.NSubstitute` itself not yet — done early, same PR + #28 review round. Delegate substitution already in the scope list + from the design-phase draft. +- [ ] `docs/mvp.md`: mark this milestone's exit criteria met once + `Compono.NSubstitute` actually ships (Phase 1/2) — still open. +- [x] `docs/public-api.md`: Provider Extensibility section now says + "Implemented" rather than "design target" — done early, same PR #28 + review round. NSubstitute Integration section correctly still says + "not yet implemented." +- [x] `docs/public-api.md`: Naming Vocabulary gains `ICompositionValueProvider` + — already added during the design phase. +- [x] `docs/adr/README.md`/`docs/plans/README.md` index rows (already added + alongside the ADRs/this plan, during the design phase). + +## Critical Files + +- `src/Compono/ICompositionValueProvider.cs`, + `CompositionProviderRequest.cs`, `CompositionProviderResult.cs` (new) — + Phase 0. +- `src/Compono/ICompositionProvider.cs`, + `src/Compono/Providers/PublicProviderAdapter.cs` (new), + `CompositionContext.cs`, `CompositionBuilder.cs`, + `CompositionConfiguration.cs`, `Composer.cs` — Phase 0. +- `src/Compono.NSubstitute/` (new project) — `NSubstituteProvider.cs`, + `NSubstituteOptions.cs`, `CompositionBuilderExtensions.cs` — Phase 1. +- `test/Compono.NSubstitute.Tests/` (new project) — Phase 2. +- `docs/mvp.md`, `docs/architecture.md`, `docs/public-api.md` — Phase 3. + +## Test Plan + +Matches `testing.md`'s existing conventions (xUnit v3 on MTP v2, +Arrange-Act-Assert, fixed-seed determinism assertions where relevant, one +test project per `src` project). Per `testing.md`'s "verifying a new public +entry point" rule, `ICompositionValueProvider`/`AddSemanticProvider`/ +`AddTestDoubleProvider` need their own isolated `Compono.Tests` coverage +(Phase 0) with a hand-written test double, independent of +`Compono.NSubstitute` — the same "core entry point tested in isolation +before the package that will really use it exists" pattern PLAN-0004 Phase 0 +already established for `CompositionRow`. `Compono.NSubstitute.Tests` +(Phase 2) then covers the package's own real behavior, plus one real-runner +proof (a sample xUnit v3 project, PLAN-0004 Phase 3's precedent) since that +specific testing shape is what caught a real packaging bug last milestone. + +## Breaking-Change Handling During Alpha + +Per [ADR-0024](../adr/0024-public-provider-extensibility-model.md)'s Alpha +Compatibility Policy: `Compono` is pre-MVP-completion alpha software, and +`ICompositionValueProvider`/`CompositionProviderRequest`/`CompositionProviderResult` +(and `Compono.NSubstitute`'s own public surface, per ADR-0025) are public but +provisional — Milestone 6 and Milestone 7 are expected to validate them further, +and a breaking change to any of them is allowed during alpha when real +integration/dogfooding evidence justifies it. This plan's own Phase 0/1 work is +the *first* shipment of that contract, not a breaking change to an already-shipped +one — nothing below applies to this plan's initial implementation. It applies to +**any future PR** (in this plan's later phases, or after `Status: Done`) that +changes the shape of a contract this plan already shipped. + +This repo's established breaking-change mechanism is Release Drafter +(`.github/release-drafter.yml`): its `version-resolver` maps the `breaking` (or +`major`) GitHub label directly to a major version bump, distinct from every other +category (`feat`/`fix`/`chore`/`refactor`/`deps`/`docs`/`ci`, which all resolve to +a minor or patch bump). There is currently no title/branch-pattern autolabeler rule +for `breaking` (unlike `feat`/`fix`/`docs`/etc., which autolabel from a +Conventional-Commit-style PR title prefix) — applying the `breaking` label to the +PR is a manual, deliberate step, which is itself the point: a breaking change to a +public contract should never be silently swept into a `feat`/`refactor` bump. + +A PR that changes an already-shipped piece of this plan's public surface during +alpha must: + +- Apply the `breaking` label to the PR (`.github/release-drafter.yml`'s + `version-resolver.major` category) so Release Drafter's next draft release + produces a major version bump, not a minor/patch one. +- Call out the breaking change explicitly in the PR description — what changed, + what a consumer needs to do differently, not just "see diff." +- Update the relevant ADR (a dated `## Amendment N (YYYY-MM-DD)` section per + `design-decisions.md`'s Amendment mechanic — never a silent edit to the ADR's + original Decision Outcome text), `docs/public-api.md`, and this plan's own Notes + section, all in the same PR — not as follow-up cleanup. +- No separate "migration notes" document exists in this repo yet + (`docs/mvp.md` is pre-1.0 and has never needed one) — until one does, the PR + description plus the ADR's Amendment section together are this repo's record of + the change; a future PR is free to introduce a dedicated migration-notes doc if + breaking changes during alpha turn out to be frequent enough to warrant one, but + that's not designed here. + +## Open Items + +- No `Compono.Benchmarks` entry for a substitute-composed graph is planned + as part of this plan's own exit criteria — worth adding once + `Compono.NSubstitute` ships, to characterize NSubstitute's own proxy- + generation cost against `docs/performance.md`'s existing baselines, but + not required to call this milestone done. +- The "NSubstitute itself throws for a superficially-matching type" residual + diagnostics gap (ADR-0025's Diagnostics section) is accepted, not solved, + in this plan. Revisit only if real usage surfaces this as an actual pain + point. + +## Notes + +**Phase 0 (Done):** + +- Implemented in the same branch/PR as the design docs, per explicit user + direction — Phase 1-3 remain separate PRs, per `design-decisions.md`'s + phase rule. +- The 3-arg `CompositionContext(seed, registrations, serviceProvider)` test + seam (used by `CompositionRegistrationTests`) also forwards into the + extended constructor chain and needed its own `semanticProviders: []`/ + `testDoubleProviders: []` update — not called out explicitly in the + plan's task wording (only the 5-arg/6-arg constructors were), but the + same mechanical change threaded one level further down the existing + constructor-forwarding chain. +- `CompositionValueProviderTests` (new file, + `test/Compono.Tests/CompositionValueProviderTests.cs`) covers the public + `Composer`-level surface (`AddSemanticProvider`/`AddTestDoubleProvider`, + stage precedence, registration order, `NotHandled` fallthrough, + diagnostics identity through the adapter, shared-scope reuse, uncaught + provider exceptions) — the shared-scope-reuse test uses the internal + `CompositionContext(...)` test seam directly (wrapping the test double in + `PublicProviderAdapter` explicitly, since that seam takes internal + `ICompositionProvider`, not the public `ICompositionValueProvider`) to + reach `ResolveSharedForTesting`, the same seam + `CompositionScopeTests`/`CompositionDiagnosticsTests` already use for + scope-level assertions with no public `[Shared]` entry point available at + the `Compono` core layer. +- Full suite green: `Compono.Tests` 408/408 (204 × 2 TFMs — 196 pre-existing + + 8 new), `Compono.Generators.Tests` 166/166 (unaffected), + `Compono.XunitV3.Tests` 94/94 (unaffected) — `dotnet build`/`dotnet test` + against the whole solution, not just the new test file. +- **PR #28 review (Codex, two P1 findings) caught a real gap in this + phase's first pass, fixed before merge — see ADR-0024 Amendment 1 for + the full account.** A provider calling the descriptor-less + `context.Resolve()` threw unconditionally (no manual-resolve frame + was ever pushed for public-provider dispatch), and a provider recursing + on its own requested type had no cycle protection at all and ran until + `StackOverflowException`. Fixed with a new internal + `CompositionContext.InvokeProvider` method — structurally parallel to + `InvokeFactory` but reentrance-keyed on `(provider instance, requested + type)` rather than delegate identity, and deliberately **not** reusing + `InvokeFactory`'s exception-wrapping catch block, to keep ADR-0024's + Provider Failure Semantics decision (uncaught propagation) intact. + `PublicProviderAdapter` now carries its own `PipelineStage` (set at + construction in `CompositionBuilder.Build()`) so `InvokeProvider`'s + reentrance-failure trace entry is recorded against the right stage. + Two new regression tests added; the pre-existing + `ThrownException_FromAPublicProvider_PropagatesUncaught` test continues + to pass unchanged. Full suite re-verified green after the fix: 672/672 + across the whole solution (`Compono.Tests` 412/412 = 206 × 2 TFMs). +- **A second PR #28 review round (Codex, two P2 findings) caught doc/API + wording that went stale the moment the fixes above merged.** (1) + `docs/architecture.md`/`mvp.md`/`public-api.md` still described the + provider extension point as "design only"/stages 5/6 as unconditionally + "empty" — no longer true for the core mechanism, only for + `Compono.NSubstitute` itself; pulled forward from this plan's own Phase + 3 task list rather than left contradicting the code until that phase + lands (see Phase 3's task list above for exactly what was pulled + forward versus what's still open there). (2) + `ICompositionContext.Resolve()`'s public XML doc and its + `InvalidOperationException` message still described a factory-only + contract, not mentioning that a provider's `TryProvide` (via the + `InvokeProvider` fix above) is now also a valid caller — updated both, + plus the same "factory-only" framing on `PathSegment.ManualResolve`/ + `CompositionRequestKind.ManualResolve`'s XML docs for consistency. Full + suite re-verified green: 672/672. + +[ADR-0024](../adr/0024-public-provider-extensibility-model.md)/ +[ADR-0025](../adr/0025-compono-nsubstitute-package-design.md) reached +`Accepted` on 2026-07-31, after design review confirmed the provider +contract should be scoped to the architectural guarantees listed in +ADR-0024's Decision Outcome (named interface, small decoupled request/ +result contract, deterministic stage placement/order, isolation from +internal engine types, logical-provider diagnostics identity, immutable +reusable registration, `NotHandled`/`Handled` semantics) rather than +over-designed for hypothetical post-alpha stability — see ADR-0024's Alpha +Compatibility Policy. No task below is checked off yet; implementation has +not started. diff --git a/docs/plans/README.md b/docs/plans/README.md index 3956177..8a4137d 100644 --- a/docs/plans/README.md +++ b/docs/plans/README.md @@ -47,3 +47,4 @@ one. This file is just the mechanics: numbering, status, and the index. | [0002](0002-milestone-2-core-composition-engine.md) | Milestone 2: Core Composition Engine | Done | | [0003](0003-milestone-3-profiles-and-configuration.md) | Milestone 3: Profiles and Configuration | Done | | [0004](0004-milestone-4-xunit-integration.md) | Milestone 4: xUnit v3 Integration | Done | +| [0005](0005-milestone-5-nsubstitute-integration.md) | Milestone 5: NSubstitute Integration | Not Started | diff --git a/docs/public-api.md b/docs/public-api.md index 26d4e5a..18e66c7 100644 --- a/docs/public-api.md +++ b/docs/public-api.md @@ -389,8 +389,43 @@ builder.For() Correlation syntax is a design goal, not an MVP commitment. +## Provider Extensibility + +Implemented, per [ADR-0024](adr/0024-public-provider-extensibility-model.md) — +PLAN-0005 Phase 0, a Milestone 5 deliverable (see +[PLAN-0005](plans/0005-milestone-5-nsubstitute-integration.md) for phase status). +An integration package (or a consumer's own code) contributes open-ended, +pattern-matching composition logic to pipeline stage 5 (semantic value providers) +or stage 6 (test-double providers) — the cases a closed-set `.For()` rule can't +express, like "any interface type": + +```csharp +public interface ICompositionValueProvider +{ + CompositionProviderResult TryProvide(in CompositionProviderRequest request, ICompositionContext context); +} + +builder.AddSemanticProvider(new MySemanticProvider()); +builder.AddTestDoubleProvider(new MyTestDoubleProvider()); +``` + +`CompositionProviderRequest` exposes `RequestedType`/`DeclaringType`/`Name`/ +`Nullability` — enough to match "any interface" (`RequestedType.IsInterface`) or +"any member literally named `Email`" (`Name == "Email"`) without exposing the +engine's own internal request/path types. A provider returns +`CompositionProviderResult.NotHandled` for anything it doesn't apply to (so a later +provider or pipeline stage still gets a chance) or `CompositionProviderResult.Handled(value)`. +`Compono.NSubstitute`'s `UseNSubstitute()` (below) is the first real consumer of +this contract; `Compono.Bogus`'s future `UseBogus()` is expected to use the exact +same interface, registered via `AddSemanticProvider` instead of +`AddTestDoubleProvider`. + ## NSubstitute Integration +Design target, resolved (not yet implemented) by +[ADR-0025](adr/0025-compono-nsubstitute-package-design.md), built on the Provider +Extensibility contract above. + Activation: ```csharp @@ -400,9 +435,14 @@ builder.UseNSubstitute(); Default behavior: - Compose interfaces as substitutes -- Optionally compose abstract classes -- Reuse substitutes through shared scope -- Avoid automatic recursive member configuration in the MVP +- Compose delegate types as substitutes +- Optionally compose abstract classes (on by default; `SubstituteAbstractClasses`) +- Reuse substitutes through shared scope (falls out of the engine's existing + `[Shared]`/scope mechanism — `Compono.NSubstitute` contributes no code toward + this specifically) +- Avoid automatic recursive member configuration in the MVP — a composed + substitute is exactly what `Substitute.For()` would produce; its `Returns`/ + `Received` configuration stays the consumer's own test-body concern NSubstitute-specific configuration belongs in the integration package: @@ -498,7 +538,10 @@ Preferred concepts: - CompositionPlan: generated construction logic - CompositionScope: shared-instance lifetime - ICompositionProfile: reusable configuration, applied by name -- CompositionProvider: extension point +- CompositionProvider: extension point (internal, engine-owned) +- ICompositionValueProvider: the public extension point stages 5/6 expose to + integration packages — see Provider Extensibility, resolved by + [ADR-0024](adr/0024-public-provider-extensibility-model.md) - Shared: reuse within a scope ## API Design Rules diff --git a/src/Compono/Composer.cs b/src/Compono/Composer.cs index 0a7320f..f6e997f 100644 --- a/src/Compono/Composer.cs +++ b/src/Compono/Composer.cs @@ -64,6 +64,8 @@ public T Create() _configuration.Registrations, _configuration.ServiceProvider, _configuration.Rules, + _configuration.SemanticProviders, + _configuration.TestDoubleProviders, _configuration.CollectionSizePolicy); return context.ResolveRoot(); } @@ -95,6 +97,8 @@ public IReadOnlyList CreateMany(int count) => ComposeMany( _configuration.Registrations, _configuration.ServiceProvider, _configuration.Rules, + _configuration.SemanticProviders, + _configuration.TestDoubleProviders, _configuration.CollectionSizePolicy); /// @@ -118,6 +122,8 @@ public CompositionRow CreateRow(Type declaringType) _configuration.Registrations, _configuration.ServiceProvider, _configuration.Rules, + _configuration.SemanticProviders, + _configuration.TestDoubleProviders, _configuration.CollectionSizePolicy, declaringType); return new CompositionRow(context, unchecked((int)seed.Value)); @@ -141,7 +147,7 @@ internal static T CreateRootForTesting(CompositionSeed seed) /// seed-derivation contract, bypassing configuration entirely. /// internal static IReadOnlyList CreateManyForTesting(int count, CompositionSeed seed) => - ComposeMany(count, seed, CompositionRegistrations.Empty, serviceProvider: null, rules: [], CollectionSizePolicy.Empty); + ComposeMany(count, seed, CompositionRegistrations.Empty, serviceProvider: null, rules: [], semanticProviders: [], testDoubleProviders: [], CollectionSizePolicy.Empty); // Shared by the public CreateMany(count) (this composer's configured/generated batch seed and // configuration) and CreateManyForTesting(count, seed) (an explicit seed, empty configuration) - @@ -152,6 +158,8 @@ private static IReadOnlyList ComposeMany( CompositionRegistrations registrations, IServiceProvider? serviceProvider, IReadOnlyList rules, + IReadOnlyList semanticProviders, + IReadOnlyList testDoubleProviders, CollectionSizePolicy collectionSizePolicy) { ArgumentOutOfRangeException.ThrowIfNegative(count); @@ -164,7 +172,7 @@ private static IReadOnlyList ComposeMany( for (var i = 0; i < count; i++) { var itemSeed = batchSeed.Fork(i.ToString(CultureInfo.InvariantCulture)); - var context = new CompositionContext(itemSeed, registrations, serviceProvider, rules, collectionSizePolicy); + var context = new CompositionContext(itemSeed, registrations, serviceProvider, rules, semanticProviders, testDoubleProviders, collectionSizePolicy); results.Add(context.ResolveRoot()); } diff --git a/src/Compono/CompositionBuilder.cs b/src/Compono/CompositionBuilder.cs index 56f39e4..fd7f701 100644 --- a/src/Compono/CompositionBuilder.cs +++ b/src/Compono/CompositionBuilder.cs @@ -26,6 +26,8 @@ public sealed class CompositionBuilder private readonly Dictionary<(Type DeclaringType, string MemberName), (Type MemberType, Func Factory)> _memberRuleFactories = new(); private readonly Dictionary<(Type DeclaringType, string MemberName), List> _memberCollectionSizeSources = new(); private readonly Dictionary<(Type DeclaringType, string MemberName), (Type MemberType, int Size)> _memberCollectionSizes = new(); + private readonly List _semanticProviders = []; + private readonly List _testDoubleProviders = []; private readonly Stack _applyingProfiles = new(); internal CompositionBuilder() @@ -160,6 +162,42 @@ public CompositionBuilder AddProfile(ICompositionProfile profile) /// The type to configure a rule for. public CompositionTypeRuleBuilder For() => new(this); + /// + /// Registers into pipeline stage 5 (semantic value providers) - the + /// open-ended, pattern-matching extension point an integration package (e.g. a future + /// Compono.Bogus) uses instead of a closed-set rule. See + /// docs/adr/0024-public-provider-extensibility-model.md. + /// + /// + /// Multiple providers may be registered - tried in registration order, same as every other + /// extensible pipeline stage; the first to report a value wins. + /// + /// The provider to register. + public CompositionBuilder AddSemanticProvider(ICompositionValueProvider provider) + { + ArgumentNullException.ThrowIfNull(provider); + _semanticProviders.Add(provider); + return this; + } + + /// + /// Registers into pipeline stage 6 (test-double providers) - the + /// open-ended, pattern-matching extension point an integration package (e.g. + /// Compono.NSubstitute) uses instead of a closed-set rule. See + /// docs/adr/0024-public-provider-extensibility-model.md. + /// + /// + /// Multiple providers may be registered - tried in registration order, same as every other + /// extensible pipeline stage; the first to report a value wins. + /// + /// The provider to register. + public CompositionBuilder AddTestDoubleProvider(ICompositionValueProvider provider) + { + ArgumentNullException.ThrowIfNull(provider); + _testDoubleProviders.Add(provider); + return this; + } + /// /// Sets this composer's global default collection size - the size a generated collection plan /// builds when no member-scoped .For<T>().Member(x => x.Y).WithCollectionSize(...) @@ -232,6 +270,15 @@ .. _memberRuleFactories.Select(entry => (ICompositionProvider)new MemberRuleProv .. _typeRuleFactories.Select(entry => (ICompositionProvider)new TypeRuleProvider(entry.Key, entry.Value)), ]; + // Public-provider registrations compile into PublicProviderAdapter, the same "public builder + // data -> internal ICompositionProvider" shape the rules above already use for .For() - + // per docs/adr/0024-public-provider-extensibility-model.md. Registration order is preserved; + // no reordering/deduplication happens here. + IReadOnlyList semanticProviders = + [.. _semanticProviders.Select(provider => (ICompositionProvider)new PublicProviderAdapter(provider, PipelineStage.SemanticProvider))]; + IReadOnlyList testDoubleProviders = + [.. _testDoubleProviders.Select(provider => (ICompositionProvider)new PublicProviderAdapter(provider, PipelineStage.TestDoubleProvider))]; + return new CompositionConfiguration { Seed = _seed.HasValue ? _seed.Value : null, @@ -240,6 +287,8 @@ .. _typeRuleFactories.Select(entry => (ICompositionProvider)new TypeRuleProvider : new CompositionRegistrations(_registrationFactories), ServiceProvider = _serviceProvider.HasValue ? _serviceProvider.Value : null, Rules = rules, + SemanticProviders = semanticProviders, + TestDoubleProviders = testDoubleProviders, CollectionSizePolicy = _memberCollectionSizes.Count == 0 && !_globalCollectionSize.HasValue ? CollectionSizePolicy.Empty : new CollectionSizePolicy(_globalCollectionSize.HasValue ? _globalCollectionSize.Value : null, _memberCollectionSizes), diff --git a/src/Compono/CompositionConfiguration.cs b/src/Compono/CompositionConfiguration.cs index e1a702f..e535589 100644 --- a/src/Compono/CompositionConfiguration.cs +++ b/src/Compono/CompositionConfiguration.cs @@ -39,6 +39,22 @@ internal sealed class CompositionConfiguration /// internal required IReadOnlyList Rules { get; init; } + /// + /// This composer's compiled pipeline stage-5 semantic value providers, in registration order - + /// empty if AddSemanticProvider was never called. Each entry wraps one registered + /// in a . See + /// docs/adr/0024-public-provider-extensibility-model.md. + /// + internal required IReadOnlyList SemanticProviders { get; init; } + + /// + /// This composer's compiled pipeline stage-6 test-double providers, in registration order - empty + /// if AddTestDoubleProvider was never called. Each entry wraps one registered + /// in a . See + /// docs/adr/0024-public-provider-extensibility-model.md. + /// + internal required IReadOnlyList TestDoubleProviders { get; init; } + /// /// This composer's collection-size configuration - /// if neither WithCollectionSize nor a member-scoped override was ever called. diff --git a/src/Compono/CompositionContext.cs b/src/Compono/CompositionContext.cs index 5d96dae..d5375a5 100644 --- a/src/Compono/CompositionContext.cs +++ b/src/Compono/CompositionContext.cs @@ -28,6 +28,7 @@ internal sealed class CompositionContext : ICompositionContext private readonly CompositionScope _scope = new(); private readonly List _activeFrames = []; private readonly List> _activeFactories = []; + private readonly List<(ICompositionValueProvider Provider, Type RequestedType)> _activeProviderRequests = []; private readonly List _manualResolveFrames = []; private readonly CompositionTraceBuffer _trace = new(); private readonly IReadOnlyList _configurationRuleProviders; @@ -84,14 +85,14 @@ internal CompositionContext(CompositionSeed seed, CompositionRegistrations regis /// Compono.Tests uses to exercise stage 3 directly. /// internal CompositionContext(CompositionSeed seed, CompositionRegistrations registrations, IServiceProvider? serviceProvider) - : this(seed, registrations, serviceProvider, configurationRuleProviders: [], CollectionSizePolicy.Empty) + : this(seed, registrations, serviceProvider, configurationRuleProviders: [], semanticProviders: [], testDoubleProviders: [], CollectionSizePolicy.Empty) { } /// /// Creates a with the real stage-7 built-in providers, the given /// explicit stage-3 registrations and configured IServiceProvider, the given compiled - /// stage-4 configuration-rule providers and collection-size policy, and the given explicit root + /// stage-4/5/6 provider lists and collection-size policy, and the given explicit root /// seed - the shape / use once /// a has been configured. /// @@ -100,8 +101,10 @@ internal CompositionContext( CompositionRegistrations registrations, IServiceProvider? serviceProvider, IReadOnlyList configurationRuleProviders, + IReadOnlyList semanticProviders, + IReadOnlyList testDoubleProviders, CollectionSizePolicy collectionSizePolicy) - : this(seed, registrations, serviceProvider, configurationRuleProviders, semanticProviders: [], testDoubleProviders: [], builtInProviders: BuiltInProviders.Default, collectionSizePolicy) + : this(seed, registrations, serviceProvider, configurationRuleProviders, semanticProviders, testDoubleProviders, builtInProviders: BuiltInProviders.Default, collectionSizePolicy) { } @@ -119,9 +122,11 @@ internal CompositionContext( CompositionRegistrations registrations, IServiceProvider? serviceProvider, IReadOnlyList configurationRuleProviders, + IReadOnlyList semanticProviders, + IReadOnlyList testDoubleProviders, CollectionSizePolicy collectionSizePolicy, Type rootType) - : this(seed, registrations, serviceProvider, configurationRuleProviders, collectionSizePolicy) + : this(seed, registrations, serviceProvider, configurationRuleProviders, semanticProviders, testDoubleProviders, collectionSizePolicy) { _path = CompositionPath.Root(rootType); _random = RandomSource.FromSeed(seed); @@ -247,7 +252,8 @@ public TValue Resolve() { throw new InvalidOperationException( $"{nameof(ICompositionContext)}.{nameof(Resolve)}<{typeof(TValue).Name}>() with no descriptor can only " + - "be called from inside a registration or configuration-rule factory."); + "be called from inside a registration or configuration-rule factory, or a public " + + $"{nameof(ICompositionValueProvider)}.{nameof(ICompositionValueProvider.TryProvide)} invocation."); } var frame = _manualResolveFrames[^1]; @@ -631,6 +637,69 @@ private string BuildFactoryReentranceMessage(Type requestedType) => "factory again, which is already in progress. Use a different registration/rule, or terminate the " + "recursion inside the factory itself (e.g. by returning a value that doesn't call Resolve() for this type)."; + // Invokes a public ICompositionValueProvider's TryProvide - the single point every stage-5/6 + // public-provider dispatch (PublicProviderAdapter) goes through, mirroring InvokeFactory's manual- + // resolve-frame push for stage-3/4 factories (per docs/adr/0019-registrations-and-service-provider-injection.md) + // so a provider can call context.Resolve() (the descriptor-less overload) to compose a nested + // value, exactly as ICompositionValueProvider's own contract promises. Unlike InvokeFactory, this + // method never catches or wraps an exception TryProvide throws - ADR-0024's Provider Failure + // Semantics commit to a public provider's own thrown exception propagating uncaught, exactly like + // any other ordinary stage-4/7 provider's TryCompose already does; reentrance below produces the + // engine's own diagnosed CompositionException, which is a distinct concern from wrapping a + // provider's unrelated thrown exception. + // + // Reentrance is keyed on (provider instance, requested type), not provider identity alone - + // unlike TypeRuleProvider/MemberRuleProvider, where one instance is compiled 1:1 for exactly one + // type (so "the same factory delegate re-entered" and "the same type re-entered" are equivalent by + // construction), one ICompositionValueProvider instance can legitimately handle many different + // types (e.g. "any interface"), including composing a different type as one of its own nested + // dependencies - keying on the provider alone would wrongly block that legitimate case. Only the + // exact same provider asked to resolve the exact same type it's already resolving is a real cycle + // (PR #28 review, Codex: recursing through the public descriptor overload previously ran until + // StackOverflowException instead of producing this diagnostic). + internal CompositionProviderResult InvokeProvider(ICompositionValueProvider provider, in CompositionProviderRequest request, PipelineStage stage, Type providerType) + { + var requestedType = request.RequestedType; + + if (IsProviderRequestActive(provider, requestedType)) + { + _trace.Record(stage, providerType, CompositionAttemptOutcome.Failure); + throw BuildException(requestedType, BuildProviderReentranceMessage(requestedType)); + } + + _activeProviderRequests.Add((provider, requestedType)); + _manualResolveFrames.Add(new ManualResolveFrame()); + try + { + return provider.TryProvide(in request, this); + } + finally + { + _manualResolveFrames.RemoveAt(_manualResolveFrames.Count - 1); + _activeProviderRequests.RemoveAt(_activeProviderRequests.Count - 1); + } + } + + // A plain indexed loop, same reasoning as IsFactoryActive above - avoids a per-call closure/ + // allocation on the composition hot path. Reference identity for the provider, ordinary type + // equality for the requested type. + private bool IsProviderRequestActive(ICompositionValueProvider provider, Type requestedType) + { + for (var i = 0; i < _activeProviderRequests.Count; i++) + { + if (ReferenceEquals(_activeProviderRequests[i].Provider, provider) && _activeProviderRequests[i].RequestedType == requestedType) + return true; + } + + return false; + } + + private string BuildProviderReentranceMessage(Type requestedType) => + $"Recursive provider request detected while composing " + + $"'{CompositionPath.FriendlyTypeName(requestedType)}': '{_path!.ToDisplayString()}' would ask the same " + + "provider to compose the same type again, which is already in progress. Use a registration or a shared " + + "value to terminate the recursion, or have the provider avoid resolving its own requested type recursively."; + // One mutable counter per active manual-resolve invocation frame - shared and incremented by // every descriptor-less Resolve() call made during that one factory invocation, per ADR-0019. private sealed class ManualResolveFrame @@ -671,18 +740,18 @@ private bool TryProviders( foreach (var candidate in providers) { var checkpoint = _trace.Checkpoint; - _trace.Record(stage, candidate.GetType(), CompositionAttemptOutcome.Pending); + _trace.Record(stage, candidate.ProviderType, CompositionAttemptOutcome.Pending); var result = candidate.TryCompose(request, this); _trace.Rewind(checkpoint); if (result is CompositionResult.Success success) { value = success.Value; - provider = candidate.GetType(); + provider = candidate.ProviderType; return true; } - _trace.Record(stage, candidate.GetType(), CompositionAttemptOutcome.NotHandled); + _trace.Record(stage, candidate.ProviderType, CompositionAttemptOutcome.NotHandled); } value = null; diff --git a/src/Compono/CompositionProviderRequest.cs b/src/Compono/CompositionProviderRequest.cs new file mode 100644 index 0000000..9c4cad6 --- /dev/null +++ b/src/Compono/CompositionProviderRequest.cs @@ -0,0 +1,53 @@ +namespace Compono; + +/// +/// A composition request, as seen by a public - decoupled +/// from the engine's own internal (no path, no shared-scope flag, +/// no pipeline plumbing a provider author has no legitimate use for). See +/// docs/adr/0024-public-provider-extensibility-model.md. +/// +public readonly struct CompositionProviderRequest +{ + /// Creates a . + /// The requested CLR type. + /// + /// The type whose constructor/required member declares this request, or + /// for a request with no member identity of its own. + /// + /// + /// The declaring constructor parameter/required member/test-method-parameter's own name, or + /// for a request with no name of its own. + /// + /// Whether the requesting parameter or member is nullable-annotated. + public CompositionProviderRequest(Type requestedType, Type? declaringType, string? name, Nullability nullability) + { + ArgumentNullException.ThrowIfNull(requestedType); + RequestedType = requestedType; + DeclaringType = declaringType; + Name = name; + Nullability = nullability; + } + + /// The requested CLR type. + public Type RequestedType { get; } + + /// + /// The type whose constructor/required member declares this request, or + /// for a request with no member identity (a collection element, a manual resolve, or the + /// composition root itself) - the same field/same semantics + /// already carries, per + /// docs/adr/0020-composition-configuration-rules.md. + /// + public Type? DeclaringType { get; } + + /// + /// The declaring constructor parameter/required member/test-method-parameter's own name, for + /// diagnostic display and name-based provider matching (e.g. a future Compono.Bogus + /// member-name convention) - for a request with no name of its own (a + /// collection element, dictionary key/value, or manual resolve). + /// + public string? Name { get; } + + /// Whether the requesting parameter or member is nullable-annotated. + public Nullability Nullability { get; } +} diff --git a/src/Compono/CompositionProviderResult.cs b/src/Compono/CompositionProviderResult.cs new file mode 100644 index 0000000..dcc5ef9 --- /dev/null +++ b/src/Compono/CompositionProviderResult.cs @@ -0,0 +1,33 @@ +namespace Compono; + +/// +/// What an reports for one . +/// +/// +/// Deliberately only two cases, mirroring the engine's own internal provider result contract: a +/// public provider can report that it doesn't apply, or that it produced a value - never a +/// stronger "failure." An unhandled exception a provider's own +/// implementation throws propagates uncaught, +/// exactly like an internal pipeline-stage provider's exception does today. See +/// docs/adr/0024-public-provider-extensibility-model.md. +/// +public readonly struct CompositionProviderResult +{ + private CompositionProviderResult(bool isHandled, object? value) + { + IsHandled = isHandled; + Value = value; + } + + /// The provider does not handle this request. + public static CompositionProviderResult NotHandled => default; + + /// The provider produced for this request. + public static CompositionProviderResult Handled(object? value) => new(isHandled: true, value); + + // Internal - a public provider constructs a result only through the two static members above, + // never by inspecting or round-tripping one it didn't just receive. + internal bool IsHandled { get; } + + internal object? Value { get; } +} diff --git a/src/Compono/CompositionRequestKind.cs b/src/Compono/CompositionRequestKind.cs index e1b0735..d62842d 100644 --- a/src/Compono/CompositionRequestKind.cs +++ b/src/Compono/CompositionRequestKind.cs @@ -26,8 +26,10 @@ public enum CompositionRequestKind /// /// The request is one descriptor-less call - /// made inside a registration or configuration-rule factory - never emitted by generated code, - /// per docs/adr/0019-registrations-and-service-provider-injection.md. + /// made inside a registration or configuration-rule factory, or a public + /// invocation - never emitted by generated + /// code, per docs/adr/0019-registrations-and-service-provider-injection.md and + /// docs/adr/0024-public-provider-extensibility-model.md. /// ManualResolve, diff --git a/src/Compono/ICompositionContext.cs b/src/Compono/ICompositionContext.cs index 1247aca..c73d13f 100644 --- a/src/Compono/ICompositionContext.cs +++ b/src/Compono/ICompositionContext.cs @@ -27,13 +27,15 @@ public interface ICompositionContext /// /// Resolves a value of type from inside a registration or - /// configuration-rule factory - the hand-written counterpart to + /// configuration-rule factory, or a public + /// invocation - the hand-written counterpart to /// , which only generated code - /// calls. Only valid while such a factory is actively being invoked. + /// calls. Only valid while one of those three is actively being invoked. /// /// The requested value's type. /// - /// No registration or configuration-rule factory is currently being invoked. + /// No registration/configuration-rule factory or public provider invocation is currently in + /// progress. /// /// /// No explicit value, shared value, registration, provider, or generated plan could satisfy the diff --git a/src/Compono/ICompositionProvider.cs b/src/Compono/ICompositionProvider.cs index d76d224..7f4a9f6 100644 --- a/src/Compono/ICompositionProvider.cs +++ b/src/Compono/ICompositionProvider.cs @@ -11,6 +11,16 @@ namespace Compono; /// internal interface ICompositionProvider { + /// + /// The concrete provider type diagnostics should report for an attempt this instance makes - + /// defaults to this instance's own runtime type. + /// overrides this to report the wrapped 's own concrete + /// type instead of the adapter's, so keeps naming the + /// real, meaningful provider for a public-provider attempt exactly as it already does for an + /// internal one. See docs/adr/0024-public-provider-extensibility-model.md. + /// + Type ProviderType => GetType(); + /// /// Attempts to compose a value for . /// diff --git a/src/Compono/ICompositionValueProvider.cs b/src/Compono/ICompositionValueProvider.cs new file mode 100644 index 0000000..09db3d9 --- /dev/null +++ b/src/Compono/ICompositionValueProvider.cs @@ -0,0 +1,38 @@ +namespace Compono; + +/// +/// A public extension point for pipeline stage 5 (semantic value providers) or stage 6 +/// (test-double providers) - open-ended, pattern-matching composition logic a closed-set +/// .For<T>() rule can't express ("any interface type," "any member named +/// Email"). Registered via or +/// - which method an integration's own +/// UseX() extension calls decides which stage a given instance participates in; the +/// interface itself is not stage-specific. See +/// docs/adr/0024-public-provider-extensibility-model.md. +/// +/// +/// An implementation must be safe to invoke repeatedly, including concurrently, once constructed - +/// a 's configuration (and every provider registered into it) is immutable +/// and reused across every composition call it ever serves, exactly like every other +/// builder-compiled piece of configuration (a .For<T>() rule, a registration factory). +/// +public interface ICompositionValueProvider +{ + /// + /// Attempts to produce a value for . Returns + /// for any request this provider doesn't + /// apply to, so a later provider or pipeline stage still gets a chance - never throws for an + /// expected non-match. + /// + /// The request to attempt. + /// + /// The active composition context - a provider may call context.Resolve<T>() to + /// compose part of its value from a nested request, exactly as an internal provider already may + /// (docs/architecture.md's Providers section). Asking this same provider to resolve the + /// exact same requested type again (a genuine cycle, not an ordinary nested request for a + /// different type) is detected and reported as a diagnosed + /// rather than recursing indefinitely - see docs/adr/0024-public-provider-extensibility-model.md's + /// Amendment 1. + /// + CompositionProviderResult TryProvide(in CompositionProviderRequest request, ICompositionContext context); +} diff --git a/src/Compono/PathSegment.cs b/src/Compono/PathSegment.cs index 1f6eddd..ef2c1eb 100644 --- a/src/Compono/PathSegment.cs +++ b/src/Compono/PathSegment.cs @@ -34,9 +34,11 @@ internal sealed record DictionaryValue(int Index) : PathSegment; /// /// One descriptor-less call made inside a - /// registration or configuration-rule factory, identified by its call sequence within that one - /// factory invocation - never by requested type. See - /// docs/adr/0019-registrations-and-service-provider-injection.md. + /// registration or configuration-rule factory, or a public + /// invocation, identified by its call sequence + /// within that one invocation - never by requested type. See + /// docs/adr/0019-registrations-and-service-provider-injection.md and + /// docs/adr/0024-public-provider-extensibility-model.md. /// internal sealed record ManualResolve(int Ordinal) : PathSegment; diff --git a/src/Compono/Providers/PublicProviderAdapter.cs b/src/Compono/Providers/PublicProviderAdapter.cs new file mode 100644 index 0000000..c06fc4e --- /dev/null +++ b/src/Compono/Providers/PublicProviderAdapter.cs @@ -0,0 +1,56 @@ +namespace Compono.Providers; + +/// +/// Wraps a public as an internal +/// so it can be dropped into pipeline stage 5/6's existing ordered-provider dispatch - the same +/// "compile public builder data into an internal provider" shape +/// docs/adr/0020-composition-configuration-rules.md's / +/// already use for .For<T>() rules. Constructed once per +/// registered provider, at time - never per request. See +/// docs/adr/0024-public-provider-extensibility-model.md. +/// +internal sealed class PublicProviderAdapter : ICompositionProvider +{ + private readonly ICompositionValueProvider _inner; + private readonly PipelineStage _stage; + + // stage is which pipeline stage this instance was registered into (SemanticProvider or + // TestDoubleProvider, per CompositionBuilder.Build) - carried alongside the wrapped provider so + // CompositionContext.InvokeProvider's own reentrance-failure trace entry (PR #28 review) is + // recorded against the correct stage, not guessed or omitted. + internal PublicProviderAdapter(ICompositionValueProvider inner, PipelineStage stage) + { + _inner = inner; + _stage = stage; + } + + /// + public Type ProviderType => _inner.GetType(); + + /// + public CompositionResult TryCompose(in CompositionRequest request, ICompositionContext context) + { + var publicRequest = new CompositionProviderRequest(request.RequestedType, request.DeclaringType, NameOf(request), request.Nullability); + + // Routed through InvokeProvider (not called directly) so TryProvide gets the same manual- + // resolve frame and reentrance guard TypeRuleProvider/MemberRuleProvider's own factories + // already get from InvokeFactory - see InvokeProvider's remarks for why a separate method, + // not a reuse of InvokeFactory itself (PR #28 review, Codex). + var result = ((CompositionContext)context).InvokeProvider(_inner, in publicRequest, _stage, ProviderType); + + return result.IsHandled + ? new CompositionResult.Success(result.Value) + : CompositionResult.NotHandled.Instance; + } + + // Only ConstructorParameter/RequiredMember/TestParameter segments carry a name at all - every + // other segment kind (collection element, dictionary key/value, manual resolve) has none, mirroring + // CompositionRequestDescriptor.DeclaringType's own "null means no member identity" contract. + private static string? NameOf(in CompositionRequest request) => request.Path.Segment switch + { + PathSegment.ConstructorParameter p => p.Name, + PathSegment.RequiredMember m => m.Name, + PathSegment.TestParameter t => t.Name, + _ => null, + }; +} diff --git a/test/Compono.Tests/CompositionValueProviderTests.cs b/test/Compono.Tests/CompositionValueProviderTests.cs new file mode 100644 index 0000000..05b6517 --- /dev/null +++ b/test/Compono.Tests/CompositionValueProviderTests.cs @@ -0,0 +1,221 @@ +namespace Compono.Tests; + +/// +/// Milestone 5 Phase 0's public provider extension point - +/// // +/// - exercised through the real +/// path, in isolation from Compono.NSubstitute (which doesn't exist +/// yet), per testing.md's "verifying a new public entry point" rule. See +/// docs/adr/0024-public-provider-extensibility-model.md. +/// +public sealed class CompositionValueProviderTests +{ + [Fact] + public void AddSemanticProvider_ParticipatesInStage5_AndSatisfiesTheRequest() + { + var composer = Composer.Create(builder => builder.AddSemanticProvider(new StubProvider(handles: true, value: "from-semantic"))); + + var result = composer.Create(); + + result.Value.Should().Be("from-semantic"); + } + + [Fact] + public void AddTestDoubleProvider_ParticipatesInStage6_AndSatisfiesTheRequest() + { + var composer = Composer.Create(builder => builder.AddTestDoubleProvider(new StubProvider(handles: true, value: "from-test-double"))); + + var result = composer.Create(); + + result.Value.Should().Be("from-test-double"); + } + + [Fact] + public void SemanticProvider_TakesPrecedenceOverTestDoubleProvider_ForTheSameRequest() + { + // Stage 5 (semantic) runs before stage 6 (test-double) - docs/architecture.md's fixed + // resolution order, unchanged by this milestone. + var composer = Composer.Create(builder => builder + .AddSemanticProvider(new StubProvider(handles: true, value: "from-semantic")) + .AddTestDoubleProvider(new StubProvider(handles: true, value: "from-test-double"))); + + var result = composer.Create(); + + result.Value.Should().Be("from-semantic"); + } + + [Fact] + public void MultipleProvidersInTheSameStage_AreTriedInRegistrationOrder() + { + var callOrder = new List(); + var composer = Composer.Create(builder => builder + .AddTestDoubleProvider(new RecordingProvider(callOrder, "first", handles: false)) + .AddTestDoubleProvider(new RecordingProvider(callOrder, "second", handles: true)) + .AddTestDoubleProvider(new RecordingProvider(callOrder, "third", handles: true))); + + composer.Create(); + + // "third" never runs - "second" already won. + callOrder.Should().Equal("first", "second"); + } + + [Fact] + public void NotHandled_FallsThroughToTheNextPipelineStage() + { + // No PlanCache is registered, so once every provider declines, this reaches stage 9's + // ordinary "nothing could satisfy this" failure - proving NotHandled doesn't terminate the + // pipeline early. + var composer = Composer.Create(builder => builder.AddTestDoubleProvider(new StubProvider(handles: false, value: null))); + + var act = () => composer.Create(); + + act.Should().Throw().WithMessage("*Widget*"); + } + + [Fact] + public void ProviderAttempt_NamesTheRealProviderType_NotThePublicProviderAdapter() + { + var provider = new StubProvider(handles: false, value: null); + var composer = Composer.Create(builder => builder.AddTestDoubleProvider(provider)); + + var act = () => composer.Create(); + + var diagnostic = act.Should().Throw().Which.Diagnostic; + + // The adapter that wraps a public ICompositionValueProvider must never leak its own type into + // diagnostics - ProviderAttempt.Provider has to keep naming the real, logical provider + // (ADR-0024's diagnostics-identity guarantee), exactly as it already does for an internal + // stage-4/7 provider. + diagnostic!.Trace.Should().Contain(attempt => + attempt.Stage == PipelineStage.TestDoubleProvider + && attempt.Provider == typeof(StubProvider) + && attempt.Outcome == CompositionAttemptOutcome.NotHandled); + diagnostic.Trace.Should().NotContain(attempt => attempt.Provider != null && attempt.Provider!.Name.Contains("Adapter")); + } + + [Fact] + public void SharedRequest_SatisfiedByAPublicProvider_IsReusedByALaterOrdinaryRequest() + { + // Mirrors ADR-0011/ADR-0021's existing shared-scope mechanism - a public provider's successful + // result for a shared request is stored and reused exactly like any other stage's, with zero + // new code (ADR-0024's "shared substitute reuse: no new mechanism" claim, verified directly). + var provider = new StubProvider(handles: true, value: "shared-value"); + var context = new CompositionContext( + configurationRuleProviders: [], + semanticProviders: [], + testDoubleProviders: [new Compono.Providers.PublicProviderAdapter(provider, PipelineStage.TestDoubleProvider)], + builtInProviders: []); + + var first = context.ResolveSharedForTesting(ordinal: 0, name: "a"); + var second = context.ResolveRoot(); + + second.Should().BeSameAs(first); + } + + [Fact] + public void ThrownException_FromAPublicProvider_PropagatesUncaught() + { + // Per ADR-0024's Provider Failure Semantics: an ordinary pipeline stage never wraps a + // provider's own thrown exception - a public provider is no exception to that, even though it + // reaches the pipeline through PublicProviderAdapter rather than implementing + // ICompositionProvider directly. + var composer = Composer.Create(builder => builder.AddTestDoubleProvider(new ThrowingProvider())); + + var act = () => composer.Create(); + + act.Should().Throw().WithMessage("boom"); + } + + [Fact] + public void Provider_CanComposeANestedValue_ViaTheDescriptorLessResolveOverload() + { + // Regression for PR #28 review (Codex, finding 1): a provider calling context.Resolve() + // (no descriptor) used to throw InvalidOperationException unconditionally, because + // PublicProviderAdapter never pushed the manual-resolve frame that overload requires - + // ICompositionValueProvider's own contract explicitly promises this works. NestedResolvingProvider + // also proves the (provider, requested type) reentrance key doesn't over-block: the same + // provider instance handles both Gadget and, while still "active" for Gadget, its nested + // Widget request too - a different type, so it isn't treated as a false cycle. + var composer = Composer.Create(builder => builder.AddTestDoubleProvider(new NestedResolvingProvider())); + + var result = composer.Create(); + + result.Inner.Value.Should().Be("from-nested-resolve"); + } + + [Fact] + public void Provider_RecursivelyResolvingItsOwnRequestedType_ThrowsADiagnosedException_NotStackOverflow() + { + // Regression for PR #28 review (Codex, finding 2): a provider that resolves its own requested + // type again (via the descriptor-based overload, which needs no manual-resolve frame) used to + // recurse until StackOverflowException instead of producing this diagnostic. + var composer = Composer.Create(builder => builder.AddTestDoubleProvider(new SelfRecursingProvider())); + + var act = () => composer.Create(); + + act.Should().Throw().WithMessage("*Recursive provider request*Loop*"); + } + + private sealed record Widget(string? Value = null); + + private sealed record Gadget(Widget Inner); + + private sealed record Loop; + + private sealed class StubProvider(bool handles, string? value) : ICompositionValueProvider + { + public CompositionProviderResult TryProvide(in CompositionProviderRequest request, ICompositionContext context) => + handles ? CompositionProviderResult.Handled(new Widget(value)) : CompositionProviderResult.NotHandled; + } + + private sealed class RecordingProvider(List callOrder, string name, bool handles) : ICompositionValueProvider + { + public CompositionProviderResult TryProvide(in CompositionProviderRequest request, ICompositionContext context) + { + callOrder.Add(name); + return handles ? CompositionProviderResult.Handled(new Widget()) : CompositionProviderResult.NotHandled; + } + } + + private sealed class ThrowingProvider : ICompositionValueProvider + { + public CompositionProviderResult TryProvide(in CompositionProviderRequest request, ICompositionContext context) => + throw new InvalidOperationException("boom"); + } + + // Handles two different types through one instance - Gadget composes by nested-resolving Widget + // (the descriptor-less overload), which this same provider also handles. + private sealed class NestedResolvingProvider : ICompositionValueProvider + { + public CompositionProviderResult TryProvide(in CompositionProviderRequest request, ICompositionContext context) + { + if (request.RequestedType == typeof(Gadget)) + { + var inner = context.Resolve(); + return CompositionProviderResult.Handled(new Gadget(inner)); + } + + if (request.RequestedType == typeof(Widget)) + return CompositionProviderResult.Handled(new Widget("from-nested-resolve")); + + return CompositionProviderResult.NotHandled; + } + } + + private sealed class SelfRecursingProvider : ICompositionValueProvider + { + public CompositionProviderResult TryProvide(in CompositionProviderRequest request, ICompositionContext context) + { + if (request.RequestedType != typeof(Loop)) + return CompositionProviderResult.NotHandled; + + var descriptor = new CompositionRequestDescriptor( + CompositionRequestKind.ConstructorParameter, ordinal: 0, name: "inner", declaringType: null, Nullability.NotNullable); + + // Never reached without throwing first - context.Resolve(descriptor) re-enters this + // same provider for the same type, which InvokeProvider's reentrance guard must catch. + context.Resolve(descriptor); + return CompositionProviderResult.Handled(new Loop()); + } + } +}