From da1e9e90e116e566c1148fdd2c2a75f31eac63d8 Mon Sep 17 00:00:00 2001 From: Steve Coffman Date: Sat, 4 May 2024 20:27:28 -0400 Subject: [PATCH 1/9] Add TraceCaller interface for extensibility Signed-off-by: Steve Coffman --- errtrace.go | 12 ++++++++++++ unwrap.go | 26 ++++++++++++++++++++++---- unwrap_test.go | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 4 deletions(-) diff --git a/errtrace.go b/errtrace.go index 62f5b02..5d811f3 100644 --- a/errtrace.go +++ b/errtrace.go @@ -118,3 +118,15 @@ func (e *errTrace) Format(s fmt.State, verb rune) { fmt.Fprintf(s, fmt.FormatString(s, verb), e.err) } + +// TraceCall returns the program counter for the location in this frame. +func (e *errTrace) TraceCall() uintptr { + return e.pc +} + +type TraceCaller interface { + TraceCall() uintptr +} + +// compile time TraceCaller interface check +var _ TraceCaller = &errTrace{} diff --git a/unwrap.go b/unwrap.go index 4a1844c..068443d 100644 --- a/unwrap.go +++ b/unwrap.go @@ -9,18 +9,36 @@ import "runtime" // // You can use this for structured access to trace information. func UnwrapFrame(err error) (frame runtime.Frame, inner error, ok bool) { //nolint:revive // error is intentionally middle return - e, ok := err.(*errTrace) + e, ok := err.(TraceCaller) if !ok { return runtime.Frame{}, err, false } - frames := runtime.CallersFrames([]uintptr{e.pc}) + wrapErr := UnwrapOnce(err) + frames := runtime.CallersFrames([]uintptr{e.TraceCall()}) f, _ := frames.Next() if f == (runtime.Frame{}) { // Unlikely, but if PC didn't yield a frame, // just return the inner error. - return runtime.Frame{}, e.err, false + return runtime.Frame{}, wrapErr, false } - return f, e.err, true + return f, wrapErr, true +} + +// UnwrapOnce accesses the direct cause of the error if any, otherwise +// returns nil. +// +// It supports both errors implementing causer (`Cause()` method, from +// github.com/pkg/errors) and `Wrapper` (`Unwrap()` method, from the +// Go 2 error proposal). +func UnwrapOnce(err error) error { + switch e := err.(type) { + case interface{ Cause() error }: + return e.Cause() + case interface{ Unwrap() error }: + return e.Unwrap() + } + + return nil } diff --git a/unwrap_test.go b/unwrap_test.go index 2f1ff20..6a3d130 100644 --- a/unwrap_test.go +++ b/unwrap_test.go @@ -53,3 +53,52 @@ func TestUnwrapFrame_badPC(t *testing.T) { t.Errorf("inner: got %v, want %v", inner, giveErr) } } + +func TestUnwrapOnce(t *testing.T) { + rootErr := New("root") + var errTrace *errTrace + errors.As(rootErr, &errTrace) + unwrapped := errTrace.err + wrapper := Wrap(rootErr) + + type want struct { + wantErr bool + matchErr error + matchString string + } + tests := []struct { + name string + arg error + want want + }{ + { + name: "unwrap wrapped provides root", + arg: wrapper, + want: want{ + wantErr: true, + matchErr: rootErr, + matchString: "root", + }, + }, + { + name: "unwrap root provides unwrapped", + arg: rootErr, + want: want{ + wantErr: true, + matchErr: unwrapped, + matchString: "root", + }, + }, + {name: "unwrap nil provides nil"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := UnwrapOnce(tt.arg) + if (err != nil) != tt.want.wantErr { + t.Errorf("UnwrapOnce() error = %v, but wantErr %v", err, tt.want.wantErr) + } else if !errors.Is(err, tt.want.matchErr) { + t.Errorf("UnwrapOnce() error = %v, does not match matchErr %v", err, tt.want.matchErr) + } + }) + } +} From 557f9930065283f0115429a8a7fad5fab4bee0a5 Mon Sep 17 00:00:00 2001 From: Steve Coffman Date: Sun, 5 May 2024 17:45:45 -0400 Subject: [PATCH 2/9] Address linter and add another line of coverage Signed-off-by: Steve Coffman --- errtrace.go | 5 +++++ unwrap_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/errtrace.go b/errtrace.go index 5d811f3..09ad995 100644 --- a/errtrace.go +++ b/errtrace.go @@ -124,6 +124,11 @@ func (e *errTrace) TraceCall() uintptr { return e.pc } +// TraceCaller is a provider of the Program Counter +// that the error originated with. +// The returned PC is intended to be used with +// runtime.CallersFrames or runtime.FuncForPC +// to aid in generating the error return trace type TraceCaller interface { TraceCall() uintptr } diff --git a/unwrap_test.go b/unwrap_test.go index 6a3d130..c8c0f6e 100644 --- a/unwrap_test.go +++ b/unwrap_test.go @@ -54,6 +54,19 @@ func TestUnwrapFrame_badPC(t *testing.T) { } } +// caused follows the pkg/errors Cause interface +type caused struct { + err error +} + +func (e *caused) Error() string { + return e.err.Error() +} + +func (e *caused) Cause() error { + return e.err +} + func TestUnwrapOnce(t *testing.T) { rootErr := New("root") var errTrace *errTrace @@ -61,6 +74,8 @@ func TestUnwrapOnce(t *testing.T) { unwrapped := errTrace.err wrapper := Wrap(rootErr) + causedErr := &caused{rootErr} + type want struct { wantErr bool matchErr error @@ -90,6 +105,15 @@ func TestUnwrapOnce(t *testing.T) { }, }, {name: "unwrap nil provides nil"}, + { + name: "unwrap caused provides root", + arg: causedErr, + want: want{ + wantErr: true, + matchErr: rootErr, + matchString: "root", + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From bd1be4a209ab24450bbb6c07362ed927844d6f33 Mon Sep 17 00:00:00 2001 From: Steve Coffman Date: Sun, 5 May 2024 21:25:46 -0400 Subject: [PATCH 3/9] Make changes per review Signed-off-by: Steve Coffman --- errtrace.go | 20 ++++++++++++-------- unwrap.go | 10 +++++----- unwrap_test.go | 6 +++--- 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/errtrace.go b/errtrace.go index 09ad995..540570a 100644 --- a/errtrace.go +++ b/errtrace.go @@ -69,7 +69,7 @@ func wrap(err error, callerPC uintptr) error { } // Format writes the return trace for given error to the writer. -// The output takes a fromat similar to the following: +// The output takes a format similar to the following: // // // @@ -79,6 +79,8 @@ func wrap(err error, callerPC uintptr) error { // : // [...] // +// Any error that has a method `TracePC() uintptr` will +// contribute to the trace. // If the error doesn't have a return trace attached to it, // only the error message is reported. // If the error is comprised of multiple errors (e.g. with [errors.Join]), @@ -90,6 +92,8 @@ func Format(w io.Writer, target error) (err error) { } // FormatString writes the return trace for err to a string. +// Any error that has a method `TracePC() uintptr` will +// contribute to the trace. // See [Format] for details of the output format. func FormatString(target error) string { var s strings.Builder @@ -119,19 +123,19 @@ func (e *errTrace) Format(s fmt.State, verb rune) { fmt.Fprintf(s, fmt.FormatString(s, verb), e.err) } -// TraceCall returns the program counter for the location in this frame. -func (e *errTrace) TraceCall() uintptr { +// TracePC returns the program counter for the location in this frame. +func (e *errTrace) TracePC() uintptr { return e.pc } -// TraceCaller is a provider of the Program Counter +// tracePCprovider is a provider of the Program Counter // that the error originated with. // The returned PC is intended to be used with // runtime.CallersFrames or runtime.FuncForPC // to aid in generating the error return trace -type TraceCaller interface { - TraceCall() uintptr +type tracePCprovider interface { + TracePC() uintptr } -// compile time TraceCaller interface check -var _ TraceCaller = &errTrace{} +// compile time tracePCprovider interface check +var _ tracePCprovider = &errTrace{} diff --git a/unwrap.go b/unwrap.go index 068443d..33e0b53 100644 --- a/unwrap.go +++ b/unwrap.go @@ -9,13 +9,13 @@ import "runtime" // // You can use this for structured access to trace information. func UnwrapFrame(err error) (frame runtime.Frame, inner error, ok bool) { //nolint:revive // error is intentionally middle return - e, ok := err.(TraceCaller) + e, ok := err.(interface{ TracePC() uintptr }) if !ok { return runtime.Frame{}, err, false } - wrapErr := UnwrapOnce(err) - frames := runtime.CallersFrames([]uintptr{e.TraceCall()}) + wrapErr := unwrapOnce(err) + frames := runtime.CallersFrames([]uintptr{e.TracePC()}) f, _ := frames.Next() if f == (runtime.Frame{}) { // Unlikely, but if PC didn't yield a frame, @@ -26,13 +26,13 @@ func UnwrapFrame(err error) (frame runtime.Frame, inner error, ok bool) { //noli return f, wrapErr, true } -// UnwrapOnce accesses the direct cause of the error if any, otherwise +// unwrapOnce accesses the direct cause of the error if any, otherwise // returns nil. // // It supports both errors implementing causer (`Cause()` method, from // github.com/pkg/errors) and `Wrapper` (`Unwrap()` method, from the // Go 2 error proposal). -func UnwrapOnce(err error) error { +func unwrapOnce(err error) error { switch e := err.(type) { case interface{ Cause() error }: return e.Cause() diff --git a/unwrap_test.go b/unwrap_test.go index c8c0f6e..1228d04 100644 --- a/unwrap_test.go +++ b/unwrap_test.go @@ -117,11 +117,11 @@ func TestUnwrapOnce(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := UnwrapOnce(tt.arg) + err := unwrapOnce(tt.arg) if (err != nil) != tt.want.wantErr { - t.Errorf("UnwrapOnce() error = %v, but wantErr %v", err, tt.want.wantErr) + t.Errorf("unwrapOnce() error = %v, but wantErr %v", err, tt.want.wantErr) } else if !errors.Is(err, tt.want.matchErr) { - t.Errorf("UnwrapOnce() error = %v, does not match matchErr %v", err, tt.want.matchErr) + t.Errorf("unwrapOnce() error = %v, does not match matchErr %v", err, tt.want.matchErr) } }) } From c102ca887ee8f37770a0059dca43b972704668df Mon Sep 17 00:00:00 2001 From: Steve Coffman Date: Mon, 6 May 2024 06:51:22 -0400 Subject: [PATCH 4/9] more review changes Signed-off-by: Steve Coffman --- errtrace.go | 16 +++++------ unwrap.go | 6 ----- unwrap_test.go | 73 -------------------------------------------------- 3 files changed, 6 insertions(+), 89 deletions(-) diff --git a/errtrace.go b/errtrace.go index 540570a..0633b1f 100644 --- a/errtrace.go +++ b/errtrace.go @@ -123,19 +123,15 @@ func (e *errTrace) Format(s fmt.State, verb rune) { fmt.Fprintf(s, fmt.FormatString(s, verb), e.err) } -// TracePC returns the program counter for the location in this frame. -func (e *errTrace) TracePC() uintptr { - return e.pc -} - -// tracePCprovider is a provider of the Program Counter -// that the error originated with. +// TracePC returns the program counter for the location +// in the frame that the error originated with. +// // The returned PC is intended to be used with // runtime.CallersFrames or runtime.FuncForPC // to aid in generating the error return trace -type tracePCprovider interface { - TracePC() uintptr +func (e *errTrace) TracePC() uintptr { + return e.pc } // compile time tracePCprovider interface check -var _ tracePCprovider = &errTrace{} +var _ interface{ TracePC() uintptr } = &errTrace{} diff --git a/unwrap.go b/unwrap.go index 33e0b53..113407c 100644 --- a/unwrap.go +++ b/unwrap.go @@ -28,14 +28,8 @@ func UnwrapFrame(err error) (frame runtime.Frame, inner error, ok bool) { //noli // unwrapOnce accesses the direct cause of the error if any, otherwise // returns nil. -// -// It supports both errors implementing causer (`Cause()` method, from -// github.com/pkg/errors) and `Wrapper` (`Unwrap()` method, from the -// Go 2 error proposal). func unwrapOnce(err error) error { switch e := err.(type) { - case interface{ Cause() error }: - return e.Cause() case interface{ Unwrap() error }: return e.Unwrap() } diff --git a/unwrap_test.go b/unwrap_test.go index 1228d04..2f1ff20 100644 --- a/unwrap_test.go +++ b/unwrap_test.go @@ -53,76 +53,3 @@ func TestUnwrapFrame_badPC(t *testing.T) { t.Errorf("inner: got %v, want %v", inner, giveErr) } } - -// caused follows the pkg/errors Cause interface -type caused struct { - err error -} - -func (e *caused) Error() string { - return e.err.Error() -} - -func (e *caused) Cause() error { - return e.err -} - -func TestUnwrapOnce(t *testing.T) { - rootErr := New("root") - var errTrace *errTrace - errors.As(rootErr, &errTrace) - unwrapped := errTrace.err - wrapper := Wrap(rootErr) - - causedErr := &caused{rootErr} - - type want struct { - wantErr bool - matchErr error - matchString string - } - tests := []struct { - name string - arg error - want want - }{ - { - name: "unwrap wrapped provides root", - arg: wrapper, - want: want{ - wantErr: true, - matchErr: rootErr, - matchString: "root", - }, - }, - { - name: "unwrap root provides unwrapped", - arg: rootErr, - want: want{ - wantErr: true, - matchErr: unwrapped, - matchString: "root", - }, - }, - {name: "unwrap nil provides nil"}, - { - name: "unwrap caused provides root", - arg: causedErr, - want: want{ - wantErr: true, - matchErr: rootErr, - matchString: "root", - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := unwrapOnce(tt.arg) - if (err != nil) != tt.want.wantErr { - t.Errorf("unwrapOnce() error = %v, but wantErr %v", err, tt.want.wantErr) - } else if !errors.Is(err, tt.want.matchErr) { - t.Errorf("unwrapOnce() error = %v, does not match matchErr %v", err, tt.want.matchErr) - } - }) - } -} From 28a89e4a3b5ffd491b3025d6a095f25c44f98dbc Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Mon, 6 May 2024 05:50:45 -0700 Subject: [PATCH 5/9] Use errors.Unwrap --- unwrap.go | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/unwrap.go b/unwrap.go index 113407c..576d6c1 100644 --- a/unwrap.go +++ b/unwrap.go @@ -1,6 +1,9 @@ package errtrace -import "runtime" +import ( + "errors" + "runtime" +) // UnwrapFrame unwraps the outermost frame from the given error, // returning it and the inner error. @@ -14,7 +17,7 @@ func UnwrapFrame(err error) (frame runtime.Frame, inner error, ok bool) { //noli return runtime.Frame{}, err, false } - wrapErr := unwrapOnce(err) + wrapErr := errors.Unwrap(err) frames := runtime.CallersFrames([]uintptr{e.TracePC()}) f, _ := frames.Next() if f == (runtime.Frame{}) { @@ -25,14 +28,3 @@ func UnwrapFrame(err error) (frame runtime.Frame, inner error, ok bool) { //noli return f, wrapErr, true } - -// unwrapOnce accesses the direct cause of the error if any, otherwise -// returns nil. -func unwrapOnce(err error) error { - switch e := err.(type) { - case interface{ Unwrap() error }: - return e.Unwrap() - } - - return nil -} From 009dfa73d7d6c829fae69294c4849acf1915a694 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Mon, 6 May 2024 05:53:03 -0700 Subject: [PATCH 6/9] doc(UnwrapFrame): Mention TracePC() uintptr --- unwrap.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/unwrap.go b/unwrap.go index 576d6c1..63429fc 100644 --- a/unwrap.go +++ b/unwrap.go @@ -11,6 +11,9 @@ import ( // and false otherwise, or if the error is not an errtrace error. // // You can use this for structured access to trace information. +// +// Any error that has a method `TracePC() uintptr` will +// contribute a frame to the trace. func UnwrapFrame(err error) (frame runtime.Frame, inner error, ok bool) { //nolint:revive // error is intentionally middle return e, ok := err.(interface{ TracePC() uintptr }) if !ok { From eff2d7d85dcb4b11ea1adbf19590546a317d7ea9 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Mon, 6 May 2024 05:53:58 -0700 Subject: [PATCH 7/9] UnwrapFrame: Reuse name "inner" --- unwrap.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/unwrap.go b/unwrap.go index 63429fc..63261a8 100644 --- a/unwrap.go +++ b/unwrap.go @@ -20,14 +20,14 @@ func UnwrapFrame(err error) (frame runtime.Frame, inner error, ok bool) { //noli return runtime.Frame{}, err, false } - wrapErr := errors.Unwrap(err) + inner = errors.Unwrap(err) frames := runtime.CallersFrames([]uintptr{e.TracePC()}) f, _ := frames.Next() if f == (runtime.Frame{}) { // Unlikely, but if PC didn't yield a frame, // just return the inner error. - return runtime.Frame{}, wrapErr, false + return runtime.Frame{}, inner, false } - return f, wrapErr, true + return f, inner, true } From c9671b91d26ea1d6984fa84d07218a2bbccda5ea Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Mon, 6 May 2024 05:58:41 -0700 Subject: [PATCH 8/9] test(UnwrapFrame): Verify against custom TracePC --- unwrap_test.go | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/unwrap_test.go b/unwrap_test.go index 2f1ff20..70e8ad3 100644 --- a/unwrap_test.go +++ b/unwrap_test.go @@ -3,6 +3,7 @@ package errtrace import ( "errors" "path/filepath" + "reflect" "strings" "testing" ) @@ -40,6 +41,26 @@ func TestUnwrapFrame(t *testing.T) { t.Errorf("frame.File: got %v, want %v", got, want) } }) + + t.Run("custom error", func(t *testing.T) { + wrapped := wrapCustomTrace(giveErr) + frame, inner, ok := UnwrapFrame(wrapped) + if got, want := ok, true; got != want { + t.Errorf("ok: got %v, want %v", got, want) + } + + if got, want := inner, giveErr; got != want { + t.Errorf("inner: got %v, want %v", inner, giveErr) + } + + if got, want := frame.Function, ".wrapCustomTrace"; !strings.HasSuffix(got, want) { + t.Errorf("frame.Func: got %q, does not contain %q", got, want) + } + + if got, want := filepath.Base(frame.File), "unwrap_test.go"; got != want { + t.Errorf("frame.File: got %v, want %v", got, want) + } + }) } func TestUnwrapFrame_badPC(t *testing.T) { @@ -53,3 +74,27 @@ func TestUnwrapFrame_badPC(t *testing.T) { t.Errorf("inner: got %v, want %v", inner, giveErr) } } + +type customTraceError struct { + err error + pc uintptr +} + +func wrapCustomTrace(err error) error { + return &customTraceError{ + err: err, + pc: reflect.ValueOf(wrapCustomTrace).Pointer(), + } +} + +func (e *customTraceError) Error() string { + return e.err.Error() +} + +func (e *customTraceError) TracePC() uintptr { + return e.pc +} + +func (e *customTraceError) Unwrap() error { + return e.err +} From 1229395c2a1e41ca3cc50080c209b8963807bc35 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Mon, 6 May 2024 06:54:52 -0700 Subject: [PATCH 9/9] CHANGELOG: Add entry --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3320e4..f97be82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Add UnwrapFrame function to extract a single frame from an error. You can use this to implement your own trace formatting logic. +- Support extracting trace frames from custom errors. + Any error value that implements `TracePC() uintptr` will now + contribute to the trace. - cmd/errtrace: Add `-no-wrapn` option to disable wrapping with generic `WrapN` functions. This is only useful for toolexec mode due to tooling limitations.