-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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 Errors function to undo Join #57358
Comments
CC @neild |
The original proposal #53435 included Split, which was explicitly dropped. This proposal doesn't appear to bring with it much new information? |
@seankhliao I suppose in terms of code, etc, this does not add much. If anything please take this as "I actually needed it in the field, and |
Can you give a concrete example of where you needed this function?
|
@neild Thanks, here's some example. Hope it makes some sense. The case I was handling was collecting data from multiple goroutines graph LR
A(User) --> B[Driver]
B --> C1[Worker1]
B --> C2[Worker2]
B --> C3[Worker3]
C3 --> | report error | B
C2 --> | report error | B
C1 --> | report error | B
B--> |report error| A
In the old version we just did not bother to write User code that analyzes the different failure modes, because only one would be able to be sent back to the user anyways. But now that there was It was very easy to create an error that contains all of the worker errors using it. Now it was time to write the example code to show how Users could react to the different errors. This is where we stumbled upon our initial problem. How does the User know which errors occurred? All errors were created using Once we read a bit of the Go source code (because there wasn't any real examples in the user-visible docs for this sort of thing) and learned that the stdlib actually differentiates between a single error/unwrappable with single error/unwrappable with multiple errors, the first instinct was to look for a named interface like these, just like in // These were the things we looked for, I believe they don't exist in Go ATM
package errors
type Unwrapper interface {
Unwrap() error
}
type MultiUnwrapper interface {
Unwrap() []error
} So that we could do: if err := driverCode(); err != nil {
switch err := err.(type) {
case errors.MultiUnwrapper:
for _, childErr := range err.Unwrap() {
if childErr.String() == .... { // because errors haven't been made distinguishable yet
...
}
}
default:
// some other error
}
} But obviously this didn't exist, so now we were back to if err := driverCode(); err != nil {
switch err := err.(type) {
case interface{ Unwrap() []error }:
if childErr.String() == .... { // because errors haven't been made distinguishable yet... but at least it works as an escape hatch
...
}
default:
// some other error
}
} The main issue we had at this point was not so much if it was possible to do it or not, but as providers of the Driver/Worker module, we thought that it would discourage users from using this module code once they see the cumbersome boilerplate code just to handle errors. So then the next step in our search was to find a possibly more idiomatic way to achieve the same thing without resorting to type conversions. We found that func interesting() error {
return fmt.Join(
fmt.Errorf(`interesting error 1`),
fmt.Errorf(`interesting error 2`),
}
}
// Yes, I know this does not work :)
if err := interesting(); err != nil {
var interestingErr error
if errors.As(err, &interestingErr) {
// handle specific case
}
} Which means that we would have to rewrite the majority of our error cases with something equivalent to type InterestingError struct {
...
}
func (err InterestingError) Error() string {
...
}
if err := interesting(); err != nil {
j var interestingErr mypkg.InterestingError
if errors.As(err, &interestingErr) {
// handle specific case
}
} And, even with all of this, we still end up forcing the end user to write boilerplate like this. if err := driverCode(); err != nil {
// there could possibly more error types to handle than one
var interestingErr1 mypkg.InterestingErr1
var interestingErr2 mypkg.InterestingErr2
if errors.As(err, &interestingErr1) {
// handle specific error
} else if errors.As(err, &interestingErr2) {
// handle specific error
} else {
// some other error
}
} We understand that boilerplate code is required nonetheless, but we didn't find that the presence of distinguishable types made the boilerplate any simpler. Once we found this out, it just seemed more trouble than it's worth to convert our code to use if err := driverCode(); err != nil {
for _, child := range errors.Errors(err) {
switch child := child.(type) {
case *mypkg.InterestingErr1:
// handle specific error
case *mypkg.InterestingErr2:
// handle specific error
default:
// some other error
}
}
} While this is still boilerplate, one major advantage that we find is that we would be able to point users to the And finally, I understand that the last boilerplate breaks if the |
ISTM, this is blocked until #56413 is resolved, then it should just return an iterator. Suggested name |
Thanks for the detailed explanation @lestrrat. You suggest that you'd like users to write code like this:
This approach only works when For example, if we write a function that wraps the result of
then a single-level unwrapping of the error returned by To avoid the need to reason about the structure of the error tree, I would recommend using If there is a need to provide a specific list of errors to the user, I'd recommend defining concrete error type that contains that error. For example:
Users can extract the |
@neild I think the main point of my argument got lost because of my bad example. My main point was that In real like I can the user to enumerate, and do whatever they want to do with the contained errors, without having to explicitly name their error types for i, err := range errors.Errors(err) {
// do what you want, without knowing the exact type that err can take
// maybe: log.Printf("error %d was %s", i, err), or
// errCh <- err // feed individual errors somewhere
} |
In general, if you want to provide semantically meaningful errors to users, you need to either expose sentinel values (such as |
This proposal has been added to the active column of the proposals project |
@neild I think you are right that that's the "correct" way to code. But I argue that there are at least two circumstances when I still want to examine individual errors First: The code has not been coded correctly. I've done this myself, and I've seen plenty of cases where nobody foresaw that an error needed to be distinguished from another one using sentinel values or concrete types. Sometimes you just need to use an escape hatch. func Interesting() error {
...
return fmt.Errorf(`you will never need to distinguish this from other errors, would ya?`)
}
if err := Interesting(); err != nil {
for _, err := range ExtractErrors(err) {
if err.Error() == `you will never need to distinguish this from other errors, would ya?` { // escape hatch until next release
...
}
}
} Second: You are debugging some error that you haven't seen before. I often sneak in a func Interesting() error { ... }
if err := Interesting(); err != nil {
for _, err := range ExtractErrors(err) {
log.Printf("%#v", err)
}
} I apologize that my arguments have been shifting a bit, but all I really want to say throughout is that I'm pretty sure there's going to be needs in the wild to dissect the contained errors in a multi-error value without knowing the concrete error type/value beforehand. |
It doesn't seem like there is consensus that we should add Split. We only just added Join. Perhaps we should wait for more use cases. If third-party users write and use the Split function, it will be easy enough to add later and will not invalidate any of the other copies. |
Based on the discussion above, this proposal seems like a likely decline. |
No change in consensus, so declined. |
Hm, should simply |
Assuming iterators are added to a future version of Go, there could be a new proposal to add errors.Iter or whatever to walk the error tree if people can show that third parties are already using similar functions elsewhere (eg in popular error logging packages). |
The naming is not that relevant, but the idea of having |
After my previous comment I have realized that joined errors have much more semantic meaning in the join itself than regular wrap. Regular wrap can be used for many reasons to annotate the original error (with a stack trace, additional data, messages, etc.) and so automatic traversal of that wraps make sense when you are searching for example for which layer of wrapping recorded a stack trace or which type the base error is. When joining errors, semantic if much clearer: those multiple errors happened and we are combining them now here. But reversing/traversing those joined errors automatically have much less sense: if you are searching for a stack trace, which one you want from all those errors? If you are searching for a base type, does it make sense to return if any of the errors have that base type? So in my errors package I have made it so that unjoining is a very different action than just simple linear unwrapping. |
An error wrapped with If |
@neild I think that the fact that
Maybe But it is true that one can currently already search for |
#53435 contains fairly extensive discussion on the rationale for the interactions between In particular, see the questions "Why should this be in the standard library?" and "Why is there no function to unwrap a multiply-wrapped error?" in #53435 (comment). |
I propose to add
errors.Errors
(note: I don't like the name either, please think of it as a placeholder) function to handle errors that contain multiple errors.Problem
Upon trying out go1.20rc1's new features around
errors.Join
, I could not immediately find out how one should access individual errors in sequence:This sort of sequential scan is necessary if the contained error does not implement a distinct type or its own
Is(error) bool
function, as you cannot extract out plain errors created byerrors.New
/fmt.Errorf
usingAs()
. You also would need to know what kind of errors exists before hand in order to useAs
, which is not always the case.It can be argued that users are free to use something like
interface{ Unwrap() []error }
to convert the type of the error first, and then proceed to extract the container errors:This works, but if the error returned could be either a plain error or an error containing other errors, you would have to write two code paths to handle the error:
This forces the user to write much boilerplate code. But if we add a simple function to abstract out the above type conversion, the user experience can be greatly enhanced.
Proposed solution
Implement a thin wrapper function around
error
types:Pros/Cons
Pros:
errors.Join
Cons:
error
value itself is returned for plain errors, the result for errors containing other errors does not include the original error itself.The text was updated successfully, but these errors were encountered: