p0lunin / teloc

Simple, compile-time DI framework for Rust
Apache License 2.0
147 stars 14 forks source link

Dependency lifetimes resolved through `ServiceProvider::parent` are not preserved #19

Closed aburkhalter512 closed 2 years ago

aburkhalter512 commented 2 years ago

Parent container lifetimes are not preserved during container resolution. This means if you fork a service provider, any resolved borrows have the lifetime of the forked service provider rather than the parent. I expect that any containers resolved have the lifetime of the service provider they were added to. The following code snippet produces a compiler error saying forked_provider does not live long enough, but I expect it to compile given that Singleton lives longer than forked_provider.

use teloc::{inject, Resolver, ServiceProvider};

#[derive(Debug, PartialEq, Dependency)]
struct Singleton {}

#[test]
fn test_forked_lifetime() {
    let provider = ServiceProvider::new().add_singleton::<Singleton>();
    let singleton: &Singleton = provider.resolve();

    let forked_singleton: &Singleton = {
        let forked_provider = provider.fork();
        forked_provider.resolve()
    };

    assert_eq!(singleton, forked_singleton);
}

I think this is a problem with the Selector trait that implemented for ServiceProvider. I think the Selector implementation that derefs the parent needs to handle lifetimes more accurate, which might mean a custom Selector implementation, I'm not totally sure.

p0lunin commented 2 years ago

Let's see on the Selector trait

pub trait Selector<S, I> {
    fn get(&self) -> &S;
}

As you can see, lifetime of the output type of the get function depends on the &self reference, which itself contains &self.parent reference in ServiceProvider. So, &S cannot depend on parent value because self does not know which lifetime has parent value, it's only contains reference with 'a lifetime which can reach only &self lifetime at maximum. That's how I understand rust borrowchecker works. If you be able to run this example, that means I was wrong. But now I don't see a way to fix problem you describe. Maybe GATs can help us, but I'm not sure at all.

p0lunin commented 2 years ago

Can you provide a code snippet from your project where you get this error? Maybe we can find another way to fix your issue.

aburkhalter512 commented 2 years ago

@p0lunin I modified your example to compile. Essentially, you have to pass around 2 lifetimes instead of 1, one for self and the other for the return value. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f8e308ad6e1718630792f504294a9641

I'm still pretty new to Rust so I'm unsure if GATs will be able to solve this problem. I do know they are close to becoming stable so I might take a shot at using them in my experiments.

aburkhalter512 commented 2 years ago

As for providing a code snippet from the project I'm hitting this issue on, I can't. It's proprietary :-/

But the snippet I posted in the main comment body is fundamentally the issue I am hitting: its a lifetime problem.

I do have workarounds for this problem, but they are far from ideal (essentially I have to manually resolve dependencies before forking the service provider and manually construct the object)

p0lunin commented 2 years ago

Ok, I'll try to change signature of the Selector trait, but even if i can, this is very inefficient way to contain two lifetimes in the trait definition.

p0lunin commented 2 years ago

@aburkhalter512 as I thought, GATs will save us: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=11cbed7dd6544b602b1c468f0f2609d2. This can help us to avoid unnecessary lifetimes in trait definition, so we do not pass lifetimes into Dependency, GetDependencies and other high-level abstractions. Yes, we need to create empty type Getter, but it will be internal implementation and user will see never this type. So, this issue is frozen until GATs will be implemented. There are tracking issue where you can track for the stabilization process. As I heard it will be in a few months.

aburkhalter512 commented 2 years ago

That looks super promising! It looks like there is one unresolved issue for GATs left so I'll take a crack at implementing this. Maybe we could have a GAT-master branch we could merge into while we wait for GATs to stabilize? We use GATs already in the project I'm working on, so its fine if I pull in a crate that uses them too.

p0lunin commented 2 years ago

You are welcome. I can give you access to the repository if it that's more convenient for you.

aburkhalter512 commented 2 years ago

That would be great! I wouldn't have to bug you as much for small things I don't currently have control of.

p0lunin commented 2 years ago

@aburkhalter512 https://github.com/p0lunin/teloc/invitations