Pointer receivers in Error() method of custom errors

[mariaefi29]

Dear @jszwec,

I have noticed that you use pointer receivers when implementing Error() method on custom errors.

For example, https://github.com/jszwec/csvutil/blob/17269445f80a0c9edb167631efc53ee30134e6ed/error.go#L146 and everywhere else.

Do you have any particular reason for that?

The reason I asked is that I cannot easily use methods such errors.As/Is with such implementations, because the compiler complains that this error type is not actually an error (it does not implement Error() method).

The complier warning looks like that: errors.As(err, csvutil.MissingColumnsError{})

Second argument to 'errors.As' must be a pointer to an interface or to a type implementing error

instead I have to do something like that (manually creating a pointer):

    var missingColumnError *csvutil.MissingColumnsError
    errors.As(err, &missingColumnError)

When I started doing research on the issue, I found this discussion on StackOverflow.

Standard library has methods with both approaches (with pointer receivers and values).

Do you have a strong reason that supports your approach in particular? I would be grateful if you could share)

[jszwec]

Hello @mariaefi29

Before Go1.13:

The usual way of checking errors looked something like this

myErr, ok := err.(*FooError)

if Error() was defined on a value receiver you would have to do two checks to be safe:

myErr, ok := err.(*FooError)

and

myErr, ok := err.(FooError)

So it was better to just implement it through a pointer receiver to avoid this problem.

After Go1.13:

The same problem, different tools.

Your errors.As example is correct and this is how errors.As is supposed to be used:

    var missingColumnError *csvutil.MissingColumnsError
    errors.As(err, &missingColumnError)

The complier warning looks like that: errors.As(err, csvutil.MissingColumnsError{}) Second argument to 'errors.As' must be a pointer to an interface or to a type implementing error

go vet is correct, the second argument to errors.As must be a pointer (even if Error() was implemented by a value receiver)

If error type was a val receiver you could do:

errors.As(err, &csvutil.MissingColumnsError{})

With Err being a ptr receiver you can do:

errors.As(err, new(*csvutil.MissingColumnsError))

or

if myErr := new(csvutil.MissingColumnsError); errors.As(err, &myErr) {
  // handle it
}

Now, notice that if error type was implemented via val receiver, but err was returned as a ptr, then your first check:

errors.As(err, &csvutil.MissingColumnsError{})

wouldn't work - it's the same problem as I described in "Before Go1.13"

You probably don't want to use errors.Is with this because errors.Is is intended for equality checks (just like == operator) but with unwrapping logic. errors.Is(err, io.EOF) works just like if err == io.EOF used to work (plus unwrapping logic). If the returned error value is:

err = csvutil.MissingColumnsError{
  Columns: []string{"foo", "bar"}
}

You would not be checking it by comparsion - if err == (csvutil.MissingColumnsError{}) - because it would never equal. Instead you would rather check its type like:

myErr, ok := err.(*csvutil.MissingColumnsError)

Extra examples to show ambiguity: https://play.golang.org/p/Ly8UpBmsiVp

Notice that json pkg also uses ptr receivers (for errors): https://go.googlesource.com/go/+/go1.16.3/src/encoding/json/encode.go#235

In my opinion, everything is like it's supposed to be. There are no benefits to having these types implemented via value receiver. It would only add extra ambiguity.

[mariaefi29]

Dear @jszwec,

Thank you so much for a detailed answer! It gave me a lot of insights to think about and I have spend some time looking at that and doing my "doodling" in playground (https://play.golang.org/p/6k0zh_YbNG3)

What I like about a pointer receiver now that the compiler would restrict returning a value and we would be bound to one particular behaviour. That's great!

However, it is hard to agree with your point against the value receiver. You have pointed out that we would have to check it against a pointer and a value type to be safe. However, it is really not a problem due to docs and API versions. We would know for sure what type to check.

My colleges also gave me an idea that it would be great if a package provided methods to check custom errors explicitly. Instead of doing all the checking himself and worrying about types the user could just use one method. For example: Is MissingColumnsError(err error) bool

[jszwec]

However, it is hard to agree with your point against the value receiver.

Docs are quickly getting out of date and most people don't read them. If you define error as a val receiver, then you are explicitly telling me that both *Foo and Foo implement error. Then the fair question is: since val and ptr receivers both implement it, then why am I returning a value instead of a pointer (or vice versa)? And after someone changes it to pointer (or value), this person just broke people's code who relied on docs, because their error checks don't work any more.

I myself had a situation using certain libraries where sometimes the same error type was being returned as a pointer and sometimes as a value. This was very annoying to handle correctly.

That's why it is always better to allow one way of doing things - in this case it's a ptr receiver and no mistake can be made.

My colleges also gave me an idea that it would be great if a package provided methods to check custom errors explicitly.

I would probably agree before Go1.13 (before errors.As), and/or if the error check was complicated and longer than one line.

Instead of doing all the checking himself and worrying about types the user could just use one method.

We don't have to worry about types if there is only one way your type implements error.

To Summarize:

• I can't change errors to value receivers and/or start returning values instead of ptrs because I will break backward-compatibility
• There is no bug, and error can be checked using std library tools as Go doc recommends
• If I add MissingColumnsError I would probably have to do the same for the rest of the defined errors in this package. I can't justify adding 8 new functions that exchange one "one-liner" for another

  if IsMissingColumnError(err) { }

  vs

  if errors.As(err, new(*csvutil.MissingColumnError)) { }

I don't see any actionable items here. I consider this issue closed.

[mariaefi29]

Thank you so much for an interesting discussion! Great points! I am convinced now that is the better better. :)))