Repository: uber-go/fx
Pull request #687 (2019-11-15 22:53:39)

fxreflect: Add CallStack
This adds an `fxreflect.CallStack` function that generalizes prior
`Caller()` logic.

Each commit should be reviewed individually.

codecov[bot]: # [Codecov](https://codecov.io/gh/uber-go/fx/pull/687?src=pr&el=h1) Report
> Merging [#687](https://codecov.io/gh/uber-go/fx/pull/687?src=pr&el=desc) into [master](https://codecov.io/gh/uber-go/fx/commit/aefec2f2749986fdc0a61bd10f0fb5479a5a1ba0?src=pr&el=desc) will **increase** coverage by `1.08%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/uber-go/fx/pull/687/graphs/tree.svg?width=650&token=PYY9PSfaC6&height=150&src=pr)](https://codecov.io/gh/uber-go/fx/pull/687?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #687      +/-   ##
==========================================
+ Coverage   96.83%   97.92%   +1.08%     
==========================================
  Files          12       13       +1     
  Lines         443      481      +38     
==========================================
+ Hits          429      471      +42     
+ Misses         11        8       -3     
+ Partials        3        2       -1
```


| [Impacted Files](https://codecov.io/gh/uber-go/fx/pull/687?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/fxreflect/stack.go](https://codecov.io/gh/uber-go/fx/pull/687/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZnhyZWZsZWN0L3N0YWNrLmdv) | `100% <100%> (ø)` | |
| [internal/fxreflect/fxreflect.go](https://codecov.io/gh/uber-go/fx/pull/687/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZnhyZWZsZWN0L2Z4cmVmbGVjdC5nbw==) | `100% <100%> (+6.66%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/uber-go/fx/pull/687?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/uber-go/fx/pull/687?src=pr&el=footer). Last update [aefec2f...c6440e9](https://codecov.io/gh/uber-go/fx/pull/687?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

twilly: Seems good to me.

Patch:
diff --git a/internal/fxreflect/fxreflect.go b/internal/fxreflect/fxreflect.go
index 631bee7..ccdb3b7 100644
--- a/internal/fxreflect/fxreflect.go
+++ b/internal/fxreflect/fxreflect.go
@@ -114,30 +114,14 @@ func sanitize(function string) string {
 
 // Caller returns the formatted calling func name
 func Caller() string {
-	// Ascend at most 8 frames looking for a caller outside fx.
-	pcs := make([]uintptr, 8)
-
-	// Don't include this frame.
-	n := runtime.Callers(2, pcs)
-	if n == 0 {
-		return "n/a"
-	}
-
-	frames := runtime.CallersFrames(pcs)
-	for f, more := frames.Next(); more; f, more = frames.Next() {
-		if shouldIgnoreFrame(f) {
-			continue
-		}
-		return sanitize(f.Function)
-	}
-	return "n/a"
+	return CallerStack(1, 0).CallerName()
 }
 
 // FuncName returns a funcs formatted name
 func FuncName(fn interface{}) string {
 	fnV := reflect.ValueOf(fn)
 	if fnV.Kind() != reflect.Func {
-		return "n/a"
+		return fmt.Sprint(fn)
 	}
 
 	function := runtime.FuncForPC(fnV.Pointer()).Name()
@@ -152,12 +136,22 @@ func isErr(t reflect.Type) bool {
 // 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.
-func shouldIgnoreFrame(f runtime.Frame) bool {
+func shouldIgnoreFrame(f Frame) bool {
+	// Treat test files as leafs.
 	if strings.Contains(f.File, "_test.go") {
 		return false
 	}
-	if strings.Contains(f.File, "go.uber.org/fx") {
+
+	// The unique, fully-qualified name for all functions begins with
+	// "{{importPath}}.". We'll ignore Fx and its subpackages.
+	s := strings.TrimPrefix(f.Function, "go.uber.org/fx")
+	if len(s) > 0 && s[0] == '.' || s[0] == '/' {
+		// We want to match,
+		//   go.uber.org/fx.Foo
+		//   go.uber.org/fx/something.Foo
+		// But not, go.uber.org/fxfoo
 		return true
 	}
+
 	return false
 }
diff --git a/internal/fxreflect/stack.go b/internal/fxreflect/stack.go
new file mode 100644
index 0000000..02048cf
--- /dev/null
+++ b/internal/fxreflect/stack.go
@@ -0,0 +1,149 @@
+// Copyright (c) 2019 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
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package fxreflect
+
+import (
+	"fmt"
+	"io"
+	"runtime"
+	"strings"
+)
+
+// Frame holds information about a single frame in the call stack.
+type Frame struct {
+	// Unique, package path-qualified name for the function of this call
+	// frame.
+	Function string
+
+	// File and line number of our location in the frame.
+	//
+	// Note that the line number does not refer to where the function was
+	// defined but where in the function the next call was made.
+	File string
+	Line int
+}
+
+func (f Frame) String() string {
+	// This takes the following forms.
+	//  (path/to/file.go)
+	//  (path/to/file.go:42)
+	//  path/to/package.MyFunction
+	//  path/to/package.MyFunction (path/to/file.go)
+	//  path/to/package.MyFunction (path/to/file.go:42)
+
+	var sb strings.Builder
+	sb.WriteString(f.Function)
+	if len(f.File) > 0 {
+		if sb.Len() > 0 {
+			sb.WriteRune(' ')
+		}
+		fmt.Fprintf(&sb, "(%v", f.File)
+		if f.Line > 0 {
+			fmt.Fprintf(&sb, ":%d", f.Line)
+		}
+		sb.WriteRune(')')
+	}
+
+	if sb.Len() == 0 {
+		return "unknown"
+	}
+
+	return sb.String()
+}
+
+const _defaultCallersDepth = 8
+
+// Stack is a stack of call frames.
+//
+// Formatted with %v, the output is in a single-line, in the form,
+//
+//   foo/bar.Baz() (path/to/foo.go:42); bar/baz.Qux() (bar/baz/qux.go:12); ...
+//
+// Formatted with %+v, the output is in the form,
+//
+//   foo/bar.Baz()
+//   	path/to/foo.go:42
+//   bar/baz.Qux()
+//   	bar/baz/qux.go:12
+type Stack []Frame
+
+// Returns a single-line, semi-colon representation of a Stack. For a
+// multi-line representation, use %+v.
+func (fs Stack) String() string {
+	items := make([]string, len(fs))
+	for i, f := range fs {
+		items[i] = f.String()
+	}
+	return strings.Join(items, "; ")
+}
+
+// Format implements fmt.Formatter to handle "%+v".
+func (fs Stack) Format(w fmt.State, c rune) {
+	if !w.Flag('+') {
+		// Without %+v, fall back to String().
+		io.WriteString(w, fs.String())
+		return
+	}
+
+	for _, f := range fs {
+		fmt.Fprintln(w, f.Function)
+		fmt.Fprintf(w, "\t%v:%v\n", f.File, f.Line)
+	}
+}
+
+// CallerName returns the name of the first caller in this stack that isn't
+// owned by the Fx library.
+func (fs Stack) CallerName() string {
+	for _, f := range fs {
+		if shouldIgnoreFrame(f) {
+			continue
+		}
+		return f.Function
+	}
+	return "n/a"
+}
+
+// CallerStack returns the call stack for the calling function, up to depth frames
+// deep, skipping the provided number of frames, not including Callers itself.
+//
+// If zero, depth defaults to 8.
+func CallerStack(skip, depth int) Stack {
+	if depth <= 0 {
+		depth = _defaultCallersDepth
+	}
+
+	pcs := make([]uintptr, depth)
+
+	// +2 to skip this frame and runtime.Callers.
+	n := runtime.Callers(skip+2, pcs)
+	pcs = pcs[:n] // truncate to number of frames actually read
+
+	result := make([]Frame, 0, n)
+	frames := runtime.CallersFrames(pcs)
+	for f, more := frames.Next(); more; f, more = frames.Next() {
+		result = append(result, Frame{
+			Function: sanitize(f.Function),
+			File:     f.File,
+			Line:     f.Line,
+		})
+	}
+	return result
+}


Test patch:
diff --git a/internal/fxreflect/fxreflect_test.go b/internal/fxreflect/fxreflect_test.go
index aab87a0..8a51e95 100644
--- a/internal/fxreflect/fxreflect_test.go
+++ b/internal/fxreflect/fxreflect_test.go
@@ -89,8 +89,28 @@ func TestCaller(t *testing.T) {
 func someFunc() {}
 
 func TestFuncName(t *testing.T) {
-	assert.Equal(t, "go.uber.org/fx/internal/fxreflect.someFunc()", FuncName(someFunc))
-	assert.Equal(t, "n/a", FuncName(struct{}{}))
+	tests := []struct {
+		desc string
+		give interface{}
+		want string
+	}{
+		{
+			desc: "function",
+			give: someFunc,
+			want: "go.uber.org/fx/internal/fxreflect.someFunc()",
+		},
+		{
+			desc: "not a function",
+			give: 42,
+			want: "42",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			assert.Equal(t, tt.want, FuncName(tt.give))
+		})
+	}
 }
 
 func TestSanitizeFuncNames(t *testing.T) {
diff --git a/internal/fxreflect/stack_test.go b/internal/fxreflect/stack_test.go
new file mode 100644
index 0000000..5acfda4
--- /dev/null
+++ b/internal/fxreflect/stack_test.go
@@ -0,0 +1,236 @@
+// Copyright (c) 2019 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
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package fxreflect
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestStack(t *testing.T) {
+	// NOTE:
+	// We don't assert the length of the stack because we cannot make
+	// guarantees about how many frames the test runner is allowed to
+	// introduce.
+
+	t.Run("default", func(t *testing.T) {
+		frames := CallerStack(0, 0)
+		require.NotEmpty(t, frames)
+		f := frames[0]
+		assert.Equal(t, "go.uber.org/fx/internal/fxreflect.TestStack.func1", f.Function)
+		assert.Contains(t, f.File, "internal/fxreflect/stack_test.go")
+		assert.NotZero(t, f.Line)
+	})
+
+	t.Run("default/deeper", func(t *testing.T) {
+		// Introduce a few frames.
+		frames := func() []Frame {
+			return func() []Frame {
+				return CallerStack(0, 0)
+			}()
+		}()
+
+		require.True(t, len(frames) > 3, "expected at least three frames")
+		for i, name := range []string{"func2.1.1", "func2.1", "func2"} {
+			f := frames[i]
+			assert.Equal(t, "go.uber.org/fx/internal/fxreflect.TestStack."+name, f.Function)
+			assert.Contains(t, f.File, "internal/fxreflect/stack_test.go")
+			assert.NotZero(t, f.Line)
+		}
+	})
+
+	t.Run("skip", func(t *testing.T) {
+		// Introduce a few frames and skip 2.
+		frames := func() []Frame {
+			return func() []Frame {
+				return CallerStack(2, 0)
+			}()
+		}()
+
+		require.NotEmpty(t, frames)
+		f := frames[0]
+		assert.Equal(t, "go.uber.org/fx/internal/fxreflect.TestStack.func3", f.Function)
+		assert.Contains(t, f.File, "internal/fxreflect/stack_test.go")
+		assert.NotZero(t, f.Line)
+	})
+}
+
+func TestStackCallerName(t *testing.T) {
+	tests := []struct {
+		desc string
+		give Stack
+		want string
+	}{
+		{desc: "empty", want: "n/a"},
+		{
+			desc: "skip Fx components",
+			give: Stack{
+				{
+					Function: "go.uber.org/fx.Foo()",
+					File:     "go.uber.org/fx/foo.go",
+				},
+				{
+					Function: "foo/bar.Baz()",
+					File:     "foo/bar/baz.go",
+				},
+			},
+			want: "foo/bar.Baz()",
+		},
+		{
+			desc: "skip Fx in wrong directory",
+			give: Stack{
+				{
+					Function: "go.uber.org/fx.Foo()",
+					File:     "fx/foo.go",
+				},
+				{
+					Function: "foo/bar.Baz()",
+					File:     "foo/bar/baz.go",
+				},
+			},
+			want: "foo/bar.Baz()",
+		},
+		{
+			desc: "skip Fx subpackage",
+			give: Stack{
+				{
+					Function: "go.uber.org/fx/internal/fxreflect.Foo()",
+					File:     "fx/internal/fxreflect/foo.go",
+				},
+				{
+					Function: "foo/bar.Baz()",
+					File:     "foo/bar/baz.go",
+				},
+			},
+			want: "foo/bar.Baz()",
+		},
+		{
+			desc: "don't skip Fx tests",
+			give: Stack{
+				{
+					Function: "some/thing.Foo()",
+					File:     "go.uber.org/fx/foo_test.go",
+				},
+			},
+			want: "some/thing.Foo()",
+		},
+		{
+			desc: "don't skip fx prefix",
+			give: Stack{
+				{
+					Function: "go.uber.org/fxfoo.Bar()",
+					File:     "go.uber.org/fxfoo/bar.go",
+				},
+			},
+			want: "go.uber.org/fxfoo.Bar()",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			assert.Equal(t, tt.want, tt.give.CallerName())
+		})
+	}
+}
+
+func TestFrameString(t *testing.T) {
+	tests := []struct {
+		desc string
+		give Frame
+		want string
+	}{
+		{
+			desc: "zero",
+			give: Frame{},
+			want: "unknown",
+		},
+		{
+			desc: "file and line",
+			give: Frame{File: "foo.go", Line: 42},
+			want: "(foo.go:42)",
+		},
+		{
+			desc: "file only",
+			give: Frame{File: "foo.go"},
+			want: "(foo.go)",
+		},
+		{
+			desc: "function only",
+			give: Frame{Function: "foo"},
+			want: "foo",
+		},
+		{
+			desc: "function and file",
+			give: Frame{Function: "foo", File: "bar.go"},
+			want: "foo (bar.go)",
+		},
+		{
+			desc: "function and line",
+			give: Frame{Function: "foo", Line: 42},
+			want: "foo", // line without file is meaningless
+		},
+		{
+			desc: "function, file, and line",
+			give: Frame{Function: "foo", File: "bar.go", Line: 42},
+			want: "foo (bar.go:42)",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			assert.Equal(t, tt.want, tt.give.String())
+		})
+	}
+}
+
+func TestStackFormat(t *testing.T) {
+	stack := Stack{
+		{
+			Function: "path/to/module.SomeFunction()",
+			File:     "path/to/file.go",
+			Line:     42,
+		},
+		{
+			Function: "path/to/another/module.AnotherFunction()",
+			File:     "path/to/another/file.go",
+			Line:     12,
+		},
+	}
+
+	t.Run("single line", func(t *testing.T) {
+		assert.Equal(t,
+			"path/to/module.SomeFunction() (path/to/file.go:42); "+
+				"path/to/another/module.AnotherFunction() (path/to/another/file.go:12)",
+			fmt.Sprintf("%v", stack))
+	})
+
+	t.Run("multi line", func(t *testing.T) {
+		assert.Equal(t, `path/to/module.SomeFunction()
+	path/to/file.go:42
+path/to/another/module.AnotherFunction()
+	path/to/another/file.go:12
+`, fmt.Sprintf("%+v", stack))
+	})
+
+}