go: proposal: errors: add With(err, other error) error

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:

• https://pkg.go.dev/go.uber.org/multierr
• https://pkg.go.dev/github.com/hashicorp/go-multierror
• https://pkg.go.dev/github.com/go-openapi/errors#CompositeError
• https://pkg.go.dev/v2ray.com/core/common/errors#Combine
• https://pkg.go.dev/github.com/hashicorp/errwrap#Wrap
• https://pkg.go.dev/github.com/juju/errors#Wrap
• https://pkg.go.dev/github.com/cosmos/cosmos-sdk/types/errors#Wrap

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”.

This seems like it would make discoverability slightly worse as the extra error matching is shifted to point of construction vs having the error type implement errors.Is

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.

@jimmyfrasche

So this is fmt.Errorf(“msg: %w”, err) but msg can be a custom error type err does not contribute to the returned error’s .Error() Is that correct?

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)

@rittneje

Again, this is only because you are trying to use sentinel errors instead of error types.

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.

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) 2.) call os.IsNotExist(err) on the returned error. (also gross, but less obviously so)

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
}

edit: this implementation doesn’t actually work 😦 edit2: But this does: https://go.dev/play/p/MkXOS_aXtM0 edit3: Now without causing unecessary unwrappings: https://go.dev/play/p/K99JW49Js-G

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).

Didn’t you just disprove this with https://go.dev/play/p/usV6mFRjxxb? 30 lines of very straight forward implementation.

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.

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 error and want to combine them. What I see are structs with a field Err error, with Error and Unwrap methods. The Error method returns some string that incorporates Err.Error(), and the Unwrap method returns Err.

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 os.SyscallError, which can become

type syscallError string
func (se syscallError) ErrorWrap(err error) { return string(se) + ": " + err.Error() }
type SyscallError errors.ErrorWrapper[syscallError]

@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 os.IsNotFound or the like), you can still do so by not implementing Unwrap and making Err private.

I have some support for this proposal, but also some concerns and at least one design question.

In support

I 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 handleError function shown above.

https://github.com/go-kit/examples/blob/e18c82edc2f9c7de3aff9748751d63270c052c1b/shipping/booking/transport.go#L181-L195

// encode errors from business-logic
func encodeError(_ context.Context, err error, w http.ResponseWriter) {
	w.Header().Set("Content-Type", "application/json; charset=utf-8")
	switch err {
	case cargo.ErrUnknown:
		w.WriteHeader(http.StatusNotFound)
	case ErrInvalidArgument:
		w.WriteHeader(http.StatusBadRequest)
	default:
		w.WriteHeader(http.StatusInternalServerError)
	}
	json.NewEncoder(w).Encode(map[string]interface{}{
		"error": err.Error(),
	})
}

That code predates errors.Is, but a newly written one would likely use it or maybe errors.As. Go kit has an FAQ about error encoding that hints at some of the dilemmas involved. What I always found unsettling was that despite the nice degree of decoupling that a typical Go kit application has between the various pieces, these error handling functions were the most likely to have the coupling skeletons hidden in them.

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.

Concerns

I am not confident the approach taken here is general enough for the standard library. https://github.com/golang/go/issues/52607#issuecomment-1112666938 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 Questions

A portion of the errors.As docs for reference:

// As finds the first error in err's chain that matches target, and if one is found, sets
// target to that error value and returns true. Otherwise, it returns false.
...
// An error matches target if the error's concrete value is assignable to the value
// pointed to by target, or if the error has a method As(interface{}) bool such that
// As(target) returns true. In the latter case, the As method is responsible for
// setting target.
...
func As(err error, target any) bool

Restating this proposal:

// With returns an error that represents the union of the two errors. 
// It will report true from errors.Is and errors.As if calling
// that function would return true on either err or other. 
// Calling errors.Unwrap will return err.
func With(err, other error) error

The docs for With do not address which of the two errors that As sets target to when it returns true. The choice becomes ambiguous if both err and other are assignable to the value pointed to by target. The only hint is the last sentence which says that Unwrap returns err which suggests that perhaps err is given priority over other in general and one might guess that As would follow suit. But the linked implementation prioritizes other in its As method:

func (u union) As(target any) bool {
	if errors.As(u.other, target) {
		return true
	}
	return errors.As(u.error, target)
}

What is the rationale for Unwrap and As to behave “differently” here? I also wonder how weird this might get with multiple layers of With wrappings.

it seems like it would make sense to implement this outside the standard library and gauge how useful it is.

@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.

fmt.Errorf faces most of the same problems as errors.With, actually. You can’t use it to wrap an error without including that error’s text in the new error message.

This isn’t quite true, it turns out:

https://go.dev/play/p/e2_pY73AsEw

package main

import (
	"errors"
	"fmt"
)

var ErrSomethingBad = errors.New("something bad")

func main() {
	fmt.Println(fmt.Errorf("silent wrapped error%.0w", ErrSomethingBad))
}

Prints:

silent wrapped error

As defined in this proposal, With clearly creates a tree. The error chain of the returned error is (somewhat) a preorder or postorder traversal of that tree.

Unwrap wraps the elements of the first branch traversed in a transient data structure, to maintain state of the tree traversal through successive Unwrap calls. This means that Unwrap can allocate, which makes errors.Is unexpectedly expensive. I don’t think Unwrap should ever allocate.

The wrapping means that the error chain doesn’t quite contain a traversal of the tree: For some nodes, it traverses over wrapper errors that contain the actual error. There is no way to recover the wrapped error in this case. This seems surprising, given that With goes to some lengths to preserve the chain structure of both its error parameters.

The original Go 1.13 errors proposal included stack frame information in errors. While this was ultimately dropped from what made it into Go 1.13, it is possible that we may want to revisit this feature–making information about the stack frames associated with errors is a common desire. If we imagine associating each error with the stack location of its creation, the chain produced by With will jump around in a confusing fashion. This is a point we should be careful about.

The text of errors produced by With is not obvious. In the current proposal, With(err1, err2) produces an error with the text “err2: err1”. Why is the order of the errors reversed from the parameters? Is the ": " universally applicable here? What does a user do if this isn’t the text they want? One benefit of the %w formatting verb is that it makes the text of the wrapping error explicit and under the control of the user; the proposed With loses that.

Some possible directions for future refinement that I see:

• Is it necessary to graft together two error chains, or would a function func(wrap, is error) error that returns an error where err.Unwrap() == wrap, err.Error() == wrap.Error(), and errors.Is(err, is) == true suffice?
• Or even simpler, instead of Wrap(err1, err2), would fmt.Errorf("%w: %v", err2, err1) suffice? This produces an error which excludes err1 from the chain entirely. Since the motivating examples are all of cases where the inner error is intended be opaque to the user, this seems like a feature rather than a defect. (In one example from the original post, “Nobody outside of SetUserName needs to know anything about postgres driver errors.” If nobody needs to know anything about these errors, why should they be in the error chain?)
• Contrariwise, if it is necessary to graft together multiple error chains into a tree structure, wouldn’t a simpler interface be to permit multiple %w verbs in fmt.Errorf? Instead of Wrap(err1, err2), fmt.Errorf("%w: %w", err2, err1). This (I believe) could behave identically to the current proposal for Wrap, while making the error text explicit.

@ianlancetaylor

When I look at the cases in the standard library, I don’t see cases where we have two types that implement error and want to combine them.

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 net package, but I haven’t sat down to figure out how much it could be simplified with either of our approaches. I think anywhere where you need to implement a type, then With is not a huge benefit (but it still is a benefit, because you don’t have to worry about implementing unwrapping).

type WrappedError[T ErrorWrapper] struct {

The problems with your generic wrapping errors are that 1.) You’re introducing a whole new interface that people have to care about, ErrorWrapper 2.) You have to implement a type for every error you want to check for.

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 Error() like this:

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 NewSyscallError(syscall string, err error) error could just be

errors.With(err, SyscallError(syscall))

So this is fmt.Errorf("msg: %w", err) but

• msg can be a custom error type
• err does not contribute to the returned error’s .Error()

Is that correct?

There’s no way to say “ok, unwrap until you get to the bottom of u.other and then start unwrapping u.error”.

Challenge accepted: https://go.dev/play/p/be3AVWRAoTO

Based on the discussion above, this proposal seems like a likely decline. — rsc for the proposal review group

A sentinel and an error are two different kinds of things.

FWIW, some of us have been using “sentinel error” to refer to an error value intended to be tested for equality or with errors.Is. For example, io.EOF is a sentinel error. Contrast *os.PathError, which is a type; or an error created in situ with errors.New, which has a different identity each time it is created.

If I write errors.With(errors.With(err1, err2), errors.With(err3, err4)), then it seems to me that I have a tree of errors.

If I write errors.With(errors.With(MyError{"a"), io.EOF), errors.With(MyError("b"), io.EOF)) and then pass that to var me MyError; errors.As(err, &me) it seems ambiguous whether I get MyError("a") or MyError("b").

In https://github.com/cockroachdb/errors, we have defined the following API:

// Mark extends 'err' so that errors.Is(err, reference) returns true.
// The error message of 'err' is unchanged.
func Mark(err error, reference error) error

it would be better to have the sentinel error’s message prepended to the base error message

From our experience using the above API, the extra sentinel error string should not be combined with the annotated error. Any form of combination (before, after, whatever) is utterly confusing because both errors typically have their own prefix, with context and sometimes multi-line text. In our API, the extra annotated sentinel is only visible when printing the error with %+v.

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
}

This implementation of Unwrap allocates a union for every error in the chain below the first union. That seems very expensive; errors.Is and errors.As shouldn’t allocate.

This proposal makes a fairly major change to the structure of wrapped errors. At the moment, we talk about the “error chain”–an ordered list of errors from outermost to innermost. Under this proposal, the chain becomes a tree. However, there is no way for users to reconstruct the tree structure; union.Unwrap presents the tree as a linear list.

The motivating examples are of APIs which wrap an underlying error such as *pq.Error while adding their own condition to it. I don’t understand why these examples wrap the underlying errors. For example, the proposal says of one example: “Nobody outside of SetUserName needs to know anything about postgres driver errors.” In this case, I think the underlying *pq.Error should not be part of the error chain, to prevent callers from being exposed to it.

I do think there is space for a function which associates a new sentinel error with an error, to make it simpler to provide distinguishable error conditions without the need to write new error types. Perhaps something along these lines:

// Add returns an error that wraps err and matches sentinel.
// Unwrap(Add(err, sentinel)) returns err.
// Is(Add(err, sentinel)) returns true.
func Add(err, sentinel error) error {
  return withSentinel{err, sentinel}
}

type withSentinel {
  err, sentinel error
}

func (e withSentinel) Error() string { return e.err.Error() }
func (e withSentinel) Unwrap() error { return e.err }
func (e withSentinel) Is(target error) bool { return e.sentinel == target }

(I’m not sold on the name Add, it’s just the first thing that came to mind that isn’t With.)

Add retains a linear chain of wrapped errors, does not introduce additional allocations to Is/As, and leaves the complexity of Is/As as O(N) where N is the depth of the chain.

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.

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.

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 errors.Is/errors.As and have dragged their ugly old APIs around, or are doing something else that’s much uglier than the examples in this proposal.

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.

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:

• 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)

except s/With/Mark/.

We declined Mark back in February saying:

As Ian said, it seems like it would make sense to implement this outside the standard library and gauge how useful it is.

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.

context.Canceled and context.DeadlineExceeded are two sentinel errors in the standard library that I don’t remember seeing mentioned in the discussion yet. That is making me think about https://github.com/golang/go/issues/51365, and what might have been or maybe could still be improved with the debuggability of context cancellation. 🤔

This is exactly like writing append(append(slice1, value2), append(slice3, value4)...) or even append(append(slice1, slice2...), append(slice3, slice4...)...)

Well, except it’s prepend, not append. But the point is the structure of calls is the same, but both end up with a list as the output.

Hmmm, OK, but I don’t see that in the proposal. append works that way because it is building slice of elements. You can’t have a slice inside a slice. If you want errors.With to build a linear chain of errors, then it seems to me that you need to say that errors.With looks at one of the arguments, checks to see whether it is the result of a earlier call to errors.With, and, if so, unpacks it to recreate a linear chain of errors. If you don’t do that, then I don’t understand how you can wind up with anything other than a tree of errors.

Maybe one of the implementations in the proposal does work that way. I’m not sure. But it’s not in the proposal text.

Even if it did I’m concerned that we would be left with a case where it’s not obvious to the reader which error is the “real” error and which error is the “sentinel” error. A sentinel and an error are two different kinds of things.

Here, for example, is a package that does something like this proposal, but uses sentinel codes: https://pkg.go.dev/google.golang.org/grpc/status

Of course it’s likely that sentinel values are diverse enough that it doesn’t make sense to put them in a widely shared package, but that doesn’t seem like an argument for pretending that sentinels and errors are the same kind of thing.

FWIW At Khan Academy, we follow exactly the pattern @natefinch describes, where we wrap every third party error with an appropriate sentinel error that allows us to use errors.Is(err, errors.NotFoundKind) as an abstract concept, rather than an implementation detail specific errors.Is(err, pgconn.PgError) that would need to be interrogated far from those details:

  if errors.As(err, &pgErr) {
    fmt.Println(pgErr.Message) // => case_not_found
    fmt.Println(pgErr.Code) // => 20000
  }

I have also seen this pattern in the wild by many of the consumers of cockroachdb/errors and the other libraries that were mentioned above. Very few of the many, many bespoke implementations of Wrap() are entirely correct, (h/t @knz ).

If we cannot get support for adding this functionality to the stdlib outright, what about adding it to x/exp/?

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.

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!

you can write a complicated error wrapping struct that contains the metadata you want to check for, and behaves correctly for errors.Is, .As, and .Unwrap to allow callers to access the underlying cause of the error.

FWIW you don’t have to do anything special for Is or As. You just need to implement Unwrap. For example:

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.

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?

// Wrap wraps err with newErr, joining their error messages.
// errors.Wrap(err, newErr) // produces newErr.Error() + ": " + err.Error()

// With returns an error that produces the same error message as err, 
// but uses both errors for comparisons with Is and As.
// errors.With(err, newErr) // produces err.Error()

// Mask returns an error that replaces err's error message with newErr's,
// but uses both errors for comparisons with Is and As.
// errors.Mask(err, newErr) // produces newErr.Error()

Only the result of .Error() changes. The behavior for all is identical to the original With proposal, as above.

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

return errors.With(errors.Silent(err), ErrNotFound) // silences the underlying error message

That’s a bit wordy, but is a bit more explicit at the callsite than Wrap/With/Mask.

I think we are agreed that this is all about sentinels, such as io.EOF or fs.ErrNotExist aka os.ErrNotExist. And of course different applications will have their own sentinels as well.

A sentinel isn’t a general error, but having a sentinel implement error is convenient for using errors.Is and errors.As.

But one thing that a regular error can have but that a sentinel will not have is an Unwrap method. So I wonder whether errors.With should panic if the sentinel argument has an Unwrap method. That would be a step toward ensuring that the call is used correctly. Or, we could perhaps implement this as a vet check.

(I presume you also want errors.As(err, &is) == true)

This doesn’t really make sense when is is an error value (as is the case with sentinels). And presumably, if it’s an error type, whoever created the type can decide how Unwrap, As and Is might work.

I like your proposal, but I find it most useful (and less questionable), when used with sentinels to tag errors. And sentinels are still how I mostly do errors.

I use sentinels in a couple of different ways:

var ErrUnsupported = errors.New("pkg: unsupported option")

// Return directly
return ErrUnsupported

// Add some context
return fmt.Errorf("%w: size < 0", ErrUnsupported)

// Wrap an error, making it opaque (had never crossed my mind, but maybe I'll use it)
fmt.Errorf("%w: %v", ErrUnsupported, err)

// Then I can test the sentinel
if errors.Is(err, ErrUnsupported) {}

Your proposal adds a very interesting new way of using sentinels:

// Wrap sentinel and error
return errors.With(err, ErrUnsupported)

// Then I can test the sentinel
if errors.Is(err, ErrUnsupported) {}

// But I can also test the wrapped error
var eerr *exec.ExitError
if errors.As(err, &eerr) {}
if errors.Is(err, fs.ErrNotFound) {}

I’d find this very useful. So much so that I’ll definitely use something along this lines, even if doesn’t get approved.

But, and I don’t want to needlessly constrain your proposal, for sentinels, you don’t really need to deal with recursive wrapping, unwrapping, tree traversal, etc, because for sentinels all you really want is errors.Is to work on the sentinel.

And I wish we had better support sentinels, because no matter how hard people try to kill them, the standard library is full of them, and they popup all the time.

I really don’t like reversing the arguments. It only makes sense if someone’s calling the function embedded in itself multiple times in the same line… which I think is going to be really rare.

I like having errors.With mimic the order of arguments in errors.Is and errors.As. errors.With(err, ErrNotFound) errors.Is(err, ErrNotFound)

Any method that would otherwise add data to an error would clearly have the original error first and the added data second. Why should this be different?

Cockroach’s Mark works that way. The flags package I wrote about here (and implemented at Mattel) worked that way. Juju’s Wrap works that way.

(Notably, Hashicorp’s Wrap does not work that way)

I know that fmt.Errorf works kind of the opposite, but that’s really an artifact of fmt being a string formatter that takes arguments to plug into a format string.

I think most people would write a general function that “adds some textual context to this error” as pkg.AddText(error, string) … and indeed, that’s how github.com/pkg/errors’s Wrap, cockroach’s WithDetails, and juju’s Annotate all work.

Most methods in Go follow a right to left assignment, just like a = b means assign b to a. That’s why io.Copy is Copy(dst, src), that’s why channel send and receive both point left in the direction of data flow. That’s why append is append(original, added). That’s why all the context functions are WithSomething(ctx, something).

FWIW, github.com/natefinch/wrap exists to be played around with in the playground. I tried to show a sorta real-world example here: https://go.dev/play/p/9Yr-3ojqwiK. (note that there it is called wrap.With).

As soon as I wrote errors.Is(err, ErrNotFound) after having just written wrap.With(err, ErrNotFound), I was really sold on that order. I am sure that having the order reversed would absolutely trip people up all the time after writing errors.Is(err, something) a million and one times, they’d use the same order for With, and then it would be backwards.

@natefinch wrote:

With, as written, is essentially errors.Prepend(orig, err2), resulting in a logical list of errors like [err2, orig].

I re-implemented this as a stack which makes this behavior a bit more obvious. (I’m not suggesting to use this implementation, union may have better performance)

fmt.Errorf(“%w: %w”, err1, err2)

Yeah, if it returned a structure that would unwrap to err1 and then err2, I suppose that would be ok. Though I really don’t like that error wrapping is currently hidden away in the fmt package.

For better or worse, we already have fmt.Errorf("%w"). Removing a restriction on an existing feature is a smaller API change and doesn’t create a second way to wrap errors we would need to choose between.

It would also allow more flexibility in how the error message gets composed. Here is a relevant experience report.

A couple years ago I wrote a custom error type for a private package at $work that had the same usage pattern as this proposal, specifically we wanted to export a sentinel error from the package for use with errors.Is, but we also wanted to have the details of lower level errors in our logs. One difference was we had an additional piece of context we wanted in the error messages beyond the sentinel error and the lower level error. We also had a message parsing context we cared about.

The package in question was for an RTSP client/server package. RTSP is interesting because both client and server may send requests on the connection and must be prepared to handle requests and send responses. We wanted the error messages returned from the message parsing layer to indicate whether a failure happened in a request or a response, but we didn’t need separate sentinels for each case.

If fmt.Errorf supported multiple %w verbs we could have written:

var ErrBadMessage = errors.New("bad RTSP")

func parseMessage(...) (Message, error) { // Message is an interface implemented by both *Request and *Response
    msgData, err := readMessageHeaders(...) // might return network level errors
    if err != nil {
        return nil, fmt.Errorf("%w message: %w", ErrBadMessage, err)
    }
    // ...
    if request {
        return parseRequest(msgData)
    } else if response {
        return parseResponse(msgData)
    } else {
        return nil, fmt.Errorf("%w message: unrecognized message type", ErrBadMessage)
    }
}

func parseRequest(msg msgData) (*Request, error) {
    // ... 
    body, err := readBody() // might return network level errors
    if err != nil {
        return nil, fmt.Errorf("%w request: %w", ErrBadMessage, err)
    }
}

func parseResponse(msg msgData) (*Response, error) {
    // ...
    body, err := readBody() // might return network level errors
    if err != nil {
        return nil, fmt.Errorf("%w response: %w", ErrBadMessage, err)
    }
}

If we had the version of errors.With that included both errors in its error message then instead of a single call to fmt.Errorf like this;

return nil, fmt.Errorf("%w response: %w", ErrBadMessage, err)

we would need to use both errors.With and fmt.Errorf as follows.

return nil, errors.With(fmt.Errorf("%w response", ErrBadMessage), err)

BUT … for our real RTSP package we decided NOT to wrap the lower level errors in order not to leak those details in the package API and end up with some code depending on them. So the code we really wrote was equivalent to replacing the second %w in all of the above cases with a %v and we didn’t have a need for errors.With or support for more than one %w in fmt.Errorf.

I don’t know if this story supports this proposal or not, so that’s why I called it an experience report.

Speculating … If errors.With was available when we wrote that code two years ago we may well have used it because it was convenient. Arguably, that would have resulted in a leakier abstraction. I wonder what people think about that aspect.

If the point was simply to return a sentinel error with extra details in its message then fmt.Errorf("%w: %v", sentinelErr, detailErr) is sufficient and no new feature is needed. So it seems like the ability to have errors.Is match BOTH the sentinel and the detail errors is the point, but is that good design practice?

In your experience @natefinch, or others:

• How often does the code inspecting the errors need to programmatically inspect the detail errors after you’ve wrapped them with a sentinel?
• If it happens often, does that raise questions about why the sentinel values are not pulling enough weight?
• Do you often wrap errors with multiple sentinels and that’s when a “union” error type becomes necessary?

FWIW, the more I think about it, the more I think it would be better to have the sentinel error’s message prepended to the base error message. It would likely be surprising otherwise.

Something simple like

func (u union) Error() string {
    return u.top.Error() + ": " + u.base.Error()
}

If you don’t want the sentinel to print out, it’s trivial to write a wrapper to squelch its output.

If you go that route maybe it is better written like this:

fmt.Errorf("%w: %w", err1, err2)

Of course that’s not allowed now, but if that’s the feature you want, maybe the proposal becomes to allow that and to extend the implementation of the non-exported fmt.wrapError to handle more than one error.

@ianlancetaylor The reason these sentinels are errors is because you can check them with the existing Is/As. You don’t have to differentiate between sentinels on errors and “normal” errors. They’re all just errors. Nobody needs to care if os.ErrNotExist is a sentinel added to a lower level error, or just a standalone error. Why have two ways to check basically the same thing?

Oh, and this also then lets you use any existing error as a sentinel on any other error, without either one needing any special code.

FWIW, I wrote a library exactly like your Sentinel code there. It’s what I wrote about in https://npf.io/2021/04/errorflags/ … and it works fine. BUt it means you have to know when to use sentinel.IsCode() vs. errors.Is. I like that this function lets you just always use errors.Is.

I guess that what I’m stuck on is that I don’t understand why the sentinel should have type error. Will we ever see the sentinel all by itself?

That said, admittedly os.ErrNotExist is returned by itself and is also used as a sentinel. Calling errors.Is(syscall.ENOENT, os.ErrNotExist) will return true.

But for general purpose sentinel values we don’t need errors.

type SentinelCode int

const (
    None SentinelCode = iota
    NotFound
    Conflict
)

type SentinelError struct {
    code SentinelCode
    err  error
}

func (se *SentinelError) Error() string {
    return se.err.Error()
}

func (se *SentinelError) Unwrap() error {
    return se.err
}

func Code(err error) SentinelCode {
    for err != nil {
        if se, ok := err(*SentinelError); ok {
            return se.code
        }
        err = errors.Unwrap(err)
    }
    return None
}

func IsCode(err error, code SentinelCode) bool {
    return Code(err) == code
}

@neild I think Is would have to be implemented like this, or you could run into weird edge cases.

func (e withSentinel) Is(target error) bool {
    return errors.Is(e.sentinel, target)
}

I do still find this general approach confusing though. What the client will see in their logs is the original error, so it won’t be obvious they should be using some completely unrelated sentinel error in their checks.

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! 😃

which was the case in #50189 as well

That’s a typo for #50819.

I think the post-mortem activity on https://github.com/golang/go/issues/47811 is interesting, and I think the best bet for getting something like errors.With/Mark into the standard library is to solve the multierror problem at the same time.

I was initially glad to see this proposal scaled back to its current form, because having a tree of errors makes Unwrap difficult to implement cheaply, as @neild observed.

But now I worry that we’re leaving out the multiple-error use case. That’s clearly also important, as @tux21b pointed out.

I think it would be more general to support multiple errors. Basically,

errors.Multi(err1, err2, ...)

would return an error that is Is and As each of the argument errors, in the order they appear. There is no Unwrap, but it is possible to recover the argument errors by calling Errors() []error. The error message is a concatenation of all the argument messages, with some separator.

This is Tailscale’s design. The implementation is straightforward.

Except for the error message, With is then just Multi with reversed arguments and without Unwrap. I think that’s an improvement, because the consensus seems to be that almost no one uses Unwrap directly anyway, and having Unwrap return only the first argument of Wrap seems like a pitfall to me.

The current proposal doesn’t specify the implementation of Error (at least not in the initial comment; maybe it was added later?). But I gathered from the discussion that concatenating the messages is probably not what you want.

