Improve error messages when receiving a property that forms a cycle (#95

)

Sources/SafeDICore/Generators/DependencyTreeGenerator.swift

@@ -88,6 +88,8 @@ public final class DependencyTreeGenerator {
         case noInstantiableFound(TypeDescription)
         case unfulfillableProperties([UnfulfillableProperty])
         case instantiableHasForwardedProperty(property: Property, instantiableWithForwardedProperty: Instantiable, parent: Instantiable)
+        case constantDependencyCycleDetected([TypeDescription])
+        case receivedInstantiatorDependencyCycleDetected(property: Property, directParent: TypeDescription, cycle: [TypeDescription])
 
         var description: String {
             switch self {
@@ -97,15 +99,31 @@ public final class DependencyTreeGenerator {
                 """
                 \(unfulfillableProperties.map {
                     """
-                    @\(Dependency.Source.receivedRawValue) property `\($0.property.asSource)` is not @\(Dependency.Source.instantiatedRawValue) or @\(Dependency.Source.forwardedRawValue) in chain: \(([$0.instantiable] + $0.parentStack)
+                    @\(Dependency.Source.receivedRawValue) property `\($0.property.asSource)` is not @\(Dependency.Source.instantiatedRawValue) or @\(Dependency.Source.forwardedRawValue) in chain: \(([$0.instantiable.concreteInstantiable] + $0.parentStack)
                         .reversed()
-                        .map(\.concreteInstantiable.asSource)
+                        .map(\.asSource)
                         .joined(separator: " -> "))
                     """
                 }.joined(separator: "\n"))
                 """
             case let .instantiableHasForwardedProperty(property, instantiable, parent):
                 "Property `\(property.asSource)` on \(parent.concreteInstantiable.asSource) has at least one @\(Dependency.Source.forwardedRawValue) property. Property should instead be of type `\(Dependency.instantiatorType)<\(instantiable.concreteInstantiable.asSource)>`."
+            case let .constantDependencyCycleDetected(instantiables):
+                """
+                Dependency cycle detected!
+                \(instantiables
+                    .map(\.asSource)
+                    .reversed()
+                    .joined(separator: " -> "))
+                """
+            case let .receivedInstantiatorDependencyCycleDetected(property, directParent, instantiables):
+                """
+                Dependency cycle detected! @\(Dependency.Source.instantiatedRawValue) `\(property.asSource)` is @\(Dependency.Source.receivedRawValue) in tree created by @\(Dependency.Source.instantiatedRawValue) `\(property.asSource)`. Declare @\(Dependency.Source.receivedRawValue) `\(property.asSource)` on `\(directParent.asSource)` as @\(Dependency.Source.instantiatedRawValue) to fix. Full cycle:
+                \(instantiables
+                    .map(\.asSource)
+                    .reversed()
+                    .joined(separator: " -> "))
+                """
             }
         }
 
@@ -116,7 +134,7 @@ public final class DependencyTreeGenerator {
 
             let property: Property
             let instantiable: Instantiable
-            let parentStack: [Instantiable]
+            let parentStack: [TypeDescription]
         }
     }
 
@@ -301,8 +319,10 @@ public final class DependencyTreeGenerator {
         func validateReceivedProperties(
             on scope: Scope,
             receivableProperties: Set<Property>,
-            instantiables: OrderedSet<Instantiable>
-        ) {
+            property: Property?,
+            propertyStack: OrderedSet<Property>,
+            root: TypeDescription
+        ) throws {
             let createdProperties = Set(
                 scope
                     .instantiable
@@ -325,51 +345,76 @@ public final class DependencyTreeGenerator {
                 let parentContainsProperty = receivableProperties.contains(receivedProperty)
                 let propertyIsCreatedAtThisScope = createdProperties.contains(receivedProperty)
                 if !parentContainsProperty, !propertyIsCreatedAtThisScope {
-                    if instantiables.elements.isEmpty {
+                    if property == nil {
                         // This property's scope is not a real root instantiable! Remove it from the list.
                         rootInstantiables.remove(scope.instantiable.concreteInstantiable)
                     } else {
                         // This property is in a dependency tree and is unfulfillable. Record the problem.
                         unfulfillableProperties.insert(.init(
                             property: receivedProperty,
                             instantiable: scope.instantiable,
-                            parentStack: instantiables.elements
-                        )
-                        )
+                            parentStack: propertyStack.map(\.typeDescription) + [root]
+                        ))
                     }
                 }
             }
 
+            // Check children for cycles.
+            var childPropertyStack = propertyStack
+            if let property {
+                childPropertyStack.insert(property, at: 0)
+            }
+            for dependency in scope.instantiable.dependencies {
+                let propertyForDependency = dependency.source.fulfillingProperty ?? dependency.property
+                guard let cycleIndex = childPropertyStack.firstIndex(of: propertyForDependency) else {
+                    continue
+                }
+                let typesInCycle = (
+                    [propertyForDependency]
+                        + childPropertyStack.elements[0...cycleIndex]
+                ).map(\.typeDescription)
+                if propertyForDependency.propertyType.isConstant {
+                    // We can break a constant dependency cycle if there's lazy instantiation in the tree.
+                    if !typesInCycle.contains(where: { !$0.propertyType.isConstant }) {
+                        throw DependencyTreeGeneratorError.constantDependencyCycleDetected(typesInCycle)
+                    }
+                } else if dependency.source.isReceived {
+                    throw DependencyTreeGeneratorError.receivedInstantiatorDependencyCycleDetected(
+                        property: propertyForDependency,
+                        directParent: scope.instantiable.concreteInstantiable,
+                        cycle: typesInCycle
+                    )
+                }
+            }
+
             for childPropertyToGenerate in scope.propertiesToGenerate {
                 switch childPropertyToGenerate {
-                case let .instantiated(childProperty, childScope, _):
-                    guard !instantiables.contains(childScope.instantiable) else {
-                        // We've previously visited this child scope.
+                case let .instantiated(property, childScope, _):
+                    guard !childPropertyStack.contains(property) else {
                         // There is a cycle in our scope tree. Do not re-enter it.
                         continue
                     }
-                    var instantiables = instantiables
-                    instantiables.insert(scope.instantiable, at: 0)
-
-                    validateReceivedProperties(
+                    try validateReceivedProperties(
                         on: childScope,
                         receivableProperties: receivableProperties
-                            .union(scope.properties)
-                            .removing(childProperty),
-                        instantiables: instantiables
+                            .union(scope.properties),
+                        property: property,
+                        propertyStack: childPropertyStack,
+                        root: root
                     )
-
                 case .aliased:
                     break
                 }
             }
         }
 
         for rootScope in rootInstantiables.compactMap({ typeDescriptionToScopeMap[$0] }) {
-            validateReceivedProperties(
+            try validateReceivedProperties(
                 on: rootScope,
                 receivableProperties: Set(rootScope.properties),
-                instantiables: []
+                property: nil,
+                propertyStack: [],
+                root: rootScope.instantiable.concreteInstantiable
             )
         }
 
@@ -423,13 +468,3 @@ extension Collection<Dependency> {
         }) == nil
     }
 }
-
-// MARK: - Set
-
-extension Set {
-    fileprivate func removing(_ element: Element) -> Self {
-        var setWithoutElement = self
-        setWithoutElement.remove(element)
-        return setWithoutElement
-    }
-}

Sources/SafeDICore/Generators/ScopeGenerator.swift

@@ -367,10 +367,14 @@ actor ScopeGenerator: CustomStringConvertible {
                     return
                 }
                 guard !stack.contains(property) else {
-                    throw GenerationError.dependencyCycleDetected(
-                        stack.drop(while: { $0 != property }) + [property],
-                        scope: self
-                    )
+                    if property.propertyType.isConstant {
+                        throw GenerationError.dependencyCycleDetected(
+                            stack.drop(while: { $0 != property }) + [property],
+                            scope: self
+                        )
+                    } else {
+                        return
+                    }
                 }
 
                 let scopeDependencies = propertyToUnfulfilledScopeMap

Sources/SafeDICore/Models/Dependency.swift

@@ -84,6 +84,15 @@ public struct Dependency: Codable, Hashable {
             }
         }
 
+        public var fulfillingProperty: Property? {
+            switch self {
+            case let .aliased(fulfillingProperty, _):
+                fulfillingProperty
+            case .instantiated, .received, .forwarded:
+                nil
+            }
+        }
+
         public static let instantiatedRawValue = "Instantiated"
         public static let receivedRawValue = "Received"
         public static let forwardedRawValue = "Forwarded"

Sources/SafeDICore/Models/Scope.swift

@@ -80,68 +80,51 @@ final class Scope: Hashable {
         propertyStack: OrderedSet<Property>,
         erasedToConcreteExistential: Bool
     ) throws -> ScopeGenerator {
-        if
-            let property,
-            property.propertyType.isConstant,
-            let cycleIndex = propertyStack.firstIndex(of: property),
-            !propertyStack.elements[0...cycleIndex].contains(where: { !$0.propertyType.isConstant })
-        {
-            throw ScopeError.dependencyCycleDetected([property.typeDescription] + propertyStack.elements[0...cycleIndex].map(\.typeDescription))
+        var childPropertyStack = propertyStack
+        let isPropertyCycle: Bool
+        if let property {
+            isPropertyCycle = propertyStack.contains(property)
+            childPropertyStack.insert(property, at: 0)
         } else {
-            var childPropertyStack = propertyStack
-            let isPropertyCycle: Bool
-            if let property {
-                isPropertyCycle = propertyStack.contains(property)
-                childPropertyStack.insert(property, at: 0)
-            } else {
-                isPropertyCycle = false
-            }
-            let scopeGenerator = try ScopeGenerator(
-                instantiable: instantiable,
-                property: property,
-                propertiesToGenerate: isPropertyCycle ? [] : propertiesToGenerate.map {
-                    switch $0 {
-                    case let .instantiated(property, scope, erasedToConcreteExistential):
-                        try scope.createScopeGenerator(
-                            for: property,
-                            propertyStack: childPropertyStack,
-                            erasedToConcreteExistential: erasedToConcreteExistential
-                        )
-                    case let .aliased(property, fulfillingProperty, erasedToConcreteExistential):
-                        ScopeGenerator(
-                            property: property,
-                            fulfillingProperty: fulfillingProperty,
-                            erasedToConcreteExistential: erasedToConcreteExistential
-                        )
-                    }
-                },
-                erasedToConcreteExistential: erasedToConcreteExistential,
-                isPropertyCycle: isPropertyCycle
-            )
-            Task.detached {
-                // Kick off code generation.
-                try await scopeGenerator.generateCode()
-            }
-            return scopeGenerator
+            isPropertyCycle = false
+        }
+        let scopeGenerator = try ScopeGenerator(
+            instantiable: instantiable,
+            property: property,
+            propertiesToGenerate: isPropertyCycle ? [] : propertiesToGenerate.map {
+                switch $0 {
+                case let .instantiated(property, scope, erasedToConcreteExistential):
+                    try scope.createScopeGenerator(
+                        for: property,
+                        propertyStack: childPropertyStack,
+                        erasedToConcreteExistential: erasedToConcreteExistential
+                    )
+                case let .aliased(property, fulfillingProperty, erasedToConcreteExistential):
+                    ScopeGenerator(
+                        property: property,
+                        fulfillingProperty: fulfillingProperty,
+                        erasedToConcreteExistential: erasedToConcreteExistential
+                    )
+                }
+            },
+            erasedToConcreteExistential: erasedToConcreteExistential,
+            isPropertyCycle: isPropertyCycle
+        )
+        Task.detached {
+            // Kick off code generation.
+            try await scopeGenerator.generateCode()
         }
+        return scopeGenerator
     }
+}
 
-    // MARK: ScopeError
-
-    private enum ScopeError: Error, CustomStringConvertible {
-        case dependencyCycleDetected([TypeDescription])
-
-        var description: String {
-            switch self {
-            case let .dependencyCycleDetected(instantiables):
-                """
-                Dependency cycle detected!
-                \(instantiables
-                    .map(\.asSource)
-                    .reversed()
-                    .joined(separator: " -> "))
-                """
-            }
+extension Dependency.Source {
+    var isReceived: Bool {
+        switch self {
+        case .received:
+            true
+        case .instantiated, .aliased, .forwarded:
+            false
         }
     }
 }