Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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 this issue Jan 9, 2024 · 1 comment
Assignees
Labels

Comments

@kklimonda-fn
Copy link

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.

@sywhang sywhang self-assigned this Jan 11, 2024
@sywhang sywhang added the bug label Jan 11, 2024
@abhinav
Copy link
Collaborator

abhinav commented Feb 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants