proposal: errors: add With(err, other error) error #52607
Comments
|
This has some similarities to #48831. |
ianlancetaylor
added
the
error-handling
|
I understand that the bar is high to put something new in the standard library, but I think the fact that there are at least 3 prior proposals about this (and probably more, I know I've seen multierror support requested a few times) speaks to how many people need this behavior. The comment that it needs to exist in an external library seems to be satisfied by these:
As for why it needs to be in the standard library, I think because it is not an obvious or easy function to create, and it's hard to write correctly. Having a function like this in the errors package gives everyone an answer to "how do I decorate an existing error with more metadata without losing the original error information?" And the answer is right there next to the rest of the error handling code. Right now, there's no real answer to that. Just as we have put error wrapping code into the stdlib to be able to decorate an error's message without losing the original error, adding this function could make it so that we can decorate an error with more than just more text. This will help avoid leaking implementation details and distant coupling that can happen when your only options are "add some text to the existing error" or "throw away that error and return your own".
I'm not sure this means. Discoverability is worse than what? The error returned does implement errors.Is. And adding metadata at the point where an error is first encountered in your code is the best place to do that. That's where you have the most context about what an error means and what it represents. |
Didn't you just disprove this with https://go.dev/play/p/usV6mFRjxxb? 30 lines of very straight forward implementation. I'd like to point out that nothing that consumes this needs to know how such an error was constructed; the consumer doesn't even know if you used an stdlib implementation or 3rd party one. As far as I know, the listed 3rd party projects largely predate I really do appreciate that this proposal explicitly does not attempt to display multiple errors. This is more like decorating the error with more data for programmatic access, which if not used is ignored in the final display. I find that much saner. |
FWIW you don't have to do anything special for type NotFoundError struct {
Err error
}
func (e NotFoundError) Error() string {
return e.Err.Error()
}
func (e NotFoundError) Unwrap() error {
return e.Err
}Then the client can do: var nfe NotFoundError
if errors.As(err, &nfe) {
...
}The only reason a more complicated implementation is needed is if you specifically try to return another sentinel error. |
30 lines of code that had two subtle bugs in it that I missed even after writing the initial tests.... and I've written almost this exact library before. The first time I wrote it, it was a couple days of work to wrap (heh) my head around errors.Is and errors.As and what the interfaces really expected. And even then, since I was new to the idea, I came up with something much more complicated than this. One of the major benefits of having this in the standard library is that it makes the functionality discoverable. By having it in the standard library, we're saying "hey, this is a thing you might want to do". We give devs a standard and easy way to let people programmatically check their errors, without having to throw away the underlying implementation errors. |
|
While I agree with the argument "it's simple to do", I would suggest we're overestimating engineers. I would prefer everyone be aware of a convention like in the aforementioned counter arguments, but I do not think it's realistic and what follows is lower quality error handling and discovery than what could exist with this change. If we intend to have ubiquitous adoption of the aforementioned convention, I would argue it makes sense to include in the stdlib? Thank you for the proposal and looking forward to the discussion! |
|
I have some support for this proposal, but also some concerns and at least one design question. In supportI can see the utility of something like this. I believe there is a real challenge to maintain the discipline to raise the abstraction level of errors as they are returned up the call stack while at the same time not obliterating the useful debugging information contained in the original lower level errors. In particular, API services I've worked on—which have typically used github.com/go-kit/kit packages and design patterns—often have error handling complexities that fit the use cases described in this proposal. Here is some code taken from one of the Go kit examples, it is akin to the That code predates I believe a well known way to transform a system-domain error into a business-domain error without losing the ability to log the original system-domain details for triaging and debugging purposes would add value. I believe that's what this proposal is aiming for. ConcernsI am not confident the approach taken here is general enough for the standard library. #52607 (comment) references several existing packages providing some form of composite errors. Are we seeing convergence on a common approach across those implementations? I've always been reluctant to use any of those that I've researched in the past because some aspect of their semantics wasn't quite what I needed at the time. Design QuestionsA portion of the Restating this proposal: The docs for What is the rationale for |
Not across those implementations, but also those are mostly older than error wrapping, which was introduced only 2.5 years ago. Since this is often application-specific code, wrapping may often be done as a one-off library inside application code, which can be hard to find and in many cases is in private repos (like the code I've written at Mattel and GitHub).
Honestly, almost no one is going to call Unwrap directly. The Unwrap interface mostly exists to support Is and As. Is and As, because there is a notion of failing, can effectively operate on both errors, checking one and falling back to check the other value. But for Unwrap, there's no way to fall back (as far as I can tell). There's no way to say "ok, unwrap until you get to the bottom of For Is and As, I decided to check the For Unwrap, I had to choose something, and the idea is that
That should actually work just fine. Each errors.With effectively adds one error to the top of the stack. Is and As would detect any errors in any branches of the tree. |
|
Is the actual problem here that you want error unwrapping that does not escape to external clients? If so, this feels like a roundabout way of addressing it. |
|
So this is
Is that correct?
Challenge accepted: https://go.dev/play/p/be3AVWRAoTO |
|
What I want is to solve this: func (l *Loader) LoadConfig(name string) (*Config, error) {
b, err := os.ReadFile(filepath.Join(l.configDir, name))
if os.IsNotExist(err) {
return nil, fmt.Errorf("can't find config: %w", err)
}
if err != nil {
// I don't know, something went wrong.
return nil, fmt.Errorf("error opening config: %w", err)
}
cfg, err := parseConfigFile(b)
if err != nil {
// malformed config
return nil, fmt.Errorf("malformed config: %w", err)
}
return cfg, nil
}If you're calling LoadConfig, how do you differentiate programmatically between "config not found" and other errors, so that you can handle that error differently? Without errors.With, you have two choices.... 1.) look for the string "can't find config" in err.Error() (gross) Most people do #2. The problem is that calling os.IsNotExist is causing your code to rely on the hidden implementation details of LoadConfig. That method intentionally hides how it stores configs. If it changes so it's getting the config from a database instead of off of disk, os.IsNotExist will stop detecting the not found condition, and your code that is supposed to do something in that case won't trigger. If you instead write this code with errors.With, you can wrap the error with a sentinel error in the not found branch. Then it is detectable by callers, without losing the possibly valuable information in the underlying error that would happen if you just returned the sentinel error instead: var ErrConfigNotFound = errors.New("config not found")
var ErrConfigNotValid = errors.New("config not valid")
func (l *Loader) LoadConfig(name string) (*Config, error) {
b, err := os.ReadFile(filepath.Join(l.configDir, name))
if os.IsNotExist(err) {
return nil, errors.With(err, ErrConfigNotFound)
}
if err != nil {
// I don't know, something went wrong.
return nil, fmt.Errorf("error opening config: %w", err)
}
cfg, err := parseConfigFile(b)
if err != nil {
// malformed config
return nil, errors.With(err, ErrConfigNotValid)
}
return cfg, nil
}Now when you want to check for when the config is not found, you just do cfg, err := l.LoadConfig(name)
if errors.Is(err, loader.ErrConfigNotFound) {
// handle missing config
}
if errors.Is(err, loader.ErrConfigNotValid) {
// handle invalid config
}
if err != nil {
// handle unknown error
} |
|
@natefinch Again, this is only because you are trying to use sentinel errors instead of error types. type ConfigError struct {
Err error
ErrorCode ErrorCode
}
func (e ConfigError) Error() string {
return e.Err.Error()
}
func (e ConfigError) Unwrap() error {
return e.Err
}var cfgErr ConfigError
if errors.As(err, &cfgErr) {
switch cfgErr.ErrorCode {
...
}
}The other nice thing is if you do want the underlying error to be opaque (so clients cannot use |
Yeah, basically? u.other could contribute to the returned error's string. It could have func (u union) Error() string {
return u.other.Error() + ": " u.error.Error()
}I kinda like not doing it by default, in case you don't want the added error to change the error string, and you can always use fmt.Errorf to add the string to the wrapped error if you wanted to, e.g.: err = fmt.Errorf("config not found: %w", err)
return nil, errors.With(err, ConfigNotFound)
Your way is 10 lines of code (though you could get it down to 6) for every single sentinel error you need. Experience shows us that people generally are just too lazy to do that. In addition, it's not an obvious pattern. If there's errors.With right there in the package, it's much easier to find and figure out how to use, especially if there's a nice piece of example code. And then your sentinel errors are literally just a single line. |
|
Quick note, I updated the proposed implementation because it would end up running through the list of wrapped errors more times than it needed to. This is more correct, I think: https://go.dev/play/p/wHYEv5QrQXD (told you it was easy to write incorrectly! :) |
|
Does this proposal support iterating the list of errors? As far as I can tell if you have a nested chain of With calls like err = errors.With(errors.With(errors.With(err, ErrFoo), ErrBar), ErrBaz)You can only test if ErrFoo, ErrBar or ErrBaz exist individually but it doesn't seem possible to get them all as a slice. |
|
I'm a big fan of sentinel errors (particularly I like it, but it sends a mixed message, IMO. |
|
We at @symflower are in support of this proposal for a simple reason: we have implemented the same functionality (plus additions such as adding stack traces and arbitrary data without doing custom types again and again) for our whole code base of the company. The quote "The main use case for this function is incredibly common, and I've seen it in basically every go application I've ever written." resonance so strongly with me because even before this company i have done the same thing in private and open source projects. We strongly believe that handling chains/lists of errors via the standard is what is missing in Go. There are a multitude of open source projects that implement such functionality and it is basically needed in every project that does error handling with adding context to their errors. The later is something that should IMHO done by everyone to improve discoverability and debuggability of programs. I have seen enough hours wasted of earth to know that more context for errors is a good thing. For the reason why we did the same functionality: wrapping with fmt.Errorf, doing the textual wrapping, is no good for checking/switching for specific errors really fast and for actually using the meta information for other reasons than displaying it textually to the user. |
|
edit: this implementation doesn't actually work :( Actually, I think @jimmyfrasche has the key here. You can replace the whole underlying implementation with this: // With returns the union of err and other, effectively stacking
// other on "top" of err. Unwrap, Is, and As consider
// other first and then move to err if other does not match.
func With(err, other error) error {
return union{top: other, error: err}
}
type union struct {
top error
error
}
func (u union) Unwrap() error {
// unwrap the "top" errors first.
if err := Unwrap(u.top); err != nil {
return union{top: err, error: u.error}
}
// we've run out of unwrapping the errors
// stacked on "top", so return the original error.
return u.error
}That's it. That's the implementation. It logically stacks the second error on top of the first, and lets Unwrap iterate through both, so now Is and As work through the same list as Unwrap (and thus we don't even need to implement those interfaces). |
|
Nice, that solves my concern of being unable to iterate through the errors. Its implementation is also very simple. Even if this proposal does not pass, I can use this as a private type in my own packages. |
|
OK, so that's what I get for not actually running the tests on my "simple fix" above. After more experimentation, I combined the iterative unwrapping from @jimmyfrasche with explicit implementation of Is and As, so that Unwrap, Is, and As, all iterate through the errors in the same order. I had to copy the implementation details of errors.Is and errors.As so that we don't unwrap the same error multiple times when calling Is or As... which also shows that this really should live in the standard library. Nobody should have to write this code: |
|
I don't like this proposal because IMO it makes it easier to construct bad APIs. I do recognize that the fact that the issue has been proposed and rejected before means there probably is some underlying itch that needs to be scratched, but I don't think this is quite it. I think the "error flags" API in Nate's https://npf.io/2021/04/errorflags/ blog post is a better solution, if a little more complicated. Assuming the existence of an errorflags package as explained in that blog post, Nate's motivating example would look like: This API is better than using errors.As/Is because it a) puts all the error conditions in one place for an application, allowing better mastery of the error domain b) allows for custom error logic (such as having custom user messages and response codes) c) allows for custom default values (treat missing flags as I think changes to the standard library should focus on making it easier to write a good errorflags package, rather than making it easier to avoid writing an errorflags package and just use an adhoc system of errors.With surrogates. |
|
Error handling usually has to satisfy three parties:
The reason why I dislike this proposal is that it only deals with the first two. A more general approach like The existence of all those multierr packages is probably a sign that something should be added to the standard library. |
|
If I understand this proposal correctly, it doesn't really capture the main cases in which wrapping errors are currently used. When I look at the cases in the standard library, I don't see cases where we have two types that implement That is, the pattern is type MyError struct {
// various fields
Err error // underlying error
}
func (m *MyError) Error() string {
return fmt.Sprintf(format, various fields, m.Err)
}
func (m *MyError) Unwrap() error {
return m.Err
}Based on that perhaps we should consider // ErrorWrapper describes a type that can be used to wrap an error.
// The ErrorWrap message takes the wrapped error and returns a string describing the overall error.
type ErrorWrapper interface {
ErrorWrap(error) string
}
// WrappedError is a wrapper around an error.
type WrappedError[T ErrorWrapper] struct {
Wrapper T
Err error
}
func (we *WrappedError[T]) Error() string {
return we.Wrapper.ErrorWrap(we.Err)
}
func (we *WrappedError[T]) Unwrap() error {
return we.Err
}Now if a general concept like "not found" is meaningful, and does not require any additional information, it becomes a type rather than a value. type notFound struct {}
func (notFound) ErrorWrap(err error) string { return err.Error() }
type NotFound errors.ErrorWrapper[notFound]We can also move general cases like type syscallError string
func (se syscallError) ErrorWrap(err error) { return string(se) + ": " + err.Error() }
type SyscallError errors.ErrorWrapper[syscallError] |
|
A minor suggestion, instead of exposing the |
This case happens most often when an error is returned from another package and needs to be annotated as it is returned from this package. That happens all the time in application code, but it's more rare in library code. The standard library is mostly library code. But what most Go developers write every day at work is application code. There's some wonky error handling in the
The problems with your generic wrapping errors are that 1 is clearly adding complexity. 2... To check for these, you have to use errors.As, even though you don't actually care about the value. so you have to write this: f, err := FindSomething()
if nf := new(NotFound); errors.As(err, &nf) {
// handle not found
}When you could have this with a sentinel value: f, err := FindSomething()
if errors.Is(err, NotFound) {
// handle not found
}The latter is much easier to read IMO. Also, if we have my union error implement func (u union) Error() string {
return u.top.Error() + ": " + u.error.Error()
}Then any two errors can be wrapped, and neither one has to implement Unwrap or ErrorWrap and you still get their error messages concatenated. And 99% of the time, the error output of these wrapping structures is just "mystring: errorString". Then you can make SyscallError a normal error type: type SyscallError string
func (se SyscallError) Error() string { return string(se) }And errors.With(err, SyscallError(syscall)) |
|
I think a useful principle of the current
|
|
@jba We can clarify |
|
@neild Would |
My multierrors package has |
|
I apologize: it looks like multi-errors were already discussed and rejected (#47811). I missed that discussion. It appears hasty to me, especially compared to this one, and I think the answers to most of the conundrums posed there are (now, to me) obvious. I'll post there, FWIW, but it seems that the proposal process has spoken and we should continue discussion of this proposal on its own merits. |
|
I was thinking over the weekend about how some people on this thread (and elsewhere) want one error's message or the other or both, and how that's kinda hard to do nicely in one function... but I wonder if the answer is just to make 3 functions instead of one? Only the result of .Error() changes. The behavior for all is identical to the original As much as I want it to be doable with a single function, I don't think there's a single behavior that'll satisfy everyone, and I'm afraid that'll be a dealbreaker. I guess another option is that we always join the error messages, but also have an errors.Silent(err) that'll squelch an error's output... so you could write
That's a bit wordy, but is a bit more explicit at the callsite than Wrap/With/Mask. |
I think this is worthwhile line of thought. And it would be nice to have a function do one thing and not be so configurable. If the chief concern is control over how the wrapped error formats, then perhaps the answer is something like: // Join returns a new error that joins two errors together such that:
// - errors.As and errors.Is applies to both of them in order (first, then second)
// - errors.Unwrap returns the second error
// - Error() string returns the formatted string: fmt.Sprintf(format,first,second)
//
// Example:
//
// err := errors.Join("%v: %v",err1,err2) // Common
// err := errors.Join("%v%.0v",err1,err2) // Rare: silence err2
// err := errors.Join("%.0v%v",err1,err2) // Rare: silence err1
// See example: https://go.dev/play/p/bKiZLM41OR2
func Join(format string, first error, second error) error {
return union{format: format, first: first, second: second}
}
type union struct {
format string
first error
second error
}
// implementation for example only. It might be better to precompute the message like in fmt.Errorf
func (u union) Error() string {
return fmt.Sprintf(u.format,u.first,u.second)
}Perhaps if it was really necessary to get at the first error, then the // First returns the first error of the pair
func (u union) First() error {
return u.first
}
// Second is unnecessary since Unwrap() will take care of it. could be defined for symmetry?In your own code, you could write your own helper methods like: func With(err error, newErr error) {
return errors.Join("%v%.0v",err1,err2)
}
func Mask(err error, newErr error) {
return errors.Join("%.0v%v",err1,err2)
}Since the tough job of getting the NOTE: I'm explicitly leaving out the possibility of joining more than two errors because of the problem I've mentioned in a prior comment. Why not extend fmt.Errorf?To be fair, this looks like it devolves into supporting: fmt.Errorf with two Overloading this existing function with a new behavior makes it more difficult to explain and probably easier to misuse: If i can use two %w, why not three? Will this function panic? Silently succeed but only wrap two of them? |
|
Also my problem with fmt.Errorf with more than one For example, if the function wrapped the way we have been here (original, new) with new wrapping "over" original, but you wanted to have the error message be And also the same thing with the I much prefer a discoverable syntax, not making our main API for error wrapping stringly typed. |
rsc
changed the title
|
The part that's confusing to me after reading the proposal text (I'm sorry if that was addressed in the comments, already; I looked through the discussions above, but haven't noticed an answer to that): // the snippet from the proposal description as of today
func (st *Storage) SetUserName(···) error {
// ···
if errors.Is(err, pq.ErrNoRows) {
return nil, errors.With(err, flags.NotFound)
}
}It seems that, with the suggested implementation, a user of the err := s.SetUserName()
if errors.Is(err, flags.NotFound) { // works
if errors.Is(err, pq.ErrNoRows) { // doesn't work
if errors.Is(err, flags.NotFound) {
if errors.Is(err.Unwrap(), pq.ErrNoRows) { // works
}
err2 := fmt.Errorf("set user name: %w", err)
if errors.Is(err2, flags.NotFound) {
if errors.Is(err2.Unwrap(), pq.ErrNoRows) { // doesn't work
} |
|
There have been a number of proposals, so it's hard to keep track of which ones do what, but most of them keep |
|
This proposal is essentially a duplicate of #50819 (errors.Mark), except that 50819 did not talk about errors.As (but could have). In particular it had the properties listed above:
except s/With/Mark/. We declined Mark back in February saying:
It seems like the same is true here: not much has changed in the intervening three months. It seems like we should decline this one too. |
|
Based on the discussion above, this proposal seems like a likely decline. |
@rsc I guess I don't understand this comment. It already has been implemented by many individuals and organizations. What evidence of usefulness is the proposal review group looking for in particular? Would it be a viable option to add something like this to golang.org/x/exp/ to gauge its uptake? I think having a semi-official repo would do a lot to consolidate users of various existing libraries across the community. |
|
I have to agree with @natefinch. Multiple libraries doing this already exist, demonstrating the need. Some of the parties responsible for those even chimed in. The point of a proposal at this time would be standardizing around something. I'm not saying this is it, but what is the avenue for standardization then? Without something that says: “the Go team is considering this,” every big team will just continue to roll their own. |
|
There's no one path to standardization, but it doesn't seem like we have the right semantics yet, which was the case in #50819 as well, with not much changing since then. The best place to experiment with different semantics is outside the standard library. |
|
No change in consensus, so declined. |
and others
Background
Right now, the only way to wrap an error in the standard library is with
fmt.Errorfwhich means that all you can do to an error is add text to its.Error()output.When you receive an error from a function and want to return it up the stack with additional information, you have 3 options
fmt.Errorf, which just adds textual output, and therefore isn't something callers can programmatically check forerrors.Is,.As, and.Unwrapto allow callers to access the underlying cause of the error.Proposal
This proposal offers a much easier way to do number 3, by implementing a new function in the stdlib errors package that supports wrapping any error with any other error, such that they are both discoverable with errors.Is and errors.As.
This means that:
errors.With(err, flag).Error() returns err.Error()
errors.Unwrap(With(err, flag)) returns err
errors.Is(With(err, flag), target) is the same as calling errors.Is(flag, target) || errors.Is(err, target)
errors.As(With(err, flag), target) is the same as calling errors.As(flag, target) || errors.As(err, target)
Why This Approach?
By reusing the existing error wrapping pattern, we make the smallest possible change to the standard library, while allowing the broadest applicability of the functionality. We introduce no new interfaces or concepts. The function is general enough to support many different use cases, and yet, its use would be invisible to anyone who is not interested in using error wrapping themselves. It imposes no added burden on third parties that check errors (beyond what is already standard with fmt.Errorf wrapping), and can be ignored by authors producing errors, if that is their wish.
Use Cases
The main use case for this function is incredibly common, and I've seen it in basically every go application I've ever written. You have a package that returns a domain-specific error, like a postgres driver returning pq.ErrNoRows. You want to pass that error up the stack to maintain the context of the original error, but you don't want your callers to have to know about postgres errors in order to know how to deal with this error from your storage layer. With the proposed
Withfunction, you can add metadata via a well-known error type so that the error your function returns can be checked consistently, regardless of the underlying implementation.This kind of error is often called a Sentinel error. fs.ErrNotExist is a good example of a sentinel error that wraps the platform-specific syscall error.
This keeps the error categorization very near to the code that produces the error. Nobody outside of SetUserName needs to know anything about postgres driver errors.
Now in the API layer, you can translate this error to an HTTP error code trivially:
This code doesn't know anything about postgres. It uses the standard errors.Is to check for errors it knows about. But if it then decides to log that error, it has full access to the original error's full context if it wants to dig into it.
This code is very insulated from any implementation changes to the storage layer, so long as it maintains its API contract by continuing to categorize the errors with the same error flags using errors.With.
What This Code Replaces
Without code like this, the handleError functions above would have to do something like this itself:
Not only is that super gross to be doing in the API layer, but it would silently break if someone decided to change database or even just the name of the constraint, and didn't realize that this far away method was digging deep into that error. I've seen and written this kind of code many times in my career, and it feels really wrong, but without stdlib support, it's hard to know what the alternative is.
Production Experience
I used this kind of error wrapping at Mattel for a long time, and now I'm introducing it to GitHub's codebase. It VASTLY improved our error handling in both cases, with very little cognitive or processing overhead. It has made it much easier to understand what an error return really means. It has made writing correctly-behaving API code much simpler by breaking the coupling between two disparate parts of the system.
Community Examples
Conclusion
Adding this method to the standard library would encourage the use of this pattern. Packages using this pattern have better-defined and more stable contracts about how callers can programmatically respond to different categories of errors, without having to worry about implementation details. This is especially true of application code, where error categorization has traditionally been difficult.
Special thanks to @rogpeppe for his suggested improvement to my original error flags idea.
The text was updated successfully, but these errors were encountered: