Closed rogpeppe closed 2 months ago
CC @neild
Iterating the error tree became substantially more complicated when we introduced multiple-wrapping. Prior to then, you could just call errors.Unwrap
in a loop, which isn't so bad. I seem to recall that we considered adding a tree-iterator, but decided to wait until we figured out what the iterator API was going to look like.
We've got an iterator API now, so it seems reasonable to add an error-tree iteration function.
Perhaps errors.All
and errors.AllAs[T]
might be more in line with the naming we've been using for iterator functions?
As a higher-level comment, however, I believe that any error semantics where you need to iterate over the tree for everything matching a code are too confusing. I's reasonable for the errors
package to export functions for tree-walking. It isn't reasonable for any other package to expect its callers to use tree-walking functions to examine its errors.
Adding AllAs[E error](error) iter.Seq[E]
for cases where you want to capture all instance of type E seems reasonable to me. I don’t think the other helpers are as obviously necessary.
Perhaps
errors.All
anderrors.AllAs[T]
might be more in line with the naming we've been using for iterator functions?
Yup, I was thinking exactly that last night. I've updated my PoC CL accordingly.
As a higher-level comment, however, I believe that any error semantics where you need to iterate over the tree for everything matching a code are too confusing. I's reasonable for the
errors
package to export functions for tree-walking. It isn't reasonable for any other package to expect its callers to use tree-walking functions to examine its errors.
I'm not so sure. Consider a function F that can return an error with a code as I described. I don't think that's too unreasonable.
Then consider a function G that runs a collection of functions concurrently and returns all the errors as a single error
. That seems OK too to me. Say I'm using G to run multiple calls to F. In that case I think it's perfectly reasonable to use errors.All
to iterate over all the errors looking for any that has a given code.
As another concrete example, and part of the motivation for creating this proposal, the Open Container Initiative (OCI) protocol specifies that errors are returned like this:
{
"errors": [
{
"code": "<error identifier, see below>",
"message": "<message describing condition>",
"detail": "<unstructured>"
},
...
]
}
That is, it's always possible for an API call to return multiple errors. To me it seems natural to represent this as a single error containing an error for each entry in errors
. But then there's currently no easy way to say "give me any error with a given code, including its detail
information".
So although I'd like to agree with your sentiment, I think that there are legitimate cases where it's OK, or even necessary, to expect clients to use the tree-walking function directly.
@earthboundkid Apologies for duplicating your proposal! Happy to close this as a dupe if you like, although it's a little different so perhaps worth keeping for a bit until a decision is made?
Adding
AllAs[E error](error) iter.Seq[E]
for cases where you want to capture all instance of type E seems reasonable to me. I don’t think the other helpers are as obviously necessary.
When you say "other helpers", I think there's only one, right? errors.All
.
Although it's true that that is technically unnecessary (as I pointed out, errors.AllAs[error]
is exactly equivalent to errors.All
), I think that it's still worth including errors.All
. For one, it's the foundation of all the other functions (Is
, As
and AllAs
). Within the errors
package there is currently unnecessary duplication of the tree-walking logic which can be removed by using All
. As such, I'd suggest that it's the simplest and most obvious primitive available, and hence worth providing as part of the errors
API.
Change https://go.dev/cl/573357 mentions this issue: errors: implement All and AllAs
In the other issue there was some skepticism about how common the need for users to walk the tree themselves is. I think making all
a package private function and only exporting AllAs
would probably be fine for most users, but OTOH, it probably doesn't hurt to export All
either.
In the other issue there was some skepticism about how common the need for users to walk the tree themselves is.
If there wasn't a good use case for users to walk the tree themselves, we wouldn't be proposing adding AllAs
either :)
One other thing that's perhaps worth pointing out: I tried a generic version of As
with this implementation:
func AsType[T any](err error) (T, bool) {
for err := range All(err) {
if x, ok := err.(T); ok {
return x, true
}
if err, ok := err.(interface{ As(any) bool }); ok {
var x T
if err.As(&x) {
return x, true
}
}
}
return *new(T), false
}
It turned out quite a bit more efficient than the regular As
:
BenchmarkIs-8 15840567 67.78 ns/op 24 B/op 1 allocs/op
BenchmarkAs-8 5445650 219.6 ns/op 40 B/op 2 allocs/op
BenchmarkAsType-8 16187889 72.72 ns/op 24 B/op 1 allocs/op
I also tried implementing AsType
atop AllAs
:
func AsType[T any](err error) (T, bool) {
for x := range AllAs[T](err) {
return x, true
}
return *new(T), false
}
Unfortunately the performance was considerably worse:
BenchmarkIs-8 17058822 68.82 ns/op 24 B/op 1 allocs/op
BenchmarkAs-8 5518171 218.3 ns/op 40 B/op 2 allocs/op
BenchmarkAsType-8 4377482 279.8 ns/op 192 B/op 10 allocs/op
Hopefully the compiler will be able to make that better in time.
@earthboundkid Apologies for responding to this proposal and not #65428; I must have missed yours when it was created.
@rogpeppe
Then consider a function G that runs a collection of functions concurrently and returns all the errors as a single error. That seems OK too to me. Say I'm using G to run multiple calls to F. In that case I think it's perfectly reasonable to use errors.All to iterate over all the errors looking for any that has a given code.
I don't think G should return an error that wraps the results of all the individual sub-operations. If the user is likely to care about individual errors, it should return an error that contains the per-operation errors, but does not wrap them.
As a concrete example, let's say we have a function that downloads a set of URLs to disk:
// Download writes the contents of each URL in urls to a file under target.
func Download(target string, urls ...*url.URL) error {}
If Download
wraps the errors of each individual fetch, it could return an error that simultaneously indicates that the operation failed because a server rejected the request, because a file could not be written to, and because a network timeout occurred. All this, while also successfully fetching at least one other file. And unpacking this information requires the user to understand the structure of the error tree.
I'd instead either return an []error
:
// Download writes the contents of each URL in urls to a file under target.
// It returns one error for each URL.
func Download(target string, urls ...*url.URL) []error {}
Or define a structured error that contains the individual errors (and does not wrap them):
type DownloadError struct {
ErrByURL map[*url.URL]error
}
This makes the structure of the error explicit and is easy to work with.
As another concrete example, and part of the motivation for creating this proposal, the Open Container Initiative (OCI) protocol specifies that errors are returned like this:
I would probably represent this as something like:
// CodeError is an OCI error code.
type CodeError string
func (e CodeError) Error() string { return string(e) }
const (
ErrBlobUnknown = CodeError("BLOB_UNKNOWN")
// etc.
)
// ErrorComponent is a single component of an OCI error response.
//
// It intentionally does not implement error, since an error can consist of multiple components.
type ErrorComponent struct {
Code CodeError
Message string
Detail any // or string, or whatever
}
// Error is an OCI error response.
type Error struct {
Errs []ErrorComponent
}
func (e *Error) Error() string { /* return some string here */ }
func (e *Error) Is(target error) bool {
for _, c := range e.Errs {
if c.Code == target {
return true
}
}
}
Usage:
// Does this error contain a BLOB_UNKNOWN error?
if errors.Is(err, ErrBlobUnknown) {
}
// Examine the individual components of the error.
// (Hopefully not common, this seems like a real pain to deal with.)
var e Error
if errors.As(err, &e) {
for _, c := range e.errs {
// ...
}
}
@neild I agree that both of your suggestions are viable (although the error message strings implied by your CodeError
definitely leave something to be desired), but ISTM that you're essentially arguing against errors.Join
for any situation where the underlying errors aren't well known by the caller of Join
.
That's a reasonable stance, but given that we have errors.Join
(and it's definitely being used), I think it's reasonable to want to traverse the resulting error tree.
When implementing traversing inside my gitlab.com/tozd/go/errors errors package, I realized that there are multiple ways to traverse and that it is really hard to make one function how you want to traverse:
So I think traversing errors should have API closer to walking directory structure with a function callback and not an iterator. In the case of errors, the directory is an joined error. At that point you have to make a decision how you want to continue iterating and if at all. Instead of files, you can have a chain of parent errors until the joined error, or something.
Here is an observation, that is IMO interesting, but not terribly actionable: When the errors.Is
and errors.As
APIs where originally proposed, my main objection to them was that they are effectively implementing nominal subtyping (“inheritance”) - but purely dynamically. Under that lens, introducing errors.Join
and Unwrap() []error
then added multiple inheritance.
The problem @rogpeppe is now running into is thus the diamond problem of multiple inheritance: There are multiple paths along which his errorWithCode
is "inherited" by the error value that is being inspected and you get different error codes, depending on which path you follow. Saying "we need a more complicated API for walking the tree" pretty much means "we should make it simpler to implement custom Method Resolution Order algorithms".
Ultimately, these design problems around the errors
API are exactly why Go chose structural subtyping for its interfaces: It avoids the path-dependency in the subtype graph.
That's not actionable, of course. The API is, what it is and we must deal with it, pragmatically. But it describes and predicts the scope of the problems.
Depth-first or breadth-first (example: you are searching if existing error has a stack trace or not)?
Depth-first. Is
and As
do a depth-first traversal, and any new functions that traverse an error tree should follow suit.
(Why depth-first? We needed to pick one, and when we added error wrapping to std
every other package that I surveyed was using depth-first traversal.)
a function callback and not an iterator
An iterator is a function that takes a callback; see #61897.
The proposal defines AllAs
's type parameter as any
. Is there any reason this shouldn't be error
?
func AllAs[T error](err error) iter.Seq[T]
I remember there being a reason As
permits its target parameter to be a *T
, where T
is an interface that doesn't implement error
, but I can't remember what it is.
The proposal defines
AllAs
's type parameter asany
. Is there any reason this shouldn't beerror
?func AllAs[T error](err error) iter.Seq[T]
I remember there being a reason
As
permits its target parameter to be a*T
, whereT
is an interface that doesn't implementerror
, but I can't remember what it is.
I think it's probably because not all interfaces that we might be interested in inspecting the error for necessarily implement error
themselves. As a not-very-good example, it's legitimate to use errors.As
to look for an error that implements json.Marshaler
, even though json.Marshaler
clearly doesn't implement error
.
I suspect that consideration was amplified by the fact before Go 1.14, issue #6977 meant that it generally wasn't a good idea to embed interfaces because that ran the risk of conflicts with other types doing the same thing.
In general, I think I agree with you and support using error
rather than any
(it's always be possible to embed error
at the call site).
There's one thing that gives me pause (and was the reason I chose any
for this proposal): the inconsistency with errors.As
, and the fact that it's possible that some As
method implementations may be looking for a pointer to a particular interface type that does not contain error
. I'm not sure that consideration is very important though.
@mitar
Depth-first or breadth-first (example: you are searching if existing error has a stack trace or not)?
For your particular use case (a package deeply involved with the core error implementation), I'd suggest that you'd be best off just writing your own tree-traversal function. Most users will not be in that situation.
Both of your other points are easily implemented using the proposed API.
FWIW I have also considered whether it might be worth adding an API like this:
package errors
// Find traverses the error tree of err and returns the
// first error e for which test(e) returns true.
func Find(err error, test func(error) bool) error
but on balance I thought this was too trivial to be worthwhile.
So I think traversing errors should have API closer to walking directory structure with a function callback and not an iterator.
You might be amused to see that I've actually proposed an API for walking a directory structure with an iterator: #64341 I think it can work pretty decently.
In there a benefit in making the constraint error
? I don't see anything in the signature or implementation of AllAs
that needs - or even benefits from - the type argument being an error
.
Making the constraint error
avoids accidentally passing *SomeError
when you meant SomeError
or vice-versa. We have a vet check to catch this mistake in errors.As
, but a compile-time check is better.
The current proposal as I understand it:
package errors
import "iter"
// All returns an iterator that iterates over all the non-nil errors in err's tree.
//
// The tree consists of err itself, followed by the errors obtained by repeatedly calling its
// Unwrap() error or Unwrap() []error method. When err matches multiple errors,
// All examines err followed by a depth-first traversal of its children.
func All(err error) iter.Seq[error]
// AllAs returns an iterator that iterates over all the errors in err's tree that match target.
//
// See All for a definition of err's tree.
// See As for a definition of how an error matches the target.
func AllAs[T error](err error) iter.Seq[T]
Is that right?
I remember there being a reason As permits its target parameter to be a *T, where T is an interface that doesn't implement error, but I can't remember what it is.
I asked ChatGPT about that and it made a good argument: By accepting interface{} as the target, errors.As allows you to check if an error implements any interface, providing greater flexibility and utility in type assertion scenarios.
So you maybe have your errors implement an interface which does not embed error
interface itself, but you still want to search for that interface. I think that is a valuable use case. For example, you could search for an error which implements Marshaler interface.
That's a great example of ChatGPT saying something that looks useful, but really isn't.
Yes, you can check for a json.Marshaler
:
var m json.Marshaler
if errors.As(err, &m) {
b, err := m.MarshalJSON()
// ...
}
But you also write that with:
var m interface {
json.Marshaler
error
}
if errors.As(err, &m) {
// ...
}
So this isn't about flexibility or utility, but maybe it's about convenience.
I still don't recall whether there was one motivating example that informed the decision on As
, but I do recall now that we drew an analogy to type assertions. As
is essentially a type assertion mapped across the error chain. You can type assert from an error
to a json.Marshaler
, so As
permits the equivalent. You can't type assert from an error
to an int
, so As
rejects this. These checks necessarily need to happen at runtime, so vet
includes a static check to allow early detection of mistakes.
On one hand, we could say that the same argument applies to AllAs
, and AllAs
's type constraint should be any
. In this case, we will need to extend the As
vet check.
On the other hand, we could say that type parameters allow us a greater degree of compile-time checking than was available when we added As
and that you can easily write a loop with All
in the event that you do need to assert to a non-error
interface type, and that therefore AllAs
's type constraint should be error
.
I lean somewhat towards a constraint of error
, as being clearer, offering more compile-time safety, and requiring less mechanism.
I lean somewhat towards a constraint of error, as being clearer, offering more compile-time safety, and requiring less mechanism.
I agree. The only slight friction to my mind is the kind of scenario demonstrated here: https://go.dev/play/p/Ayyuk-oAwiW?v=gotip
In this case there's no way to get errors.AllAs
to produce the error that errors.As
extracts.
This is, however, a fictional scenario: I'm not convinced that this situation would ever arise in practice,
and the fact that the caller can't use the type as-is is perhaps a good indication to the caller that
there might be an issue.
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
These seem okay to me but maybe we should wait on these until after Go 1.23, so that we can make sure we understand iterators for basic uses before we push on them for "advanced" uses like this.
If we were going to do this, it seems like the API would be something like:
package errors
func UnwrapAll(err error) iter.Seq[error]
func UnwrapAllType[T any](err error) iter.Seq[T]
Let's assume that's the API. The bigger question is do we want to add these? They can be written in a separate package, and they seem to encourage fine-grained examination of trees of errors in a way that I've never seen in a Go program and seems very error-prone and maybe a bit too pedantic. It also implies that people should be producing these kinds of detailed trees.
Do we really want to encourage this kind of detailed error construction and deconstruction? Why? What are the use cases where this makes sense?
I don't think UnwrapAll
is useful unless you work for an error logging platform (Sentry, Honeycomb, etc.). I think UnwrapAllType
is potentially useful. For example, maybe you unwrap an error and find both a domain.NotFound and a domain.Forbidden error, and then decide to only act on the Forbidden error since NotFound is information you don't want to leak to outsiders.
Our use case is as a GraphQL API service for end user applications, we want to be able to suppress most errors (except to developers) and allow through "user-visible" errors. GraphQL allows for multiple errors per request (by field), so extracting all the errors of the user-visible type is useful.
These can be written outside the standard library. To put them in the standard library I think we'd need evidence of widespread utility / use. That doesn't seem to be here, at least not yet. Both the posted comments are pretty rare cases.
These can be written outside the standard library.
All of this can be written outside the standard library, because there is nothing in errors
that's truly private.
But I'd contend that the errors
package is the canonical place that "knows" how to traverse the error tree, and the mechanism for doing so has changed over time, so ISTM that the correct place to expose a way to do so is the errors package itself. If things ever changed again, then people could continue using the same mechanism without needing to update dependencies.
I don't think it's that rare to want to check an error for many possible error conditions, and using the errors package to do so currently is rather inefficient in the presence of deeply wrapped errors. There is a reason why the original proposal suggested that users might wish to traverse the error chain themselves, but back then it was considerable easier to do so. The proposed API makes it easy again (and more future-proof).
I also think that the fact that it becomes straightforward to write the existing errors primitives in terms of these new primitives counts in their favour too.
There's a lot of discussion on how it'd be implemented, but I don't see much in terms of how these would actually be used in practice?
I don't really have a strong opinion either way here.
We added errors.Unwrap
in Go 1.13, because we wanted to codify the usage of the optional Unwrap() error
method. We weren't really certain who would need to call this function or why, but we did feel that providing a supported way to unwrap a single layer of an error was better than asking everyone to write their own type assertion.
We don't have an equivalent for the Go 1.20 Unwrap() []error
method. I'd originally proposed an errors.Split
to unwrap a multierr, but the proposal discussion ended with the conclusion that this function was an invitation to misuse: https://github.com/golang/go/issues/53435#issuecomment-1189317089
Maybe it makes sense to include UnwrapAll
, to codify the rules for unwrapping an error chain in a single place.
But maybe it makes sense to leave it out, since most users should be calling errors.Is
or errors.As
.
@earthboundkid
For example, maybe you unwrap an error and find both a domain.NotFound and a domain.Forbidden error
@rogpeppe
I don't think it's that rare to want to check an error for many possible error conditions
Doing this with UnwrapAll
is going to be difficult to do without being subtly different from Is
or As
, since the naive UnwrapAll
loop won't call Is
or As
methods.
(UnwrapAllType
doesn't help with checking for multiple possible conditions, since it unwraps to a single type. It's only useful if you have several conditions in the same error, which is extremely confusing semantically.)
I think that this sort of thing is better handled by defining a domain.ErrCode
type:
var code domain.ErrCode
if errors.As(err, &code) {
switch code {
case domain.NotFound:
case domain.Forbidden:
}
}
@carldunham
GraphQL allows for multiple errors per request (by field), so extracting all the errors of the user-visible type is useful.
I think this case is much better handled by defining an error type which contains the underlying errors.
type FieldErrors struct {
Errs []*FieldError
}
func (e *FieldErrors) Error() string { ... }
type FieldError struct {
Message string
Locations []Location
Path Path
}
var fe *FieldErrors
if errors.As(err, &fe) {
for _, fieldErr := range fe.Errs {
// handle individual field error
}
}
This puts the structure of the error firmly in the type system: A set of GraphQL field errors is represented as a distinguishable type, rather than a vaguely defined collection of wrapped errors. (You could have the FieldErrors
type implement Errors() []error
if you really want, but I wouldn't. Actually, I'm not sure there's much value in having FieldError
implement error
in this scenario.)
I think that this sort of thing is better handled by defining a
domain.ErrCode
type
I'm imagining a scenario where you want to look at multiple things of type domain.ErrCode
. So for example, I have a StatusCoder
interface that's an error + StatusCode() int
. You might want to loop through all StatusCoders in a tree and say if there's a 5XX, prefer that to a 4XX or 3XX, and if there's an error greater than 2XX, prefer that to 2XX.
That could be placed in a map. It's better we find real idioms and not hypothetical ones that make sense.
If people implemented their own error types, then yes. For instance if people regularly implemented list of errors as maps or slices. (but they probably don't because of the current semantics of checking for nil for instance, or comparability).
And especially since an error value can be seen has an iterator of cardinality one I guess.
At best it should probably go into x/ at first. It's true however that if it is a widely used idiom (I still doubt it) then the std lib should be a better place in the end since it can be used pretty generically, for any error returns implemented as an iterator (or not even)
@neild
I think that this sort of thing is better handled by defining a
domain.ErrCode
type:var code domain.ErrCode if errors.As(err, &code) { switch code { case domain.NotFound: case domain.Forbidden: } }
I mentioned this approach in the original issue description, and also mentioned why I believe it's not a great approach.
As an example, imagine there's a "concurrently execute functions" API that returns its errors as a multiple error.
Just because we've found one error that fits the domain.ErrCode
type doesn't mean that there aren't several such errors.
The logic above will not check any error codes other than the first one found.
Also, this approach does not work when there are several entirely different kinds of errors we wish to check for.
@carldunham
GraphQL allows for multiple errors per request (by field), so extracting all the errors of the user-visible type is useful.
I think this case is much better handled by defining an error type which contains the underlying errors.
ISTM that what you're saying there is that the multiple-error functionality isn't really fit for purpose. When is it the correct decision to return a multiple-error error, in fact?
ISTM that what you're saying there is that the multiple-error functionality isn't really fit for purpose.
The reason we have multiple-error wrapping is because objectively, there were a number of packages outside the standard library which attempted to provide it in one way or another. Lacking a commonly-accepted interface to describe a multiply-wrapped error, these packages generally did not interoperate well. Providing that interface in the standard library satisfies a clear demand in the ecosystem. There's extensive discussion in #53435 about the value of a standardized interface for multiply-wrapped errors.
The Error() []error interface provides a way to wrap multiple errors and make those errors visible to Is and As. I've personally found that this is rarely a useful semantic to provide, but the common usage of multierror packages throughout the ecosystem makes it clear that it's a popular one.
The errors package does not provide a simple way to unwrap a specific error value into a list of errors. We'd originally proposed one (the errors.Split function), but removed it from the proposal when it became clear that even when existing third-party packages provided this functionality, it was rarely used.
So I think the multiple-error functionality in the errors package succeeds at its goals, which were
A non-goal was to support easily unwrapping an error into a list of errors, because we lacked evidence that this is a feature that sees real-world usage, and because defining a concrete type that contains a list of errors is generally a clearer API choice.
I think that what would really help for the proposed All and AllAs functions is to see examples of real-world, existing code that would be simplified by these functions. A deciding factor on errors.Join and the rest of #53435 making it into 1.20 (as I recall, perhaps my memory is bad) was that there were popular, widely-used packages providing this functionality. If there's a non-trivial amount of code out there now which iterates error trees, then that's a strong argument in favor of supporting it in the errors package.
To echo what Damien wrote:
I think that what would really help for the proposed All and AllAs functions is to see examples of real-world, existing code that would be simplified by these functions.
This is exactly right. Should we put this on hold until those examples have been gathered, or to give time for those kinds of examples to arise?
I said hold last time but I think we should probably just decline this and we can always file a new one with more evidence later if it is gathered.
Based on the discussion above, this proposal seems like a likely decline. — rsc for the proposal review group
I'm coming to this proposal from https://github.com/golang/go/issues/65428, both of which I support and hoped were revived.
fine-grained examination of trees of errors in a way that I've never seen in a Go program
I don't see much in terms of how these would actually be used in practice?
But maybe it makes sense to leave [
UnwrapAll
] out, since most users should be callingerrors.Is
orerrors.As
.we lacked evidence that [unwrapping an error into a list of errors] is a feature that sees real-world usage
what would really help for the proposed All and AllAs functions is to see examples of real-world
There are realistic scenarios where you want to (more or less exhaustively) process nodes in an error tree, not just search for the first node of interest. See
The (very real) scenario I have in mind is that of a CORS middleware whose configuration can fail for multiple reasons, all of which must be addressed for configuration to succeed. Most clients of the library would just read the messages of the various errors and address the latter one by one; but a SaaS company that relies on the library in their backend and allows its tenants to configure the CORS middleware themselves in the SaaS's Web UI would want to iterate through the error tree, programmatically process each node of interest, and translate the various error messages to something tenants can understand (perhaps even in various natural languages).
These can be written outside the standard library.
~~I can only strongly agree with @rogpeppe about this (see https://github.com/golang/go/issues/66455#issuecomment-2263565464). Do we really want to again find ourselves in the kind of multi-error schism that existed before Go 1.20? I think that, now that iterators have landed, the time is ripe for standardising how to walk error trees. And what better place to do that than the errors package?~~
defining a concrete type that contains a list of errors is generally a clearer API choice.
This seems to conflict with the current trend of proposals aiming to "replace" functions that provide access to elements of a collection via a slice by functions that return an iterator. See
Backwards
to Reverse
)Keys
to the homonymous function in golang.org/x/exp/maps)Edit: TL;DR: I think there sometimes is a need to iterate through an error tree, but what to make of the resulting sequence often depends on the context and the shape of the tree. Therefore, I'm no longer convinced that "generic" All
and AllAs
functions are warranted in the errors
package.
We don't have an equivalent for the Go 1.20
Unwrap() []error
method. I'd originally proposed anerrors.Split
to unwrap a multierr, but the proposal discussion ended with the conclusion that this function was an invitation to misuse: https://github.com/golang/go/issues/53435#issuecomment-1189317089
I agree with @josharian's sentiment in that comment:
I see no evidence that anyone cares much about the structure of the error tree.
But it doesn't apply to the function proposed in this proposal, since an iter.Seq[error]
represents a flat sequence of error
s. The structure of the tree is incidental and immaterial in the overwhelming majority of use cases; what matters is accessing the tree's nodes in a sequential manner (depth first indeed seems sensible).
The only difficulty might be to discriminate leafs from internal nodes (e.g. the results of calls to errors.Join
) in the error tree; the latter may not be as interesting as the former.
Edit: I may have changed my mind about this... 😅 The more I think about it, the less I believe that adding "generic" All
and AllAs
functions in the errors
package would be useful.
In the scenario I outlined in my previous comment, I happen to build the error tree with errors.Join
exclusively. Therefore, in that specific case, I'm only interested in iterating over the leafs of that tree.
However, as Josh pointed out elsewhere, other nodes of the error tree may also be of interest, in the general case. The problem is that an iter.Seq[error]
discards the structure of the tree, and what to make of such a sequence of errors without knowing the structure of the tree they come from isn't clear to me (or to Damien).
For my scenario, I think I'm going to export my own All(error) iter.Seq[error]
function for iterating over just the nodes relevant to me (the leaf nodes). Other packages may have other iteration needs, and perhaps they should export their own custom All
function. 🤷
I want to say I just thought of the perfect names for this. Instead of errors.AllAs
, it should be errors.Are
. 😄 for corsErr := range errors.Are[cors.Error](err) {
etc.
Proposal Details
The original error inspection proposal contained the following paragraph:
Traversing the error chain multiple times to call
Is
is wasteful, and it's not entirely trivial to do it oneself. In fact, the way to iterate through errors has changed recently with the advent of multiple error wrapping. If many people had been traversing the error chain directly, their code would now be insufficient.If we're checking for some particular code in some error type, it's tempting (but wrong) to write something like this:
The above code is wrong because there might be several errors in the error tree of type
*errorWithCode
, but we will only ever see the first one. It would be possible to abuse theIs
method to consider only theCode
field when comparingerrorWithCode
types, but that seems like an abuse:Is
is really intended for identical errors, not errors that one might sometimes wish to consider equivalent.With the advent of iterators, we now have a natural way to design an API that efficiently provides access to all the nested errors without requiring creation of an intermediate slice.
I propose the following two additions to the
errors
package:Technically only
IterAs
is necessary, becauseIterAs[error]
is entirely equivalent toIter
, butIter
is more efficient andIterAs
is easily implemented in terms of it.Both
Is
andAs
are easily and efficiently implemented in terms of the above API.I consider
IterAs
to be worthwhile because it's convenient and type-safe to use, and it hides the not-entirely-trivial interface check behind the API.The flawed code above could now be written as follows, correctly this time, and slightly shorter than the original:
I've pushed a straw-man implementation at https://go-review.googlesource.com/c/go/+/573357