This is a proposal for adding error wrapping to go.

It is a counter-proposal to Go2 error values (the original proposal).

Please share feedback, opinions etc in this issue.

Find the proposal details in https://github.com/JavierZunzunegui/xerrors.

At a high level, compared to the original proposal, this one: - has no requirement on error types (no Unwrap() error or equivalent) - allows for custom error conversion to string in a more powerful manner - has no automatic migration to wrapping form (code is not immediately using wrapping, no %w or equivalent) - transparently ads stack information to wrapped errors - compile-time safe implementation with few gotchas - can compare errors without requiring modification of reflect.DeepEqual

There was a first iteration of the ideas behind this proposal, but that is now to be regarded as obsolete.

Comment From: jba

has no requirement on error types (no Unwrap() error or equivalent)

Unwrap isn't a requirement; it's optional. Using an interface allows us to retrofit existing errors. In your proposal, only the type WrappingError can wrap. How would existing errors like os.PathError support Find and FindTyped?

compile-time safe implementation with few gotchas

We agree that FindTyped is safer than As, but we've already rejected it because it's less general and more clumsy (requires writing the type twice). And now we have a vet check for As.

can compare errors without requiring modification of reflect.DeepEqual

I don't understand this. Errors with stack information will have the same problem as the current proposal. Errors without stack information will not, also the same.

The substantive differences seem to be: - Your New doesn't add stack information. As we've discussed elsewhere, we think the extra information will be worth it. - You've added a Similar function to compare errors while omitting stack traces. We'd prefer if something like that were added to the cmp package, so it could work on errors that were nested inside other values.

Thanks for taking the time to improve and clarify your proposal. Speaking for myself, I don't see anything here that causes me to rethink our current proposal.

Comment From: JavierZunzunegui

Unwrap isn't a requirement; it's optional. Using an interface allows us to retrofit existing errors. In your proposal, only the type WrappingError can wrap. How would existing errors like os.PathError support Find and FindTyped?

In this proposal wrapping is not an interface but a concrete type, WrappingError. Any error (such as os.PathError) can be wrapped, that means creating a WrappingError containing the error (via Wrap or WrapWithOpts). Find and FindTyped are methods that find specific errors in a WrappingError, os.PathError is already supported just like any other errors. See https://github.com/JavierZunzunegui/xerrors/blob/master/find_test.go#L16

Also in the original while Unwrap is not required by the compiler it is required to achieve the desired functionality (As, Is, print with Printer, ...) so I think it is pretty much required. In this all an error has to implement is Error() string.

can compare errors without requiring modification of reflect.DeepEqual

I don't understand this. Errors with stack information will have the same problem as the current proposal. Errors without stack information will not, also the same.

reflect.DeepEqual fails equally in both proposals for the same reason (wrapped stacks). This proposal has Similar which is basically a DeepEqual but ignores the stacks, and requires nothing from the wrapped errors. This can't be done in the original, because the fundamental issue is how to compare errors which are identical in every respect, except that they wrapped different errors, and in that one wrapping errors are interfaces containing a reference to the wrapped error, a Compare(error) bool or similar method would be required but there is no guarantee every error will implement it and do so correctly. In this one only one error has that property (WrappingError), and supports Similar and Compare correctly.

Your New doesn't add stack information. As we've discussed elsewhere, we think the extra information will be worth it.

In this proposal, errors only hold the information of that error. Stack information is an error, namely StackError. The error from New (stringError) has no stack, but it gets the stack once it is wrapped - Wrap(nil, New("whatever")) the resulting WrappingError has a StackError and a stringError. Again it means there is no requirement for any error to interact with the stack or any other feature, it is al abstracted away via the WrappingError produced by Wrap. See https://github.com/JavierZunzunegui/xerrors/blob/master/stack_test.go#L19, shows exactly that, a stringError with a stack.

You've added a Similar function to compare errors while omitting stack traces. We'd prefer if something like that were added to the cmp package, so it could work on errors that were nested inside other values.

Sure. It will be much easier to do here since only WrappingError has nested errors.

Comment From: jba

Any error (such as os.PathError) can be wrapped,

No, I meant how would os.PathError expose the error it currently wraps (in its Err field) to Find et. al.

except that they wrapped different errors,

No: except that they have different stack frames. And it isn't necessary that every error implement something, only that the recursive comparison function ignores frames.

Comment From: JavierZunzunegui

No, I meant how would os.PathError expose the error it currently wraps (in its Err field) to Find et. al.

I see. For reference, PathError is 

type PathError struct {
        Op   string
        Path string
        Err  error
}

PathError is basically doing it's own wrapping and that can't integrate with WrappingError and all the features proposed here, two changes are needed:

Change it's Error method from func (e *PathError) Error() string { return e.Op + " " + e.Path + ": " + e.Err.Error() } to func (e *PathError) Error() string { return e.Op + " " + e.Path }. Then Wrap(err, &PathError{}).Error() {this proposal} == PathError{Err: err}.Error() {current go}.

The other change is is outright removing the Err field, and accessing it via xerrors.Find(err, ...), for things like timeout, etc. In fact os already has IsTimeout, IsPermission, etc.

At that point a PathError (generated via xerrors.Wrap, actually a WrapperError type) supports both it's existing functionality and the new one introduced here.

Of course this is a breaking change, but in the meantime what is broken is not PathError but the new wrapping functionality. This is the same (or similar) in the original proposal, until the error gets Unwrap and FormatError(p Printer) (next error) it doesn't work well under those changes either. The change is larger in this one, true, but that is because in my proposal this pattern (error containing another error) is to be considered an anti-pattern and has to be phased out and that takes a little more work.

Comment From: JavierZunzunegui

except that they wrapped different errors,

No: except that they have different stack frames. And it isn't necessary that every error implement something, only that the recursive comparison function ignores frames.

The point still remains. The or the comparison function ignores the frames is a red flag to me - want a reflect based method comparing two types that distinguishes based on the type of a field? That is 'reflection magic' and imho is a symptom of a flawed design. Is there anywhere else in the standard library where such approach is taken?

Also, you focus on frames but maybe, down the line, some other similar property wants to be added. Then you have to add more reflection magic. In this proposal there is nothing special about StackError, it is simply integrated into the default Formatter and Wrap method but is nothing any user couldn't change if it didn't suit them.

Comment From: ianlancetaylor

Thanks, but we would up going in a different direction. Closing this issue.