golang-v2: error handling

[pelletier]

Following #530, let's start the discussion here about what errors should look like and how they should be implemented. I'll write up my thoughts later today, but feel free to start the discussion if you'd like! @vincentserpoul @moorereason

---

Overall I think the errors should help the user of the library understand what went wrong and give context to fix this. I think this means two things:

1. Errors should be descriptive.
2. When decoding, they should describe where in the document the problem happened.

For the first point, I think encoding/json is a good model. For example:

func (e *UnmarshalTypeError) Error() string {
	if e.Struct != "" || e.Field != "" {
		return "json: cannot unmarshal " + e.Value + " into Go struct field " + e.Struct + "." + e.Field + " of type " + e.Type.String()
	}
	return "json: cannot unmarshal " + e.Value + " into Go value of type " + e.Type.String()
}

d.saveError(fmt.Errorf("json: cannot set embedded pointer to unexported struct: %v", subv.Type().Elem()))

Error messages a clear, and created at the point the error occurred. No need to wrap it. If we can't write an error message that accurately describes the issue without providing the call stack that means there is an issue in the structure of the code.

For the second point, I think it's mostly a matter of making sure all the errors that come from decoding are stored in a toml.decodeError. With this format, the top level function adds the surrounding lines in the file and highlight the bytes that caused the issue.

[vincentserpoul]

I'll also link here uber style guide for errors.
I agree the hack of adding the stack in the error is not good.

Regarding the cost of initialization of errors, have any of you seen an article or an opinion about this?
Is it worth turning these into struct, even if more verbose? Or is it better to simply not declare them as globals?

[pelletier]

(just want to say that it's a good discussion to have, and I'm enjoying it!)

[vincentserpoul]

(just want to say that it's a good discussion to have, and I'm enjoying it!)

I'm enjoying it too, I learnt a lot deep diving in this!

[vincentserpoul]

I think I now see most what supports/motivates your opinions on this and it's going to be easier to go back to the code and straight things up!

To nitpick/discuss further for the sake of it (without trying to change the decision made here):

Panic: I feel it's still fairer for the lib user to not panic and not decide for him what to do if the decode is going hay wire. We can never know in what circumstances this lib is used.

Const typed sentinel errors vs inline errors: it allows to react differently on error types without much downsides (except verbosity). I still feel there's something wrong with go undifferentiated errors, but maybe it's because my eyes can't unsee rust now :P.

Unexported fields: got it, that's a very good point actually.

[pelletier]

Panics: I'm following Effective Go's recommendation:

It's also a way to indicate that something impossible has happened, such as exiting an infinite loop.

This is exactly the set of cases I'm suggesting we use panic for (no more, no less). Sdlib's encoding/json does the same:

https://github.com/golang/go/blob/ce92a2023ccd77ca609126aa8a6e881c9def57f0/src/encoding/json/decode.go#L812

If the user cannot accept the library to panic even if something supposedly impossible happened, they are free to wrap the call with a recover.

Const typed sentinel errors vs inline errors: I'm not saying we should never do that. Just pointing out that they don't bring anything until there is actually a use for matching on specific errors internally. If we end up in such situation, happy to start using sentinel values. In the meantime, I'd rather just fmt.Errorf where the error happen. Makes it easy to read and encourages adding a little bit of info right there (encoding/json: https://github.com/golang/go/blob/ce92a2023ccd77ca609126aa8a6e881c9def57f0/src/encoding/json/decode.go#L728).

[vincentserpoul]

Very productive discussion! all cleared out!