ProvideOption to override digreflect.Func

[aaronc]

Problem

It is really hard to debug errors with constructors created by reflection because the error will just say "reflect".makeFuncStub (/Users/arc/sdk/go1.16.3/src/reflect/asm_amd64.s:14).

Proposed Solution

I propose creating a ProvideOption that allows setting digreflect.Func manually so that frameworks that are building functions with reflection can override this information to provide better error messages to users. I imagine this would improve Fx as well because Supply builds a constructor by reflection.

[aaronc]

I opened #282 with an example of how this would look.

[prashantv]

Thanks to @abhinav for flagging this as similar to my request on fx: uber-go/fx#604

I took a look at #282 and while it solves this problem, I could see potential issues with invalid pkg/func/file/lines being passed in. One alternate approach that allows for a better caller without allowing arbitrary values could be to capture the location within dig, e.g.,

// CallerLocation is similar to runtime.Caller, but instead returns a single struct with unexported fields representing the location information.
loc := dig.CallerLocation(1 /* callerSkip */)

// loc implements ProvideOption so it can be passed in to Provide
dig.Provide(..., loc)

This allows dig to capture the caller stack (not just the single caller frame). What do you think of this approach?

[aaronc]

Thanks for taking a look @prashantv.

So I understand your concern about invalid values being passed in. At a high-level, I do want to ask whether the goal of dig is more to be a DI container for casual users who are likely to mess things up or for framework authors who are going to make sure things are correct. It seems from the README that dig is more targeted at framework authors so I would argue dig should make debug info as tweakable as possible and let frameworks like Fx make sure users can't do anything really terrible.

That said, the runtime.Caller approach would only work if the function we're trying to wrap with reflection is in the call stack. In my particular use case I'm trying to wrap a struct method, so passing a function pointer would work better. Something like this could work:

fptr := reflect.TypeOf(MyStruct{}).Method(0).Func.Pointer()
loc := dig.FuncPointerLocation(fptr)
dig.Provide(..., loc)

For Fx's Supply method though, I don't actually think either approach would work well and a fully customizable approach like in #282 is probably be better.

[prashantv]

Regarding DI container vs framework: we primarily use it within the fx framework, but I do think there's some use-cases where it's used directly.

For fx.Supply, my expectation is that the framework tracks where the value is provided -- so we'd capture the caller of fx.Supply, in which case the proposed CallerLocation API should work fine (fx.Supply would call CallerLocation and pass that in, so the stack doesn't point to the reflect function).

The function pointer approach gives a single call frame, not the full call stack (similar to the Location provide option), I thought it might be useful to capture the full call stack.

[aaronc]

The function pointer approach gives a single call frame, not the full call stack (similar to the Location provide option), I thought it might be useful to capture the full call stack.

A full call-stack would only apply in the fx.Supply case from what I can tell. With regular constructors or functions wrapped by reflection (as in my use case), there is no call stack just a function pointer until constructor invocation.

[aaronc]

@prashantv I updated #282 to the function pointer approach which should work for fx.Supply as well as the reflection-based use cases I can imagine. Can you take another look?

[abhinav]

I think the current function pointer approach, as implemented in #282, might be good enough for now, especially because it leaves room in the future for providing arbitrary locations, as well as full-fledged call-stack capturing. @prashantv WDYT?