Make Error an interface rather than a concrete type

[adrienkohlbecker]

In Go, error is an interface, so making errors.Error a concrete type broke code such as the following:

func goError() error {
    return nil
}

func myError() *errors.Error {
    return nil
}

func main() {

    err := goError()
    if err != nil {
        fmt.Println("goError not nil")
    }

    err = myError()
    if err != nil {
        fmt.Println("myError not nil")
    }

}

The code above prints "myError not nil", which can be troubling. This is due to the way interfaces work in go (see https://news.ycombinator.com/item?id=5196241 for reference).

Introducing an Error interface is a breaking change, but resolves this kind of mistake by unexperienced go developpers.

In this commit, I have:

• renamed Error to wrappedError (not exported)
• added a new Error interface with the existing methods
• Added an Underlying() method to replace the Err property that cannot be accessed on the interface

Fixes #3

[ConradIrwin]

I think we don't want to change this. Instead use the builtin error interface as return values from your functions, and have the error subsystem convert them to an errors.Error using the errors.New function if you need that (it's a bit subtle that errors.New can return the argument, suggestions for a better name or more explicit docs would be welcome):

func a() error {
  if true {
  return errors.New("Oops");
  } else {
  return nil;
  }
}

func main() {
  err := a();
  if err != nil {
    e := errors.New(err);
   // ...
  } 
}

This is consistent with how the go FAQ suggests dealing with the issue: https://golang.org/doc/faq#nil_error. (To be honest though, I'm not sure why they didn't define equality on nil to be that nils of all types are equal, I guess it's too late now)

[adrienkohlbecker]

We are using *errors.Error as a return value to ensure all our errors have stacktraces (that is, we can't return an error from the standard library, by design, we have to Wrap it) If we were to use error, we would have no way of knowing if what we're getting in the caller function has a stacktrace or not. We could of course Wrap it everywhere, but then the code is less readable for 80% of the codebase where we're using our own functions

I do understand your point of view though, thanks for taking the time to review the PR.

[ConradIrwin]

Hmm, I've only needed to do the explicit casting in logging functions, you may be able to simplify the code by moving some of the complexity into a log package.

[adrienkohlbecker]

Yes, that solves the displaying part, but if someone up the stack forget to Wrap an error from the standard library, we're left with an incomplete stacktrace (or none). That's what I'm trying to solve with the return values

[adrienkohlbecker]

It did not start that way, but after a few mishaps in production, we've adopted this method :)

[ConradIrwin]

Ok, that sounds sane. I guess you can easily add an interface to your code to enforce this:

type WrappedError interface {
   ErrorStack() string
}