Make errors.As work for errorx

[PeterIvanov]

This PR at this stage is more a design review than a code review. I did not do any code polishing until we agree on whether or not this idea is OK and what to change about it. A concept is simple: if an opinionated errors.As implementation prioritises assignability, let's remove it. A special wrapper is introduced in order to beat this, and *Error::Is() can then do its magic. If a wrapper is then used directly, it works as an error implementation, too. If we go this way, I would also suggest to panic if errorx error is passed as target directly. Such cases are always intentional and by errorx user, so it seems like a good idea not to allow a broken usage at all.

[PeterIvanov]

@g7r that's an interesting view you shared in https://github.com/joomcode/errorx/pull/32#discussion_r578359425 I'll try and make an opposing argument.

If we consider a proper pattern to be

        var target *errorx.Error
        if errors.As(err, &target){
            // smth
        }

then, indeed, there's nothing wrong with it. However, if a user of errorx library would write this in a manner consistent with current library usage, it would look this way:

        target := myErrorType.NewWithNoMessage()
        if errors.As(err, &target){
            // smth
        }

and the result would be quite unexpected. It would be reasonable to assume that target is still of myErrorType type when you enter the inner block, but it isn't.

I agree with the surmise that there's actually no need to use this approach with errorx, as errors.Is plus error properties will suffice. But the goals of this proposal are:

1. Minimise the number of cases where errorx + standard error-relates flow would give an unexpected result
2. If possible and reasonable, make behaviour of errors.As on errorx errors consistent with the rest of errorx package.

The second point is highly debatable and I'm ready to abandon it if there's a good argument as to which harm it brings. The first point I find rather important.

[g7r]

@PeterIvanov

One thing to note here is that standard error types are also prone to the same problem:

        target := errors.New("foo")
        if errors.As(err, &target){
            // smth
        }

Is it expected that errors.As should return an error with "foo" message? I think no.

I also should point that implementation of errors.As implies matching by assignability: https://github.com/golang/go/blob/go1.16/src/errors/wrap.go#L91-L94. You may, for example, pass pointer to interface{ IsTemporary() bool } and retrieve first error in error chain that could be assigned to that. This is another evidence that errors.As is designed to be purely type-based.

I agree that concept of errors.As looks alien to errorx design. But implementing it in errorx-way would break expected errors.As contracts.

errors.Is is an entirely different beast from errors.As. errors.As shouldn't be implemented in terms of errors.Is. The main difference is that errors.Is takes error value into account while errors.As only uses type.

Here's what Is documentation says:

An error is considered to match a target if it is equal to that target or if it implements a method Is(error) bool such that Is(target) returns true.

Here's what As documentation says:

An error matches target if the error's concrete value is assignable to the value pointed to by target, or if the error has a method As(interface{}) bool such that As(target) returns true. In the latter case, the As method is responsible for setting target.

Implementing As in terms of Is is a violation of the difference between errors.As and errors.Is. Link to design: https://go.googlesource.com/proposal/+/master/design/29934-error-values.md. Several citations again:

The Is function follows the chain of errors by calling Unwrap, searching for one that matches a target. It is intended to be used instead of equality for matching sentinel errors (unique error values). The As function searches the wrapping chain for an error whose type matches that of a target. An error type can implement As to override the default behavior.

We could interpret that mention of "type" above as error type in terms of errorx. But as we already learned, default errors.As implementation resists such interpretation.

[PeterIvanov]

We could interpret that mention of "type" above as error type in terms of errorx. But as we already learned, default errors.As implementation resists such interpretation.

Yes, this is the very foundation for this PR. errorx predates these additions to errors library. It is opinionated in a way that adds logical types over the same go types, and current errors.As is opinionated in a way that implies that different logical types are different go types. And yes, naive go errors are also broken in this way.

We will have to choose which way to err here. In favor of this current proposal I can say that this feature is completely optional: if you don't use it, you are not affected by this change. If we don't add panic on passing errorx error as target, that is.

[PeterIvanov]

@g7r I would suggest to rework this PR as follows:

• errors.As() works for *errorx.Error, not an errorx logical type
• use of errors.As() with initialised (that is, created from errorx logical type) error as target is forbidden (that is, such call panics)

[g7r]

@PeterIvanov, OK!

[PeterIvanov]

It won't work, not reliably, at least: if errors are compatible by go types, errors.As won't even call errorx.Error::As(). So I'm dropping this PR entirely.