uber / needle

Compile-time safe Swift dependency injection framework
Apache License 2.0
1.84k stars 144 forks source link

Runtime error with "properly generated needle code" #456

Open dbagwell opened 1 year ago

dbagwell commented 1 year ago

To my understanding, there are basically two steps the generator performs to ensure compile time safety.

  1. Try to ensure components are only instantiated from inside their parent components. This is so that pattern matching can be used to detect these instantiations and create the component graph.
  2. Ensure that all dependencies of a component are satisfied in one of it's parent components in the graph.

If one tries to instantiate a component outside of it's parent say like

final class SomeClass {

    private let component: ChildComponent 

    init(parentScope: Scope) {
        self.component = ChildComponent(parent: parentScope)
    }

}

The following error is produced at compile time

ChildComponent is instantiated incorrectly in {file_url}. All components must be instantiated by parent components, by passing `self` as the argument to the parent parameter.

However, because this uses pattern matching of the component name, .init syntax is not recognized. (This issue is also outlined here https://github.com/uber/needle/issues/394 )

The following code does not produce the previously mentioned error because the generator doesn't recognize it as a component instantiation:

final class SomeClass {

    private let component: ChildComponent 

    init(parentScope: Scope) {
        self.component = .init(parent: parentScope)
    }

}

This is fine as long as the parentScope satisfies all of ChildComponent's dependencies but the generator doesn't guarantee this.

For example, using the following components:

final class RootComponent: BootstrapComponent {
    var firstComponent: FirstComponent { return FirstComponent(parent: self) }
}

final class FirstComponent: Component<EmptyDependency> {
    var name: String { return "Some Name" }
    var secondComponent: SecondComponent { return SecondComponent(parent: self) }
}

protocol SecondDependency: Dependency {
    var name: String { get }
}

final class SecondComponent: Component<SecondDependency> {}

The following code crashes:

final class SomeClass {

    let secondComponent: SecondComponent

    init() {
        self.secondComponent = .init(parent: RootComponent())
    }

    func printName() {
        print(self.secondComponent.name) // Crashes here
    }

}

With the following error that is documented as This case should never occur with properly generated Needle code.:

Missing dependency provider factory for ["^", "RootComponent", "SecondComponent"]

I think in order to fix this issue, the generator would need to use the AST to detect component instantiations instead of just pattern matching, but I have no idea how feasible that is. If it can be done, it also shouldn't matter if components get instantiated outside of their parent at all because the AST would be used to detect them wherever they are in order to build the graph (it would be really nice if this were possible so that the boilerplate of defining child components in their parents is no longer necessary).

rudro commented 1 year ago

Yes, as you noticed .init breaks needle's understand of the graph and that causes runtime issues. As you noted, it's really hard to know the type of the class being constructed because the AST has no form of type-inference. In a simple one line function, maybe the return type can tell us the type but in the case you give above, we have to walk back to the let secondComponent: SecondComponent property and that would be quite hard and difficult to generalize for all cases.

When we initially came up with the API, we thought about using comments so that the you could just tell us who the children of a particular components were. We decided that comments were prone to typos and such, so we decide to go with the current pattern where a parent component must instantiate it's children somewhere in the body to let needle know about parent -> child relationships.

dbagwell commented 1 year ago

Yeah, I wasn't sure how easy it would be to use the AST to figure it all out properly.

I did have an interesting thought for a way to resolve the issue with .init and remove the required child component bolierplate code.

Instead of trying to detect child parent relationships by searching the parent for instantiations of its child components. It might be possible to instead use convenience initializers in in the child for each of it's possible parents.

ie:

final class BaseComponent: BootstrapComponent {}

final class A1Component: Component<EmptyDependency> {
    var name: String { return "A1" }

    private override init(parent: Scope) {
        super.init(parent: parent)
    }

    convenience init(parentComponent: BaseComponent) {
        self.init(parent: parentComponent)
    }
}

final class A2Component: Component<EmptyDependency> {
    var name: String { return "A2" }

    private override init(parent: Scope) {
        super.init(parent: parent)
    }

    convenience init(parentComponent: BaseComponent) {
        self.init(parent: parentComponent)
    }
}

protocol BDependency: Dependency {
    var name: String { get }
}

final class BComponent: Component<BDependency> {
    private override init(parent: Scope) {
        super.init(parent: parent)
    }

    convenience init(parentComponent: A1Component) {
        self.init(parent: parentComponent)
    }

    convenience init(parentComponent: A2Component) {
        self.init(parent: parentComponent)
    }
}

This replaces the boilerplate declaring child components in their parents with declaring parent components in their children. But in the majority of cases it is more likely a component has more children than parents, so the boilerplate should be reduced.

The generator can then create the dependency graph by identifiying the convenience initializers that match the expected pattern and using the parentComponent type names to link the component with its parents.

The private override init(parent: Scope) is there to prevent potential runtime errors that might happen if someone were to try using the component's designated initializer with a parent that the graph doesn't know about. the generator could enforce that the component override this init as private (as well as ensure that super.init isn't called anywhere else in the component) or produce an error at compile time. This would resolve the issue with .init because the generator no longer needs to try to detect component instantiations because the only available initializers are ones that require the components exact parent types as defined in the dependency graph.