Merge branch 'master' of github.com:maldoinc/wireup

Author: maldoinc

test/unit/test_container_creation.py

@@ -10,6 +10,8 @@
 import wireup
 from wireup._annotations import Inject, abstract, injectable
 from wireup.errors import WireupError
+from wireup.ioc import registry_validation
+from wireup.ioc.types import get_container_object_id
 
 from test.unit.services.no_annotations.random.random_service import RandomService
 
@@ -208,6 +210,131 @@ def make_foo_no_dependency() -> Foo:
     wireup.create_sync_container(injectables=[make_foo, make_bar, make_foo_no_dependency])
 
 
+def test_validates_container_walks_shared_dependencies_once(monkeypatch: pytest.MonkeyPatch) -> None:
+    @wireup.injectable
+    class Leaf: ...
+
+    @wireup.injectable
+    @dataclass
+    class Shared:
+        leaf: Leaf
+
+    @wireup.injectable
+    @dataclass
+    class Foo:
+        shared: Shared
+
+    @wireup.injectable
+    @dataclass
+    class Bar:
+        shared: Shared
+
+    @wireup.injectable
+    @dataclass
+    class Baz:
+        foo: Foo
+        bar: Bar
+        shared: Shared
+
+    reached: list[type] = []
+    descended: list[type] = []
+    walk = registry_validation.assert_valid_resolution_path
+
+    def recording_walk(**kwargs) -> None:
+        dependency = kwargs["dependency"]
+        reached.append(dependency.klass)
+        object_id = get_container_object_id(dependency.klass, dependency.qualifier_value)
+        # Dependencies known to be cycle-free return immediately, only record the rest.
+        if object_id not in kwargs["known_cycle_free_objects"]:
+            descended.append(dependency.klass)
+        walk(**kwargs)
+
+    monkeypatch.setattr(registry_validation, "assert_valid_resolution_path", recording_walk)
+    wireup.create_sync_container(injectables=[Leaf, Shared, Foo, Bar, Baz])
+
+    assert reached.count(Shared) > 1
+    # Every dependency in the graph is walked exactly once no matter how many paths reach it.
+    assert descended.count(Shared) == 1
+    assert descended.count(Leaf) == 1
+    assert descended.count(Foo) == 1
+    assert descended.count(Bar) == 1
+
+
+def test_validates_container_raises_when_cycle_is_behind_a_walked_dependency() -> None:
+    class Shared: ...
+
+    class Foo:
+        def __init__(self, shared, bar): ...
+
+    class Bar:
+        def __init__(self, shared, foo): ...
+
+    @wireup.injectable
+    def make_shared() -> Shared:
+        return Shared()
+
+    # Walking 'shared' clears it, the cycle behind 'bar' must still be found.
+    @wireup.injectable
+    def make_foo(shared: Shared, bar: Bar) -> Foo:
+        return Foo(shared, bar)
+
+    @wireup.injectable
+    def make_bar(shared: Shared, foo: Foo) -> Bar:
+        return Bar(shared, foo)
+
+    with pytest.raises(
+        WireupError,
+        match=re.escape(
+            f"Circular dependency detected for {Bar!r} (created via {make_bar.__module__}.{make_bar.__name__})"
+            f"\n -> {Foo!r} (created via {make_foo.__module__}.{make_foo.__name__})"
+            f"\n -> {Bar!r} (created via {make_bar.__module__}.{make_bar.__name__})"
+            " ! Cycle here"
+        ),
+    ):
+        wireup.create_sync_container(injectables=[make_shared, make_foo, make_bar])
+
+
+def test_validates_container_does_not_reuse_walked_dependencies_between_containers() -> None:
+    class Foo:
+        def __init__(self, bar): ...
+
+    class Bar:
+        def __init__(self, foo): ...
+
+    class Baz:
+        def __init__(self, foo): ...
+
+    @wireup.injectable
+    def make_foo() -> Foo:
+        return Foo(None)
+
+    @wireup.injectable
+    def make_baz(foo: Foo) -> Baz:
+        return Baz(foo)
+
+    # Foo is walked and recorded cycle-free here, which says nothing about another container's graph.
+    wireup.create_sync_container(injectables=[make_foo, make_baz])
+
+    @wireup.injectable
+    def make_cyclical_foo(bar: Bar) -> Foo:
+        return Foo(bar)
+
+    @wireup.injectable
+    def make_bar(foo: Foo) -> Bar:
+        return Bar(foo)
+
+    with pytest.raises(
+        WireupError,
+        match=re.escape(
+            f"Circular dependency detected for {Bar!r} (created via {make_bar.__module__}.{make_bar.__name__})"
+            f"\n -> {Foo!r} (created via {make_cyclical_foo.__module__}.{make_cyclical_foo.__name__})"
+            f"\n -> {Bar!r} (created via {make_bar.__module__}.{make_bar.__name__})"
+            " ! Cycle here"
+        ),
+    ):
+        wireup.create_sync_container(injectables=[make_cyclical_foo, make_bar])
+
+
 def test_lifetimes_match_factories() -> None:
     class ScopedService: ...
 

