Unconstructable optional types provided to the container should not silently fail

[dnathe4th]

Take the following minimal example:

package main

import (
	"fmt"
	"go.uber.org/dig"
)

type A struct {
	b *B
	c *C
}
type B struct{}
type C struct{}
type D struct{}

type paramsA struct {
	dig.In

	B *B
	C *C `optional:"true"`
}

func main() {
	c := dig.New()

	c.Provide(func(p paramsA) *A {
		return &A{
			b: p.B,
			c: p.C,
		}
	})
	c.Provide(func() *B { return &B{} })
	c.Provide(func(d *D) *C { return &C{} })

	if err := c.Invoke(func(a *A) {
		fmt.Printf("a.c == nil is %t \n", a.c == nil)
	}); err != nil {
		panic(err)
	}
}

Running this will return the string "a.c == nil is true". Despite the fact that a constructor was Provided to the container for *C, there is no feedback to the user that a.c == nil because *C's dependencies were not fulfilled. The only way to arrive at this conclusion without tracing back every optional dependency to all of their dependencies is to remove the optional:"true" tag temporarily and see what error dig returns when the container is resolved.

Instead, I contend that when an optional dependency does have a constructor Provided to the container, either (1) an error should always be returned if the optional dependency's dependencies cannot be met, or (2) a flag should able to be provided to the container (maybe some "strict" mode?) for cause optional dependency errors to be returned for easy debuggability.

[Groxx]

There's a design decision to be made between the current behavior and this request:

1. (current) If you ignore "optional-X has missing dependencies" and no-op, you allow libraries to provide stuff-using-X all the time for convenience, and only things that are constructable become non-nil. E.g. you could provide a few cache impls with different data sources, collect them optionally in a Cache provider, and only things with satisfied dependencies will be constructed. If none are, you can do some dependency-free default / return err / no-op as desired, and the user of your fx lib doesn't need to care (unless you choose to force them to care, by returning an err). This also comes with harder to diagnose "soft" errors, like you described.
2. If you require optional-x's to be satisfied, you're requiring that transitively. So lib authors need to make no-op versions of anything they provide that may be used optionally, and consumers need to make sure it's not a noop version if they want to ensure they have a usable instance (via... some interface? Valid() error maybe?). Lib authors that don't do this will need to produce an injectable module for every combination, and the end-user will need to select them by hand. This also comes with no "soft" errors (unless you ignore the noop check) as you can't "partially include" an optional thing, it's all or nothing.

So it's sorta a tradeoff between implicit and explicit behaviors, complicated because it applies transitively.

Potentially worth pointing out:

• "Kitchen sink" modules that combine a bunch of other common modules will often rely on 1, as otherwise there would be no practical way to provide things that may not be satisfiable in literally every case.
• Changing implicit to explicit is fairly easy. Add an Invoke and return an error if something you expect to exist is nil.
• Changing explicit to implicit is fairly hard. You effectively need to make a no-op of the dependency, which is basically only possible as the lib author, or if they provide / consumer-libs depend on an interface rather than a type (which is probably rare).

I'm not particularly fond of "kitchen sink" modules... but the ease of implicit -> explicit does kinda make me prefer 1. It also makes intent of "optional, but not to me" clear in code. The difficulty of diagnosing failures might be fixable / improve-able with more logging, which I'm basically always in favor of.

Since invoke does change behavior in cases where it's constructable but not used, I would still prefer #182 . That may not be hard to add?

[dnathe4th]

You are correct, I could have turned the soft error into a hard error but adding an c.Invoke() with the dependency only required optionally. I had not considered this debugging technique before rage-filing this task.

I similarly had not considered the usecase of intentionally providing things that may not have satisfied dependencies knowing that the only thing that depends on them does so optionally. Had you asked me, before your response, to design this theoretical caching library I would have put just the core cache constructor in the Module, and informed developers in the README about the various sourceX.New constructors available for them to provide in their container at their convenience, rather than providing every permutation in various Modules.

I'm not sure I follow the argument of lib authors needing to provide no-op versions in your second scenario. Are you talking about a scenario where the library is providing a type the library consumer is going to depend on optionally? Or the other way around?

Regardless, it does sound like this is squarely in "feature" not "bug" for better or worse. I will take away the Invoke strategy for debugging in the future. The [Blink] #182 idea of CanInvoke is interesting, but maybe only useful if you know about it, similar to just adding an Invoke on it's own, since we're talking about the manual testing stage of development.

[Groxx]

I had not considered this debugging technique before rage-filing this task.

It definitely does suck when it bites you. Implicit no-ops are frequently rage-inducing and take a long time to unravel, and I pretty much only know that strategy because I've fought with similar things a few times :)

I'm not sure I follow the argument of lib authors needing to provide no-op versions in your second scenario. Are you talking about a scenario where the library is providing a type the library consumer is going to depend on optionally? Or the other way around?

It'd just be to achieve the same "provide everything, only satisfied stuff gets constructed" behavior, with the opposite implicit/explicit decision. You'd need to construct and return something for all the stuff you might provide, but it won't be usable without further checks. (though I suppose a nil pointer is pretty clear too, if you don't put it behind an interface)

Not that I think that's a good alternative to sourceX.New + docs. I'd definitely do separate modules / provides, personally.

[Blink] #182