Add go 1.13 errors feature support #32

PeterIvanov · 2021-02-18T08:30:05Z

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 · 2021-02-18T08:37:56Z

Cc @pacejackson

PeterIvanov · 2021-02-18T08:54:05Z

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

isopov

LGTM

type.go

g7r · 2021-02-18T11:58:38Z

error_113.go

+
+// todo add errorx.As()?
+// todo make another 'go error type' for passing into errors.As()?
+// todo when fixed, add tests for wrap/decorate etc.


I don't really see the problem with As. Let's remember the use case of errors.As:

// errors.As is designed to replace the following error drilling pattern: if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 42 { // handle error code 42 }

When you don't need actual error value buried deep in an error chain, errors.Is would suffice.

When you do actually need that, let's remember that all errorx.Error properties are being propagated to topmost error. And that topmost errorx.Error is really what you need because there is no other way to attach additional information to error other than carried by properties.

Therefore, I suggest that errorx.Error shouldn't implement As method.

I suggest you copy this comment to the next PR I sent, and I'll remove this part of change from current PR.

g7r · 2021-02-18T21:20:52Z

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:

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))

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))

error_113.go

PeterIvanov · 2021-02-19T08:15:06Z

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 8000 , 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.

g7r · 2021-02-19T12:01:58Z

type.go

 // The original error will not pass its dynamic properties, and those are accessible only via direct walk over Cause() chain.
 // Without args, leaves the original message intact, so a message may be generated or provided externally.
 // With args, a formatting is performed, and it is therefore expected a format string to be constant.
+// NB: Wrap is NOT the reverse of Error.Unwrap method; name may be changed in future releases to avoid confusion.


Maybe we should refer to errors.Unwrap instead of Error.Unwrap?

g7r · 2021-02-19T12:04:15Z

error_113_test.go

+
+	t.Run("Wrap", func(t *testing.T) {
+		err := testTypeBar1.Wrap(testType.NewWithNoMessage(), "")
+		unwrapped := errors.Unwrap(err)


This is where unrelatedness between Wrap and Unwrap hits particularly hard :(

g7r · 2021-02-19T12:06:23Z

error_113_test.go

+		err := Decorate(testType.NewWithNoMessage(), "")
+		unwrapped := errors.Unwrap(err)
+		require.NotNil(t, unwrapped)
+		require.True(t, IsOfType(unwrapped, testType))


Technically, unwrapped may be the same as err and the assertions will still pass.

g7r · 2021-02-19T12:07:36Z

error_113_test.go

+		err := Decorate(io.EOF, "")
+		unwrapped := errors.Unwrap(err)
+		require.NotNil(t, unwrapped)
+		require.True(t, errors.Is(unwrapped, io.EOF))


This will be true even if errors.Unwrap returned original err.

g7r · 2021-02-19T12:09:55Z

error_113_test.go

+	})
+
+	t.Run("Wrap", func(t *testing.T) {
+		err := testTypeBar1.Wrap(testType.NewWithNoMessage(),"")


Lack of gofmt detect I.

g7r · 2021-02-19T12:11:26Z

error_113_test.go

+	t.Run("DecorateForeign", func(t *testing.T) {
+		err := Decorate(io.EOF,"")
+		require.True(t, errors.Is(err, io.EOF))
+	})


We should add a test that errorx.IsOfType(err, errorType) penetrates fmt.Errorf("blabla: %w", errorType.NewWithNoMessage()).

As of this PR, it doesn't. I intend to add both feature and tests in a separate PR.

PeterIvanov added 5 commits February 17, 2021 16:56

add basic Unwrap impl and tests

e1de739

tests for nested unwrap

60b8dca

tests for errors.Is

9e108ae

tests for errors.As

553e5d7

godoc

fc849e5

PeterIvanov changed the base branch from master to unstable_master February 18, 2021 08:30

PeterIvanov requested review from g7r and isopov February 18, 2021 08:36

PeterIvanov mentioned this pull request Feb 18, 2021

Add helper methods for the "errors" package #28

Closed

PeterIvanov changed the base branch from unstable_master to master February 18, 2021 08:53

isopov approved these changes Feb 18, 2021

View reviewed changes

g7r requested changes Feb 18, 2021

View reviewed changes

PeterIvanov changed the base branch from master to unstable_master February 18, 2021 13:31

remove errors.As-related changes

1a3d895

PeterIvanov requested a review from g7r February 18, 2021 13:39

PeterIvanov mentioned this pull request Feb 18, 2021

Make errors.As work for errorx #33

Closed

g7r reviewed Feb 19, 2021

View reviewed changes

error_113.go Outdated Show resolved Hide resolved

remove build tag from extention methods

e84f1b1

PeterIvanov requested a review from g7r February 19, 2021 11:57

g7r requested changes Feb 19, 2021

View reviewed changes

add tests and godoc

a620e5c

PeterIvanov requested a review from g7r February 23, 2021 08:15

g7r approved these changes Feb 23, 2021

View reviewed changes

PeterIvanov merged commit c749e95 into unstable_master Feb 24, 2021

PeterIvanov deleted the feature/1.13_errors branch February 25, 2021 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add go 1.13 errors feature support #32

Add go 1.13 errors feature support #32

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add go 1.13 errors feature support #32

Add go 1.13 errors feature support #32

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants