uber-go / fx

A dependency injection based application framework for Go.
https://uber-go.github.io/fx/
MIT License
5.65k stars 287 forks source link

When combining fx.As to annotate return type as interface with optional parameters nil value check is not working as expected #1145

Open kklimonda-fn opened 8 months ago

kklimonda-fn commented 8 months ago

Describe the bug When using an annotated return type of the function along with optional tag, the tagged parameter is not nil in a if b.Client == nil comparison

To Reproduce An example program:

package main

import (
    "context"
    "fmt"
    "log"
    "net/http"
    "reflect"

    "go.uber.org/fx"
)

func NewAProvide() (*http.Client, error) {
    return nil, nil
}

type B struct {
    Client HTTPClient
}

type BParams struct {
    fx.In

    Client HTTPClient `optional:"true"`
}

type HTTPClient interface {
    Do(req *http.Request) (*http.Response, error)
}

func NewBProvide(in BParams) (*B, error) {
    return &B{Client: in.Client}, nil
}

func main() {
    app := fx.New(
        fx.Provide(fx.Annotate(NewAProvide, fx.As(new(HTTPClient))), NewBProvide),
        //fx.Provide(NewAProvide, NewBProvide),
        fx.Invoke(func(b *B) {
            fmt.Printf("reflect.ValueOf(b.Client).IsNil() == %v\n", reflect.ValueOf(b.Client).IsNil())
            if b.Client == nil {
                log.Fatalf("B client is nil: %v (%v)", b.Client, reflect.TypeOf(b.Client))
            } else {
                log.Fatalf("B client is not nil: %v (%v)", b.Client, reflect.TypeOf(b.Client))
            }
        }))
    err := app.Start(context.Background())
    if err != nil {
        log.Fatalf("Error starting app: %v", err)
    }
}

Expected behavior I'd expect to see a message similar to:

B client is nil: <nil> (<nil>)

instead of

B client is not nil: <nil> (*http.Client)

Additional context Add any other context about the problem here.

abhinav commented 6 months ago

Hello! This is working as expected.

For background, the concept of typed nils comes from Go.

var v *http.Client = nil
var i HTTPClient = v
fmt.Println(v == nil) // true
fmt.Println(i == nil) // false

// https://go.dev/play/p/ZZS2T9XhMnb

It's a common gotcha, but it's completely valid to have a nil implementation in a non-nil interface.

type myClient struct{}

func (c *myClient) Do(req *http.Request) (*http.Response, error) {
    if c == nil {
        return http.DefaultClient.Do(req)
    }

    panic("TODO: custom logic here")
}

// Same as before:
var c *myClient = nil
var i HTTPClient = c
fmt.Println(i == nil) // false

// Can still send requests:
res, err := i.Do(req)
// ...

By providing a function that returns a nil *http.Client, and casting it to an HTTPClient, we're asking for a non-nil HTTPClient with a nil *http.Client inside it.

From Fx's point of view, this is behaving as expected. Fx should not be trying to glean the meaning of a typed nil. It should not discard my nil-safe implementation of an interface.