commonsensesoftware / more-rs-di

Rust Dependency Injection (DI) framework
MIT License
21 stars 3 forks source link

Service cannot depend on other mutable service #19

Closed Drewol closed 10 months ago

Drewol commented 11 months ago

This might belong in #13 but it seemed like a different thing.

I've recreated a minimal example of something I ran into when trying out this crate.

use di::*;

#[injectable]
struct Foo;

#[injectable]
struct Bar {
    foo: RefMut<Foo>,
}

fn error_build() {
    ServiceCollection::new()
        .add(Foo::singleton().as_mut())
        .add(Bar::singleton())
        .build_provider()
        .unwrap();
    // ValidationError: "Service 'more_di_error::Bar' requires dependent service 'more_di_error::Foo', which has not be registered"
}

fn error_get() {
    let service_provider = ServiceCollection::new()
        .add(Foo::singleton())
        .add(Bar::singleton())
        .build_provider()
        .unwrap();

    service_provider.get_required::<Bar>();
    // No service for type 'std::sync::rwlock::RwLock<more_di_error::Foo>' has been registered.
}

fn main() {
    error_build();
    error_get();
}

It doesn't seem to matter if async is used. I just started with that because I was using it in my proper project.

It might just be an issue of the injectable macro not using the mutable version in the dependency list.

commonsensesoftware commented 11 months ago

This is indeed a bug. I can't believe I missed this case. As you suspected, it is in the macro generation. The construction part is correct, but the validation part isn't using the correct types. This should be a pretty simple fix. I'll work on getting it out ASAP. Thanks for trying it out and reporting the issue.

The issue can be verified using cargo expand (if you have it installed). Here is the macro expansion:

impl di::Injectable for Bar {
    fn inject(lifetime: di::ServiceLifetime) -> di::InjectBuilder {
        di::InjectBuilder::new(
                di::Activator::new::<
                    Self,
                    Self,
                >(
                    |sp: &di::ServiceProvider| di::Ref::new(Self {
                        foo: sp.get_required_mut::<Foo>(),
                    }),
                    |sp: &di::ServiceProvider| di::RefMut::new(
                        Self {
                            foo: sp.get_required_mut::<Foo>(),
                        }
                            .into(),
                    ),
                ),
                lifetime,
            )
            .depends_on(
                di::ServiceDependency::new(
                    di::Type::of::<Foo>(),
                    di::ServiceCardinality::ExactlyOne,
                ),
            )
    }
}

The dependency should be either di::Type::of::<RefCell<Foo>> for sync or di::Type::of::<RwLock<Foo>> for async. That is why the validation fails. You can see that the resolution method is actually correct.

commonsensesoftware commented 10 months ago

Doh! 🤦🏽 I figured out how this happened. I had everything in place to verify code gen, but no test actually exercised what was generated. The code was valid, but not the right code. Determining what should be generated for a mutable service is a little tricky since you can use RefMut or any of its compatible variants. It looks like I'm stuck making the Mut alias public for code generation purposes due to the variance between RefCell and RwLock, but I intend to the hide the documentation for it because you really shouldn't ever need to directly use it. It's likely confusing to use in all but the most advanced of scenarios.

I have something working with additional test coverage to prevent regression. I'll have the fix out later today.

commonsensesoftware commented 10 months ago

3.1.0 has been published and contains the necessary fix. Thanks for trying things out and reporting the issue.

Drewol commented 10 months ago

Works as expected now. Thanks!