Check types and params for optionals

[recht]

If I have one provider like this:

type Params struct {
  fx.In

  Dep SomeDep `optional:"true"`
}

and is SomeDep has a constructor function registered in Fx which is invalid (because it cannot be satisfied) then it's just ignored silently. For pluggable mechanisms this is pretty dangerous since you might make a small change to a type which makes it incompatible, and that then means that functionality is missing. Is there any reason for allowing anything registered in Fx to be invalid? Or is this just a bug?

[glibsm]

@recht I see the confusion, but everything is working as intended here.

The optional:"true" instructs dig that it's ok for SomeDep to be missing from container. Remove that tag and you will see the error message about a failed constructor. Optionals were added exactly for the case when something is not critical.

[recht]

Sure, but if I added it to the container and it's not valid shouldn't that trigger an error? Especially because I might not be in control of the optional tag.

[glibsm]

Sure, but if I added it to the container and it's not valid shouldn't that trigger an error?

Not necessarily. I agree that your intent perhaps was not captured correctly, but it's very difficult for Fx to understand exactly what you meant by optional, and all errors currently are ignored.

Consider the following example:

fx.Provide(func() A{})
fx.Provide(func(C) B{})
fx.Provide(func(D) C{})
// A -> {}
// B -> C
// C -> D (missing)

type Param struct {
  fx.In

  MyA A
  MyB B `optional:"true"`
}
fx.Invoke(func(Param) {...})

In the end Param.A is populated, but Param.B will be empty value (since the transitive dependency C was not in a container). To draw a parallel to your earlier example, B was provided to the container, but it's dependencies were not.

And on and on it can go spanning multiple levels. For that very reason it will never error out. Does that make sense?

Better visibility into the errors is definitely on the radar, as well as a potential event system https://github.com/uber-go/dig/issues/132. We plan to make it easier to debug and understand optionals, but it would be a breaking change to introduce a switch in how optional operates, as it will have a cascading effect throughout the whole ecosystem we have built up.

[recht]

I still don't understand why it's legal to register something into Fx which cannot be instantiated? It seems more or less trivial to look at all components registered and check if it's possible to satisfy all non-optional parameters. Or what am I missing?

[glibsm]

If components were checked upon registration, a strict order would have to be enforced on the constructors getting registered which is difficult/impossible given the modular nature of Fx.

You're free at any point to declare a dependency on any other types, i.e. the graph can be built in any order.

The ordering is maintained for Invokes as that has a massive implication on the actual graph resolution, i.e. which objects get created and executed.

Not all objects in the graph are expected to be called/instantiated.

[abhinav]

Hey,

Being able to register things without using them is an explicit design requirement for Fx. The idea is that you can register a standard kitchen-sink of a module (like one of our internal modules) without any runtime cost. Nothing will be instantiated unless you explicitly requested it.

Keeping that requirement in mind, note that originally, the Fx container wasn't "closed". You could register things into it after setting up the fx.App, so dependencies can be satisfied in the future if thy aren't right now. Therefore it made sense to allow registering whatever without any extra validation. This behavior was not altered when Fx transitioned to closing the container off.

This brings up an interesting point though. With the closed container, since you can add no more constructors, we know everything that can be provided has been provided. We could start validating that everything is satisfiable to eliminate this class of bugs.

[abhinav]

Actually, just a quick follow up: Validation doesn't make sense even with a closed container because of the "I want to provide something without using it" requirement.

Imagine you have an HTTP server module which has a constructor similar to,

package http

func New(http.Handler) *http.Server

With the expectation that the end-user is supposed to provide the http.Handler.

And you have a kitchensink module similar to our internal module.

package kitchensink

var Module = fx.Options(
    http.Module,
    logging.Module,
    metrics.Module,
    ...
)

In your application, when you want to set up an HTTP server, the expectation is to do,

fx.New(
    kitchensink.Module,
    fx.Provide(
        func(..) http.Handler {
            // ...
        },
    ),
)

If we were to validate satisfiability of every provided constructor, this would mean that for the case where you don't want to use an HTTP server, you'll see an error message asking you to provide an http.Handler anyway.

[recht]

That makes sense - I just assumed that's why optional exists, so that you can declare on the server provider that it doesn't need a handler.

I get why it works as it does right now. I just have a very hard time seeing the case where someone writes a service (not a library) and you don't want to have everything that's provided being valid.

For now, I have added this to my Invoke block:

func (http.Handler) {}

This ensures that my handler is actually being created and wired up. It's not a lot of code, it's just weird to have to have it.

[dplepage-dd]

fx.Provide(func() A{})
fx.Provide(func(C) B{})
fx.Provide(func(D) C{})
// A -> {}
// B -> C
// C -> D (missing)

type Param struct {
  fx.In

  MyA A
  MyB B `optional:"true"`
}
fx.Invoke(func(Param) {...})

This example seems like a bug to me - if there were no provider for B I would expect Param.MyB to be empty, but since there IS a provider for B I would expect that this provider would fail and I would get an error, in the same way it would if there were any other bug in the provider for B. Otherwise 'optional' is a black hole that hides all missing dependency errors. Is there any option to make recursive missing dependencies appear as errors?