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

proposal: errors: add support for wrapping multiple errors #53435

Open
neild opened this issue Jun 17, 2022 · 20 comments
Open

proposal: errors: add support for wrapping multiple errors #53435

neild opened this issue Jun 17, 2022 · 20 comments
Labels
Projects
Milestone

Comments

@neild
Copy link
Contributor

@neild neild commented Jun 17, 2022

This is a variation on the rejected proposal #47811 (and perhaps that proposal should just be reopened), and an expansion on a comment in it.

Background

Since Go 1.13, an error may wrap another by providing an Unwrap method returning the wrapped error. The errors.Is and errors.As functions operate on chains of wrapped errors.

A common request is for a way to combine a list of errors into a single error.

Proposal

An error wraps multiple errors if its type has the method

Unwrap() []error

Reusing the name Unwrap avoids ambiguity with the existing singular Unwrap method. Returning a 0-length list from Unwrap means the error doesn't wrap anything. Callers must not modify the list returned by Unwrap. The list returned by Unwrap must not contain any nil errors.

We replace the term "error chain" with "error tree".

The errors.Is and errors.As functions are updated to unwrap multiple errors. Is reports a match if any error in the tree matches. As finds the first matching error in a preorder traversal of the tree.

The errors.Join function provides a simple implementation of a multierr. It does not flatten errors.

// Join returns an error that wraps the given errors.
// Any nil error values are discarded.
// The error formats as the text of the given errors, separated by sep.
// Join returns nil if errs contains no non-nil values.
func Join(sep string, errs ...error) error

The fmt.Errorf function permits multiple instances of the %w formatting verb.

The errors.Split function retrieves the original errors from a combined error.

// Split returns the result of calling the Unwrap method on err,
// if err's type contains an Unwrap method returning []error.
// Otherwise, Split returns nil.
func Split(err error) []error

The errors.Unwrap function is unaffected: It returns nil when called on an error with an Unwrap() []error method.

Questions

Prior proposals have been declined on the grounds that this functionality can be implemented outside the standard library, and there was no good single answer to several important questions.

Why should this be in the standard library?

This proposal adds something which cannot be provided outside the standard library: Direct support for error trees in errors.Is and errors.As. Existing combining errors operate by providing Is and As methods which inspect the contained errors, requiring each implementation to duplicate this logic, possibly in incompatible ways. This is best handled in errors.Is and errors.As, for the same reason those functions handle singular unwrapping.

In addition, this proposal provides a common method for the ecosystem to use to represent combined errors, permitting interoperation between third-party implementations.

How are multiple errors formatted?

A principle of the errors package is that error formatting is up to the user. This proposal upholds that principle: The errors.Join function combines error text with a user-provided separator, and fmt.Errorf wraps multiple errors in a user-defined layout. If users have other formatting requirements, they can still create their own error implementations.

How do Is and As interact with combined errors?

Every major multierror package that I looked at (see "Prior art" below) implements the same behavior for Is and As: Is reports true if any error in the combined error matches, and As returns the first matching error. This proposal follows common practice.

Does creating a combined error flatten combined errors in the input?

The errors.Join function does not flatten errors. This is simple and comprehensible. Third-party packages can easily provide flattening if desired.

Should Split unwrap errors that wrap a single error?

The errors.Split function could call the single-wrapping Unwrap() error method when present, converting a non-nil result into a single-element slice. This would allow traversing an error tree with only calls to Split.

This might allow for a small improvement in the convenience of code which manually traverses an error tree, but it is rare for programs to manually traverse error chains today. Keeping Split as the inverse of Join is simpler.

Why does the name of the Split function not match the Unwrap method it calls?

Giving the single- and multiple-error wrapping methods the same name neatly avoids any questions of how to handle errors that implement both.

Split is a natural name for the function that undoes a Join.

While we could call the method Split, or the function UnwrapMultiple, or some variation on these options, the benefits of the above points outweigh the value in aligning the method name with the function name.