So Multi has the wrong argument order and the wrong error message for With, but you could now easily write With in terms of Multi and a type that implemented Error however you wanted. But you couldn’t write Multi in terms of With.

But one thing that a regular error can have but that a sentinel will not have is an Unwrap method. So I wonder whether errors.With should panic if the sentinel argument has an Unwrap method. That would be a step toward ensuring that the call is used correctly. Or, we could perhaps implement this as a vet check.

That is an interesting suggestion. If I understand correctly, that would suppress the feeling that errors.With could build error trees since one of its two arguments must not wrap anything.

Finally, there was evidence presented here that was not presented in the previous proposal that many developers have written similar functionality, yet often times in ways that are not interoperable. Imagine if everyone was developing their own slightly-different versions of io.Reader and io.Writer, and how that would have impacted the Go ecosystem.

My understanding of why the stack traces from xerrors didn’t make it into Go 1.13 with errors.Is/As was that there was too many subtle details around situations where you did want a trace but didn’t want the error unwrapping chain and vice versa. (cc @mpvl) Introducing errors.With would create similar subtle problems around what parts of the error tree are or are not included. This thread has seen multiple different implementations mooted and you can poke around OSS and find other ways of doing things. I personally am fond of the idea of just overloading the Is() method to add the one sentinel, but I can see why it would be confusing that a sentinel error could be found by errors.Is but not errors.Unwrap.

Unlike everyone creating their own io.Reader/Writer variants, in this case, the error sentinels are application specific, so I don’t think there needs to be an ecosystem wide standard. Going from many people using pkg/errors to having a standard errors.As/Is was good because it meant that the standard library itself could use error wrapping. In this case though, I don’t think the standard library would end up marking sentinels, because the idea of errors.With is you’re converting from one system to another. You have a pgx.ErrNoRows and then you transform it into a domain.NotFound error, for example. The standard library is the core system, so it doesn’t need to translate its own errors into a second domain.

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

How does this proposal differ from #50819? It looks the same to me.

I think if this were the case then the proposal would close. We can already do fmt.Errorf(“%w: %v”, err2, err1). Don’t need a convenience function for it.

Yes.

What is x? If it’s err1 or err2 how do to get the other one?

These are precisely the questions raised by the proposal for errors.With. If we have good answers for them there, then the same answers would presumably apply to multiple %w verbs.

The tricky part isn’t flattening the tree of With(a, With(b, c)). The tricky part is flattening With(a, SomeWrapper(With(b, c))). I don’t see how you keep SomeWrapper while flattening the inner With.

@ianlancetaylor thanks for taking the time to clarify for me, this helps me understand your concerns.

If I write errors.With(errors.With(err1, err2), errors.With(err3, err4)), then it seems to me that I have a tree of errors.

This is exactly like writing append(append(slice1, value2), append(slice3, value4)...) or even append(append(slice1, slice2...), append(slice3, slice4...)...)

Well, except it’s prepend, not append. But the point is the structure of calls is the same, but both end up with a list as the output.

Unwrap, by definition, ensures that we have a linear list of errors. Imagine it’s called Next() instead of Unwrap(), and it just returns the next error in the list of errors. It’s the same contract.

If I write errors.With(errors.With(MyError(“a”), io.EOF), errors.With(MyError(“b”), io.EOF)) and then pass that to var me MyError; errors.As(err, &me) it seems ambiguous whether I get MyError(“a”) or MyError(“b”).

In practice, I think it will be very uncommon to use a wrapped error as the second argument to the function. Based on how similar functionality has been used, I would guess that 99.9% of the time you’ll see return errors.With(err, SomeSentinel) or return errors.With(err, applicationMetadata) where the second error is just a sentinel created with errors.New() or an application-specific structure that implements the error interface.

err := errors.With(errors.With(MyError("a"), io.EOF), errors.With(MyError("b"), io.EOF))
var me MyError
if errors.As(err, &me) {
   fmt.Println(me)
}

The result of the above code is totally deterministic, and will always be “b”.

With, as written, is essentially errors.Prepend(orig, err2), resulting in a logical list of errors like [err2, orig].

In theory we could implement With as append instead, but I think that optimizes for the 0.1% case to the detriment of the 99.9% case.

If With was implemented as an append, then the above list of errors could be read left to right as ["a", io.EOF, "b", io.EOF].

