result type of Result<Self,_> for inject macro

[ilhamfu]

i think it make more sense to use result type of Result when using inject macro, as some service might fail to initialize, ex api service need to check whether the endpoint is accessible or not.

Edit : just read on how inject macro work and it turns out it just create InejctBuilder Instance so i can work around with that. but having it with inject macro might be useful imo

[commonsensesoftware]

Using Result has many of the same challenges as function colorization. In fact, since Rust doesn't formally have constructors and the constructor is just a regular 'ol function and it is colored if it supports async. The crux of the problem occurs from a dependency chain of results:

C: Result<C, E3> → B: Result<B, E2> → A: Result<A, E1>

There is no guarantee nor requirement that all of the errors are the same or compatible. One approach would be to assume that the errors have an Into or simple conversion and render that. If there is no conversion or it's invalid, you get a compiler error. Things get considerably more complex if there are multiple possible results. Imagine two dependencies that produce two different errors. That should theoretically still be possible to code generate, but it's certainly more complex.

Although constructors in Rust are not the same as in other languages, a constructor should be infallible IMO. There are a few rare edge cases such as invalid arguments, but Rust does a pretty good job helping make that a non-issue (ex: no null, signed vs unsigned integers, etc).

All that being said, if you want to be able to use Result, then you should be able to. I fully expect there to be additional enhancements to the macro code generation and perhaps it should support Result. I think it just needs to flush out what that would look like and some of the obscure edge cases. I've tried to avoid those, but some crept up any; for example, impl Iterator<Item> only works with a constructor and non-injected types on bare struct definitions and tuple structs are assumed to implement Default.

The workaround for edge cases is meant to be solved by implementing Injectable yourself or using the builder functions. If you have some specific examples or additional thoughts about supporting Result, I'm open to them. A number of ideas and patterns have been flushed out by implementing by hand first and then reworking them into macro code generation.

[ilhamfu]

understandable, i was thinking the Result type is to be used as fail-safe when dependency get requested, so like when a code need to use certain api service and it get constructed but it return an Error, it basically mark the dependency as not registered with certain message. it also add more possibility for ServiceProvider::get() function to return Result<T,Err> instead of Option.

[commonsensesoftware]

A Result which addresses an error during construction is certainly a valid scenario.

ServiceProvider::get() intentionally returns Option because a service is not necessarily required to be registered. There are several use cases for that. A consumer might assume a default if the service is not registered or the service simply might not be used at all. For example:

// spell check if the service has been registered
if let Some(spelling) = provider.get::<dyn SpellChecker>() {
   if let Err(typo) = spelling.check(&text) {
      // TODO: report spelling error
   }
}

The validation features ensure that if dependencies are properly registered (which #[injectable] always does), they will be validated and enforced when the ServiceProvider is created. On the other hand, indicating a service is unregistered due to service construction from something like a failed API call would be a misnomer.

What I would expect to happen is returning a Result that becomes part of the resolution. Perhaps something like:

if let Some(result) = provider.get_required::<Result<Ref<dyn Service>, ConfigError>>() {
    match result {
        Ok(svc) => svc.invoke(),
        Err(error) => println!("{:#?}", error),
    }
}

One of the annoying things about Result is that it is an enumeration, which means the values all have to be Sized. That means it has to, at least, be wrapped in Box, but more likely Ref (e.g. Rc or Arc). It's workable, but kind of icky.

There's a few other possibilities, but they are just subjective design choices.

Option 1

Use an initialization method. I get the argument against this approach. Why not just initialize and configure in one go? It also likely requires registering as mutable when you only need it for initialization or use Interior Mutability. For example:

let service = provider.get_required_mut::<dyn Service>();

if let Err(error) = service.borrow_mut().init() {
    println!("An initialization error occurred.\n\n{:#?}", error);
}

Option 2

Another option would be use a factory to separate construction from initialization. How the factory achieves that might encapsulate something like in Option 1 or it might have full ownership of item creation. For example:

let factory = provider.get_required::<dyn Factory>();

match factory.create() {
    Ok(svc) => svc.invoke(),
    Err(error) => println!("{:#?}", error),
}

---

Naturally, if this behavior occurs when a lower-level dependency then it propagates up. This leads to a lot of handcrafted configuration - at the moment. I do think it's possible to get something like:

type ResolveResult<T, E> = Result<Ref<T>, E>;
type ResolveMutResult<T, E> = Result<RefMut<T>, E>;

It's not entirely unreasonable, even if consumption would look rather strange and verbose. For example:

struct InitFromApi {
    item: Ref<Item>,
}

#[injectable]
impl InitFromApi {
    fn new(result: Ref<ResolveResult<Item, ApiError>>) -> ResolveResult<Self, ApiError> {
        Ok(Ref::new(Self { item: result? }))
    } 
}

This can get pretty nasty if the struct maps to a trait. Self::new would likely prefer to return itself, but you can inject it into a trait without converting it. I would expect to the macro would have to transform the result as follows:

let next: ResolveResult<Ref<dyn Service>, ApiError> =
    sp.get_required::<ResolveResult<Item, ApiError>>().map(|s| Ref::new(s));

Hopefully, that highlights some of the complexity. I'm not sure people would want Result injected as an argument. If Result isn't injected, then the macro needs to know a lot more about how to use a dependency to produce the return value. I'm not writing off as impossible, but it is definitely not trivial.

[commonsensesoftware]

It's also worth mentioning that I have a similar problem in the more-options crate. OptionsFactory::create has all the mechanics to perform validation and return Result<T, ValidationResult>, but there currently isn't a way to surface that in a graceful way.

Unfortunately, that means the only other option is to panic (a la Result::unwrap) when OptionsManager::get calls into the factory. In this particular use case, it's probably ok because that means the application is in a bad state and is unrecoverable. However, I'd prefer to propagate the Result and let the caller decide.

I'm not sure what throwing Result into the mix really looks like, but there is a use case for it. I'm certainly invested in thinking about how this issue might be addressed. 😉

[ilhamfu]

indicating a service is unregistered due to service construction from something like a failed API call would be a misnomer

true. my suggestion about using Result<Self,_> is just suggestion, and after thinking Result doesn't really fit this situation, probably enum to add more variant for the return type, probably for bailing etc. there is some scenario where i think its kinda useful. like for notification service, i can define a config struct which contain option of configs for all of notification provider that i want to use, and for some reason i don't want a notification service, i just need to set the configs for those service as None, and when my service try to register it, it just need to return, for ex ServiceRegistraction::Cancel

What I would expect to happen is returning a Result that becomes part of the resolution

that might work but it just too verbose IMO