Prior art

There have been several previous proposals to add some form of combining error, including:

https://go.dev/issue/47811: add Errors as a standard way to represent multiple errors as a single error
https://go.dev/issue/48831: add NewJoinedErrors
https://go.dev/issue/20984: composite errors
https://go.dev/issue/52607: add With(err, other error) error
fmt.Errorf("%w: %w", err1, err2) is largely equivalent to With(err1, err2).

Credit to @jimmyfrasche for suggesting the method name Unwrap.

There are many implementations of combining errors in the world, including:

https://pkg.go.dev/github.com/hashicorp/go-multierror (8720 imports)
https://pkg.go.dev/go.uber.org/multierr (1513 imports)
https://pkg.go.dev/tailscale.com/util/multierr (2 imports)

@neild neild added the Proposal label Jun 17, 2022
@gopherbot gopherbot added this to the Proposal milestone Jun 17, 2022
@DeedleFake
Copy link

@DeedleFake DeedleFake commented Jun 17, 2022

How does walking up the tree work in errors.Is() and errors.As()? Is it depth-first, i.e. Unwrap() to []error and then fully walk each resulting tree, or is it breadth-first, i.e. Unwrap() to []error and then do each of those before unwrapping them and doing the next set. I assume that it's depth-first, as that's simpler, but breadth-first could also make sense. It would also be possible to convert from a default of breadth-first to a depth-first by implementing custom unwrapping logic, but going the other way could be difficult.

@neild
Copy link
Contributor Author

@neild neild commented Jun 17, 2022

How does walking up the tree work in errors.Is() and errors.As()?

Depth-first. Unwrap to an []error and walk each error in the list in turn. This is what every existing multierr implementation I looked at does.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Jun 18, 2022

This would also subsume #50819 and I imagine any other similar proposals.

Maybe it should wait for a generic iteration standard, but should there be some errors.Each function that goes through each link in the chain? My suspicion is there’s not much to do besides As-ing and Is-ing, but it does seem like an absence, since there will be some internal function that works like that, which could potentially be exported.

@neild
Copy link
Contributor Author

@neild neild commented Jun 18, 2022

Is and As are separate implementations for efficiency reasons, so there's no internal Each function. This proposal does make the case for an exported Each or similar stronger, since iteration becomes more complicated.

We should probably wait and see what happens with generic iteration, however.

@balasanjay
Copy link
Contributor

@balasanjay balasanjay commented Jun 18, 2022

Does Unwrap return a reference to the internal slice in the error?
If so, that would make the internal slice reference externally mutable, which seems unfortunate (but not a dealbreaker).

If not, and it returns a copy, then it would allocate, which also seems unfortunate (but probably not a dealbreaker).

@neild
Copy link
Contributor Author

@neild neild commented Jun 18, 2022

Does Unwrap return a reference to the internal slice in the error?

Unspecified, but the contract for it says that the caller must not modify the slice, so it may.

Perhaps Split should copy the result into a new slice. This would leave Is and As potentially allocation-free, while adding a layer of safety (and an allocation) to the relatively unusual case of manually inspecting the individual errors.

@josharian
Copy link
Contributor

@josharian josharian commented Jun 20, 2022

This strikes me as a significant step forward in figuring out the multi-error question. Thank you!

The only part of this that doesn't seem entirely spot-on is errors.Split.

What is the use case for errors.Split?

Figuring errors.Split would be used like errors.Unwrap, I grepped through the code sitting around on my laptop and found only a single use of errors.Unwrap. It was used to check recursively whether any error in the chain satisfied a predicate. It would be interesting to analyze other uses in a larger corpus.

Walking an arbitrary error tree using errors.Split and errors.Unwrap will be annoying. I wonder whether a callback-based errors.Walk that walks the entire error tree would be a better API. There are a lot of details to specify for such an API, but with a bit of care, it could be more powerful and flexible than errors.Split. It could be used to implement errors.Is and errors.As. And it sidesteps questions about ownership and modification of returned error slices.