However, that makes the common case harder to read, IMO. Right now, you’ll see a lot of code like this:

err := doSomething()
if errors.Is(err, fs.IsNotExist) {
    return errors.With(err, ErrNotFound)
}
if err != nil {
   return err
}

vs. if it’s append:

err := doSomething()
if errors.Is(err, fs.IsNotExist) {
    return errors.With(ErrNotFound, err)
}
if err != nil {
   return err
}

In the latter, the data you’re adding is kind of buried in the middle of the line, whereas the former it’s easier to see because it’s always right at the end of the line.

The current (prepend) order of arguments also follows the convention of context.WithValue, the various logger.WithValue methods, and even append, that take the original thing first and the added thing second. I worry that if we make it errors.With(add, orig) that many people will make the mistake of writing it the opposite way, just from being used to the many other functions that follow the other order.

The underlying issue here is that we need a way to provide context to errors. People have come with different solutions:

• returning multiple errors
• errors with sentinel errors or wrapped errors
• error types
• specific interfaces
• errors with flags

All of them try to answer the same question “What is this error about?” for some code that needs to act depending on the answer.

The stdlib should provide a solution, preferably one that fits most of the uses cases with the least added complexity.

Fwiw, in cockroachdb/errors we are also fine with semantics that attach an ancillary error to compare true on errors.Is but can’t be retrieved with errors.As

– Verstuurd vanaf mijn Android apparaat met K-9 Mail. Excuseer mijn beknoptheid.

It’s not ambiguous if we specify the traversal is depth first.

You could ask the same question of &myErr{cause:&myErr{cause:someErr}}

There are two candidates for errors.As and yet everyone is fine with the semantics.

Ian Lance Taylor @.***> schreef op 2 mei 2022 06:25:09 CEST:

If I write errors.With(errors.With(err1, err2), errors.With(err3, err4)), then it seems to me that I have a tree of errors.

If I write errors.With(errors.With(MyError{"a"), io.EOF), errors.With(MyError("b"), io.EOF)) and then pass that to var me MyError; errors.As(err, &me) it seems ambiguous whether I get MyError("a") or MyError("b").

– Reply to this email directly or view it on GitHub: https://github.com/golang/go/issues/52607#issuecomment-1114484157 You are receiving this because you were mentioned.

Message ID: @.***> – Verstuurd vanaf mijn Android apparaat met K-9 Mail. Excuseer mijn beknoptheid.

I prefer the idea of adding support for multiple %w verbs in fmt.Errorf to adding errors.With. Allowing multiple %w verbs feels more like lifting a restriction on the current API than adding a new thing, and unlike errors.With, it’s not something you could do in a third party package. There are already plenty of third party multierror and error tree libraries, so the benefit from adding errors.With seems sort of low.

Instead of a single chain of errors wrapping other errors, we have a tree of errors.

I’m not sure why people keep saying this. It’s not a tree. It’s a list, it’s just the underlying data structure looks like a tree, but it’s always traversed depth-first, so it has a defined order, so it’s a list.

errors.Is and errors.As become Ambiguous

I don’t understand that statement. Many people already write wrapping errors by hand, such that one error value can return true for more than one Is or As call. What’s different about this one?

fmt.Errorf(“%w: %w”, err1, err2)

Yeah, if it returned a structure that would unwrap to err1 and then err2, I suppose that would be ok. Though I really don’t like that error wrapping is currently hidden away in the fmt package.

Wrap might be a better name 🤷🏻‍♂️. I’m not beholden to the name (or the underlying implementation).

Why have two ways to check basically the same thing?

Because if we join two errors, then errors.Is and errors.As become ambiguous. Instead of a single chain of errors wrapping other errors, we have a tree of errors.

FWIW, the more I think about it, the more I think it would be better to have the sentinel error’s message prepended to the base error message. It would likely be surprising otherwise.

Something simple like

func (u union) Error() string {
    return u.top.Error() + ": " + u.base.Error()
}

If you don’t want the sentinel to print out, it’s trivial to write a wrapper to squelch its output.

More discussion here, if anyone’s bored this weekend 😃 https://www.reddit.com/r/golang/comments/ue73jz/proposal_errorswitherr_other_error_error/

At the moment, we talk about the “error chain”–an ordered list of errors from outermost to innermost. Under this proposal, the chain becomes a tree.

It’s still a chain with this implementation. Top’s chain gets added in front of all of the base error. That’s all.