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: support returning a wrapped error without nil checks #38193

Closed
arp242 opened this issue Apr 1, 2020 · 5 comments
Closed

proposal: errors: support returning a wrapped error without nil checks #38193

arp242 opened this issue Apr 1, 2020 · 5 comments
Labels
Projects
Milestone

Comments

@arp242
Copy link

@arp242 arp242 commented Apr 1, 2020

I use github.com/pkg/errors, and it allows me to do:

func f() error {
    return errors.Wrap(f2(), "f")
}

errors.Wrap() will return nil if the error is nil, so this works as expected both in cases of errors and no errors. I use this pattern quite a lot in my code, since it removes the need for many err != nil checks.

I looked in to using the new (well, new-ish) error wrapping from the stdlib last week, but it's quite a lot more verbose:

func f() error {
    err := f2()
    if err != nil {
        return fmt.Errorf("f: %w", err)
    }
    return nil
}

In my own code, there are many cases where I'll have to replace a return errors.Wrap(err, "..") with err != nil checks. It seems to me that the errors.Wrap() method is quite a bit more ergonomic.

I think it's a useful enough pattern to investigate if it's worth supporting something like this in the standard library.

I'm not especially attached to the Wrap() function call as such – it's just what currently being used by github.com/pkg/errors. Perhaps there is a better name for the function, or a better way to get the same effect. Either way, I think it might be worth investigate supporting returning errors without a nil check in some way.

@gopherbot gopherbot added this to the Proposal milestone Apr 1, 2020
@gopherbot gopherbot added the Proposal label Apr 1, 2020
@arp242
Copy link
Author

@arp242 arp242 commented Apr 1, 2020

For the time being, I just added this to my application by the way; but I'm not a huge fan of wrapping stdlib stuff as-such, and I miss it in various libraries where it's not available (and don't really want to add it to every library I write).

// Package errors adds Wrap() and Wrapf() to stdlib's errors.
//
// This removes the need for quite a few if err != nil checks.
package errors

import (
    "errors"
    "fmt"
)

func New(text string) error                 { return errors.New(text) }
func Unwrap(err error) error                { return errors.Unwrap(err) }
func Is(err, target error) bool             { return errors.Is(err, target) }
func As(err error, target interface{}) bool { return errors.As(err, target) }

// Wrap an error with fmt.Errorf(), returning nil if err is nil.
func Wrap(err error, s string) error {
    if err == nil {
        return nil
    }
    return fmt.Errorf(s+": %w", err)
}

// Wrapf an error with fmt.Errorf(), returning nil if err is nil.
func Wrapf(err error, format string, a ...interface{}) error {
    if err == nil {
        return nil
    }
    return fmt.Errorf(format+": %w", append(a, err)...)
}
@rsc rsc added this to Incoming in Proposals Apr 15, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Apr 15, 2020

It seems like this can be implemented very easily outside the standard library if that's the pattern you want (as you have). Personally, when I see an error constructor I expect it means there is a real (non-nil) error, not that it might be a success case sliding through.

We're taking a break from new error APIs for a while, to get a few years more experience with the recent changes.

@rsc rsc moved this from Incoming to Active in Proposals Apr 15, 2020
@rsc rsc changed the title Proposal: errors: support returning a wrapped error without nil checks proposal: errors: support returning a wrapped error without nil checks Apr 15, 2020
@arp242
Copy link
Author

@arp242 arp242 commented Apr 16, 2020

We're taking a break from new error APIs for a while, to get a few years more experience with the recent changes.

Yeah, that sounds reasonable 👍 It's just a UX/convenience issue that I've run in to; I don't know if it's just me. Probably should checked this out sooner when it was still x/errors, but also been busy 😅

@rsc
Copy link
Contributor

@rsc rsc commented Apr 22, 2020

Based on the discussion above, this sounds like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals Apr 22, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Apr 29, 2020

No change in consensus. Declined.

@rsc rsc closed this Apr 29, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.