(The details about errors.Walk include things like: Does it call back for all errors, or only leaf errors? Does it use a single callback like io/fs.WalkDir or separate pre- and post- callbacks like golang.org/x/tools/ast/astutil.Apply? Does it support early stopping?)

On the topic of slice copies, a minor question. Does errors.Join make a copy of the variadic arg? I'd argue for yes, and ameliorate the pain a bit by adding a small default backing store to the struct. Aliasing bugs are miserable.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Jun 21, 2022

I find that the main reason I unwrap my own multierrors is so that I can report them separately to my logging service or display them as a list in a CLI. The original xerrors formatting proposal was dropped for being too complicated but I think that an easy to use error walking mechanism might make it easy to let users figure out how they want to display and format all the suberrors on their own.

@rsc rsc changed the title proposal: wrapping multiple errors proposal: errors: add support for wrapping multiple errors Jun 22, 2022
@rsc rsc moved this from Incoming to Active in Proposals Jun 22, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Jun 22, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@hopehook
Copy link
Contributor

@hopehook hopehook commented Jun 23, 2022

If I hadn't read the comments, I might have thought that after the "Join" error, I could use "Split" to parse it out.

"errors.Split" does the same thing as "errors.Unwrap", but deals with multiple errors. So instead of "Split" which is the opposite of "Join", it should be "UnwrapMultiple" or "Walk" which is more appropriate.

@hopehook
Copy link
Contributor

@hopehook hopehook commented Jun 23, 2022

The errors.Unwrap function is unaffected: It returns nil when called on an error with an Unwrap() []error

Since there is no reaction to multiple error conditions, the naming should also be differentiated, such as ”UnwrapMultiple() []error “.

@neild
Copy link
Contributor Author

@neild neild commented Jun 23, 2022

What is the use case for errors.Split?

We need to provide some reasonably convenient way to get at the underlying errors of a multierr. Perhaps Split isn't the right API here (although I still feel pretty good about it), but we need something.

I wonder whether a callback-based errors.Walk that walks the entire error tree would be a better API.

Perhaps there's a case for such an API, but I think it's distinct from this proposal. Accessing the component errors of a multierr is different from a full tree walk.

Does errors.Join make a copy of the variadic arg?

Yes.

@jimmyfrasche
Copy link
Member

@jimmyfrasche jimmyfrasche commented Jun 23, 2022

I'm 100% on

  • defining Unwrap() []error
  • changing Is/As to support the new Unwrap
  • updating fmt.Errorf to allow multiple %w

If those were the only things accepted I'd be happy and it would be more than enough to allow external multierrors to interoperate with each other and std.

I do think there 100% does need to be a tree walking API—but it's too early to decide what that looks like. Once the protocol above is enshrined in errors, it's simple enough to experiment with that outside std.

I'm not 100% sold on the name for Split but there should probably be such a func in std, regardless of name, to pair with Unwrap. I don't think it would be much of an impediment if this weren't included, however.

I'm not really sold on Join. I see the utility of including a basic multierror in std, but just gluing the errors together with a sep seems like it could get messy when multierrors get composed. I think, like a tree walking API, it should be left out and experimentation allowed to continue outside std for now.

@josharian
Copy link
Contributor

@josharian josharian commented Jun 23, 2022

We need to provide some reasonably convenient way to get at the underlying errors of a multierr.

I'm not so sure that we do.

For uses (testing?) in which you really specifically want to unwrap from an error that has an Unwrap() [] method, you can use a type assertion. (errors.Unwrap is less than 10 LOC, and consists entirely of a type assertion.)

But I suspect that almost nobody wants specifically to unwrap a multi-error exactly one layer deep. Rather they want to ask the general question "what errors are in this tree?". So I think that's the right form of API to add. And I think it'll get created internally anyway to implement Is and As.

