From 785921b1c1ecc027064eaf6616a7a01b4c292597 Mon Sep 17 00:00:00 2001 From: Dave Cheney Date: Thu, 14 Jul 2016 15:57:03 -0600 Subject: [PATCH 1/3] Destructure New/Errorf Destructure New/Errorf into two components, a call to the stdlib errors.New or fmt.Errorf to generate a _fundamental_ error, and then a call to withStack to attach a stack trace to the message. --- errors.go | 53 ++++++++++++++++++++++++++--------------------------- stack.go | 13 +++++++++++++ 2 files changed, 39 insertions(+), 27 deletions(-) diff --git a/errors.go b/errors.go index 71f1431..d3d2234 100644 --- a/errors.go +++ b/errors.go @@ -87,37 +87,16 @@ package errors import ( + "errors" "fmt" "io" ) -// _error is an error implementation returned by New and Errorf -// that implements its own fmt.Formatter. -type _error struct { - msg string - *stack -} - -func (e _error) Error() string { return e.msg } - -func (e _error) Format(s fmt.State, verb rune) { - switch verb { - case 'v': - if s.Flag('+') { - io.WriteString(s, e.msg) - fmt.Fprintf(s, "%+v", e.StackTrace()) - return - } - fallthrough - case 's': - io.WriteString(s, e.msg) - } -} - // New returns an error with the supplied message. func New(message string) error { - return _error{ - message, + err := errors.New(message) + return &withStack{ + err, callers(), } } @@ -125,12 +104,32 @@ func New(message string) error { // Errorf formats according to a format specifier and returns the string // as a value that satisfies error. func Errorf(format string, args ...interface{}) error { - return _error{ - fmt.Sprintf(format, args...), + err := fmt.Errorf(format, args...) + return &withStack{ + err, callers(), } } +type withStack struct { + error + *stack +} + +func (w *withStack) Format(s fmt.State, verb rune) { + switch verb { + case 'v': + if s.Flag('+') { + io.WriteString(s, w.Error()) + w.stack.Format(s, verb) + return + } + fallthrough + case 's': + io.WriteString(s, w.Error()) + } +} + type cause struct { cause error msg string diff --git a/stack.go b/stack.go index 243a64a..6b1f289 100644 --- a/stack.go +++ b/stack.go @@ -100,6 +100,19 @@ func (st StackTrace) Format(s fmt.State, verb rune) { // stack represents a stack of program counters. type stack []uintptr +func (s *stack) Format(st fmt.State, verb rune) { + switch verb { + case 'v': + switch { + case st.Flag('+'): + for _, pc := range *s { + f := Frame(pc) + fmt.Fprintf(st, "\n%+v", f) + } + } + } +} + func (s *stack) StackTrace() StackTrace { f := make([]Frame, len(*s)) for i := 0; i < len(f); i++ { From 777ed74de5b8cdca2147e5da066c3bd418c26083 Mon Sep 17 00:00:00 2001 From: Dave Cheney Date: Thu, 28 Jul 2016 20:36:54 +1000 Subject: [PATCH 2/3] Destructure Wrap{,f} into WithStack(WithMessage(err, msg)) Introduces WithMessage as well as errors.fundamental, errors.withMessage and errors.withStack internal types. Adjust tests for the new wrapped format when combining fundamental and wrapped errors. --- errors.go | 145 ++++++++++++++++++++++++++++++++----------------- format_test.go | 73 +++++++++++++++---------- stack_test.go | 12 ++-- 3 files changed, 146 insertions(+), 84 deletions(-) diff --git a/errors.go b/errors.go index d3d2234..915bbdb 100644 --- a/errors.go +++ b/errors.go @@ -87,24 +87,57 @@ package errors import ( - "errors" "fmt" "io" ) // New returns an error with the supplied message. +// New also records the stack trace at the point it was called. func New(message string) error { - err := errors.New(message) - return &withStack{ - err, - callers(), + return &fundamental{ + msg: message, + stack: callers(), } } // Errorf formats according to a format specifier and returns the string // as a value that satisfies error. +// Errorf also records the stack trace at the point it was called. func Errorf(format string, args ...interface{}) error { - err := fmt.Errorf(format, args...) + return &fundamental{ + msg: fmt.Sprintf(format, args...), + stack: callers(), + } +} + +// fundamental is an error that has a message and a stack, but no caller. +type fundamental struct { + msg string + *stack +} + +func (f *fundamental) Error() string { return f.msg } + +func (f *fundamental) Format(s fmt.State, verb rune) { + switch verb { + case 'v': + if s.Flag('+') { + io.WriteString(s, f.msg) + f.stack.Format(s, verb) + return + } + fallthrough + case 's', 'q': + io.WriteString(s, f.msg) + } +} + +// WithStack annotates err with a stack trace at the point WithStack was called. +// If err is nil, WithStack returns nil. +func WithStack(err error) error { + if err == nil { + return nil + } return &withStack{ err, callers(), @@ -116,47 +149,19 @@ type withStack struct { *stack } +func (w *withStack) Cause() error { return w.error } + func (w *withStack) Format(s fmt.State, verb rune) { switch verb { case 'v': if s.Flag('+') { - io.WriteString(s, w.Error()) + fmt.Fprintf(s, "%+v", w.Cause()) w.stack.Format(s, verb) return } fallthrough case 's': io.WriteString(s, w.Error()) - } -} - -type cause struct { - cause error - msg string -} - -func (c cause) Error() string { return fmt.Sprintf("%s: %v", c.msg, c.Cause()) } -func (c cause) Cause() error { return c.cause } - -// wrapper is an error implementation returned by Wrap and Wrapf -// that implements its own fmt.Formatter. -type wrapper struct { - cause - *stack -} - -func (w wrapper) Format(s fmt.State, verb rune) { - switch verb { - case 'v': - if s.Flag('+') { - fmt.Fprintf(s, "%+v\n", w.Cause()) - io.WriteString(s, w.msg) - fmt.Fprintf(s, "%+v", w.StackTrace()) - return - } - fallthrough - case 's': - io.WriteString(s, w.Error()) case 'q': fmt.Fprintf(s, "%q", w.Error()) } @@ -164,31 +169,73 @@ func (w wrapper) Format(s fmt.State, verb rune) { // Wrap returns an error annotating err with message. // If err is nil, Wrap returns nil. +// Wrap is conceptually the same as calling +// +// errors.WithStack(errors.WithMessage(err, msg)) func Wrap(err error, message string) error { if err == nil { return nil } - return wrapper{ - cause: cause{ - cause: err, - msg: message, - }, - stack: callers(), + err = &withMessage{ + cause: err, + msg: message, + } + return &withStack{ + err, + callers(), } } // Wrapf returns an error annotating err with the format specifier. // If err is nil, Wrapf returns nil. +// Wrapf is conceptually the same as calling +// +// errors.WithStack(errors.WithMessage(err, format, args...)) func Wrapf(err error, format string, args ...interface{}) error { if err == nil { return nil } - return wrapper{ - cause: cause{ - cause: err, - msg: fmt.Sprintf(format, args...), - }, - stack: callers(), + err = &withMessage{ + cause: err, + msg: fmt.Sprintf(format, args...), + } + return &withStack{ + err, + callers(), + } +} + +// WithMessage annotates err with a new message. +// If err is nil, WithStack returns nil. +func WithMessage(err error, message string) error { + if err == nil { + return nil + } + return &withMessage{ + cause: err, + msg: message, + } +} + +type withMessage struct { + cause error + msg string +} + +func (w *withMessage) Error() string { return w.msg + ": " + w.cause.Error() } +func (w *withMessage) Cause() error { return w.cause } + +func (w *withMessage) Format(s fmt.State, verb rune) { + switch verb { + case 'v': + if s.Flag('+') { + fmt.Fprintf(s, "%+v\n", w.Cause()) + io.WriteString(s, w.msg) + return + } + fallthrough + case 's', 'q': + io.WriteString(s, w.Error()) } } diff --git a/format_test.go b/format_test.go index d1b2f1b..3b1746c 100644 --- a/format_test.go +++ b/format_test.go @@ -29,8 +29,8 @@ func TestFormatNew(t *testing.T) { "\t.+/github.com/pkg/errors/format_test.go:25", }} - for _, tt := range tests { - testFormatRegexp(t, tt.error, tt.format, tt.want) + for i, tt := range tests { + testFormatRegexp(t, i, tt.error, tt.format, tt.want) } } @@ -55,8 +55,8 @@ func TestFormatErrorf(t *testing.T) { "\t.+/github.com/pkg/errors/format_test.go:51", }} - for _, tt := range tests { - testFormatRegexp(t, tt.error, tt.format, tt.want) + for i, tt := range tests { + testFormatRegexp(t, i, tt.error, tt.format, tt.want) } } @@ -83,14 +83,32 @@ func TestFormatWrap(t *testing.T) { Wrap(io.EOF, "error"), "%s", "error: EOF", + }, { + Wrap(io.EOF, "error"), + "%v", + "error: EOF", + }, { + Wrap(io.EOF, "error"), + "%+v", + "EOF\n" + + "error\n" + + "github.com/pkg/errors.TestFormatWrap\n" + + "\t.+/github.com/pkg/errors/format_test.go:91", + }, { + Wrap(Wrap(io.EOF, "error1"), "error2"), + "%+v", + "EOF\n" + + "error1\n" + + "github.com/pkg/errors.TestFormatWrap\n" + + "\t.+/github.com/pkg/errors/format_test.go:98\n", }, { Wrap(New("error with space"), "context"), "%q", `"context: error with space"`, }} - for _, tt := range tests { - testFormatRegexp(t, tt.error, tt.format, tt.want) + for i, tt := range tests { + testFormatRegexp(t, i, tt.error, tt.format, tt.want) } } @@ -100,20 +118,24 @@ func TestFormatWrapf(t *testing.T) { format string want string }{{ + Wrapf(io.EOF, "error%d", 2), + "%s", + "error2: EOF", + }, { + Wrapf(io.EOF, "error%d", 2), + "%v", + "error2: EOF", + }, { + Wrapf(io.EOF, "error%d", 2), + "%+v", + "EOF\n" + + "error2\n" + + "github.com/pkg/errors.TestFormatWrapf\n" + + "\t.+/github.com/pkg/errors/format_test.go:129", + }, { Wrapf(New("error"), "error%d", 2), "%s", "error2: error", - }, { - Wrap(io.EOF, "error"), - "%v", - "error: EOF", - }, { - Wrap(io.EOF, "error"), - "%+v", - "EOF\n" + - "error\n" + - "github.com/pkg/errors.TestFormatWrapf\n" + - "\t.+/github.com/pkg/errors/format_test.go:111", }, { Wrapf(New("error"), "error%d", 2), "%v", @@ -123,22 +145,15 @@ func TestFormatWrapf(t *testing.T) { "%+v", "error\n" + "github.com/pkg/errors.TestFormatWrapf\n" + - "\t.+/github.com/pkg/errors/format_test.go:122", - }, { - Wrap(Wrap(io.EOF, "error1"), "error2"), - "%+v", - "EOF\n" + - "error1\n" + - "github.com/pkg/errors.TestFormatWrapf\n" + - "\t.+/github.com/pkg/errors/format_test.go:128\n", + "\t.+/github.com/pkg/errors/format_test.go:144", }} - for _, tt := range tests { - testFormatRegexp(t, tt.error, tt.format, tt.want) + for i, tt := range tests { + testFormatRegexp(t, i, tt.error, tt.format, tt.want) } } -func testFormatRegexp(t *testing.T, arg interface{}, format, want string) { +func testFormatRegexp(t *testing.T, n int, arg interface{}, format, want string) { got := fmt.Sprintf(format, arg) lines := strings.SplitN(got, "\n", -1) for i, w := range strings.SplitN(want, "\n", -1) { @@ -147,7 +162,7 @@ func testFormatRegexp(t *testing.T, arg interface{}, format, want string) { t.Fatal(err) } if !match { - t.Errorf("fmt.Sprintf(%q, err): got: %q, want: %q", format, got, want) + t.Errorf("test %d: line %d: fmt.Sprintf(%q, err): got: %q, want: %q", n+1, i+1, format, got, want) } } } diff --git a/stack_test.go b/stack_test.go index da53daf..2624e45 100644 --- a/stack_test.go +++ b/stack_test.go @@ -120,8 +120,8 @@ func TestFrameFormat(t *testing.T) { "unknown:0", }} - for _, tt := range tests { - testFormatRegexp(t, tt.Frame, tt.format, tt.want) + for i, tt := range tests { + testFormatRegexp(t, i, tt.Frame, tt.format, tt.want) } } @@ -204,7 +204,7 @@ func TestStackTrace(t *testing.T) { "\t.+/github.com/pkg/errors/stack_test.go:198", // this is the stack of Errorf's caller's caller }, }} - for _, tt := range tests { + for i, tt := range tests { x, ok := tt.err.(interface { StackTrace() StackTrace }) @@ -214,7 +214,7 @@ func TestStackTrace(t *testing.T) { } st := x.StackTrace() for j, want := range tt.want { - testFormatRegexp(t, st[j], "%+v", want) + testFormatRegexp(t, i, st[j], "%+v", want) } } } @@ -286,7 +286,7 @@ func TestStackTraceFormat(t *testing.T) { `\[\]errors.Frame{stack_test.go:225, stack_test.go:284}`, }} - for _, tt := range tests { - testFormatRegexp(t, tt.StackTrace, tt.format, tt.want) + for i, tt := range tests { + testFormatRegexp(t, i, tt.StackTrace, tt.format, tt.want) } } From 1b876e063eebebbcbab83aafa8bc631edef98fff Mon Sep 17 00:00:00 2001 From: Dave Cheney Date: Mon, 8 Aug 2016 14:39:38 +1000 Subject: [PATCH 3/3] Remove WithStack and WithMessage public functions The refactoring to use withStack and withMessage types is useful enough to land indepdently of exposing these helpers publically. --- errors.go | 24 ------------------------ stack_test.go | 4 ++-- 2 files changed, 2 insertions(+), 26 deletions(-) diff --git a/errors.go b/errors.go index 915bbdb..d98f310 100644 --- a/errors.go +++ b/errors.go @@ -132,18 +132,6 @@ func (f *fundamental) Format(s fmt.State, verb rune) { } } -// WithStack annotates err with a stack trace at the point WithStack was called. -// If err is nil, WithStack returns nil. -func WithStack(err error) error { - if err == nil { - return nil - } - return &withStack{ - err, - callers(), - } -} - type withStack struct { error *stack @@ -205,18 +193,6 @@ func Wrapf(err error, format string, args ...interface{}) error { } } -// WithMessage annotates err with a new message. -// If err is nil, WithStack returns nil. -func WithMessage(err error, message string) error { - if err == nil { - return nil - } - return &withMessage{ - cause: err, - msg: message, - } -} - type withMessage struct { cause error msg string diff --git a/stack_test.go b/stack_test.go index 2624e45..915d11f 100644 --- a/stack_test.go +++ b/stack_test.go @@ -155,12 +155,12 @@ func TestTrimGOPATH(t *testing.T) { "github.com/pkg/errors/stack_test.go", }} - for _, tt := range tests { + for i, tt := range tests { pc := tt.Frame.pc() fn := runtime.FuncForPC(pc) file, _ := fn.FileLine(pc) got := trimGOPATH(fn.Name(), file) - testFormatRegexp(t, got, "%s", tt.want) + testFormatRegexp(t, i, got, "%s", tt.want) } }