Change Wrap() to return an error even if we provide nil

[silbinarywolf]

This issue has been raised elsewhere many times, the tldr is that we don't want to break BC See: https://github.com/pkg/errors/issues/90#issuecomment-336857602

The problem While working very tiredly yesterday, I hit an "impossible" issue where one of my functions was returning nil, nil. When I went to vet my code visually, it seemed all okay. None of my code paths returned nil, nil in a way that was obvious to me.

I ended up breaking out the debugger and just stepping through to see what was happening. That's when I realized what I'd done and then I also found, that Wrap didn't work as I expected.

example snippet, loosely based on what I did yesterday

func someFunction(ctx context.Context) (*Page, error) {
    // ... pretend there's more code here that created `nstmt` variable or anything else i've missed.
    var pageList []Page
    err := nstmt.SelectContext(ctx, &pageList, map[string]interface{}{
        "id": request.ID,
    })
    if err != nil {
        return nil, errors.Wrapf(err, "unable to retrieve records")
    }
    if len(pageList) == 0 {
        // Being sleep-deprived, I accidentally used Wrap instead of New.
        // This leads to `nil, nil` being returned.
        // This is my mistake.
        return nil, errors.Wrap(err, "expected at least one result")
    }
}

Proposed solution Consider making Wrap() create an error even if a nil error is passed in.

This is assuming that it doesn't break common usage patterns elsewhere. I don't know what the reasoning is behind making Wrap() return nil and I feel like there's just a use-case I'm missing here.

In the case that I'm not though, I figured raising this as an issue is a good idea.

[davecheney]

Thank you for raising this issue. Would you mind confirming if it is a duplicate of an exisiting issue. Thank you.

[silbinarywolf]

Ah I'm so sorry. I looked through open issues but not closed ones. I now see there's been discussion elsewhere here and multiple other places: https://github.com/pkg/errors/issues/228 https://github.com/pkg/errors/issues/90

Considering this problem keeps being raised, is it worth adding code comments so that it's clear that the behaviour won't change + intention is that you just fork it yourself?

[puellanivis]

I am not really sure what additional benefit would come from adding more code comments to the already well documented behavior. You clearly understood that this change could be a breaking change, and I think once people are aware of the behavior, they can also work out the implications of changing it.

I feel like there's just a use-case I'm missing here.

func f(data []byte, v interface{}) error {
return errors.Wrap(json.Unmarshal(data, v), "context to the possible error")
}

func g(data []byte) error { _, err := w.Write(data) return errors.Wrap(err, "context to the possible error") }

Additionally, there is a semantics argument: any wrapping of a non-error should be a non-error, right? That is, it doesn’t really make sense to turn a non-error into an error. A wrapped error with no cause is… weird.

In particular, your given code example, even without `errors.Wrap` was still buggy:

func someFunction(ctx context.Context) (*Page, error) { // ... pretend there's more code here that created nstmt variable or anything else i've missed. var pageList []Page err := nstmt.SelectContext(ctx, &pageList, map[string]interface{}{ "id": request.ID, }) if err != nil { return nil, err } if len(pageList) == 0 { // This is still a mistake, and one that I have seen in the wild from time to time. return nil, err } }