Maybe this proposal should simply omit Split?

@neild
Copy link
Contributor Author

@neild neild commented Jun 23, 2022

Rather they want to ask the general question "what errors are in this tree?".

The fact that errors.Unwrap is so seldom used indicates to me that people don't want to ask this question. They use Is and As to ask "is this error in the tree?" or "is there an error of this type in the tree?" instead.

I'm not certain how much of a use case there is for a tree walk that doesn't preserve the structure of the tree. It would be nice to see real-world examples of this in practice.

@josharian
Copy link
Contributor

@josharian josharian commented Jun 23, 2022

I grepping in my own dependencies and grabbed all the uses. I ignored the standard library, static analyzers, linters, and then pulled all the uses that were not in tests. Here they are:

x/tools uses errors.Unwrap to get to the innermost error, without regard for the structure of what is above it. I suspect that this could would be better served by being able to evaluate a predicate on every error in the tree.

aws-sdk-go uses errors.Unwrap to recursively evaluate a predicate on every error in the tree, with early stopping.

containerd uses errors.Unwrap in a way that I find confusing and might be buggy. It uses it to peel back exactly one layer of wrapping and then compares this to a particular error using string matching. This is fragile (if another wrapping layer gets introduced) and would probably be better served by a way to check every error in the tree.

containerd uses errors.Unwrap again to peel back exactly one layer of wrapping. It looks like errors.As would be a better fit here; I don't see any reason why exactly one layer is the right answer here.

jwt uses errors.Unwrap to implement Is. In this case, errors.Unwrap is unnecessary, because e is known to have a concrete type that implements Unwrap; the code could call e.Unwrap directly instead.

I then grabbed two uses in tests at random:

errwrap uses errors.Unwrap specifically to test that it is compatible with how the standard library defines errors.Unwrap. It has no actual use for errors.Unwrap or opinion about its behavior.

pgconn uses errors.Unwrap to test its own error's compability with package errors. It looks like it would be equally well served by errors.As.

So far, I see no evidence that anyone cares much about the structure of the error tree. It looks to me like they are all: misusing the API (evidence that the API is wrong); using the wrong API (evidence that the API is wrong); using the API pointlessly; or using the API to evaluate a predicate "anywhere along the error chain".

@neild
Copy link
Contributor Author

@neild neild commented Jun 23, 2022

That's useful data; thanks.

We don't have error trees right now; we have an error chain. We can't tell whether people will care about the structure of trees by looking at how they work with linear chains.

It'd be interesting to look at use of existing multierr packages to see how often multierrs are converted back to lists of errors.

@josharian
Copy link
Contributor

@josharian josharian commented Jun 24, 2022

It'd be interesting to look at use of existing multierr packages to see how often multierrs are converted back to lists of errors.

I agree. Can I leave that for you to do? I've exceeded my time allotment for this for a while. :)

@hopehook
Copy link
Contributor

@hopehook hopehook commented Jun 24, 2022

I'm 100% on

  • defining Unwrap() []error
  • changing Is/As to support the new Unwrap
  • updating fmt.Errorf to allow multiple %w

If those were the only things accepted I'd be happy and it would be more than enough to allow external multierrors to interoperate with each other and std.

I do think there 100% does need to be a tree walking API—but it's too early to decide what that looks like. Once the protocol above is enshrined in errors, it's simple enough to experiment with that outside std.

I agree with you very much, we should focus on the original core API supporting the multiple error model first, and then expose the complexity to the user. Before that, we can experiment with Join, Split, Walk, etc.

Most importantly, decide the definition of Unwrap() []error.

@neild
Copy link
Contributor Author

@neild neild commented Jun 24, 2022

I agree. Can I leave that for you to do? I've exceeded my time allotment for this for a while. :)

I'm going to go look, but probably not until next week. Had a few days of meetings, and I'm now exhausted and behind on everything. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

9 participants