Closed neild closed 1 year ago
3. A gotcha with multi-errors is that is that
errors.Is
is ill-defined as whether it meansexactly is
orcontains
.
That is an interesting point that didn't occur to me until now, but it also brings to mind what the semantics of errors.Is
when the target is a multi-error.
isErr := errors.Is(errors.Join(err1, err2), errors.Join(err2, err3)) // ???
I realize this may not come up much in most code since the target is often a sentinel error value. I can imagine it possibly coming up in test code checking if returned errors are consistent across an API perhaps.
I suspect this isn't a big concern, but wanted to mention it in case anyone else has thoughts or concerns about it.
errors.Is
doesn't unwrap the target today. That wouldn't change. This is important to keep the complexity of Is
reasonably bounded.
So
errors.Is(errors.Join(err1, err2), errors.Join(err2, err3))
is false, because the target doesn't match either err1
or err2
.
Updated proposal, incorporating the following changes:
Split
.Join
. Join
uses "\n"
as the separator.Join
contains exactly one non-nil error, Join
returns that error.As
to inorder, not preorder.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 inorder 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 newlines.
// Join returns nil if errs contains no non-nil values.
func Join(errs ...error) error
The fmt.Errorf
function permits multiple instances of the %w
formatting verb.
The errors.Unwrap
function is unaffected: It returns nil
when called on an error with an Unwrap() []error
method.
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.
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.
The errors.Join
function separates the text of the joined errors with newlines. The fmt.Errorf
wraps multiple errors in a user-defined layout. If users have other formatting requirements, they can still create their own error implementations.
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.
The errors.Join
function does not flatten errors. This is simple and comprehensible. Third-party packages can easily provide flattening if desired.
While existing multierror implementations generally provide a mechanism for extracting the individual errors from a combined error, this functionality seems to be used infrequently in practice.
A package which wishes to provide a list of errors for the caller to iterate through should either return an explicit []error
(which the caller may Join
into a single error if needed), or a concrete type that wraps the contained errors. For example:
type ValidationErrors struct {
Errs []error
}
func (e *ValidationErrors) Unwrap() []error { return e.Errs }
func (e *ValidationErrors) Error() string { return errors.Join(e.Errs).Error() }
var ve *ValidationErrors
if errors.As(err, &ve) {
for _, e :-= range ve.Errs { /* ... */ }
}
Returning a concrete type like ValidationErrors
has the advantage of permitting iteration over the individual errors even if the multiply-wrapped error has been itself wrapped.
A goal is for existing multierror implementations to implement the common multiple-unwrap interface.
Most existing multierr implementations include Is
and As
methods on their error type which descend into each wrapped error. If such an implementation simply adds an Unwrap() []error
method with no other changes, errors.Is
and errors.As
will examine the wrapped errors twice: Once when the custom Is
or As
method is called, and a second time when descending into the wrapped errors returned by Unwrap() []error
.
To avoid this double-examination, multierr implementations which add support for Unwrap() []error
should use build constraints to define Is
and As
methods when built with older versions of Go, and an Unwrap() []error
method when built with newer ones.
[Edited: Changed inorder back to preorder]
[Edited: Corrected typo: Unwrap() error
-> Unwrap() []error
]
errors.Is
doesn't unwrap the target today. That wouldn't change. This is important to keep the complexity ofIs
reasonably bounded.So
errors.Is(errors.Join(err1, err2), errors.Join(err2, err3))
is false, because the target doesn't match either
err1
orerr2
.
Fair enough. Exploring this a bit more, what about:
errj := errors.Join(err2, err3)
errors.Is(errors.Join(err1, errj), errj)
errj := errors.Join(err2, err3)
errors.Is(errors.Join(err1, errj), errj)
This case turns on whether errj == errj
. If it does, then Is
reports true.
My unwritten assumption has been that a joined error is equal to itself, and that the underlying type of the error returned by Join
is something like *struct { errs []error }
.
Why the change to an in-order traversal for As
, and what does an in-order traversal mean here? It seems to me (and perhaps I'm totally missing something) that since our error tree is not a binary tree a pre- or post-order traversal would be the more sensible way; either an error node or its children should have precedence over the other as there's no meaningful "left" or "right" child to have precedence. (I can see arguments for either pre-order or post-order; I'd lean towards pre-order as the toplevel error is closest to the caller and thus most likely to be meaningful.)
Does anyone have any objections to the most recent statement of the proposal in https://github.com/golang/go/issues/53435#issuecomment-1191752789?
I do tend to agree with @benjaminjkraft that I don't know what in-order means for, say, a node with 3 children. It seems like it should be either pre-order or post-order. @neild, thoughts?
Callers must not modify the list returned by Unwrap.
I think that should be in the contract, but also probably the standard implementation should make a defensive copy.
I do tend to agree with @benjaminjkraft that I don't know what in-order means for, say, a node with 3 children. It seems like it should be either pre-order or post-order. @neild, thoughts?
Yes, this should be pre-order. I was confused.
Programs parsing raw line-oriented log output are already going to have a bad time on any number of fronts. And there are proper fixes (structured output) and cheap hacks (a passthrough io.Writer that rewrites non-trailing
\n
to\n\t
, or just using a standard logging prefix) to handle that.Ironically, given that many people choose to log multi-errors individually, plain
\n
might make some systems easier.
Yes, they have problems. \n
in output increases the problems, and structured logs and pass-through's don't always help. I've had programs before where parts of the program out of my control log out raw data that just dumps out information with \n
, so parsing then required checking the line starts to try to associate the lines. It'd be best just to avoid it to start and push anything ending in a \n
into a separate log entry or remove the \n
. It's better to not create issues.
Adding this section into the latest version of the proposal:
A goal is for existing multierror implementations to implement the common multiple-unwrap interface.
Most existing multierr implementations include Is
and As
methods on their error type which descend into each wrapped error. If such an implementation simply adds an Unwrap() []error
method with no other changes, errors.Is
and errors.As
will examine the wrapped errors twice: Once when the custom Is
or As
method is called, and a second time when descending into the wrapped errors returned by Unwrap() []error
.
To avoid this double-examination, multierr implementations which add support for Unwrap() error
should use build constraints to define Is
and As
methods when built with older versions of Go, and an Unwrap() error
method when built with newer ones.
Let's wait a little while for any comments to arrive via the pings on uber-go/multierr#64 and hashicorp/go-multierror#63, but things seem pretty positive about this.
@neild:
To avoid this double-examination, multierr implementations which add support for Unwrap() error should use build constraints to define Is and As methods when built with older versions of Go, and an Unwrap() error method when built with newer ones.
Do you mean Unwrap() []error in this sentence?
Do you mean Unwrap() []error in this sentence?
Yes.
Hello, thanks for the proposal, and thanks for the ping on https://github.com/uber-go/multierr/issues/64.
We support the Unwrap() []error
method, and will include an implementation of it for whichever Go version this targets. We also really like the idea of multiple %w
s supported in fmt.Errorf
.
Our collective eyebrows are raised at the newline delimiter for errors.Join
, but we don't see that as a major issue. That's probably what most users want at the presentation layer, and other implementations are still free to use something else.
Thanks!
I haven't red all the thread, only skimmed through the conversation. I would like to see this proposal being integrated into the standard library for various reasons. The most obvious one is that concatenating the errors with the "%w" fmt
directive using errors.New()
would solve the multi errors to string conversion problem, but the main issue I'm facing with the current approach is that when calling a function in a loop which should return an error or nil, in case of using this approach is quite tedious to return a nil error when there is no error. So the ideal solution would be to return a concatenated error string in case of multiple errors or nil
in case there is no error. With errors.New()
as far I know is not really possible.
// If errs contains exactly one non-nil error, Join returns that error.
I've been thinking about this case, and I now think it's a mistake. Join
should consistently return an error with an Unwrap() []error
method.
err
" and "a list of errors containing only err
".I've been thinking about this case, and I now think it's a mistake.
Join
should consistently return an error with anUnwrap() []error
method.
- Returning different types based on the number of wrapped errors makes it more likely that one case goes untested.
- There's a meaningful semantic difference between "
err
" and "a list of errors containing onlyerr
".- The efficiency win from avoiding an allocation in this path is small.
Are we sure that we need the guarantee that the returned error
must implement Unwrap() []error
?
errors.Join
already states that the returned value may be nil, so there's already an expectation to check the result.
Is it too much of a stretch from that to say that the returned error may implement Unwrap() []error
, but you have to check?
That aligns with how other upcasting-based API contracts work—there's never a guarantee.
Users that want to access the individual errors will likely want a function like the following that operates on any error, not just those returned by errors.Join
, so they'll have to account for the possibility that it does not implement the method.
func unwrapErrors(err error) []error {
if merr, ok := err.(interface{ Unwrap() []error }); ok {
return slices.Copy(merr.Unwrap())
}
return []error{err}
}
Are we sure that we need the guarantee that the returned
error
must implementUnwrap() []error
?errors.Join
already states that the returned value may be nil, so there's already an expectation to check the result. Is it too much of a stretch from that to say that the returned error may implementUnwrap() []error
, but you have to check? That aligns with how other upcasting-based API contracts work—there's never a guarantee.
for a standard library type why should we impose the extra burden of checking the interface? The Error
interface should be fully implemented; this should especially be the case for Error types.
for a standard library type why should we impose the extra burden of checking the interface? The
Error
interface should be fully implemented; this should especially be the case for Error types.
I'm sorry, @BenjamenMeyer , I may be misunderstanding this comment. Could you clarify what you mean by the interface being fully implemented?
I also want to state that my last comment is only about the return value of errors.Join
.
My suggestion is that the returned error by errors.Join
may implement the Unwrap() []error
method but does not have to—depending on how many non-nil errors were provided to it.
This is similar to how the error returned by fmt.Errorf
may implement Unwrap() error
, Unwrap() []error
, or neither depending on how many %w
s were present in the template string.
fmt.Errorf("foo: %w", err) // implements Unwrap() error
fmt.Errorf("foo: %w, bar: %w", err1, err2) // implements Unwrap() []error
fmt.Errorf("foo: %v", err) // implements neither
errors.Join(nil) // == nil, implements nothing
errors.Join(err) // == err
errors.Join(err1, err2) // implements Unwrap() []error
I'm suggesting that because the advantage of changing that is not clear to me from the discussion above. @neild suggested three reasons:
- Returning different types based on the number of wrapped errors makes it more likely that one case goes untested.
- There's a meaningful semantic difference between "
err
" and "a list of errors containing onlyerr
".- The efficiency win from avoiding an allocation in this path is small.
(The third one is less a reason to do this, and more a reason why we can do this, so I'll skip that.)
For the first one, I don't think we actually get additional safety from making this change. The change will not enable, say, unchecked type casts of returned errors because of how the errors are intended to be used. Functions will normally return joined errors to their callers, with some code paths returning plain errors. So callers still need to match on the interface if they want to access individual errors directly. E.g. given,
func foo(name string) (err error) {
f, err := os.Open(name, ...)
if err != nil {
return err
}
defer func() {
err = errors.Join(err, f.Close())
}()
// Errors returned from this point onwards are wrapped in errors.Join.
...
}
Callers of foo()
cannot assume that the returned error implements Unwrap() []error
because if os.Open
failed, it's a plain error.
@neild, maybe there's something more to this point you made. Would you mind elaborating on this?
- There's a meaningful semantic difference between "
err
" and "a list of errors containing onlyerr
".
for a standard library type why should we impose the extra burden of checking the interface? The
Error
interface should be fully implemented; this should especially be the case for Error types.I'm sorry, @BenjamenMeyer , I may be misunderstanding this comment. Could you clarify what you mean by the interface being fully implemented?
If the interface of Error includes Unwrap() []error
then anything returning error
should be returning something that complies with the Error interface. We shouldn't be guessing what is provided and what isn't especially for standard library defined types.
Now if someone is implementing a new error type, f.e MyNewErrorType, that should implement the standard library Error interface and anything they want to add so that it is still compliant with error
.
Now if it returns nil
then that's still valid - it's a non-object. Honestly, fmt.Errorf()
should also only return error
values that fully define the Error interface.
IOW - the standard library provides the minimum contracts for types it provides.
Various discussion here over the last two weeks, but I don't see any objections to accepting this proposal. Do I have that right? Does anyone object to the proposal?
What is the final version of the proposal though?
@neild (the author) said
// If errs contains exactly one non-nil error, Join returns that error.
I've been thinking about this case, and I now think it's a mistake.
Join
should consistently return an error with anUnwrap() []error
method. But the proposal still states the "maybe mistake" as the chosen path still.
Separately, @neild would you consider capturing all the weird cases like errj := errors.Join(err2, err3); errors.Is(errors.Join(err1, errj), errj)==?
discussed throughout in the comments as part of the proposal itself? It might make it easier to grok from a single place.
Updated proposal (https://github.com/golang/go/issues/53435#issuecomment-1191752789) to restore the original behavior of Join
, dropping the line "If errs contains exactly one non-nil error, Join returns that error".
I'm not certain what weird cases need further explanation; we could explicitly state that the error returned by Join
is equal to itself, but that seems like a sine qua non to me.
Here is a possible objection: if someone writes a library that returns a type with Unwrap() []error and then a user on an old version of Go imports the library, it will just silently fail errors.Is/As checks instead of failing loudly. Maybe a check should be back ported to the prior version when this comes out.
@carlmjohnson Some thoughts. The program would fail to compile with old Go versions if errors.{Join,Split}
is used. So adding var _ error = errors.Join("")
in the package declaring Unwrap() []error
would guard against this. Seems like kinda bad advice though. We might be able to add a vet check to look for Unwrap() []error
methods declared in a package with a go.mod go directive with a version that is too old.
Updated proposal (#53435 (comment))
Thanks for updating, @neild. No objections from uber-go/multierr on this version of the proposal.
Re @carlmjohnson it's unclear what we can do except caution people about rollout. Tim's var _ = errors.Join
(no need to call it and cause runtime overhead) would be fine for people who want to be extra cautious. I don't see how to do a vet check with sufficient precision, since it won't bother the majority of people, and a go.mod in a dependency is allowed to be newer than the toolchain. (Also the older toolchain won't have the vet check.)
It doesn't sound like there are any other objections, so moving this to likely accept.
The number of multierror libraries is smaller than the number of multierror users, so telling libraries to include _ = errors.Join
if they aren't backwards compatible seems fine.
Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group
The errors.Unwrap function is unaffected: It returns nil when called on an error with an Unwrap() []error method.
Can errors.Unwrap be changed to return the next error in the chain, which when called with errors.Unwrap again returns the next error in the chain (and so on and so forth)? I'm not comfortable with how there are two Unwrap interfaces to implement but errors.Unwrap only respects one.
interface{ Unwrap() error } // <-- errors.Unwrap returns the wrapped error.
interface{ Unwrap() []error } // <-- errors.Unwrap returns nil, potentially confusing.
Instead, errors.Unwrap could flatten an error tree into a chain and return each error in order as errors.Unwrap repeatedly gets called on it. That way it could serve as the iterator API as well.
No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group
Change https://go.dev/cl/432575 mentions this issue: go/analysis/passes/stdmethods: recognize Error() []error
Change https://go.dev/cl/432576 mentions this issue: go/analysis/passes/printf: permit multiple %w format verbs
Change https://go.dev/cl/432898 mentions this issue: errors, fmt: add support for wrapping multiple errors
Change https://go.dev/cl/433057 mentions this issue: cmd: update vendored golang.org/x/tools for multiple error wrapping
I was sort of surprised looking at the errors.Join
documentation that it is not mentioned that the returned error
implements the Unwrap() []error
method. In the absence of the errors.Split
function, this method has significantly reduced discoverability. Making it more discoverable would ease the issue of how to format multiple errors — at least in this case — for different use cases because it would become obvious to users that there was an easy path from the returned value of errors.Join
to a []error
that they could format as they wish.
The addition would not need to be more than "A non-nil error returned by Join implements the Unwrap() []error method.".
Change https://go.dev/cl/450376 mentions this issue: doc/go1.20: add a release note for multiple error wrapping
https://github.com/goware/superr might be interesting to some
Multiple error wrapping will be in 1.20.
it's really sad, I have no understanding why you need it. just will complicate the apps a lot and make people use it in a weird way because we are not smart in general.
I probably missed it simply, but is errors.Join
concurrency-safe?
I probably missed it simply, but is
errors.Join
concurrency-safe?
Yes, see https://go-review.googlesource.com/c/go/+/432898/11/src/errors/join.go
Looks great and can't wait to apply it to engineering!
Referring to bokwoon95 https://github.com/golang/go/issues/53435#issuecomment-1250206592
interface{ Unwrap() []error } // <-- errors.Unwrap returns nil, potentially confusing.
and kortschak https://github.com/golang/go/issues/53435#issuecomment-1287964610
I was sort of surprised looking at the
errors.Join
documentation that it is not mentioned that the returnederror
implements theUnwrap() []error
method. In the absence of theerrors.Split
function, this method has significantly reduced discoverability.
This muliterror feature is incomplete, because I am required to make err.(interface{ Unwrap() []error })
checks myself to call Unwrap() []error
explicitly to get and handle multiple errors. Split(error) []error
func, that return a slice of errors for both regular single error and multierror cases would help here.
@dimandzhi This issue is closed. I created #65428 as a new issue for follow up. You can propose new ideas there.
For the most recent version of this proposal, see: https://github.com/golang/go/issues/53435#issuecomment-1191752789 below.
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. Theerrors.Is
anderrors.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
Reusing the name
Unwrap
avoids ambiguity with the existing singular Unwrap method. Returning a 0-length list fromUnwrap
means the error doesn't wrap anything. Callers must not modify the list returned byUnwrap
. The list returned byUnwrap
must not contain anynil
errors.We replace the term "error chain" with "error tree".
The
errors.Is
anderrors.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.The
fmt.Errorf
function permits multiple instances of the%w
formatting verb.The
errors.Split
function retrieves the original errors from a combined error.The
errors.Unwrap
function is unaffected: It returnsnil
when called on an error with anUnwrap() []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
anderrors.As
. Existing combining errors operate by providingIs
andAs
methods which inspect the contained errors, requiring each implementation to duplicate this logic, possibly in incompatible ways. This is best handled inerrors.Is
anderrors.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: Theerrors.Join
function combines error text with a user-provided separator, andfmt.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
andAs
interact with combined errors?Every major multierror package that I looked at (see "Prior art" below) implements the same behavior for
Is
andAs
:Is
reports true if any error in the combined error matches, andAs
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-wrappingUnwrap() error
method when present, converting a non-nil result into a single-element slice. This would allow traversing an error tree with only calls toSplit
.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 ofJoin
is simpler.Why does the name of the
Split
function not match theUnwrap
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 aJoin
.While we could call the method
Split
, or the functionUnwrapMultiple
, 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)