errors/fmt: don't wrap in Errorf by default
Instead, wrap when the format ends in ": %w" instead
of ": %s" or ": %v"
Also, improved test coverage.
Change-Id: I42125f9c201230e619aaf0d61593154df8921993
Reviewed-on: https://go-review.googlesource.com/c/151258
Reviewed-by: Russ Cox <rsc@golang.org>
diff --git a/errors/fmt/errors.go b/errors/fmt/errors.go
index a4de524..4bc00b3 100644
--- a/errors/fmt/errors.go
+++ b/errors/fmt/errors.go
@@ -12,15 +12,20 @@
"golang.org/x/exp/errors/internal"
)
-// Errorf formats according to a format specifier and returns the string
-// as a value that satisfies error.
+// Errorf formats according to a format specifier and returns the string as a
+// value that satisfies error.
//
-// The returned error includes the file and line number of the caller
-// when formatted with additional detail enabled.
+
+// The returned error includes the file and line number of the caller when
+// formatted with additional detail enabled. If the last argument is an error
+// the returned error's Format method will return it if the format string ends
+// with ": %s", ": %v", or ": %w". If the last argument is an error and the
+// format string ends with ": %w", the returned error implements errors.Wrapper
+// with an Unwrap method returning it.
func Errorf(format string, a ...interface{}) error {
- err := lastError(format, a)
+ err, wrap := lastError(format, a)
if err == nil {
- return &simpleErr{Sprintf(format, a...), errors.Caller(1)}
+ return &noWrapError{Sprintf(format, a...), nil, errors.Caller(1)}
}
// TODO: this is not entirely correct. The error value could be
@@ -28,67 +33,70 @@
// substitutions. With relatively small changes to doPrintf we can
// have it optionally ignore extra arguments and pass the argument
// list in its entirety.
- format = format[:len(format)-len(": %s")]
- if !internal.EnableTrace {
- return &withChain{msg: Sprintf(format, a[:len(a)-1]...), err: err}
+ msg := Sprintf(format[:len(format)-len(": %s")], a[:len(a)-1]...)
+ frame := errors.Frame{}
+ if internal.EnableTrace {
+ frame = errors.Caller(1)
}
- return &withChain{
- msg: Sprintf(format, a[:len(a)-1]...),
- err: err,
- frame: errors.Caller(1),
+ if wrap {
+ return &wrapError{msg, err, frame}
}
+ return &noWrapError{msg, err, frame}
}
-func lastError(format string, a []interface{}) error {
- if !strings.HasSuffix(format, ": %s") && !strings.HasSuffix(format, ": %v") {
- return nil
+func lastError(format string, a []interface{}) (err error, wrap bool) {
+ wrap = strings.HasSuffix(format, ": %w")
+ if !wrap &&
+ !strings.HasSuffix(format, ": %s") &&
+ !strings.HasSuffix(format, ": %v") {
+ return nil, false
}
if len(a) == 0 {
- return nil
+ return nil, false
}
err, ok := a[len(a)-1].(error)
if !ok {
- return nil
+ return nil, false
}
- return err
+ return err, wrap
}
-type simpleErr struct {
- msg string
- frame errors.Frame
-}
-
-func (e *simpleErr) Error() string {
- return Sprint(e)
-}
-
-func (e *simpleErr) Format(p errors.Printer) (next error) {
- p.Print(e.msg)
- e.frame.Format(p)
- return nil
-}
-
-type withChain struct {
- // TODO: add frame information
+type noWrapError struct {
msg string
err error
frame errors.Frame
}
-func (e *withChain) Error() string {
+func (e *noWrapError) Error() string {
return Sprint(e)
}
-func (e *withChain) Format(p errors.Printer) (next error) {
+func (e *noWrapError) Format(p errors.Printer) (next error) {
p.Print(e.msg)
e.frame.Format(p)
return e.err
}
-func (e *withChain) Unwrap() error {
+type wrapError struct {
+ msg string
+ err error
+ frame errors.Frame
+}
+
+func (e *wrapError) Error() string {
+ return Sprint(e)
+}
+
+func (e *wrapError) Format(p errors.Printer) (next error) {
+ p.Print(e.msg)
+ e.frame.Format(p)
+ return e.err
+}
+
+func (e *wrapError) Unwrap() error {
return e.err
}
diff --git a/errors/fmt/errors_test.go b/errors/fmt/errors_test.go
index ff7e661..554992b 100644
--- a/errors/fmt/errors_test.go
+++ b/errors/fmt/errors_test.go
@@ -30,6 +30,12 @@
got error
want []string
}{{
+ fmt.Errorf("no args"),
+ chain("no args/path.TestErrorf/path.go:xxx"),
+ }, {
+ fmt.Errorf("no args: %s"),
+ chain("no args: %!s(MISSING)/path.TestErrorf/path.go:xxx"),
+ }, {
fmt.Errorf("nounwrap: %s", "simple"),
chain(`nounwrap: simple/path.TestErrorf/path.go:xxx`),
}, {
@@ -37,15 +43,23 @@
chain(`nounwrap: simple/path.TestErrorf/path.go:xxx`),
}, {
fmt.Errorf("%s failed: %v", "foo", chained),
+ chain("foo failed/path.TestErrorf/path.go:xxx",
+ "chained/somefile.go:xxx"),
+ }, {
+ fmt.Errorf("no wrap: %s", chained),
+ chain("no wrap/path.TestErrorf/path.go:xxx",
+ "chained/somefile.go:xxx"),
+ }, {
+ fmt.Errorf("%s failed: %w", "foo", chained),
chain("wraps:foo failed/path.TestErrorf/path.go:xxx",
"chained/somefile.go:xxx"),
}, {
- fmt.Errorf("wraps: %s", chained),
- chain("wraps:wraps/path.TestErrorf/path.go:xxx",
+ fmt.Errorf("nowrapv: %v", chained),
+ chain("nowrapv/path.TestErrorf/path.go:xxx",
"chained/somefile.go:xxx"),
}, {
- fmt.Errorf("wrapv: %v", chained),
- chain("wraps:wrapv/path.TestErrorf/path.go:xxx",
+ fmt.Errorf("wrapw: %w", chained),
+ chain("wraps:wrapw/path.TestErrorf/path.go:xxx",
"chained/somefile.go:xxx"),
}, {
fmt.Errorf("not wrapped: %+v", chained),
@@ -135,7 +149,7 @@
fmt: "%+v",
want: "something:" +
"\n golang.org/x/exp/errors/fmt_test.TestErrorFormatter" +
- "\n /Users/mpvl/dev/go/text/src/golang.org/x/exp/errors/fmt/errors_test.go:84" +
+ "\n /Users/mpvl/dev/go/text/src/golang.org/x/exp/errors/fmt/errors_test.go:98" +
"\n something more",
}, {
err: fmtTwice("Hello World!"),
@@ -386,7 +400,7 @@
func errToParts(err error) (a []string) {
for err != nil {
var p testPrinter
- if _, ok := err.(errors.Wrapper); ok {
+ if errors.Unwrap(err) != nil {
p.str += "wraps:"
}
f, ok := err.(errors.Formatter)
diff --git a/errors/wrap_test.go b/errors/wrap_test.go
index 4a5c663..c4d6fc6 100644
--- a/errors/wrap_test.go
+++ b/errors/wrap_test.go
@@ -14,10 +14,10 @@
func TestIs(t *testing.T) {
err1 := errors.New("1")
- erra := fmt.Errorf("wrap 2: %v", err1)
- errb := fmt.Errorf("wrap 3: %v", erra)
+ erra := fmt.Errorf("wrap 2: %w", err1)
+ errb := fmt.Errorf("wrap 3: %w", erra)
erro := errors.Opaque(err1)
- errco := fmt.Errorf("opaque: %v", erro)
+ errco := fmt.Errorf("opaque: %w", erro)
err3 := errors.New("3")
@@ -89,7 +89,7 @@
target interface{}
match bool
}{{
- fmt.Errorf("pittied the fool: %v", errorT{}),
+ fmt.Errorf("pittied the fool: %w", errorT{}),
&errT,
true,
}, {
@@ -144,7 +144,7 @@
func TestUnwrap(t *testing.T) {
err1 := errors.New("1")
- erra := fmt.Errorf("wrap 2: %v", err1)
+ erra := fmt.Errorf("wrap 2: %w", err1)
erro := errors.Opaque(err1)
testCases := []struct {
@@ -155,10 +155,10 @@
{wrapped{nil}, nil},
{err1, nil},
{erra, err1},
- {fmt.Errorf("wrap 3: %v", erra), erra},
+ {fmt.Errorf("wrap 3: %w", erra), erra},
{erro, nil},
- {fmt.Errorf("opaque: %v", erro), erro},
+ {fmt.Errorf("opaque: %w", erro), erro},
}
for _, tc := range testCases {
if got := errors.Unwrap(tc.err); got != tc.want {
