Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Go 1.13 error chains #206

Merged
merged 4 commits into from Nov 9, 2019
Merged

Conversation

@jayschwa
Copy link
Contributor

jayschwa commented Jul 3, 2019

Go 1.13 adds support for error chains to the standard libary's errors
package. The new standard library functions require an Unwrap method to
be provided by an error type. This change adds a new Unwrap method
(identical to the existing Cause method) to the unexported error types.

Go 1.13 adds support for error chains to the standard libary's errors
package. The new standard library functions require an Unwrap method to
be provided by an error type. This change adds a new Unwrap method
(identical to the existing Cause method) to the unexported error types.
@knz

This comment has been minimized.

Copy link

knz commented Jul 5, 2019

I was about to open exactly the same PR. Thank you!
We would love this to go through as it will enable https://github.com/cockroachdb/errors to remain backward-compatible with pkg/errors.

@sagikazarmark

This comment has been minimized.

Copy link

sagikazarmark commented Jul 8, 2019

I wonder if it makes sense to add unwrapping to the Cause function as well so that it can unwrap Go 1.13 errors as well. It would automatically make old code using Cause work with Unwrap and give time to transition to Unwrap.

@knz

This comment has been minimized.

Copy link

knz commented Jul 8, 2019

that sounds reasonable but perhaps a different PR?

@sagikazarmark

This comment has been minimized.

Copy link

sagikazarmark commented Jul 8, 2019

Well, the title says Add support for Go 1.13 error chains, so for me it belongs here.

@jayschwa

This comment has been minimized.

Copy link
Contributor Author

jayschwa commented Jul 8, 2019

Unwrapping Go 1.13 error chains can be done with the new functions in the standard library, so I don't see a need to support it here. Perhaps I could have named the PR better, but I'm going to leave it as-is unless I hear differently from Dave.

@sagikazarmark

This comment has been minimized.

Copy link

sagikazarmark commented Jul 8, 2019

Unwrapping Go 1.13 error chains can be done with the new functions in the standard library

Existing code using errors.Cause won't be able to unwrap errors. I think it would be a great forward compatible migration path, if it could.

@natefinch

This comment has been minimized.

Copy link

natefinch commented Sep 3, 2019

IMO this is necessary so that existing codebases can gracefully transition to the stdlib error wrapping without having to go whole-hog changing thousands of lines of code to use UnWrap() rather than Cause. Please take a look, @davecheney

@slomek

This comment has been minimized.

Copy link

slomek commented Sep 21, 2019

What about using Unwrap() instead of Cause() for unwrapping as well? This would add the compatibility the other way round, so if eg. I'm using errors.Cause(...) in my application, but the inner library starts using Go 1.13 errors chain, I can still extract the root error?

@aperezg

This comment has been minimized.

Copy link
Member

aperezg commented Sep 21, 2019

We're forked this library and modify for using internally new package errors on go 1.13, furthermore on if you use a previous version the library was been compiled about xerrors package.

If anyone want use it our version, will can find it here:
https://github.com/friendsofgo/errors

Any feedback is more than welcome!

@knz

This comment has been minimized.

Copy link

knz commented Sep 22, 2019

We've made a more extensive errors library that's both compatible with xerrors and pkg/errors and adds a lot of bonus features, see https://github.com/cockroachdb/errors

@sagikazarmark

This comment has been minimized.

Copy link

sagikazarmark commented Sep 22, 2019

I created a drop-in replacement for both the stdlib errors package and pkg/errors: https://github.com/emperror/errors

@chuckha

This comment has been minimized.

Copy link

chuckha commented Oct 17, 2019

I believe this PR is very much needed as the Frame capture did not make it through from xerrors into go1.13. github.com/pkg/errors still has a lot of utility and given the choice between using only one error package, the standard library's error package is less useful than this one.

@slomek

This comment has been minimized.

Copy link

slomek commented Oct 19, 2019

@chuckha You don't really need the change here to start using Go 1.13 errors, I've describe a workaround on my blog: https://mycodesmells.com/post/migrating-pkg-errors-to-go-113-errors 😉

@Songmu

This comment has been minimized.

Copy link

Songmu commented Oct 20, 2019

Go 1.13 has been released and Is and As func is introduced in standard error package.

Once this pull request to be merged, we can use errors.As and errors.Is for withMessage and withStack. This is very useful for standard error package and pkg/errors interoperability.

I hope this pull request will be merged soon.

@davecheney

This comment has been minimized.

Copy link
Member

davecheney commented Oct 22, 2019

Thank you for this PR. I'm sorry I have had no time to work on this package since I called for the 1.0 release earlier in the year -- my day job is very demanding. I wasn't expecting that Go 1.13 would ship without stack trace support in its errors. I hope to have time to review this properly in November.