test/unit/test_inject_from_container.py

@@ -18,6 +18,9 @@
 from test.unit.services.no_annotations.random.random_service import RandomService
 from test.unit.services.with_annotations.services import Foo, FooImpl, OtherFooImpl, random_service_factory
 
+# CPython renders id() as uppercase hex on Windows and lowercase elsewhere.
+HEX_ADDR = r"0x[0-9a-fA-F]+"
+
 
 async def test_injects_targets(container: Container) -> None:
     class NotManagedByWireup: ...
@@ -149,7 +152,7 @@ class NotManagedByWireup: ...
     with pytest.raises(
         WireupError,
         match=(
-            r"Parameter 'not_managed_by_wireup' of <function .*test_raises_on_unknown_service.*\._ at 0x[0-9a-f]+> "
+            rf"Parameter 'not_managed_by_wireup' of <function .*test_raises_on_unknown_service.*\._ at {HEX_ADDR}> "
             + re.escape(f"has an unknown dependency on {NotManagedByWireup!r}{expected_qualifier_str}.")
         ),
     ):
@@ -164,7 +167,7 @@ async def test_raises_on_unknown_parameter(container: Container) -> None:
     with pytest.raises(
         WireupError,
         match=(
-            r"Parameter 'not_managed_by_wireup' of <function .*test_raises_on_unknown_parameter.*\._ at 0x[0-9a-f]+> "
+            rf"Parameter 'not_managed_by_wireup' of <function .*test_raises_on_unknown_parameter.*\._ at {HEX_ADDR}> "
             + re.escape("depends on an unknown Wireup config key 'invalid'.")
         ),
     ):

wireup/ioc/registry_validation.py

@@ -13,11 +13,16 @@
 
 if TYPE_CHECKING:
     from wireup.ioc.registry import ContainerRegistry, InjectableFactory
-    from wireup.ioc.types import Qualifier
+    from wireup.ioc.types import ContainerObjectIdentifier, Qualifier
 
 
 def validate_registry(registry: ContainerRegistry) -> None:
     """Assert that all required dependencies exist for this registry instance."""
+    # Dependencies are shared between injectables, so the same subtree is reachable via many paths.
+    # Remember the ones already known to be cycle-free to avoid walking them again.
+    # Only valid for this run as a different registry describes a different graph.
+    known_cycle_free_objects: set[ContainerObjectIdentifier] = set()
+
     for obj_id, injectable_factory in registry.factories.items():
         if isinstance(obj_id, tuple):
             impl, impl_qualifier = obj_id
@@ -54,6 +59,7 @@ def validate_registry(registry: ContainerRegistry) -> None:
                 dependencies=registry.dependencies,
                 dependency=dependency,
                 path=[],
+                known_cycle_free_objects=known_cycle_free_objects,
             )
 
         for name in unknown_dependencies_with_default:
@@ -118,18 +124,26 @@ def assert_dependency_exists(
         raise WireupError(msg)
 
 
-def assert_valid_resolution_path(
+def assert_valid_resolution_path(  # noqa: PLR0913
     *,
     interfaces: dict[type, dict[Qualifier | None, type]],
     factories: dict[Any, InjectableFactory],
     dependencies: dict[Any, dict[str, AnnotatedParameter]],
     dependency: AnnotatedParameter,
     path: list[tuple[AnnotatedParameter, Any]],
+    known_cycle_free_objects: set[ContainerObjectIdentifier],
 ) -> None:
     """Assert that the resolution path for a dependency does not create a cycle."""
     if dependency.klass in interfaces or dependency.is_parameter:
         return
-    dependency_injectable_factory = factories[get_container_object_id(dependency.klass, dependency.qualifier_value)]
+    object_id = get_container_object_id(dependency.klass, dependency.qualifier_value)
+
+    # A dependency whose subtree came back clean cannot lead to a cycle. Were it part of one,
+    # the walk would have come back to it while it was still on the current path.
+    if object_id in known_cycle_free_objects:
+        return
+
+    dependency_injectable_factory = factories[object_id]
     new_path: list[tuple[AnnotatedParameter, Any]] = [*path, (dependency, dependency_injectable_factory)]
 
     if any(p.klass == dependency.klass and p.qualifier_value == dependency.qualifier_value for p, _ in path):
@@ -154,4 +168,7 @@ def stringify_dependency(p: AnnotatedParameter, factory: Any) -> str:
             dependencies=dependencies,
             dependency=next_dependency,
             path=new_path,
+            known_cycle_free_objects=known_cycle_free_objects,
         )
+
+    known_cycle_free_objects.add(object_id)