Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add formatting for multi-cause errors #115

Merged

Conversation

dhartunian
Copy link
Contributor

@dhartunian dhartunian commented Aug 1, 2023

Previously, error formatting logic was based on a single linear chain
of causality. Error causes would be printed vertically down the page,
and their interpretation was natural.

This commit adds multi-cause formatting support with two goals in
mind:

  1. Preserve output exactly as before for single-cause error chains
  2. Format multi-errors in a way that preserves the existing vertical
    layout style.

For non-verbose error display (.Error(), %s, %v) there are no
changes implemented here. We rely on the error's own display logic and
typically a multi-cause error will have a message within that has been
constructed from all of its causes during instantiation.

For verbose error display (%+v) which relies on object
introspection, whenever we encounter a multi-cause error in the chain,
we mark that subtree as being displayed with markers for the "depth"
of various causes. All child errors of the parent, will be at depth
"1", their child errors will be at depth "2", etc. During display, we
indent the error by its "depth" and add a └─ symbol to illustrate
the parent/child relationship.

Example:

Printing the error produced by this construction using the format directive %+v

fmt.Errorf(
	"prefix1: %w",
	fmt.Errorf(
		"prefix2 %w",
		goErr.Join(
			fmt.Errorf("a%w", fmt.Errorf("b%w", fmt.Errorf("c%w", goErr.New("d")))),
			fmt.Errorf("e%w", fmt.Errorf("f%w", fmt.Errorf("g%w", goErr.New("h")))),
)))

Produces the following output:

prefix1: prefix2: abcd
(1) prefix1
Wraps: (2) prefix2
Wraps: (3) abcd
  | efgh
  └─ Wraps: (4) efgh
    └─ Wraps: (5) fgh
      └─ Wraps: (6) gh
        └─ Wraps: (7) h
  └─ Wraps: (8) abcd
    └─ Wraps: (9) bcd
      └─ Wraps: (10) cd
        └─ Wraps: (11) d
Error types: (1) *fmt.wrapError (2) *fmt.wrapError (3)
*errors.joinError (4) *fmt.wrapError (5) *fmt.wrapError (6)
*fmt.wrapError (7) *errors.errorString (8) *fmt.wrapError (9)
*fmt.wrapError (10) *fmt.wrapError (11) *errors.errorString`,

Note the following properties of the output:

  • The top-level single cause chain maintains the left-aligned Wrap
    lines which contain each cause one at a time.
  • As soon as we hit the multi-cause errors within the joinError
    struct ((3)), we add new indentation to show that The child errors
    of (3) are (4) and (8)
  • Subsequent causes of errors after joinError, are also indented to
    disambiguate the causal chain
  • The Error types section at the bottom remains the same and the
    numbering of the types can be matched to the errors above using the
    integers next to the Wrap lines.
  • No special effort has been made to number the errors in a way that
    describes the causal chain or tree. This keeps it easy to match up the
    error to its type descriptor.

Note for reviewers: only take a look at final commit


This change is Reviewable

@dhartunian dhartunian requested a review from knz August 1, 2023 13:59
@dhartunian dhartunian force-pushed the multierror-formatting branch from 3da6823 to c010358 Compare August 1, 2023 19:02
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: 0 of 37 files reviewed, 2 unresolved discussions (waiting on @dhartunian)


-- commits line 61 at r4:
This reviewer would welcome an example of the envisioned presentation in the commit message.

I would like to understand where you'd like to do with it before I start the review. The examples used in the tests are too complicated to have that high-level overview. (The examples in the tests are very good as tests, just not good at explaining what's going on)


errbase/format_error.go line 105 at r4 (raw file):

		// post-order traversal. Since we have a linked list, the best we
		// can do is a recursion.
		p.formatRecursive(err, true, true, false, 0)

nit: pls keep comments that name the arguments. ditto below.

@dhartunian dhartunian force-pushed the multierror-formatting branch from c010358 to c03a0c7 Compare August 16, 2023 13:40
@dhartunian dhartunian changed the title [WIP] multi-cause error formatting add formatting for multi-cause errors Aug 16, 2023
@dhartunian dhartunian requested a review from knz August 16, 2023 13:41
@dhartunian
Copy link
Contributor Author

@knz updated last PR and added a detailed example of changes. Tried to keep this pass simple and note in test comments the specific tweaks I want to fix later.

@dhartunian dhartunian force-pushed the multierror-formatting branch 2 times, most recently from 6cb12a7 to af098e8 Compare August 16, 2023 23:26
@dhartunian dhartunian changed the base branch from master to go-1.20-upgrade August 16, 2023 23:26
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to talk about multierrors that have a custom SafeFormatError. And include tests for them. See below.

Reviewed 1 of 17 files at r8.
Reviewable status: 1 of 37 files reviewed, 2 unresolved discussions (waiting on @dhartunian)


errbase/format_error.go line 512 at r8 (raw file):

// impossible for a multi-cause tree to later "switch" to a
// single-cause chain, so any time a multi-cause error is recursing
// you're guaranteed to only have its children within.

Consider this error structure:

A
|- B
|  |- B1
|  +- B2
|
+- C
   |- C1
   +- C2

The recursion will be in this order:

C2
C1
C
B2
B1
B
A

Now, what if "B" is an error which overrides its cause texts? We need to elide the entries B2,B1 but not C,C1,C2

I think where you are coming from is this: if A is a multierror constructed via e.g. Join or Errorf("%w %w"), then based on experience it "overrides its error message" and so everything before A will be elided.

OK, but is that the only possible implementation of multierrors?

What if a custom multierror type implements SafeFormatError, and doesn't return nil to mean that the printing should be interrupted?

(This needs unit tests)

Previously, error formatting logic was based on a single linear chain
of causality. Error causes would be printed vertically down the page,
and their interpretation was natural.

This commit adds multi-cause formatting support with two goals in
mind:
1. Preserve output exactly as before for single-cause error chains
2. Format multi-errors in a way that preserves the existing vertical
layout style.

For non-verbose error display (`.Error()`, `%s`, `%v`) there are no
changes implemented here. We rely on the error's own display logic and
typically a multi-cause error will have a message within that has been
constructed from all of its causes during instantiation.

For verbose error display (`%+v`) which relies on object
introspection, whenever we encounter a multi-cause error in the chain,
we mark that subtree as being displayed with markers for the "depth"
of various causes. All child errors of the parent, will be at depth
"1", their child errors will be at depth "2", etc. During display, we
indent the error by its "depth" and add a `└─` symbol to illustrate
the parent/child relationship.

Example:

Printing the error produced by this construction using the format directive `%+v`
```
fmt.Errorf(
	"prefix1: %w",
	fmt.Errorf(
		"prefix2 %w",
		goErr.Join(
			fmt.Errorf("a%w", fmt.Errorf("b%w", fmt.Errorf("c%w", goErr.New("d")))),
			fmt.Errorf("e%w", fmt.Errorf("f%w", fmt.Errorf("g%w", goErr.New("h")))),
)))
```

Produces the following output:

```
prefix1: prefix2: abcd
(1) prefix1
Wraps: (2) prefix2
Wraps: (3) abcd
  | efgh
  └─ Wraps: (4) efgh
    └─ Wraps: (5) fgh
      └─ Wraps: (6) gh
        └─ Wraps: (7) h
  └─ Wraps: (8) abcd
    └─ Wraps: (9) bcd
      └─ Wraps: (10) cd
        └─ Wraps: (11) d
