-
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 All and AllAs iterators #66455
Comments
CC @neild |
Iterating the error tree became substantially more complicated when we introduced multiple-wrapping. Prior to then, you could just call We've got an iterator API now, so it seems reasonable to add an error-tree iteration function. Perhaps 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 |
See #65428 |
Adding |
Yup, I was thinking exactly that last night. I've updated my PoC CL accordingly.
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. 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:
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 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?
When you say "other helpers", I think there's only one, right? Although it's true that that is technically unnecessary (as I pointed out, |
Change https://go.dev/cl/573357 mentions this issue: |
In the other issue there was some skepticism about how common the need for users to walk the tree themselves is. I think making |
If there wasn't a good use case for users to walk the tree themselves, we wouldn't be proposing adding |
One other thing that's perhaps worth pointing out: I tried a generic version of
It turned out quite a bit more efficient than the regular
I also tried implementing
Unfortunately the performance was considerably worse:
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.
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:
If I'd instead either return an
Or define a structured error that contains the individual errors (and does not wrap them):
This makes the structure of the error explicit and is easy to work with.
I would probably represent this as something like:
Usage:
|
@neild I agree that both of your suggestions are viable (although the error message strings implied by your That's a reasonable stance, but given that we have |
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 The problem @rogpeppe is now running into is thus the diamond problem of multiple inheritance: There are multiple paths along which his Ultimately, these design problems around the 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. (Why depth-first? We needed to pick one, and when we added error wrapping to
An iterator is a function that takes a callback; see #61897. |
The proposal defines
I remember there being a reason |
I think it's probably because not all interfaces that we might be interested in inspecting the error for necessarily implement 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 There's one thing that gives me pause (and was the reason I chose |
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:
but on balance I thought this was too trivial to be worthwhile.
You might be amused to see that I've actually proposed an API for walking a directory structure with an iterator: #64341 |
In there a benefit in making the constraint |
Making the constraint |
The current proposal as I understand it:
Is that right? |
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 |
That's a great example of ChatGPT saying something that looks useful, but really isn't. Yes, you can check for a
But you also write that with:
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 On one hand, we could say that the same argument applies to 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 I lean somewhat towards a constraint of |
I agree. The only slight friction to my mind is the kind of scenario demonstrated here: In this case there's no way to get |
This proposal has been added to the active column of the proposals project |
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:
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 |
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. |
All of this can be written outside the standard library, because there is nothing in But I'd contend that the 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 We don't have an equivalent for the Go 1.20 Maybe it makes sense to include But maybe it makes sense to leave it out, since most users should be calling
Doing this with ( I think that this sort of thing is better handled by defining a var code domain.ErrCode
if errors.As(err, &code) {
switch code {
case domain.NotFound:
case domain.Forbidden:
}
}
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 |
I'm imagining a scenario where you want to look at multiple things of type |
That could be placed in a map. If people implemented their own error types, then yes. For instance if people regularly implemented list of errors as maps or slices. 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. |
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. Also, this approach does not work when there are several entirely different kinds of errors we wish to check for.
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:
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. |
No change in consensus, so declined. |
I'm coming to this proposal from #65428,
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).
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
|
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"
Edit: I may have changed my mind about this... 😅 The more I think about it, the less I believe that adding "generic" In the scenario I outlined in my previous comment, I happen to build the error tree with 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 For my scenario, I think I'm going to export my own |
I want to say I just thought of the perfect names for this. Instead of |
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 thefirst one. It would be possible to abuse the
Is
method to consideronly the
Code
field when comparingerrorWithCode
types, but thatseems 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
The text was updated successfully, but these errors were encountered: