Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve + consolidate cycle detection #100

Merged
merged 5 commits into from
Jul 7, 2024
Merged

Improve + consolidate cycle detection #100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8625c00
Improve + consolidate cycle detection
dfed Jul 7, 2024
5df901e
Simplify
dfed Jul 7, 2024
74de541
no try
dfed Jul 7, 2024
03ad312
Another test for higher coverage
dfed Jul 7, 2024
0a2d42e
simplify
dfed Jul 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 61 additions & 9 deletions Sources/SafeDICore/Generators/DependencyTreeGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ public final class DependencyTreeGenerator {
case instantiableHasForwardedProperty(property: Property, instantiableWithForwardedProperty: Instantiable, parent: Instantiable)
case constantDependencyCycleDetected([TypeDescription])
case receivedInstantiatorDependencyCycleDetected(property: Property, directParent: TypeDescription, cycle: [TypeDescription])
case receivedConstantCycleDetected(instantiated: Property, receivedPropertyChain: [Property])

var description: String {
switch self {
Expand Down Expand Up @@ -124,6 +125,14 @@ public final class DependencyTreeGenerator {
.reversed()
.joined(separator: " -> "))
"""
case let .receivedConstantCycleDetected(instantiated, receivedPropertyChain):
"""
Dependency received in same chain it is instantiated!
\("@\(Dependency.Source.instantiatedRawValue) \(instantiated.asSource) -> "
+ receivedPropertyChain
.map { "@\(Dependency.Source.receivedRawValue) \($0.asSource)" }
.joined(separator: " -> "))
"""
}
}

Expand Down Expand Up @@ -151,7 +160,7 @@ public final class DependencyTreeGenerator {
try validateReachableTypeDescriptions()

let typeDescriptionToScopeMap = try createTypeDescriptionToScopeMapping()
try validateReceivedProperties(typeDescriptionToScopeMap: typeDescriptionToScopeMap)
try validatePropertiesAreFulfillable(typeDescriptionToScopeMap: typeDescriptionToScopeMap)
return try rootInstantiables
.sorted()
.compactMap {
Expand Down Expand Up @@ -314,9 +323,9 @@ public final class DependencyTreeGenerator {
return typeDescriptionToScopeMap
}

private func validateReceivedProperties(typeDescriptionToScopeMap: [TypeDescription: Scope]) throws {
private func validatePropertiesAreFulfillable(typeDescriptionToScopeMap: [TypeDescription: Scope]) throws {
var unfulfillableProperties = Set<DependencyTreeGeneratorError.UnfulfillableProperty>()
func validateReceivedProperties(
func validatePropertiesAreFulfillable(
on scope: Scope,
receivableProperties: Set<Property>,
property: Property?,
Expand All @@ -341,6 +350,38 @@ public final class DependencyTreeGenerator {
}
.map(\.property)
)
if let property {
func validateNoCycleInReceivedProperties(
scope: Scope,
receivedPropertyStack: OrderedSet<Property>
) throws {
for childProperty in scope.receivedProperties {
guard childProperty != property else {
throw DependencyTreeGeneratorError.receivedConstantCycleDetected(
instantiated: property,
receivedPropertyChain: receivedPropertyStack + [childProperty]
)
}
guard !receivedPropertyStack.contains(childProperty) else {
// We've found a cycle, but it's not our cycle. Bail and let a future loop find this.
return
}
if let receivedPropertyScope = typeDescriptionToScopeMap[childProperty.typeDescription] {
var childPropertyStack = receivedPropertyStack
childPropertyStack.append(childProperty)
try validateNoCycleInReceivedProperties(
scope: receivedPropertyScope,
receivedPropertyStack: childPropertyStack
)
}
}
}
try validateNoCycleInReceivedProperties(
scope: scope,
receivedPropertyStack: []
)
}

for receivedProperty in scope.receivedProperties {
let parentContainsProperty = receivableProperties.contains(receivedProperty)
let propertyIsCreatedAtThisScope = createdProperties.contains(receivedProperty)
Expand Down Expand Up @@ -389,16 +430,17 @@ public final class DependencyTreeGenerator {

for childPropertyToGenerate in scope.propertiesToGenerate {
switch childPropertyToGenerate {
case let .instantiated(property, childScope, _):
guard !childPropertyStack.contains(property) else {
case let .instantiated(childProperty, childScope, _):
guard !childPropertyStack.contains(childProperty) else {
// There is a cycle in our scope tree. Do not re-enter it.
continue
}
try validateReceivedProperties(
try validatePropertiesAreFulfillable(
on: childScope,
receivableProperties: receivableProperties
.union(scope.properties),
property: property,
.union(scope.properties)
.removing(childProperty),
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brought this line back from #95. Now that we're doing our cycle checking before we're doing our "initialized in chain" checking, we can make this more semantically correct

property: childProperty,
propertyStack: childPropertyStack,
root: root
)
Expand All @@ -409,7 +451,7 @@ public final class DependencyTreeGenerator {
}

for rootScope in rootInstantiables.compactMap({ typeDescriptionToScopeMap[$0] }) {
try validateReceivedProperties(
try validatePropertiesAreFulfillable(
on: rootScope,
receivableProperties: Set(rootScope.properties),
property: nil,
Expand Down Expand Up @@ -468,3 +510,13 @@ extension Collection<Dependency> {
}) == nil
}
}

// MARK: - Set

extension Set {
fileprivate func removing(_ element: Element) -> Self {
var setWithoutElement = self
setWithoutElement.remove(element)
return setWithoutElement
}
}
79 changes: 29 additions & 50 deletions Sources/SafeDICore/Generators/ScopeGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ actor ScopeGenerator: CustomStringConvertible, Sendable {
}

func generateDOT() async throws -> String {
let orderedPropertiesToGenerate = try orderedPropertiesToGenerate
let orderedPropertiesToGenerate = orderedPropertiesToGenerate
let instantiatedProperties = orderedPropertiesToGenerate.map(\.scopeData.asDOTNode)
var childDOTs = [String]()
for orderedPropertyToGenerate in orderedPropertiesToGenerate {
Expand Down Expand Up @@ -351,58 +351,43 @@ actor ScopeGenerator: CustomStringConvertible, Sendable {
private var generateCodeTask: Task<String, Error>?

private var orderedPropertiesToGenerate: [ScopeGenerator] {
get throws {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we no longer need to throw here since we do this checking much earlier now. best reviewed with whitespace turned off

var orderedPropertiesToGenerate = [ScopeGenerator]()
var propertyToUnfulfilledScopeMap = propertiesToGenerate
.reduce(into: OrderedDictionary<Property, ScopeGenerator>()) { partialResult, scope in
if let property = scope.property {
partialResult[property] = scope
}
}
func fulfill(_ scope: ScopeGenerator, stack: OrderedSet<Property> = []) throws {
guard
let property = scope.property,
propertyToUnfulfilledScopeMap[property] != nil
else {
return
}
guard !stack.contains(property) else {
if property.propertyType.isConstant {
throw GenerationError.dependencyCycleDetected(
stack.drop(while: { $0 != property }) + [property],
scope: self
)
} else {
return
}
var orderedPropertiesToGenerate = [ScopeGenerator]()
var propertyToUnfulfilledScopeMap = propertiesToGenerate
.reduce(into: OrderedDictionary<Property, ScopeGenerator>()) { partialResult, scope in
if let property = scope.property {
partialResult[property] = scope
}

let scopeDependencies = propertyToUnfulfilledScopeMap
.keys
.intersection(scope.requiredReceivedProperties)
.compactMap { propertyToUnfulfilledScopeMap[$0] }
// Fulfill the scopes we depend upon.
for dependentScope in scopeDependencies {
var stack = stack
stack.append(property)
try fulfill(dependentScope, stack: stack)
}
// We can now be marked as fulfilled!
orderedPropertiesToGenerate.append(scope)
propertyToUnfulfilledScopeMap[property] = nil
}

for scope in propertiesToGenerate {
try fulfill(scope)
func fulfill(_ scope: ScopeGenerator) {
guard
let property = scope.property,
propertyToUnfulfilledScopeMap[property] != nil
else {
return
}
let scopeDependencies = propertyToUnfulfilledScopeMap
.keys
.intersection(scope.requiredReceivedProperties)
.compactMap { propertyToUnfulfilledScopeMap[$0] }
// Fulfill the scopes we depend upon.
for dependentScope in scopeDependencies {
fulfill(dependentScope)
}
// We can now be marked as fulfilled!
orderedPropertiesToGenerate.append(scope)
propertyToUnfulfilledScopeMap[property] = nil
}

return orderedPropertiesToGenerate
for scope in propertiesToGenerate {
fulfill(scope)
}

return orderedPropertiesToGenerate
}

private func generateProperties(leadingMemberWhitespace: String) async throws -> [String] {
var generatedProperties = [String]()
for childGenerator in try orderedPropertiesToGenerate {
for childGenerator in orderedPropertiesToGenerate {
try await generatedProperties.append(
childGenerator
.generateCode(leadingWhitespace: leadingMemberWhitespace)
Expand All @@ -421,17 +406,11 @@ actor ScopeGenerator: CustomStringConvertible, Sendable {

private enum GenerationError: Error, CustomStringConvertible {
case erasedInstantiatorGenericDoesNotMatch(property: Property, instantiable: Instantiable)
case dependencyCycleDetected([Property], scope: ScopeGenerator)

var description: String {
switch self {
case let .erasedInstantiatorGenericDoesNotMatch(property, instantiable):
"Property `\(property.asSource)` on \(instantiable.concreteInstantiable.asSource) incorrectly configured. Property should instead be of type `\(Dependency.erasedInstantiatorType)<\(instantiable.concreteInstantiable.asSource).ForwardedProperties, \(property.typeDescription.asInstantiatedType.asSource)>`."
case let .dependencyCycleDetected(properties, scope):
"""
Dependency cycle detected on \(scope)!
\(properties.map(\.asSource).joined(separator: " -> "))
"""
}
}
}
Expand Down
67 changes: 59 additions & 8 deletions Tests/SafeDIToolTests/SafeDIToolCodeGenerationErrorTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,8 +301,8 @@ final class SafeDIToolCodeGenerationErrorTests: XCTestCase {
func test_run_onCodeWithReceivedPropertyThatRefersToCurrentInstantiable_throwsError() async throws {
await assertThrowsError(
"""
Dependency cycle detected!
AuthService -> AuthService
Dependency received in same chain it is instantiated!
@Instantiated authService: AuthService -> @Received authService: AuthService
"""
) {
try await executeSafeDIToolTest(
Expand Down Expand Up @@ -468,8 +468,8 @@ final class SafeDIToolCodeGenerationErrorTests: XCTestCase {
func test_run_onCodeWhereAliasedReceivedPropertyRefersToCurrentInstantiable_throwsError() async throws {
await assertThrowsError(
"""
Dependency cycle detected!
AuthService -> AuthService
Dependency received in same chain it is instantiated!
@Instantiated authService: AuthService -> @Received authService: AuthService
"""
) {
try await executeSafeDIToolTest(
Expand Down Expand Up @@ -911,8 +911,8 @@ final class SafeDIToolCodeGenerationErrorTests: XCTestCase {
func test_run_onCodeWithCircularReceivedDependencies_throwsError() async {
await assertThrowsError(
"""
Dependency cycle detected on Root!
a: A -> b: B -> c: C -> a: A
Dependency received in same chain it is instantiated!
@Instantiated a: A -> @Received b: B -> @Received c: C -> @Received a: A
"""
) {
try await executeSafeDIToolTest(
Expand Down Expand Up @@ -960,8 +960,59 @@ final class SafeDIToolCodeGenerationErrorTests: XCTestCase {
func test_run_onCodeWithCircularReceivedRenamedDependencies_throwsError() async {
await assertThrowsError(
"""
Dependency cycle detected on A!
b: B -> c: C -> renamedB: B -> b: B
Dependency received in same chain it is instantiated!
@Instantiated a: A -> @Received renamedB: B -> @Received c: C -> @Received a: A
Comment on lines +963 to +964
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test better mirrors the above test. I liked the parallelism

"""
) {
try await executeSafeDIToolTest(
swiftFileContent: [
"""
@Instantiable
public struct Root {
@Instantiated
private let a: A
@Instantiated
private let b: B
@Received(fulfilledByDependencyNamed: "b", ofType: B.self)
private let renamedB: B
@Instantiated
private let c: C
}
""",
"""
@Instantiable
public struct A {
@Received
private let renamedB: B
}
""",
"""
@Instantiable
public struct B {
@Received
private let c: C
}
""",
"""
@Instantiable
public struct C {
@Received
private let a: A
}
""",
],
buildDependencyTreeOutput: true,
filesToDelete: &filesToDelete
)
}
}

@MainActor
func test_run_onCodeWithMultipleCircularReceivedRenamedDependencies_throwsError() async {
await assertThrowsError(
"""
Dependency received in same chain it is instantiated!
@Instantiated c: C -> @Received renamedB: B -> @Received c: C
"""
) {
try await executeSafeDIToolTest(
Expand Down