diff --git a/architecture/resolution.md b/architecture/resolution.md index c49c89c1..d0319094 100644 --- a/architecture/resolution.md +++ b/architecture/resolution.md @@ -47,14 +47,24 @@ map produced at provider-declaration time by `types_parser.parse_creator` — an 1. **Static kwarg shadows** — if the parameter was supplied via the provider's declaration-time `kwargs`, it is taken from there. Provider-valued static kwargs go to the plan's `provider_kwargs`; plain values go to `static_kwargs`. - These bypass type-based wiring and are *not* recorded in the plan's `dependencies` view, so `validate()` does not - traverse them. + These bypass *type-based wiring* — the declaration names the provider outright instead of inferring it — but they are + dependencies like any other, and `validate()` traverses them. 2. **Provider lookup** — otherwise `find_dep_provider` searches `providers_registry` for a provider matching the parameter's resolved type (`arg_type`) or, for union types, any of the union members (`args`); self-references are excluded. A matching `ContextProvider` goes to `context_kwargs` (resolved live, see Step 5); any other provider goes - to `provider_kwargs`. Type-matched providers are also recorded in the plan's `dependencies` view, which `validate()` - reads. + to `provider_kwargs`. + +> **One edge set.** The plan's `edges` view — what `validate()` traverses — is *derived* from `provider_kwargs` and +> `context_kwargs`, the same buckets `resolve()` reads. The validated graph therefore cannot drift from the resolved +> one. What differs between the two routes above is *how the edge is declared*, never *whether it exists*: a cycle or a +> scope inversion routed through `kwargs={...}` is reported by `validate()` exactly as a type-matched one is. +> +> Self-reference is the one asymmetry, and it is not an exception to the rule. `find_dep_provider` excludes the owner, +> so a *type-matched* self-reference is not an edge and falls through to the creator default — inference declining to +> wire a provider to itself. The `kwargs` overlay has no such exclusion, because a provider cannot be passed to its own +> constructor: a `kwargs` value is always a backward reference to an already-built provider. (For the same reason, a +> cycle through `kwargs` always contains at least one type-matched edge to forward-reference through.) > **Union vs. single parameterized generics.** A bare parameterized generic (e.g. `list[str]`) is *rejected at > declaration* — it cannot be resolved by type. Inside a union, however, each member degrades to its origin for diff --git a/architecture/validation.md b/architecture/validation.md index cea7f94b..b4605ac6 100644 --- a/architecture/validation.md +++ b/architecture/validation.md @@ -51,6 +51,13 @@ by class name so a report mixing several error kinds reads as one section per ki `providers_registry`, so a root APP-scope container validates deeper-scoped providers without building child containers. +**The graph it walks is the graph that resolves.** Edges come from `WiringPlan.edges`, a view *derived* +from the same buckets `resolve()` reads (`provider_kwargs` + `context_kwargs`) rather than assembled +separately — so the validated graph cannot drift from the resolved one. In particular a provider supplied +via a declaration-time `kwargs={...}` is an edge like any type-matched one, and a cycle or scope inversion +routed through it is caught here rather than surfacing at resolve time as a bare `RecursionError` or a +`ScopeNotInitializedError`. See [resolution.md](resolution.md) for how the buckets are filled. + **Validated-version short-circuit.** `ProvidersRegistry` carries a `validated_version: int | None`. On a successful walk `validate()` stamps `validated_version = version`; a later `validate()` whose `validated_version == version` returns immediately without re-walking, so a repeat `validate()` is free. diff --git a/modern_di/dependency_graph.py b/modern_di/dependency_graph.py index 13f985c1..436a45a1 100644 --- a/modern_di/dependency_graph.py +++ b/modern_di/dependency_graph.py @@ -3,9 +3,12 @@ ``DependencyGraph.walk`` is the single traversal that other capabilities (validation, the runtime cycle guard) consume. It is deliberately *explicit-stack* — no recursion — because a later caller runs it inside a ``RecursionError`` handler near CPython's stack -limit, where headroom for a recursive walk is not guaranteed. The event order mirrors -``Container.validate``'s recursive ``_visit`` exactly, so a consumer can reproduce -validate()'s output byte-for-byte. +limit, where headroom for a recursive walk is not guaranteed. + +The graph it walks is ``WiringPlan.edges``: every provider a plan resolves, however that +dependency was declared. Type-matched parameters and providers supplied via +``kwargs={...}`` are edges alike — so what ``validate()`` traverses is exactly what +``resolve()`` follows. Import discipline: this module must not import ``Container`` (nor any concrete provider) at runtime — ``container.py`` imports this module, so a runtime back-import would cycle. diff --git a/modern_di/providers/factory.py b/modern_di/providers/factory.py index 35de2153..17f01f57 100644 --- a/modern_di/providers/factory.py +++ b/modern_di/providers/factory.py @@ -224,7 +224,7 @@ def get_dependencies(self, container: "Container") -> dict[str, "AbstractProvide kwargs=self._kwargs, registry=container.providers_registry, owner=self, - ).dependencies + ).edges def iter_validation_issues(self, container: "Container") -> typing.Iterable[Exception]: """Yield ArgumentResolutionError for parameters with no provider, no default, no static kwarg.""" diff --git a/modern_di/wiring.py b/modern_di/wiring.py index cb938ff4..a3cb8acc 100644 --- a/modern_di/wiring.py +++ b/modern_di/wiring.py @@ -69,9 +69,6 @@ class WiringPlan: provider_kwargs: name → provider resolved live each resolve call. static_kwargs: name → literal value (including nullable-None). context_kwargs: name → (ContextProvider, SignatureItem) looked up live. - dependencies: type-matched providers only (regular + context), - excluding providers supplied via ``kwargs={...}``. - Used by ``validate()``'s graph traversal. unwireable: UNWIRABLE parameters as (param-name, SignatureItem) records rather than pre-built exceptions, so a fresh ``ArgumentResolutionError`` can be constructed at @@ -84,9 +81,21 @@ class WiringPlan: provider_kwargs: dict[str, "AbstractProvider[typing.Any]"] static_kwargs: dict[str, typing.Any] context_kwargs: dict[str, "tuple[ContextProvider[typing.Any], SignatureItem]"] - dependencies: dict[str, "AbstractProvider[typing.Any]"] unwireable: "list[tuple[str, SignatureItem]]" + @property + def edges(self) -> dict[str, "AbstractProvider[typing.Any]"]: + """Every provider this plan resolves — the graph ``validate()`` traverses. + + Derived from the buckets ``resolve()`` reads, so the validated graph cannot + drift from the resolved one. Providers supplied via ``kwargs={...}`` are edges + like any other: only the *declaration* differs, not the dependency. + """ + return { + **self.provider_kwargs, + **{name: provider for name, (provider, _item) in self.context_kwargs.items()}, + } + @classmethod def build( cls, @@ -100,7 +109,6 @@ def build( provider_kwargs: dict[str, AbstractProvider[typing.Any]] = {} static_kwargs: dict[str, typing.Any] = {} context_kwargs: dict[str, tuple[ContextProvider[typing.Any], SignatureItem]] = {} - dependencies: dict[str, AbstractProvider[typing.Any]] = {} unwireable: list[tuple[str, SignatureItem]] = [] for name, item in parsed_kwargs.items(): @@ -109,7 +117,6 @@ def build( provider = find_dep_provider(registry, owner, item) if provider is not None: - dependencies[name] = provider # validate-visible (type-matched only) if isinstance(provider, ContextProvider): context_kwargs[name] = (provider, item) else: @@ -125,7 +132,7 @@ def build( # UNWIRABLE: record the (name, item) pair but do not raise unwireable.append((name, item)) - if kwargs: # static overlay — NOT added to `dependencies` + if kwargs: # static overlay for name, value in kwargs.items(): if isinstance(value, AbstractProvider): provider_kwargs[name] = value @@ -136,6 +143,5 @@ def build( provider_kwargs=provider_kwargs, static_kwargs=static_kwargs, context_kwargs=context_kwargs, - dependencies=dependencies, unwireable=unwireable, ) diff --git a/planning/changes/2026-07-14.06-unify-wiring-edge-set.md b/planning/changes/2026-07-14.06-unify-wiring-edge-set.md new file mode 100644 index 00000000..816e8348 --- /dev/null +++ b/planning/changes/2026-07-14.06-unify-wiring-edge-set.md @@ -0,0 +1,158 @@ +--- +summary: Derive the plan's edge set from the buckets the runtime resolves from, so `validate()` sees `kwargs=`-supplied providers — closing a cycle/scope hole that surfaced as a raw `RecursionError`. +--- + +# Design: One edge set for the runtime and for validate() + +## Summary + +`WiringPlan` builds three provider-bearing dicts. The runtime resolves from +`provider_kwargs` + `context_kwargs`; `validate()` traverses a *third* dict, +`dependencies`, built in the same loop. The declaration-time `kwargs={...}` +overlay writes to `provider_kwargs` and not to `dependencies`, so a provider +supplied via `kwargs=` is a real runtime edge that validation cannot see. + +Delete the `dependencies` field. Replace it with `WiringPlan.edges`, a property +derived from the two buckets the runtime actually resolves from. `validate()` +then traverses exactly what `resolve()` will follow, and the two cannot +diverge — the fix falls out of the derivation instead of being a second write +someone has to remember to make. + +## Motivation + +Two defects, both reproduced against `fe090f4`. + +**A cycle through `kwargs=` passes `validate()` and then blows the stack:** + +```python +G.b = Factory(scope=Scope.APP, creator=B, kwargs={"a": G.a}) +G.a = Factory(scope=Scope.APP, creator=A, kwargs={"b": G.b}) + +c.validate() # PASSES — reports no cycle +c.resolve(A) # RecursionError: maximum recursion depth exceeded +``` + +A raw `RecursionError` is the exact failure mode `CircularDependencyError` +exists to prevent. The runtime cycle guard (`container.py:29-43`) cannot save +it either: `find_cycle_from` walks `get_dependencies`, sees no edge, and +re-raises the bare `RecursionError`. + +**A scope inversion through `kwargs=` does the same,** surfacing as +`ScopeNotInitializedError` at resolve time instead of at `validate()`. It fails +unconditionally — `Factory.resolve` re-targets to its own scope's container via +`find_container`, so an APP-scoped provider depending on a REQUEST-scoped one +through `kwargs=` can never resolve from anywhere. + +Both are the same root cause: the July 12 unification +([2026-07-12.01](2026-07-12.01-dependency-graph-module.md), PR #308) unified the +*walk* but not the *graph*. One walker, two edge sets. + +`architecture/resolution.md:52` currently documents the gap as intended +("*not* recorded in the plan's `dependencies` view, so `validate()` does not +traverse them"). That sentence describes what the code does, not a decision +anyone made; it is rewritten here. What varies between the two ways of +declaring a dependency is *how the edge is declared*, not *whether it exists*. + +## Design + +`WiringPlan` loses `dependencies` and gains: + +```python +@property +def edges(self) -> dict[str, AbstractProvider[typing.Any]]: + """Every provider this plan will resolve — the graph validate() traverses.""" + return { + **self.provider_kwargs, + **{name: provider for name, (provider, _item) in self.context_kwargs.items()}, + } +``` + +`Factory.get_dependencies` returns `plan.edges`. `build()` stops populating a +third dict. + +Work out what `edges` contains: type-matched regular providers + `kwargs=`-supplied +providers + type-matched context providers. That is precisely today's +`dependencies` **plus** the missing `kwargs=` edges — the fix, obtained by +derivation rather than by a parallel write. + +The runtime keeps its partition: `provider_kwargs` and `context_kwargs` take +different resolution paths (`factory.py:259-264`), and context resolution needs +the `SignatureItem` carried alongside to decide omit-vs-`None`-vs-raise when a +context value is absent. The partition is load-bearing; the *third dict* was not. + +**Self-reference.** `find_dep_provider` excludes the owner, so a type-matched +self-reference is not an edge and falls through to the creator default. The +`kwargs=` overlay does **not** inherit that exclusion: inference declining to +self-wire and a user explicitly handing a provider to itself are different acts. +An explicit self-pass is a 1-node cycle and is reported as +`CircularDependencyError` rather than left to recurse. + +**Cold path only.** `edges` is read once per node per `validate()` and never +during `resolve()` — `_resolve_kwargs` reads the buckets directly. Measured on a +60-provider chain: the derivation adds ~11 µs to a ~225 µs `validate()` (+4.9%), +and zero to resolve. + +## Non-goals + +- **The double-build.** `validate()` builds each Factory's plan twice (measured: + 120 `WiringPlan.build` calls for 60 providers) because `get_dependencies` and + `iter_validation_issues` are separate hooks with no shared context. Every fix + either makes `validate()` touch the cache (violating `get_dependencies`' + documented "no cache touch" contract), adds validate-only lifecycle state to + `Container`, breaks `dependency_graph.py`'s no-concrete-providers layering + rule, or changes `AbstractProvider`'s interface. That last is a provider-seam + change and belongs in its own design. +- **Flattening the runtime partition.** Collapsing `provider_kwargs` and + `context_kwargs` into one dict would discard the `SignatureItem` that context + resolution depends on. + +## Testing + +`just test-ci` (gated, 100% line coverage). + +TDD — both failing tests first, from the reproductions above: + +- **Cycle through `kwargs=`** (`tests/test_dependency_graph.py`): two providers + wired to each other via `kwargs=`. Before: `validate()` returns clean and + `resolve()` raises `RecursionError`. After: `validate()` raises + `CircularDependencyError`. +- **Self-pass through `kwargs=`** (`tests/test_dependency_graph.py`): a provider + handed to itself via `kwargs=` yields a 1-node cycle. +- **Scope inversion through `kwargs=`** (`tests/test_container.py`): an + APP-scoped provider depending on a REQUEST-scoped one via `kwargs=`. Before: + `validate()` clean, `ScopeNotInitializedError` at resolve. After: + `InvalidScopeDependencyError` from `validate()`. + +Existing tests that must stay green, as the guard on the partition: +`test_factory_self_reference` and +`test_factory_self_reference_in_union_falls_through_to_default` +(`tests/providers/test_factory.py`) — type-matched self-reference must keep +falling through to the default. + +`tests/test_wiring.py:190-194` asserts on `plan.dependencies` and is rewritten +against `plan.edges`. + +## Risk + +**A green `validate()` goes red on upgrade** — likely × moderate. Any config this +newly reports was already broken at runtime (verified: `RecursionError` / +`ScopeNotInitializedError`), so there are no false positives. But a user with a +*dormant* broken config — one they never resolve — will see `validate()` start +failing. Mitigation: ship in a 2.x minor as a bug fix and say so prominently in +the release notes. It is not gated behind 3.0 because `validate()`'s whole +contract is to find these before the runtime does. + +**`validate()` slows ~4.9%** — likely × negligible. Cold path, runs once at boot. +The double-build above is the larger cost and is left in place deliberately. + +## Other + +Two stale artifacts from the July 12 unification, cleaned up here since this +change lands in the same code: + +- `dependency_graph.py:7` describes the module's event order against + `Container.validate`'s recursive `_visit`, which was deleted in PR #308. A + reader will hunt for a function that no longer exists. +- `tests/test_dependency_graph_parity.py` no longer tests parity between two + implementations — none remains. The name advertises a synchronization hazard + the code does not have; renamed to `tests/test_dependency_graph_contract.py`. diff --git a/tests/test_container.py b/tests/test_container.py index 2f04a7c7..80034f89 100644 --- a/tests/test_container.py +++ b/tests/test_container.py @@ -254,6 +254,32 @@ class G(Group): assert issue.dep_provider.scope == Scope.REQUEST +def test_validate_raises_on_inverted_scope_dependency_supplied_via_kwargs() -> None: + """A `kwargs=`-supplied provider is a real edge: its scope is checked like any other.""" + + @dataclasses.dataclass(kw_only=True, slots=True) + class Inner: + pass + + class Outer: + # `inner: object` never type-matches; the edge exists only via the kwargs overlay. + def __init__(self, inner: object = None) -> None: ... + + inner = providers.Factory(scope=Scope.REQUEST, creator=Inner) + outer = providers.Factory(scope=Scope.APP, creator=Outer, kwargs={"inner": inner}) + + container = Container(validate=False) + container.providers_registry.add_providers(inner, outer) + + with pytest.raises(ValidationFailedError) as exc: + container.validate() + [issue] = exc.value.errors + assert isinstance(issue, InvalidScopeDependencyError) + assert issue.parameter_name == "inner" + assert issue.provider.scope == Scope.APP + assert issue.dep_provider.scope == Scope.REQUEST + + def test_validate_raises_on_missing_required_dependency() -> None: @dataclasses.dataclass(kw_only=True, slots=True) class Missing: diff --git a/tests/test_dependency_graph.py b/tests/test_dependency_graph.py index ebf27410..77b5af09 100644 --- a/tests/test_dependency_graph.py +++ b/tests/test_dependency_graph.py @@ -181,3 +181,28 @@ class G(Group): assert events[1].provider is G.alias # dangling alias contributes no edges past the error assert not any(isinstance(e, Edge) for e in events) + + +# A cycle that closes through the declaration-time `kwargs=` overlay. +# It needs one type-matched edge to forward-reference: a `kwargs=` value is always a +# backward reference to an already-built provider, so a pure-`kwargs=` cycle cannot +# be constructed at all. +class KwCycA: + def __init__(self, b: "KwCycB") -> None: ... # type-matched edge A -> B + + +class KwCycB: + def __init__(self, a: object = None) -> None: ... # `object` never type-matches + + +def test_walk_emits_cycle_closed_through_kwargs_overlay() -> None: + a = Factory(scope=Scope.APP, creator=KwCycA) + b = Factory(scope=Scope.APP, creator=KwCycB, kwargs={"a": a}) # kwargs edge B -> A + + c = Container(scope=Scope.APP, validate=False) + c.providers_registry.add_providers(a, b) + + events = list(DependencyGraph().walk([a], c)) + cycles = [e for e in events if isinstance(e, Cycle)] + assert len(cycles) == 1 + assert [p.display_name for p in cycles[0].providers] == ["KwCycA", "KwCycB", "KwCycA"] diff --git a/tests/test_dependency_graph_parity.py b/tests/test_dependency_graph_contract.py similarity index 89% rename from tests/test_dependency_graph_parity.py rename to tests/test_dependency_graph_contract.py index 7d74cd3c..e17bbe76 100644 --- a/tests/test_dependency_graph_parity.py +++ b/tests/test_dependency_graph_contract.py @@ -1,3 +1,10 @@ +"""Contract tests for the single graph traversal: cycle shape, error kinds, and the walk short-circuits. + +(Formerly `test_dependency_graph_parity.py`, from when validate() and the runtime guard each +had their own walk to keep in sync. PR #308 unified them; there is no second implementation +to be at parity with.) +""" + import pytest from modern_di import Container, Scope, dependency_graph, exceptions diff --git a/tests/test_wiring.py b/tests/test_wiring.py index 3d57c644..810906e7 100644 --- a/tests/test_wiring.py +++ b/tests/test_wiring.py @@ -155,7 +155,7 @@ def test_wiring_plan_unwireable_no_raise() -> None: # --------------------------------------------------------------------------- -# Test 3: dependencies excludes static-supplied providers +# Test 3: edges include static-supplied providers # --------------------------------------------------------------------------- @@ -164,7 +164,7 @@ def __init__(self, x: _ServiceA, y: _ServiceB) -> None: pass # pragma: no cover -def test_wiring_plan_dependencies_excludes_static_supplied_providers() -> None: +def test_wiring_plan_edges_include_static_supplied_providers() -> None: factory_a = providers.Factory(scope=Scope.APP, creator=_ServiceA) factory_b = providers.Factory(scope=Scope.APP, creator=_ServiceB) @@ -185,13 +185,15 @@ def test_wiring_plan_dependencies_excludes_static_supplied_providers() -> None: owner=owner, ) - # `x` is in provider_kwargs (resolved live), but NOT in dependencies + # `x` is supplied via the kwargs overlay: resolved live AND visible to validate(). assert "x" in plan.provider_kwargs - assert "x" not in plan.dependencies + assert plan.edges["x"] is factory_a - # `y` is type-matched → IS in dependencies - assert "y" in plan.dependencies - assert plan.dependencies["y"] is factory_b + # `y` is type-matched → an edge like any other. + assert plan.edges["y"] is factory_b + + # The edge set is exactly what the runtime resolves — however the edge was declared. + assert set(plan.edges) == {"x", "y"} assert plan.unwireable == []