@davecheney davecheney self-requested a review Oct 22, 2019
@Sherlock-Holo

This comment has been minimized.

Copy link
Contributor

Sherlock-Holo commented Oct 31, 2019

I think if you add Is As Unwrap functions, we can keep using pkg/errors with the new functions.
For now, with this PR, we still need to

import (
	stderrors "errors"

	"github.com/pkg/errors"
)

to use Is As Unwrap functions

@Sherlock-Holo

This comment has been minimized.

Copy link
Contributor

Sherlock-Holo commented Oct 31, 2019

some error variable in std library (example), after go1.13, can not use == to check simply, need to use errors.Is so add Is As Unwrap function into pkg/errors too is a good idea

@Songmu

This comment has been minimized.

Copy link

Songmu commented Oct 31, 2019

I think we only need pkg/errors.Unwrap, and Is and As aren't needed. We can use standard errors.Is/As.

@Songmu

This comment has been minimized.

Copy link

Songmu commented Oct 31, 2019

The pkg/errors will no longer be needed if the standard package will provide a stack feature. I think that is the desired future.

I think support only Unwrap is necessary for the interoperability and ease of migration between pkg/errors and standard errors, but I don't think other features is necessary.

@Sherlock-Holo

This comment has been minimized.

Copy link
Contributor

Sherlock-Holo commented Oct 31, 2019

@Songmu if pkg/errors don't support Is As Unwrap, we need to import stderrors "errors" and it doesn't look good: why I used an error package but still need another one to compare error

@aperezg

This comment has been minimized.

Copy link
Member

aperezg commented Oct 31, 2019

@Sherlock-Holo I'm agree with you that if the package errors had implemented the functions Is and As would be better for don't import two packages at the same time. But if we adding this functions to this package we have three options, one is adding with xerrors package for compatibility with previous version of v1.13, other is using the building tags, and use xerrors and standard and the last is implemented by the pkg/errors.

But in this case I think that this PRs is only for add the compatibility with the standard library, and adding only the Unwrap was enough. If you want, you can add a new issue related with this PRs for discuss if util for this package implement the functions Is and As

@aperezg aperezg self-requested a review Oct 31, 2019
@Sherlock-Holo

This comment has been minimized.

Copy link
Contributor

Sherlock-Holo commented Oct 31, 2019

@aperezg good idea, I will create a PR to add Is As Unwrap support. Actually I forked this repo and added these functions last night. Let me make it good enough to create a PR

@johan-lejdung

This comment has been minimized.

Copy link

johan-lejdung commented Nov 1, 2019

I'm really looking forward to this PR getting merged 🙏Nice work everyone!

Copy link
Member

aperezg left a comment

could you change the name of file from go1.13_compat_test.go to go113_test.go

