Skip to content

Commit

Permalink
wip multi-cause error formatting
Browse files Browse the repository at this point in the history 
Basic idea is to keep the existing numbering style during verbose
printing  in order to facilitate matching errors with their types
printed down below. Multi-cause errors trigger indentation of their
details when printed within the "tree" they own.

This approach maintains the existing verbose output for single-cause
chains by avoiding the indentation tree.
  • Loading branch information
@dhartunian
dhartunian committed Aug 1, 2023
1 parent 8514096 commit c010358
Show file tree
Hide file tree
Showing 13 changed files with 1,417 additions and 63 deletions.
87 changes: 62 additions & 25 deletions errbase/format_error.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func formatErrorInternal(err error, s fmt.State, verb rune, redactableOutput boo
// to enable stack trace de-duplication. This requires a
// post-order traversal. Since we have a linked list, the best we
// can do is a recursion.
p.formatRecursive(err, true /* isOutermost */, true /* withDetail */)
p.formatRecursive(err, true, true, false, 0)

// We now have all the data, we can render the result.
p.formatEntries(err)
Expand Down Expand Up @@ -146,7 +146,7 @@ func formatErrorInternal(err error, s fmt.State, verb rune, redactableOutput boo
// by calling FormatError(), in which case we'd get an infinite
// recursion. So we have no choice but to peel the data
// and then assemble the pieces ourselves.
p.formatRecursive(err, true /* isOutermost */, false /* withDetail */)
p.formatRecursive(err, true, false, false, 0)
p.formatSingleLineOutput()
p.finishDisplay(verb)

Expand Down Expand Up @@ -195,7 +195,12 @@ func (s *state) formatEntries(err error) {
// Wraps: (N) <details>
//
for i, j := len(s.entries)-2, 2; i >= 0; i, j = i-1, j+1 {
fmt.Fprintf(&s.finalBuf, "\nWraps: (%d)", j)
s.finalBuf.WriteByte('\n')
for m := 0; m < s.entries[i].depth-1; m += 1 {
s.finalBuf.WriteByte('|')
s.finalBuf.WriteByte(' ')
}
fmt.Fprintf(&s.finalBuf, "Wraps: (%d)", j)
entry := s.entries[i]
s.printEntry(entry)
}
Expand Down Expand Up @@ -330,13 +335,28 @@ func (s *state) formatSingleLineOutput() {
// s.finalBuf is untouched. The conversion of s.entries
// to s.finalBuf is done by formatSingleLineOutput() and/or
// formatEntries().
func (s *state) formatRecursive(err error, isOutermost, withDetail bool) {
func (s *state) formatRecursive(err error, isOutermost, withDetail, withDepth bool, depth int) int {
cause := UnwrapOnce(err)
numChildren := 0
if cause != nil {
// Recurse first.
s.formatRecursive(cause, false /*isOutermost*/, withDetail)
// Recurse first, which populates entries list starting from innermost
// entry. If we've previously seen a multi-cause wrapper, `withDepth`
// will be true, and we'll record the depth below ensuring that extra
// indentation is applied to this inner cause during printing.
// Otherwise, we maintain "straight" vertical formatting by keeping the
// parent callers `withDepth` value of `false` by default.
numChildren += s.formatRecursive(cause, false, withDetail, withDepth, depth+1)
}

causes := UnwrapMulti(err)
for _, c := range causes {
// Override `withDepth` to true for all child entries ensuring they have
// indentation applied during formatting to distinguish them from
// parents.
numChildren += s.formatRecursive(c, false, withDetail, true, depth+1)
}
// inserted := len(s.entries) - 1 - startChildren

// Reinitialize the state for this stage of wrapping.
s.wantDetail = withDetail
s.needSpace = false
Expand All @@ -355,21 +375,19 @@ func (s *state) formatRecursive(err error, isOutermost, withDetail bool) {
bufIsRedactable = true
desiredShortening := v.SafeFormatError((*safePrinter)(s))
if desiredShortening == nil {
// The error wants to elide the short messages from inner
// causes. Do it.
for i := range s.entries {
s.entries[i].elideShort = true
}
// The error wants to elide the short messages from inner causes.
// Read backwards through list of entries up to the number of new
// entries created "under" this one amount and mark `elideShort`
// true.
s.elideShortChildren(numChildren)
}

case Formatter:
desiredShortening := v.FormatError((*printer)(s))
if desiredShortening == nil {
// The error wants to elide the short messages from inner
// causes. Do it.
for i := range s.entries {
s.entries[i].elideShort = true
}
s.elideShortChildren(numChildren)
}

case fmt.Formatter:
Expand All @@ -394,9 +412,7 @@ func (s *state) formatRecursive(err error, isOutermost, withDetail bool) {
if elideChildren {
// The error wants to elide the short messages from inner
// causes. Do it.
for i := range s.entries {
s.entries[i].elideShort = true
}
s.elideShortChildren(numChildren)
}
}

Expand All @@ -419,9 +435,7 @@ func (s *state) formatRecursive(err error, isOutermost, withDetail bool) {
if desiredShortening == nil {
// The error wants to elide the short messages from inner
// causes. Do it.
for i := range s.entries {
s.entries[i].elideShort = true
}
s.elideShortChildren(numChildren)
}
break
}
Expand All @@ -431,18 +445,20 @@ func (s *state) formatRecursive(err error, isOutermost, withDetail bool) {
// fmt.Formatter, but it is a wrapper, still attempt best effort:
// print what we can at this level.
elideChildren := s.formatSimple(err, cause)
// always elideChildren when dealing with multi-cause errors.
if len(causes) > 0 {
elideChildren = true
}
if elideChildren {
// The error wants to elide the short messages from inner
// causes. Do it.
for i := range s.entries {
s.entries[i].elideShort = true
}
s.elideShortChildren(numChildren)
}
}
}

// Collect the result.
entry := s.collectEntry(err, bufIsRedactable)
entry := s.collectEntry(err, bufIsRedactable, withDepth, depth)

// If there's an embedded stack trace, also collect it.
// This will get either a stack from pkg/errors, or ours.
Expand All @@ -456,9 +472,21 @@ func (s *state) formatRecursive(err error, isOutermost, withDetail bool) {
// Remember the entry for later rendering.
s.entries = append(s.entries, entry)
s.buf = bytes.Buffer{}

return numChildren + 1
}

func (s *state) collectEntry(err error, bufIsRedactable bool) formatEntry {
func (s *state) elideShortChildren(newEntries int) {
// TODO(davidh): test case that ensures this works correctly
//for i := range s.entries {
// s.entries[i].elideShort = true
//}
for i := 0; i < newEntries; i++ {
s.entries[len(s.entries)-1-i].elideShort = true
}
}

func (s *state) collectEntry(err error, bufIsRedactable bool, withDepth bool, depth int) formatEntry {
entry := formatEntry{err: err}
if s.wantDetail {
// The buffer has been populated as a result of formatting with
Expand Down Expand Up @@ -495,6 +523,10 @@ func (s *state) collectEntry(err error, bufIsRedactable bool) formatEntry {
}
}

if withDepth {
entry.depth = depth
}

return entry
}

Expand Down Expand Up @@ -708,6 +740,11 @@ type formatEntry struct {
// truncated to avoid duplication of entries. This is used to
// display a truncation indicator during verbose rendering.
elidedStackTrace bool

// depth, if positive, represents a nesting depth of this
// error as a causer of others. This is used with verbose
// printing to illustrate the nesting depth.
depth int
}

// String is used for debugging only.
Expand Down
77 changes: 77 additions & 0 deletions errbase/format_error_internal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,88 @@
package errbase

import (
goErr "errors"
"fmt"
"testing"

"github.com/cockroachdb/redact"
)

type wrapMini struct {
msg string
cause error
}

func (e *wrapMini) Error() string {
return e.msg
}

func (e *wrapMini) Unwrap() error {
return e.cause
}

func TestFormatErrorInternal(t *testing.T) {
tests := []struct {
name string
err error
fmtStr string
expected string
}{
{
name: "single wrapper",
err: fmt.Errorf("%w", fmt.Errorf("a%w", goErr.New("b"))),
fmtStr: "%s",
expected: "ab",
},
{
name: "simple multi-wrapper",
err: goErr.Join(goErr.New("a"), goErr.New("b")),
fmtStr: "%s",
expected: "a\nb",
},
{
name: "simple multi-wrapper with single-cause chains inside",
err: goErr.Join(fmt.Errorf("a%w", goErr.New("b")), fmt.Errorf("c%w", goErr.New("d"))),
fmtStr: "%s",
expected: "ab\ncd",
},
{
name: "simple multi-wrapper with single-cause chains inside (verbose)",
err: goErr.Join(fmt.Errorf("a%w", goErr.New("b")), fmt.Errorf("c%w", goErr.New("d"))),
fmtStr: "%+v",
expected: `ab
(1) ab
| cd
Wraps: (2) cd
| Wraps: (3) d
Wraps: (4) ab
| Wraps: (5) b
Error types: (1) *errors.joinError (2) *fmt.wrapError (3) *errors.errorString (4) *fmt.wrapError (5) *errors.errorString`,
},
{
name: "test wrapMini",
err: &wrapMini{"whoa: d", goErr.New("d")},
fmtStr: "%s",
expected: "whoa: d",
},
{
name: "multi-wrapper where not all children should be elided during printing",
err: fmt.Errorf(""),
fmtStr: "%s",
expected: "whoa: d\nwhoa: d zz",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
fe := Formattable(tt.err)
s := fmt.Sprintf(tt.fmtStr, fe)
if s != tt.expected {
t.Errorf("\nexpected: \n%s\nbut got:\n%s\n", tt.expected, s)
}
})
}
}

func TestPrintEntry(t *testing.T) {
b := func(s string) []byte { return []byte(s) }

Expand Down
96 changes: 93 additions & 3 deletions fmttests/testdata/format/wrap-fmt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1658,7 +1658,37 @@ outerthree
(1) outerthree
| outerfour - innerone
| innertwo sibling error in wrapper
Error types: (1) *fmt.wrapErrors
Wraps: (2) sibling error in wrapper
| github.com/cockroachdb/errors/fmttests.glob...funcNN...
| <tab><path>:<lineno>
| github.com/cockroachdb/errors/fmttests.TestDatadriven.func2.1
| <tab><path>:<lineno>
| github.com/cockroachdb/datadriven.runDirective.func1
| <tab><path>:<lineno>
| github.com/cockroachdb/datadriven.runDirective
| <tab><path>:<lineno>
| github.com/cockroachdb/datadriven.runDirectiveOrSubTest
| <tab><path>:<lineno>
| github.com/cockroachdb/datadriven.runTestInternal
| <tab><path>:<lineno>
| github.com/cockroachdb/datadriven.RunTest
| <tab><path>:<lineno>
| github.com/cockroachdb/errors/fmttests.TestDatadriven.func2
| <tab><path>:<lineno>
| github.com/cockroachdb/datadriven.Walk
| <tab><path>:<lineno>
| github.com/cockroachdb/datadriven.Walk.func1
| <tab><path>:<lineno>
| testing.tRunner
| <tab><path>:<lineno>
| runtime.goexit
| <tab><path>:<lineno>
Wraps: (3) innerone
| innertwo
| -- this is innerone
| innertwo's
| multi-line leaf payload
Error types: (1) *fmt.wrapErrors (2) *errors.fundamental (3) *fmttests.errFmt
=====
===== redactable formats
=====
Expand All @@ -1676,7 +1706,37 @@ Error types: (1) *fmt.wrapErrors
(1) ‹outerthree›
‹ | outerfour - innerone›
‹ | innertwo sibling error in wrapper›
Error types: (1) *fmt.wrapErrors
Wraps: (2) ‹sibling error in wrapper›
‹ | github.com/cockroachdb/errors/fmttests.glob..func23›
‹ | <tab><path>:<lineno>›
‹ | github.com/cockroachdb/errors/fmttests.TestDatadriven.func2.1›
‹ | <tab><path>:<lineno>›
‹ | github.com/cockroachdb/datadriven.runDirective.func1›
‹ | <tab><path>:<lineno>›
‹ | github.com/cockroachdb/datadriven.runDirective›
‹ | <tab><path>:<lineno>›
‹ | github.com/cockroachdb/datadriven.runDirectiveOrSubTest›
‹ | <tab><path>:<lineno>›
‹ | github.com/cockroachdb/datadriven.runTestInternal›
‹ | <tab><path>:<lineno>›
‹ | github.com/cockroachdb/datadriven.RunTest›
‹ | <tab><path>:<lineno>›
‹ | github.com/cockroachdb/errors/fmttests.TestDatadriven.func2›
‹ | <tab><path>:<lineno>›
‹ | github.com/cockroachdb/datadriven.Walk›
‹ | <tab><path>:<lineno>›
‹ | github.com/cockroachdb/datadriven.Walk.func1›
‹ | <tab><path>:<lineno>›
‹ | testing.tRunner›
‹ | <tab><path>:<lineno>›
‹ | runtime.goexit›
‹ | <tab><path>:<lineno>›
Wraps: (3) ‹innerone›
‹ | innertwo›
‹ | -- this is innerone›
‹ | innertwo's›
‹ | multi-line leaf payload›
Error types: (1) *fmt.wrapErrors (2) *errors.fundamental (3) *fmttests.errFmt
=====
===== Sentry reporting
=====
Expand All @@ -1685,7 +1745,37 @@ Error types: (1) *fmt.wrapErrors
(1) ×
×
×
Error types: (1) *fmt.wrapErrors
Wraps: (2) ×
×
×
×
×
×
×
×
×
×
×
×
×
×
×
×
×
×
×
×
×
×
×
×
×
Wraps: (3) ×
×
×
×
×
Error types: (1) *fmt.wrapErrors (2) *errors.fundamental (3) *fmttests.errFmt
-- report composition:
*fmt.wrapErrors
== Extra "error types"
Expand Down
Loading

0 comments on commit c010358

Please sign in to comment.