Skip to content
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 With(err, other error) error #52607

Closed
natefinch opened this issue Apr 28, 2022 · 119 comments
Closed

proposal: errors: add With(err, other error) error #52607

natefinch opened this issue Apr 28, 2022 · 119 comments
Labels
error-handling Language & library change proposals that are about error handling. FrozenDueToAge Proposal
Milestone

Comments

@natefinch
Copy link
Contributor

natefinch commented Apr 28, 2022

Background

Right now, the only way to wrap an error in the standard library is with fmt.Errorf which 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

  1. you can return some other error, which loses the original error's context
  2. you can wrap the error with fmt.Errorf, which just adds textual output, and therefore isn't something callers can programmatically check for
  3. 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.

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.

// With returns an error that adds flag to the list of errors that can be
// discovered by errors.Is and errors.As.
func With(err, flag error) error {
	if err == nil {
		return nil
	}
	if flag == nil {
		return err
	}

	return flagged{error: err, flag: flag}
}

type flagged struct {
	flag error
	error
}

func (f flagged) Is(target error) bool {
	return errors.Is(f.flag, target)
}

func (f flagged) As(target interface{}) bool {
	return errors.As(f.flag, target)
}

func (f flagged) Unwrap() error {
	return f.error
}

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 With function, 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.

// SetUserName sets the name of the user with the given id. This method returns 
// flags.NotFound if the user isn't found or flags.Conflict if a user with that
// name already exists. 
func (st *Storage) SetUserName(id uuid.UUID, name string) error {
    err := st.db.SetUser(id, "name="+name)
    if errors.Is(err, pq.ErrNoRows) {
       return nil, errors.With(err, flags.NotFound)
    }
    var pqErr *pq.Error
    if errors.As(err, &pqErr) && pqErr.Constraint == "unique_user_name" {
        return errors.With(err, flags.Conflict)
    }
    if err != nil {
       // some other unknown error
       return fmt.Errorf("error setting name on user with id %v: %w", err) 
    }
    return nil
}

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:

func (h *Handlers) HandleSetName(w http.ResponseWriter, r *http.Request) {
    name, id := getNameAndID(r)
    err := h.storage.SetUserName(id, name)
    if err != nil {
        handleError(err, w)
        return
    }
    // other stuff
}

func handleError(err error, w http.ResponseWriter) {
    switch {
    case errors.Is(err, flags.NotFound):
        http.Error(w, 404, "not found")
    case errors.Is(err, flags.Conflict):
        http.Error(w, 409, "conflict")
    default:
        // other, uncategorized error
        http.Error(w, 500, "internal server error")
        // probably log it, too
    }
}

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:

    var pqErr *pq.Error
    if errors.As(err, &pqErr) && pqErr.Constraint == "unique_user_name" {
        http.Error(w, 409, "conflict")
        return
    }

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.

@gopherbot gopherbot added this to the Proposal milestone Apr 28, 2022
@ianlancetaylor
Copy link
Contributor

This has some similarities to #48831.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Apr 28, 2022
@ianlancetaylor ianlancetaylor added the error-handling Language & library change proposals that are about error handling. label Apr 28, 2022
@seankhliao
Copy link
Member

Note we've declined multiple errors in #47811 and error marks (which looks like the same as this one?) #50819

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

@natefinch
Copy link
Contributor Author

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

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.

@tv42
Copy link

tv42 commented Apr 29, 2022

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.

@rittneje
Copy link

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.

@natefinch
Copy link
Contributor Author

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.

@hampgoodwin
Copy link

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!

@ChrisHines
Copy link
Contributor

ChrisHines commented Apr 29, 2022

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

@natefinch
Copy link
Contributor Author

existing packages providing some form of composite errors. Are we seeing convergence on a common approach across those implementations?

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

What is the rationale for Unwrap and As to behave "differently" here?

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 u.other and then start unwrapping u.error".

For Is and As, I decided to check the other error first, because it is newest in the stack and also most likely to just be a single sentinel error, avoiding the need to repeatedly unwrap. So it's both logically the best choice and likely less work than unwrapping the base error, which may well be several layers of wrapped errors.

For Unwrap, I had to choose something, and the idea is that u.error is more representative of the true underlying cause than the added context in u.other.

I also wonder how weird this might get with multiple layers of With wrappings.

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.

@darkfeline
Copy link
Contributor

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.

@jimmyfrasche
Copy link
Member

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

@natefinch
Copy link
Contributor Author

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
}

@rittneje
Copy link

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

@natefinch
Copy link
Contributor Author

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

@natefinch
Copy link
Contributor Author

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! :)

@bokwoon95
Copy link

bokwoon95 commented Apr 29, 2022

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.

@ncruces
Copy link
Contributor

ncruces commented Apr 29, 2022

I'm a big fan of sentinel errors (particularly const sentinel errors, I recreate(d) type constError string in almost every package). But I feel that we had lost that battle in favour or error types long ago.

I like it, but it sends a mixed message, IMO.

@zimmski
Copy link
Contributor

zimmski commented Apr 29, 2022

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.

@natefinch
Copy link
Contributor Author

natefinch commented Apr 29, 2022

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

@bokwoon95
Copy link

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.

@natefinch
Copy link
Contributor Author

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:

https://go.dev/play/p/K99JW49Js-G

@earthboundkid
Copy link
Contributor

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:

-- errorflags/constants.go --
const (
    NotFound Flags = iota
    NotValid
    // ...
)

// Or if the flags are non-exclusive, they could be bitfields
const (
    NotFound Flags = 1 << iota
    NotValid
)

-- config/config.go --
func (l *Loader) LoadConfig(name string) (*Config, error) {
   b, err := os.ReadFile(filepath.Join(l.configDir, name))
   if os.IsNotExist(err) {
       return nil, errorflags.With(err, errorflags.NotFound)
   }
   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, errorflags.With(err, errorflags.NotValid)
   }
   return cfg, nil
}

-- cmd/app/main.go --
cfg, err := l.LoadConfig(name)
if errorflags.Has(err, errorflags.NotFound) {
    // handle missing config
}
if errorflags.Has(err, errorflags.NotValid) {
    // handle invalid config
}
if err != nil {
    // handle unknown error
}

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 errorflags.UnknownError or some such).

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.

@tux21b
Copy link
Contributor

tux21b commented Apr 29, 2022

Error handling usually has to satisfy three parties:

  • The devlopers / devops need detailed errors, usually created by wrapping errors in every function by calling errors.Errorf. Go is doing fine here.
  • The application needs a small set of error codes that can be interpreted by the application itself. Those error codes are usually something like NotFound, InvalidArgument, AccessDenied, etc. (HTTP and GRPC provide a similar list)
  • The end user needs translated, easy to read error messages which tell them what to do. The error message must not contain any wrapped error messages intended for the developer (those messages are confusing and might leak internal information - e.g. to avoid leaking the existence of a email address if the password was incorrect).

The reason why I dislike this proposal is that it only deals with the first two. A more general approach like errors.Multi(err ...error) error implemented by tailscale and the many multierr packages listed above solves the same problem and allows annotating the error with the error code for the application and with an error message for the user.

The existence of all those multierr packages is probably a sign that something should be added to the standard library.

@ianlancetaylor
Copy link
Contributor

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]

@earthboundkid
Copy link
Contributor

A minor suggestion, instead of exposing the ErrorWrapper and WrappedError types, you could take a callback errors.WrapOf[T any](value T, stringer func(T, error) string) error and then have errors.UnwrapperOf[T any](error) (T, bool).

@natefinch
Copy link
Contributor Author

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

@timothy-king
Copy link
Contributor

Minor quibble. The if err == nil { return nil } case in With means

errors.Is(With(err, flag), target) is the same as calling errors.Is(flag, target) || errors.Is(err, target)

may not hold when err == nil. The following prints "false true" (https://go.dev/play/p/vunjYYFzpRA):

type MyErr string

func (e MyErr) Error() string { return string(e) }

func main() {
	err, flag, target := error(nil), MyErr("lorem"), MyErr("lorem")

	fmt.Println(errors.Is(With(err, flag), target), errors.Is(flag, target) || errors.Is(err, target))
}

Ditto with As.

@natefinch
Copy link
Contributor Author

Yeah, I'm not strongly attached to that functionality either way.

@jba
Copy link
Contributor

jba commented May 6, 2022

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.

@neild
Copy link
Contributor

neild commented May 6, 2022

I think a useful principle of the current errors package design is that the error text is always under the full control of the user. So for multiple errors, perhaps instead something like this:

err1 := errors.New("err1")
err2 := errors.New("err2")
err := errors.Join(": ", err1, err2)

fmt.Println(err.Error())          // "err1: err2"
fmt.Println(errors.Is(err, err1)) // true
fmt.Println(errors.Is(err, err2)) // true
fmt.Println(errors.Unwrap(err))   // nil
errs := errors.Split(err)         // []error{err1, err2}

Split would look for an Errors() []error method.

@timothy-king
Copy link
Contributor

@jba We can clarify Unwrap with Multi internally by representing this as a struct{wrapped error; tag []error} (and if we want to support arbitrary messages a we can also add a string field too). I think the trick is that the api to create one errors.Multi(err1, err2, ...) does not make it clear that some element is special. Maybe a different function name would be clear? Wrap(text string, wrapped err, tags ...error)? Seems slightly heavy weight to use that though.

@timothy-king
Copy link
Contributor

@neild Would Split guarantee an order if it looked for Errors() methods? If so what order? Left-right leaves of the tree? Left-right BFS?

@earthboundkid
Copy link
Contributor

Split would look for an Errors() []error method.

My multierrors package has AsSlice which turns an error into []error based on whether it finds a Multierror in the error it's looking at. (And nil of course is a slice of length 0.) The downside of this is if you have Wrapper(Multierror(err1, err2)) then AsSlice loses Wrapper. I'm just willing to live with it for my purposes, but it was one of the considerations against #47811.

@jba
Copy link
Contributor

jba commented May 6, 2022

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.

@natefinch
Copy link
Contributor Author

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.

@justenwalker
Copy link

justenwalker commented May 11, 2022

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?

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.
That said, I fear if we can't reach consensus on one function, it will be unlikely we can reach one on three.

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 union could implement:

// 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 Is and As logic is taken care of by errors.Join -- which is one of the main reasons for the proposal.

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 %w verbs, but I think doing that confuses the idea of "Wrapping" an error which is what the %w verb represents. ie: WrapperError{ Err: err} where we are just adding some additional string detail or context literally around the err with a trivial linear unwrap chain; Whereas two verbs are actually unioning/joining them together into one error such that it is both err1 AND err2.

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?

@natefinch
Copy link
Contributor Author

natefinch commented May 11, 2022

Also my problem with fmt.Errorf with more than one %w is that the order in which the errors are listed in the args defines the wrapping order, but also corresponds to the order in which the text output is created (by default).

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 new: original... then you'd have to write fmt.Errorf("%[1]w: %[0]w", err, newFlag) ... which is not something most people even realize is possible (and even I had to look up the exact syntax... and I'm still not even sure if the indices are 0 or 1 based).

And also the same thing with the %.0w that I'd never even thought about.

I much prefer a discoverable syntax, not making our main API for error wrapping stringly typed.

@rsc rsc changed the title proposal: errors: Add With(err, other error) error proposal: errors: add With(err, other error) error May 11, 2022
@narqo
Copy link
Contributor

narqo commented May 12, 2022

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 Storage.SetUserName() method won't be able to test the returned error against pq.ErrNoRows, w/o unwrapping it explicitly, right? That feels ok for a case, where the number of layers in the error propagation path is low (http → storage → db). But that makes it confusing, when there are more intermediate layers, who propagate/wrap the error:

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
}

@earthboundkid
Copy link
Contributor

There have been a number of proposals, so it's hard to keep track of which ones do what, but most of them keep errors.Is(err, pq.ErrNoRows) == true.

@rsc
Copy link
Contributor

rsc commented May 25, 2022

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.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) May 25, 2022
@rsc
Copy link
Contributor

rsc commented May 25, 2022

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

@natefinch
Copy link
Contributor Author

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.

@ncruces
Copy link
Contributor

ncruces commented May 28, 2022

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? golang.org/x/exp?

Without something that says: “the Go team is considering this,” every big team will just continue to roll their own.

@rsc
Copy link
Contributor

rsc commented Jun 1, 2022

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.

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jun 1, 2022
@rsc
Copy link
Contributor

rsc commented Jun 1, 2022

No change in consensus, so declined.
— rsc for the proposal review group

@earthboundkid
Copy link
Contributor

which was the case in #50189 as well

That's a typo for #50819.

I think the post-mortem activity on #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.

@golang golang locked and limited conversation to collaborators Jun 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
error-handling Language & library change proposals that are about error handling. FrozenDueToAge Proposal
Projects
No open projects
Development

No branches or pull requests