Error types: (1) *fmt.wrapError (2) *fmt.wrapError (3)
*errors.joinError (4) *fmt.wrapError (5) *fmt.wrapError (6)
*fmt.wrapError (7) *errors.errorString (8) *fmt.wrapError (9)
*fmt.wrapError (10) *fmt.wrapError (11) *errors.errorString`,
```

Note the following properties of the output:
* The top-level single cause chain maintains the left-aligned `Wrap`
lines which contain each cause one at a time.
* As soon as we hit the multi-cause errors within the `joinError`
struct (`(3)`), we add new indentation to show that The child errors
of `(3)` are `(4)` and `(8)`
* Subsequent causes of errors after `joinError`, are also indented to
disambiguate the causal chain
* The `Error types` section at the bottom remains the same and the
numbering of the types can be matched to the errors above using the
integers next to the `Wrap` lines.
* No special effort has been made to number the errors in a way that
describes the causal chain or tree. This keeps it easy to match up the
error to its type descriptor.
Previous support for multi-cause encode/decode functionality, did not
include support for custom encoder and decoder logic.

This commits adds the ability to register encoders and decoders for
multi-cause errors to encode custom types unknown to this library.
@dhartunian dhartunian force-pushed the multierror-formatting branch from af098e8 to 6adb34f Compare August 22, 2023 12:15
Copy link
Contributor Author

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@knz added datadriven test with custom multi-cause error with a custom formatter and also extended public API to allow for custom encoder/decoder definitions for multi-cause errors. This is also demonstrated in the datadriven test.

The public API change ends up a bit clumsy because I have to add methods to the API instead of re-using the existing wrapper encoder/decoder API. Let me know if you think a different approach is needed here.

Reviewable status: 0 of 37 files reviewed, 2 unresolved discussions (waiting on @knz)


-- commits line 61 at r4:

Previously, knz (Raphael 'kena' Poss) wrote…

This reviewer would welcome an example of the envisioned presentation in the commit message.

I would like to understand where you'd like to do with it before I start the review. The examples used in the tests are too complicated to have that high-level overview. (The examples in the tests are very good as tests, just not good at explaining what's going on)

Done.


errbase/format_error.go line 105 at r4 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

nit: pls keep comments that name the arguments. ditto below.

Done.


errbase/format_error.go line 512 at r8 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

Consider this error structure:

A
|- B
|  |- B1
|  +- B2
|
+- C
   |- C1
   +- C2

The recursion will be in this order:

C2
C1
C
B2
B1
B
A

Now, what if "B" is an error which overrides its cause texts? We need to elide the entries B2,B1 but not C,C1,C2

I think where you are coming from is this: if A is a multierror constructed via e.g. Join or Errorf("%w %w"), then based on experience it "overrides its error message" and so everything before A will be elided.

OK, but is that the only possible implementation of multierrors?

What if a custom multierror type implements SafeFormatError, and doesn't return nil to mean that the printing should be interrupted?

(This needs unit tests)

Added this example to tests.

@dhartunian dhartunian requested a review from knz August 22, 2023 12:17
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty good. I like it.

Reviewed 11 of 21 files at r7, 2 of 2 files at r9, 12 of 16 files at r10, all commit messages.
Reviewable status: 14 of 37 files reviewed, all discussions resolved

@dhartunian
Copy link
Contributor Author

TFTR!

@dhartunian dhartunian merged commit f0a2a69 into cockroachdb:go-1.20-upgrade Aug 22, 2023
@dhartunian dhartunian deleted the multierror-formatting branch August 22, 2023 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants