Skip to content
Permalink
Browse files

Redesign the formatting logic

tldr: This commit ensures that `%+v` works more reliably, and updates the
user manual to properly recommend the implementation of a `Format()`
method in all cases for wrappers, that redirects to
`errors.FormatError()`.

Details follows.

Prior to this commit, the library was unable to do "the right thing"
when formatting a wrapper error using `%+v`, if the implementer of the
wrapper did not also implement a `Format()` method (from
`fmt.Formatter`). In that case, the code in `fmt` would simply call
`Error()` which always produces the shortened error message.

The main step to address the issue was thus to change the manual (the
UX spec) to spell out that "all wrappers should implement `Format()`"
and add a `Format()` method to all the wrappers provided by the
library.

However, a naive implementation of `Format()` would cause an excessive
amount of boilerplate, for example:

```go
func (w *withHint) Format(s fmt.State, verb rune) {
  switch verb {
  case 'v':
      if s.Flag('+') {
          fmt.Fprintf(s, "%+v", w.cause)
          fmt.Fprintf(s, "\n-- hint:\n%s", w.hint)
          return
      }
      fallthrough
  case 's', 'q':
      fmt.Fprintf(s, fmt.Sprintf("%%%c", verb), w.cause)
  }
}
func (w *withDetail) Format(s fmt.State, verb rune) {
  switch verb {
  case 'v':
      if s.Flag('+') {
          fmt.Fprintf(s, "%+v", w.cause)
          fmt.Fprintf(s, "\n-- detail:\n%s", w.detail)
          return
      }
      fallthrough
  case 's', 'q':
      fmt.Fprintf(s, fmt.Sprintf("%%%c", verb), w.cause)
  }
}
```

This boilerplate is error-prone and hard to maintain, and thus
intolerable. Fortunately, [the Go 2 error
proposal](https://go.googlesource.com/proposal/+/master/design/29934-error-values.md)
suggests an alternative: make errors implement a `FormatError()`
method containing the bare minimum and make the Go library call that
automatically.

With that proposal, the code above is simplified thus:

```go
func (w *withHint) FormatError(p errors.Printer) error {
	if p.Detail() {
		p.Printf("error with user hint: %s", w.hint)
	}
	return w.cause
}
func (w *withDetail) FormatError(p errors.Printer) error {
	if p.Detail() {
		p.Printf("error with user detail: %s", w.detail)
	}
	return w.cause
}
```

Is this all well and fine? Unfortunately, no.

The Go 2 error proposal only pertains to go versions after 1.12 (which
is still used) and the formatting logic was even rejected for Go 1.13.
This means that the `fmt` code does not yet know how to call the
`FormatError()` method.

So in order for `%s`/`%+v` etc to work one *still* needs to implement
`Format()`. Yet we do not wish to implement all the boilerplate shown
above!

To achieve this, the [`xerrors`
library](https://github.com/golang/xerrors) conveniently/gracefully
provides an `errors.FormatError()` *function* which knows about
calling the `FormatError()` *method*. Wrapper implementers can then
provide always the same identical implementation of `Format()` that
forwards the call to `errors.FormatError()`:

```go
func (w *withHint) Format(s fmt.State, verb rune) { errors.FormatError(w, s, verb) }
func (w *withDetail) Format(s fmt.State, verb rune) { errors.FormatError(w, s, verb) }
```

This way, excessive boilerplate is averted.

Yet, the story does not end here. While implementing this patch, an
attempt was made initially to use the `errors.FormatError()` function
from the `xerrors` library directly. Unfortunately, that
implementation was found to be defective in the following ways:

1. it gets confused if a `FormatError()` method prints nothing in the
   case no detail was requested with `%+v`. This makes it impossible
   to use with wrappers like barriers, marks etc which do not
   emit anything in the common case of formatting with `%s` / `%v` etc.
2. it requires *all* intermediate wrappers to implement a
   `FormatError()` method, otherwise the `%+v` formatting does
   not apply recursively. This makes it impossible to obtain
   good `%+v` behavior when the error chain uses wrappers
   from 3rd party libraries like `github.com/pkg/errors`.

(This feedback was sent upstream as
golang/go#29934 (comment))

So, instead, this library provides its own (re-)implementation of
`errors.FormatError()` which does the right thing:

- it properly elides message prefixes when a wrapper has no output in
  `FormatError()`.
- it understands and properly recurses when a wrapper implements
  `Cause()`/`Unwrap()` but does not implement `Format()` nor
  `FormatError()`.
- it understand and properly recurses when a wrapper implements
  `Format()` redirecting to `errors.FormatError()` but does not
  yet implement a `FormatError()` method itself.

The last point in particular is useful when building software
incrementally: it makes it possible to implement proper formatting for
a newly created wrapper type, before the programmer decides when and
how to implement a `FormatError()` method.
  • Loading branch information
knz committed Jul 5, 2019
1 parent 36356a2 commit 78b6caa8a0455b5fa93f17d4b4c3132597342c0e
@@ -65,6 +65,7 @@ older version of the package.
- implement your own error leaf types and wrapper types:
- implement the `error` and `errors.Wrapper` interfaces as usual.
- register encode/decode functions: call `errors.Register{Leaf,Wrapper}{Encoder,Decoder}()` in a `init()` function in your package.
- implement `Format()` that redirects to `errors.FormatError()`.
- see the section [Building your own error types](#Building-your-own-error-types) below.

## What comes out of an error?
@@ -277,7 +278,14 @@ interface (an `Unwrap()` method). You may also want to implement the
`Cause()` method for backward compatibility with
`github.com/pkg/errors`, if your project also uses that.

Additionally, you may want your new error type to be portable across
If your error type is a wrapper, you should implement a `Format()`
method that redirects to `errors.FormatError()`, otherwise `%+v` will
not work. Additionally, if your type has a payload not otherwise
visible via `Error()`, you may want to implement
`errors.Formatter`. See [making `%+v` work with your
type](#Making-v-work-with-your-type) below for details.

Finally, you may want your new error type to be portable across
the network.

If your error type is a leaf, and already implements `proto.Message`
@@ -410,6 +418,67 @@ library that need a custom encoder:
- Secondary error wrappers in [`secondary/with_secondary.go`](secondary/with_secondary.go).
- Marker error wrappers at the end of [`markers/markers.go`](markers/markers.go).

### Making `%+v` work with your type

In short:

- When in doubt, you should always implement the `fmt.Formatter`
interface (`Format(fmt.State, rune)`) on your custom error types,
exactly as follows:

```go
func (e *yourType) Format(s *fmt.State, verb rune) { errors.FormatError(e, s, verb) }
```

(If you do not provide this redirection for your own custom wrapper
type, this will disable the recursive application of the `%+v` flag
to the causes chained from your wrapper.)

- You may optionally implement the `errors.Formatter` interface:
`FormatError(p errors.Printer) (next error)`. This is optional, but
should be done when some details are not included by `Error()` and
should be emitted upon `%+v`.

The example `withHTTPCode` wrapper [included in the source tree](exthttp/ext_http.go)
achieves this as follows:

```go
// Format() implements fmt.Formatter, is required until Go knows about FormatError.
func (w *withHTTPCode) Format(s fmt.State, verb rune) { errors.FormatError(w, s, verb) }
// FormatError() formats the error.
func (w *withHTTPCode) FormatError(p errors.Printer) (next error) {
// Note: no need to print out the cause here!
// FormatError() knows how to do this automatically.
if p.Detail() {
p.Printf("http code: %d", w.code)
}
return w.cause
}
```

Technical details follow:

- The errors library follows [the Go 2
proposal](https://go.googlesource.com/proposal/+/master/design/29934-error-values.md).

- At some point in the future, Go's standard `fmt` library will learn
[how to recognize error wrappers, and how to use the `errors.Formatter`
interface automatically](https://github.com/golang/go/issues/29934). Until
then, you must ensure that you also implement a `Format()` method
(from `fmt.Formatter`) that redirects to `errors.FormatError`.

Note: you may implement `fmt.Formatter` (`Format()` method) in this
way without implementing `errors.Formatter` (a `FormatError()`
method). In that case, `errors.FormatError` will use a separate code
path that does "the right thing", even for wrappers.

- The library provides an implementation of `errors.FormatError()`,
modeled after the same function in Go 2. This is responsible for
printing out error details, and knows how to present a chain of
causes in a semi-structured format upon formatting with `%+v`.

## Error composition (summary)

| Constructor | Composes |
@@ -75,6 +75,8 @@ type barrierError struct {

var _ error = (*barrierError)(nil)
var _ errbase.SafeDetailer = (*barrierError)(nil)
var _ errbase.Formatter = (*barrierError)(nil)
var _ fmt.Formatter = (*barrierError)(nil)

// barrierError is an error.
func (e *barrierError) Error() string { return e.msg }
@@ -90,21 +92,14 @@ func (e *barrierError) SafeDetails() []string {
}

// Printing a barrier reveals the details.
func (e *barrierError) Format(s fmt.State, verb rune) {
switch verb {
case 'v':
if s.Flag('+') {
fmt.Fprintf(s, "%s", e.msg)
if e.maskedErr != nil {
fmt.Fprintf(s, "\n-- original cause:\n")
errbase.FormatError(s, verb, e.maskedErr)
}
return
}
fallthrough
case 's', 'q':
fmt.Fprintf(s, fmt.Sprintf("%%%c", verb), e.msg)
func (e *barrierError) Format(s fmt.State, verb rune) { errbase.FormatError(e, s, verb) }

func (e *barrierError) FormatError(p errbase.Printer) (next error) {
p.Print(e.msg)
if p.Detail() {
p.Printf("\noriginal cause behind barrier:\n%+v", e.maskedErr)
}
return nil
}

// A barrier error is encoded exactly.
@@ -16,6 +16,7 @@ package barriers_test

import (
"context"
goErr "errors"
"fmt"
"strings"
"testing"
@@ -98,3 +99,89 @@ func TestHandledWithMessagef(t *testing.T) {

tt.CheckEqual(b1.Error(), b2.Error())
}

func TestFormat(t *testing.T) {
tt := testutils.T{t}

const woo = `woo`
const waawoo = `waa: woo`
testCases := []struct {
name string
err error
expFmtSimple string
expFmtVerbose string
}{
{"handled", barriers.Handled(goErr.New("woo")), woo, `
woo:
original cause behind barrier:
woo`},

{"handled + handled", barriers.Handled(barriers.Handled(goErr.New("woo"))), woo, `
woo:
original cause behind barrier:
woo:
original cause behind barrier:
woo`},

{"handledmsg", barriers.HandledWithMessage(goErr.New("woo"), "waa"), "waa", `
waa:
original cause behind barrier:
woo`},

{"handledmsg + handledmsg", barriers.HandledWithMessage(
barriers.HandledWithMessage(
goErr.New("woo"), "waa"), "wuu"), `wuu`, `
wuu:
original cause behind barrier:
waa:
original cause behind barrier:
woo`},

{"handled + wrapper", barriers.Handled(&werrFmt{goErr.New("woo"), "waa"}), waawoo, `
waa: woo:
original cause behind barrier:
waa:
-- verbose wrapper:
waa
- woo`},
}

for _, test := range testCases {
tt.Run(test.name, func(tt testutils.T) {
err := test.err

// %s is simple formatting
tt.CheckEqual(fmt.Sprintf("%s", err), test.expFmtSimple)

// %v is simple formatting too, for compatibility with the past.
tt.CheckEqual(fmt.Sprintf("%v", err), test.expFmtSimple)

// %q is always like %s but quotes the result.
ref := fmt.Sprintf("%q", test.expFmtSimple)
tt.CheckEqual(fmt.Sprintf("%q", err), ref)

// %+v is the verbose mode.
refV := strings.TrimPrefix(test.expFmtVerbose, "\n")
spv := fmt.Sprintf("%+v", err)
tt.CheckEqual(spv, refV)
})
}
}

type werrFmt struct {
cause error
msg string
}

var _ errbase.Formatter = (*werrFmt)(nil)

func (e *werrFmt) Error() string { return fmt.Sprintf("%s: %v", e.msg, e.cause) }
func (e *werrFmt) Unwrap() error { return e.cause }
func (e *werrFmt) Format(s fmt.State, verb rune) { errbase.FormatError(e, s, verb) }
func (e *werrFmt) FormatError(p errbase.Printer) error {
p.Print(e.msg)
if p.Detail() {
p.Printf("-- verbose wrapper:\n%s", e.msg)
}
return e.cause
}
@@ -16,6 +16,9 @@ package contexttags_test

import (
"context"
goErr "errors"
"fmt"
"strings"
"testing"

"github.com/cockroachdb/errors"
@@ -104,8 +107,8 @@ func TestTagRedaction(t *testing.T) {

// This will be our reference expected value.
refStrings := [][]string{
[]string{"planet1=string", "planet2=universe"},
[]string{"foo1=int", "xint", "bar1", "foo2=123", "y456", "bar2"},
[]string{"planet1=<string>", "planet2=universe"},
[]string{"foo1=<int>", "x<int>", "bar1", "foo2=123", "y456", "bar2"},
}

// Construct the error object.
@@ -135,3 +138,81 @@ func TestTagRedaction(t *testing.T) {
tt.Run("remote", func(tt testutils.T) { theTest(tt, newErr) })

}

func TestFormat(t *testing.T) {
tt := testutils.T{t}

ctx := logtags.AddTag(context.Background(), "thetag", nil)
baseErr := goErr.New("woo")
const woo = `woo`
const waawoo = `waa: woo`
testCases := []struct {
name string
err error
expFmtSimple string
expFmtVerbose string
}{
{"tags",
contexttags.WithContextTags(baseErr, ctx),
woo, `
error with context tags: thetag
- woo`},

{"tags + wrapper",
contexttags.WithContextTags(&werrFmt{baseErr, "waa"}, ctx),
waawoo, `
error with context tags: thetag
- waa:
-- verbose wrapper:
waa
- woo`},

{"wrapper + tags",
&werrFmt{contexttags.WithContextTags(baseErr, ctx), "waa"},
waawoo, `
waa:
-- verbose wrapper:
waa
- error with context tags: thetag
- woo`},
}

for _, test := range testCases {
tt.Run(test.name, func(tt testutils.T) {
err := test.err

// %s is simple formatting
tt.CheckEqual(fmt.Sprintf("%s", err), test.expFmtSimple)

// %v is simple formatting too, for compatibility with the past.
tt.CheckEqual(fmt.Sprintf("%v", err), test.expFmtSimple)

// %q is always like %s but quotes the result.
ref := fmt.Sprintf("%q", test.expFmtSimple)
tt.CheckEqual(fmt.Sprintf("%q", err), ref)

// %+v is the verbose mode.
refV := strings.TrimPrefix(test.expFmtVerbose, "\n")
spv := fmt.Sprintf("%+v", err)
tt.CheckEqual(spv, refV)
})
}
}

type werrFmt struct {
cause error
msg string
}

var _ errbase.Formatter = (*werrFmt)(nil)

func (e *werrFmt) Error() string { return fmt.Sprintf("%s: %v", e.msg, e.cause) }
func (e *werrFmt) Unwrap() error { return e.cause }
func (e *werrFmt) Format(s fmt.State, verb rune) { errbase.FormatError(e, s, verb) }
func (e *werrFmt) FormatError(p errbase.Printer) error {
p.Print(e.msg)
if p.Detail() {
p.Printf("-- verbose wrapper:\n%s", e.msg)
}
return e.cause
}
@@ -39,6 +39,8 @@ type withContext struct {

var _ error = (*withContext)(nil)
var _ errbase.SafeDetailer = (*withContext)(nil)
var _ fmt.Formatter = (*withContext)(nil)
var _ errbase.Formatter = (*withContext)(nil)

// withContext is an error. The original error message is preserved.
func (w *withContext) Error() string { return w.cause.Error() }
@@ -48,20 +50,13 @@ func (w *withContext) Cause() error { return w.cause }
func (w *withContext) Unwrap() error { return w.cause }

// Printing a withContext reveals the tags.
func (w *withContext) Format(s fmt.State, verb rune) {
switch verb {
case 'v':
if s.Flag('+') {
fmt.Fprintf(s, "%+v", w.cause)
if w.tags != nil {
fmt.Fprintf(s, "\n-- context: %s", w.tags.String())
}
return
}
fallthrough
case 's', 'q':
errbase.FormatError(s, verb, w.cause)
func (w *withContext) Format(s fmt.State, verb rune) { errbase.FormatError(w, s, verb) }

func (w *withContext) FormatError(p errbase.Printer) error {
if p.Detail() && w.tags != nil {
p.Printf("error with context tags: %s", w.tags.String())
}
return w.cause
}

// SafeDetails implements the errbase.SafeDetailer interface.

0 comments on commit 78b6caa

Please sign in to comment.
You can’t perform that action at this time.