Repository: uber-go/fx
Pull request #750 (2021-06-24 22:11:15)

fxreflect: Delete ReturnTypes in favor of dig.Info
With uber-go/dig#280 done, we can use the ProvideInfo produced by Dig to
determine what types will be produced by a constructor and log them, so we
no longer need to reimplement the `dig.Out` inspection logic in Fx.

To surface this information with the new event API introduced in #747, this
adds a new field to fxevent.Provide:

    OutputTypeNames []string

This, in lieu of exposing `dig.Info` wholesale ensures that we have some
wiggle room here.

Note that this introduces one behavioral difference on when the Provide
event is logged: previously, the Provide event would be logged whether
it succeeded or failed. I've created GO-677 internally to discuss
consistency here.

With this change, it will be logged only if it succeeds -- after it
succeeds. This makes sense because otherwise we can't extract information
about the function from Dig.

This temporarily pins Fx to the unreleased `master` branch of Dig. We'll
have to tag a releaes of Dig before we can release this feature.

Refs GO-676


codecov[bot]: # [Codecov](https://codecov.io/gh/uber-go/fx/pull/750?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) Report
> Merging [#750](https://codecov.io/gh/uber-go/fx/pull/750?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) (3b1b5be) into [master](https://codecov.io/gh/uber-go/fx/commit/95337c559ed988a843a305ef45dae1b0d77ddc2f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) (95337c5) will **decrease** coverage by `0.46%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/uber-go/fx/pull/750/graphs/tree.svg?width=650&height=150&src=pr&token=PYY9PSfaC6&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go)](https://codecov.io/gh/uber-go/fx/pull/750?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go)

```diff
@@            Coverage Diff             @@
##           master     #750      +/-   ##
==========================================
- Coverage   97.92%   97.45%   -0.47%     
==========================================
  Files          22       22              
  Lines         530      511      -19     
==========================================
- Hits          519      498      -21     
- Misses          9       11       +2     
  Partials        2        2              
```


| [Impacted Files](https://codecov.io/gh/uber-go/fx/pull/750?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) | Coverage Δ | |
|---|---|---|
| [fxevent/event.go](https://codecov.io/gh/uber-go/fx/pull/750/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-ZnhldmVudC9ldmVudC5nbw==) | `100.00% <ø> (ø)` | |
| [fxevent/zap.go](https://codecov.io/gh/uber-go/fx/pull/750/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-ZnhldmVudC96YXAuZ28=) | `100.00% <ø> (ø)` | |
| [internal/fxreflect/fxreflect.go](https://codecov.io/gh/uber-go/fx/pull/750/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-aW50ZXJuYWwvZnhyZWZsZWN0L2Z4cmVmbGVjdC5nbw==) | `88.23% <ø> (-11.77%)` | :arrow_down: |
| [app.go](https://codecov.io/gh/uber-go/fx/pull/750/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-YXBwLmdv) | `96.00% <100.00%> (+0.21%)` | :arrow_up: |
| [supply.go](https://codecov.io/gh/uber-go/fx/pull/750/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-c3VwcGx5Lmdv) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/uber-go/fx/pull/750?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/uber-go/fx/pull/750?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go). Last update [95337c5...3b1b5be](https://codecov.io/gh/uber-go/fx/pull/750?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go).


Patch:
diff --git a/app.go b/app.go
index 35fe585..4f30e89 100644
--- a/app.go
+++ b/app.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2020 Uber Technologies, Inc.
+// Copyright (c) 2020-2021 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -632,14 +632,6 @@ func (app *App) provide(p provide) {
 	}
 
 	constructor := p.Target
-
-	switch {
-	case p.IsSupply:
-		app.log.LogEvent(&fxevent.Supply{TypeName: p.SupplyType.String()})
-	default:
-		app.log.LogEvent(&fxevent.Provide{Constructor: constructor})
-	}
-
 	if _, ok := constructor.(Option); ok {
 		app.err = fmt.Errorf("fx.Option should be passed to fx.New directly, "+
 			"not to fx.Provide: fx.Provide received %v from:\n%+v",
@@ -647,8 +639,32 @@ func (app *App) provide(p provide) {
 		return
 	}
 
+	var info dig.ProvideInfo
+	opts := []dig.ProvideOption{
+		dig.FillProvideInfo(&info),
+	}
+	defer func() {
+		if app.err != nil {
+			return
+		}
+
+		switch {
+		case p.IsSupply:
+			app.log.LogEvent(&fxevent.Supply{TypeName: p.SupplyType.String()})
+		default:
+			outputNames := make([]string, len(info.Outputs))
+			for i, o := range info.Outputs {
+				outputNames[i] = o.String()
+			}
+
+			app.log.LogEvent(&fxevent.Provide{
+				Constructor:     constructor,
+				OutputTypeNames: outputNames,
+			})
+		}
+	}()
+
 	if ann, ok := constructor.(Annotated); ok {
-		var opts []dig.ProvideOption
 		switch {
 		case len(ann.Group) > 0 && len(ann.Name) > 0:
 			app.err = fmt.Errorf(
@@ -685,7 +701,7 @@ func (app *App) provide(p provide) {
 		}
 	}
 
-	if err := app.container.Provide(constructor); err != nil {
+	if err := app.container.Provide(constructor, opts...); err != nil {
 		app.err = fmt.Errorf("fx.Provide(%v) from:\n%+vFailed: %v", fxreflect.FuncName(constructor), p.Stack, err)
 	}
 }
diff --git a/fxevent/event.go b/fxevent/event.go
index b50b899..8f17d8c 100644
--- a/fxevent/event.go
+++ b/fxevent/event.go
@@ -20,7 +20,9 @@
 
 package fxevent
 
-import "os"
+import (
+	"os"
+)
 
 // Event defines an event emitted by fx.
 type Event interface {
@@ -63,9 +65,13 @@ type Supply struct {
 	TypeName string
 }
 
-// Provide is emitted whenever Provide was called and is not provided by fx.Supply.
+// Provide is emitted when we add a constructor to the container.
 type Provide struct {
 	Constructor interface{}
+
+	// OutputTypeNames is a list of names of types that are produced by
+	// this constructor.
+	OutputTypeNames []string
 }
 
 // Invoke is emitted whenever a function is invoked.
diff --git a/fxevent/zap.go b/fxevent/zap.go
index ada0511..3283122 100644
--- a/fxevent/zap.go
+++ b/fxevent/zap.go
@@ -46,7 +46,7 @@ func (l *ZapLogger) LogEvent(event Event) {
 	case *Supply:
 		l.Logger.Info("supplying", zap.String("type", e.TypeName))
 	case *Provide:
-		for _, rtype := range fxreflect.ReturnTypes(e.Constructor) {
+		for _, rtype := range e.OutputTypeNames {
 			l.Logger.Info("providing",
 				zap.String("constructor", fxreflect.FuncName(e.Constructor)),
 				zap.String("type", rtype),
diff --git a/go.mod b/go.mod
index 3b03410..8fb4a2f 100644
--- a/go.mod
+++ b/go.mod
@@ -10,3 +10,5 @@ require (
 	go.uber.org/zap v1.16.0
 	golang.org/x/sys v0.0.0-20190412213103-97732733099d
 )
+
+replace go.uber.org/dig => go.uber.org/dig v1.11.1-0.20210622212612-931c2ba30782
diff --git a/go.sum b/go.sum
index 6ecbab1..e75a858 100644
--- a/go.sum
+++ b/go.sum
@@ -21,8 +21,8 @@ github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJy
 github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
 go.uber.org/atomic v1.6.0 h1:Ezj3JGmsOnG1MoRWQkPBsKLe9DwWD9QeXzTRzzldNVk=
 go.uber.org/atomic v1.6.0/go.mod h1:sABNBOSYdrvTF6hTgEIbc7YasKWGhgEQZyfxyTvoXHQ=
-go.uber.org/dig v1.11.0 h1:tGTnPJE8TIozwn2VdsAsK7yr7sxtI5IHFYHujIeLf1w=
-go.uber.org/dig v1.11.0/go.mod h1:X34SnWGr8Fyla9zQNO2GSO2D+TIuqB14OS8JhYocIyw=
+go.uber.org/dig v1.11.1-0.20210622212612-931c2ba30782 h1:wfMqAcA7VmYM63riKRCFC2yNFtoRf5Fmg2TsEao3t+w=
+go.uber.org/dig v1.11.1-0.20210622212612-931c2ba30782/go.mod h1:X34SnWGr8Fyla9zQNO2GSO2D+TIuqB14OS8JhYocIyw=
 go.uber.org/goleak v0.10.0 h1:G3eWbSNIskeRqtsN/1uI5B+eP73y3JUuBsv9AZjehb4=
 go.uber.org/goleak v0.10.0/go.mod h1:VCZuO8V8mFPlL0F5J5GK1rtHV3DrFcQ1R8ryq7FK0aI=
 go.uber.org/multierr v1.5.0 h1:KCa4XfM8CWFCpxXRGok+Q0SS/0XBhMDbHHGABQLvD2A=
diff --git a/internal/fxreflect/fxreflect.go b/internal/fxreflect/fxreflect.go
index ccdb3b7..c4ab1c6 100644
--- a/internal/fxreflect/fxreflect.go
+++ b/internal/fxreflect/fxreflect.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2019 Uber Technologies, Inc.
+// Copyright (c) 2019-2021 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -27,77 +27,11 @@ import (
 	"regexp"
 	"runtime"
 	"strings"
-
-	"go.uber.org/dig"
 )
 
 // Match from beginning of the line until the first `vendor/` (non-greedy)
 var vendorRe = regexp.MustCompile("^.*?/vendor/")
 
-// ReturnTypes takes a func and returns a slice of string'd types.
-func ReturnTypes(t interface{}) []string {
-	if reflect.TypeOf(t).Kind() != reflect.Func {
-		// Invalid provide, will be logged as an error.
-		return []string{}
-	}
-
-	rtypes := []string{}
-	ft := reflect.ValueOf(t).Type()
-
-	for i := 0; i < ft.NumOut(); i++ {
-		t := ft.Out(i)
-
-		traverseOuts(key{t: t}, func(s string) {
-			rtypes = append(rtypes, s)
-		})
-	}
-
-	return rtypes
-}
-
-type key struct {
-	t    reflect.Type
-	name string
-}
-
-func (k *key) String() string {
-	if k.name != "" {
-		return fmt.Sprintf("%v:%s", k.t, k.name)
-	}
-	return k.t.String()
-}
-
-func traverseOuts(k key, f func(s string)) {
-	// skip errors
-	if isErr(k.t) {
-		return
-	}
-
-	// call function on non-Out types
-	if dig.IsOut(k.t) {
-		// keep recursing down on field members in case they are ins
-		for i := 0; i < k.t.NumField(); i++ {
-			field := k.t.Field(i)
-			ft := field.Type
-
-			if field.PkgPath != "" {
-				continue // skip private fields
-			}
-
-			// keep recursing to traverse all the embedded objects
-			k := key{
-				t:    ft,
-				name: field.Tag.Get("name"),
-			}
-			traverseOuts(k, f)
-		}
-
-		return
-	}
-
-	f(k.String())
-}
-
 // sanitize makes the function name suitable for logging display. It removes
 // url-encoded elements from the `dot.git` package names and shortens the
 // vendored paths.
@@ -128,11 +62,6 @@ func FuncName(fn interface{}) string {
 	return fmt.Sprintf("%s()", sanitize(function))
 }
 
-func isErr(t reflect.Type) bool {
-	errInterface := reflect.TypeOf((*error)(nil)).Elem()
-	return t.Implements(errInterface)
-}
-
 // Ascend the call stack until we leave the Fx production code. This allows us
 // to avoid hard-coding a frame skip, which makes this code work well even
 // when it's wrapped.
diff --git a/supply.go b/supply.go
index ae16129..4b68591 100644
--- a/supply.go
+++ b/supply.go
@@ -94,15 +94,9 @@ func (o supplyOption) apply(app *App) {
 
 func (o supplyOption) String() string {
 	items := make([]string, 0, len(o.Targets))
-	for _, target := range o.Targets {
-		switch target := target.(type) {
-		case Annotated:
-			items = append(items, fxreflect.ReturnTypes(target.Target)...)
-		default:
-			items = append(items, fxreflect.ReturnTypes(target)...)
-		}
+	for _, typ := range o.Types {
+		items = append(items, typ.String())
 	}
-
 	return fmt.Sprintf("fx.Supply(%s)", strings.Join(items, ", "))
 }
 


Test patch:
diff --git a/app_test.go b/app_test.go
index bde3d50..b85343a 100644
--- a/app_test.go
+++ b/app_test.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2020 Uber Technologies, Inc.
+// Copyright (c) 2020-2021 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -35,7 +35,6 @@ import (
 	"go.uber.org/fx/fxevent"
 	"go.uber.org/fx/fxtest"
 	"go.uber.org/fx/internal/fxlog"
-	"go.uber.org/fx/internal/fxreflect"
 	"go.uber.org/multierr"
 )
 
@@ -70,7 +69,8 @@ func TestNewApp(t *testing.T) {
 		require.Equal(t,
 			[]string{"Provide", "Provide", "Provide", "Provide", "Running"},
 			spy.EventTypes())
-		assert.Contains(t, fxreflect.ReturnTypes(spy.Events()[0].(*fxevent.Provide).Constructor), "struct {}")
+
+		assert.Contains(t, spy.Events()[0].(*fxevent.Provide).OutputTypeNames, "struct {}")
 	})
 
 	t.Run("CircularGraphReturnsError", func(t *testing.T) {
@@ -426,8 +426,8 @@ func TestOptions(t *testing.T) {
 			Provide(&bytes.Buffer{}), // error, not a constructor
 			WithLogger(spy),
 		)
-		require.Equal(t, []string{"Provide", "ProvideError"}, spy.EventTypes())
-		assert.Contains(t, spy.Events()[1].(*fxevent.ProvideError).Err.Error(), "must provide constructor function")
+		require.Equal(t, []string{"ProvideError"}, spy.EventTypes())
+		assert.Contains(t, spy.Events()[0].(*fxevent.ProvideError).Err.Error(), "must provide constructor function")
 	})
 }
 
diff --git a/fxevent/zap_test.go b/fxevent/zap_test.go
index 2002646..eecdf10 100644
--- a/fxevent/zap_test.go
+++ b/fxevent/zap_test.go
@@ -91,7 +91,7 @@ func TestZapLogger(t *testing.T) {
 	})
 	t.Run("Provide", func(t *testing.T) {
 		defer ts.Reset()
-		zapLogger.LogEvent(&Provide{bytes.NewBuffer})
+		zapLogger.LogEvent(&Provide{bytes.NewBuffer, []string{"*bytes.Buffer"}})
 		ts.AssertMessages("INFO\tproviding\t{\"constructor\": \"bytes.NewBuffer()\", \"type\": \"*bytes.Buffer\"}")
 	})
 	t.Run("Invoke", func(t *testing.T) {
diff --git a/internal/fxreflect/fxreflect_test.go b/internal/fxreflect/fxreflect_test.go
index e1e25ae..f2aaa36 100644
--- a/internal/fxreflect/fxreflect_test.go
+++ b/internal/fxreflect/fxreflect_test.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2019 Uber Technologies, Inc.
+// Copyright (c) 2019-2021 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -21,69 +21,11 @@
 package fxreflect
 
 import (
-	"bytes"
-	"errors"
-	"log"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"go.uber.org/dig"
 )
 
-type hollerer interface {
-	Holler()
-}
-
-type impl struct{}
-
-func (impl) Holler() {}
-
-type result struct {
-	dig.Out // referencing fx introduces import cycles
-
-	buff   *bytes.Buffer
-	Logger *log.Logger
-}
-
-func TestReturnTypes(t *testing.T) {
-	t.Run("non-function", func(t *testing.T) {
-		assert.Empty(t, ReturnTypes(42))
-	})
-	t.Run("primitive", func(t *testing.T) {
-		fn := func() (int, string) {
-			return 0, ""
-		}
-		assert.Equal(t, []string{"int", "string"}, ReturnTypes(fn))
-	})
-	t.Run("pointer", func(t *testing.T) {
-		type s struct{}
-		fn := func() *s {
-			return &s{}
-		}
-		assert.Equal(t, []string{"*fxreflect.s"}, ReturnTypes(fn))
-	})
-	t.Run("interface", func(t *testing.T) {
-		fn := func() hollerer {
-			return impl{}
-		}
-		assert.Equal(t, []string{"fxreflect.hollerer"}, ReturnTypes(fn))
-	})
-	t.Run("result struct", func(t *testing.T) {
-		fn := func() result {
-			return result{
-				buff: bytes.NewBufferString("foo"),
-			}
-		}
-		assert.Equal(t, []string{"*log.Logger"}, ReturnTypes(fn))
-	})
-	t.Run("skips errors", func(t *testing.T) {
-		fn := func() (string, error) {
-			return "", errors.New("err")
-		}
-		assert.Equal(t, []string{"string"}, ReturnTypes(fn))
-	})
-}
-
 func TestCaller(t *testing.T) {
 	assert.Equal(t, "go.uber.org/fx/internal/fxreflect.TestCaller", Caller())
 }