go1.13_compat_test.go Outdated Show resolved Hide resolved
go1.13_compat_test.go Outdated Show resolved Hide resolved
go1.13_compat_test.go Outdated Show resolved Hide resolved
go1.13_compat_test.go Outdated Show resolved Hide resolved
go1.13_compat_test.go Outdated Show resolved Hide resolved
func TestErrorChainCompat(t *testing.T) {
err := stdlib_errors.New("error that gets wrapped")
wrapped := errors.Wrap(err, "wrapped up")
if !stdlib_errors.Is(wrapped, err) {

This comment has been minimized.

Copy link
@aperezg

aperezg Nov 1, 2019

Member
Suggested change
if !stdlib_errors.Is(wrapped, err) {
if !errors.Is(wrapped, err) {

This comment has been minimized.

Copy link
@aperezg

aperezg Nov 1, 2019

Member

I think that is not necessary use the package _test in this case because you can use directly the std library and check against the package itself

This comment has been minimized.

Copy link
@jayschwa

jayschwa Nov 1, 2019

Author Contributor

I wanted it to be a black box test, and that is why I put it in a separate package.

This comment has been minimized.

Copy link
@aperezg

aperezg Nov 6, 2019

Member

Ok so is a good argument to me, we need to merge this #212 before that we can merge your PR, because in otherwise the travis ci will never pass. But it's look great, and I would like to merge this ASAP :)

This comment has been minimized.

Copy link
@jayschwa

jayschwa Nov 6, 2019

Author Contributor

Once #212 is merged, I will rebase and squash this branch.

This comment has been minimized.

Copy link
@puellanivis

puellanivis Nov 7, 2019

I do not see the utility here in actually pushing it into a separate package. You can still do blackbox testing even if technically you could look inside the box.

The design is in the design, not in the arbitrarily forcing a semantic. Consider as well all the people forking the project, and now that test is going to pull this package for their tests, and not their own forked repo.

This comment has been minimized.

Copy link
@aperezg

aperezg Nov 8, 2019

Member

I see your point and is very reasonable, but the package itself can't be tested normally from a fork, because have the repository path hardcode in some tests, for example:

https://github.com/pkg/errors/blob/master/format_test.go#L29

So if anyone fork this repository, he/she must be moved under $GOPATH/src/github.com/pkg/errors or they also could use docker instead. Otherwise the test will never pass on local.

Furthermore this can also be resolved using go modules, but yes for now the package haven't this functionality.

But, I agree that in this case one way on another to do this black box test is similar to the other. So maybe would be ok, if we try to avoid more accoplated code with the package itself.

What do you think @puellanivis?

This comment has been minimized.

Copy link
@puellanivis

puellanivis Nov 8, 2019

Hm… fun test. But a critical difference is that the format_test would break loudly, while this would not even break, because a forking author may not even realize that this import is here, and thus get a false sense of security that their tests are passing, even though they are not even running against their own code.

This comment has been minimized.

Copy link
@aperezg

aperezg Nov 9, 2019

Member

Yes, I totally agree that are two different case, this case could produce as you said false sense of security. My point was that with the current state of the package tests, the package couldn't be tested from another directory. But, I see your point.

@jayschwa could you change the test and use the package errors instead the package errors_test? Is truth that Go provide with the package _test to realize the black box test on secure way, but we can mantain this test as black box even without that.

Thanks 😁

PS. the PR #212 is already merged, so your pipeline should be pass this time

This comment has been minimized.

Copy link
@jayschwa

jayschwa Nov 9, 2019

Author Contributor

I moved the test back into the errors package and CI is green.

@aperezg
aperezg approved these changes Nov 6, 2019
@aperezg aperezg added this to the 0.9 milestone Nov 6, 2019
Sherlock-Holo added a commit to Sherlock-Holo/errors that referenced this pull request Nov 7, 2019
some change are doing by PR pkg#206 and pkg#212 , so I don't need to do it

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
jayschwa added 2 commits Nov 9, 2019
@aperezg aperezg merged commit 7f95ac1 into pkg:master Nov 9, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@johan-lejdung

This comment has been minimized.

Copy link

johan-lejdung commented Nov 9, 2019

Consider making a release with these changes, it doesn't seem like there has been a release since 5th of January

@aperezg

This comment has been minimized.

Copy link
Member

aperezg commented Nov 9, 2019

@johan-lejdung Yes, the idea is finish to check #213 (for conclude the support to the 1.13 errors) and see if another pending PR could be approved, and check the Milestone to v0.9.0 and then publish, we try this ASAP on this month, it is possible

@johan-lejdung

This comment has been minimized.

Copy link

johan-lejdung commented Nov 10, 2019

Would it make sense to also extend the Is and As methods in this package?

The only reason I ask is because I've been able to include this package instead of the "errors" package in the past. But as of this change I have to include both packages in order to make use of the Is and As methods. Because the name of the packages are the same it because slightly more cumbersome.

It's not the end of the world, but if this is thought of as a drop-and-replace package I think it would make sense.

@aperezg

This comment has been minimized.

Copy link
Member

aperezg commented Nov 10, 2019

@johan-lejdung there a PR with the method Is and As you can follow his process here: #213.

aperezg added a commit that referenced this pull request Jan 3, 2020
* feat: support std errors functions

add function `Is`, `As` and `Unwrap`, like std errors, so that we can
continue to use pkg/errors with go1.13 compatibility

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

* style: delete useless comments

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

* build: update makefile

update makefile to download dependencies before test anything

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

* build: fix makefile

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

* chore: delete useless comments

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

* Restore Makefile

* revert: revert some change

some change are doing by PR #206 and #212 , so I don't need to do it

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

* test: add more check for As unit test

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

* revert: only support Is As Unwrap for >=go1.13

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

* feat(Unwrap): allow <go1.13 can use Unwrap

`Unwrap` just use type assert, it doesn't need go1.13 actually

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

* test: add go1.13 errors compatibility check

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>

* refactor(Unwrap): don't allow <go1.13 use Unwrap

If we implement Unwrap ourselves, may create a risk of incompatibility
if Go 1.14 subtly changes its `Unwrap` implementation.
<go1.13 users doesn't have `Is` or `As`, if they want, they will use
xerrors and it also provides `Unwrap`

Signed-off-by: Sherlock Holo <sherlockya@gmail.com>
@jasonkeene

This comment has been minimized.

Copy link

jasonkeene commented Jan 6, 2020

@aperezg Any chance we could get a tagged release for this? 🙏 Would be nice to no longer use a fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.