joomcode / errorx

A comprehensive error handling library for Go
MIT License
1.12k stars 29 forks source link

Add go 1.13 errors feature support #32

Closed PeterIvanov closed 3 years ago

PeterIvanov commented 3 years ago

As tests show, errors.Unwrap and errors.Is work exactly as intended and preserve all existing errorx contracts. On the other hand, errors.As is broken for errorx. It is not a regression of this PR, some tests are added for cases that work, issue will be addressed further in future PRs.

PeterIvanov commented 3 years ago

Cc @pacejackson

PeterIvanov commented 3 years ago

Travis tests are not run for some reason. https://travis-ci.com/github/joomcode/errorx/requests

g7r commented 3 years ago

I think we're trying to solve wrong problem here. errorx has its own means for checking error type. It is different from errors.Is and errors.As. The problem is that it is difficult to extract errorx error wrapped by third party type and it is difficult to extract third party type wrapped by errorx error.

What we really want is:

  1. errorx.IsOfType should be working no matter how *errorx.Error was wrapped. We already have tools to check errorx error type. We don't need to invent clever wrappers to use with errors.Is and errors.As. That is:
    err := fmt.Errorf("wrapped: %w", errorx.IllegalState.NewWithNoMessage())
    require.True(t, errorx.IsOfType(err, errorx.IllegalState))
  2. errors.Is and errors.As should be working for third party types after wrapping by errorx. That is:
    err := errorx.Decorate(context.Canceled, "fancy decoration")
    require.True(t, errors.Is(err, context.Canceled))
PeterIvanov commented 3 years ago

Let's dissect those arguments a bit. First, I agree that both of these cases are valid and must be addressed. The second one is covered by this PR already, the first one is not, I would suggest a separate PR for that after we finish with this one. I disagree that there's a conflict of "wrong problem" here. In my opinion, both are valid problems, and if there's a reasonable and sane way to address both, the result would be better than a more timid solution.

At this point, as I see it, we must choose whether to have a solution that solves only the case shown above, or a wider range of scenarios. The former must be supported by the argument as to what harm does those additions bring.

Historically, similar concepts in errorx predate go 1.13 error features, and the design is based on logical types, but the idea behind it is very similar. Now, especially after an errorx release with official 1.13 support is made, it would be reasonable to expect that constructs like errors.Is works natively for errorx types. An absence of such support would mean that any errorx user is forced to use strictly errorx syntax for such checks for no obvious good reason, and that package godoc would have to try and make an argument as to why this reason is, indeed, good. Such argument is yet to be presented.

The second argument here is that optional interface{ Is(error) bool } looks to me like something designed to do precisely what is being done here. errors package is quite shy in explaining the intended and unintended use, but I've failed to find anything there that discourages such behaviour and there are parts that seem to agree with it.

Is unwraps its first argument sequentially looking for an error that matches the second

An error type might provide an Is method so it can be treated as equivalent to an existing error

So, it seems to me that making a check in code like

// err is errorx.IllegalArgument
if errors.Is(err, errorx.TimeoutElapsed.NewWithNoMessage()) {
  // try again later
}

return true on any errorx type is not only a lost opportunity, it is a potential for bugs that are hard to track, as we all know that negative cases are often neglected in tests.

The only argument I can find not to do this is in having several ways of doing the same thing, which may be confusing. Currently, godoc aims to help with this, it can be enhanced further. Possibly this is in argument in favour of deprecation and then removal of some of errorx existing API, but I'm not sure the confusion is big enough to justify the breaking change.