errors.Wrap() created bugs in our codebase.

[arianitu]

After switching to pkg/errors, lots of these bugs below were introduced. Can we make errors.Wrap() on a nil value actually return an error?

I know this is a user error, but surprisingly very common and we see pkg/errors as a risk to the health of our project and we most likely will need to fork/create our own implementation.

user := user.FindById(userId)
if err != nil {
  return errors.Wrap(err, "database error - failed to find user")
}

// !! BUG HERE !! errors.Wrap() on a nil value returns a nil error and thus
// the error is never returned.
// Instead you have to do errors.New() or errors.Errorf(). 
if user == nil {
  return errors.Wrap(err, "user not found")
}

[puellanivis]

This is a duplicate of https://github.com/pkg/errors/issues/174 and https://github.com/pkg/errors/issues/90 and it has repeatedly been declined.

While we now have a dependable package management system, this would still be a breaking change to a library that has largely been superseded by fmt.Errorf("… %w", err). The only feature of this package that still remains useful over standard libraries is the attachment of a stack trace.

Additionally, the proposal would silently break programs depending on the old behavior. So, even more argument against such a breaking change.

Dave Cheney’s original suggestion remains valid: “This package is released under a permissive open source licence, I suggest you fork it and modify it for your requirements.”

P.S.: I continue to assert that blindly calling errors.Wrap on your errors is already bad behavior as a) it attaches a stacktrace even if there already is one, and b) even if it returns a non-nil error if given a nil-error, this would only be masking a deeper fundamental structural issue of mishandling errors.

[arianitu]

@puellanivis thanks for the response, makes sense.

What is the correct way of using errors.Wrap then? We have an architecture where it could be placed in one place and everything else could use Errorf perhaps?

[puellanivis]

Most Highly Properly, you should try and scope all of your error values as narrowly as possible (this will have the best odds of not accidentally using a nil error), then errors.Wrap() only when the value coming from your call is known to not be from the errors package already. Then each other layer should use either the error without any wrapping, or errors.WithMessage(err, "…") or if using go1.13 or above, fmt.Errorf("… %w", err).

Example:

func f() error {
  if err := external.Func(); err != nil {
    return errors.Wrap(err, "message") // initial incoming error, so errors.Wrap
  }

  if condition {
    // I cannot accidentally use a `nil` error value here, because `err` is not even defined here.
    return errors.New("condition")
  }

  return nil
}

func g() error {
  if err := f(); err != nil {
    // all non-nil errors from `f` have a stack attached, avoid adding a new one
    return errors.WithMessage(err, "message")
  }

  return nil
}

I just looked and fmt.Errorf("… %w") will block the stack dump from the %+v directive from this package. 😢 You can still get the stack trace though using errors.As(…) into a var stacktracer interface{ Stacktrace() errors.StackTrace }.